Re: [PATCH] usb: core: Prevent null pointer dereference in update_port_device_state

[Udipto Goswami]

Hi Greg, Alan,

On 1/4/2024 8:26 PM, Alan Stern wrote:
> On Thu, Jan 04, 2024 at 02:13:51PM +0100, Greg Kroah-Hartman wrote:
> > On Thu, Jan 04, 2024 at 06:35:38PM +0530, Udipto Goswami wrote:
> > > Hi Greg,
> > >
> > > On 1/4/2024 4:14 PM, Greg Kroah-Hartman wrote:
> > > > On Thu, Jan 04, 2024 at 03:56:16PM +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.
> > > >
> > > > Is this true for any real (or fake) hardware?
> > >
> > > We saw this in our QCOM hardwares where lvstest.c was calling
> > > get_dev_desc_store:
> > >
> > > 	usb_set_device_state+0x128/0x17c
> > > 	create_lvs_device+0x60/0xf8 [lvstest]
> > > 	get_dev_desc_store+0x94/0x18c [lvstest]
> > > 	dev_attr_store+0x30/0x48
> > >
> > > I think the part of the test procedure is to first unbind the hub driver
> > > which calls hub_disconnect setting the maxchild = 0.
> >
> > Are you sure lvstest is correct here?
By the commit description of lvstest driver this seems to be the procedure:

https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/usb/misc/lvstest.c?h=v6.6.9&id=ce21bfe603b3401c258c415456c915634998e133

As you can see it mentions unbind is necessary before further steps 
carried out. Also, since the test was passing before 
update_port_device_state was introduced, wasn't doubting this.

Either way, usb_hub_to_struct_hub() can potentially return NULL not only 
for maxchild == 0, but other cases like actconfig == NULL or hdev == 
NULL as well, so it isn't wise to access the hub in subsequent line.

> This is what happens when people work behind the hub driver's back.  :-(
>
> If you can't find another way to fix the problem, you should at least
> change the patch to include a comment before the "if (hub)" test,
> explaining why it is necessary.  Otherwise somebody in the future will
> remove the test, because under normal circumstances hub would never be
> NULL here.
Thanks for the review Alan. Sure I'll put a comment here stating the 
necessity of the check for clarity in the next version.

I agree under normal conditions this won't fail for example even in this 
case we unbinded 2-1. Since 1-1 wasn't unbinded that therefore usb1 has 
a maxchild still present.

Thanks,
-Udipto