| From e3e6e1d16a4cf7b63159ec71774e822194071954 Mon Sep 17 00:00:00 2001 |
| From: Hawkins Jiawei <yin31149@gmail.com> |
| Date: Tue, 27 Sep 2022 07:34:59 +0800 |
| Subject: wifi: wext: use flex array destination for memcpy() |
| |
| From: Hawkins Jiawei <yin31149@gmail.com> |
| |
| commit e3e6e1d16a4cf7b63159ec71774e822194071954 upstream. |
| |
| Syzkaller reports buffer overflow false positive 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> |
| Reviewed-by: Kees Cook <keescook@chromium.org> |
| Signed-off-by: Hawkins Jiawei <yin31149@gmail.com> |
| Signed-off-by: Johannes Berg <johannes.berg@intel.com> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| include/linux/wireless.h | 10 +++++++++- |
| net/wireless/wext-core.c | 17 ++++++++++------- |
| 2 files changed, 19 insertions(+), 8 deletions(-) |
| |
| --- 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) |
| --- a/net/wireless/wext-core.c |
| +++ b/net/wireless/wext-core.c |
| @@ -468,6 +468,7 @@ void wireless_send_event(struct net_devi |
| 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_devi |
| 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_devi |
| 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); |