diff mbox series

[v9] usb: f_fs: Fix use-after-free for epfile

Message ID 1641961956-30641-1-git-send-email-quic_ugoswami@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v9] usb: f_fs: Fix use-after-free for epfile | expand

Commit Message

Udipto Goswami Jan. 12, 2022, 4:32 a.m. UTC
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>
---
v9: Removed unnecessary spinlock from epfiles_create.

 drivers/usb/gadget/function/f_fs.c | 57 ++++++++++++++++++++++++++++----------
 1 file changed, 42 insertions(+), 15 deletions(-)

Comments

Greg Kroah-Hartman Jan. 26, 2022, 12:41 p.m. UTC | #1
On Wed, Jan 12, 2022 at 10:02:36AM +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)
> 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>
> ---
> v9: Removed unnecessary spinlock from epfiles_create.
> 
>  drivers/usb/gadget/function/f_fs.c | 57 ++++++++++++++++++++++++++++----------
>  1 file changed, 42 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 3c584da..10294ca 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;
> @@ -1919,12 +1939,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 +1965,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;
>  
> -- 
> 2.7.4
> 

This does not apply against 5.17-rc1.  Can you please rebase and resend?

thanks,

greg k-h
Udipto Goswami Jan. 26, 2022, 1:38 p.m. UTC | #2
Hi Greg ,

sure let me rebase it and send again.

On 26-01-2022 06:11 pm, Greg Kroah-Hartman wrote:
> On Wed, Jan 12, 2022 at 10:02:36AM +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)
>> 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>
>> ---
>> v9: Removed unnecessary spinlock from epfiles_create.
>>
>>   drivers/usb/gadget/function/f_fs.c | 57 ++++++++++++++++++++++++++++----------
>>   1 file changed, 42 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 3c584da..10294ca 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;
>> @@ -1919,12 +1939,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 +1965,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;
>>   
>> -- 
>> 2.7.4
>>
> This does not apply against 5.17-rc1.  Can you please rebase and resend?
>
> thanks,
>
> greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 3c584da..10294ca 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;
@@ -1919,12 +1939,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 +1965,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;