From: Peter Chen <peter.chen@nxp.com>
To: Jack Pham <jackp@codeaurora.org>
Cc: Felipe Balbi <balbi@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: Re: [PATCH] usb: gadget: f_fs: Use local copy of descriptors for userspace copy
Date: Wed, 2 Dec 2020 11:09:17 +0000 [thread overview]
Message-ID: <20201202110848.GA26985@b29397-desktop> (raw)
In-Reply-To: <20201202025538.GA20623@jackp-linux.qualcomm.com>
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
next prev parent reply other threads:[~2020-12-02 11:10 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-30 20:34 [PATCH] usb: gadget: f_fs: Use local copy of descriptors for userspace copy Jack Pham
2020-12-01 8:43 ` Peter Chen
2020-12-02 2:55 ` Jack Pham
2020-12-02 11:09 ` Peter Chen [this message]
2020-12-02 11:10 ` Peter Chen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201202110848.GA26985@b29397-desktop \
--to=peter.chen@nxp.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=jackp@codeaurora.org \
--cc=linux-usb@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.