From: Greg Kroah-Hartman <firstname.lastname@example.org>
To: Dmitry Vyukov <email@example.com>
Cc: netdev <firstname.lastname@example.org>,
USB list <email@example.com>,
Jakub Kicinski <firstname.lastname@example.org>,
David Miller <email@example.com>,
Himadri Pandya <firstname.lastname@example.org>
Subject: Re: [Linux-kernel-mentees] [PATCH] net: usb: Fix uninit-was-stored issue in asix_read_cmd()
Date: Sun, 23 Aug 2020 12:58:08 +0200 [thread overview]
Message-ID: <20200823105808.GB87391@kroah.com> (raw)
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
> <email@example.com> 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
> > > <firstname.lastname@example.org> 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: email@example.com
> > > >
> > > > Signed-off-by: Himadri Pandya <firstname.lastname@example.org>
> > > > ---
> > > > 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
Linux-kernel-mentees mailing list
next prev parent reply other threads:[~2020-08-23 10:57 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-23 8:20 [Linux-kernel-mentees] [PATCH] net: usb: Fix uninit-was-stored issue in asix_read_cmd() Himadri Pandya
2020-08-23 9:26 ` Dmitry Vyukov via Linux-kernel-mentees
2020-08-23 10:19 ` Greg Kroah-Hartman
2020-08-23 10:31 ` Dmitry Vyukov via Linux-kernel-mentees
2020-08-23 10:56 ` Greg Kroah-Hartman
2020-08-23 10:58 ` Greg Kroah-Hartman [this message]
2020-08-23 12:38 ` Himadri Pandya
2020-08-24 8:55 ` Dmitry Vyukov via Linux-kernel-mentees
2020-08-25 6:51 ` Greg Kroah-Hartman
2020-08-25 6:54 ` Greg Kroah-Hartman
2020-08-25 14:39 ` Alan Stern
2020-08-25 14:44 ` Greg Kroah-Hartman
2020-08-25 15:44 ` Greg Kroah-Hartman
2020-08-24 18:16 ` Jakub Kicinski
2020-08-25 6:36 ` Himadri Pandya
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).