On Mon, May 24, 2021 at 02:49:42PM +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(). > usb_find_common_endpoints() includes it. > > 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> > --- > v3: > Remove the check of endpoint number in rtl_check_vendor_ok(). > > Adjust the error message and ccommit message. > > v2: > Use usb_find_common_endpoints() and usb_endpoint_num() to replace original > code. > > remove the check of endpoint number in rtl8152_probe(). It has been done > in rtl_check_vendor_ok(). > > drivers/net/usb/r8152.c | 42 ++++++++++++++++++++++++++++++++++++----- > 1 file changed, 37 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c > index 136ea06540ff..f6abb2fbf972 100644 > --- a/drivers/net/usb/r8152.c > +++ b/drivers/net/usb/r8152.c > @@ -8107,6 +8107,37 @@ 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 (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) { > + dev_err(&intf->dev, "Expected endpoints are not found\n"); > + return false; > + } > + > + /* Check Rx endpoint address */ > + if (usb_endpoint_num(in) != 1) { > + dev_err(&intf->dev, "Invalid Rx endpoint address\n"); > + return false; > + } > + > + /* Check Tx endpoint address */ > + if (usb_endpoint_num(out) != 2) { > + dev_err(&intf->dev, "Invalid Tx endpoint address\n"); > + return false; > + } > + > + /* Check interrupt endpoint address */ > + if (usb_endpoint_num(intr) != 3) { > + dev_err(&intf->dev, "Invalid interrupt endpoint address\n"); > + return false; > + } > + > + return true; > +} > + > static bool rtl_vendor_mode(struct usb_interface *intf) > { > struct usb_host_interface *alt = intf->cur_altsetting; > @@ -8115,12 +8146,15 @@ static bool rtl_vendor_mode(struct usb_interface *intf) > int i, num_configs; > > if (alt->desc.bInterfaceClass == USB_CLASS_VENDOR_SPEC) > - return true; > + return rtl_check_vendor_ok(intf); > > /* The vendor mode is not always config #1, so to find it out. */ > udev = interface_to_usbdev(intf); > c = udev->config; > num_configs = udev->descriptor.bNumConfigurations; > + if (num_configs < 2) > + return false; > + Nit: This check looks unnecessary also as the driver can handle a single configuration just fine, and by removing it you'd be logging "Unexpected Device\n" below also in the single config case. > for (i = 0; i < num_configs; (i++, c++)) { > struct usb_interface_descriptor *desc = NULL; > > @@ -8135,7 +8169,8 @@ static bool rtl_vendor_mode(struct usb_interface *intf) > } > } > > - WARN_ON_ONCE(i == num_configs); > + if (i == num_configs) > + dev_err(&intf->dev, "Unexpected Device\n"); > > return false; > } > @@ -9381,9 +9416,6 @@ static int rtl8152_probe(struct usb_interface *intf, > if (!rtl_vendor_mode(intf)) > return -ENODEV; > > - if (intf->cur_altsetting->desc.bNumEndpoints < 3) > - return -ENODEV; > - > usb_reset_device(udev); > netdev = alloc_etherdev(sizeof(struct r8152)); > if (!netdev) { Other than that, looks good to me now. Reviewed-by: Johan Hovold <johan@xxxxxxxxxx> Johan
