[syzbot] [bpf?] [net?] BUG: unable to handle kernel NULL pointer dereference in __build_skb_around

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 3c2611bac08a selftests/bpf: Fix trace_virtqueue_add_sgs te..
git tree: bpf-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=1026d472c80000
kernel config: https://syzkaller.appspot.com/x/.config?x=cab35c936731a347
dashboard link: https://syzkaller.appspot.com/bug?extid=e1d1b65f7c32f2a86a9f
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15826bc6c80000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=15cd12e2c80000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/36a32f4d222a/disk-3c2611ba.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/f5c0da04f143/vmlinux-3c2611ba.xz
kernel image: https://storage.googleapis.com/syzbot-assets/ae2ca9bce51a/bzImage-3c2611ba.xz

The issue was bisected to:

commit 9c94bbf9a87b264294f42e6cc0f76d87854733ec
Author: Alexander Lobakin <aleksande...@intel.com>
Date: Mon Mar 13 21:55:52 2023 +0000

xdp: recycle Page Pool backed skbs built from XDP frames

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=11deec2ac80000
final oops: https://syzkaller.appspot.com/x/report.txt?x=13deec2ac80000
console output: https://syzkaller.appspot.com/x/log.txt?x=15deec2ac80000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+e1d1b6...@syzkaller.appspotmail.com
Fixes: 9c94bbf9a87b ("xdp: recycle Page Pool backed skbs built from XDP frames")

BUG: kernel NULL pointer dereference, address: 0000000000000d28
#PF: supervisor write access in kernel mode
#PF: error_code(0x0002) - not-present page
PGD 7b741067 P4D 7b741067 PUD 7c1ca067 PMD 0
Oops: 0002 [#1] PREEMPT SMP KASAN
CPU: 1 PID: 5080 Comm: syz-executor371 Not tainted 6.2.0-syzkaller-13030-g3c2611bac08a #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 03/02/2023
RIP: 0010:memset_erms+0xd/0x20 arch/x86/lib/memset_64.S:66
Code: 01 48 0f af c6 f3 48 ab 89 d1 f3 aa 4c 89 c8 c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 66 0f 1f 00 49 89 f9 40 88 f0 48 89 d1 <f3> aa 4c 89 c8 c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 66 0f 1f
RSP: 0018:ffffc90003baf730 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888028b94000 RCX: 0000000000000020
RDX: 0000000000000020 RSI: 0000000000000000 RDI: 0000000000000d28
RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000d28
R10: ffffed100517281c R11: 0000000000094001 R12: 0000000000000d48
R13: 0000000000000d28 R14: 0000000000000f68 R15: 0000000000000100
FS: 0000555555979300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000d28 CR3: 0000000028e2d000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
__finalize_skb_around net/core/skbuff.c:321 [inline]
__build_skb_around+0x232/0x3a0 net/core/skbuff.c:379
build_skb_around+0x32/0x290 net/core/skbuff.c:444
__xdp_build_skb_from_frame+0x121/0x760 net/core/xdp.c:622
xdp_recv_frames net/bpf/test_run.c:248 [inline]
xdp_test_run_batch net/bpf/test_run.c:334 [inline]
bpf_test_run_xdp_live+0x1289/0x1930 net/bpf/test_run.c:362
bpf_prog_test_run_xdp+0xa05/0x14e0 net/bpf/test_run.c:1418
bpf_prog_test_run kernel/bpf/syscall.c:3675 [inline]
__sys_bpf+0x1598/0x5100 kernel/bpf/syscall.c:5028
__do_sys_bpf kernel/bpf/syscall.c:5114 [inline]
__se_sys_bpf kernel/bpf/syscall.c:5112 [inline]
__x64_sys_bpf+0x79/0xc0 kernel/bpf/syscall.c:5112
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x39/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f320b4efca9
Code: 28 c3 e8 2a 14 00 00 66 2e 0f 1f 84 00 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 c7 c1 c0 ff ff ff f7 d8 64 89 01 48
RSP: 002b:00007ffd2c9924d8 EFLAGS: 00000246 ORIG_RAX: 0000000000000141
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f320b4efca9
RDX: 0000000000000048 RSI: 0000000020000080 RDI: 000000000000000a
RBP: 00007f320b4b3e50 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f320b4b3ee0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>
Modules linked in:
CR2: 0000000000000d28
---[ end trace 0000000000000000 ]---
RIP: 0010:memset_erms+0xd/0x20 arch/x86/lib/memset_64.S:66
Code: 01 48 0f af c6 f3 48 ab 89 d1 f3 aa 4c 89 c8 c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 66 0f 1f 00 49 89 f9 40 88 f0 48 89 d1 <f3> aa 4c 89 c8 c3 66 66 2e 0f 1f 84 00 00 00 00 00 66 90 66 0f 1f
RSP: 0018:ffffc90003baf730 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff888028b94000 RCX: 0000000000000020
RDX: 0000000000000020 RSI: 0000000000000000 RDI: 0000000000000d28
RBP: 0000000000000001 R08: 0000000000000001 R09: 0000000000000d28
R10: ffffed100517281c R11: 0000000000094001 R12: 0000000000000d48
R13: 0000000000000d28 R14: 0000000000000f68 R15: 0000000000000100
FS: 0000555555979300(0000) GS:ffff8880b9900000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000d28 CR3: 0000000028e2d000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
----------------
Code disassembly (best guess), 1 bytes skipped:
0: 48 0f af c6 imul %rsi,%rax
4: f3 48 ab rep stos %rax,%es:(%rdi)
7: 89 d1 mov %edx,%ecx
9: f3 aa rep stos %al,%es:(%rdi)
b: 4c 89 c8 mov %r9,%rax
e: c3 retq
f: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
16: 00 00 00 00
1a: 66 90 xchg %ax,%ax
1c: 66 0f 1f 00 nopw (%rax)
20: 49 89 f9 mov %rdi,%r9
23: 40 88 f0 mov %sil,%al
26: 48 89 d1 mov %rdx,%rcx
* 29: f3 aa rep stos %al,%es:(%rdi) <-- trapping instruction
2b: 4c 89 c8 mov %r9,%rax
2e: c3 retq
2f: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
36: 00 00 00 00
3a: 66 90 xchg %ax,%ax
3c: 66 data16
3d: 0f .byte 0xf
3e: 1f (bad)


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Alexander Lobakin]

