Skip to contentSkip to conversation

usbip: vhci_hcd: fix NULL deref and NR_HCS upper bound

PATCH In review Adrian Wowk 2 participants 5 messages Apr 14 – May 5

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

cover usbip: vhci_hcd: fix NULL deref and NR_HCS upper bound

These two patches fix a NULL pointer dereference in vhci_sysfs.c triggered when a VHCI host controller fails to probe, and tighten the Kconfig upper bound for USBIP_VHCI_NR_HCS to reflect the real maximum imposed by USB_MAXBUS.

Tested on Ubuntu 6.8.0-110-generic (6.8.12) on a Dell PowerEdge R640, issue is reproducible with CONFIG_USBIP_VHCI_NR_HCS > 32 or potentially lower if there is already other USB hardware connected.

Adrian Wowk (2):
usbip: vhci_hcd: fix NULL deref in status_show_vhci
usbip: vhci_hcd: reduce CONFIG_USBIP_VHCI_NR_HCS upper bound to 32

 drivers/usb/usbip/Kconfig      |  2 +-
 drivers/usb/usbip/vhci_sysfs.c | 52 +++++++++++++++++++---------------
 2 files changed, 30 insertions(+), 24 deletions(-)
Signature · 1 line
2.53.0
#1 lore raw

PATCH 1/2 usbip: vhci_hcd: fix NULL deref in status_show_vhci

platform_get_drvdata() can return NULL if a VHCI host controller's probe failed (e.g. due to USB bus number exhaustion). status_show_vhci() checked for a NULL pdev but not for a NULL hcd returned by platform_get_drvdata(). Passing NULL to hcd_to_vhci_hcd() does not return NULL - it returns a pointer offset of 0x260, causing a NULL pointer dereference when that value is subsequently dereferenced.

Add a NULL check on hcd before calling hcd_to_vhci_hcd(). Move status_show_not_ready() above status_show_vhci() to make it callable from the new error path without a forward declaration.

---
 drivers/usb/usbip/vhci_sysfs.c | 52 +++++++++++++++++++---------------
 1 file changed, 29 insertions(+), 23 deletions(-)
drivers/usb/usbip/vhci_sysfs.c +29 -23
index d5865460e82..336fb4d92c6 100644
@@ -59,6 +59,29 @@ static void port_show_vhci(char **out, int hub, int port, struct vhci_device *vd
 	*out += sprintf(*out, "\n");
 }
 
+static ssize_t status_show_not_ready(int pdev_nr, char *out)
+{
+	char *s = out;
+	int i = 0;
+
+	for (i = 0; i < VHCI_HC_PORTS; i++) {
+		out += sprintf(out, "hs  %04u %03u ",
+				    (pdev_nr * VHCI_PORTS) + i,
+				    VDEV_ST_NOTASSIGNED);
+		out += sprintf(out, "000 00000000 0000000000000000 0-0");
+		out += sprintf(out, "\n");
+	}
+
+	for (i = 0; i < VHCI_HC_PORTS; i++) {
+		out += sprintf(out, "ss  %04u %03u ",
+				    (pdev_nr * VHCI_PORTS) + VHCI_HC_PORTS + i,
+				    VDEV_ST_NOTASSIGNED);
+		out += sprintf(out, "000 00000000 0000000000000000 0-0");
+		out += sprintf(out, "\n");
+	}
+	return out - s;
+}
+
 /* Sysfs entry to show port status */
 static ssize_t status_show_vhci(int pdev_nr, char *out)
 {
@@ -76,6 +99,12 @@ static ssize_t status_show_vhci(int pdev_nr, char *out)
 	}
 
 	hcd = platform_get_drvdata(pdev);
+
+	if (!hcd) {
+		usbip_dbg_vhci_sysfs("show status error (hcd is NULL)\n");
+		return status_show_not_ready(pdev_nr, out);
+	}
+
 	vhci_hcd = hcd_to_vhci_hcd(hcd);
 	vhci = vhci_hcd->vhci;
 
@@ -104,29 +133,6 @@ static ssize_t status_show_vhci(int pdev_nr, char *out)
 	return out - s;
 }
 
