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@xxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxxxxx>
> > > Signed-off-by: Jack Pham <jackp@xxxxxxxxxxxxxx>
> > > ---
> > > 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
