Hello,
syzbot found the following crash on:
HEAD commit: 96c6c319 net: kasan: kmsan: support CONFIG_GENERIC_CSUM on..
git tree: https://github.com/google/kmsan.git master
console output: https://syzkaller.appspot.com/x/log.txt?x=11f103bce00000
kernel config: https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
dashboard link: https://syzkaller.appspot.com/bug?extid=0631d878823ce2411636
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10dd9774e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13651a24e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: [email protected]
=====================================================
BUG: KMSAN: uninit-value in cdc_ncm_set_dgram_size+0x6ba/0xbc0
drivers/net/usb/cdc_ncm.c:587
CPU: 0 PID: 11865 Comm: kworker/0:3 Not tainted 5.4.0-rc5+ #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x191/0x1f0 lib/dump_stack.c:113
kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
__msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
cdc_ncm_set_dgram_size+0x6ba/0xbc0 drivers/net/usb/cdc_ncm.c:587
cdc_ncm_setup drivers/net/usb/cdc_ncm.c:673 [inline]
cdc_ncm_bind_common+0x2b54/0x3c50 drivers/net/usb/cdc_ncm.c:928
cdc_ncm_bind+0x2de/0x330 drivers/net/usb/cdc_ncm.c:1042
usbnet_probe+0x10d3/0x39d0 drivers/net/usb/usbnet.c:1730
usb_probe_interface+0xd19/0x1310 drivers/usb/core/driver.c:361
really_probe+0xd91/0x1f90 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:721
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:828
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:430
__device_attach+0x489/0x750 drivers/base/dd.c:894
device_initial_probe+0x4a/0x60 drivers/base/dd.c:941
bus_probe_device+0x131/0x390 drivers/base/bus.c:490
device_add+0x25b5/0x2df0 drivers/base/core.c:2202
usb_set_configuration+0x309f/0x3710 drivers/usb/core/message.c:2027
generic_probe+0xe7/0x280 drivers/usb/core/generic.c:210
usb_probe_device+0x146/0x200 drivers/usb/core/driver.c:266
really_probe+0xd91/0x1f90 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:721
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:828
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:430
__device_attach+0x489/0x750 drivers/base/dd.c:894
device_initial_probe+0x4a/0x60 drivers/base/dd.c:941
bus_probe_device+0x131/0x390 drivers/base/bus.c:490
device_add+0x25b5/0x2df0 drivers/base/core.c:2202
usb_new_device+0x23e5/0x2fb0 drivers/usb/core/hub.c:2536
hub_port_connect drivers/usb/core/hub.c:5098 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
port_event drivers/usb/core/hub.c:5359 [inline]
hub_event+0x581d/0x72f0 drivers/usb/core/hub.c:5441
process_one_work+0x1572/0x1ef0 kernel/workqueue.c:2269
process_scheduled_works kernel/workqueue.c:2331 [inline]
worker_thread+0x189c/0x2460 kernel/workqueue.c:2417
kthread+0x4b5/0x4f0 kernel/kthread.c:256
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Local variable description: ----max_datagram_size@cdc_ncm_set_dgram_size
Variable was created at:
cdc_ncm_set_dgram_size+0xf5/0xbc0 drivers/net/usb/cdc_ncm.c:564
cdc_ncm_set_dgram_size+0xf5/0xbc0 drivers/net/usb/cdc_ncm.c:564
=====================================================
---
This bug 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 [email protected].
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
syzbot <[email protected]> writes:
> syzbot found the following crash on:
>
> HEAD commit: 96c6c319 net: kasan: kmsan: support CONFIG_GENERIC_CSUM on..
> git tree: https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=11f103bce00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
> dashboard link: https://syzkaller.appspot.com/bug?extid=0631d878823ce2411636
> compiler: clang version 9.0.0 (/home/glider/llvm/clang
> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10dd9774e00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13651a24e00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: [email protected]
>
> =====================================================
> BUG: KMSAN: uninit-value in cdc_ncm_set_dgram_size+0x6ba/0xbc0
> drivers/net/usb/cdc_ncm.c:587
> CPU: 0 PID: 11865 Comm: kworker/0:3 Not tainted 5.4.0-rc5+ #0
> Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x191/0x1f0 lib/dump_stack.c:113
> kmsan_report+0x128/0x220 mm/kmsan/kmsan_report.c:108
> __msan_warning+0x73/0xe0 mm/kmsan/kmsan_instr.c:245
> cdc_ncm_set_dgram_size+0x6ba/0xbc0 drivers/net/usb/cdc_ncm.c:587
..
> Variable was created at:
> cdc_ncm_set_dgram_size+0xf5/0xbc0 drivers/net/usb/cdc_ncm.c:564
> cdc_ncm_set_dgram_size+0xf5/0xbc0 drivers/net/usb/cdc_ncm.c:564
> =====================================================
This looks like a false positive to me. max_datagram_size is two bytes
declared as
__le16 max_datagram_size;
and the code leading up to the access on drivers/net/usb/cdc_ncm.c:587
is:
/* read current mtu value from device */
err = usbnet_read_cmd(dev, USB_CDC_GET_MAX_DATAGRAM_SIZE,
USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
0, iface_no, &max_datagram_size, 2);
if (err < 0) {
dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
goto out;
}
if (le16_to_cpu(max_datagram_size) == ctx->max_datagram_size)
AFAICS, there is no way max_datagram_size can be uninitialized here.
usbnet_read_cmd() either read 2 bytes into it or returned an error,
causing the access to be skipped. Or am I missing something?
I tried to read the syzbot manual to figure out how to tell this to the
bot, but couldn't find that either. Not my day today it seems ;-)
Please let me know what to do about this.
Bjørn
Am Mittwoch, den 30.10.2019, 12:22 -0700 schrieb syzbot:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 96c6c319 net: kasan: kmsan: support CONFIG_GENERIC_CSUM on..
> git tree: https://github.com/google/kmsan.git master
> console output: https://syzkaller.appspot.com/x/log.txt?x=11f103bce00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
> dashboard link: https://syzkaller.appspot.com/bug?extid=0631d878823ce2411636
> compiler: clang version 9.0.0 (/home/glider/llvm/clang
> 80fee25776c2fb61e74c1ecb1a523375c2500b69)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10dd9774e00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13651a24e00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: [email protected]
#syz test: https://github.com/google/kmsan.git 96c6c319
Am Montag, den 04.11.2019, 22:22 +0100 schrieb Bjørn Mork:
> This looks like a false positive to me. max_datagram_size is two bytes
> declared as
>
> __le16 max_datagram_size;
>
> and the code leading up to the access on drivers/net/usb/cdc_ncm.c:587
> is:
>
> /* read current mtu value from device */
> err = usbnet_read_cmd(dev, USB_CDC_GET_MAX_DATAGRAM_SIZE,
> USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
> 0, iface_no, &max_datagram_size, 2);
At this point err can be 1.
> if (err < 0) {
> dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
> goto out;
> }
>
> if (le16_to_cpu(max_datagram_size) == ctx->max_datagram_size)
>
>
>
> AFAICS, there is no way max_datagram_size can be uninitialized here.
> usbnet_read_cmd() either read 2 bytes into it or returned an error,
No. usbnet_read_cmd() will return the number of bytes transfered up
to the number requested or an error.
> causing the access to be skipped. Or am I missing something?
Yes. You can get half the MTU. We have a similar class of bugs
with MAC addresses.
Regards
Oliver
Oliver Neukum <[email protected]> writes:
> Am Montag, den 04.11.2019, 22:22 +0100 schrieb Bjørn Mork:
>> This looks like a false positive to me. max_datagram_size is two bytes
>> declared as
>>
>> __le16 max_datagram_size;
>>
>> and the code leading up to the access on drivers/net/usb/cdc_ncm.c:587
>> is:
>>
>> /* read current mtu value from device */
>> err = usbnet_read_cmd(dev, USB_CDC_GET_MAX_DATAGRAM_SIZE,
>> USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
>> 0, iface_no, &max_datagram_size, 2);
>
> At this point err can be 1.
>
>> if (err < 0) {
>> dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
>> goto out;
>> }
>>
>> if (le16_to_cpu(max_datagram_size) == ctx->max_datagram_size)
>>
>>
>>
>> AFAICS, there is no way max_datagram_size can be uninitialized here.
>> usbnet_read_cmd() either read 2 bytes into it or returned an error,
>
> No. usbnet_read_cmd() will return the number of bytes transfered up
> to the number requested or an error.
Ah, OK. So that could be fixed with e.g.
if (err < 2)
goto out;
Or would it be better to add a strict length checking variant of this
API? There are probably lots of similar cases where we expect a
multibyte value and a short read is (or should be) considered an error.
I can't imagine any situation where we want a 2, 4, 6 or 8 byte value
and expect a flexible length returned.
>> causing the access to be skipped. Or am I missing something?
>
> Yes. You can get half the MTU. We have a similar class of bugs
> with MAC addresses.
Right. And probably all 16 or 32 bit integer reads...
Looking at the NCM spec, I see that the wording is annoyingly flexible
wrt length - both ways. E.g for GetNetAddress:
To get the entire network address, the host should set wLength to at
least 6. The function shall never return more than 6 bytes in response
to this command.
Maybe the correct fix is simply to let usbnet_read_cmd() initialize the
full buffer regardless of what the device returns? I.e.
diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
index dde05e2fdc3e..df3efafca450 100644
--- a/drivers/net/usb/usbnet.c
+++ b/drivers/net/usb/usbnet.c
@@ -1982,7 +1982,7 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
cmd, reqtype, value, index, size);
if (size) {
- buf = kmalloc(size, GFP_KERNEL);
+ buf = kzalloc(size, GFP_KERNEL);
if (!buf)
goto out;
}
@@ -1992,7 +1992,7 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
USB_CTRL_GET_TIMEOUT);
if (err > 0 && err <= size) {
if (data)
- memcpy(data, buf, err);
+ memcpy(data, buf, size);
else
netdev_dbg(dev->net,
"Huh? Data requested but thrown away.\n");
What do you think?
Personally, I don't think it makes sense for a device to return a 1-byte
mtu or 3-byte mac address. But the spec allows it and this would at
least make it safe.
We have a couple of similar bugs elsewhere in the same driver, BTW..
Bjørn
Hello,
syzbot has tested the proposed patch and the reproducer did not trigger
crash:
Reported-and-tested-by:
[email protected]
Tested on:
commit: 96c6c319 net: kasan: kmsan: support CONFIG_GENERIC_CSUM on..
git tree: https://github.com/google/kmsan.git
kernel config: https://syzkaller.appspot.com/x/.config?x=9e324dfe9c7b0360
dashboard link: https://syzkaller.appspot.com/bug?extid=0631d878823ce2411636
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
patch: https://syzkaller.appspot.com/x/patch.diff?x=16b798dce00000
Note: testing is done by a robot and is best-effort only.
Am Dienstag, den 05.11.2019, 13:25 +0100 schrieb Bjørn Mork:
> Oliver Neukum <[email protected]> writes:
> > Am Montag, den 04.11.2019, 22:22 +0100 schrieb Bjørn Mork:
> Ah, OK. So that could be fixed with e.g.
>
> if (err < 2)
> goto out;
>
>
> Or would it be better to add a strict length checking variant of this
> API? There are probably lots of similar cases where we expect a
We would lose flexibilty and the check needs to be there anyway.
> Right. And probably all 16 or 32 bit integer reads...
>
> Looking at the NCM spec, I see that the wording is annoyingly flexible
> wrt length - both ways. E.g for GetNetAddress:
>
> To get the entire network address, the host should set wLength to at
> least 6. The function shall never return more than 6 bytes in response
> to this command.
>
> Maybe the correct fix is simply to let usbnet_read_cmd() initialize the
> full buffer regardless of what the device returns? I.e.
This issue has never been observed in the wild. We are defending
against a possible attack. It is better to react drastically.
> at do you think?
>
> Personally, I don't think it makes sense for a device to return a 1-byte
> mtu or 3-byte mac address. But the spec allows it and this would at
> least make it safe.
Hence we should ignore such a reply. The support is optional anyway.
For usbnet as such, however, we cannot really hardcode the size of
a MAC.
Regards
Oliver
+ Greg K-H
On Tue, Nov 5, 2019 at 1:25 PM Bjørn Mork <[email protected]> wrote:
>
> Oliver Neukum <[email protected]> writes:
> > Am Montag, den 04.11.2019, 22:22 +0100 schrieb Bjørn Mork:
> >> This looks like a false positive to me. max_datagram_size is two bytes
> >> declared as
> >>
> >> __le16 max_datagram_size;
> >>
> >> and the code leading up to the access on drivers/net/usb/cdc_ncm.c:587
> >> is:
> >>
> >> /* read current mtu value from device */
> >> err = usbnet_read_cmd(dev, USB_CDC_GET_MAX_DATAGRAM_SIZE,
> >> USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
> >> 0, iface_no, &max_datagram_size, 2);
> >
> > At this point err can be 1.
> >
> >> if (err < 0) {
> >> dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
> >> goto out;
> >> }
> >>
> >> if (le16_to_cpu(max_datagram_size) == ctx->max_datagram_size)
> >>
> >>
> >>
> >> AFAICS, there is no way max_datagram_size can be uninitialized here.
> >> usbnet_read_cmd() either read 2 bytes into it or returned an error,
> >
> > No. usbnet_read_cmd() will return the number of bytes transfered up
> > to the number requested or an error.
>
> Ah, OK. So that could be fixed with e.g.
>
> if (err < 2)
> goto out;
It'd better be (err < sizeof(max_datagram_size)), and probably in the
call to usbnet_read_cmd() as well.
>
> Or would it be better to add a strict length checking variant of this
> API? There are probably lots of similar cases where we expect a
> multibyte value and a short read is (or should be) considered an error.
> I can't imagine any situation where we want a 2, 4, 6 or 8 byte value
> and expect a flexible length returned.
This is really a widespread problem on syzbot: a lot of USB devices
use similar code calling usb_control_msg() to read from the device and
not checking that the buffer is fully initialized.
Greg, do you know how often usb_control_msg() is expected to read less
than |size| bytes? Is it viable to make it return an error if this
happens?
Almost nobody is using this function correctly (i.e. checking that it
has read the whole buffer before accessing it).
> >> causing the access to be skipped. Or am I missing something?
> >
> > Yes. You can get half the MTU. We have a similar class of bugs
> > with MAC addresses.
>
> Right. And probably all 16 or 32 bit integer reads...
>
> Looking at the NCM spec, I see that the wording is annoyingly flexible
> wrt length - both ways. E.g for GetNetAddress:
>
> To get the entire network address, the host should set wLength to at
> least 6. The function shall never return more than 6 bytes in response
> to this command.
>
> Maybe the correct fix is simply to let usbnet_read_cmd() initialize the
> full buffer regardless of what the device returns? I.e.
>
> diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c
> index dde05e2fdc3e..df3efafca450 100644
> --- a/drivers/net/usb/usbnet.c
> +++ b/drivers/net/usb/usbnet.c
> @@ -1982,7 +1982,7 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
> cmd, reqtype, value, index, size);
>
> if (size) {
> - buf = kmalloc(size, GFP_KERNEL);
> + buf = kzalloc(size, GFP_KERNEL);
> if (!buf)
> goto out;
> }
> @@ -1992,7 +1992,7 @@ static int __usbnet_read_cmd(struct usbnet *dev, u8 cmd, u8 reqtype,
> USB_CTRL_GET_TIMEOUT);
> if (err > 0 && err <= size) {
> if (data)
> - memcpy(data, buf, err);
> + memcpy(data, buf, size);
> else
> netdev_dbg(dev->net,
> "Huh? Data requested but thrown away.\n");
>
>
>
>
> What do you think?
>
> Personally, I don't think it makes sense for a device to return a 1-byte
> mtu or 3-byte mac address. But the spec allows it and this would at
> least make it safe.
>
> We have a couple of similar bugs elsewhere in the same driver, BTW..
>
>
> Bjørn
--
Alexander Potapenko
Software Engineer
Google Germany GmbH
Erika-Mann-Straße, 33
80636 München
Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg
On Tue, Nov 05, 2019 at 02:55:09PM +0100, Alexander Potapenko wrote:
> + Greg K-H
> On Tue, Nov 5, 2019 at 1:25 PM Bj?rn Mork <[email protected]> wrote:
> >
> > Oliver Neukum <[email protected]> writes:
> > > Am Montag, den 04.11.2019, 22:22 +0100 schrieb Bj?rn Mork:
> > >> This looks like a false positive to me. max_datagram_size is two bytes
> > >> declared as
> > >>
> > >> __le16 max_datagram_size;
> > >>
> > >> and the code leading up to the access on drivers/net/usb/cdc_ncm.c:587
> > >> is:
> > >>
> > >> /* read current mtu value from device */
> > >> err = usbnet_read_cmd(dev, USB_CDC_GET_MAX_DATAGRAM_SIZE,
> > >> USB_TYPE_CLASS | USB_DIR_IN | USB_RECIP_INTERFACE,
> > >> 0, iface_no, &max_datagram_size, 2);
> > >
> > > At this point err can be 1.
> > >
> > >> if (err < 0) {
> > >> dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
> > >> goto out;
> > >> }
> > >>
> > >> if (le16_to_cpu(max_datagram_size) == ctx->max_datagram_size)
> > >>
> > >>
> > >>
> > >> AFAICS, there is no way max_datagram_size can be uninitialized here.
> > >> usbnet_read_cmd() either read 2 bytes into it or returned an error,
> > >
> > > No. usbnet_read_cmd() will return the number of bytes transfered up
> > > to the number requested or an error.
> >
> > Ah, OK. So that could be fixed with e.g.
> >
> > if (err < 2)
> > goto out;
> It'd better be (err < sizeof(max_datagram_size)), and probably in the
> call to usbnet_read_cmd() as well.
> >
> > Or would it be better to add a strict length checking variant of this
> > API? There are probably lots of similar cases where we expect a
> > multibyte value and a short read is (or should be) considered an error.
> > I can't imagine any situation where we want a 2, 4, 6 or 8 byte value
> > and expect a flexible length returned.
> This is really a widespread problem on syzbot: a lot of USB devices
> use similar code calling usb_control_msg() to read from the device and
> not checking that the buffer is fully initialized.
>
> Greg, do you know how often usb_control_msg() is expected to read less
> than |size| bytes? Is it viable to make it return an error if this
> happens?
> Almost nobody is using this function correctly (i.e. checking that it
> has read the whole buffer before accessing it).
It could return less than size if the endpoint size isn't the same as
the message. I think. It's been a long time since I've read the USB
spec in that area, but usually if the size is "short" then status should
also be set saying something went wrong, right? Ah wait, you are
playing the "malicious device" card, I think we always just need to
check the thing :(
sorry,
greg k-h