[syzbot] WARNING in u32_change

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit: 483fed3b5dc8 Add linux-next specific files for 20220921
git tree: linux-next
console+strace: https://syzkaller.appspot.com/x/log.txt?x=16becbd5080000
kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
dashboard link: https://syzkaller.appspot.com/bug?extid=a2c4601efc75848ba321
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=13bc196f080000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=152b15f8880000

Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/1cb3f4618323/disk-483fed3b.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/cc02cb30b495/vmlinux-483fed3b.xz

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+a2c460...@syzkaller.appspotmail.com

------------[ cut here ]------------
memcpy: detected field-spanning write (size 80) of single field "&n->sel" at net/sched/cls_u32.c:1043 (size 16)
WARNING: CPU: 0 PID: 3608 at net/sched/cls_u32.c:1043 u32_change+0x2962/0x3250 net/sched/cls_u32.c:1043
Modules linked in:
CPU: 0 PID: 3608 Comm: syz-executor971 Not tainted 6.0.0-rc6-next-20220921-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
RIP: 0010:u32_change+0x2962/0x3250 net/sched/cls_u32.c:1043
Code: f4 df 14 fa 48 8b b5 78 fe ff ff b9 10 00 00 00 48 c7 c2 20 f7 f5 8a 48 c7 c7 a0 f6 f5 8a c6 05 55 b3 63 06 01 e8 db d6 df 01 <0f> 0b e9 73 f3 ff ff e8 c2 df 14 fa 48 c7 c7 00 fc f5 8a e8 66 ed
RSP: 0018:ffffc90003d7f300 EFLAGS: 00010282
RAX: 0000000000000000 RBX: ffffc90003d7f618 RCX: 0000000000000000
RDX: ffff8880235f1d40 RSI: ffffffff81620348 RDI: fffff520007afe52
RBP: ffffc90003d7f4a0 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 203a7970636d656d R12: ffff888021d420e0
R13: ffffc90003d7f5b8 R14: ffff888021d43c30 R15: ffff888021d42000
FS: 0000555555f71300(0000) GS:ffff8880b9a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000000064a110 CR3: 000000002824c000 CR4: 00000000003506f0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Call Trace:
<TASK>
tc_new_tfilter+0x938/0x2190 net/sched/cls_api.c:2146
rtnetlink_rcv_msg+0x955/0xca0 net/core/rtnetlink.c:6082
netlink_rcv_skb+0x153/0x420 net/netlink/af_netlink.c:2540
netlink_unicast_kernel net/netlink/af_netlink.c:1319 [inline]
netlink_unicast+0x543/0x7f0 net/netlink/af_netlink.c:1345
netlink_sendmsg+0x917/0xe10 net/netlink/af_netlink.c:1921
sock_sendmsg_nosec net/socket.c:714 [inline]
sock_sendmsg+0xcf/0x120 net/socket.c:734
____sys_sendmsg+0x712/0x8c0 net/socket.c:2482
___sys_sendmsg+0x110/0x1b0 net/socket.c:2536
__sys_sendmsg+0xf3/0x1c0 net/socket.c:2565
do_syscall_x64 arch/x86/entry/common.c:50 [inline]
do_syscall_64+0x35/0xb0 arch/x86/entry/common.c:80
entry_SYSCALL_64_after_hwframe+0x63/0xcd
RIP: 0033:0x7f97a4bf4e69
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:00007ffdcaf10028 EFLAGS: 00000246 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007f97a4bf4e69
RDX: 0000000000000000 RSI: 0000000020000340 RDI: 0000000000000006
RBP: 00007f97a4bb9010 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007f97a4bb90a0
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
</TASK>


---
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.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Jamal Hadi Salim]

Is there a way to tell the boat "looking into it?"

cheers,
jamal

[Jamal Hadi Salim]

On Sun, Sep 25, 2022 at 11:38 AM Jamal Hadi Salim <j...@mojatatu.com> wrote:
>
> Is there a way to tell the boat "looking into it?"

I guess I have to swim across to it to get the message;->

I couldnt see the warning message but it is obvious by inspection that
the memcpy is broken. We should add more test coverage.
This should fix it. Will send a formal patch later:

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4d27300c2..591cbbf27 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -1019,7 +1019,7 @@ static int u32_change(struct net *net, struct
sk_buff *in_skb,
}

