Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* KMSAN: uninit-value in cdc_ncm_set_dgram_size
@ 2019-10-30 19:22 syzbot
  2019-11-04 21:22 ` Bjørn Mork
  2019-11-05 11:11 ` Oliver Neukum
  0 siblings, 2 replies; 9+ messages in thread
From: syzbot @ 2019-10-30 19:22 UTC (permalink / raw)
  To: davem, glider, linux-kernel, linux-usb, netdev, oliver, syzkaller-bugs

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+0631d878823ce2411636@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 syzkaller@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-10-30 19:22 KMSAN: uninit-value in cdc_ncm_set_dgram_size syzbot
@ 2019-11-04 21:22 ` Bjørn Mork
  2019-11-05 11:15   ` Oliver Neukum
  2019-11-05 11:11 ` Oliver Neukum
  1 sibling, 1 reply; 9+ messages in thread
From: Bjørn Mork @ 2019-11-04 21:22 UTC (permalink / raw)
  To: syzbot
  Cc: davem, glider, linux-kernel, linux-usb, netdev, oliver, syzkaller-bugs

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-10-30 19:22 KMSAN: uninit-value in cdc_ncm_set_dgram_size syzbot
  2019-11-04 21:22 ` Bjørn Mork
@ 2019-11-05 11:11 ` Oliver Neukum
  2019-11-05 12:51   ` syzbot
  1 sibling, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-11-05 11:11 UTC (permalink / raw)
  To: syzbot, davem, glider, linux-kernel, linux-usb, netdev, syzkaller-bugs

[-- Attachment #1: Type: text/plain, Size: 948 bytes --]

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+0631d878823ce2411636@syzkaller.appspotmail.com
#syz test: https://github.com/google/kmsan.git 96c6c319

[-- Attachment #2: 0001-CDC-NCM-handle-incomplete-transfer-of-MTU.patch --]
[-- Type: text/x-patch, Size: 1146 bytes --]

From 090ac0305bb47da9336c0188e0e59e50ff2243c3 Mon Sep 17 00:00:00 2001
From: Oliver Neukum <oneukum@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.

Reported-by: syzbot+0631d878823ce2411636@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 00cab3f43a4c..939487a5f4bc 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -579,7 +579,7 @@ static void cdc_ncm_set_dgram_size(struct usbnet *dev, int new_size)
 	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) {
+	if (err < 2) {
 		dev_dbg(&dev->intf->dev, "GET_MAX_DATAGRAM_SIZE failed\n");
 		goto out;
 	}
-- 
2.16.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-04 21:22 ` Bjørn Mork
@ 2019-11-05 11:15   ` Oliver Neukum
  2019-11-05 12:25     ` Bjørn Mork
  0 siblings, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2019-11-05 11:15 UTC (permalink / raw)
  To: Bjørn Mork, syzbot
  Cc: davem, glider, linux-kernel, linux-usb, netdev, syzkaller-bugs

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-05 11:15   ` Oliver Neukum
@ 2019-11-05 12:25     ` Bjørn Mork
  2019-11-05 13:51       ` Oliver Neukum
  2019-11-05 13:55       ` Alexander Potapenko
  0 siblings, 2 replies; 9+ messages in thread
From: Bjørn Mork @ 2019-11-05 12:25 UTC (permalink / raw)
  To: Oliver Neukum
  Cc: syzbot, davem, glider, linux-kernel, linux-usb, netdev, syzkaller-bugs

Oliver Neukum <oneukum@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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-05 11:11 ` Oliver Neukum
@ 2019-11-05 12:51   ` syzbot
  0 siblings, 0 replies; 9+ messages in thread
From: syzbot @ 2019-11-05 12:51 UTC (permalink / raw)
  To: davem, glider, linux-kernel, linux-usb, netdev, oneukum, syzkaller-bugs

Hello,

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

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-05 12:25     ` Bjørn Mork
@ 2019-11-05 13:51       ` Oliver Neukum
  2019-11-05 13:55       ` Alexander Potapenko
  1 sibling, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2019-11-05 13:51 UTC (permalink / raw)
  To: Bjørn Mork
  Cc: syzbot, davem, glider, linux-kernel, linux-usb, netdev, syzkaller-bugs

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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-05 12:25     ` Bjørn Mork
  2019-11-05 13:51       ` Oliver Neukum
@ 2019-11-05 13:55       ` Alexander Potapenko
  2019-11-05 15:35         ` Greg Kroah-Hartman
  1 sibling, 1 reply; 9+ messages in thread
From: Alexander Potapenko @ 2019-11-05 13:55 UTC (permalink / raw)
  To: Bjørn Mork, Greg Kroah-Hartman
  Cc: Oliver Neukum, syzbot, David Miller, LKML, USB list, Networking,
	syzkaller-bugs

+ Greg K-H
On Tue, Nov 5, 2019 at 1:25 PM Bjørn Mork <bjorn@mork.no> wrote:
>
> Oliver Neukum <oneukum@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).

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size
  2019-11-05 13:55       ` Alexander Potapenko
@ 2019-11-05 15:35         ` Greg Kroah-Hartman
  0 siblings, 0 replies; 9+ messages in thread
From: Greg Kroah-Hartman @ 2019-11-05 15:35 UTC (permalink / raw)
  To: Alexander Potapenko
  Cc: Bjørn Mork, Oliver Neukum, syzbot, David Miller, LKML,
	USB list, Networking, syzkaller-bugs

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 <bjorn@mork.no> wrote:
> >
> > Oliver Neukum <oneukum@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).

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-30 19:22 KMSAN: uninit-value in cdc_ncm_set_dgram_size syzbot
2019-11-04 21:22 ` Bjørn Mork
2019-11-05 11:15   ` Oliver Neukum
2019-11-05 12:25     ` Bjørn Mork
2019-11-05 13:51       ` Oliver Neukum
2019-11-05 13:55       ` Alexander Potapenko
2019-11-05 15:35         ` Greg Kroah-Hartman
2019-11-05 11:11 ` Oliver Neukum
2019-11-05 12:51   ` syzbot

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git