| Message ID | 20240108130706.15698-1-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] usb: core: Prevent null pointer dereference in update_port_device_state | expand |
On Mon, Jan 08, 2024 at 06:37:06PM +0530, Udipto Goswami wrote: > 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> > --- > v2: Introduced comment for the if check & CC'ed stable. > > drivers/usb/core/hub.c | 15 ++++++++++++--- > 1 file changed, 12 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c > index ffd7c99e24a3..d40b5500f95b 100644 > --- a/drivers/usb/core/hub.c > +++ b/drivers/usb/core/hub.c > @@ -2053,9 +2053,18 @@ 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); > + > + /* > + * usb_hub_to_struct_hub() if returns NULL can > + * potentially cause NULL pointer dereference upon further > + * access. > + * Avoid this with an if check. > + */ This is not what I meant. It's perfectly obvious that if usb_hub_to_struct_hub() returns NULL then there will be a NULL-pointer dereference. You don't need to explain that to anybody. Instead, you need to explain why it is _possible_ for usb_hub_to_struct_hub() to return NULL. The reason is because the lvstest driver messes around with usbcore internals without telling the hub driver, so hub will be NULL in cases where udev was created by lvstest. Alan Stern
On 1/8/2024 8:56 PM, Alan Stern wrote: > On Mon, Jan 08, 2024 at 06:37:06PM +0530, Udipto Goswami wrote: >> 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> >> --- >> v2: Introduced comment for the if check & CC'ed stable. >> >> drivers/usb/core/hub.c | 15 ++++++++++++--- >> 1 file changed, 12 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c >> index ffd7c99e24a3..d40b5500f95b 100644 >> --- a/drivers/usb/core/hub.c >> +++ b/drivers/usb/core/hub.c >> @@ -2053,9 +2053,18 @@ 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); >> + >> + /* >> + * usb_hub_to_struct_hub() if returns NULL can >> + * potentially cause NULL pointer dereference upon further >> + * access. >> + * Avoid this with an if check. >> + */ > > This is not what I meant. It's perfectly obvious that if > usb_hub_to_struct_hub() returns NULL then there will be a NULL-pointer > dereference. You don't need to explain that to anybody. > > Instead, you need to explain why it is _possible_ for > usb_hub_to_struct_hub() to return NULL. The reason is because the > lvstest driver messes around with usbcore internals without telling the > hub driver, so hub will be NULL in cases where udev was created by > lvstest. aah okay, Thanks for the clarification. I'll re-write the comment mentioning this. Thanks, -Udipto
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..d40b5500f95b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,18 @@ 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); + + /* + * usb_hub_to_struct_hub() if returns NULL can + * potentially cause NULL pointer dereference upon further + * access. + * Avoid this with an if check. + */ + 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> --- v2: Introduced comment for the if check & CC'ed stable. drivers/usb/core/hub.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)