[PATCH v2 resend] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array()

[PATCH v2 resend] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array()

[Zixuan Fu]

In xhci_create_rhub_port_array(), when rhub->num_ports is zero, 
rhub->ports would not be set; when kcalloc_node() fails, rhub->ports
would be set to NULL. In these two cases, xhci_create_rhub_port_array()
just returns void, and thus its callers are unaware of the error.

Then rhub->ports is dereferenced in xhci_usb3_hub_descriptor() or 
xhci_usb2_hub_descriptor().

To fix the bug, xhci_setup_port_arrays() should return an integer to 
indicate a possible error, and its callers should handle the error.

In the driver, xhci_create_rhub_port_array() is only used in 
xhci_setup_port_arrays(), and only xhci_mem_init() calls
xhci_setup_port_arrays() and does all cleanup work when it fails.
Specifically, xhci_mem_init() calls xhci_mem_cleanup(), which eventually
calls kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports). For
this reason, when xhci_create_rhub_port_array() fails, 
xhci_setup_port_arrays() should return the error code directly, without
freeing the memory allocated in this function.

The error log in our fault-injection testing is shown as follows:

[   24.001309] BUG: kernel NULL pointer dereference, address: 0000000000000000
...
[   24.003992] RIP: 0010:xhci_hub_control+0x3f5/0x60d0 [xhci_hcd]
...
[   24.009803] Call Trace:
[   24.010014]  <TASK>
[   24.011310]  usb_hcd_submit_urb+0x1233/0x1fd0
[   24.017071]  usb_start_wait_urb+0x115/0x310
[   24.017641]  usb_control_msg+0x28a/0x450
[   24.019046]  hub_probe+0xb16/0x2320
[   24.019757]  usb_probe_interface+0x4f1/0x930
[   24.019765]  really_probe+0x33d/0x970
[   24.019768]  __driver_probe_device+0x157/0x210
[   24.019772]  driver_probe_device+0x4f/0x340
[   24.019775]  __device_attach_driver+0x2ee/0x3a0
...

Reported-by: TOTE Robot <oslab@tsinghua.edu.cn>
Signed-off-by: Zixuan Fu <r33s3n6@gmail.com>
---
v2:
* Explain the reason why xhci_setup_port_arrays() returns without freeing the memory in error handling code.
  Thank Greg KH for helpful advice.
---
 drivers/usb/host/xhci-mem.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index bbb27ee2c6a3..024515346c39 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2235,7 +2235,7 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 	/* FIXME: Should we disable ports not in the Extended Capabilities? */
 }
 
-static void xhci_create_rhub_port_array(struct xhci_hcd *xhci,
+static int xhci_create_rhub_port_array(struct xhci_hcd *xhci,
 					struct xhci_hub *rhub, gfp_t flags)
 {
 	int port_index = 0;
@@ -2243,11 +2243,11 @@ static void xhci_create_rhub_port_array(struct xhci_hcd *xhci,
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
 
 	if (!rhub->num_ports)
-		return;
+		return -EINVAL;
 	rhub->ports = kcalloc_node(rhub->num_ports, sizeof(*rhub->ports),
 			flags, dev_to_node(dev));
 	if (!rhub->ports)
-		return;
+		return -ENOMEM;
 
 	for (i = 0; i < HCS_MAX_PORTS(xhci->hcs_params1); i++) {
 		if (xhci->hw_ports[i].rhub != rhub ||
@@ -2259,6 +2259,7 @@ static void xhci_create_rhub_port_array(struct xhci_hcd *xhci,
 		if (port_index == rhub->num_ports)
 			break;
 	}
+	return 0;
 }
 
 /*
@@ -2277,6 +2278,7 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
 	int cap_count = 0;
 	u32 cap_start;
 	struct device *dev = xhci_to_hcd(xhci)->self.sysdev;
+	int ret;
 
 	num_ports = HCS_MAX_PORTS(xhci->hcs_params1);
 	xhci->hw_ports = kcalloc_node(num_ports, sizeof(*xhci->hw_ports),
@@ -2367,8 +2369,13 @@ static int xhci_setup_port_arrays(struct xhci_hcd *xhci, gfp_t flags)
 	 * Not sure how the USB core will handle a hub with no ports...
 	 */
 
-	xhci_create_rhub_port_array(xhci, &xhci->usb2_rhub, flags);
-	xhci_create_rhub_port_array(xhci, &xhci->usb3_rhub, flags);
+	ret = xhci_create_rhub_port_array(xhci, &xhci->usb2_rhub, flags);
+	if (ret)
+		return ret;
+
+	ret = xhci_create_rhub_port_array(xhci, &xhci->usb3_rhub, flags);
+	if (ret)
+		return ret;
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v2 resend] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array()

[Mathias Nyman]

On 28.4.2022 12.23, Zixuan Fu wrote:
> In xhci_create_rhub_port_array(), when rhub->num_ports is zero, 
> rhub->ports would not be set; when kcalloc_node() fails, rhub->ports
> would be set to NULL. In these two cases, xhci_create_rhub_port_array()
> just returns void, and thus its callers are unaware of the error.
> 
> Then rhub->ports is dereferenced in xhci_usb3_hub_descriptor() or 
> xhci_usb2_hub_descriptor().

As it turned out this dereference is only an issue if kcalloc_node() failed,
not if rhub->num_ports is zero.

So this is more of a theoretical issue. Not urgent, not for stable.

So as Greg suggested its probably better to add this fix on top of
the one roothub support series it conflicts with.


Thanks
Mathias