diff mbox series

USB: gadget: Fix use-after-free Read in usb_udc_uevent()

Message ID YtlrnhHyrHsSky9m@rowland.harvard.edu
State New
Headers show
Series USB: gadget: Fix use-after-free Read in usb_udc_uevent() | expand

Commit Message

Alan Stern July 21, 2022, 3:07 p.m. UTC
The syzbot fuzzer found a race between uevent callbacks and gadget
driver unregistration that can cause a use-after-free bug:

---------------------------------------------------------------
BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130
drivers/usb/gadget/udc/core.c:1732
Read of size 8 at addr ffff888078ce2050 by task udevd/2968

CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
06/29/2022
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
 print_address_description mm/kasan/report.c:317 [inline]
 print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
 kasan_report+0xbe/0x1f0 mm/kasan/report.c:495
 usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732
 dev_uevent+0x290/0x770 drivers/base/core.c:2424
---------------------------------------------------------------

The bug occurs because usb_udc_uevent() dereferences udc->driver but
does so without acquiring the udc_lock mutex, which protects this
field.  If the gadget driver is unbound from the udc concurrently with
uevent processing, the driver structure may be accessed after it has
been deallocated.

To prevent the race, we make sure that the routine holds the mutex
around the racing accesses.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com
CC: stable@vger.kernel.org
Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com>

---

As far as I can tell, this bug has always been present.  However, the
udc_lock mutex used by the patch was added in commit fc274c1e9973
("USB: gadget: Add a new bus for gadgets"), so this patch won't apply
to trees which don't include that commit or a backport of it.  I don't
know what tag, if any, can express this requirement.


[as1983]


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

Comments

Greg KH July 27, 2022, 12:17 p.m. UTC | #1
On Thu, Jul 21, 2022 at 11:07:10AM -0400, Alan Stern wrote:
> The syzbot fuzzer found a race between uevent callbacks and gadget
> driver unregistration that can cause a use-after-free bug:
> 
> ---------------------------------------------------------------
> BUG: KASAN: use-after-free in usb_udc_uevent+0x11f/0x130
> drivers/usb/gadget/udc/core.c:1732
> Read of size 8 at addr ffff888078ce2050 by task udevd/2968
> 
> CPU: 1 PID: 2968 Comm: udevd Not tainted 5.19.0-rc4-next-20220628-syzkaller #0
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google
> 06/29/2022
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:88 [inline]
>  dump_stack_lvl+0xcd/0x134 lib/dump_stack.c:106
>  print_address_description mm/kasan/report.c:317 [inline]
>  print_report.cold+0x2ba/0x719 mm/kasan/report.c:433
>  kasan_report+0xbe/0x1f0 mm/kasan/report.c:495
>  usb_udc_uevent+0x11f/0x130 drivers/usb/gadget/udc/core.c:1732
>  dev_uevent+0x290/0x770 drivers/base/core.c:2424
> ---------------------------------------------------------------
> 
> The bug occurs because usb_udc_uevent() dereferences udc->driver but
> does so without acquiring the udc_lock mutex, which protects this
> field.  If the gadget driver is unbound from the udc concurrently with
> uevent processing, the driver structure may be accessed after it has
> been deallocated.
> 
> To prevent the race, we make sure that the routine holds the mutex
> around the racing accesses.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-and-tested-by: syzbot+b0de012ceb1e2a97891b@syzkaller.appspotmail.com
> CC: stable@vger.kernel.org
> Link: <https://lore.kernel.org/all/0000000000004de90405a719c951@google.com>
> 
> ---
> 
> As far as I can tell, this bug has always been present.  However, the
> udc_lock mutex used by the patch was added in commit fc274c1e9973
> ("USB: gadget: Add a new bus for gadgets"), so this patch won't apply
> to trees which don't include that commit or a backport of it.  I don't
> know what tag, if any, can express this requirement.

As per the stable_kernel_rules document, you can say:
	cc: stable@vger.kernel.org	# fc274c1e9973

and I should hopefully figure it out :)

I'll add that here and see how well it works...

