Re: [PATCH v6] usb: f_fs: Fix use-after-free for epfile

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Mon, Dec 20, 2021 at 07:16:14PM +0530, Udipto Goswami wrote:
> Hi Greg,
> 
> Apologies for the delay, please find the following comments as per my
> understanding inline.
> 
> On 13-12-2021 07:38 pm, Greg Kroah-Hartman wrote:
> > On Tue, Dec 07, 2021 at 11:37:54AM +0530, Udipto Goswami wrote:
> > > Consider a case where ffs_func_eps_disable is called from
> > > ffs_func_disable as part of composition switch and at the
> > > same time ffs_epfile_release get called from userspace.
> > > ffs_epfile_release will free up the read buffer and call
> > > ffs_data_closed which in turn destroys ffs->epfiles and
> > > mark it as NULL. While this was happening the driver has
> > > already initialized the local epfile in ffs_func_eps_disable
> > > which is now freed and waiting to acquire the spinlock. Once
> > > spinlock is acquired the driver proceeds with the stale value
> > > of epfile and tries to free the already freed read buffer
> > > causing use-after-free.
> > > 
> > > Following is the illustration of the race:
> > > 
> > >        CPU1                                  CPU2
> > > 
> > >     ffs_func_eps_disable
> > >     epfiles (local copy)
> > > 					ffs_epfile_release
> > > 					ffs_data_closed
> > > 					if (last file closed)
> > > 					ffs_data_reset
> > > 					ffs_data_clear
> > > 					ffs_epfiles_destroy
> > > spin_lock
> > > dereference epfiles
> > > 
> > > Fix this races by taking epfiles local copy & assigning it under
> > > spinlock and if epfiles(local) is null then update it in ffs->epfiles
> > > then finally destroy it.
> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
> ffs_func_eps_disable)
> > > Reviewed-by: John Keeping <john@metanate.com>
> > > Signed-off-by: Pratham Pratap <quic_ppratap@quicinc.com>
> > > Co-developed-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > Signed-off-by: Udipto Goswami <quic_ugoswami@quicinc.com>
> > > ---
> > > v6: Addressing minor code formatting.
> > > 
> > >   drivers/usb/gadget/function/f_fs.c | 44 +++++++++++++++++++++++++++++---------
> > >   1 file changed, 34 insertions(+), 10 deletions(-)
> > What commit does this fix?
> > 
> > Does this need to go to stable kernel releases?
> > 
> > thanks,
> > 
> > greg k-h
> > 
> This is fixing Commit:  a9e6f83c2df (usb: gadget: f_fs: stop sleeping in
> ffs_func_eps_disable)
> 
> Yes this fix is needed for >=5.4 Kernel releases.

Please put that in the patch when you resend it.

> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index 3c584da..6f23a66 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -1711,16 +1711,24 @@ static void ffs_data_put(struct ffs_data *ffs)
> > >   static void ffs_data_closed(struct ffs_data *ffs)
> > >   {
> > > +	struct ffs_epfile *epfiles;
> > > +	unsigned long flags;
> > > +
> > >   	ENTER();
> > >   	if (atomic_dec_and_test(&ffs->opened)) {
> > >   		if (ffs->no_disconnect) {
> > >   			ffs->state = FFS_DEACTIVATED;
> > > -			if (ffs->epfiles) {
> > > -				ffs_epfiles_destroy(ffs->epfiles,
> > > -						   ffs->eps_count);
> > > -				ffs->epfiles = NULL;
> > > -			}
> > > +			spin_lock_irqsave(&ffs->eps_lock, flags);
> > > +			epfiles = ffs->epfiles;
> > > +			ffs->epfiles = NULL;
> > > +			spin_unlock_irqrestore(&ffs->eps_lock,
> > > +							flags);
> > > +
> > > +			if (epfiles)
> > > +				ffs_epfiles_destroy(epfiles,
> > > +						 ffs->eps_count);
> > You are accessing epfiles outside of the lock.  How is that ok?
> There are 2 parties that are trying to free the ffs->epfiles. We use the
> lock to serialize which party gets to free the ffs->epfiles. Where we are
> making it NULL we protected that, The freeing part doesn't need to be inside
> spinlock because now it is epfiles(local_ variable) with which we are trying
> to operate now. We removed the ffs->epfiles as the common structure to
> operate and made the local variable epfiles to do the job.

Why is the lock used only for that one field and not all other fields in
that structure?  What about the no_disconnect access above these lines?
The setup_state access below them?  Why does only one pointer deserve
this locking and not all of them?

thanks,

greg k-h