s = nla_data(tb[TCA_U32_SEL]);
- sel_size = struct_size(s, keys, s->nkeys);
+ sel_size = struct_size(s, keys, s->nkeys) + sizeof(n->sel);
if (nla_len(tb[TCA_U32_SEL]) < sel_size) {
err = -EINVAL;
goto erridr;

[Eric Dumazet]

This patch is not needed, please look at struct_size() definition.

Here, we might switch to unsafe_memcpy() instead of memcpy()

[Jamal Hadi Salim]

Yes, after testing i realize there is nothing wrong here.
What warning was i supposed to see from running the reproducer?

We will still add the test will multiple keys later

cheers,
jamal

[Jamal Hadi Salim]

To be clear, that splat didnt happen for me.
Is there something else syzbot does to activate it?

cheers,
jamal

[Eric Dumazet]

On Sun, Sep 25, 2022 at 10:13 AM Jamal Hadi Salim <j...@mojatatu.com> wrote:
>
> To be clear, that splat didnt happen for me.
> Is there something else syzbot does to activate it?

Sure, please look at:

commit 54d9469bc515dc5fcbc20eecbe19cea868b70d68
Author: Kees Cook <kees...@chromium.org>
Date: Thu Jun 24 15:39:26 2021 -0700

fortify: Add run-time WARN for cross-field memcpy()

[Kees Cook]

On Sun, Sep 25, 2022 at 10:34:37AM -0700, Eric Dumazet wrote:
> Sure, please look at:
>
> commit 54d9469bc515dc5fcbc20eecbe19cea868b70d68
> Author: Kees Cook <kees...@chromium.org>
> Date: Thu Jun 24 15:39:26 2021 -0700
>
> fortify: Add run-time WARN for cross-field memcpy()

> [...]

> Here, we might switch to unsafe_memcpy() instead of memcpy()

I would tend to agree. Something like:

diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
index 4d27300c287c..21e0e6206ecc 100644
--- a/net/sched/cls_u32.c
+++ b/net/sched/cls_u32.c
@@ -1040,7 +1040,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
}
#endif

- memcpy(&n->sel, s, sel_size);
+ unsafe_memcpy(&n->sel, s, sel_size,
+ /* A composite flex-array structure destination,
+ * which was correctly sized and allocated above. */);
RCU_INIT_POINTER(n->ht_up, ht);
n->handle = handle;
n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;

This alloc/partial-copy pattern is relatively common in the kernel, so
I've been considering adding a helper for it. It'd be like kmemdup(),
but more like kmemdup_offset(), which only the object from a certainly
point is copied.

--
Kees Cook

[Kees Cook]

On Sun, Sep 25, 2022 at 07:39:23PM -0700, Kees Cook wrote:
> On Sun, Sep 25, 2022 at 10:34:37AM -0700, Eric Dumazet wrote:
> > Sure, please look at:
> >
> > commit 54d9469bc515dc5fcbc20eecbe19cea868b70d68
> > Author: Kees Cook <kees...@chromium.org>
> > Date: Thu Jun 24 15:39:26 2021 -0700
> >
> > fortify: Add run-time WARN for cross-field memcpy()
> > [...]
> > Here, we might switch to unsafe_memcpy() instead of memcpy()
>
> I would tend to agree. Something like:
>
> diff --git a/net/sched/cls_u32.c b/net/sched/cls_u32.c
> index 4d27300c287c..21e0e6206ecc 100644
> --- a/net/sched/cls_u32.c
> +++ b/net/sched/cls_u32.c
> @@ -1040,7 +1040,9 @@ static int u32_change(struct net *net, struct sk_buff *in_skb,
> }
> #endif
>
> - memcpy(&n->sel, s, sel_size);
> + unsafe_memcpy(&n->sel, s, sel_size,
> + /* A composite flex-array structure destination,
> + * which was correctly sized and allocated above. */);
> RCU_INIT_POINTER(n->ht_up, ht);
> n->handle = handle;
> n->fshift = s->hmask ? ffs(ntohl(s->hmask)) - 1 : 0;

Ah, there is another in the same source file, in u32_init_knode():

memcpy(&new->sel, s, struct_size(s, keys, s->nkeys));

(I've been trying to convince Coccinelle to produce a list of all the
composite structure targets, but I keep running into weird glitches.
That it hadn't found this one let me track down the latest issue, so now
I should be able to find more! Whew.)

--
Kees Cook

[Dan Carpenter]

Smatch will soon start complaining about these. Can we instead do:

sel_size = size_add(struct_size(s, keys, s->nkeys), sizeof(n->sel));

Probably eventually Smatch will be able to detect some times when
struct_size() is used for readability and not for safety so maybe it
won't warn...

regards,
dan carpenter

[Jamal Hadi Salim]

Do you want to submit that patch then?
FWIW, I could not recreate what syzbot saw even after setting
CONFIG_FORTIFY_SOURCE=y

cheers,
jamal

[Dmitry Vyukov]

On Sun, 25 Sept 2022 at 17:38, Jamal Hadi Salim <j...@mojatatu.com> wrote:
>
> Is there a way to tell the boat "looking into it?"

Hi Jamal,

No, there is no way.
How do you propose the bot use that information? If it won't use it,
then there is no point in telling it.

Though, it makes sense to tell this to other people. But for that I
guess you just leave a note on the email thread.

> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bug...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/CAM0EoMnJ%3DSTtk5BnZ9oJtnkXY2Q%2BPx2cKa4gowFRGpp40UNKww%40mail.gmail.com.