| Message ID | 20201130203453.28154-1-jackp@codeaurora.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: f_fs: Use local copy of descriptors for userspace copy | expand |
On 20-11-30 12:34:53, Jack Pham wrote: > From: Vamsi Krishna Samavedam <vskrishn@codeaurora.org> > > 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 <vskrishn@codeaurora.org> > Signed-off-by: Jack Pham <jackp@codeaurora.org> > --- > drivers/usb/gadget/function/f_fs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 046f770a76da..c727cb5de871 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -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; > -- 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. -- Thanks, Peter Chen
(removed Vamsi as he has moved on from USB and his email address is bouncing) Hi Peter, On Tue, Dec 01, 2020 at 08:43:14AM +0000, Peter Chen wrote: > On 20-11-30 12:34:53, Jack Pham wrote: > > From: Vamsi Krishna Samavedam <vskrishn@codeaurora.org> > > > > 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 <vskrishn@codeaurora.org> > > Signed-off-by: Jack Pham <jackp@codeaurora.org> > > --- > > drivers/usb/gadget/function/f_fs.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > index 046f770a76da..c727cb5de871 100644 > > --- a/drivers/usb/gadget/function/f_fs.c > > +++ b/drivers/usb/gadget/function/f_fs.c > > @@ -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; > > -- > > 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 -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
On 20-12-01 18:55:38, Jack Pham wrote: > (removed Vamsi as he has moved on from USB and his email address is > bouncing) > > Hi Peter, > > On Tue, Dec 01, 2020 at 08:43:14AM +0000, Peter Chen wrote: > > On 20-11-30 12:34:53, Jack Pham wrote: > > > From: Vamsi Krishna Samavedam <vskrishn@codeaurora.org> > > > > > > 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 <vskrishn@codeaurora.org> > > > Signed-off-by: Jack Pham <jackp@codeaurora.org> > > > --- > > > drivers/usb/gadget/function/f_fs.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > > index 046f770a76da..c727cb5de871 100644 > > > --- a/drivers/usb/gadget/function/f_fs.c > > > +++ b/drivers/usb/gadget/function/f_fs.c > > > @@ -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; > > > -- > > > > 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(). Jack, after checking more, I think this fix is a good one. ffs->refs is global reference counter to avoid the ffs_data is freed before all users have used up. But ffs_epfile_ioctl is per endpoint, it could be freed earlier. Peter > > 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 > -- > The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project -- Thanks, Peter Chen
> From: Vamsi Krishna Samavedam <vskrishn@codeaurora.org> > > 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 <vskrishn@codeaurora.org> > Signed-off-by: Jack Pham <jackp@codeaurora.org> Reviewed-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/gadget/function/f_fs.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 046f770a76da..c727cb5de871 100644 > --- a/drivers/usb/gadget/function/f_fs.c > +++ b/drivers/usb/gadget/function/f_fs.c > @@ -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 >
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 046f770a76da..c727cb5de871 100644 --- a/drivers/usb/gadget/function/f_fs.c +++ b/drivers/usb/gadget/function/f_fs.c @@ -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;