-static ssize_t status_show_not_ready(int pdev_nr, char *out)
-{
-	char *s = out;
-	int i = 0;
-
-	for (i = 0; i < VHCI_HC_PORTS; i++) {
-		out += sprintf(out, "hs  %04u %03u ",
-				    (pdev_nr * VHCI_PORTS) + i,
-				    VDEV_ST_NOTASSIGNED);
-		out += sprintf(out, "000 00000000 0000000000000000 0-0");
-		out += sprintf(out, "\n");
-	}
-
-	for (i = 0; i < VHCI_HC_PORTS; i++) {
-		out += sprintf(out, "ss  %04u %03u ",
-				    (pdev_nr * VHCI_PORTS) + VHCI_HC_PORTS + i,
-				    VDEV_ST_NOTASSIGNED);
-		out += sprintf(out, "000 00000000 0000000000000000 0-0");
-		out += sprintf(out, "\n");
-	}
-	return out - s;
-}
-
 static int status_name_to_id(const char *name)
 {
 	char *c;
Signature · 1 line
2.53.0
maintainer Reviewed-by on v1 1/2
#2 lore raw

Re: [PATCH 1/2] usbip: vhci_hcd: fix NULL deref in status_show_vhci

Quoted 7 lines · Adrian Wowk

platform_get_drvdata() can return NULL if a VHCI host controller's probe failed (e.g. due to USB bus number exhaustion). status_show_vhci() checked for a NULL pdev but not for a NULL hcd returned by platform_get_drvdata(). Passing NULL to hcd_to_vhci_hcd() does not return NULL - it returns a pointer offset of 0x260, causing a NULL pointer dereference when that value is subsequently dereferenced.

Quoted 9 lines

Add a NULL check on hcd before calling hcd_to_vhci_hcd(). Move status_show_not_ready() above status_show_vhci() to make it callable from the new error path without a forward declaration.

---
  drivers/usb/usbip/vhci_sysfs.c | 52 +++++++++++++++++++---------------
  1 file changed, 29 insertions(+), 23 deletions(-)

Looks good to me.

Greg, Please pick this up.

thanks,
-- Shuah

#3 lore raw

PATCH 2/2 usbip: vhci_hcd: reduce CONFIG_USBIP_VHCI_NR_HCS upper bound to 32

In reply to Adrian Wowk

Each VHCI HC instance registers two USB buses (one HS, one SS). USB_MAXBUS in drivers/usb/core/hcd.c is hard-coded to 64, giving an effective maximum of 32 VHCI HC instances (32 * 2 = 64 buses).

The Kconfig range for USBIP_VHCI_NR_HCS currently allows up to 128, which will cause probe failures for any HC instance beyond the 32nd. These probe failures trigger the NULL pointer dereference fixed in the previous commit.

Reduce the upper bound to 32 to reflect the real maximum imposed by USB_MAXBUS. Note that probe failures can still occur below this limit if real hardware has already claimed enough USB bus numbers, making the NULL check fix necessary regardless.

---
 drivers/usb/usbip/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/usbip/Kconfig +1 -1
index b9f94e2e278..50945b6fae1 100644
@@ -40,7 +40,7 @@ config USBIP_VHCI_HC_PORTS
 
 config USBIP_VHCI_NR_HCS
 	int "Number of USB/IP virtual host controllers"
-	range 1 128
+	range 1 32
 	default 1
 	depends on USBIP_VHCI_HCD
 	help
Signature · 1 line
2.53.0
maintainer Reviewed-by on v1 2/2
#4 lore raw

Re: [PATCH 2/2] usbip: vhci_hcd: reduce CONFIG_USBIP_VHCI_NR_HCS upper bound to 32

Quoted 24 lines · Adrian Wowk

Each VHCI HC instance registers two USB buses (one HS, one SS). USB_MAXBUS in drivers/usb/core/hcd.c is hard-coded to 64, giving an effective maximum of 32 VHCI HC instances (32 * 2 = 64 buses).

The Kconfig range for USBIP_VHCI_NR_HCS currently allows up to 128, which will cause probe failures for any HC instance beyond the 32nd. These probe failures trigger the NULL pointer dereference fixed in the previous commit.

Reduce the upper bound to 32 to reflect the real maximum imposed by USB_MAXBUS. Note that probe failures can still occur below this limit if real hardware has already claimed enough USB bus numbers, making the NULL check fix necessary regardless.

---
  drivers/usb/usbip/Kconfig | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/usbip/Kconfig +0 -0
index b9f94e2e278..50945b6fae1 100644

drivers/usb/usbip/Kconfig config USBIP_VHCI_HC_PORTS · line 46

Quoted 8 lines
drivers/usb/usbip/Kconfig +1 -1
@@ -40,7 +40,7 @@ config USBIP_VHCI_HC_PORTS
 
  config USBIP_VHCI_NR_HCS
  	int "Number of USB/IP virtual host controllers"
-	range 1 128
+	range 1 32
  	default 1
  	depends on USBIP_VHCI_HCD
  	help

Looks good to me.

Greg, Please pick this up.

thanks,
-- Shuah