Re: [PATCH] usb: gadget: f_fs: Invalidate io_data when USB request is dequeued or completed

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Mar 31, 2025 at 03:17:08PM +0200, Frode Isaksen wrote:
> On 3/31/25 10:57 AM, Greg KH wrote:
> > On Mon, Mar 31, 2025 at 10:18:29AM +0200, Frode Isaksen wrote:
> > > On 3/28/25 10:02 PM, Greg KH wrote:
> > > > On Fri, Mar 28, 2025 at 05:17:15PM +0100, Frode Isaksen wrote:
> > > > > From: Frode Isaksen <frode@meta.com>
> > > > > 
> > > > > Invalidate io_data by setting context to NULL when USB request is
> > > > > dequeued or completed, and check for NULL io_data in epfile_io_complete().
> > > > > The invalidation of io_data in req->context is done when exiting
> > > > > epfile_io(), since then io_data will become invalid as it is allocated
> > > > > on the stack.
> > > > > The epfile_io_complete() may be called after ffs_epfile_io() returns
> > > > > in case the wait_for_completion_interruptible() is interrupted.
> > > > > This fixes a use-after-free error with the following call stack:
> > > > > 
> > > > > Unable to handle kernel paging request at virtual address ffffffc02f7bbcc0
> > > > > pc : ffs_epfile_io_complete+0x30/0x48
> > > > > lr : usb_gadget_giveback_request+0x30/0xf8
> > > > > Call trace:
> > > > > ffs_epfile_io_complete+0x30/0x48
> > > > > usb_gadget_giveback_request+0x30/0xf8
> > > > > dwc3_remove_requests+0x264/0x2e8
> > > > > dwc3_gadget_pullup+0x1d0/0x250
> > > > > kretprobe_trampoline+0x0/0xc4
> > > > > usb_gadget_remove_driver+0x40/0xf4
> > > > > usb_gadget_unregister_driver+0xdc/0x178
> > > > > unregister_gadget_item+0x40/0x6c
> > > > > ffs_closed+0xd4/0x10c
> > > > > ffs_data_clear+0x2c/0xf0
> > > > > ffs_data_closed+0x178/0x1ec
> > > > > ffs_ep0_release+0x24/0x38
> > > > > __fput+0xe8/0x27c
> > > > > 
> > > > > Signed-off-by: Frode Isaksen <frode@meta.com>
> > > > > ---
> > > > > This bug was discovered, tested and fixed (no more crashes seen) on Meta Quest 3 device.
> > > > > Also tested on T.I. AM62x board.
> > > > What commit id does this fix?  Should it go to stable?
> > > This has always been there, so the is no specific commit when this was
> > > added.
> > > 
> > > Will add the Cc tag to stable in v2.
> > > 
> > > > >    drivers/usb/gadget/function/f_fs.c | 5 +++++
> > > > >    1 file changed, 5 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > > > index 2dea9e42a0f8..f1be0a5c0bd0 100644
> > > > > --- a/drivers/usb/gadget/function/f_fs.c
> > > > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > > > @@ -738,6 +738,9 @@ static void ffs_epfile_io_complete(struct usb_ep *_ep, struct usb_request *req)
> > > > >    {
> > > > >    	struct ffs_io_data *io_data = req->context;
> > > > > +	if (WARN_ON(io_data == NULL))
> > > > > +		return;
> > > > If this happens you just crashed the box (remember about panic-on-warn,
> > > > which is still set in a few billion Linux systems these days...)
> > > > 
> > > > Just handle the issue properly, no need to dump the stack and crash a
> > > > device.
> > > OK, removing the WARN_ON for v2.
> > > > But, what keeps io_data from changing after you have checked it?  Where
> > > > is the lock here?
> > > There is no lock here, as I didn't want to introduce extra complexity (and
> > > bugs...). But this code has been running without a crash on millions of
> > > devices for more than a year.
> > The fix has?  Great, but again, you need to at least say why this value
> > will not change right after testing for it, otherwise you have just
> > reduced the race window, not removed it.
> 
> I agree that this is only reducing the race window and not eliminating it
> completely, but I have no idea how to fix this easily.

The comment in the code explains where the race can not happen, which
implies where it can happen, so perhaps that is a good start?

good luck!

greg k-h