| Message ID | 1394712342-15778-364-Taiwan-albertk@realtek.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d7f1b59655efb5a285d227c8f9853a98eab5c2fd |
| Headers | show |
| Series | r8152: serial fixes | expand |
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 1fb85c79bd33..7efeddad1fc8 100644 --- a/drivers/net/usb/r8152.c +++ b/drivers/net/usb/r8152.c @@ -312,6 +312,7 @@ /* PLA_PHY_PWR */ #define TX_10M_IDLE_EN 0x0080 #define PFM_PWM_SWITCH 0x0040 +#define TEST_IO_OFF BIT(4) /* PLA_MAC_PWR_CTRL */ #define D3_CLK_GATED_EN 0x00004000 @@ -5538,6 +5539,15 @@ static void r8153b_init(struct r8152 *tp) ocp_data &= ~PLA_MCU_SPDWN_EN; ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data); + if (tp->version == RTL_VER_09) { + /* Disable Test IO for 32QFN */ + if (ocp_read_byte(tp, MCU_TYPE_PLA, 0xdc00) & BIT(5)) { + ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR); + ocp_data |= TEST_IO_OFF; + ocp_write_word(tp, MCU_TYPE_PLA, PLA_PHY_PWR, ocp_data); + } + } + set_bit(GREEN_ETHERNET, &tp->flags); /* rx aggregation */
For RTL8153B with QFN32, disable test IO. Otherwise, it may cause abnormal behavior for the device randomly. Signed-off-by: Hayes Wang <hayeswang@realtek.com> --- drivers/net/usb/r8152.c | 10 ++++++++++ 1 file changed, 10 insertions(+)