[PATCH] usb: dwc3: gadget: Bail from dwc3_gadget_exit() if dwc->gadget is NULL

[PATCH] usb: dwc3: gadget: Bail from dwc3_gadget_exit() if dwc->gadget is NULL

[Jack Pham]

There exists a possible scenario in which dwc3_gadget_init() can fail:
during during host -> peripheral mode switch in dwc3_set_mode(), and
a pending gadget driver fails to bind.  Then, if the DRD undergoes
another mode switch from peripheral->host the resulting
dwc3_gadget_exit() will attempt to reference an invalid and dangling
dwc->gadget pointer as well as call dma_free_coherent() on unmapped
DMA pointers.

The exact scenario can be reproduced as follows:
 - Start DWC3 in peripheral mode
 - Configure ConfigFS gadget with FunctionFS instance (or use g_ffs)
 - Run FunctionFS userspace application (open EPs, write descriptors, etc)
 - Bind gadget driver to DWC3's UDC
 - Switch DWC3 to host mode
   => dwc3_gadget_exit() is called. usb_del_gadget() will put the
	ConfigFS driver instance on the gadget_driver_pending_list
 - Stop FunctionFS application (closes the ep files)
 - Switch DWC3 to peripheral mode
   => dwc3_gadget_init() fails as usb_add_gadget() calls
	check_pending_gadget_drivers() and attempts to rebind the UDC
	to the ConfigFS gadget but fails with -19 (-ENODEV) because the
	FFS instance is not in FFS_READY state.
 - Switch DWC3 back to host mode
   => dwc3_gadget_exit() is called again, but this time dwc->gadget
	is invalid.

Although it can be argued that userspace should take responsibility
for ensuring that the FunctionFS application be ready prior to
allowing the composite driver bind to the UDC, failure to do so
should not result in a panic from the kernel driver.

Fix this by setting dwc->gadget to NULL in the failure path of
dwc3_gadget_init() and add a check to dwc3_gadget_exit() to bail out
unless the gadget pointer is valid.

Fixes: e81a7018d93a ("usb: dwc3: allocate gadget structure dynamically")
Cc: <stable@vger.kernel.org>
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
Hi Felipe,

Although I marked the 'Fixes' tag above to e81a7018d93a, the problem
theoretically exists prior to Peter's change. But I'm not sure how
best to fix on versions prior to this change since dwc->gadget used
to be an embedded struct so we can't do a simple NULL check as below.
Suggestions on alternative approaches welcome if we want to proceed
with backporting to older (pre-5.9) stable releases.

Thanks,
Jack

 drivers/usb/dwc3/gadget.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 612825a39f82..65d9b7227752 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -4046,6 +4046,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	dwc3_gadget_free_endpoints(dwc);
 err4:
 	usb_put_gadget(dwc->gadget);
+	dwc->gadget = NULL;
 err3:
 	dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
 			dwc->bounce_addr);
