| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 26 Sep 2022 09:14:29 -0700
From: Eric Dumazet <edumazet@...gle.com>
To: Hawkins Jiawei <yin31149@...il.com>
Cc: syzbot+473754e5af963cf014cf@...kaller.appspotmail.com,
Johannes Berg <johannes@...solutions.net>,
"David S. Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
LKML <linux-kernel@...r.kernel.org>,
linux-wireless <linux-wireless@...r.kernel.org>,
netdev <netdev@...r.kernel.org>,
syzkaller-bugs <syzkaller-bugs@...glegroups.com>,
18801353760@....com, Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH] wext: use flex array destination for memcpy()
On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin31149@...il.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@...kaller.appspotmail.com
> Link: https://lore.kernel.org/all/00000000000070db2005e95a5984@google.com/
> Suggested-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Hawkins Jiawei <yin31149@...il.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
>
Powered by blists - more mailing lists