* [syzbot] WARNING in wireless_send_event
@ 2022-09-23 16:05 syzbot
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
` (3 more replies)
0 siblings, 4 replies; 18+ messages in thread
From: syzbot @ 2022-09-23 16:05 UTC (permalink / raw)
To: davem, edumazet, johannes, kuba, linux-kernel, linux-wireless,
netdev, pabeni, syzkaller-bugs
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=1154ddd5080000
kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
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=157c196f080000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
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+473754e5af963cf014cf@syzkaller.appspotmail.com
------------[ 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
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
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
RIP: 0033:0x7fbda6736af9
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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
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 syzkaller@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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] Add linux-next specific files for 20220923
2022-09-23 16:05 [syzbot] WARNING in wireless_send_event syzbot
@ 2022-09-24 7:10 ` Hawkins Jiawei
2022-09-24 7:26 ` Kees Cook
2022-09-24 7:32 ` [syzbot] WARNING in wireless_send_event syzbot
2022-09-26 11:59 ` Hawkins Jiawei
` (2 subsequent siblings)
3 siblings, 2 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-24 7:10 UTC (permalink / raw)
To: syzbot+473754e5af963cf014cf, Johannes Berg, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, linux-wireless, netdev, syzkaller-bugs,
18801353760, yin31149, keescook, Stephen Rothwell
From: Stephen Rothwell <sfr@canb.auug.org.au>
> 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=1154ddd5080000
> kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
> dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
> 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=157c196f080000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
>
> 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+473754e5af963cf014cf@syzkaller.appspotmail.com
>
> ------------[ 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
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
> RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
> Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
> RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
> RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
> R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
> R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
> FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> 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
> RIP: 0033:0x7fbda6736af9
> 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
> RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
> RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
> R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> </TASK>
I think this is the samiliar problem as what Kees Cook pointed out in
https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
It seems that memcpy() will performs run-time buffer bounds
checking, which triggers this warning.
#syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
diff --git a/include/linux/wireless.h b/include/linux/wireless.h
index 2d1b54556eff..81603848b0aa 100644
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -26,7 +26,10 @@ 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;
+ __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
+ };
};
#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..9d0b50abbe09 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
extra, extra_len);
} else {
/* extra_len must be zero, so no if (extra) needed */
- memcpy(&compat_event->pointer, wrqu,
+ memcpy(&compat_event->pointer_flex, wrqu,
hdr_len - IW_EV_COMPAT_LCP_LEN);
}
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
@ 2022-09-24 7:26 ` Kees Cook
2022-09-24 11:44 ` Hawkins Jiawei
2022-09-24 7:32 ` [syzbot] WARNING in wireless_send_event syzbot
1 sibling, 1 reply; 18+ messages in thread
From: Kees Cook @ 2022-09-24 7:26 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+473754e5af963cf014cf, Johannes Berg, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel,
linux-wireless, netdev, syzkaller-bugs, 18801353760,
Stephen Rothwell
On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
> From: Stephen Rothwell <sfr@canb.auug.org.au>
>
> > 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=1154ddd5080000
> > kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
> > dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
> > 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=157c196f080000
> > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
> >
> > 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+473754e5af963cf014cf@syzkaller.appspotmail.com
> >
> > ------------[ 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
> > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
> > RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
> > Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
> > RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
> > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
> > RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
> > R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
> > R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
> > FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > 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
> > RIP: 0033:0x7fbda6736af9
> > 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
> > RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
> > RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
> > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
> > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > </TASK>
>
> I think this is the samiliar problem as what Kees Cook pointed out in
> https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
>
> It seems that memcpy() will performs run-time buffer bounds
> checking, which triggers this warning.
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> master
>
>
> diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> index 2d1b54556eff..81603848b0aa 100644
> --- a/include/linux/wireless.h
> +++ b/include/linux/wireless.h
> @@ -26,7 +26,10 @@ 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;
> + __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
> + };
Is this expected to be dynamically sized? I assume so, given the "Real
length" comment. :)
> };
> #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..9d0b50abbe09 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
adding in more context code:
memcpy(&compat_event->pointer,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
hdr_len - IW_EV_COMPAT_LCP_LEN);
if (extra_len)
memcpy(((char *) compat_event) + hdr_len,
extra, extra_len);
The code above has "pointer" as a memcpy destination as well. I think
that should be changed to pointer_flex as well, as the length calculation
is the same. I wonder what FORTIFY will think about the second memcpy
above. If I'm reading the math correctly, it might need to be:
if (extra_len) {
size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
}
> } else {
> /* extra_len must be zero, so no if (extra) needed */
> - memcpy(&compat_event->pointer, wrqu,
> + memcpy(&compat_event->pointer_flex, wrqu,
> hdr_len - IW_EV_COMPAT_LCP_LEN);
> }
>
But otherwise, yes, looks like the right modification. Thanks for tackling
this! It is quite a weird structure! :)
-Kees
--
Kees Cook
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [syzbot] WARNING in wireless_send_event
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
2022-09-24 7:26 ` Kees Cook
@ 2022-09-24 7:32 ` syzbot
1 sibling, 0 replies; 18+ messages in thread
From: syzbot @ 2022-09-24 7:32 UTC (permalink / raw)
To: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: syzbot+473754e5af963cf014cf@syzkaller.appspotmail.com
Tested on:
commit: aaa11ce2 Add linux-next specific files for 20220923
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=13b03250880000
kernel config: https://syzkaller.appspot.com/x/.config?x=186d1ff305f10294
dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=11b03250880000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 7:26 ` Kees Cook
@ 2022-09-24 11:44 ` Hawkins Jiawei
2022-09-24 15:55 ` Hawkins Jiawei
0 siblings, 1 reply; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-24 11:44 UTC (permalink / raw)
To: keescook
Cc: 18801353760, davem, edumazet, johannes, kuba, linux-kernel,
linux-wireless, netdev, pabeni, sfr, syzbot+473754e5af963cf014cf,
syzkaller-bugs, yin31149
Hi Kees,
On Sat, 24 Sept 2022 at 15:26, Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
> > From: Stephen Rothwell <sfr@canb.auug.org.au>
> >
> > > 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=1154ddd5080000
> > > kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
> > > dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
> > > 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=157c196f080000
> > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
> > >
> > > 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+473754e5af963cf014cf@syzkaller.appspotmail.com
> > >
> > > ------------[ 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
> > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
> > > RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
> > > Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
> > > RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
> > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > > RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
> > > RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
> > > R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
> > > R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
> > > FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
> > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > 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
> > > RIP: 0033:0x7fbda6736af9
> > > 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
> > > RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
> > > RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
> > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
> > > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > > </TASK>
> >
> > I think this is the samiliar problem as what Kees Cook pointed out in
> > https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
> >
> > It seems that memcpy() will performs run-time buffer bounds
> > checking, which triggers this warning.
> >
> > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > master
> >
> >
> > diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> > index 2d1b54556eff..81603848b0aa 100644
> > --- a/include/linux/wireless.h
> > +++ b/include/linux/wireless.h
> > @@ -26,7 +26,10 @@ 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;
> > + __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
> > + };
>
> Is this expected to be dynamically sized? I assume so, given the "Real
> length" comment. :)
I think this is dynamically sized.
hdr_len = compat_event_type_size[descr->header_type];
event_len = hdr_len + extra_len;
[...]
/* Add the wireless events in the netlink packet */
nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
if (!nla) {
kfree_skb(skb);
kfree_skb(compskb);
return;
}
compat_event = nla_data(nla);
[...]
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);
if (extra_len)
memcpy(((char *) compat_event) + hdr_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);
}
according to the above code, it seems that this structure is used to
parse ths payload from buffer, so the field **pointer** should just
be a position label to the unused bytes in buffer. Its unused bytes will be
parsed as different structure according to event type.
>
> > };
> > #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..9d0b50abbe09 100644
> > --- a/net/wireless/wext-core.c
> > +++ b/net/wireless/wext-core.c
> > @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
>
> adding in more context code:
>
> memcpy(&compat_event->pointer,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> hdr_len - IW_EV_COMPAT_LCP_LEN);
> if (extra_len)
> memcpy(((char *) compat_event) + hdr_len,
> extra, extra_len);
>
> The code above has "pointer" as a memcpy destination as well. I think
> that should be changed to pointer_flex as well, as the length calculation
> is the same. I wonder what FORTIFY will think about the second memcpy
> above. If I'm reading the math correctly, it might need to be:
>
> if (extra_len) {
> size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
> memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
> }
>
I agree with you. It seems that in this situation,
the event type has been cleared, the unuesd bytes start from **pointer**
field should be parsed as struct iw_point type as below, which is a bigger
structure than **pointer**, it will also triggers the memcpy() warning.
/*
* The problem for 64/32 bit.
*
* On 64-bit, a regular event is laid out as follows:
* An iw_point event is laid out like this instead:
* | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
* | event.len | event.cmd | p a d d i n g |
* | iwpnt.len | iwpnt.flg | p a d d i n g |
* | extra data ...
*
* The second padding exists because struct iw_point is extended,
* but this depends on the platform...
*
* On 32-bit, all the padding shouldn't be there.
*/
And as for the value of offsetof in calculating **offset**,
I wonder if we can use the macro defined in
include/linux/wireless.h as below, which makes code simplier:
#define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>
> > } else {
> > /* extra_len must be zero, so no if (extra) needed */
> > - memcpy(&compat_event->pointer, wrqu,
> > + memcpy(&compat_event->pointer_flex, wrqu,
> > hdr_len - IW_EV_COMPAT_LCP_LEN);
> > }
> >
>
> But otherwise, yes, looks like the right modification. Thanks for tackling
> this! It is quite a weird structure! :)
>
> -Kees
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 11:44 ` Hawkins Jiawei
@ 2022-09-24 15:55 ` Hawkins Jiawei
2022-09-24 15:55 ` syzbot
2022-09-24 16:26 ` [PATCH] Add linux-next specific files for 20220923 Kees Cook
0 siblings, 2 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-24 15:55 UTC (permalink / raw)
To: yin31149
Cc: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzbot+473754e5af963cf014cf, syzkaller-bugs
Hi,
On Sat, 24 Sept 2022 at 19:44, Hawkins Jiawei <yin31149@gmail.com> wrote:
>
> Hi Kees,
> On Sat, 24 Sept 2022 at 15:26, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
> > > From: Stephen Rothwell <sfr@canb.auug.org.au>
> > >
> > > > 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=1154ddd5080000
> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
> > > > 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=157c196f080000
> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
> > > >
> > > > 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+473754e5af963cf014cf@syzkaller.appspotmail.com
> > > >
> > > > ------------[ 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
> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
> > > > RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
> > > > Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
> > > > RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
> > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> > > > RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
> > > > RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
> > > > R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
> > > > R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
> > > > FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > > > 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
> > > > RIP: 0033:0x7fbda6736af9
> > > > 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
> > > > RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
> > > > RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
> > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
> > > > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> > > > </TASK>
> > >
> > > I think this is the samiliar problem as what Kees Cook pointed out in
> > > https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
> > >
> > > It seems that memcpy() will performs run-time buffer bounds
> > > checking, which triggers this warning.
> > >
> > > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > > master
> > >
> > >
> > > diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> > > index 2d1b54556eff..81603848b0aa 100644
> > > --- a/include/linux/wireless.h
> > > +++ b/include/linux/wireless.h
> > > @@ -26,7 +26,10 @@ 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;
> > > + __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
> > > + };
> >
> > Is this expected to be dynamically sized? I assume so, given the "Real
> > length" comment. :)
> I think this is dynamically sized.
>
> hdr_len = compat_event_type_size[descr->header_type];
> event_len = hdr_len + extra_len;
>
> [...]
>
> /* Add the wireless events in the netlink packet */
> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
> if (!nla) {
> kfree_skb(skb);
> kfree_skb(compskb);
> return;
> }
> compat_event = nla_data(nla);
>
> [...]
>
> 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);
> if (extra_len)
> memcpy(((char *) compat_event) + hdr_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);
> }
>
> according to the above code, it seems that this structure is used to
> parse ths payload from buffer, so the field **pointer** should just
> be a position label to the unused bytes in buffer. Its unused bytes will be
> parsed as different structure according to event type.
>
> >
> > > };
> > > #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..9d0b50abbe09 100644
> > > --- a/net/wireless/wext-core.c
> > > +++ b/net/wireless/wext-core.c
> > > @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
> >
> > adding in more context code:
> >
> > memcpy(&compat_event->pointer,
> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> > hdr_len - IW_EV_COMPAT_LCP_LEN);
> > if (extra_len)
> > memcpy(((char *) compat_event) + hdr_len,
> > extra, extra_len);
> >
> > The code above has "pointer" as a memcpy destination as well. I think
> > that should be changed to pointer_flex as well, as the length calculation
> > is the same. I wonder what FORTIFY will think about the second memcpy
> > above. If I'm reading the math correctly, it might need to be:
> >
> > if (extra_len) {
> > size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
> > memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
> > }
> >
> I agree with you. It seems that in this situation,
> the event type has been cleared, the unuesd bytes start from **pointer**
> field should be parsed as struct iw_point type as below, which is a bigger
> structure than **pointer**, it will also triggers the memcpy() warning.
> /*
> * The problem for 64/32 bit.
> *
> * On 64-bit, a regular event is laid out as follows:
> * An iw_point event is laid out like this instead:
> * | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
> * | event.len | event.cmd | p a d d i n g |
> * | iwpnt.len | iwpnt.flg | p a d d i n g |
> * | extra data ...
> *
> * The second padding exists because struct iw_point is extended,
> * but this depends on the platform...
> *
> * On 32-bit, all the padding shouldn't be there.
> */
>
> And as for the value of offsetof in calculating **offset**,
> I wonder if we can use the macro defined in
> include/linux/wireless.h as below, which makes code simplier:
> #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>
>
> >
> > > } else {
> > > /* extra_len must be zero, so no if (extra) needed */
> > > - memcpy(&compat_event->pointer, wrqu,
> > > + memcpy(&compat_event->pointer_flex, wrqu,
> > > hdr_len - IW_EV_COMPAT_LCP_LEN);
> > > }
> > >
> >
> > But otherwise, yes, looks like the right modification. Thanks for tackling
> > this! It is quite a weird structure! :)
> >
> > -Kees
> >
> > --
> > Kees Cook
hdr_len = compat_event_type_size[descr->header_type];
event_len = hdr_len + extra_len;
[...]
/* Add the wireless events in the netlink packet */
nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
if (!nla) {
kfree_skb(skb);
kfree_skb(compskb);
return;
}
compat_event = nla_data(nla);
if (descr->header_type == IW_HEADER_TYPE_POINT) {
[...]
memcpy(&compat_event->pointer,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
hdr_len - IW_EV_COMPAT_LCP_LEN);
if (extra_len)
memcpy(((char *) compat_event) + hdr_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);
}
According to above code, it seems that kernel will saves enough memory
(hdr_len + extra_len bytes) for payload structure in
nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
memory() check as below, which seems more simple:
#syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 76a80a41615b..a967da647e2b 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -609,19 +609,26 @@ void wireless_send_event(struct net_device * dev,
compat_event->len = event_len;
compat_event->cmd = cmd;
+
+ /* kernel reserves event_len's bytes for compat_event,
+ * so we don't need memcpy()'s bounds check
+ */
if (descr->header_type == IW_HEADER_TYPE_POINT) {
compat_wrqu.length = wrqu->data.length;
compat_wrqu.flags = wrqu->data.flags;
- memcpy(&compat_event->pointer,
+ unsafe_memcpy(&compat_event->pointer,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ hdr_len - IW_EV_COMPAT_LCP_LEN,
+ /* compat_event has enough room */);
if (extra_len)
- memcpy(((char *) compat_event) + hdr_len,
- extra, extra_len);
+ unsafe_memcpy(((char *) compat_event) + hdr_len,
+ extra, extra_len,
+ /* compat_event has enough room */);
} else {
/* extra_len must be zero, so no if (extra) needed */
- memcpy(&compat_event->pointer, wrqu,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ unsafe_memcpy(&compat_event->pointer, wrqu,
+ hdr_len - IW_EV_COMPAT_LCP_LEN,
+ /* compat_event has enough room */);
}
nlmsg_end(compskb, nlh);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 15:55 ` Hawkins Jiawei
@ 2022-09-24 15:55 ` syzbot
2022-09-24 16:13 ` Hawkins Jiawei
2022-09-24 16:26 ` [PATCH] Add linux-next specific files for 20220923 Kees Cook
1 sibling, 1 reply; 18+ messages in thread
From: syzbot @ 2022-09-24 15:55 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
> Hi,
>
> On Sat, 24 Sept 2022 at 19:44, Hawkins Jiawei <yin31149@gmail.com> wrote:
>>
>> Hi Kees,
>> On Sat, 24 Sept 2022 at 15:26, Kees Cook <keescook@chromium.org> wrote:
>> >
>> > On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
>> > > From: Stephen Rothwell <sfr@canb.auug.org.au>
>> > >
>> > > > 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=1154ddd5080000
>> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
>> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
>> > > > 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=157c196f080000
>> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
>> > > >
>> > > > 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+473754e5af963cf014cf@syzkaller.appspotmail.com
>> > > >
>> > > > ------------[ 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
>> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
>> > > > RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
>> > > > Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
>> > > > RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
>> > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
>> > > > RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
>> > > > RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
>> > > > R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
>> > > > R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
>> > > > FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
>> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> > > > CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
>> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>> > > > 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
>> > > > RIP: 0033:0x7fbda6736af9
>> > > > 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
>> > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
>> > > > RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
>> > > > RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
>> > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
>> > > > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
>> > > > </TASK>
>> > >
>> > > I think this is the samiliar problem as what Kees Cook pointed out in
>> > > https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
>> > >
>> > > It seems that memcpy() will performs run-time buffer bounds
>> > > checking, which triggers this warning.
>> > >
>> > > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>> > > master
>> > >
>> > >
>> > > diff --git a/include/linux/wireless.h b/include/linux/wireless.h
>> > > index 2d1b54556eff..81603848b0aa 100644
>> > > --- a/include/linux/wireless.h
>> > > +++ b/include/linux/wireless.h
>> > > @@ -26,7 +26,10 @@ 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;
>> > > + __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
>> > > + };
>> >
>> > Is this expected to be dynamically sized? I assume so, given the "Real
>> > length" comment. :)
>> I think this is dynamically sized.
>>
>> hdr_len = compat_event_type_size[descr->header_type];
>> event_len = hdr_len + extra_len;
>>
>> [...]
>>
>> /* Add the wireless events in the netlink packet */
>> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
>> if (!nla) {
>> kfree_skb(skb);
>> kfree_skb(compskb);
>> return;
>> }
>> compat_event = nla_data(nla);
>>
>> [...]
>>
>> 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);
>> if (extra_len)
>> memcpy(((char *) compat_event) + hdr_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);
>> }
>>
>> according to the above code, it seems that this structure is used to
>> parse ths payload from buffer, so the field **pointer** should just
>> be a position label to the unused bytes in buffer. Its unused bytes will be
>> parsed as different structure according to event type.
>>
>> >
>> > > };
>> > > #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..9d0b50abbe09 100644
>> > > --- a/net/wireless/wext-core.c
>> > > +++ b/net/wireless/wext-core.c
>> > > @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
>> >
>> > adding in more context code:
>> >
>> > memcpy(&compat_event->pointer,
>> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
>> > hdr_len - IW_EV_COMPAT_LCP_LEN);
>> > if (extra_len)
>> > memcpy(((char *) compat_event) + hdr_len,
>> > extra, extra_len);
>> >
>> > The code above has "pointer" as a memcpy destination as well. I think
>> > that should be changed to pointer_flex as well, as the length calculation
>> > is the same. I wonder what FORTIFY will think about the second memcpy
>> > above. If I'm reading the math correctly, it might need to be:
>> >
>> > if (extra_len) {
>> > size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
>> > memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
>> > }
>> >
>> I agree with you. It seems that in this situation,
>> the event type has been cleared, the unuesd bytes start from **pointer**
>> field should be parsed as struct iw_point type as below, which is a bigger
>> structure than **pointer**, it will also triggers the memcpy() warning.
>> /*
>> * The problem for 64/32 bit.
>> *
>> * On 64-bit, a regular event is laid out as follows:
>> * An iw_point event is laid out like this instead:
>> * | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
>> * | event.len | event.cmd | p a d d i n g |
>> * | iwpnt.len | iwpnt.flg | p a d d i n g |
>> * | extra data ...
>> *
>> * The second padding exists because struct iw_point is extended,
>> * but this depends on the platform...
>> *
>> * On 32-bit, all the padding shouldn't be there.
>> */
>>
>> And as for the value of offsetof in calculating **offset**,
>> I wonder if we can use the macro defined in
>> include/linux/wireless.h as below, which makes code simplier:
>> #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>>
>>
>> >
>> > > } else {
>> > > /* extra_len must be zero, so no if (extra) needed */
>> > > - memcpy(&compat_event->pointer, wrqu,
>> > > + memcpy(&compat_event->pointer_flex, wrqu,
>> > > hdr_len - IW_EV_COMPAT_LCP_LEN);
>> > > }
>> > >
>> >
>> > But otherwise, yes, looks like the right modification. Thanks for tackling
>> > this! It is quite a weird structure! :)
>> >
>> > -Kees
>> >
>> > --
>> > Kees Cook
>
> hdr_len = compat_event_type_size[descr->header_type];
> event_len = hdr_len + extra_len;
>
> [...]
>
> /* Add the wireless events in the netlink packet */
> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
> if (!nla) {
> kfree_skb(skb);
> kfree_skb(compskb);
> return;
> }
> compat_event = nla_data(nla);
>
> if (descr->header_type == IW_HEADER_TYPE_POINT) {
> [...]
> memcpy(&compat_event->pointer,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> hdr_len - IW_EV_COMPAT_LCP_LEN);
> if (extra_len)
> memcpy(((char *) compat_event) + hdr_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);
> }
>
>
> According to above code, it seems that kernel will saves enough memory
> (hdr_len + extra_len bytes) for payload structure in
> nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
> So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
> memory() check as below, which seems more simple:
>
> #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
want 2 args (repo, branch), got 5
>
> diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> index 76a80a41615b..a967da647e2b 100644
> --- a/net/wireless/wext-core.c
> +++ b/net/wireless/wext-core.c
> @@ -609,19 +609,26 @@ void wireless_send_event(struct net_device * dev,
>
> compat_event->len = event_len;
> compat_event->cmd = cmd;
> +
> + /* kernel reserves event_len's bytes for compat_event,
> + * so we don't need memcpy()'s bounds check
> + */
> if (descr->header_type == IW_HEADER_TYPE_POINT) {
> compat_wrqu.length = wrqu->data.length;
> compat_wrqu.flags = wrqu->data.flags;
> - memcpy(&compat_event->pointer,
> + unsafe_memcpy(&compat_event->pointer,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> - hdr_len - IW_EV_COMPAT_LCP_LEN);
> + hdr_len - IW_EV_COMPAT_LCP_LEN,
> + /* compat_event has enough room */);
> if (extra_len)
> - memcpy(((char *) compat_event) + hdr_len,
> - extra, extra_len);
> + unsafe_memcpy(((char *) compat_event) + hdr_len,
> + extra, extra_len,
> + /* compat_event has enough room */);
> } else {
> /* extra_len must be zero, so no if (extra) needed */
> - memcpy(&compat_event->pointer, wrqu,
> - hdr_len - IW_EV_COMPAT_LCP_LEN);
> + unsafe_memcpy(&compat_event->pointer, wrqu,
> + hdr_len - IW_EV_COMPAT_LCP_LEN,
> + /* compat_event has enough room */);
> }
>
> nlmsg_end(compskb, nlh);
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 15:55 ` syzbot
@ 2022-09-24 16:13 ` Hawkins Jiawei
2022-09-24 16:33 ` [syzbot] WARNING in wireless_send_event syzbot
0 siblings, 1 reply; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-24 16:13 UTC (permalink / raw)
To: syzbot+473754e5af963cf014cf
Cc: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
On Sat, 24 Sept 2022 at 23:55, syzbot <syzbot+473754e5af963cf014cf@syzkaller.appspotmail.com> wrote:
>
> > Hi,
> >
> > On Sat, 24 Sept 2022 at 19:44, Hawkins Jiawei <yin31149@gmail.com> wrote:
> >>
> >> Hi Kees,
> >> On Sat, 24 Sept 2022 at 15:26, Kees Cook <keescook@chromium.org> wrote:
> >> >
> >> > On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
> >> > > From: Stephen Rothwell <sfr@canb.auug.org.au>
> >> > >
> >> > > > 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=1154ddd5080000
> >> > > > kernel config: https://syzkaller.appspot.com/x/.config?x=849cb9f70f15b1ba
> >> > > > dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
> >> > > > 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=157c196f080000
> >> > > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11f12618880000
> >> > > >
> >> > > > 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+473754e5af963cf014cf@syzkaller.appspotmail.com
> >> > > >
> >> > > > ------------[ 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
> >> > > > Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 09/16/2022
> >> > > > RIP: 0010:wireless_send_event+0xab5/0xca0 net/wireless/wext-core.c:623
> >> > > > Code: fa ff ff e8 cd b9 db f8 b9 04 00 00 00 4c 89 e6 48 c7 c2 e0 56 11 8b 48 c7 c7 20 56 11 8b c6 05 94 8e 2a 05 01 e8 b8 b0 a6 00 <0f> 0b e9 9b fa ff ff e8 6f ef 27 f9 e9 a6 fd ff ff e8 c5 ef 27 f9
> >> > > > RSP: 0018:ffffc90003b2fbc0 EFLAGS: 00010286
> >> > > > RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> >> > > > RDX: ffff888021d157c0 RSI: ffffffff81620348 RDI: fffff52000765f6a
> >> > > > RBP: ffff88801e15c780 R08: 0000000000000005 R09: 0000000000000000
> >> > > > R10: 0000000080000000 R11: 20676e696e6e6170 R12: 0000000000000008
> >> > > > R13: ffff888025a72640 R14: ffff8880225d402c R15: ffff8880225d4034
> >> > > > FS: 0000555556bd9300(0000) GS:ffff8880b9b00000(0000) knlGS:0000000000000000
> >> > > > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> >> > > > CR2: 00007fbda677dfb8 CR3: 000000007b976000 CR4: 00000000003506e0
> >> > > > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> >> > > > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> >> > > > 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
> >> > > > RIP: 0033:0x7fbda6736af9
> >> > > > 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:00007ffd45e80138 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
> >> > > > RAX: ffffffffffffffda RBX: 0000000000000000 RCX: 00007fbda6736af9
> >> > > > RDX: 0000000020000000 RSI: 0000000000008b04 RDI: 0000000000000003
> >> > > > RBP: 00007fbda66faca0 R08: 0000000000000000 R09: 0000000000000000
> >> > > > R10: 0000000000000000 R11: 0000000000000246 R12: 00007fbda66fad30
> >> > > > R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000000
> >> > > > </TASK>
> >> > >
> >> > > I think this is the samiliar problem as what Kees Cook pointed out in
> >> > > https://lore.kernel.org/linux-next/202209211250.3049C29@keescook/
> >> > >
> >> > > It seems that memcpy() will performs run-time buffer bounds
> >> > > checking, which triggers this warning.
> >> > >
> >> > > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> >> > > master
> >> > >
> >> > >
> >> > > diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> >> > > index 2d1b54556eff..81603848b0aa 100644
> >> > > --- a/include/linux/wireless.h
> >> > > +++ b/include/linux/wireless.h
> >> > > @@ -26,7 +26,10 @@ 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;
> >> > > + __DECLARE_FLEX_ARRAY(__u8, pointer_flex);
> >> > > + };
> >> >
> >> > Is this expected to be dynamically sized? I assume so, given the "Real
> >> > length" comment. :)
> >> I think this is dynamically sized.
> >>
> >> hdr_len = compat_event_type_size[descr->header_type];
> >> event_len = hdr_len + extra_len;
> >>
> >> [...]
> >>
> >> /* Add the wireless events in the netlink packet */
> >> nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
> >> if (!nla) {
> >> kfree_skb(skb);
> >> kfree_skb(compskb);
> >> return;
> >> }
> >> compat_event = nla_data(nla);
> >>
> >> [...]
> >>
> >> 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);
> >> if (extra_len)
> >> memcpy(((char *) compat_event) + hdr_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);
> >> }
> >>
> >> according to the above code, it seems that this structure is used to
> >> parse ths payload from buffer, so the field **pointer** should just
> >> be a position label to the unused bytes in buffer. Its unused bytes will be
> >> parsed as different structure according to event type.
> >>
> >> >
> >> > > };
> >> > > #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..9d0b50abbe09 100644
> >> > > --- a/net/wireless/wext-core.c
> >> > > +++ b/net/wireless/wext-core.c
> >> > > @@ -620,7 +620,7 @@ void wireless_send_event(struct net_device * dev,
> >> >
> >> > adding in more context code:
> >> >
> >> > memcpy(&compat_event->pointer,
> >> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> >> > hdr_len - IW_EV_COMPAT_LCP_LEN);
> >> > if (extra_len)
> >> > memcpy(((char *) compat_event) + hdr_len,
> >> > extra, extra_len);
> >> >
> >> > The code above has "pointer" as a memcpy destination as well. I think
> >> > that should be changed to pointer_flex as well, as the length calculation
> >> > is the same. I wonder what FORTIFY will think about the second memcpy
> >> > above. If I'm reading the math correctly, it might need to be:
> >> >
> >> > if (extra_len) {
> >> > size_t offset = hdr_len - offsetof(typeof(*compat_event), pointer_flex);
> >> > memcpy(&compat_event->pointer_flex[offset], extra, extra_len);
> >> > }
> >> >
> >> I agree with you. It seems that in this situation,
> >> the event type has been cleared, the unuesd bytes start from **pointer**
> >> field should be parsed as struct iw_point type as below, which is a bigger
> >> structure than **pointer**, it will also triggers the memcpy() warning.
> >> /*
> >> * The problem for 64/32 bit.
> >> *
> >> * On 64-bit, a regular event is laid out as follows:
> >> * An iw_point event is laid out like this instead:
> >> * | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 |
> >> * | event.len | event.cmd | p a d d i n g |
> >> * | iwpnt.len | iwpnt.flg | p a d d i n g |
> >> * | extra data ...
> >> *
> >> * The second padding exists because struct iw_point is extended,
> >> * but this depends on the platform...
> >> *
> >> * On 32-bit, all the padding shouldn't be there.
> >> */
> >>
> >> And as for the value of offsetof in calculating **offset**,
> >> I wonder if we can use the macro defined in
> >> include/linux/wireless.h as below, which makes code simplier:
> >> #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
> >>
> >>
> >> >
> >> > > } else {
> >> > > /* extra_len must be zero, so no if (extra) needed */
> >> > > - memcpy(&compat_event->pointer, wrqu,
> >> > > + memcpy(&compat_event->pointer_flex, wrqu,
> >> > > hdr_len - IW_EV_COMPAT_LCP_LEN);
> >> > > }
> >> > >
> >> >
> >> > But otherwise, yes, looks like the right modification. Thanks for tackling
> >> > this! It is quite a weird structure! :)
> >> >
> >> > -Kees
> >> >
> >> > --
> >> > Kees Cook
> >
> > hdr_len = compat_event_type_size[descr->header_type];
> > event_len = hdr_len + extra_len;
> >
> > [...]
> >
> > /* Add the wireless events in the netlink packet */
> > nla = nla_reserve(compskb, IFLA_WIRELESS, event_len);
> > if (!nla) {
> > kfree_skb(skb);
> > kfree_skb(compskb);
> > return;
> > }
> > compat_event = nla_data(nla);
> >
> > if (descr->header_type == IW_HEADER_TYPE_POINT) {
> > [...]
> > memcpy(&compat_event->pointer,
> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> > hdr_len - IW_EV_COMPAT_LCP_LEN);
> > if (extra_len)
> > memcpy(((char *) compat_event) + hdr_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);
> > }
> >
> >
> > According to above code, it seems that kernel will saves enough memory
> > (hdr_len + extra_len bytes) for payload structure in
> > nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
> > So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
> > memory() check as below, which seems more simple:
> >
> > #syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
>
> want 2 args (repo, branch), got 5
>
> >
> > diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
> > index 76a80a41615b..a967da647e2b 100644
> > --- a/net/wireless/wext-core.c
> > +++ b/net/wireless/wext-core.c
> > @@ -609,19 +609,26 @@ void wireless_send_event(struct net_device * dev,
> >
> > compat_event->len = event_len;
> > compat_event->cmd = cmd;
> > +
> > + /* kernel reserves event_len's bytes for compat_event,
> > + * so we don't need memcpy()'s bounds check
> > + */
> > if (descr->header_type == IW_HEADER_TYPE_POINT) {
> > compat_wrqu.length = wrqu->data.length;
> > compat_wrqu.flags = wrqu->data.flags;
> > - memcpy(&compat_event->pointer,
> > + unsafe_memcpy(&compat_event->pointer,
> > ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> > - hdr_len - IW_EV_COMPAT_LCP_LEN);
> > + hdr_len - IW_EV_COMPAT_LCP_LEN,
> > + /* compat_event has enough room */);
> > if (extra_len)
> > - memcpy(((char *) compat_event) + hdr_len,
> > - extra, extra_len);
> > + unsafe_memcpy(((char *) compat_event) + hdr_len,
> > + extra, extra_len,
> > + /* compat_event has enough room */);
> > } else {
> > /* extra_len must be zero, so no if (extra) needed */
> > - memcpy(&compat_event->pointer, wrqu,
> > - hdr_len - IW_EV_COMPAT_LCP_LEN);
> > + unsafe_memcpy(&compat_event->pointer, wrqu,
> > + hdr_len - IW_EV_COMPAT_LCP_LEN,
> > + /* compat_event has enough room */);
> > }
> >
> > nlmsg_end(compskb, nlh);
#syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
master
diff --git a/net/wireless/wext-core.c b/net/wireless/wext-core.c
index 76a80a41615b..a967da647e2b 100644
--- a/net/wireless/wext-core.c
+++ b/net/wireless/wext-core.c
@@ -609,19 +609,26 @@ void wireless_send_event(struct net_device * dev,
compat_event->len = event_len;
compat_event->cmd = cmd;
+
+ /* kernel reserves event_len's bytes for compat_event,
+ * so we don't need memcpy()'s bounds check
+ */
if (descr->header_type == IW_HEADER_TYPE_POINT) {
compat_wrqu.length = wrqu->data.length;
compat_wrqu.flags = wrqu->data.flags;
- memcpy(&compat_event->pointer,
+ unsafe_memcpy(&compat_event->pointer,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ hdr_len - IW_EV_COMPAT_LCP_LEN,
+ /* compat_event has enough room */);
if (extra_len)
- memcpy(((char *) compat_event) + hdr_len,
- extra, extra_len);
+ unsafe_memcpy(((char *) compat_event) + hdr_len,
+ extra, extra_len,
+ /* compat_event has enough room */);
} else {
/* extra_len must be zero, so no if (extra) needed */
- memcpy(&compat_event->pointer, wrqu,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ unsafe_memcpy(&compat_event->pointer, wrqu,
+ hdr_len - IW_EV_COMPAT_LCP_LEN,
+ /* compat_event has enough room */);
}
nlmsg_end(compskb, nlh);
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 15:55 ` Hawkins Jiawei
2022-09-24 15:55 ` syzbot
@ 2022-09-24 16:26 ` Kees Cook
2022-09-25 6:25 ` Hawkins Jiawei
1 sibling, 1 reply; 18+ messages in thread
From: Kees Cook @ 2022-09-24 16:26 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: 18801353760, davem, edumazet, johannes, kuba, linux-kernel,
linux-wireless, netdev, pabeni, sfr, syzbot+473754e5af963cf014cf,
syzkaller-bugs
On Sat, Sep 24, 2022 at 11:55:14PM +0800, Hawkins Jiawei wrote:
> > And as for the value of offsetof in calculating **offset**,
> > I wonder if we can use the macro defined in
> > include/linux/wireless.h as below, which makes code simplier:
> > #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
Ah yes, that would be good.
> According to above code, it seems that kernel will saves enough memory
> (hdr_len + extra_len bytes) for payload structure in
> nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
> So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
> memcpy() check as below, which seems more simple:
I'd rather this was properly resolved with the creation of a real
flexible array so that when bounds tracking gets improved in the future,
the compiler can reason about it better. And, I think, it makes the code
more readable:
diff --git a/include/linux/wireless.h b/include/linux/wireless.h
index 2d1b54556eff..e0b4b46da63f 100644
--- a/include/linux/wireless.h
+++ b/include/linux/wireless.h
@@ -26,7 +26,10 @@ 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;
+ 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..6079c8f4b634 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,7 @@ 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 = hdr_len - IW_EV_COMPAT_LCP_LEN;
event_len = hdr_len + extra_len;
compskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
@@ -612,16 +614,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,
+ memcpy(compat_event->ptr_bytes,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ ptr_len);
if (extra_len)
- memcpy(((char *) compat_event) + hdr_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);
--
Kees Cook
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [syzbot] WARNING in wireless_send_event
2022-09-24 16:13 ` Hawkins Jiawei
@ 2022-09-24 16:33 ` syzbot
0 siblings, 0 replies; 18+ messages in thread
From: syzbot @ 2022-09-24 16:33 UTC (permalink / raw)
To: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
Hello,
syzbot tried to test the proposed patch but the build/boot failed:
failed to create VM pool: failed to create GCE image: create image operation failed: &{Code:PERMISSIONS_ERROR ErrorDetails:[] Location: Message:Required 'read' permission for 'disks/ci-upstream-linux-next-kasan-gce-root-test-job-test-job-image.tar.gz' ForceSendFields:[] NullFields:[]}.
syzkaller build log:
go env (err=<nil>)
GO111MODULE="auto"
GOARCH="amd64"
GOBIN=""
GOCACHE="/syzkaller/.cache/go-build"
GOENV="/syzkaller/.config/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOINSECURE=""
GOMODCACHE="/syzkaller/jobs/linux/gopath/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="linux"
GOPATH="/syzkaller/jobs/linux/gopath"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/linux_amd64"
GOVCS=""
GOVERSION="go1.17"
GCCGO="gccgo"
AR="ar"
CC="gcc"
CXX="g++"
CGO_ENABLED="1"
GOMOD="/syzkaller/jobs/linux/gopath/src/github.com/google/syzkaller/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build4239397487=/tmp/go-build -gno-record-gcc-switches"
git status (err=<nil>)
HEAD detached at 380f82fb6
nothing to commit, working tree clean
go list -f '{{.Stale}}' ./sys/syz-sysgen | grep -q false || go install ./sys/syz-sysgen
make .descriptions
bin/syz-sysgen
touch .descriptions
GOOS=linux GOARCH=amd64 go build "-ldflags=-s -w -X github.com/google/syzkaller/prog.GitRevision=380f82fb6ebefdaa2b4e4f84d34a9019900f0b89 -X 'github.com/google/syzkaller/prog.gitRevisionDate=20220921-114622'" "-tags=syz_target syz_os_linux syz_arch_amd64 " -o ./bin/linux_amd64/syz-fuzzer github.com/google/syzkaller/syz-fuzzer
GOOS=linux GOARCH=amd64 go build "-ldflags=-s -w -X github.com/google/syzkaller/prog.GitRevision=380f82fb6ebefdaa2b4e4f84d34a9019900f0b89 -X 'github.com/google/syzkaller/prog.gitRevisionDate=20220921-114622'" "-tags=syz_target syz_os_linux syz_arch_amd64 " -o ./bin/linux_amd64/syz-execprog github.com/google/syzkaller/tools/syz-execprog
GOOS=linux GOARCH=amd64 go build "-ldflags=-s -w -X github.com/google/syzkaller/prog.GitRevision=380f82fb6ebefdaa2b4e4f84d34a9019900f0b89 -X 'github.com/google/syzkaller/prog.gitRevisionDate=20220921-114622'" "-tags=syz_target syz_os_linux syz_arch_amd64 " -o ./bin/linux_amd64/syz-stress github.com/google/syzkaller/tools/syz-stress
mkdir -p ./bin/linux_amd64
gcc -o ./bin/linux_amd64/syz-executor executor/executor.cc \
-m64 -O2 -pthread -Wall -Werror -Wparentheses -Wunused-const-variable -Wframe-larger-than=16384 -Wno-stringop-overflow -Wno-array-bounds -Wno-format-overflow -static-pie -fpermissive -w -DGOOS_linux=1 -DGOARCH_amd64=1 \
-DHOSTGOOS_linux=1 -DGIT_REVISION=\"380f82fb6ebefdaa2b4e4f84d34a9019900f0b89\"
Tested on:
commit: aaa11ce2 Add linux-next specific files for 20220923
git tree: linux-next
kernel config: https://syzkaller.appspot.com/x/.config?x=186d1ff305f10294
dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=15585edf080000
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] Add linux-next specific files for 20220923
2022-09-24 16:26 ` [PATCH] Add linux-next specific files for 20220923 Kees Cook
@ 2022-09-25 6:25 ` Hawkins Jiawei
0 siblings, 0 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-25 6:25 UTC (permalink / raw)
To: keescook
Cc: 18801353760, davem, edumazet, johannes, kuba, linux-kernel,
linux-wireless, netdev, pabeni, sfr, syzbot+473754e5af963cf014cf,
syzkaller-bugs, yin31149
On Sun, 25 Sept 2022 at 00:26, Kees Cook <keescook@chromium.org> wrote:
>
> On Sat, Sep 24, 2022 at 11:55:14PM +0800, Hawkins Jiawei wrote:
> > > And as for the value of offsetof in calculating **offset**,
> > > I wonder if we can use the macro defined in
> > > include/linux/wireless.h as below, which makes code simplier:
> > > #define IW_EV_COMPAT_LCP_LEN offsetof(struct __compat_iw_event, pointer)
>
> Ah yes, that would be good.
>
> > According to above code, it seems that kernel will saves enough memory
> > (hdr_len + extra_len bytes) for payload structure in
> > nla_reserve()(Please correct me if I am wrong), pointed by compat_event.
> > So I wonder if we can use unsafe_memcpy(), to avoid unnecessary
> > memcpy() check as below, which seems more simple:
>
> I'd rather this was properly resolved with the creation of a real
> flexible array so that when bounds tracking gets improved in the future,
> the compiler can reason about it better. And, I think, it makes the code
> more readable:
>
> diff --git a/include/linux/wireless.h b/include/linux/wireless.h
> index 2d1b54556eff..e0b4b46da63f 100644
> --- a/include/linux/wireless.h
> +++ b/include/linux/wireless.h
> @@ -26,7 +26,10 @@ 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;
> + 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..6079c8f4b634 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,7 @@ 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 = hdr_len - IW_EV_COMPAT_LCP_LEN;
> event_len = hdr_len + extra_len;
>
> compskb = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_ATOMIC);
> @@ -612,16 +614,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,
> + memcpy(compat_event->ptr_bytes,
> ((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
> - hdr_len - IW_EV_COMPAT_LCP_LEN);
> + ptr_len);
> if (extra_len)
> - memcpy(((char *) compat_event) + hdr_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);
>
>
> --
> Kees Cook
Yes, it seems that this code is more readable. I will refactor the patch
in this way, with some comments on union in struct compat_iw_point.
By the way, do you think we need to refactor the **struct compat_iw_point**
into union with some comments, or just replace the **pointer** field with
**DECLARE_FLEX_ARRAY(__u8, ptr_bytes)**? Because it seems that this field is
only used in wireless_send_event() and some macros in this
include/linux/wireless.h
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [syzbot] WARNING in wireless_send_event
2022-09-23 16:05 [syzbot] WARNING in wireless_send_event syzbot
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
@ 2022-09-26 11:59 ` Hawkins Jiawei
2022-09-26 12:24 ` syzbot
2022-09-26 15:09 ` [PATCH] wext: use flex array destination for memcpy() Hawkins Jiawei
2022-09-26 23:34 ` [PATCH wireless-next v2] " Hawkins Jiawei
3 siblings, 1 reply; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-26 11:59 UTC (permalink / raw)
To: syzbot+473754e5af963cf014cf
Cc: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
#syz test git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
aaa11ce2ffc84166d11c4d2ac88c3fcf75425fbd
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.
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 | 13 ++++++++-----
2 files changed, 17 insertions(+), 6 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..2ca009aca865 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,
+ memcpy(compat_event->ptr_bytes,
((char *) &compat_wrqu) + IW_EV_COMPAT_POINT_OFF,
- hdr_len - IW_EV_COMPAT_LCP_LEN);
+ ptr_len);
if (extra_len)
- memcpy(((char *) compat_event) + hdr_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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [syzbot] WARNING in wireless_send_event
2022-09-26 11:59 ` Hawkins Jiawei
@ 2022-09-26 12:24 ` syzbot
0 siblings, 0 replies; 18+ messages in thread
From: syzbot @ 2022-09-26 12:24 UTC (permalink / raw)
To: 18801353760, davem, edumazet, johannes, keescook, kuba,
linux-kernel, linux-wireless, netdev, pabeni, sfr,
syzkaller-bugs, yin31149
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger any issue:
Reported-and-tested-by: syzbot+473754e5af963cf014cf@syzkaller.appspotmail.com
Tested on:
commit: aaa11ce2 Add linux-next specific files for 20220923
git tree: git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
console output: https://syzkaller.appspot.com/x/log.txt?x=1490566c880000
kernel config: https://syzkaller.appspot.com/x/.config?x=186d1ff305f10294
dashboard link: https://syzkaller.appspot.com/bug?extid=473754e5af963cf014cf
compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2
patch: https://syzkaller.appspot.com/x/patch.diff?x=162cc9e4880000
Note: testing is done by a robot and is best-effort only.
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH] wext: use flex array destination for memcpy()
2022-09-23 16:05 [syzbot] WARNING in wireless_send_event syzbot
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
2022-09-26 11:59 ` Hawkins Jiawei
@ 2022-09-26 15:09 ` Hawkins Jiawei
2022-09-26 16:14 ` Eric Dumazet
2022-09-26 17:43 ` Kees Cook
2022-09-26 23:34 ` [PATCH wireless-next v2] " Hawkins Jiawei
3 siblings, 2 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-26 15:09 UTC (permalink / raw)
To: syzbot+473754e5af963cf014cf, Johannes Berg, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni
Cc: linux-kernel, linux-wireless, netdev, syzkaller-bugs,
18801353760, yin31149, Kees Cook
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(-)
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
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH] wext: use flex array destination for memcpy()
2022-09-26 15:09 ` [PATCH] wext: use flex array destination for memcpy() Hawkins Jiawei
@ 2022-09-26 16:14 ` Eric Dumazet
2022-09-26 23:17 ` Hawkins Jiawei
2022-09-26 17:43 ` Kees Cook
1 sibling, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2022-09-26 16:14 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+473754e5af963cf014cf, Johannes Berg, David S. Miller,
Jakub Kicinski, Paolo Abeni, LKML, linux-wireless, netdev,
syzkaller-bugs, 18801353760, Kees Cook
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
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] wext: use flex array destination for memcpy()
2022-09-26 15:09 ` [PATCH] wext: use flex array destination for memcpy() Hawkins Jiawei
2022-09-26 16:14 ` Eric Dumazet
@ 2022-09-26 17:43 ` Kees Cook
1 sibling, 0 replies; 18+ messages in thread
From: Kees Cook @ 2022-09-26 17:43 UTC (permalink / raw)
To: Hawkins Jiawei
Cc: syzbot+473754e5af963cf014cf, Johannes Berg, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, linux-kernel,
linux-wireless, netdev, syzkaller-bugs, 18801353760
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>
--
Kees Cook
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] wext: use flex array destination for memcpy()
2022-09-26 16:14 ` Eric Dumazet
@ 2022-09-26 23:17 ` Hawkins Jiawei
0 siblings, 0 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-26 23:17 UTC (permalink / raw)
To: edumazet
Cc: 18801353760, davem, johannes, keescook, kuba, linux-kernel,
linux-wireless, netdev, pabeni, syzbot+473754e5af963cf014cf,
syzkaller-bugs, yin31149
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
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH wireless-next v2] wext: use flex array destination for memcpy()
2022-09-23 16:05 [syzbot] WARNING in wireless_send_event syzbot
` (2 preceding siblings ...)
2022-09-26 15:09 ` [PATCH] wext: use flex array destination for memcpy() Hawkins Jiawei
@ 2022-09-26 23:34 ` Hawkins Jiawei
3 siblings, 0 replies; 18+ messages in thread
From: Hawkins Jiawei @ 2022-09-26 23:34 UTC (permalink / raw)
To: syzbot+473754e5af963cf014cf, edumazet, Johannes Berg,
David S. Miller, Jakub Kicinski, Paolo Abeni
Cc: 18801353760, keescook, linux-kernel, linux-wireless, netdev,
syzkaller-bugs, yin31149
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>
---
v2: correct the typo error pointed out by Eric Dumazet and
Kees Cook
v1: https://lore.kernel.org/all/20220926150907.8551-1-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
^ permalink raw reply related [flat|nested] 18+ messages in thread
end of thread, other threads:[~2022-09-26 23:35 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-23 16:05 [syzbot] WARNING in wireless_send_event syzbot
2022-09-24 7:10 ` [PATCH] Add linux-next specific files for 20220923 Hawkins Jiawei
2022-09-24 7:26 ` Kees Cook
2022-09-24 11:44 ` Hawkins Jiawei
2022-09-24 15:55 ` Hawkins Jiawei
2022-09-24 15:55 ` syzbot
2022-09-24 16:13 ` Hawkins Jiawei
2022-09-24 16:33 ` [syzbot] WARNING in wireless_send_event syzbot
2022-09-24 16:26 ` [PATCH] Add linux-next specific files for 20220923 Kees Cook
2022-09-25 6:25 ` Hawkins Jiawei
2022-09-24 7:32 ` [syzbot] WARNING in wireless_send_event syzbot
2022-09-26 11:59 ` Hawkins Jiawei
2022-09-26 12:24 ` syzbot
2022-09-26 15:09 ` [PATCH] wext: use flex array destination for memcpy() Hawkins Jiawei
2022-09-26 16:14 ` Eric Dumazet
2022-09-26 23:17 ` Hawkins Jiawei
2022-09-26 17:43 ` Kees Cook
2022-09-26 23:34 ` [PATCH wireless-next v2] " Hawkins Jiawei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).