Re: [PATCH net v2] r8152: check the informaton of the device

[Johan Hovold]

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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode")
> > Signed-off-by: Hayes Wang <hayeswang@xxxxxxxxxxx>
> > ---
> > 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