| Message ID | 1394712342-15778-365-Taiwan-albertk@realtek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,v3] r8152: check the informaton of the device | expand |
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@syzkaller.appspotmail.com > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode") > Signed-off-by: Hayes Wang <hayeswang@realtek.com> > --- > 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@kernel.org> Johan
Johan Hovold <johan@kernel.org> > Sent: Monday, May 24, 2021 4:01 PM [...] > > /* 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. I just want to distinguish the devices. It is acceptable if the device contains only one configuration. A mistake occurs if the device has more configurations and there is no expected one. I would remove it if you think it is better. Best Regards, Hayes
On Mon, May 24, 2021 at 08:54:50AM +0000, Hayes Wang wrote: > Johan Hovold <johan@kernel.org> > > Sent: Monday, May 24, 2021 4:01 PM > [...] > > > /* 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. > > I just want to distinguish the devices. > It is acceptable if the device contains only one configuration. > A mistake occurs if the device has more configurations and > there is no expected one. > I would remove it if you think it is better. I'm fine with keeping the check too (e.g. as an optimisation of sort), it's just a bit inconsistent to not log an error in that one error path. Johan
Hello: This patch was applied to netdev/net.git (refs/heads/master): On Mon, 24 May 2021 14:49:42 +0800 you 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@syzkaller.appspotmail.com > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode") > Signed-off-by: Hayes Wang <hayeswang@realtek.com> > > [...] Here is the summary with links: - [net,v3] r8152: check the informaton of the device https://git.kernel.org/netdev/net/c/1a44fb38cc65 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
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; + 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) {
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@syzkaller.appspotmail.com Fixes: c2198943e33b ("r8152: search the configuration of vendor mode") Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- 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(-)