| Message ID | 20240110091422.25347-1-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] usb: core: Prevent null pointer dereference in update_port_device_state | expand |
On 1/10/24 12:14 PM, Udipto Goswami wrote: > Currently,the function update_port_device_state gets the usb_hub from Need space between comma and "the"... > udev->parent by calling usb_hub_to_struct_hub. > However, in case the actconfig or the maxchild is 0, the usb_hub would > be NULL and upon further accessing to get port_dev would result in null > pointer dereference. > > Fix this by introducing an if check after the usb_hub is populated. > > Fixes: 83cb2604f641 ("usb: core: add sysfs entry for usb device state") > Cc: stable@vger.kernel.org > Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com> [...] > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index ffd7c99e24a3..5ba1875e6bf4 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2053,9 +2053,22 @@ static void update_port_device_state(struct usb_device *udev) > > if (udev->parent) { > hub = usb_hub_to_struct_hub(udev->parent); > - port_dev = hub->ports[udev->portnum - 1]; > - WRITE_ONCE(port_dev->state, udev->state); > - sysfs_notify_dirent(port_dev->state_kn); > + > + /* > + * The Link Layer Validation System Driver (lvstest) > + * has step to unbind the hub before running the rest > + * of the procedure. This triggers hub_disconnect which > + * will set the hub's maxchild to 0, further resulting Resulting in. > + * usb_hub_to_struct_hub returning NULL. > + * > + * Add if check to avoid running into NULL pointer > + * de-reference. This is obvious from the code below, I think... > + */ > + if (hub) { > + port_dev = hub->ports[udev->portnum - 1]; > + WRITE_ONCE(port_dev->state, udev->state); > + sysfs_notify_dirent(port_dev->state_kn); > + } > } > } > MBR, Sergey
On 1/10/2024 2:52 PM, Sergei Shtylyov wrote: > On 1/10/24 12:14 PM, Udipto Goswami wrote: > >> Currently,the function update_port_device_state gets the usb_hub from > > Need space between comma and "the"... got it. > >> udev->parent by calling usb_hub_to_struct_hub. >> However, in case the actconfig or the maxchild is 0, the usb_hub would >> be NULL and upon further accessing to get port_dev would result in null >> pointer dereference. >> >> Fix this by introducing an if check after the usb_hub is populated. >> >> Fixes: 83cb2604f641 ("usb: core: add sysfs entry for usb device state") >> Cc: stable@vger.kernel.org >> Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com> > [...] > >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index ffd7c99e24a3..5ba1875e6bf4 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -2053,9 +2053,22 @@ static void update_port_device_state(struct usb_device *udev) >> >> if (udev->parent) { >> hub = usb_hub_to_struct_hub(udev->parent); >> - port_dev = hub->ports[udev->portnum - 1]; >> - WRITE_ONCE(port_dev->state, udev->state); >> - sysfs_notify_dirent(port_dev->state_kn); >> + >> + /* >> + * The Link Layer Validation System Driver (lvstest) >> + * has step to unbind the hub before running the rest >> + * of the procedure. This triggers hub_disconnect which >> + * will set the hub's maxchild to 0, further resulting > > Resulting in. got it. > >> + * usb_hub_to_struct_hub returning NULL. >> + * >> + * Add if check to avoid running into NULL pointer >> + * de-reference. > > This is obvious from the code below, I think... ok will remove this. Thanks, -Udipto
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..5ba1875e6bf4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,22 @@ static void update_port_device_state(struct usb_device *udev) if (udev->parent) { hub = usb_hub_to_struct_hub(udev->parent); - port_dev = hub->ports[udev->portnum - 1]; - WRITE_ONCE(port_dev->state, udev->state); - sysfs_notify_dirent(port_dev->state_kn); + + /* + * The Link Layer Validation System Driver (lvstest) + * has step to unbind the hub before running the rest + * of the procedure. This triggers hub_disconnect which + * will set the hub's maxchild to 0, further resulting + * usb_hub_to_struct_hub returning NULL. + * + * Add if check to avoid running into NULL pointer + * de-reference. + */ + if (hub) { + port_dev = hub->ports[udev->portnum - 1]; + WRITE_ONCE(port_dev->state, udev->state); + sysfs_notify_dirent(port_dev->state_kn); + } } }
Currently,the function update_port_device_state gets the usb_hub from udev->parent by calling usb_hub_to_struct_hub. However, in case the actconfig or the maxchild is 0, the usb_hub would be NULL and upon further accessing to get port_dev would result in null pointer dereference. Fix this by introducing an if check after the usb_hub is populated. Fixes: 83cb2604f641 ("usb: core: add sysfs entry for usb device state") Cc: stable@vger.kernel.org Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com> --- v4: Fixed minor mistakes in the comment. v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable. drivers/usb/core/hub.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)