Re: [PATCH] usb: phy: Initialize struct usb_phy list_head

Diogo Ivo
2025-11-21T16:40:05Z

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@vger.kernel.org
>>>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.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).

Best regards,
Diogo

> thanks,
>
> greg k-h