[PATCH] usb: phy: Initialize struct usb_phy list_head

[PATCH] usb: phy: Initialize struct usb_phy list_head

[Diogo Ivo]

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)

---
base-commit: 35d084745b3ea4af571ed421844f2bb1a99ad6e2
change-id: 20251113-diogo-smaug_typec-56b2059b892b

Best regards,
-- 
Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>

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

[Greg Kroah-Hartman]

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.

thanks,

greg k-h

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

[Diogo Ivo]

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().

Best regards,
Diogo

> thanks,
> 
> greg k-h

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

[Greg Kroah-Hartman]

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?

thanks,

greg k-h

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

[Diogo Ivo]

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

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

[Greg Kroah-Hartman]

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@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).

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.

thanks,

greg k-h

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

[Diogo Ivo]

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@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).
> 
> 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