lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YKi7qEWobOLRyoU8@hovoldconsulting.com>
Date:   Sat, 22 May 2021 10:07:04 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Hayes Wang <hayeswang@...ltek.com>, kuba@...nel.org,
        davem@...emloft.net, netdev@...r.kernel.org, nic_swsd@...ltek.com,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        syzbot+95afd23673f5dd295c57@...kaller.appspotmail.com
Subject: Re: [PATCH net v2] r8152: check the informaton of the device

On Sat, May 22, 2021 at 09:32:58AM +0200, Greg Kroah-Hartman wrote:
> On Sat, May 22, 2021 at 01:24:54PM +0800, Hayes Wang wrote:
> > Verify some fields of the USB descriptor to make sure the driver
> > could be used by the device.
> > 
> > Besides, remove the check of endpoint number in rtl8152_probe().
> > It has been done in rtl_check_vendor_ok().
> > 
> > BugLink: https://syzkaller.appspot.com/bug?id=912c9c373656996801b4de61f1e3cb326fe940aa
> > Reported-by: syzbot+95afd23673f5dd295c57@...kaller.appspotmail.com
> > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> > Signed-off-by: Hayes Wang <hayeswang@...ltek.com>
> > ---
> > v2:
> > Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
> > code.
> 
> Much better, just some tiny grammer changes below:
> 
> > 
> > remove the check of endpoint number in rtl8152_probe(). It has been done
> > in rtl_check_vendor_ok().
> > 
> >  drivers/net/usb/r8152.c | 44 ++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 39 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> > index 136ea06540ff..6e5230d6c721 100644
> > --- a/drivers/net/usb/r8152.c
> > +++ b/drivers/net/usb/r8152.c
> > @@ -8107,6 +8107,39 @@ static void r8156b_init(struct r8152 *tp)
> >  	tp->coalesce = 15000;	/* 15 us */
> >  }
> >  
> > +static bool rtl_check_vendor_ok(struct usb_interface *intf)
> > +{
> > +	struct usb_host_interface *alt = intf->cur_altsetting;
> > +	struct usb_endpoint_descriptor *in, *out, *intr;
> > +
> > +	if (alt->desc.bNumEndpoints < 3) {
> > +		dev_err(&intf->dev, "Unexpected bNumEndpoints %d\n", alt->desc.bNumEndpoints);
> > +		return false;
> > +	}

This check is now redundant and can be removed.

> > +
> > +	if (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) {
> > +		dev_err(&intf->dev, "Miss Endpoints\n");
> 
> "Miss" feels ackward, how about "Invalid number of endpoints"?

The helper also checks the type and direction so perhaps something like
"expected endpoints not found" (or just "missing endpoints") which is
more precise.

> > +		return false;
> > +	}
> > +
> > +	if (usb_endpoint_num(in) != 1) {
> > +		dev_err(&intf->dev, "Invalid Rx Endpoint\n");
> 
> "Invalid number of Rx endpoints"

Here it is the endpoint number (address) that is being checked so
"number of" would be wrong.

That said, perhaps none of these checks are even needed a bit depending
on how the driver is implemented. That is, if it hardcodes the endpoint
addresses or uses the result from usb_find_common_endpoints() above
(which I realise now that it does not so these checks are probably still
needed).

> > +		return false;
> > +	}

Johan

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