* [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() @ 2022-04-21 9:42 Zixuan Fu 2022-04-21 10:07 ` Greg KH 0 siblings, 1 reply; 11+ messages in thread From: Zixuan Fu @ 2022-04-21 9:42 UTC (permalink / raw) To: mathias.nyman, gregkh Cc: linux-usb, linux-kernel, baijiaju1990, Zixuan Fu, TOTE Robot 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 <oslab@tsinghua.edu.cn> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> --- 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 9:42 [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() Zixuan Fu @ 2022-04-21 10:07 ` Greg KH [not found] ` <CAMvdLAPR6JN6PqL_z+MwR=kB5o=+ydBbast9Q-Zgzt_Hwt+UJg@mail.gmail.com> 2022-04-21 11:55 ` Fu Zixuan 0 siblings, 2 replies; 11+ messages in thread From: Greg KH @ 2022-04-21 10:07 UTC (permalink / raw) To: Zixuan Fu Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot 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 <oslab@tsinghua.edu.cn> > Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > --- > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
[parent not found: <CAMvdLAPR6JN6PqL_z+MwR=kB5o=+ydBbast9Q-Zgzt_Hwt+UJg@mail.gmail.com>]
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() [not found] ` <CAMvdLAPR6JN6PqL_z+MwR=kB5o=+ydBbast9Q-Zgzt_Hwt+UJg@mail.gmail.com> @ 2022-04-21 11:33 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2022-04-21 11:33 UTC (permalink / raw) To: Fu Zixuan Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, Apr 21, 2022 at 06:29:30PM +0800, Fu Zixuan wrote: <snip> For some reason you sent this in html format, and top-posted, making this not show up on the mailing lists. Please fix your email client and resend and I will be glad to respond. thanks, greg k-h ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 10:07 ` Greg KH [not found] ` <CAMvdLAPR6JN6PqL_z+MwR=kB5o=+ydBbast9Q-Zgzt_Hwt+UJg@mail.gmail.com> @ 2022-04-21 11:55 ` Fu Zixuan 2022-04-21 12:06 ` Greg KH 1 sibling, 1 reply; 11+ messages in thread From: Fu Zixuan @ 2022-04-21 11:55 UTC (permalink / raw) To: Greg KH; +Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > > Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > > --- > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 11:55 ` Fu Zixuan @ 2022-04-21 12:06 ` Greg KH 2022-04-21 12:21 ` Fu Zixuan 0 siblings, 1 reply; 11+ messages in thread From: Greg KH @ 2022-04-21 12:06 UTC (permalink / raw) To: Fu Zixuan Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: > On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > > > Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > > > --- > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 12:06 ` Greg KH @ 2022-04-21 12:21 ` Fu Zixuan 2022-04-21 12:52 ` Mathias Nyman 0 siblings, 1 reply; 11+ messages in thread From: Fu Zixuan @ 2022-04-21 12:21 UTC (permalink / raw) To: Greg KH; +Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, 21 Apr 2022 at 20:06, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: > > On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > > > > Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > > > > --- > > > > 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 12:21 ` Fu Zixuan @ 2022-04-21 12:52 ` Mathias Nyman 2022-04-21 13:11 ` Fu Zixuan 2022-04-21 16:11 ` Greg KH 0 siblings, 2 replies; 11+ messages in thread From: Mathias Nyman @ 2022-04-21 12:52 UTC (permalink / raw) To: Fu Zixuan, Greg KH, Heiner Kallweit Cc: mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On 21.4.2022 15.21, Fu Zixuan wrote: > On Thu, 21 Apr 2022 at 20:06, Greg KH <gregkh@linuxfoundation.org> wrote: >> >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> >>>>> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> >>>>> --- >>>>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 12:52 ` Mathias Nyman @ 2022-04-21 13:11 ` Fu Zixuan 2022-04-21 13:30 ` Mathias Nyman 2022-04-21 13:32 ` Fu Zixuan 2022-04-21 16:11 ` Greg KH 1 sibling, 2 replies; 11+ messages in thread From: Fu Zixuan @ 2022-04-21 13:11 UTC (permalink / raw) To: Mathias Nyman Cc: Greg KH, Heiner Kallweit, mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman <mathias.nyman@linux.intel.com> wrote: > > On 21.4.2022 15.21, Fu Zixuan wrote: > > On Thu, 21 Apr 2022 at 20:06, Greg KH <gregkh@linuxfoundation.org> wrote: > >> > >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: > >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > >>>>> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > >>>>> --- > >>>>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 13:11 ` Fu Zixuan @ 2022-04-21 13:30 ` Mathias Nyman 2022-04-21 13:32 ` Fu Zixuan 1 sibling, 0 replies; 11+ messages in thread From: Mathias Nyman @ 2022-04-21 13:30 UTC (permalink / raw) To: Fu Zixuan Cc: Greg KH, Heiner Kallweit, mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On 21.4.2022 16.11, Fu Zixuan wrote: > On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: >> >> On 21.4.2022 15.21, Fu Zixuan wrote: >>> On Thu, 21 Apr 2022 at 20:06, Greg KH <gregkh@linuxfoundation.org> wrote: >>>> >>>> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: >>>>> On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> >>>>>>> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> >>>>>>> --- >>>>>>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 13:11 ` Fu Zixuan 2022-04-21 13:30 ` Mathias Nyman @ 2022-04-21 13:32 ` Fu Zixuan 1 sibling, 0 replies; 11+ messages in thread From: Fu Zixuan @ 2022-04-21 13:32 UTC (permalink / raw) To: Mathias Nyman Cc: Greg KH, Heiner Kallweit, mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot On Thu, Apr 21, 2022 at 9:11 PM Fu Zixuan <r33s3n6@gmail.com> wrote: > > On Thu, Apr 21, 2022 at 8:50 PM Mathias Nyman > <mathias.nyman@linux.intel.com> wrote: > > > > On 21.4.2022 15.21, Fu Zixuan wrote: > > > On Thu, 21 Apr 2022 at 20:06, Greg KH <gregkh@linuxfoundation.org> wrote: > > >> > > >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: > > >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > > >>>>> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > > >>>>> --- > > >>>>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() 2022-04-21 12:52 ` Mathias Nyman 2022-04-21 13:11 ` Fu Zixuan @ 2022-04-21 16:11 ` Greg KH 1 sibling, 0 replies; 11+ messages in thread From: Greg KH @ 2022-04-21 16:11 UTC (permalink / raw) To: Mathias Nyman Cc: Fu Zixuan, Heiner Kallweit, mathias.nyman, linux-usb, linux-kernel, baijiaju1990, TOTE Robot 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 <gregkh@linuxfoundation.org> wrote: > >> > >> On Thu, Apr 21, 2022 at 07:55:28PM +0800, Fu Zixuan wrote: > >>> On Thu, 21 Apr 2022 at 18:07, Greg KH <gregkh@linuxfoundation.org> 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 <oslab@tsinghua.edu.cn> > >>>>> Signed-off-by: Zixuan Fu <r33s3n6@gmail.com> > >>>>> --- > >>>>> 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-04-21 16:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 9:42 [PATCH] drivers: usb: host: fix NULL pointer dereferences triggered by unhandled errors in xhci_create_rhub_port_array() Zixuan Fu
2022-04-21 10:07 ` Greg KH
[not found] ` <CAMvdLAPR6JN6PqL_z+MwR=kB5o=+ydBbast9Q-Zgzt_Hwt+UJg@mail.gmail.com>
2022-04-21 11:33 ` Greg KH
2022-04-21 11:55 ` Fu Zixuan
2022-04-21 12:06 ` Greg KH
2022-04-21 12:21 ` Fu Zixuan
2022-04-21 12:52 ` Mathias Nyman
2022-04-21 13:11 ` Fu Zixuan
2022-04-21 13:30 ` Mathias Nyman
2022-04-21 13:32 ` Fu Zixuan
2022-04-21 16:11 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.