[PATCH net] r8152: check the informaton of the device

Subsystems: networking drivers, the rest, usb networking drivers

STALE1920d

13 messages, 4 authors, 2021-05-24 · open the first message on its own page

[PATCH net] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-21 09:08:33

PLA MCU clock speed down could only be enabled when tx/rx are disabled.
Otherwise, the packet loss may occur.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index bc6b2f8aaa7e..1fb85c79bd33 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -324,6 +324,7 @@
 #define MAC_CLK_SPDWN_EN	BIT(15)
 
 /* PLA_MAC_PWR_CTRL3 */
+#define PLA_MCU_SPDWN_EN	BIT(14)
 #define PKT_AVAIL_SPDWN_EN	0x0100
 #define SUSPEND_SPDWN_EN	0x0004
 #define U1U2_SPDWN_EN		0x0002

@@ -5005,6 +5006,8 @@ static void rtl8153_down(struct r8152 *tp)
 
 static void rtl8153b_up(struct r8152 *tp)
 {
+	u32 ocp_data;
+
 	if (test_bit(RTL8152_UNPLUG, &tp->flags))
 		return;

@@ -5015,17 +5018,27 @@ static void rtl8153b_up(struct r8152 *tp)
 	r8153_first_init(tp);
 	ocp_write_dword(tp, MCU_TYPE_USB, USB_RX_BUF_TH, RX_THR_B);
 
+	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data &= ~PLA_MCU_SPDWN_EN;
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
 	r8153_aldps_en(tp, true);
 	r8153b_u1u2en(tp, true);
 }
 
 static void rtl8153b_down(struct r8152 *tp)
 {
+	u32 ocp_data;
+
 	if (test_bit(RTL8152_UNPLUG, &tp->flags)) {
 		rtl_drop_queued_tx(tp);
 		return;
 	}
 
+	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data |= PLA_MCU_SPDWN_EN;
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
 	r8153b_u1u2en(tp, false);
 	r8153_u2p3en(tp, false);
 	r8153b_power_cut_en(tp, false);

@@ -5521,6 +5534,10 @@ static void r8153b_init(struct r8152 *tp)
 	ocp_data |= MAC_CLK_SPDWN_EN;
 	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL2, ocp_data);
 
+	ocp_data = ocp_read_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3);
+	ocp_data &= ~PLA_MCU_SPDWN_EN;
+	ocp_write_word(tp, MCU_TYPE_PLA, PLA_MAC_PWR_CTRL3, ocp_data);
+
 	set_bit(GREEN_ETHERNET, &tp->flags);
 
 	/* rx aggregation */

-- 
2.21.0

Re: [PATCH net] r8152: check the informaton of the device

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-05-21 09:43:37

On Fri, May 21, 2021 at 05:07:34PM +0800, Hayes Wang wrote:

quoted hunk

Verify some fields of the USB descriptor to make sure the driver
could be used by the device.

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 <redacted>
---
 drivers/net/usb/r8152.c | 71 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 69 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 136ea06540ff..f348350f5da1 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c

