[PATCH] usb: gadget: f_fs: Fix epfile null pointer access after ep enable.

[PATCH] usb: gadget: f_fs: Fix epfile null pointer access after ep enable.

[guhuinan]

A race condition occurs when ffs_func_eps_enable() runs concurrently
with ffs_data_reset(). The ffs_data_clear() called in ffs_data_reset()
sets ffs->epfiles to NULL before resetting ffs->eps_count to 0, leading
to a NULL pointer dereference when accessing epfile->ep in
ffs_func_eps_enable() after successful usb_ep_enable().

Signed-off-by: guhuinan <guhuinan@xiaomi.com>
---
 drivers/usb/gadget/function/f_fs.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 08a251df20c4..f4aae91e7864 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -2407,7 +2407,13 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 	ep = func->eps;
 	epfile = ffs->epfiles;
 	count = ffs->eps_count;
-	while(count--) {
+	if (!epfile) {
+		pr_err("%s: epfiles is NULL\n", __func__);
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	while (count--) {
 		ep->ep->driver_data = ep;
 
 		ret = config_ep_by_speed(func->gadget, &func->function, ep->ep);
@@ -2431,6 +2437,7 @@ static int ffs_func_eps_enable(struct ffs_function *func)
 	}
 
 	wake_up_interruptible(&ffs->wait);
+done:
 	spin_unlock_irqrestore(&func->ffs->eps_lock, flags);
 
 	return ret;
-- 
2.43.0

Re: [PATCH] usb: gadget: f_fs: Fix epfile null pointer access after ep enable.

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 12:05:06PM +0800, guhuinan wrote:
> A race condition occurs when ffs_func_eps_enable() runs concurrently
> with ffs_data_reset(). The ffs_data_clear() called in ffs_data_reset()
> sets ffs->epfiles to NULL before resetting ffs->eps_count to 0, leading
> to a NULL pointer dereference when accessing epfile->ep in
> ffs_func_eps_enable() after successful usb_ep_enable().
> 
> Signed-off-by: guhuinan <guhuinan@xiaomi.com>

Please use your name, not your email alias for the From: and
signed-off-by lines.

> ---
>  drivers/usb/gadget/function/f_fs.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 08a251df20c4..f4aae91e7864 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2407,7 +2407,13 @@ static int ffs_func_eps_enable(struct ffs_function *func)
>  	ep = func->eps;
>  	epfile = ffs->epfiles;
>  	count = ffs->eps_count;
> -	while(count--) {
> +	if (!epfile) {
> +		pr_err("%s: epfiles is NULL\n", __func__);

No need for this debugging line, right?

> +		ret = -ENOMEM;
> +		goto done;
> +	}
> +
> +	while (count--) {

What prevents the pointer from changing right after you check it?  This
will still race :(

You need a lock somewhere to fix this properly.

thanks,

greg k-h

Re: [PATCH] usb: gadget: f_fs: Fix epfile null pointer access after ep enable.

[Owen Gu]

On Fri, Sep 12, 2025 at 07:32:04AM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 12, 2025 at 12:05:06PM +0800, guhuinan wrote:
> > A race condition occurs when ffs_func_eps_enable() runs concurrently
> > with ffs_data_reset(). The ffs_data_clear() called in ffs_data_reset()
> > sets ffs->epfiles to NULL before resetting ffs->eps_count to 0, leading
> > to a NULL pointer dereference when accessing epfile->ep in
> > ffs_func_eps_enable() after successful usb_ep_enable().
> > 
> > Signed-off-by: guhuinan <guhuinan@xiaomi.com>
> 
> Please use your name, not your email alias for the From: and
> signed-off-by lines.
> 
Okay
> > ---
> >  drivers/usb/gadget/function/f_fs.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index 08a251df20c4..f4aae91e7864 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -2407,7 +2407,13 @@ static int ffs_func_eps_enable(struct ffs_function *func)
> >  	ep = func->eps;
> >  	epfile = ffs->epfiles;
> >  	count = ffs->eps_count;
> > -	while(count--) {
> > +	if (!epfile) {
> > +		pr_err("%s: epfiles is NULL\n", __func__);
> 
> No need for this debugging line, right?
> 
Okay
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	while (count--) {
> 
> What prevents the pointer from changing right after you check it?  This
> will still race :(
> 
> You need a lock somewhere to fix this properly.

Dear, 
The ffs->epfiles pointer is set to NULL in both ffs_data_clear() and 
ffs_data_close() functions, and its modification is protected by the
spinlock ffs->eps_lock.
And the whole ffs_func_eps_enable() function is also protected by ffs->eps_lock.

Thanks

> 
> thanks,
> 
> greg k-h

Re: [PATCH] usb: gadget: f_fs: Fix epfile null pointer access after ep enable.

[Greg Kroah-Hartman]

On Fri, Sep 12, 2025 at 09:47:26PM +0800, Owen Gu wrote:
> On Fri, Sep 12, 2025 at 07:32:04AM +0200, Greg Kroah-Hartman wrote:
> > On Fri, Sep 12, 2025 at 12:05:06PM +0800, guhuinan wrote:
> > > A race condition occurs when ffs_func_eps_enable() runs concurrently
> > > with ffs_data_reset(). The ffs_data_clear() called in ffs_data_reset()
> > > sets ffs->epfiles to NULL before resetting ffs->eps_count to 0, leading
> > > to a NULL pointer dereference when accessing epfile->ep in
> > > ffs_func_eps_enable() after successful usb_ep_enable().
> > > 
> > > Signed-off-by: guhuinan <guhuinan@xiaomi.com>
> > 
> > Please use your name, not your email alias for the From: and
> > signed-off-by lines.
> > 
> Okay
> > > ---
> > >  drivers/usb/gadget/function/f_fs.c | 9 ++++++++-
> > >  1 file changed, 8 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > > index 08a251df20c4..f4aae91e7864 100644
> > > --- a/drivers/usb/gadget/function/f_fs.c
> > > +++ b/drivers/usb/gadget/function/f_fs.c
> > > @@ -2407,7 +2407,13 @@ static int ffs_func_eps_enable(struct ffs_function *func)
> > >  	ep = func->eps;
> > >  	epfile = ffs->epfiles;
> > >  	count = ffs->eps_count;
> > > -	while(count--) {
> > > +	if (!epfile) {
> > > +		pr_err("%s: epfiles is NULL\n", __func__);
> > 
> > No need for this debugging line, right?
> > 
> Okay
> > > +		ret = -ENOMEM;
> > > +		goto done;
> > > +	}
> > > +
> > > +	while (count--) {
> > 
> > What prevents the pointer from changing right after you check it?  This
> > will still race :(
> > 
> > You need a lock somewhere to fix this properly.
> 
> Dear, 
> The ffs->epfiles pointer is set to NULL in both ffs_data_clear() and 
> ffs_data_close() functions, and its modification is protected by the
> spinlock ffs->eps_lock.
> And the whole ffs_func_eps_enable() function is also protected by ffs->eps_lock.

Ah, that's good.  Please mention that in the changelog text when you
resubmit a new version of this patch.

thanks,

greg k-h