On Tue, 27 Sept 2022 at 00:14, Eric Dumazet <edumazet@xxxxxxxxxx> wrote: > > On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin31149@xxxxxxxxx> wrote: > > > > Syzkaller reports refcount bug as follows: > > This has nothing to do with a refcount bug... Sorry for my typo error, it should be a "buffer overflow false positive" bug as Kees Cook points out. I will correct it in my v2 patch. > > > ------------[ cut here ]------------ > > > > memcpy: detected field-spanning write (size 8) of single field > > "&compat_event->pointer" at net/wireless/wext-core.c:623 (size 4) > > WARNING: CPU: 0 PID: 3607 at net/wireless/wext-core.c:623 > > wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623 > > Modules linked in: > > CPU: 1 PID: 3607 Comm: syz-executor659 Not tainted > > 6.0.0-rc6-next-20220921-syzkaller #0 > > [...] > > Call Trace: > > <TASK> > > ioctl_standard_call+0x155/0x1f0 net/wireless/wext-core.c:1022 > > wireless_process_ioctl+0xc8/0x4c0 net/wireless/wext-core.c:955 > > wext_ioctl_dispatch net/wireless/wext-core.c:988 [inline] > > wext_ioctl_dispatch net/wireless/wext-core.c:976 [inline] > > wext_handle_ioctl+0x26b/0x280 net/wireless/wext-core.c:1049 > > sock_ioctl+0x285/0x640 net/socket.c:1220 > > vfs_ioctl fs/ioctl.c:51 [inline] > > __do_sys_ioctl fs/ioctl.c:870 [inline] > > __se_sys_ioctl fs/ioctl.c:856 [inline] > > __x64_sys_ioctl+0x193/0x200 fs/ioctl.c:856 > > 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 > > [...] > > </TASK> > > > > Wireless events will be sent on the appropriate channels in > > wireless_send_event(). Different wireless events may have different > > payload structure and size, so kernel uses **len** and **cmd** field > > in struct __compat_iw_event as wireless event common LCP part, uses > > **pointer** as a label to mark the position of remaining different part. > > > > Yet the problem is that, **pointer** is a compat_caddr_t type, which may > > be smaller than the relative structure at the same position. So during > > wireless_send_event() tries to parse the wireless events payload, it may > > trigger the memcpy() run-time destination buffer bounds checking when the > > relative structure's data is copied to the position marked by **pointer**. > > > > This patch solves it by introducing flexible-array field **ptr_bytes**, > > to mark the position of the wireless events remaining part next to > > LCP part. What's more, this patch also adds **ptr_len** variable in > > wireless_send_event() to improve its maintainability. > > > > Reported-and-tested-by: syzbot+473754e5af963cf014cf@xxxxxxxxxxxxxxxxxxxxxxxxx > > Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@xxxxxxxxxx/ > > Suggested-by: Kees Cook <keescook@xxxxxxxxxxxx> > > Signed-off-by: Hawkins Jiawei <yin31149@xxxxxxxxx> > > --- > > include/linux/wireless.h | 10 +++++++++- > > net/wireless/wext-core.c | 17 ++++++++++------- > > 2 files changed, 19 insertions(+), 8 deletions(-) > > > > diff --git a/include/linux/wireless.h b/include/linux/wireless.h > > index 2d1b54556eff..e6e34d74dda0 100644 > > --- a/include/linux/wireless.h > > +++ b/include/linux/wireless.h > > @@ -26,7 +26,15 @@ struct compat_iw_point { > > struct __compat_iw_event { > > __u16 len; /* Real length of this stuff */ > > __u16 cmd; /* Wireless IOCTL */ > > - compat_caddr_t pointer; > > + > > + union { > > + compat_caddr_t pointer; > > + > > + /* we need ptr_bytes to make memcpy() run-time destination > > + * buffer bounds checking happy, nothing special > > + */ > > + DECLARE_FLEX_ARRAY(__u8, ptr_bytes); > > + }; > > }; > > #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer) > > #define IW_EV_COMPAT_POINT_OFF offsetof(struct compat_iw_point, length) > > diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c > > index 76a80a41615b..fe8765c4075d 100644 > > --- a/net/wireless/wext-core.c > > +++ b/net/wireless/wext-core.c > > @@ -468,6 +468,7 @@ void wireless_send_event(struct net_device * dev, > > struct __compat_iw_event *compat_event; > > struct compat_iw_point compat_wrqu; > > struct sk_buff *compskb; > > + int ptr_len; > > #endif > > > > /* > > @@ -582,6 +583,9 @@ void wireless_send_event(struct net_device * dev, > > nlmsg_end(skb, nlh); > > #ifdef CONFIG_COMPAT > > hdr_len = compat_event_type_size[descr->header_type]; > > + > > + /* ptr_len is remaining size in event header apart from LCP */ > > + ptr_len = hdr_len - IW_EV_COMPAT_LCP_LEN; > > event_len = hdr_len + extra_len; > > > > compskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC); > > @@ -612,16 +616,15 @@ void wireless_send_event(struct net_device * dev, > > if (descr->header_type == IW_HEADER_TYPE_POINT) { > > compat_wrqu.length = wrqu->data.length; > > compat_wrqu.flags = wrqu->data.flags; > > - memcpy(&compat_event->pointer, > > - ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF, > > - hdr_len - IW_EV_COMPAT_LCP_LEN); > > + memcpy(compat_event->ptr_bytes, > > + ((char *)&compat_wrqu) + IW_EV_COMPAT_POINT_OFF, > > + ptr_len); > > if (extra_len) > > - memcpy(((char *) compat_event) + hdr_len, > > - extra, extra_len); > > + memcpy(&compat_event->ptr_bytes[ptr_len], > > + extra, extra_len); > > } else { > > /* extra_len must be zero, so no if (extra) needed */ > > - memcpy(&compat_event->pointer, wrqu, > > - hdr_len - IW_EV_COMPAT_LCP_LEN); > > + memcpy(compat_event->ptr_bytes, wrqu, ptr_len); > > } > > > > nlmsg_end(compskb, nlh); > > -- > > 2.25.1 > >
