Received-SPF: pass (google.com: domain of linux-kernel+bounces-19083-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <7cebe5e1-4ac1-4c3d-a2f8-b283cde82105@quicinc.com>
Date: Mon, 8 Jan 2024 10:20:44 +0530
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
From: Udipto Goswami <quic_ugoswami@quicinc.com>
Subject: Re: [PATCH] usb: core: Prevent null pointer dereference in
 update_port_device_state
To: Alan Stern <stern@rowland.harvard.edu>,
        Greg Kroah-Hartman
	<gregkh@linuxfoundation.org>
CC: <linux-usb@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20240104102616.20120-1-quic_ugoswami@quicinc.com>
 <2024010447-sprite-shelter-0743@gregkh>
 <eade09eb-4454-460f-9ce6-87da986c5acf@quicinc.com>
 <2024010432-fifth-shakable-0d84@gregkh>
 <1fafda18-8806-4036-bcc1-ac08e2d3b9cd@rowland.harvard.edu>
Content-Language: en-US
In-Reply-To: <1fafda18-8806-4036-bcc1-ac08e2d3b9cd@rowland.harvard.edu>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit

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