@@ -4065,6 +4066,9 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 
 void dwc3_gadget_exit(struct dwc3 *dwc)
 {
+	if (!dwc->gadget)
+		return;
+
 	usb_del_gadget(dwc->gadget);
 	dwc3_gadget_free_endpoints(dwc);
 	usb_put_gadget(dwc->gadget);
-- 
2.24.0

Re: [PATCH] usb: dwc3: gadget: Bail from dwc3_gadget_exit() if dwc->gadget is NULL

[Peter Chen]

On 21-05-24 21:29:22, Jack Pham wrote:
> There exists a possible scenario in which dwc3_gadget_init() can fail:
> during during host -> peripheral mode switch in dwc3_set_mode(), and
> a pending gadget driver fails to bind.  Then, if the DRD undergoes
> another mode switch from peripheral->host the resulting
> dwc3_gadget_exit() will attempt to reference an invalid and dangling
> dwc->gadget pointer as well as call dma_free_coherent() on unmapped
> DMA pointers.
> 
> The exact scenario can be reproduced as follows:
>  - Start DWC3 in peripheral mode
>  - Configure ConfigFS gadget with FunctionFS instance (or use g_ffs)
>  - Run FunctionFS userspace application (open EPs, write descriptors, etc)
>  - Bind gadget driver to DWC3's UDC
>  - Switch DWC3 to host mode
>    => dwc3_gadget_exit() is called. usb_del_gadget() will put the
> 	ConfigFS driver instance on the gadget_driver_pending_list
>  - Stop FunctionFS application (closes the ep files)
>  - Switch DWC3 to peripheral mode
>    => dwc3_gadget_init() fails as usb_add_gadget() calls
> 	check_pending_gadget_drivers() and attempts to rebind the UDC
> 	to the ConfigFS gadget but fails with -19 (-ENODEV) because the
> 	FFS instance is not in FFS_READY state.

There is no such state at enum ffs_state, you mean flag desc_ready is not
true, right?

>  - Switch DWC3 back to host mode
>    => dwc3_gadget_exit() is called again, but this time dwc->gadget
> 	is invalid.
> 
> Although it can be argued that userspace should take responsibility
> for ensuring that the FunctionFS application be ready prior to
> allowing the composite driver bind to the UDC, failure to do so
> should not result in a panic from the kernel driver.
> 
> Fix this by setting dwc->gadget to NULL in the failure path of
> dwc3_gadget_init() and add a check to dwc3_gadget_exit() to bail out
> unless the gadget pointer is valid.
> 
> Fixes: e81a7018d93a ("usb: dwc3: allocate gadget structure dynamically")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>

Reviewed-by: Peter Chen <peter.chen@kernel.org>

Peter

> ---
> Hi Felipe,
> 
> Although I marked the 'Fixes' tag above to e81a7018d93a, the problem
> theoretically exists prior to Peter's change. But I'm not sure how
> best to fix on versions prior to this change since dwc->gadget used
> to be an embedded struct so we can't do a simple NULL check as below.
> Suggestions on alternative approaches welcome if we want to proceed
> with backporting to older (pre-5.9) stable releases.
> 
> Thanks,
> Jack
> 
>  drivers/usb/dwc3/gadget.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 612825a39f82..65d9b7227752 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -4046,6 +4046,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>  	dwc3_gadget_free_endpoints(dwc);
>  err4:
>  	usb_put_gadget(dwc->gadget);
> +	dwc->gadget = NULL;
>  err3:
>  	dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
>  			dwc->bounce_addr);
> @@ -4065,6 +4066,9 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>  
>  void dwc3_gadget_exit(struct dwc3 *dwc)
>  {
> +	if (!dwc->gadget)
> +		return;
> +
>  	usb_del_gadget(dwc->gadget);
>  	dwc3_gadget_free_endpoints(dwc);
>  	usb_put_gadget(dwc->gadget);
> -- 
> 2.24.0
> 

-- 

Thanks,
Peter Chen

Re: [PATCH] usb: dwc3: gadget: Bail from dwc3_gadget_exit() if dwc->gadget is NULL

[jackp]

On 2021-05-27 21:12, Peter Chen wrote:
> On 21-05-24 21:29:22, Jack Pham wrote:
>> There exists a possible scenario in which dwc3_gadget_init() can fail:
>> during during host -> peripheral mode switch in dwc3_set_mode(), and
>> a pending gadget driver fails to bind.  Then, if the DRD undergoes
>> another mode switch from peripheral->host the resulting
>> dwc3_gadget_exit() will attempt to reference an invalid and dangling
>> dwc->gadget pointer as well as call dma_free_coherent() on unmapped
>> DMA pointers.
>> 
>> The exact scenario can be reproduced as follows:
>>  - Start DWC3 in peripheral mode
>>  - Configure ConfigFS gadget with FunctionFS instance (or use g_ffs)
>>  - Run FunctionFS userspace application (open EPs, write descriptors, 
>> etc)
>>  - Bind gadget driver to DWC3's UDC
>>  - Switch DWC3 to host mode
>>    => dwc3_gadget_exit() is called. usb_del_gadget() will put the
>> 	ConfigFS driver instance on the gadget_driver_pending_list
>>  - Stop FunctionFS application (closes the ep files)
>>  - Switch DWC3 to peripheral mode
>>    => dwc3_gadget_init() fails as usb_add_gadget() calls
>> 	check_pending_gadget_drivers() and attempts to rebind the UDC
>> 	to the ConfigFS gadget but fails with -19 (-ENODEV) because the
>> 	FFS instance is not in FFS_READY state.
> 
> There is no such state at enum ffs_state, you mean flag desc_ready is 
> not
> true, right?

Ah sorry! I meant FFS_ACTIVE, but you are also correct, it would be 
equivalent to
ffs_dev::desc_ready. Shall I amend this commit description?

>>  - Switch DWC3 back to host mode
>>    => dwc3_gadget_exit() is called again, but this time dwc->gadget
>> 	is invalid.
>> 
>> Although it can be argued that userspace should take responsibility
>> for ensuring that the FunctionFS application be ready prior to
>> allowing the composite driver bind to the UDC, failure to do so
>> should not result in a panic from the kernel driver.
>> 
>> Fix this by setting dwc->gadget to NULL in the failure path of
>> dwc3_gadget_init() and add a check to dwc3_gadget_exit() to bail out
>> unless the gadget pointer is valid.
>> 
>> Fixes: e81a7018d93a ("usb: dwc3: allocate gadget structure 
>> dynamically")
>> Cc: <stable@vger.kernel.org>
>> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> 
> Reviewed-by: Peter Chen <peter.chen@kernel.org>

Thank you Peter!

>> ---
>> Hi Felipe,
>> 
>> Although I marked the 'Fixes' tag above to e81a7018d93a, the problem
>> theoretically exists prior to Peter's change. But I'm not sure how
>> best to fix on versions prior to this change since dwc->gadget used
>> to be an embedded struct so we can't do a simple NULL check as below.
>> Suggestions on alternative approaches welcome if we want to proceed
>> with backporting to older (pre-5.9) stable releases.
>> 
>> Thanks,
>> Jack
>> 
>>  drivers/usb/dwc3/gadget.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 612825a39f82..65d9b7227752 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -4046,6 +4046,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>>  	dwc3_gadget_free_endpoints(dwc);
>>  err4:
>>  	usb_put_gadget(dwc->gadget);
>> +	dwc->gadget = NULL;
>>  err3:
>>  	dma_free_coherent(dwc->sysdev, DWC3_BOUNCE_SIZE, dwc->bounce,
>>  			dwc->bounce_addr);
>> @@ -4065,6 +4066,9 @@ int dwc3_gadget_init(struct dwc3 *dwc)
>> 
>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>  {
>> +	if (!dwc->gadget)
>> +		return;
>> +
>>  	usb_del_gadget(dwc->gadget);
>>  	dwc3_gadget_free_endpoints(dwc);
>>  	usb_put_gadget(dwc->gadget);
>> --
>> 2.24.0
>>