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 --- v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable.
drivers/usb/core/hub.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..6b514546e59b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,23 @@ 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 procedure of unbinding the hub before running + * the rest of the procedure. This triggers + * hub_disconnect will set the hub's maxchild to 0. + * This would result usb_hub_to_struct_hub in this + * function to return 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); + } } }
On 1/9/24 9:17 AM, 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
v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable.
drivers/usb/core/hub.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..6b514546e59b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,23 @@ 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 procedure of unbinding the hub before running* the rest of the procedure. This triggers* hub_disconnect will set the hub's maxchild to 0.
I can't parse this sentence, s/th is missing...
* This would result usb_hub_to_struct_hub in this* function to return NULL.
"This would result in usb_hub_to_struct_hub in this function returning NULL", perhaps?
** 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);}}
MBR, Sergey
Hi Sergei,
On 1/9/2024 3:01 PM, Sergei Shtylyov wrote:
On 1/9/24 9:17 AM, 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
v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable.
drivers/usb/core/hub.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..6b514546e59b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,23 @@ 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 procedure of unbinding the hub before running* the rest of the procedure. This triggers* hub_disconnect will set the hub's maxchild to 0.I can't parse this sentence, s/th is missing...
Thanks for the review. Maybe this would sound better?
"This triggers hub_disconnect which will set hub's maxchild to 0"
* This would result usb_hub_to_struct_hub in this* function to return NULL."This would result in usb_hub_to_struct_hub in this functionreturning NULL", perhaps?
yah sound better. Will take care of it in next version.
Thanks, -Udipto
On 1/9/24 2:57 PM, 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
v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable.
drivers/usb/core/hub.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..6b514546e59b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,23 @@ 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 procedure of unbinding the hub before running + * the rest of the procedure. This triggers + * hub_disconnect will set the hub's maxchild to 0.
I can't parse this sentence, s/th is missing...
Thanks for the review. Maybe this would sound better?
"This triggers hub_disconnect which will set hub's maxchild to 0"
That seems parsable. :-)
+ * This would result usb_hub_to_struct_hub in this + * function to return NULL.
"This would result in usb_hub_to_struct_hub in this function returning NULL", perhaps?
yah sound better. Will take care of it in next version.
Probably "in this function" should be dropped altogether...
Thanks, -Udipto
MBR, Sergey
On 1/9/2024 8:10 PM, Sergei Shtylyov wrote:
On 1/9/24 2:57 PM, 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
v3: Re-wrote the comment for better context. v2: Introduced comment for the if check & CC'ed stable.
drivers/usb/core/hub.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index ffd7c99e24a3..6b514546e59b 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2053,9 +2053,23 @@ 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 procedure of unbinding the hub before running + * the rest of the procedure. This triggers + * hub_disconnect will set the hub's maxchild to 0.
I can't parse this sentence, s/th is missing...
Thanks for the review. Maybe this would sound better?
"This triggers hub_disconnect which will set hub's maxchild to 0"
That seems parsable. :-)+ * This would result usb_hub_to_struct_hub in this + * function to return NULL.
"This would result in usb_hub_to_struct_hub in this function returning NULL", perhaps?
yah sound better. Will take care of it in next version.
Probably "in this function" should be dropped altogether...
sure, i'll remove in v4.
Thanks, -Udipto
linux-stable-mirror@lists.linaro.org
-
Sergei Shtylyov -
Udipto Goswami