usb: gadget: use after free in dev_config
v3 cover usb: gadget: use after free in dev_config
There are two bugs:dev->buf does not need to be released if it already exists before executing dev_config.dev->config and dev->hs_config and dev->dev need to be cleaned if dev_config fails to avoid UAF.
v2:
1. break one patch up into two separate patches.
2. use "fail:" to clear all members.
v3:
fix a mistake in [PATCH v3 2/2]
Hangyu Hua (2):
usb: gadget: don't release an existing dev->buf
usb: gadget: clear related members when goto fail
drivers/usb/gadget/legacy/inode.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)
Signature · 1 line
2.25.1
PATCH v3 1/2 usb: gadget: don't release an existing dev->buf
dev->buf does not need to be released if it already exists before executing dev_config.
--- drivers/usb/gadget/legacy/inode.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/usb/gadget/legacy/inode.c +2 -1
@@ -1826,8 +1826,9 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)spin_lock_irq (&dev->lock); value = -EINVAL ;if (dev->buf) { +spin_unlock_irq (&dev->lock);kfree (kbuf); - goto fail; +return value; } dev->buf = kbuf;
Signature · 1 line
2.25.1
PATCH v3 2/2 usb: gadget: clear related members when goto fail
In reply to Hangyu Huadev->config and dev->hs_config and dev->dev need to be cleaned if dev_config fails to avoid UAF.
--- drivers/usb/gadget/legacy/inode.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/usb/gadget/legacy/inode.c +11 -3
@@ -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);
Signature · 1 line
2.25.1
Re: [PATCH v3 2/2] usb: gadget: clear related members when goto fail
Quoted 4 lines · Hangyu Hua
dev->configanddev->hs_configanddev->devneed to be cleaned ifdev_configfails to avoid UAF.Acked-by Alan Stern <[email protected]>
You must not do this. I never sent you an Acked-by for this patch; you shouldn't claim that I did.
Quoted 34 lines
Signed-off-by Hangyu Hua <[email protected]>--- drivers/usb/gadget/legacy/inode.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/usb/gadget/legacy/inode.c +3
-3@@ -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. */
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1882
Quoted 8 lines
drivers/usb/gadget/legacy/inode.c +3
-0@@ -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 ;
Why not just grep the lock and goto fail?
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1901
Quoted 9 lines
drivers/usb/gadget/legacy/inode.c +4
-0}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 ;
It is not necessary to have all these different statement labels. You can simply have "fail:" clear all three pointers.
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1906
Quoted 5 lines
drivers/usb/gadget/legacy/inode.c +1
-0fail: + dev->config =NULL ;spin_unlock_irq (&dev->lock);pr_debug ("%s: %s fail %zd, %p\n" , shortname, __func__, value, dev);kfree (dev->buf);
Alan Stern
Re: [PATCH v3 2/2] usb: gadget: clear related members when goto fail
Quoted 6 lines · Alan Stern
Quoted 5 lines · Hangyu Hua
dev->configanddev->hs_configanddev->devneed to be cleaned ifdev_configfails to avoid UAF.(quoted diff) +0
-0@@ -1875,6 +1875,9 @@ dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr)
Quoted 9 lines
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1882
Quoted 7 lines
drivers/usb/gadget/legacy/inode.c +3
-0value =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 ;Why not just grep the lock and goto fail?
Wrong word: I meant to write "grab", not "grep".
Alan Stern
Re: [PATCH v3 2/2] usb: gadget: clear related members when goto fail
In reply to Alan SternQuoted 2 lines · Alan Stern
You must not do this. I never sent you an Acked-by for this patch; you shouldn't claim that I did.
I am sorry about this. I should read the linux kernel community rules more carefully.
Quoted 8 lines
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1882
Quoted 7 lines
value = usb_gadget_probe_driver(&gadgetfs_driver); if (value != 0) {drivers/usb/gadget/legacy/inode.c +3
-0+ dev->dev =NULL ; + dev->hs_config =NULL ; + dev->config =NULL ;kfree (dev->buf); dev->buf =NULL ;Why not just grep the lock and goto fail?
You are right. I will modify my patch later.
Quoted 13 lines
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1901
Quoted 9 lines
} else {drivers/usb/gadget/legacy/inode.c +4
-0@@ -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.
Quoted 8 lines
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1906
Quoted 5 lines
drivers/usb/gadget/legacy/inode.c +1
-0fail: + dev->config =NULL ;spin_unlock_irq (&dev->lock);pr_debug ("%s: %s fail %zd, %p\n" , shortname, __func__, value, dev);kfree (dev->buf);Alan Stern
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.
Thanks. Happy new year.
Re: [PATCH v3 2/2] usb: gadget: clear related members when goto fail
Quoted 14 lines · Hangyu Hua
Quoted 11 lines · Alan Stern
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1901
Quoted 8 lines
drivers/usb/gadget/legacy/inode.c +4
-0@@ -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.
Quoted 3 lines
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
Re: [PATCH v3 2/2] usb: gadget: clear related members when goto fail
I get that. I will fix and resubmit my patch.
Thanks again.
Quoted 25 lines · Alan Stern
Quoted 14 lines · Hangyu Hua
Quoted 11 lines · Alan Stern
drivers/usb/gadget/legacy/inode.c dev_config (struct file *fd, const char __user *buf, size_t len, loff_t *ptr) · line 1901
Quoted 8 lines
drivers/usb/gadget/legacy/inode.c +4
-0@@ -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.
Quoted 3 lines
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