From: Syzbot <syzbot+e1d1b6...@syzkaller.appspotmail.com>
Date: Wed, 15 Mar 2023 05:03:47 -0700

Looks like skb_shinfo() returns %NULL inside __finalize_skb_around(). My
code didn't touch this at all, but I'm digging this already anyway :s

+ Toke, test_run author :p

> 2b: 4c 89 c8 mov %r9,%rax
> 2e: c3 retq
> 2f: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
> 36: 00 00 00 00
> 3a: 66 90 xchg %ax,%ax
> 3c: 66 data16
> 3d: 0f .byte 0xf
> 3e: 1f (bad)
>
>
> ---
> This report is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzk...@googlegroups.com.
>
> syzbot will keep track of this issue. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
> syzbot can test patches for this issue, for details see:
> https://goo.gl/tpsmEJ#testing-patches

Thanks,
Olek

[Alexander Lobakin]

From: Alexander Lobakin <aleksande...@intel.com>
Date: Wed, 15 Mar 2023 13:10:44 +0100

Ok got it.
So previously, on %XDP_PASS a page was released from the Pool and then a
new one allocated and its context filled. Now, the page gets recycled,
and when it gets recycled, PP doesn't reinit its context. But
xdp_scrub_frame() sets xdpf->data to %NULL, which means the ctx needs to
be reinitialized. Plus the &xdp_frame itself is located in the place
which might easily get overwritten when skb is wandering around the stack.
I'm curious then how it's been working on my side for several weeks
already and I've been using XDP trafgen extensively :D
Thus, I'd change it like that (see below). It's in general unsafe to
assume &xdp_frame residing at the beginning of data_hard_start is still
valid after the frame was cruising around. But let's wait for Toke's
comment.

Also, please merge bpf into bpf-next. I see it doesn't contain my fix
for ctx->frame. This will add merge conflicts after this one is fixed
and also can provoke additional bugs.

>
> + Toke, test_run author :p
>
>> 2b: 4c 89 c8 mov %r9,%rax
>> 2e: c3 retq
>> 2f: 66 66 2e 0f 1f 84 00 data16 nopw %cs:0x0(%rax,%rax,1)
>> 36: 00 00 00 00
>> 3a: 66 90 xchg %ax,%ax
>> 3c: 66 data16
>> 3d: 0f .byte 0xf
>> 3e: 1f (bad)
>>
>>
>> ---
>> This report is generated by a bot. It may contain errors.
>> See https://goo.gl/tpsmEJ for more information about syzbot.
>> syzbot engineers can be reached at syzk...@googlegroups.com.
>>
>> syzbot will keep track of this issue. See:
>> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
>> For information about bisection process see: https://goo.gl/tpsmEJ#bisection
>> syzbot can test patches for this issue, for details see:
>> https://goo.gl/tpsmEJ#testing-patches
>
> Thanks,
> Olek

Thanks,
Olek
---
diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 6a8b33a103a4..5b9ca36ff21d 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -217,12 +217,12 @@ static bool ctx_was_changed(struct xdp_page_head *head)