@@ -8107,6 +8107,69 @@ 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_host_endpoint *in = NULL, *out = NULL, *intr = NULL;
+	unsigned int ep;
+
+	if (alt->desc.bNumEndpoints < 3) {
+		dev_err(&intf->dev, "Unexpected bNumEndpoints %d\n", alt->desc.bNumEndpoints);
+		return false;
+	}
+
+	for (ep = 0; ep < alt->desc.bNumEndpoints; ep++) {
+		struct usb_host_endpoint *e;
+
+		e = alt->endpoint + ep;
+
+		/* ignore endpoints which cannot transfer data */
+		if (!usb_endpoint_maxp(&e->desc))
+			continue;
+
+		switch (e->desc.bmAttributes) {
+		case USB_ENDPOINT_XFER_INT:
+			if (!usb_endpoint_dir_in(&e->desc))
+				continue;
+			if (!intr)
+				intr = e;
+			break;
+		case USB_ENDPOINT_XFER_BULK:
+			if (usb_endpoint_dir_in(&e->desc)) {
+				if (!in)
+					in = e;
+			} else if (!out) {
+				out = e;
+			}
+			break;
+		default:
+			continue;
+		}
+	}
+
+	if (!in || !out || !intr) {
+		dev_err(&intf->dev, "Miss Endpoints\n");
+		return false;
+	}
+
+	if ((in->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 1) {
+		dev_err(&intf->dev, "Invalid Rx Endpoints\n");
+		return false;
+	}
+
+	if ((out->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 2) {
+		dev_err(&intf->dev, "Invalid Tx Endpoints\n");
+		return false;
+	}
+
+	if ((intr->desc.bEndpointAddress & USB_ENDPOINT_NUMBER_MASK) != 3) {
+		dev_err(&intf->dev, "Invalid interrupt Endpoints\n");
+		return false;
+	}
+
+	return true;
+}

We have a USB core function that does all of the above for you, why not
use that instead?

Look at usb_find_common_endpoints() and
usb_find_common_endpoints_reverse() and at the very least
usb_find_bulk_in_endpoint() and related functions.  Please don't
open-code this type of logic, it's easy to get things wrong.

thanks,

greg k-h

RE: [PATCH net] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-22 03:13:58

Greg KH [off-list ref]

Sent: Friday, May 21, 2021 5:43 PM

[...]

We have a USB core function that does all of the above for you, why not
use that instead?

Look at usb_find_common_endpoints() and
usb_find_common_endpoints_reverse() and at the very least
usb_find_bulk_in_endpoint() and related functions.  Please don't
open-code this type of logic, it's easy to get things wrong.

Fine. Thanks.

Best Regards,
Hayes

[PATCH net v2] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-22 05:25:30

For RTL8153B with QFN32, disable test IO. Otherwise, it may cause
abnormal behavior for the device randomly.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

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 */

-- 
2.21.0

Re: [PATCH net v2] r8152: check the informaton of the device

From: Greg KH <gregkh@linuxfoundation.org>
Date: 2021-05-22 07:33:05

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 <redacted>
---
v2:
Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
code.

Much better, just some tiny grammer changes below:

quoted hunk

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>

Re: [PATCH net v2] r8152: check the informaton of the device

From: Johan Hovold <johan@kernel.org>
Date: 2021-05-22 08:07:09

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:

quoted

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 <redacted>
---
v2:
Use usb_find_common_endpoints() and usb_endpoint_num() to replace original
code.

Much better, just some tiny grammer changes below:

quoted

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.

quoted

+
+	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.

quoted

+		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).

quoted

+		return false;
+	}

Johan

RE: [PATCH net v2] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-24 01:49:57

Johan Hovold [off-list ref]

Sent: Saturday, May 22, 2021 4:07 PM

[...]

quoted

+	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

[PATCH net v3] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-24 06:50:46

For certain platforms, it causes USB reset periodically.

Signed-off-by: Hayes Wang <redacted>
---
 drivers/net/usb/r8152.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

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 */

-- 
2.21.0

Re: [PATCH net v2] r8152: check the informaton of the device

From: Johan Hovold <johan@kernel.org>
Date: 2021-05-24 07:44:37

On Mon, May 24, 2021 at 01:49:33AM +0000, Hayes Wang wrote:

Johan Hovold [off-list ref]

quoted

Sent: Saturday, May 22, 2021 4:07 PM

[...]

quoted

+	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

Re: [PATCH net v3] r8152: check the informaton of the device

From: Johan Hovold <johan@kernel.org>
Date: 2021-05-24 08:00:45

On Mon, May 24, 2021 at 02:49:42PM +0800, Hayes Wang wrote:

quoted hunk

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 <redacted>
---
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.

quoted hunk

 	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

RE: [PATCH net v3] r8152: check the informaton of the device

From: Hayes Wang <hidden>
Date: 2021-05-24 08:55:09

Johan Hovold [off-list ref]

Sent: Monday, May 24, 2021 4:01 PM

[...]

quoted

 	/* 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

Re: [PATCH net v3] r8152: check the informaton of the device

From: Johan Hovold <johan@kernel.org>
Date: 2021-05-24 09:13:48

On Mon, May 24, 2021 at 08:54:50AM +0000, Hayes Wang wrote:

Johan Hovold [off-list ref]

quoted

Sent: Monday, May 24, 2021 4:01 PM

[...]

quoted

 	/* 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

Re: [PATCH net v3] r8152: check the informaton of the device

From: patchwork-bot+netdevbpf@kernel.org
Date: 2021-05-24 20:20:15

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 <redacted>

[...]

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help