| Message ID | 20220926150907.8551-1-yin31149@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | wext: use flex array destination for memcpy() | expand |
On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin31149@gmail.com> wrote: > > Syzkaller reports refcount bug as follows: This has nothing to do with a refcount bug... > ------------[ 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@syzkaller.appspotmail.com > Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@google.com/ > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > --- > 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 >
On Mon, Sep 26, 2022 at 11:09:06PM +0800, Hawkins Jiawei wrote: > Syzkaller reports refcount bug as follows: "buffer overflow false positive" instead of "refcount bug" > ------------[ 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@syzkaller.appspotmail.com > Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@google.com/ > Suggested-by: Kees Cook <keescook@chromium.org> > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> Thanks for spinning this and getting it tested! Reviewed-by: Kees Cook <keescook@chromium.org>
On Tue, 27 Sept 2022 at 00:14, Eric Dumazet <edumazet@google.com> wrote: > > On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin31149@gmail.com> 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@syzkaller.appspotmail.com > > Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@google.com/ > > Suggested-by: Kees Cook <keescook@chromium.org> > > Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> > > --- > > 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 > >
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);
Syzkaller reports refcount bug as follows: ------------[ 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@syzkaller.appspotmail.com Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@google.com/ Suggested-by: Kees Cook <keescook@chromium.org> Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> --- include/linux/wireless.h | 10 +++++++++- net/wireless/wext-core.c | 17 ++++++++++------- 2 files changed, 19 insertions(+), 8 deletions(-)