| Message ID | 1394712342-15778-364-Taiwan-albertk@realtek.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] r8152: check the informaton of the device | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: lee.jones@linaro.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: line length of 94 exceeds 80 columns |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
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@syzkaller.appspotmail.com > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode") > Signed-off-by: Hayes Wang <hayeswang@realtek.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; > + } > + > + 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"? > + return false; > + } > + > + if (usb_endpoint_num(in) != 1) { > + dev_err(&intf->dev, "Invalid Rx Endpoint\n"); "Invalid number of Rx endpoints" > + return false; > + } > + > + if (usb_endpoint_num(out) != 2) { > + dev_err(&intf->dev, "Invalid Tx Endpoint\n"); "Invalid number of RX endpoints" > + return false; > + } > + > + if (usb_endpoint_num(intr) != 3) { > + dev_err(&intf->dev, "Invalid interrupt Endpoint\n"); "Invalid number of interrupt endpoints" But really, this doesn't matter, all is good if you don't want to change this :) Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
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@syzkaller.appspotmail.com > > Fixes: c2198943e33b ("r8152: search the configuration of vendor mode") > > Signed-off-by: Hayes Wang <hayeswang@realtek.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
Johan Hovold <johan@kernel.org> > Sent: Saturday, May 22, 2021 4:07 PM [...] > > > + 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). The purpose of the checks is to find out the fake devices. That is, even the device supports in, out, and interrupt endpoints, it is treated as fake or malicious device, if the addresses of these endpoints are wrong. Therefore, I would keep the checks. Best Regards, Hayes
On Mon, May 24, 2021 at 01:49:33AM +0000, Hayes Wang wrote: > Johan Hovold <johan@kernel.org> > > Sent: Saturday, May 22, 2021 4:07 PM > [...] > > > > + 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). > > The purpose of the checks is to find out the fake devices. That is, even > the device supports in, out, and interrupt endpoints, it is treated as > fake or malicious device, if the addresses of these endpoints are wrong. > Therefore, I would keep the checks. Strictly, you need to check for bad input which could cause your driver to crash or malfunction. Generally you don't need to verify endpoint addresses unless the driver is hardcoding those. But since that is precisely what this particular driver is doing, these checks indeed need to stay. Johan
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; + } + + if (usb_find_common_endpoints(alt, &in, &out, &intr, NULL) < 0) { + dev_err(&intf->dev, "Miss Endpoints\n"); + return false; + } + + if (usb_endpoint_num(in) != 1) { + dev_err(&intf->dev, "Invalid Rx Endpoint\n"); + return false; + } + + if (usb_endpoint_num(out) != 2) { + dev_err(&intf->dev, "Invalid Tx Endpoint\n"); + return false; + } + + if (usb_endpoint_num(intr) != 3) { + dev_err(&intf->dev, "Invalid interrupt Endpoint\n"); + return false; + } + + return true; +} + static bool rtl_vendor_mode(struct usb_interface *intf) { struct usb_host_interface *alt = intf->cur_altsetting; @@ -8115,12 +8148,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 +8171,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 +9418,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(). It has been done in rtl_check_vendor_ok(). 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> --- 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 | 44 ++++++++++++++++++++++++++++++++++++----- 1 file changed, 39 insertions(+), 5 deletions(-)