Skip to contentSkip to conversation

USB: gadget: Fix UAF on refcount inside gadgetfs_bind()

PATCH In review Rafael Alejandro Diaz Cruz 2 participants 2 messages Sep 5 – Oct 18

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH v2 1/1 USB: gadget: Fix UAF on refcount inside gadgetfs_bind()

When usb_ep_alloc_request() fails, gadgetfs_bind() takes the following error path:

if (!dev->req)

        goto enomem;

// ...

enomem:

	gadgetfs_unbind (gadget);
	return -ENOMEM;

In normal case, get_dev() will be called to increment refcounter from gadget->dev->count but in error case, this increment is skipped and gadgetfs_unbind() will decrement it via put_dev().

Going down the error path leads to a refcount imbalance which will cause UAF in close()/umount() operations due to refcount dropping below 0.

Fix this by moving get_dev() above the error paths so that refcounter is incremented before the put_dev() call.

---
Changes from v1:
Removed syzkaller reference in description

Added Reviewed-by: tag.

 drivers/usb/gadget/legacy/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/legacy/inode.c +1 -1
index 3e2bce7543d4..181b19e7fc2f 100644
@@ -1679,6 +1679,7 @@ static int gadgetfs_bind(struct usb_gadget *gadget,
 	}
 
 	set_gadget_data (gadget, dev);
+	get_dev(dev);
 	dev->gadget = gadget;
 	gadget->ep0->driver_data = dev;
 
@@ -1696,7 +1697,6 @@ static int gadgetfs_bind(struct usb_gadget *gadget,
 	spin_lock_irq(&dev->lock);
 	dev->state = STATE_DEV_UNCONNECTED;
 	spin_unlock_irq(&dev->lock);
-	get_dev (dev);
 	return 0;
 
 enomem:
Signature · 1 line
2.43.0
maintainer
#1 lore raw
Quoted 62 lines · Rafael Alejandro Diaz Cruz

When usb_ep_alloc_request() fails, gadgetfs_bind() takes the following error path:

if (!dev->req)

        goto enomem;

// ...

enomem:

	gadgetfs_unbind (gadget);
	return -ENOMEM;

In normal case, get_dev() will be called to increment refcounter from gadget->dev->count but in error case, this increment is skipped and gadgetfs_unbind() will decrement it via put_dev().

Going down the error path leads to a refcount imbalance which will cause UAF in close()/umount() operations due to refcount dropping below 0.

Fix this by moving get_dev() above the error paths so that refcounter is incremented before the put_dev() call.

---
Changes from v1:
Removed syzkaller reference in description

Added Reviewed-by: tag.

 drivers/usb/gadget/legacy/inode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/usb/gadget/legacy/inode.c +1 -1
index 3e2bce7543d4..181b19e7fc2f 100644
@@ -1679,6 +1679,7 @@ static int gadgetfs_bind(struct usb_gadget *gadget,
 	}
 
 	set_gadget_data (gadget, dev);
+	get_dev(dev);
 	dev->gadget = gadget;
 	gadget->ep0->driver_data = dev;
 
@@ -1696,7 +1697,6 @@ static int gadgetfs_bind(struct usb_gadget *gadget,
 	spin_lock_irq(&dev->lock);
 	dev->state = STATE_DEV_UNCONNECTED;
 	spin_unlock_irq(&dev->lock);
-	get_dev (dev);
 	return 0;
 
 enomem:
Signature · 1 line
2.43.0

I think
https://lore.kernel.org/r/[email protected]
was send before yours.

thanks,

greg k-h