diff mbox series

[v3,2/2] usb: gadget: clear related members when goto fail

Message ID 20211230051132.21056-3-hbh25y@gmail.com
State New
Headers show
Series usb: gadget: use after free in dev_config | expand

Commit Message

Hangyu Hua Dec. 30, 2021, 5:11 a.m. UTC 
dev->config and dev->hs_config and dev->dev need to be cleaned if
dev_config fails to avoid UAF.

Acked-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Hangyu Hua <hbh25y@gmail.com>
---
 drivers/usb/gadget/legacy/inode.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

Comments

Alan Stern Dec. 31, 2021, 3:57 p.m. UTC | #1 
On Fri, Dec 31, 2021 at 10:31:51AM +0800, Hangyu Hua wrote:
> On 2021/12/31 上午3:46, Alan Stern wrote:

> >> @@ -1892,7 +1895,12 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
> >>      }
> >>      return value;
> >>
> >> +fail2:
> >> +    dev->dev = NULL;
> >> +fail1:
> >> +    dev->hs_config = NULL;
> >
> > It is not necessary to have all these different statement labels.  You
> > can simply have "fail:" clear all three pointers.

> I don't think so. It is not necessary to clean all three pointers if
> some of them aren't kbuf. I think it may be better to keep their own
> pointers.

If the pointers aren't set to a region inside kbuf then they are 
meaningless.  There is no reason to keep the old values.  It is better 
to avoid multiple unnecessary statement labels.

Alan Stern
diff mbox series

Patch

diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
index eaad03c0252f..d2e88f3b9131 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -1847,7 +1847,7 @@  dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 		total = le16_to_cpu(dev->hs_config->wTotalLength);
 		if (!is_valid_config(dev->hs_config, total) ||
 				total > length - USB_DT_DEVICE_SIZE)
-			goto fail;
+			goto fail1;
 		kbuf += total;
 		length -= total;
 	} else {
@@ -1858,12 +1858,12 @@  dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 
 	/* device descriptor (tweaked for paranoia) */
 	if (length != USB_DT_DEVICE_SIZE)
-		goto fail;
+		goto fail1;
 	dev->dev = (void *)kbuf;
 	if (dev->dev->bLength != USB_DT_DEVICE_SIZE
 			|| dev->dev->bDescriptorType != USB_DT_DEVICE
 			|| dev->dev->bNumConfigurations != 1)
-		goto fail;
+		goto fail2;
 	dev->dev->bcdUSB = cpu_to_le16 (0x0200);
 
 	/* triggers gadgetfs_bind(); then we can enumerate. */
@@ -1875,6 +1875,9 @@  dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 
 	value = usb_gadget_probe_driver(&gadgetfs_driver);
 	if (value != 0) {
+		dev->dev = NULL;
+		dev->hs_config = NULL;
+		dev->config = NULL;
 		kfree (dev->buf);
 		dev->buf = NULL;
 	} else {
@@ -1892,7 +1895,12 @@  dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
 	}
 	return value;
 
+fail2:
+	dev->dev = NULL;
+fail1:
+	dev->hs_config = NULL;
 fail:
+	dev->config = NULL;
 	spin_unlock_irq (&dev->lock);
 	pr_debug ("%s: %s fail %zd, %p\n", shortname, __func__, value, dev);
 	kfree (dev->buf);