thanks,

greg k-h
Alan Stern Aug. 10, 2022, 7:33 p.m. UTC | #2
On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:
> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
> > Hi Alan,
> 
> Hi.
> 
> > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: 
> > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it 
> > fixes the issue by introducing another one. It doesn't look very 
> > probable, but it would be nice to fix it to make the lock dependency 
> > checker happy.
> 
> Indeed.

> I suspect the problem is that udc_lock is held for too long.  Probably it 
> should be released during the calls to udc->driver->bind and 
> udc->driver->unbind.
> 
> Getting this right will require some careful study.  Marek, if I send you 
> a patch later, will you be able to test it?

Here's a patch for you to try, when you have the chance.  It reduces the 
scope of udc_lock to cover only the fields it's supposed to protect and 
changes the locking in a few other places.

There's still the possibility of a locking cycle, because udc_lock is 
held in the ->disconnect pathway.  It's very hard to know whether that 
might cause any trouble; it depends on how the function drivers handle 
disconnections.

Alan Stern



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
@@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad
 	ret = gadget->ops->pullup(gadget, 0);
 	if (!ret) {
 		gadget->connected = 0;
-		gadget->udc->driver->disconnect(gadget);
+		mutex_lock(&udc_lock);
+		if (gadget->udc->driver)
+			gadget->udc->driver->disconnect(gadget);
+		mutex_unlock(&udc_lock);
 	}
 
 out:
@@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev
 
 	usb_gadget_udc_set_speed(udc, driver->max_speed);
 
-	mutex_lock(&udc_lock);
 	ret = driver->bind(udc->gadget, driver);
 	if (ret)
 		goto err_bind;
@@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev
 		goto err_start;
 	usb_gadget_enable_async_callbacks(udc);
 	usb_udc_connect_control(udc);
-	mutex_unlock(&udc_lock);
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 	return 0;
@@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev
 		dev_err(&udc->dev, "failed to start %s: %d\n",
 			driver->function, ret);
 
+	mutex_lock(&udc_lock);
 	udc->driver = NULL;
 	driver->is_bound = false;
 	mutex_unlock(&udc_lock);
@@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct
 
 	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
 
-	mutex_lock(&udc_lock);
 	usb_gadget_disconnect(gadget);
 	usb_gadget_disable_async_callbacks(udc);
 	if (gadget->irq)
@@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct
 	udc->driver->unbind(gadget);
 	usb_gadget_udc_stop(udc);
 
+	mutex_lock(&udc_lock);
 	driver->is_bound = false;
 	udc->driver = NULL;
 	mutex_unlock(&udc_lock);
@@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
 	ssize_t			ret;
 
-	mutex_lock(&udc_lock);
+	device_lock(&udc->gadget->dev);
 	if (!udc->driver) {
 		dev_err(dev, "soft-connect without a gadget driver\n");
 		ret = -EOPNOTSUPP;
@@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct
 
 	ret = n;
 out:
-	mutex_unlock(&udc_lock);
+	device_unlock(&udc->gadget->dev);
 	return ret;
 }
 static DEVICE_ATTR_WO(soft_connect);
@@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi
 			     char *buf)
 {
 	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
-	struct usb_gadget_driver *drv = udc->driver;
+	struct usb_gadget_driver *drv;
+	int			rc = 0;
 
-	if (!drv || !drv->function)
-		return 0;
-	return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function);
+	mutex_lock(&udc_lock);
+	drv = udc->driver;
+	if (drv && drv->function)
+		rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function);
+	mutex_unlock(&udc_lock);
+	return rc;
 }
 static DEVICE_ATTR_RO(function);
Marek Szyprowski Aug. 11, 2022, 7:31 a.m. UTC | #3
Hi Alan,

