linux-kernel - Re: [PATCH] usb: phy: Initialize struct usb_phy list

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <7e2bc6d7-4fb2-4015-a0f8-38a34a7f8db0@tecnico.ulisboa.pt>
Date: Fri, 21 Nov 2025 17:03:11 +0000
From: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc: Felipe Balbi <felipe.balbi@...ux.intel.com>,
 Baolin Wang <baolin.wang@...ux.alibaba.com>, linux-usb@...r.kernel.org,
 linux-kernel@...r.kernel.org, stable@...r.kernel.org
Subject: Re: [PATCH] usb: phy: Initialize struct usb_phy list_head



On 11/21/25 16:48, Greg Kroah-Hartman wrote:
> On Fri, Nov 21, 2025 at 04:39:58PM +0000, Diogo Ivo wrote:
>>
>>
>> On 11/21/25 15:03, Greg Kroah-Hartman wrote:
>>> On Fri, Nov 21, 2025 at 02:55:35PM +0000, Diogo Ivo wrote:
>>>>
>>>>
>>>> On 11/21/25 14:09, Greg Kroah-Hartman wrote:
>>>>> On Thu, Nov 13, 2025 at 02:59:06PM +0000, Diogo Ivo wrote:
>>>>>> When executing usb_add_phy() and usb_add_phy_dev() it is possible that
>>>>>> usb_add_extcon() fails (for example with -EPROBE_DEFER), in which case
>>>>>> the usb_phy does not get added to phy_list via
>>>>>> list_add_tail(&x->head, phy_list).
>>>>>>
>>>>>> Then, when the driver that tried to add the phy receives the error
>>>>>> propagated from usb_add_extcon() and calls into usb_remove_phy() to
>>>>>> undo the partial registration there will be an unconditional call to
>>>>>> list_del(&x->head) which is notinitialized and leads to a NULL pointer
>>>>>> dereference.
>>>>>>
>>>>>> Fix this by initializing x->head before usb_add_extcon() has a chance to
>>>>>> fail.
>>>>>>
>>>>>> Fixes: 7d21114dc6a2d53 ("usb: phy: Introduce one extcon device into usb phy")
>>>>>> Cc: stable@...r.kernel.org
>>>>>> Signed-off-by: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
>>>>>> ---
>>>>>>     drivers/usb/phy/phy.c | 4 ++++
>>>>>>     1 file changed, 4 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/phy/phy.c b/drivers/usb/phy/phy.c
>>>>>> index e1435bc59662..5a9b9353f343 100644
>>>>>> --- a/drivers/usb/phy/phy.c
>>>>>> +++ b/drivers/usb/phy/phy.c
>>>>>> @@ -646,6 +646,8 @@ int usb_add_phy(struct usb_phy *x, enum usb_phy_type type)
>>>>>>     		return -EINVAL;
>>>>>>     	}
>>>>>> +	INIT_LIST_HEAD(&x->head);
>>>>>> +
>>>>>>     	usb_charger_init(x);
>>>>>>     	ret = usb_add_extcon(x);
>>>>>>     	if (ret)
>>>>>> @@ -696,6 +698,8 @@ int usb_add_phy_dev(struct usb_phy *x)
>>>>>>     		return -EINVAL;
>>>>>>     	}
>>>>>> +	INIT_LIST_HEAD(&x->head);
>>>>>> +
>>>>>>     	usb_charger_init(x);
>>>>>>     	ret = usb_add_extcon(x);
>>>>>>     	if (ret)
>>>>>>
>>>>>
>>>>> Shouldn't you be also removing an existing call to INIT_LIST_HEAD()
>>>>> somewhere?  This is not "moving" the code, it is adding it.
>>>>
>>>>   From my understanding that's exactly the problem, currently there is no
>>>> call to INIT_LIST_HEAD() anywhere on these code paths, meaning that if
>>>> we do not reach the point of calling list_add_tail() at the end of
>>>> usb_add_phy() and usb_phy_add_dev() then x->head will remain uninitialized
>>>> and fault when running usb_remove_phy().
>>>
>>> Then how does this work at all if the list is never initialized?
>>
>> In this case in drivers/usb/phy/phy.c a static LIST_HEAD(phy_list) is
>> declared and then for each new 'struct usb_phy *x' the x->head entry
>> will get added to this list by calling list_add_tail(&x->head, &phy_list).
> 
> Great, can you document this in the changelog text so that it makes more
> sense (and properly quote the fixes: sha1, I think you have too many
> digits there...) and resend a v2.

Yes, I'll fix it and send out a v2.

Thanks,
Diogo

> thanks,
> 
> greg k-h