Re: [Linux-kernel-mentees] [PATCH] net: usb: Fix uninit-was-stored issue in asix_read_cmd()

[Himadri Pandya <himadrispandya@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4398 bytes --]

On Sun, 23 Aug, 2020, 4:27 pm Greg Kroah-Hartman, <
gregkh@linuxfoundation.org> wrote:

> On Sun, Aug 23, 2020 at 12:31:03PM +0200, Dmitry Vyukov wrote:
> > On Sun, Aug 23, 2020 at 12:19 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:
> > > > On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
> > > > <himadrispandya@gmail.com> wrote:
> > > > >
> > > > > Initialize the buffer before passing it to usb_read_cmd()
> function(s) to
> > > > > fix the uninit-was-stored issue in asix_read_cmd().
> > > > >
> > > > > Fixes: KMSAN: kernel-infoleak in raw_ioctl
> > > > > Reported by: syzbot+a7e220df5a81d1ab400e@syzkaller.appspotmail.com
> > > > >
> > > > > Signed-off-by: Himadri Pandya <himadrispandya@gmail.com>
> > > > > ---
> > > > >  drivers/net/usb/asix_common.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > >
> > > > > diff --git a/drivers/net/usb/asix_common.c
> b/drivers/net/usb/asix_common.c
> > > > > index e39f41efda3e..a67ea1971b78 100644
> > > > > --- a/drivers/net/usb/asix_common.c
> > > > > +++ b/drivers/net/usb/asix_common.c
> > > > > @@ -17,6 +17,8 @@ int asix_read_cmd(struct usbnet *dev, u8 cmd,
> u16 value, u16 index,
> > > > >
> > > > >         BUG_ON(!dev);
> > > > >
> > > > > +       memset(data, 0, size);
> > > >
> > > > Hi Himadri,
> > > >
> > > > I think the proper fix is to check
> > > > usbnet_read_cmd/usbnet_read_cmd_nopm return value instead.
> > > > Memsetting data helps to fix the warning at hand, but the device did
> > > > not send these 0's and we use them as if the device did send them.
> > >
> > > But, for broken/abusive devices, that really is the safest thing to do
> > > here.  They are returning something that is obviously not correct, so
> > > either all callers need to check the size received really is the size
> > > they asked for, or we just plod onward with a 0 value like this.  Or we
> > > could pick some other value, but that could cause other problems if it
> > > is treated as an actual value.
> >
> > Do we want callers to do at least some error check (e.g. device did
> > not return anything at all, broke, hang)?
> > If yes, then with a separate helper function that fails on short
> > reads, we can get both benefits at no additional cost. User code will
> > say "I want 4 bytes, anything that is not 4 bytes is an error" and
> > then 1 error check will do. In fact, it seems that that was the
> > intention of whoever wrote this code (they assumed no short reads),
> > it's just they did not actually implement that "anything that is not 4
> > bytes is an error" part.
> >
> >
> > > > Perhaps we need a separate helper function (of a bool flag) that will
> > > > fail on incomplete reads. Maybe even in the common USB layer because
> I
> > > > think we've seen this type of bug lots of times and I guess there are
> > > > dozens more.
> > >
> > > It's not always a failure, some devices have protocols that are "I
> could
> > > return up to a max X bytes but could be shorter" types of messages, so
> > > it's up to the caller to check that they got what they really asked
> for.
> >
> > Yes, that's why I said _separate_ helper function. There seems to be
> > lots of callers that want exactly this -- "I want 4 bytes, anything
> > else is an error". With the current API it's harder to do - you need
> > additional checks, additional code, maybe even additional variables to
> > store the required size. APIs should make correct code easy to write.
>
> I guess I already answered both of these in my previous email...
>
> > > Yes, it's more work to do this checking.  However converting the world
> > > over to a "give me an error value if you don't read X number of bytes"
> > > function would also be the same amount of work, right?
> >
> > Should this go into the common USB layer then?
> > It's weird to have such a special convention on the level of a single
> > driver. Why are rules for this single driver so special?...
>
> They aren't special at all, so yes, we should be checking for a short
> read everywhere.  That would be the "correct" thing to do, I was just
> suggesting a "quick fix" here, sorry.
>
> Himadri, want to fix up all callers to properly check the size of the
> message recieved before they access it?  That will fix this issue
> properly.
>
> thanks,
>
> greg k-h
>

Sure. On it.

Thanks,
Himadri

>

[-- Attachment #1.2: Type: text/html, Size: 6261 bytes --]

[-- Attachment #2: Type: text/plain, Size: 201 bytes --]

_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees