| Message ID | 20240109061708.26288-1-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] usb: core: Prevent null pointer dereference in update_port_device_state | expand |
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 function > returning 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
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); + } } }
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(-)