static void reset_ctx(struct xdp_page_head *head)
{
- if (likely(!ctx_was_changed(head)))
- return;
+ if (unlikely(!ctx_was_changed(head))) {
+ head->ctx.data = head->orig_ctx.data;
+ head->ctx.data_meta = head->orig_ctx.data_meta;
+ head->ctx.data_end = head->orig_ctx.data_end;
+ }

- head->ctx.data = head->orig_ctx.data;
- head->ctx.data_meta = head->orig_ctx.data_meta;
- head->ctx.data_end = head->orig_ctx.data_end;
xdp_update_frame_from_buff(&head->ctx, &head->frm);
}

---
Alternative version, which fixes only this particular problem, but is
less safe as still assumes only xdpf->data could be nulled-out. It can
save a bunch o'cycles on hotpath tho, thus attaching it as well:

diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
index 6a8b33a103a4..55789772f039 100644
--- a/net/bpf/test_run.c
+++ b/net/bpf/test_run.c
@@ -217,13 +217,15 @@ static bool ctx_was_changed(struct xdp_page_head *head)

static void reset_ctx(struct xdp_page_head *head)
{
- if (likely(!ctx_was_changed(head)))
- return;
+ if (unlikely(ctx_was_changed(head))) {
+ head->ctx.data = head->orig_ctx.data;
+ head->ctx.data_meta = head->orig_ctx.data_meta;
+ head->ctx.data_end = head->orig_ctx.data_end;
+ head->frm.data = NULL;
+ }

- head->ctx.data = head->orig_ctx.data;
- head->ctx.data_meta = head->orig_ctx.data_meta;
- head->ctx.data_end = head->orig_ctx.data_end;
- xdp_update_frame_from_buff(&head->ctx, &head->frm);
+ if (head->frm.data != head->ctx.data)
+ xdp_update_frame_from_buff(&head->ctx, &head->frm);
}

static int xdp_recv_frames(struct xdp_frame **frames, int nframes,

[Alexander Lobakin]

From: Alexander Lobakin <aleksande...@intel.com>
Date: Wed, 15 Mar 2023 13:51:23 +0100

> From: Alexander Lobakin <aleksande...@intel.com>
> Date: Wed, 15 Mar 2023 13:10:44 +0100
>
>> From: Syzbot <syzbot+e1d1b6...@syzkaller.appspotmail.com>
>> Date: Wed, 15 Mar 2023 05:03:47 -0700
>>
>>> Hello,

[...]

> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 6a8b33a103a4..5b9ca36ff21d 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -217,12 +217,12 @@ static bool ctx_was_changed(struct xdp_page_head *head)
>
> static void reset_ctx(struct xdp_page_head *head)
> {
> - if (likely(!ctx_was_changed(head)))
> - return;
> + if (unlikely(!ctx_was_changed(head))) {

Must be `unlikely(ctx_was_changed(head))` obviously, I've got attention
deficit or what :D

> + head->ctx.data = head->orig_ctx.data;
> + head->ctx.data_meta = head->orig_ctx.data_meta;
> + head->ctx.data_end = head->orig_ctx.data_end;
> + }
>
> - head->ctx.data = head->orig_ctx.data;
> - head->ctx.data_meta = head->orig_ctx.data_meta;
> - head->ctx.data_end = head->orig_ctx.data_end;
> xdp_update_frame_from_buff(&head->ctx, &head->frm);
> }
>
> ---
> Alternative version, which fixes only this particular problem, but is
> less safe as still assumes only xdpf->data could be nulled-out. It can
> save a bunch o'cycles on hotpath tho, thus attaching it as well:
>
> diff --git a/net/bpf/test_run.c b/net/bpf/test_run.c
> index 6a8b33a103a4..55789772f039 100644
> --- a/net/bpf/test_run.c
> +++ b/net/bpf/test_run.c
> @@ -217,13 +217,15 @@ static bool ctx_was_changed(struct xdp_page_head *head)
>
> static void reset_ctx(struct xdp_page_head *head)
> {
> - if (likely(!ctx_was_changed(head)))
> - return;
> + if (unlikely(ctx_was_changed(head))) {

(here it's correct)

> + head->ctx.data = head->orig_ctx.data;
> + head->ctx.data_meta = head->orig_ctx.data_meta;
> + head->ctx.data_end = head->orig_ctx.data_end;
> + head->frm.data = NULL;
> + }
>
> - head->ctx.data = head->orig_ctx.data;
> - head->ctx.data_meta = head->orig_ctx.data_meta;
> - head->ctx.data_end = head->orig_ctx.data_end;
> - xdp_update_frame_from_buff(&head->ctx, &head->frm);
> + if (head->frm.data != head->ctx.data)
> + xdp_update_frame_from_buff(&head->ctx, &head->frm);
> }
>
> static int xdp_recv_frames(struct xdp_frame **frames, int nframes,

Olek