* [PATCH v5] usb: f_fs: Fix use-after-free for epfile
@ 2021-11-29 11:52 Udipto Goswami
2021-12-01 16:47 ` John Keeping
0 siblings, 1 reply; 3+ messages in thread
From: Udipto Goswami @ 2021-11-29 11:52 UTC (permalink / raw)
To: Felipe Balbi, Greg Kroah-Hartman, John Keeping
Cc: Udipto Goswami, linux-usb, Pratham Pratap, Pavankumar Kondeti,
Jack Pham
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.
Change-Id: I84b46f6c07cbf307a2bf92c366b933dc0e83d91a
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>
---
v5: Changed the naming of epfile to singular in
ffs_func_eps_disable
drivers/usb/gadget/function/f_fs.c | 41 ++++++++++++++++++++++++++++----------
1 file changed, 31 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 3c584da..f7be222 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -1711,16 +1711,23 @@ 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 +1774,25 @@ 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);
@@ -1919,12 +1937,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)
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v5] usb: f_fs: Fix use-after-free for epfile
2021-11-29 11:52 [PATCH v5] usb: f_fs: Fix use-after-free for epfile Udipto Goswami
@ 2021-12-01 16:47 ` John Keeping
2021-12-07 5:36 ` Udipto Goswami
0 siblings, 1 reply; 3+ messages in thread
From: John Keeping @ 2021-12-01 16:47 UTC (permalink / raw)
To: Udipto Goswami
Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Pratham Pratap,
Pavankumar Kondeti, Jack Pham
On Mon, Nov 29, 2021 at 05:22:57PM +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.
>
> Change-Id: I84b46f6c07cbf307a2bf92c366b933dc0e83d91a
> 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>
A few formatting nits below, but other than that this is:
Reviewed-by: John Keeping <john@metanate.com>
> ---
> v5: Changed the naming of epfile to singular in
> ffs_func_eps_disable
>
> drivers/usb/gadget/function/f_fs.c | 41 ++++++++++++++++++++++++++++----------
> 1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 3c584da..f7be222 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -1711,16 +1711,23 @@ 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;
Blank line after variable declarations.
> 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 +1774,25 @@ 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;
Again there should be a blank line here.
> 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.
> + */
/*
* Multi-line comment blocks have the opening "/*" and closing "*/" each
* on their own line. This may benefit from a blank line before the
* comment as well.
*/
> + if (epfiles)
> + ffs_epfiles_destroy(epfiles,
> + ffs->eps_count);
This was all one line before and you've made it shorter, so it can
probably stay on one line.
>
> if (ffs->ffs_eventfd)
> eventfd_ctx_put(ffs->ffs_eventfd);
> @@ -1919,12 +1937,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)
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v5] usb: f_fs: Fix use-after-free for epfile
2021-12-01 16:47 ` John Keeping
@ 2021-12-07 5:36 ` Udipto Goswami
0 siblings, 0 replies; 3+ messages in thread
From: Udipto Goswami @ 2021-12-07 5:36 UTC (permalink / raw)
To: John Keeping
Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Pratham Pratap,
Pavankumar Kondeti, Jack Pham
Hi John,
Thanks for the review, i'll push v6 with the nits addressed.
On 01-12-2021 10:17 pm, John Keeping wrote:
> On Mon, Nov 29, 2021 at 05:22:57PM +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.
>>
>> Change-Id: I84b46f6c07cbf307a2bf92c366b933dc0e83d91a
>> 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>
> A few formatting nits below, but other than that this is:
>
> Reviewed-by: John Keeping <john@metanate.com>
>
>> ---
>> v5: Changed the naming of epfile to singular in
>> ffs_func_eps_disable
>>
>> drivers/usb/gadget/function/f_fs.c | 41 ++++++++++++++++++++++++++++----------
>> 1 file changed, 31 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 3c584da..f7be222 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -1711,16 +1711,23 @@ 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;
> Blank line after variable declarations.
>
>> 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 +1774,25 @@ 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;
> Again there should be a blank line here.
>
>> 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.
>> + */
> /*
> * Multi-line comment blocks have the opening "/*" and closing "*/" each
> * on their own line. This may benefit from a blank line before the
> * comment as well.
> */
>
>> + if (epfiles)
>> + ffs_epfiles_destroy(epfiles,
>> + ffs->eps_count);
> This was all one line before and you've made it shorter, so it can
> probably stay on one line.
>
>>
>> if (ffs->ffs_eventfd)
>> eventfd_ctx_put(ffs->ffs_eventfd);
>> @@ -1919,12 +1937,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)
>> --
>> 2.7.4
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-12-07 5:36 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-11-29 11:52 [PATCH v5] usb: f_fs: Fix use-after-free for epfile Udipto Goswami
2021-12-01 16:47 ` John Keeping
2021-12-07 5:36 ` Udipto Goswami
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).