On 10.08.2022 21:33, Alan Stern wrote:
> On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:
>> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
>>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB:
>>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it
>>> fixes the issue by introducing another one. It doesn't look very
>>> probable, but it would be nice to fix it to make the lock dependency
>>> checker happy.
>> Indeed.
>> I suspect the problem is that udc_lock is held for too long.  Probably it
>> should be released during the calls to udc->driver->bind and
>> udc->driver->unbind.
>>
>> Getting this right will require some careful study.  Marek, if I send you
>> a patch later, will you be able to test it?
> Here's a patch for you to try, when you have the chance.  It reduces the
> scope of udc_lock to cover only the fields it's supposed to protect and
> changes the locking in a few other places.
>
> There's still the possibility of a locking cycle, because udc_lock is
> held in the ->disconnect pathway.  It's very hard to know whether that
> might cause any trouble; it depends on how the function drivers handle
> disconnections.

It looks this fixed the issue I've reported. I've checked it on all my 
test systems and none reported any issue related to the udc.

Feel free to add:

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>


> Alan Stern
>
>
>
> 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
> @@ -736,7 +736,10 @@ int usb_gadget_disconnect(struct usb_gad
>   	ret = gadget->ops->pullup(gadget, 0);
>   	if (!ret) {
>   		gadget->connected = 0;
> -		gadget->udc->driver->disconnect(gadget);
> +		mutex_lock(&udc_lock);
> +		if (gadget->udc->driver)
> +			gadget->udc->driver->disconnect(gadget);
> +		mutex_unlock(&udc_lock);
>   	}
>   
>   out:
> @@ -1489,7 +1492,6 @@ static int gadget_bind_driver(struct dev
>   
>   	usb_gadget_udc_set_speed(udc, driver->max_speed);
>   
> -	mutex_lock(&udc_lock);
>   	ret = driver->bind(udc->gadget, driver);
>   	if (ret)
>   		goto err_bind;
> @@ -1499,7 +1501,6 @@ static int gadget_bind_driver(struct dev
>   		goto err_start;
>   	usb_gadget_enable_async_callbacks(udc);
>   	usb_udc_connect_control(udc);
> -	mutex_unlock(&udc_lock);
>   
>   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>   	return 0;
> @@ -1512,6 +1513,7 @@ static int gadget_bind_driver(struct dev
>   		dev_err(&udc->dev, "failed to start %s: %d\n",
>   			driver->function, ret);
>   
> +	mutex_lock(&udc_lock);
>   	udc->driver = NULL;
>   	driver->is_bound = false;
>   	mutex_unlock(&udc_lock);
> @@ -1529,7 +1531,6 @@ static void gadget_unbind_driver(struct
>   
>   	kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE);
>   
> -	mutex_lock(&udc_lock);
>   	usb_gadget_disconnect(gadget);
>   	usb_gadget_disable_async_callbacks(udc);
>   	if (gadget->irq)
> @@ -1537,6 +1538,7 @@ static void gadget_unbind_driver(struct
>   	udc->driver->unbind(gadget);
>   	usb_gadget_udc_stop(udc);
>   
> +	mutex_lock(&udc_lock);
>   	driver->is_bound = false;
>   	udc->driver = NULL;
>   	mutex_unlock(&udc_lock);
> @@ -1612,7 +1614,7 @@ static ssize_t soft_connect_store(struct
>   	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
>   	ssize_t			ret;
>   
> -	mutex_lock(&udc_lock);
> +	device_lock(&udc->gadget->dev);
>   	if (!udc->driver) {
>   		dev_err(dev, "soft-connect without a gadget driver\n");
>   		ret = -EOPNOTSUPP;
> @@ -1633,7 +1635,7 @@ static ssize_t soft_connect_store(struct
>   
>   	ret = n;
>   out:
> -	mutex_unlock(&udc_lock);
> +	device_unlock(&udc->gadget->dev);
>   	return ret;
>   }
>   static DEVICE_ATTR_WO(soft_connect);
> @@ -1652,11 +1654,15 @@ static ssize_t function_show(struct devi
>   			     char *buf)
>   {
>   	struct usb_udc		*udc = container_of(dev, struct usb_udc, dev);
> -	struct usb_gadget_driver *drv = udc->driver;
> +	struct usb_gadget_driver *drv;
> +	int			rc = 0;
>   
> -	if (!drv || !drv->function)
> -		return 0;
> -	return scnprintf(buf, PAGE_SIZE, "%s\n", drv->function);
> +	mutex_lock(&udc_lock);
> +	drv = udc->driver;
> +	if (drv && drv->function)
> +		rc = scnprintf(buf, PAGE_SIZE, "%s\n", drv->function);
> +	mutex_unlock(&udc_lock);
> +	return rc;
>   }
>   static DEVICE_ATTR_RO(function);
>   
>
Best regards
Alan Stern Aug. 11, 2022, 4:06 p.m. UTC | #4
On Thu, Aug 11, 2022 at 09:31:34AM +0200, Marek Szyprowski wrote:
> Hi Alan,
> 
> On 10.08.2022 21:33, Alan Stern wrote:
> > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:
> >> On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
> >>> This patch landed recently in linux-next as commit 2191c00855b0 ("USB:
> >>> gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it
> >>> fixes the issue by introducing another one. It doesn't look very
> >>> probable, but it would be nice to fix it to make the lock dependency
> >>> checker happy.
> >> Indeed.
> >> I suspect the problem is that udc_lock is held for too long.  Probably it
> >> should be released during the calls to udc->driver->bind and
> >> udc->driver->unbind.
> >>
> >> Getting this right will require some careful study.  Marek, if I send you
> >> a patch later, will you be able to test it?
> > Here's a patch for you to try, when you have the chance.  It reduces the
> > scope of udc_lock to cover only the fields it's supposed to protect and
> > changes the locking in a few other places.
> >
> > There's still the possibility of a locking cycle, because udc_lock is
> > held in the ->disconnect pathway.  It's very hard to know whether that
> > might cause any trouble; it depends on how the function drivers handle
> > disconnections.
> 
> It looks this fixed the issue I've reported. I've checked it on all my 
> test systems and none reported any issue related to the udc.
> 
> Feel free to add:
> 
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

Thanks for the quick testing.  I'll submit the patch when the current 
merge window ends.

Alan Stern
Alan Stern Sept. 1, 2022, 7:29 p.m. UTC | #5
On Thu, Sep 01, 2022 at 09:22:04PM +0200, Francesco Dolcini wrote:
> On Wed, Aug 10, 2022 at 03:33:00PM -0400, Alan Stern wrote:
> > On Mon, Aug 08, 2022 at 04:26:49PM -0400, Alan Stern wrote:
> > > On Mon, Aug 08, 2022 at 04:57:35PM +0200, Marek Szyprowski wrote:
> > > > Hi Alan,
> > > 
> > > Hi.
> > > 
> > > > This patch landed recently in linux-next as commit 2191c00855b0 ("USB: 
> > > > gadget: Fix use-after-free Read in usb_udc_uevent()"). Unfortunately it 
> > > > fixes the issue by introducing another one. It doesn't look very 
> > > > probable, but it would be nice to fix it to make the lock dependency 
> > > > checker happy.
> 
> Hi just bisected a different warning [1], triggered by the same patch and I
> was about to send a revert while I noticed you already have a fix more
> or less ready, it would be great if you could send it. Please find my
> tested-by afterward.

It has already been submitted:

https://lore.kernel.org/linux-usb/YwkfhdxA%2FI2nOcK7@rowland.harvard.edu/

Greg hasn't merged it yet, though.

Alan Stern
diff mbox series

Patch

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
@@ -1728,13 +1728,14 @@  static int usb_udc_uevent(struct device
 		return ret;
 	}
 
-	if (udc->driver) {
+	mutex_lock(&udc_lock);
+	if (udc->driver)
 		ret = add_uevent_var(env, "USB_UDC_DRIVER=%s",
 				udc->driver->function);
-		if (ret) {
-			dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n");
-			return ret;
-		}
+	mutex_unlock(&udc_lock);
+	if (ret) {
+		dev_err(dev, "failed to add uevent USB_UDC_DRIVER\n");
+		return ret;
 	}
 
 	return 0;