Re: KMSAN: uninit-value in cdc_ncm_set_dgram_size

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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