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
[off-list ref] wrote:

quoted

On Sun, Aug 23, 2020 at 11:26:27AM +0200, Dmitry Vyukov wrote:

quoted

On Sun, Aug 23, 2020 at 10:21 AM Himadri Pandya
[off-list ref] wrote:

quoted

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

quoted

quoted

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

quoted

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
_______________________________________________
Linux-kernel-mentees mailing list
Linux-kernel-mentees@lists.linuxfoundation.org
https://lists.linuxfoundation.org/mailman/listinfo/linux-kernel-mentees