| Message ID | 1641391526-8737-1-git-send-email-quic_ugoswami@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v8] usb: f_fs: Fix use-after-free for epfile | expand |
On Wed, Jan 05, 2022 at 07:35:26PM +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. > Extending the scope further from the race, protecting the ep related > structures, and concurrent accesses. > > Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in > ffs_func_eps_disable) No need to line-wrap this line, the scripts will complain about it :( > 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> > --- > v8: Fixed compilation errors from previous version. > > drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- > 1 file changed, 45 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > index 3c584da..541a4af 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); > + > if (ffs->setup_state == FFS_SETUP_PENDING) > __ffs_ep0_stall(ffs); > } else { > @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > > static void ffs_data_clear(struct ffs_data *ffs) > { > + struct ffs_epfile *epfiles; > + unsigned long flags; > + > ENTER(); > > ffs_closed(ffs); > > BUG_ON(ffs->gadget); > > - if (ffs->epfiles) > - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); > + spin_lock_irqsave(&ffs->eps_lock, flags); > + epfiles = ffs->epfiles; > + ffs->epfiles = NULL; > + spin_unlock_irqrestore(&ffs->eps_lock, flags); > + > + /* > + * potential race possible between ffs_func_eps_disable > + * & ffs_epfile_release therefore maintaining a local > + * copy of epfile will save us from use-after-free. > + */ > + if (epfiles) > + ffs_epfiles_destroy(epfiles, ffs->eps_count); > > if (ffs->ffs_eventfd) > eventfd_ctx_put(ffs->ffs_eventfd); > @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) > > ffs_data_clear(ffs); > > - ffs->epfiles = NULL; > ffs->raw_descs_data = NULL; > ffs->raw_descs = NULL; > ffs->raw_strings = NULL; > @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > { > struct ffs_epfile *epfile, *epfiles; > unsigned i, count; > + unsigned long flags; > > ENTER(); > > @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > } > } > > + spin_lock_irqsave(&ffs->eps_lock, flags); > ffs->epfiles = epfiles; > + spin_unlock_irqrestore(&ffs->eps_lock, flags); Why is this lock needed when you set this value? What is that protecting? thanks, greg k-h
Hi Greg, On 06-01-2022 08:03 pm, Greg Kroah-Hartman wrote: > On Wed, Jan 05, 2022 at 07:35:26PM +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. >> Extending the scope further from the race, protecting the ep related >> structures, and concurrent accesses. >> >> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in >> ffs_func_eps_disable) > No need to line-wrap this line, the scripts will complain about it :( checkpatch didn't give any error for this though. > >> 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> >> --- >> v8: Fixed compilation errors from previous version. >> >> drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- >> 1 file changed, 45 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >> index 3c584da..541a4af 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); >> + >> if (ffs->setup_state == FFS_SETUP_PENDING) >> __ffs_ep0_stall(ffs); >> } else { >> @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >> >> static void ffs_data_clear(struct ffs_data *ffs) >> { >> + struct ffs_epfile *epfiles; >> + unsigned long flags; >> + >> ENTER(); >> >> ffs_closed(ffs); >> >> BUG_ON(ffs->gadget); >> >> - if (ffs->epfiles) >> - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); >> + spin_lock_irqsave(&ffs->eps_lock, flags); >> + epfiles = ffs->epfiles; >> + ffs->epfiles = NULL; >> + spin_unlock_irqrestore(&ffs->eps_lock, flags); >> + >> + /* >> + * potential race possible between ffs_func_eps_disable >> + * & ffs_epfile_release therefore maintaining a local >> + * copy of epfile will save us from use-after-free. >> + */ >> + if (epfiles) >> + ffs_epfiles_destroy(epfiles, ffs->eps_count); >> >> if (ffs->ffs_eventfd) >> eventfd_ctx_put(ffs->ffs_eventfd); >> @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) >> >> ffs_data_clear(ffs); >> >> - ffs->epfiles = NULL; >> ffs->raw_descs_data = NULL; >> ffs->raw_descs = NULL; >> ffs->raw_strings = NULL; >> @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >> { >> struct ffs_epfile *epfile, *epfiles; >> unsigned i, count; >> + unsigned long flags; >> >> ENTER(); >> >> @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >> } >> } >> >> + spin_lock_irqsave(&ffs->eps_lock, flags); >> ffs->epfiles = epfiles; >> + spin_unlock_irqrestore(&ffs->eps_lock, flags); > Why is this lock needed when you set this value? What is that > protecting? Was making it uniform, protection ffs->epfiles all over. Here intention is to protect the operation of epfiles getting assigned to ffs->epfiles so that we protect the ffs->epfiles instance at the time of creation as well. > > thanks, > > greg k-h
On Fri, Jan 07, 2022 at 01:52:05PM +0530, Udipto Goswami wrote: > Hi Greg, > > On 06-01-2022 08:03 pm, Greg Kroah-Hartman wrote: > > On Wed, Jan 05, 2022 at 07:35:26PM +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. > > > Extending the scope further from the race, protecting the ep related > > > structures, and concurrent accesses. > > > > > > Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in > > > ffs_func_eps_disable) > > No need to line-wrap this line, the scripts will complain about it :( > checkpatch didn't give any error for this though. It's not a checkpatch error, it will show up in the scripts we run on commits in our trees. linux-next will report a problem with this, and so I have the same scripts as well. They were posted to the users@kernel.org mailing list a year or so ago if you are curious. It's not a big deal. > > > 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> > > > --- > > > v8: Fixed compilation errors from previous version. > > > > > > drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- > > > 1 file changed, 45 insertions(+), 15 deletions(-) > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > > index 3c584da..541a4af 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); > > > + > > > if (ffs->setup_state == FFS_SETUP_PENDING) > > > __ffs_ep0_stall(ffs); > > > } else { > > > @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > > > static void ffs_data_clear(struct ffs_data *ffs) > > > { > > > + struct ffs_epfile *epfiles; > > > + unsigned long flags; > > > + > > > ENTER(); > > > ffs_closed(ffs); > > > BUG_ON(ffs->gadget); > > > - if (ffs->epfiles) > > > - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); > > > + spin_lock_irqsave(&ffs->eps_lock, flags); > > > + epfiles = ffs->epfiles; > > > + ffs->epfiles = NULL; > > > + spin_unlock_irqrestore(&ffs->eps_lock, flags); > > > + > > > + /* > > > + * potential race possible between ffs_func_eps_disable > > > + * & ffs_epfile_release therefore maintaining a local > > > + * copy of epfile will save us from use-after-free. > > > + */ > > > + if (epfiles) > > > + ffs_epfiles_destroy(epfiles, ffs->eps_count); > > > if (ffs->ffs_eventfd) > > > eventfd_ctx_put(ffs->ffs_eventfd); > > > @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) > > > ffs_data_clear(ffs); > > > - ffs->epfiles = NULL; > > > ffs->raw_descs_data = NULL; > > > ffs->raw_descs = NULL; > > > ffs->raw_strings = NULL; > > > @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > > > { > > > struct ffs_epfile *epfile, *epfiles; > > > unsigned i, count; > > > + unsigned long flags; > > > ENTER(); > > > @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > > > } > > > } > > > + spin_lock_irqsave(&ffs->eps_lock, flags); > > > ffs->epfiles = epfiles; > > > + spin_unlock_irqrestore(&ffs->eps_lock, flags); > > Why is this lock needed when you set this value? What is that > > protecting? > > Was making it uniform, protection ffs->epfiles all over. Here intention is > to protect the operation of epfiles getting assigned to ffs->epfiles so that > we protect the ffs->epfiles instance at the time of creation as well. But it is not needed here, so no need to add it. thanks, greg k-h
Hi Greg, On 07-01-2022 03:34 pm, Greg Kroah-Hartman wrote: > On Fri, Jan 07, 2022 at 01:52:05PM +0530, Udipto Goswami wrote: >> Hi Greg, >> >> On 06-01-2022 08:03 pm, Greg Kroah-Hartman wrote: >>> On Wed, Jan 05, 2022 at 07:35:26PM +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. >>>> Extending the scope further from the race, protecting the ep related >>>> structures, and concurrent accesses. >>>> >>>> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in >>>> ffs_func_eps_disable) >>> No need to line-wrap this line, the scripts will complain about it :( >> checkpatch didn't give any error for this though. > It's not a checkpatch error, it will show up in the scripts we run on > commits in our trees. linux-next will report a problem with this, and > so I have the same scripts as well. They were posted to the > users@kernel.org mailing list a year or so ago if you are curious. It's > not a big deal. > >>>> 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> >>>> --- >>>> v8: Fixed compilation errors from previous version. >>>> >>>> drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- >>>> 1 file changed, 45 insertions(+), 15 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >>>> index 3c584da..541a4af 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); >>>> + >>>> if (ffs->setup_state == FFS_SETUP_PENDING) >>>> __ffs_ep0_stall(ffs); >>>> } else { >>>> @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >>>> static void ffs_data_clear(struct ffs_data *ffs) >>>> { >>>> + struct ffs_epfile *epfiles; >>>> + unsigned long flags; >>>> + >>>> ENTER(); >>>> ffs_closed(ffs); >>>> BUG_ON(ffs->gadget); >>>> - if (ffs->epfiles) >>>> - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); >>>> + spin_lock_irqsave(&ffs->eps_lock, flags); >>>> + epfiles = ffs->epfiles; >>>> + ffs->epfiles = NULL; >>>> + spin_unlock_irqrestore(&ffs->eps_lock, flags); >>>> + >>>> + /* >>>> + * potential race possible between ffs_func_eps_disable >>>> + * & ffs_epfile_release therefore maintaining a local >>>> + * copy of epfile will save us from use-after-free. >>>> + */ >>>> + if (epfiles) >>>> + ffs_epfiles_destroy(epfiles, ffs->eps_count); >>>> if (ffs->ffs_eventfd) >>>> eventfd_ctx_put(ffs->ffs_eventfd); >>>> @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) >>>> ffs_data_clear(ffs); >>>> - ffs->epfiles = NULL; >>>> ffs->raw_descs_data = NULL; >>>> ffs->raw_descs = NULL; >>>> ffs->raw_strings = NULL; >>>> @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >>>> { >>>> struct ffs_epfile *epfile, *epfiles; >>>> unsigned i, count; >>>> + unsigned long flags; >>>> ENTER(); >>>> @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >>>> } >>>> } >>>> + spin_lock_irqsave(&ffs->eps_lock, flags); >>>> ffs->epfiles = epfiles; >>>> + spin_unlock_irqrestore(&ffs->eps_lock, flags); >>> Why is this lock needed when you set this value? What is that >>> protecting? >> Was making it uniform, protection ffs->epfiles all over. Here intention is >> to protect the operation of epfiles getting assigned to ffs->epfiles so that >> we protect the ffs->epfiles instance at the time of creation as well. > But it is not needed here, so no need to add it. we haven't encountered this race although, if ffs_func_set_alt() which is asynchronously scheduled ffs_reset_work() -> ffs_data_reset()->ffs_data_clear() this can take place while user space just opened ep0. in this case the epfiles_destroy() if took place just after this operation of ffs->epfiles = epfiles, the epfiles_create, the ffs->epfiles will be freed. It will be better if we protect this case as well. > > thanks, > > greg k-h
On Mon, Jan 10, 2022 at 06:22:48PM +0530, Udipto Goswami wrote: > Hi Greg, > > On 07-01-2022 03:34 pm, Greg Kroah-Hartman wrote: > > On Fri, Jan 07, 2022 at 01:52:05PM +0530, Udipto Goswami wrote: > > > Hi Greg, > > > > > > On 06-01-2022 08:03 pm, Greg Kroah-Hartman wrote: > > > > On Wed, Jan 05, 2022 at 07:35:26PM +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. > > > > > Extending the scope further from the race, protecting the ep related > > > > > structures, and concurrent accesses. > > > > > > > > > > Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in > > > > > ffs_func_eps_disable) > > > > No need to line-wrap this line, the scripts will complain about it :( > > > checkpatch didn't give any error for this though. > > It's not a checkpatch error, it will show up in the scripts we run on > > commits in our trees. linux-next will report a problem with this, and > > so I have the same scripts as well. They were posted to the > > users@kernel.org mailing list a year or so ago if you are curious. It's > > not a big deal. > > > > > > > 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> > > > > > --- > > > > > v8: Fixed compilation errors from previous version. > > > > > > > > > > drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- > > > > > 1 file changed, 45 insertions(+), 15 deletions(-) > > > > > > > > > > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c > > > > > index 3c584da..541a4af 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); > > > > > + > > > > > if (ffs->setup_state == FFS_SETUP_PENDING) > > > > > __ffs_ep0_stall(ffs); > > > > > } else { > > > > > @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) > > > > > static void ffs_data_clear(struct ffs_data *ffs) > > > > > { > > > > > + struct ffs_epfile *epfiles; > > > > > + unsigned long flags; > > > > > + > > > > > ENTER(); > > > > > ffs_closed(ffs); > > > > > BUG_ON(ffs->gadget); > > > > > - if (ffs->epfiles) > > > > > - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); > > > > > + spin_lock_irqsave(&ffs->eps_lock, flags); > > > > > + epfiles = ffs->epfiles; > > > > > + ffs->epfiles = NULL; > > > > > + spin_unlock_irqrestore(&ffs->eps_lock, flags); > > > > > + > > > > > + /* > > > > > + * potential race possible between ffs_func_eps_disable > > > > > + * & ffs_epfile_release therefore maintaining a local > > > > > + * copy of epfile will save us from use-after-free. > > > > > + */ > > > > > + if (epfiles) > > > > > + ffs_epfiles_destroy(epfiles, ffs->eps_count); > > > > > if (ffs->ffs_eventfd) > > > > > eventfd_ctx_put(ffs->ffs_eventfd); > > > > > @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) > > > > > ffs_data_clear(ffs); > > > > > - ffs->epfiles = NULL; > > > > > ffs->raw_descs_data = NULL; > > > > > ffs->raw_descs = NULL; > > > > > ffs->raw_strings = NULL; > > > > > @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > > > > > { > > > > > struct ffs_epfile *epfile, *epfiles; > > > > > unsigned i, count; > > > > > + unsigned long flags; > > > > > ENTER(); > > > > > @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) > > > > > } > > > > > } > > > > > + spin_lock_irqsave(&ffs->eps_lock, flags); > > > > > ffs->epfiles = epfiles; > > > > > + spin_unlock_irqrestore(&ffs->eps_lock, flags); > > > > Why is this lock needed when you set this value? What is that > > > > protecting? > > > Was making it uniform, protection ffs->epfiles all over. Here intention is > > > to protect the operation of epfiles getting assigned to ffs->epfiles so that > > > we protect the ffs->epfiles instance at the time of creation as well. > > But it is not needed here, so no need to add it. > > we haven't encountered this race although, if ffs_func_set_alt() which is > asynchronously scheduled ffs_reset_work() -> > ffs_data_reset()->ffs_data_clear() this can take place while user space just > opened ep0. > > in this case the epfiles_destroy() if took place just after this operation > of ffs->epfiles = epfiles, the epfiles_create, the ffs->epfiles will be > freed. It will be better if we protect this case as well. What exactly are you protecting here? You are only ever setting the value, never looking at the result. If you were looking to see if this was a value before setting it, it would make more sense, but as-is, I can't figure out what this lock here is protecting. confused, greg k-h
Hi Greg, On 10-01-2022 06:27 pm, Greg Kroah-Hartman wrote: > On Mon, Jan 10, 2022 at 06:22:48PM +0530, Udipto Goswami wrote: >> Hi Greg, >> >> On 07-01-2022 03:34 pm, Greg Kroah-Hartman wrote: >>> On Fri, Jan 07, 2022 at 01:52:05PM +0530, Udipto Goswami wrote: >>>> Hi Greg, >>>> >>>> On 06-01-2022 08:03 pm, Greg Kroah-Hartman wrote: >>>>> On Wed, Jan 05, 2022 at 07:35:26PM +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. >>>>>> Extending the scope further from the race, protecting the ep related >>>>>> structures, and concurrent accesses. >>>>>> >>>>>> Fixes: a9e6f83c2df (usb: gadget: f_fs: stop sleeping in >>>>>> ffs_func_eps_disable) >>>>> No need to line-wrap this line, the scripts will complain about it :( >>>> checkpatch didn't give any error for this though. >>> It's not a checkpatch error, it will show up in the scripts we run on >>> commits in our trees. linux-next will report a problem with this, and >>> so I have the same scripts as well. They were posted to the >>> users@kernel.org mailing list a year or so ago if you are curious. It's >>> not a big deal. >>> >>>>>> 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> >>>>>> --- >>>>>> v8: Fixed compilation errors from previous version. >>>>>> >>>>>> drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- >>>>>> 1 file changed, 45 insertions(+), 15 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c >>>>>> index 3c584da..541a4af 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); >>>>>> + >>>>>> if (ffs->setup_state == FFS_SETUP_PENDING) >>>>>> __ffs_ep0_stall(ffs); >>>>>> } else { >>>>>> @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) >>>>>> static void ffs_data_clear(struct ffs_data *ffs) >>>>>> { >>>>>> + struct ffs_epfile *epfiles; >>>>>> + unsigned long flags; >>>>>> + >>>>>> ENTER(); >>>>>> ffs_closed(ffs); >>>>>> BUG_ON(ffs->gadget); >>>>>> - if (ffs->epfiles) >>>>>> - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); >>>>>> + spin_lock_irqsave(&ffs->eps_lock, flags); >>>>>> + epfiles = ffs->epfiles; >>>>>> + ffs->epfiles = NULL; >>>>>> + spin_unlock_irqrestore(&ffs->eps_lock, flags); >>>>>> + >>>>>> + /* >>>>>> + * potential race possible between ffs_func_eps_disable >>>>>> + * & ffs_epfile_release therefore maintaining a local >>>>>> + * copy of epfile will save us from use-after-free. >>>>>> + */ >>>>>> + if (epfiles) >>>>>> + ffs_epfiles_destroy(epfiles, ffs->eps_count); >>>>>> if (ffs->ffs_eventfd) >>>>>> eventfd_ctx_put(ffs->ffs_eventfd); >>>>>> @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) >>>>>> ffs_data_clear(ffs); >>>>>> - ffs->epfiles = NULL; >>>>>> ffs->raw_descs_data = NULL; >>>>>> ffs->raw_descs = NULL; >>>>>> ffs->raw_strings = NULL; >>>>>> @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >>>>>> { >>>>>> struct ffs_epfile *epfile, *epfiles; >>>>>> unsigned i, count; >>>>>> + unsigned long flags; >>>>>> ENTER(); >>>>>> @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) >>>>>> } >>>>>> } >>>>>> + spin_lock_irqsave(&ffs->eps_lock, flags); >>>>>> ffs->epfiles = epfiles; >>>>>> + spin_unlock_irqrestore(&ffs->eps_lock, flags); >>>>> Why is this lock needed when you set this value? What is that >>>>> protecting? >>>> Was making it uniform, protection ffs->epfiles all over. Here intention is >>>> to protect the operation of epfiles getting assigned to ffs->epfiles so that >>>> we protect the ffs->epfiles instance at the time of creation as well. >>> But it is not needed here, so no need to add it. >> we haven't encountered this race although, if ffs_func_set_alt() which is >> asynchronously scheduled ffs_reset_work() -> >> ffs_data_reset()->ffs_data_clear() this can take place while user space just >> opened ep0. >> >> in this case the epfiles_destroy() if took place just after this operation >> of ffs->epfiles = epfiles, the epfiles_create, the ffs->epfiles will be >> freed. It will be better if we protect this case as well. > What exactly are you protecting here? You are only ever setting the > value, never looking at the result. If you were looking to see if this > was a value before setting it, it would make more sense, but as-is, I > can't figure out what this lock here is protecting. Yah you are right on this. was trying to protect ffs->epfiles only but it will be not needed here. Will push another version. > > confused, > > greg k-h
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c index 3c584da..541a4af 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); + if (ffs->setup_state == FFS_SETUP_PENDING) __ffs_ep0_stall(ffs); } else { @@ -1767,14 +1775,27 @@ static struct ffs_data *ffs_data_new(const char *dev_name) static void ffs_data_clear(struct ffs_data *ffs) { + struct ffs_epfile *epfiles; + unsigned long flags; + ENTER(); ffs_closed(ffs); BUG_ON(ffs->gadget); - if (ffs->epfiles) - ffs_epfiles_destroy(ffs->epfiles, ffs->eps_count); + spin_lock_irqsave(&ffs->eps_lock, flags); + epfiles = ffs->epfiles; + ffs->epfiles = NULL; + spin_unlock_irqrestore(&ffs->eps_lock, flags); + + /* + * potential race possible between ffs_func_eps_disable + * & ffs_epfile_release therefore maintaining a local + * copy of epfile will save us from use-after-free. + */ + if (epfiles) + ffs_epfiles_destroy(epfiles, ffs->eps_count); if (ffs->ffs_eventfd) eventfd_ctx_put(ffs->ffs_eventfd); @@ -1790,7 +1811,6 @@ static void ffs_data_reset(struct ffs_data *ffs) ffs_data_clear(ffs); - ffs->epfiles = NULL; ffs->raw_descs_data = NULL; ffs->raw_descs = NULL; ffs->raw_strings = NULL; @@ -1870,6 +1890,7 @@ static int ffs_epfiles_create(struct ffs_data *ffs) { struct ffs_epfile *epfile, *epfiles; unsigned i, count; + unsigned long flags; ENTER(); @@ -1895,7 +1916,9 @@ static int ffs_epfiles_create(struct ffs_data *ffs) } } + spin_lock_irqsave(&ffs->eps_lock, flags); ffs->epfiles = epfiles; + spin_unlock_irqrestore(&ffs->eps_lock, flags); return 0; } @@ -1919,12 +1942,15 @@ static void ffs_epfiles_destroy(struct ffs_epfile *epfiles, unsigned count) static void ffs_func_eps_disable(struct ffs_function *func) { - struct ffs_ep *ep = func->eps; - struct ffs_epfile *epfile = func->ffs->epfiles; - unsigned count = func->ffs->eps_count; + struct ffs_ep *ep; + struct ffs_epfile *epfile; + unsigned short count; unsigned long flags; spin_lock_irqsave(&func->ffs->eps_lock, flags); + count = func->ffs->eps_count; + epfile = func->ffs->epfiles; + ep = func->eps; while (count--) { /* pending requests get nuked */ if (ep->ep) @@ -1942,14 +1968,18 @@ static void ffs_func_eps_disable(struct ffs_function *func) static int ffs_func_eps_enable(struct ffs_function *func) { - struct ffs_data *ffs = func->ffs; - struct ffs_ep *ep = func->eps; - struct ffs_epfile *epfile = ffs->epfiles; - unsigned count = ffs->eps_count; + struct ffs_data *ffs; + struct ffs_ep *ep; + struct ffs_epfile *epfile; + unsigned short count; unsigned long flags; int ret = 0; spin_lock_irqsave(&func->ffs->eps_lock, flags); + ffs = func->ffs; + ep = func->eps; + epfile = ffs->epfiles; + count = ffs->eps_count; while(count--) { ep->ep->driver_data = ep;
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. Extending the scope further from the race, protecting the ep related structures, and concurrent accesses. 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> --- v8: Fixed compilation errors from previous version. drivers/usb/gadget/function/f_fs.c | 60 ++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 15 deletions(-)