| Message ID | 1394712342-15778-365-Taiwan-albertk@realtek.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a0246dafe684a6d5ad31ccd59af0334ccf0cc7b2 |
| Headers | show |
| Series | r8152: serial fixes | 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 7efeddad1fc8..b1a00f29455b 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -3391,7 +3391,8 @@ static void rtl8153b_runtime_enable(struct r8152 *tp, bool enable) r8153b_ups_en(tp, false); r8153_queue_wake(tp, false); rtl_runtime_suspend_enable(tp, false); - r8153b_u1u2en(tp, true); + if (tp->udev->speed != USB_SPEED_HIGH) + r8153b_u1u2en(tp, true); } } @@ -5024,7 +5025,9 @@ static void rtl8153b_up(struct r8152 *tp) ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data); r8153_aldps_en(tp, true); - r8153b_u1u2en(tp, true); + + if (tp->udev->speed != USB_SPEED_HIGH) + r8153b_u1u2en(tp, true); } static void rtl8153b_down(struct r8152 *tp) @@ -5527,7 +5530,9 @@ static void r8153b_init(struct r8152 *tp) ocp_data &= ~CUR_LINK_OK; ocp_data |= POLL_LINK_CHG; ocp_write_word(tp, MCU_TYPE_PLA, PLA_EXTRA_STATUS, ocp_data); - r8153b_u1u2en(tp, true); + + if (tp->udev->speed != USB_SPEED_HIGH) + r8153b_u1u2en(tp, true); usb_enable_lpm(tp->udev); /* MAC clock speed down */
For certain platforms, it causes USB reset periodically. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/usb/r8152.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)