linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Stern <stern@rowland.harvard.edu>
To: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Roger Quadros <rogerq@ti.com>, Peter Chen <peter.chen@nxp.com>,
	Anton Vasilyev <vasilyev@ispras.ru>,
	Evgeny Novikov <novikov@ispras.ru>,
	Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	USB mailing list <linux-usb@vger.kernel.org>
Subject: [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory
Date: Wed, 29 Jul 2020 16:22:31 -0400	[thread overview]
Message-ID: <20200729202231.GB1584059@rowland.harvard.edu> (raw)

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);
 

             reply	other threads:[~2020-07-29 20:22 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29 20:22 Alan Stern [this message]
2020-07-30  3:28 ` [PATCH RFC 1/4] USB: UDC: Don't wipe deallocated memory Peter Chen
2020-07-30  5:19   ` Greg KH
2020-07-30  7:31     ` Peter Chen
2020-07-30 15:07     ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200729202231.GB1584059@rowland.harvard.edu \
    --to=stern@rowland.harvard.edu \
    --cc=benh@kernel.crashing.org \
    --cc=felipe.balbi@linux.intel.com \
    --cc=linux-usb@vger.kernel.org \
    --cc=novikov@ispras.ru \
    --cc=peter.chen@nxp.com \
    --cc=rogerq@ti.com \
    --cc=vasilyev@ispras.ru \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).