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: [email protected]
Signed-off-by: Udipto Goswami <[email protected]>
---
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.
+ */
+ if (hub) {
+ port_dev = hub->ports[udev->portnum - 1];
+ WRITE_ONCE(port_dev->state, udev->state);
+ sysfs_notify_dirent(port_dev->state_kn);
+ }
}
}
--
2.17.1
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: [email protected]
> Signed-off-by: Udipto Goswami <[email protected]>
> ---
> 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: [email protected]
>> Signed-off-by: Udipto Goswami <[email protected]>
>> ---
>> 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