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

[Udipto Goswami <quic_ugoswami@quicinc.com>]

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