[syzbot] WARNING in wireless_send_event

[syzbot]

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+473754...@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 syzk...@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

[Kees Cook]

On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
> From: Stephen Rothwell <s...@canb.auug.org.au>

> 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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+473754...@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.

[syzbot]

> Hi,
>
> On Sat, 24 Sept 2022 at 19:44, Hawkins Jiawei <yin3...@gmail.com> wrote:
>>
>> Hi Kees,

>> On Sat, 24 Sept 2022 at 15:26, Kees Cook <kees...@chromium.org> wrote:
>> >
>> > On Sat, Sep 24, 2022 at 03:10:34PM +0800, Hawkins Jiawei wrote:
>> > > From: Stephen Rothwell <s...@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

>> 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);

[Kees Cook]

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

[syzbot]

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

[Hawkins Jiawei]

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

[Hawkins Jiawei]

#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/00000000000070...@google.com/
Suggested-by: Kees Cook <kees...@chromium.org>
Signed-off-by: Hawkins Jiawei <yin3...@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

[syzbot]

Hello,

syzbot has tested the proposed patch and the reproducer did not trigger any issue:

Reported-and-tested-by: syzbot+473754...@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

[Hawkins Jiawei]

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+473754...@syzkaller.appspotmail.com

Link: https://lore.kernel.org/all/00000000000070...@google.com/
Suggested-by: Kees Cook <kees...@chromium.org>
Signed-off-by: Hawkins Jiawei <yin3...@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);

[Eric Dumazet]

On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin3...@gmail.com> wrote:
>
> Syzkaller reports refcount bug as follows:

This has nothing to do with a refcount bug...

[Kees Cook]

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"

Thanks for spinning this and getting it tested!

Reviewed-by: Kees Cook <kees...@chromium.org>

--
Kees Cook

[Hawkins Jiawei]

On Tue, 27 Sept 2022 at 00:14, Eric Dumazet <edum...@google.com> wrote:
>
> On Mon, Sep 26, 2022 at 8:09 AM Hawkins Jiawei <yin3...@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.

[Hawkins Jiawei]

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+473754...@syzkaller.appspotmail.com
Link: https://lore.kernel.org/all/00000000000070...@google.com/
Suggested-by: Kees Cook <kees...@chromium.org>

Reviewed-by: Kees Cook <kees...@chromium.org>

Signed-off-by: Hawkins Jiawei <yin3...@gmail.com>
---

v2: correct the typo error pointed out by Eric Dumazet and
Kees Cook

v1: https://lore.kernel.org/all/20220926150907....@gmail.com/