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.
Here is the log when this bug occurred in our fault-injection testing:
[ 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 <[email protected]>
Signed-off-by: Zixuan Fu <[email protected]>
---
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
On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
>
> To fix the bug, xhci_setup_port_arrays() should return an integer to
> indicate a possible error, and its callers should handle the error.
>
> Here is the log when this bug occurred in our fault-injection testing:
>
> [ 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 <[email protected]>
> Signed-off-by: Zixuan Fu <[email protected]>
> ---
> 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;
What about the memory allocated by the first call to
xhci_create_rhub_port_array()? Is that now lost? Same for everything
else allocated before these calls, how is that cleaned up properly?
thanks,
greg k-h
On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
> >
> > On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> > >
> > > To fix the bug, xhci_setup_port_arrays() should return an integer to
> > > indicate a possible error, and its callers should handle the error.
> > >
> > > Here is the log when this bug occurred in our fault-injection testing:
> > >
> > > [ 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 <[email protected]>
> > > Signed-off-by: Zixuan Fu <[email protected]>
> > > ---
> > > 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;
> >
> > What about the memory allocated by the first call to
> > xhci_create_rhub_port_array()? Is that now lost? Same for everything
> > else allocated before these calls, how is that cleaned up properly?
> >
> > thanks,
> >
> > greg k-h
>
> Thanks for your swift reply. We understand your concern. In fact, we have
> checked the related code carefully and found that xhci_create_rhub_port_array()
> is only used in xhci_setup_port_arrays(). Moreover, 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 called
> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
Great, can you mention this in the changelog text to show that you have
thought this through and it can be documented as such?
thanks,
greg k-h
On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
> > On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
> > >
> > > On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> > > >
> > > > To fix the bug, xhci_setup_port_arrays() should return an integer to
> > > > indicate a possible error, and its callers should handle the error.
> > > >
> > > > Here is the log when this bug occurred in our fault-injection testing:
> > > >
> > > > [ 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 <[email protected]>
> > > > Signed-off-by: Zixuan Fu <[email protected]>
> > > > ---
> > > > 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;
> > >
> > > What about the memory allocated by the first call to
> > > xhci_create_rhub_port_array()? Is that now lost? Same for everything
> > > else allocated before these calls, how is that cleaned up properly?
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > Thanks for your swift reply. We understand your concern. In fact, we have
> > checked the related code carefully and found that xhci_create_rhub_port_array()
> > is only used in xhci_setup_port_arrays(). Moreover, 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 called
> > kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
>
> Great, can you mention this in the changelog text to show that you have
> thought this through and it can be documented as such?
>
> thanks,
>
> greg k-h
Thanks for your reply! We will do that and submit the patch v2 soon.
Thanks,
Zixuan Fu
On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> >
> > To fix the bug, xhci_setup_port_arrays() should return an integer to
> > indicate a possible error, and its callers should handle the error.
> >
> > Here is the log when this bug occurred in our fault-injection testing:
> >
> > [ 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 <[email protected]>
> > Signed-off-by: Zixuan Fu <[email protected]>
> > ---
> > 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;
>
> What about the memory allocated by the first call to
> xhci_create_rhub_port_array()? Is that now lost? Same for everything
> else allocated before these calls, how is that cleaned up properly?
>
> thanks,
>
> greg k-h
Thanks for your swift reply. We understand your concern. In fact, we have
checked the related code carefully and found that xhci_create_rhub_port_array()
is only used in xhci_setup_port_arrays(). Moreover, 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 called
kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
Thanks,
Zixuan Fu
On 21.4.2022 15.21, Fu Zixuan wrote:
> On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
>>
>> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
>>> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
>>>>
>>>> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
>>>>>
>>>>> To fix the bug, xhci_setup_port_arrays() should return an integer to
>>>>> indicate a possible error, and its callers should handle the error.
>>>>>
>>>>> Here is the log when this bug occurred in our fault-injection testing:
>>>>>
>>>>> [ 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 <[email protected]>
>>>>> Signed-off-by: Zixuan Fu <[email protected]>
>>>>> ---
>>>>> 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;
>>>>
>>>> What about the memory allocated by the first call to
>>>> xhci_create_rhub_port_array()? Is that now lost? Same for everything
>>>> else allocated before these calls, how is that cleaned up properly?
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>
>>> Thanks for your swift reply. We understand your concern. In fact, we have
>>> checked the related code carefully and found that xhci_create_rhub_port_array()
>>> is only used in xhci_setup_port_arrays(). Moreover, 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 called
>>> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
>>
>> Great, can you mention this in the changelog text to show that you have
>> thought this through and it can be documented as such?
>>
>> thanks,
>>
>> greg k-h
>
> Thanks for your reply! We will do that and submit the patch v2 soon.
>
Good to get this fixed, but there's a series by Heiner Kallweit that adds support
for xHC controllers with just one roothub [1].
It will conflict with this.
We might need to change this a bit so that this can go to stable alone, but still
being being able to somewhat neatly apply that new series on top of this.
1. https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next
Thanks
-Mathias
On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman
<[email protected]> wrote:
>
> On 21.4.2022 15.21, Fu Zixuan wrote:
> > On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
> >>
> >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
> >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
> >>>>
> >>>> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> >>>>>
> >>>>> To fix the bug, xhci_setup_port_arrays() should return an integer to
> >>>>> indicate a possible error, and its callers should handle the error.
> >>>>>
> >>>>> Here is the log when this bug occurred in our fault-injection testing:
> >>>>>
> >>>>> [ 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 <[email protected]>
> >>>>> Signed-off-by: Zixuan Fu <[email protected]>
> >>>>> ---
> >>>>> 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;
> >>>>
> >>>> What about the memory allocated by the first call to
> >>>> xhci_create_rhub_port_array()? Is that now lost? Same for everything
> >>>> else allocated before these calls, how is that cleaned up properly?
> >>>>
> >>>> thanks,
> >>>>
> >>>> greg k-h
> >>>
> >>> Thanks for your swift reply. We understand your concern. In fact, we have
> >>> checked the related code carefully and found that xhci_create_rhub_port_array()
> >>> is only used in xhci_setup_port_arrays(). Moreover, 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 called
> >>> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
> >>
> >> Great, can you mention this in the changelog text to show that you have
> >> thought this through and it can be documented as such?
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> > Thanks for your reply! We will do that and submit the patch v2 soon.
> >
>
> Good to get this fixed, but there's a series by Heiner Kallweit that adds support
> for xHC controllers with just one roothub [1].
> It will conflict with this.
>
> We might need to change this a bit so that this can go to stable alone, but still
> being being able to somewhat neatly apply that new series on top of this.
>
> 1. https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next
>
> Thanks
> -Mathias
No problem! We will submit our patch on that branch.
Thanks,
Zixuan Fu
On 21.4.2022 16.11, Fu Zixuan wrote:
> On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman
> <[email protected]> wrote:
>>
>> On 21.4.2022 15.21, Fu Zixuan wrote:
>>> On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
>>>>
>>>> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
>>>>> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
>>>>>>
>>>>>> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
minor nit: rhub->ports dereference shouldn't happen in those functions if
rhub->num_ports is zero.
The dereference is only an issue if kcalloc_node() fails.
>>>>>>>
>>>>>>> To fix the bug, xhci_setup_port_arrays() should return an integer to
>>>>>>> indicate a possible error, and its callers should handle the error.
>>>>>>>
>>>>>>> Here is the log when this bug occurred in our fault-injection testing:
>>>>>>>
>>>>>>> [ 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 <[email protected]>
>>>>>>> Signed-off-by: Zixuan Fu <[email protected]>
>>>>>>> ---
>>>>>>> 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;
>>>>>>
>>>>>> What about the memory allocated by the first call to
>>>>>> xhci_create_rhub_port_array()? Is that now lost? Same for everything
>>>>>> else allocated before these calls, how is that cleaned up properly?
>>>>>>
>>>>>> thanks,
>>>>>>
>>>>>> greg k-h
>>>>>
>>>>> Thanks for your swift reply. We understand your concern. In fact, we have
>>>>> checked the related code carefully and found that xhci_create_rhub_port_array()
>>>>> is only used in xhci_setup_port_arrays(). Moreover, 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 called
>>>>> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
>>>>
>>>> Great, can you mention this in the changelog text to show that you have
>>>> thought this through and it can be documented as such?
>>>>
>>>> thanks,
>>>>
>>>> greg k-h
>>>
>>> Thanks for your reply! We will do that and submit the patch v2 soon.
>>>
>>
>> Good to get this fixed, but there's a series by Heiner Kallweit that adds support
>> for xHC controllers with just one roothub [1].
>> It will conflict with this.
>>
>> We might need to change this a bit so that this can go to stable alone, but still
>> being being able to somewhat neatly apply that new series on top of this.
>>
>> 1. https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next
>>
>> Thanks
>> -Mathias
>
> No problem! We will submit our patch on that branch.
No need after all, took a second look at that series.
We can easily rebase it on top of your patch with just a small adjustment.
This way your fix can go to 5.18 and stable kernels, and the "one roothub"
feature can go on top of it to 5.19.
So just add the comment Greg Suggested.
Sorry about the confusion.
Thanks
-Mathias
On Thu, Apr 21, 2022 at 9:11 PM Fu Zixuan <[email protected]> wrote:
>
> On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman
> <[email protected]> wrote:
> >
> > On 21.4.2022 15.21, Fu Zixuan wrote:
> > > On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
> > >>
> > >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
> > >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
> > >>>>
> > >>>> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> > >>>>>
> > >>>>> To fix the bug, xhci_setup_port_arrays() should return an integer to
> > >>>>> indicate a possible error, and its callers should handle the error.
> > >>>>>
> > >>>>> Here is the log when this bug occurred in our fault-injection testing:
> > >>>>>
> > >>>>> [ 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 <[email protected]>
> > >>>>> Signed-off-by: Zixuan Fu <[email protected]>
> > >>>>> ---
> > >>>>> 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;
> > >>>>
> > >>>> What about the memory allocated by the first call to
> > >>>> xhci_create_rhub_port_array()? Is that now lost? Same for everything
> > >>>> else allocated before these calls, how is that cleaned up properly?
> > >>>>
> > >>>> thanks,
> > >>>>
> > >>>> greg k-h
> > >>>
> > >>> Thanks for your swift reply. We understand your concern. In fact, we have
> > >>> checked the related code carefully and found that xhci_create_rhub_port_array()
> > >>> is only used in xhci_setup_port_arrays(). Moreover, 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 called
> > >>> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
> > >>
> > >> Great, can you mention this in the changelog text to show that you have
> > >> thought this through and it can be documented as such?
> > >>
> > >> thanks,
> > >>
> > >> greg k-h
> > >
> > > Thanks for your reply! We will do that and submit the patch v2 soon.
> > >
> >
> > Good to get this fixed, but there's a series by Heiner Kallweit that adds support
> > for xHC controllers with just one roothub [1].
> > It will conflict with this.
> >
> > We might need to change this a bit so that this can go to stable alone, but still
> > being being able to somewhat neatly apply that new series on top of this.
> >
> > 1. https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=for-usb-next
> >
> > Thanks
> > -Mathias
>
> No problem! We will submit our patch on that branch.
>
> Thanks,
>
> Zixuan Fu
I'm sorry that I misunderstand what you meant. I have checked the code
on that branch
and found related code has been removed. Thus we decide to submit our patch to
stable branch.
Thanks,
Zixuan Fu
On Thu, Apr 21, 2022 at 03:52:40PM +0300, Mathias Nyman wrote:
> On 21.4.2022 15.21, Fu Zixuan wrote:
> > On Thu, 21 Apr 2022 at 20:06, Greg KH <[email protected]> wrote:
> >>
> >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote:
> >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <[email protected]> wrote:
> >>>>
> >>>> On Thu, Apr 21, 2022 at 05:42:36PM +0800, 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().
> >>>>>
> >>>>> To fix the bug, xhci_setup_port_arrays() should return an integer to
> >>>>> indicate a possible error, and its callers should handle the error.
> >>>>>
> >>>>> Here is the log when this bug occurred in our fault-injection testing:
> >>>>>
> >>>>> [ 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 <[email protected]>
> >>>>> Signed-off-by: Zixuan Fu <[email protected]>
> >>>>> ---
> >>>>> 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;
> >>>>
> >>>> What about the memory allocated by the first call to
> >>>> xhci_create_rhub_port_array()? Is that now lost? Same for everything
> >>>> else allocated before these calls, how is that cleaned up properly?
> >>>>
> >>>> thanks,
> >>>>
> >>>> greg k-h
> >>>
> >>> Thanks for your swift reply. We understand your concern. In fact, we have
> >>> checked the related code carefully and found that xhci_create_rhub_port_array()
> >>> is only used in xhci_setup_port_arrays(). Moreover, 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 called
> >>> kfree(xhci->usb2_rhub.ports) and kfree(xhci->usb3_rhub.ports).
> >>
> >> Great, can you mention this in the changelog text to show that you have
> >> thought this through and it can be documented as such?
> >>
> >> thanks,
> >>
> >> greg k-h
> >
> > Thanks for your reply! We will do that and submit the patch v2 soon.
> >
>
> Good to get this fixed, but there's a series by Heiner Kallweit that adds support
> for xHC controllers with just one roothub [1].
> It will conflict with this.
>
> We might need to change this a bit so that this can go to stable alone, but still
> being being able to somewhat neatly apply that new series on top of this.
As this is not anything a normal user will ever hit, it should be redone
on top of your xhci changes as it's not needed in any stable tree.
thanks,
greg k-h