[PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[Alan Stern]

Abusing the kernel's device model, some UDC drivers (including
dwc3 and cdns3) register and unregister their gadget structures
multiple times.  This is strictly forbidden; device structures may not
be reused.

Commit fac323471df6 ("usb: udc: allow adding and removing the same
gadget device") attempted to work around this restriction by zeroing
out the memory occupied by a gadget's embedded struct device when the
gadget is unregistered.  However, it does so at the wrong time,
immediately following the call to device_unregister().  At that point
there may still be outstanding references to the device, and
overwriting its memory is likely to cause trouble.  Even worse, if
there are no outstanding references then the gadget's memory may have
been deallocated, and so gadget must be considered to be a stale
pointer when it is passed to memset().

This patch fixes the problem by moving the offending memset to the
device's release routine, which gets called only when all references
have been dropped.  (Actually the call gets moved to the default
release routine, renamed from "usb_udc_nop_release" to
"usb_udc_zero_release" to indicate that it now zeroes out the memory.
This routine is the one used by dwc3 and cdns3; other drivers may not
use it.)

This doesn't fix the underlying problem.  UDC drivers that register
their gadgets multiple times should be rewritten to allocate their
gadget structures dynamically, using a new one each time.  Until that
is done, this will at least remove one potential source of errors.

On the other hand, the patch may create new errors if the release
routine doesn't get called until after the gadget has been
re-registered.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
CC: Roger Quadros <rogerq@ti.com>
CC: Peter Chen <peter.chen@nxp.com>
CC: Anton Vasilyev <vasilyev@ispras.ru>
CC: Evgeny Novikov <novikov@ispras.ru>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>

---

 drivers/usb/gadget/udc/core.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Index: usb-devel/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-devel.orig/drivers/usb/gadget/udc/core.c
+++ usb-devel/drivers/usb/gadget/udc/core.c
@@ -1138,9 +1138,10 @@ static void usb_udc_release(struct devic
 
 static const struct attribute_group *usb_udc_attr_groups[];
 
-static void usb_udc_nop_release(struct device *dev)
+static void usb_udc_zero_release(struct device *dev)
 {
 	dev_vdbg(dev, "%s\n", __func__);
+	memset(dev, 0, sizeof(*dev));
 }
 
 /* should be called with udc_lock held */
@@ -1184,7 +1185,7 @@ int usb_add_gadget_udc_release(struct de
 	if (release)
 		gadget->dev.release = release;
 	else
-		gadget->dev.release = usb_udc_nop_release;
+		gadget->dev.release = usb_udc_zero_release;
 
 	device_initialize(&gadget->dev);
 
@@ -1338,7 +1339,6 @@ void usb_del_gadget_udc(struct usb_gadge
 	flush_work(&gadget->work);
 	device_unregister(&udc->dev);
 	device_unregister(&gadget->dev);
-	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
 }
 EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
 

Re: [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[Peter Chen]

On 20-07-29 16:22:31, Alan Stern wrote:
> Abusing the kernel's device model, some UDC drivers (including
> dwc3 and cdns3) register and unregister their gadget structures
> multiple times.  This is strictly forbidden; device structures may not
> be reused.

Register and unregister gadget structures multiple times should be
allowed if we pass a clean (zeroed) gadget device structure. I checked
the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget
before calling usb_add_gadget_udc when start device mode.

> 
> Commit fac323471df6 ("usb: udc: allow adding and removing the same
> gadget device") attempted to work around this restriction by zeroing
> out the memory occupied by a gadget's embedded struct device when the
> gadget is unregistered.  However, it does so at the wrong time,
> immediately following the call to device_unregister().  At that point
> there may still be outstanding references to the device, and
> overwriting its memory is likely to cause trouble.  Even worse, if
> there are no outstanding references then the gadget's memory may have
> been deallocated, and so gadget must be considered to be a stale
> pointer when it is passed to memset().
> 
> This patch fixes the problem by moving the offending memset to the
> device's release routine, which gets called only when all references
> have been dropped.  (Actually the call gets moved to the default
> release routine, renamed from "usb_udc_nop_release" to
> "usb_udc_zero_release" to indicate that it now zeroes out the memory.
> This routine is the one used by dwc3 and cdns3; other drivers may not
> use it.)

In fact, many new written UDC drivers uses usb_add_gadget_udc directly
which uses default .release function defined at core.c.

> 
> This doesn't fix the underlying problem.  UDC drivers that register
> their gadgets multiple times should be rewritten to allocate their
> gadget structures dynamically, using a new one each time.  Until that
> is done, this will at least remove one potential source of errors.
> 
> On the other hand, the patch may create new errors if the release
> routine doesn't get called until after the gadget has been
> re-registered.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> CC: Roger Quadros <rogerq@ti.com>
> CC: Peter Chen <peter.chen@nxp.com>
> CC: Anton Vasilyev <vasilyev@ispras.ru>
> CC: Evgeny Novikov <novikov@ispras.ru>
> CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> 
> ---
> 
>  drivers/usb/gadget/udc/core.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> Index: usb-devel/drivers/usb/gadget/udc/core.c
> ===================================================================
> --- usb-devel.orig/drivers/usb/gadget/udc/core.c
> +++ usb-devel/drivers/usb/gadget/udc/core.c
> @@ -1138,9 +1138,10 @@ static void usb_udc_release(struct devic
>  
>  static const struct attribute_group *usb_udc_attr_groups[];
>  
> -static void usb_udc_nop_release(struct device *dev)
> +static void usb_udc_zero_release(struct device *dev)
>  {
>  	dev_vdbg(dev, "%s\n", __func__);
> +	memset(dev, 0, sizeof(*dev));
>  }
>  
>  /* should be called with udc_lock held */
> @@ -1184,7 +1185,7 @@ int usb_add_gadget_udc_release(struct de
>  	if (release)
>  		gadget->dev.release = release;
>  	else
> -		gadget->dev.release = usb_udc_nop_release;
> +		gadget->dev.release = usb_udc_zero_release;
>  
>  	device_initialize(&gadget->dev);

According to kernel-doc for device_initialize

 * All fields in @dev must be initialized by the caller to 0, except
 * for those explicitly set to some other value.  The simplest
 * approach is to use kzalloc() to allocate the structure containing
 * @dev.
 *

Is it better to zeroed gadget->dev before calling device_initialize?

>  
> @@ -1338,7 +1339,6 @@ void usb_del_gadget_udc(struct usb_gadge
>  	flush_work(&gadget->work);
>  	device_unregister(&udc->dev);
>  	device_unregister(&gadget->dev);
> -	memset(&gadget->dev, 0x00, sizeof(gadget->dev));
>  }
>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
>  

-- 

Thanks,
Peter Chen

Re: [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[Greg KH]

On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote:
> On 20-07-29 16:22:31, Alan Stern wrote:
> > Abusing the kernel's device model, some UDC drivers (including
> > dwc3 and cdns3) register and unregister their gadget structures
> > multiple times.  This is strictly forbidden; device structures may not
> > be reused.
> 
> Register and unregister gadget structures multiple times should be
> allowed if we pass a clean (zeroed) gadget device structure. I checked
> the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget
> before calling usb_add_gadget_udc when start device mode.

How do you "know" that the structure really was properly freed/released
by the driver core at that point in time?

That's the issue, even if you do unregister it, the driver core, or any
other part of the kernel, can hold on to the memory for an unbounded
amount of time, due to the fact that this is a reference counted
pointer.

So please, never "recycle" memory structures like this.  The
documentation for the kernel explicitly says "do not do this!"

thanks,

greg k-h

RE: [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[Peter Chen]

 
> On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote:
> > On 20-07-29 16:22:31, Alan Stern wrote:
> > > Abusing the kernel's device model, some UDC drivers (including
> > > dwc3 and cdns3) register and unregister their gadget structures
> > > multiple times.  This is strictly forbidden; device structures may
> > > not be reused.
> >
> > Register and unregister gadget structures multiple times should be
> > allowed if we pass a clean (zeroed) gadget device structure. I checked
> > the cdns3 code (cdns3_gadget_start), it always zeroed struct
> > usb_gadget before calling usb_add_gadget_udc when start device mode.
> 
> How do you "know" that the structure really was properly freed/released by the
> driver core at that point in time?
> 
> That's the issue, even if you do unregister it, the driver core, or any other part of
> the kernel, can hold on to the memory for an unbounded amount of time, due to the
> fact that this is a reference counted pointer.
> 

Yes, I find many UDC drivers have issues for that. The UDC driver's .remove has
no sync with device reference counter for gadget device, they free device private
structure which contains gadget device unconditional without considering device
reference counter for gadget device.

The UDC driver may need to call get_device/put_device for &gadget->dev to sync
with device core, and at gadget->dev .release callback, free the whole device's
private structure. So,  a common .release (usb_udc_nop_release) callback at 
core.c may not suitable for most of UDC driver.

> So please, never "recycle" memory structures like this.  The documentation for the
> kernel explicitly says "do not do this!"
 
Yes, "recycle" memory structures have issue, but at cdns3 driver(cdns3/gadget.c), it does not
use the same memory, it always allocates a NEW memory region for device structure before adding
to device core.

Peter

Re: [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory

[Alan Stern]

On Thu, Jul 30, 2020 at 07:19:04AM +0200, Greg KH wrote:
> On Thu, Jul 30, 2020 at 03:28:09AM +0000, Peter Chen wrote:
> > On 20-07-29 16:22:31, Alan Stern wrote:
> > > Abusing the kernel's device model, some UDC drivers (including
> > > dwc3 and cdns3) register and unregister their gadget structures
> > > multiple times.  This is strictly forbidden; device structures may not
> > > be reused.
> > 
> > Register and unregister gadget structures multiple times should be
> > allowed if we pass a clean (zeroed) gadget device structure. I checked
> > the cdns3 code (cdns3_gadget_start), it always zeroed struct usb_gadget
> > before calling usb_add_gadget_udc when start device mode.
> 
> How do you "know" that the structure really was properly freed/released
> by the driver core at that point in time?
> 
> That's the issue, even if you do unregister it, the driver core, or any
> other part of the kernel, can hold on to the memory for an unbounded
> amount of time, due to the fact that this is a reference counted
> pointer.

In theory, you can know that the driver core is done using a structure 
if you wait for the release routine to run.  But of course, that can 
mean you have to wait an indefinitely long time.

Alan Stern

> So please, never "recycle" memory structures like this.  The
> documentation for the kernel explicitly says "do not do this!"
> 
> thanks,
> 
> greg k-h