KMSAN: uninit-value in cdc_ncm_set_dgram_size

[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: syzbot+0631d8...@syzkaller.appspotmail.com

=====================================================
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 syzk...@googlegroups.com.

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

[Bjørn Mork]

..

> 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

[Oliver Neukum]

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: syzbot+0631d8...@syzkaller.appspotmail.com

#syz test: https://github.com/google/kmsan.git 96c6c319

[Oliver Neukum]

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

[Bjørn Mork]

Oliver Neukum <one...@suse.com> 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

[syzbot]

Hello,

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

Reported-and-tested-by:
syzbot+0631d8...@syzkaller.appspotmail.com

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.

[Oliver Neukum]

Am Dienstag, den 05.11.2019, 13:25 +0100 schrieb Bjørn Mork:
> Oliver Neukum <one...@suse.com> 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

[Alexander Potapenko]

+ Greg K-H

On Tue, Nov 5, 2019 at 1:25 PM Bjørn Mork <bj...@mork.no> wrote:
>
> Oliver Neukum <one...@suse.com> 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).

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

[Greg Kroah-Hartman]

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

[Oliver Neukum]

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..

#syz test: https://github.com/google/kmsan.git 96c6c319

From 23a6624cf7d8e146c35f2eec135d6c22e24f9a2e Mon Sep 17 00:00:00 2001
From: Oliver Neukum <one...@suse.com>
Date: Tue, 5 Nov 2019 12:04:44 +0100
Subject: [PATCH] CDC-NCM: handle incomplete transfer of MTU

A malicious device may give half an answer when asked
for its MTU. The driver will proceed after this with
a garbage MTU. Anything but a complete answer must be treated
as an error.

V2: used sizeof as request by Alexander

Reported-by: syzbot+0631d8...@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <one...@suse.com>
---
drivers/net/usb/cdc_ncm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 00cab3f43a4c..a245597a3902 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -578,8 +578,8 @@ static void cdc_ncm_set_dgram_size(struct usbnet *dev, int new_size)

/* 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) {
+ 0, iface_no, &max_datagram_size, sizeof(max_datagram_size));
+ if (err < sizeof(max_datagram_size)) {

dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
goto out;
}

@@ -590,7 +590,7 @@ static void cdc_ncm_set_dgram_size(struct usbnet *dev, int new_size)
max_datagram_size = cpu_to_le16(ctx->max_datagram_size);
err = usbnet_write_cmd(dev, USB_CDC_SET_MAX_DATAGRAM_SIZE,
USB_TYPE_CLASS | USB_DIR_OUT | USB_RECIP_INTERFACE,
- 0, iface_no, &max_datagram_size, 2);
+ 0, iface_no, &max_datagram_size, sizeof(max_datagram_size));
if (err < 0)
dev_dbg(&dev->intf->dev, "SET_MAX_DATAGRAM_SIZE failed\n");

--
2.16.4

[syzbot]

Hello,

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

Reported-and-tested-by:
syzbot+0631d8...@syzkaller.appspotmail.com

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=11599e8ae00000