usb: gadget: f_fs: Use local copy of descriptors for userspace copy

From: Vamsi Krishna Samavedam <[email protected]>

The function may be unbound causing the ffs_ep and its descriptors to be freed while userspace is in the middle of an ioctl requesting the same descriptors. Avoid dangling pointer reference by first making a local copy of desctiptors before releasing the spinlock.

Fixes c559a3534109 ("usb: gadget: f_fs: add ioctl returning ep descriptor")

Signed-off-by Vamsi Krishna Samavedam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +1 -1

index 046f770a76da..c727cb5de871 100644
@@ -1324,7 +1324,7 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 	case FUNCTIONFS_ENDPOINT_DESC:
 	{
 		int desc_idx;
-		struct usb_endpoint_descriptor *desc;
+		struct usb_endpoint_descriptor desc1, *desc;
 
 		switch (epfile->ffs->gadget->speed) {
 		case USB_SPEED_SUPER:

@@ -1336,10 +1336,12 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 		default:
 			desc_idx = 0;
 		}

Quoted 35 lines · Jack Pham

From: Vamsi Krishna Samavedam <[email protected]>

The function may be unbound causing the ffs_ep and its descriptors to be freed while userspace is in the middle of an ioctl requesting the same descriptors. Avoid dangling pointer reference by first making a local copy of desctiptors before releasing the spinlock.

Fixes c559a3534109 ("usb: gadget: f_fs: add ioctl returning ep descriptor")

Signed-off-by Vamsi Krishna Samavedam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +3 -1

index 046f770a76da..c727cb5de871 100644
@@ -1324,7 +1324,7 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 	case FUNCTIONFS_ENDPOINT_DESC:
 	{
 		int desc_idx;
-		struct usb_endpoint_descriptor *desc;
+		struct usb_endpoint_descriptor desc1, *desc;
 
 		switch (epfile->ffs->gadget->speed) {
 		case USB_SPEED_SUPER:

@@ -1336,10 +1336,12 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 		default:
 			desc_idx = 0;
 		}
+
 		desc = epfile->ep->descs[desc_idx];
+		memcpy(&desc1, desc, desc->bLength);
 

drivers/usb/gadget/function/f_fs.c static long ffs_epfile_ioctl(struct file *file, unsigned code, · line 1347

Quoted 7 lines

drivers/usb/gadget/function/f_fs.c +1 -1

 		spin_unlock_irq(&epfile->ffs->eps_lock);
-		ret = copy_to_user((void __user *)value, desc, desc->bLength);
+		ret = copy_to_user((void __user *)value, &desc1, desc1.bLength);
 		if (ret)
 			ret = -EFAULT;
 		return ret;

Do you have any backtrace to show the problems? I see ffs->ref will be increased at .open, and the .unbind should not free memory if ffs->ref is still two.

(removed Vamsi as he has moved on from USB and his email address is bouncing)

Hi Peter,

Quoted 36 lines · Peter Chen

Quoted 35 lines · Jack Pham

From: Vamsi Krishna Samavedam <[email protected]>

The function may be unbound causing the ffs_ep and its descriptors to be freed while userspace is in the middle of an ioctl requesting the same descriptors. Avoid dangling pointer reference by first making a local copy of desctiptors before releasing the spinlock.

Fixes c559a3534109 ("usb: gadget: f_fs: add ioctl returning ep descriptor")

Signed-off-by Vamsi Krishna Samavedam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +3 -1

index 046f770a76da..c727cb5de871 100644
@@ -1324,7 +1324,7 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 	case FUNCTIONFS_ENDPOINT_DESC:
 	{
 		int desc_idx;
-		struct usb_endpoint_descriptor *desc;
+		struct usb_endpoint_descriptor desc1, *desc;
 
 		switch (epfile->ffs->gadget->speed) {
 		case USB_SPEED_SUPER:

@@ -1336,10 +1336,12 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
 		default:
 			desc_idx = 0;
 		}
+
 		desc = epfile->ep->descs[desc_idx];
+		memcpy(&desc1, desc, desc->bLength);
 

Quoted 11 lines

drivers/usb/gadget/function/f_fs.c static long ffs_epfile_ioctl(struct file *file, unsigned code, · line 1347

Quoted 7 lines

drivers/usb/gadget/function/f_fs.c +1 -1

 		spin_unlock_irq(&epfile->ffs->eps_lock);
-		ret = copy_to_user((void __user *)value, desc, desc->bLength);
+		ret = copy_to_user((void __user *)value, &desc1, desc1.bLength);
 		if (ret)
 			ret = -EFAULT;
 		return ret;

Do you have any backtrace to show the problems? I see ffs->ref will be increased at .open, and the .unbind should not free memory if ffs->ref is still two.

kfree(func->eps) is getting called directly from ffs_func_unbind() which can happen when configfs.c does purge_configs_funcs(). ffs_func_unbind() does not check for ffs->refs count here, so it looks like it can proceed to free it as soon as it releases the eps_lock, while the ioctl happens in parallel and then accesses the now stale pointer after acquiring & releasing the same eps_lock.

But I think I see what you're getting at--would you suggest that we avoid freeing func->eps and tie its lifetime to ffs->refs? I agree in principle but it also looks a bit tricky as there seem to be several reference counters being used in this driver [ffs->refs (refcount_t); ffs->opened (atomic_t); ffs_opts->refcnt (unsigned)] that I have a little trouble figuring out which one to use. Appreciate any pointers if you have any.

Here is a quite old backtrace from an internal bug report which was on our downstream 4.14 kernel, but I think the issue can still happen on current mainline. Also I believe we saw this with ADB but the current AOSP version of ADB no longer uses the FUNCTIONFS_ENDPOINT_DESC ioctl() so it may not happen with this function. However there are other Android functions that use FFS (MTP, Fastboot) which still do use this ioctl so I believe it still has the potential to occur if ffs_func_unbind() races with ffs_epfile_ioctl().

init: processing action (sys.usb.config=none && sys.usb.configfs=1) from (/vendor/etc/init/hw/init.msm.usb.configfs.rc:30)
android_work: sent uevent USB_STATE=DISCONNECTED

Unable to handle kernel paging request at virtual address ffffffefa007fa57

Mem abort info:
Exception class = DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
FSC = 7
Data abort info:
ISV = 0, ISS = 0x00000007
CM = 0, WnR = 0
swapper pgtable: 4k pages, 39-bit VAs, pgd = ffffff956e787000
[ffffffefa007fa57] *pgd=00000001bd6e1003, *pud=00000001bd6e1003, *pmd=00000001bd5e0003, *pte=00e800016007f712

Internal error: Oops: 96000007 [#1] PREEMPT SMP
init: Command 'rm /config/usb_gadget/g1/configs/b.1/f1' action=sys.usb.config=none && sys.usb.configfs=1 (/vendor/etc/init/hw/init.msm.usb.configfs.rc:31) took 77ms and succeeded
init: processing action (sys.usb.config=none && sys.usb.configfs=1) from (/init.usb.configfs.rc:1)
init: Command 'write /config/usb_gadget/g1/UDC none' action=sys.usb.config=none && sys.usb.configfs=1 (/init.usb.configfs.rc:2) took 0ms and failed: Unable to write to file '/config/usb_gadget/g1/UDC': Unable to write file contents: No such device
Modules linked in: wlan(O) machine_dlkm(O) wcd934x_dlkm(O) mbhc_dlkm(O) wcd9360_dlkm(O) swr_ctrl_dlkm(O) wcd9xxx_dlkm(O) wsa881x_dlkm(O) wcd_core_dlkm(O) stub_dlkm(O) wcd_spi_dlkm(O) hdmi_dlkm(O) swr_dlkm(O) pinctrl_wcd_dlkm(O) usf_dlkm(O) native_dlkm(O) platform_dlkm(O) q6_dlkm(O) adsp_loader_dlkm(O) apr_dlkm(O) q6_notifier_dlkm(O) q6_pdr_dlkm(O) wglink_dlkm(O) msm_11ad_proxy
CPU: 6 PID: 13186 Comm: ->transport Tainted: G S      W  O    4.14.83+ #1
Hardware name: Qualcomm Technologies, Inc. SM8150 V2 PM8150 QRD DVT (DT)
task: ffffffef8f491200 task.stack: ffffff800d298000
pc : ffs_epfile_ioctl+0x1c4/0x364
lr : ffs_epfile_ioctl+0x1c4/0x364

sp : ffffff800d29bd40 pstate : 80400145
x29: ffffff800d29bdc0 x28: ffffffef8f491200
x27: ffffff956cb01000 x26: 000000000000001d
x25: ffffffefa007f8b0 x24: ffffffef499698f0
x23: ffffffefd448c280 x22: ffffffefae6df940
x21: ffffffefa007fa57 x20: 00000079a8101318
x19: ffffffef49969958 x18: 00000895b16f2301
x17: ffffffeffcf624f8 x16: 0000000000004e20
x15: 0000000000000001 x14: ffffffeffcf61af8
x13: 0000000000000008 x12: 00000002dfc5023a
x11: 00000002dfc5023a x10: 0000000000000015
x9 : 0000000000000000 x8 : 0000000000000008
x7 : 6320383030303030 x6 : ffffffeff16037b8
x5 : ffffffef9bc0a380 x4 : 0000000001312d00
x3 : ffffff800d29bb18 x2 : ffffff956ba8ebd0
x1 : ffffff956caf1f4c x0 : ffffff956caf1f4c

PC: 0xffffff956c38a818:

a818  f9402308 910043e1 9103a100 97de046c 17ffffc2 f9402300 f8448408 b9404908
a838  71000d1f 1a9f17e9 7100151f 321f03e8 9a890108 8b080f28 f9400915 941db318
a858  394002b3 320003e2 aa1503e0 aa1303e1 97e32f88 90005d80 52801061 9134bc00
a878  97e2556a d5384108 aa1403ea f9402109 ab13014a 9a8983e9 da9f314a fa09015f

LR: 0xffffff956c38a818:

a818  f9402308 910043e1 9103a100 97de046c 17ffffc2 f9402300 f8448408 b9404908
a838  71000d1f 1a9f17e9 7100151f 321f03e8 9a890108 8b080f28 f9400915 941db318
a858  394002b3 320003e2 aa1503e0 aa1303e1 97e32f88 90005d80 52801061 9134bc00
a878  97e2556a d5384108 aa1403ea f9402109 ab13014a 9a8983e9 da9f314a fa09015f

SP: 0xffffff800d29bd00:

bd00  6c38a858 ffffff95 80400145 00000000 0d29bd30 ffffff80 6caf7504 ffffff95
bd20  ffffffff 0000007f 49969958 ffffffef 0d29bdc0 ffffff80 6c38a858 ffffff95
bd40  00000003 00000000 6baea5a0 ffffff95 00000001 00000000 0d29bdd0 ffffff80
bd60  dd4ad288 ffffffef 94706900 cd6f8d01 8f491200 ffffffef 94706900 cd6f8d01

Process ->transport (pid: 13186, stack limit = 0xffffff800d298000)

Call trace:
 ffs_epfile_ioctl+0x1c4/0x364
 vfs_ioctl+0x3c/0x5c
 do_vfs_ioctl+0x670/0x928
 SyS_ioctl+0x90/0x9c
 el0_svc_naked+0x34/0x38
Code: 9a890108 8b080f28 f9400915 941db318 (394002b3) 
---[ end trace 198e86364be2f515 ]---
Kernel panic - not syncing: Fatal exception

SMP: stopping secondary CPUs

Thanks,
Jack

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

From: Vamsi Krishna Samavedam <[email protected]>

The function may be unbound causing the ffs_ep and its descriptors to be freed while userspace is in the middle of an ioctl requesting the same descriptors. Avoid dangling pointer reference by first making a local copy of desctiptors before releasing the spinlock.

Fixes c559a3534109 ("usb: gadget: f_fs: add ioctl returning ep descriptor")

Signed-off-by Vamsi Krishna Samavedam <[email protected]>

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/gadget/function/f_fs.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/usb/gadget/function/f_fs.c +4 -3

index 046f770a76da..c727cb5de871 100644
@@ -1324,7 +1324,7 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
        case FUNCTIONFS_ENDPOINT_DESC:
        {
                int desc_idx;
-               struct usb_endpoint_descriptor *desc;
+               struct usb_endpoint_descriptor desc1, *desc;
 
                switch (epfile->ffs->gadget->speed) {
                case USB_SPEED_SUPER:

@@ -1336,10 +1336,12 @@ static long ffs_epfile_ioctl(struct file *file, unsigned code,
                default:
                        desc_idx = 0;
                }
+
                desc = epfile->ep->descs[desc_idx];
+               memcpy(&desc1, desc, desc->bLength);
 
                spin_unlock_irq(&epfile->ffs->eps_lock);
-               ret = copy_to_user((void __user *)value, desc, desc->bLength);
+               ret = copy_to_user((void __user *)value, &desc1, desc1.bLength);
                if (ret)
                        ret = -EFAULT;
                return ret;
--

2.24.0