USB: UDC core: fix double-free in usb_add_gadget_udc_release

USB: UDC core: fix double-free in usb_add_gadget_udc_release

[Peter Chen]

On Wed, Jan 03, 2018 at 12:51:51PM -0500, Alan Stern wrote:
> The error-handling pathways in usb_add_gadget_udc_release() are messed
> up.  Aside from the uninformative statement labels, they can deallocate
> the udc structure after calling put_device(), which is a double-free.
> This was observed by KASAN in automatic testing.
> 
> This patch cleans up the routine.  It preserves the requirement that
> when any failure occurs, we call put_device(&gadget->dev).
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
> Reported-by: Fengguang Wu <fengguang.wu@intel.com>
> CC: <stable@vger.kernel.org>
> 
> ---
> 
> Not having received any feedback on this patch for a while, I have 
> decided to submit it.  The problem it fixes is quite obviously a real 
> one, when you look at the existing code.
> 
> Alan Stern
> 
> 
> [as1856]
> 
> 
>  drivers/usb/gadget/udc/core.c |   28 +++++++++++++---------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> Index: usb-4.x/drivers/usb/gadget/udc/core.c
> ===================================================================
> --- usb-4.x.orig/drivers/usb/gadget/udc/core.c
> +++ usb-4.x/drivers/usb/gadget/udc/core.c
> @@ -1147,11 +1147,7 @@ int usb_add_gadget_udc_release(struct de
>  
>  	udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>  	if (!udc)
> -		goto err1;
> -
> -	ret = device_add(&gadget->dev);
> -	if (ret)
> -		goto err2;
> +		goto err_put_gadget;
>  
>  	device_initialize(&udc->dev);
>  	udc->dev.release = usb_udc_release;
> @@ -1160,7 +1156,11 @@ int usb_add_gadget_udc_release(struct de
>  	udc->dev.parent = parent;
>  	ret = dev_set_name(&udc->dev, "%s", kobject_name(&parent->kobj));
>  	if (ret)
> -		goto err3;
> +		goto err_put_udc;
> +
> +	ret = device_add(&gadget->dev);
> +	if (ret)
> +		goto err_put_udc;
>  
>  	udc->gadget = gadget;
>  	gadget->udc = udc;
> @@ -1170,7 +1170,7 @@ int usb_add_gadget_udc_release(struct de
>  
>  	ret = device_add(&udc->dev);
>  	if (ret)
> -		goto err4;
> +		goto err_unlist_udc;
>  
>  	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
>  	udc->vbus = true;
> @@ -1178,27 +1178,25 @@ int usb_add_gadget_udc_release(struct de
>  	/* pick up one of pending gadget drivers */
>  	ret = check_pending_gadget_drivers(udc);
>  	if (ret)
> -		goto err5;
> +		goto err_del_udc;
>  
>  	mutex_unlock(&udc_lock);
>  
>  	return 0;
>  
> -err5:
> + err_del_udc:
>  	device_del(&udc->dev);
>  
> -err4:
> + err_unlist_udc:
>  	list_del(&udc->list);
>  	mutex_unlock(&udc_lock);
>  
> -err3:
> -	put_device(&udc->dev);
>  	device_del(&gadget->dev);
>  
> -err2:
> -	kfree(udc);
> + err_put_udc:
> +	put_device(&udc->dev);
>  
> -err1:
> + err_put_gadget:
>  	put_device(&gadget->dev);
>  	return ret;
>  }
> 

Reviewed-by: Peter Chen <peter.chen@nxp.com>

USB: UDC core: fix double-free in usb_add_gadget_udc_release

[Alan Stern]

The error-handling pathways in usb_add_gadget_udc_release() are messed
up.  Aside from the uninformative statement labels, they can deallocate
the udc structure after calling put_device(), which is a double-free.
This was observed by KASAN in automatic testing.

This patch cleans up the routine.  It preserves the requirement that
when any failure occurs, we call put_device(&gadget->dev).

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Reported-by: Fengguang Wu <fengguang.wu@intel.com>
CC: <stable@vger.kernel.org>
---

Not having received any feedback on this patch for a while, I have 
decided to submit it.  The problem it fixes is quite obviously a real 
one, when you look at the existing code.

Alan Stern


[as1856]


 drivers/usb/gadget/udc/core.c |   28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Index: usb-4.x/drivers/usb/gadget/udc/core.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/udc/core.c
+++ usb-4.x/drivers/usb/gadget/udc/core.c
@@ -1147,11 +1147,7 @@ int usb_add_gadget_udc_release(struct de
 
 	udc = kzalloc(sizeof(*udc), GFP_KERNEL);
 	if (!udc)
-		goto err1;
-
-	ret = device_add(&gadget->dev);
-	if (ret)
-		goto err2;
+		goto err_put_gadget;
 
 	device_initialize(&udc->dev);
 	udc->dev.release = usb_udc_release;
@@ -1160,7 +1156,11 @@ int usb_add_gadget_udc_release(struct de
 	udc->dev.parent = parent;
 	ret = dev_set_name(&udc->dev, "%s", kobject_name(&parent->kobj));
 	if (ret)
-		goto err3;
+		goto err_put_udc;
+
+	ret = device_add(&gadget->dev);
+	if (ret)
+		goto err_put_udc;
 
 	udc->gadget = gadget;
 	gadget->udc = udc;
@@ -1170,7 +1170,7 @@ int usb_add_gadget_udc_release(struct de
 
 	ret = device_add(&udc->dev);
 	if (ret)
-		goto err4;
+		goto err_unlist_udc;
 
 	usb_gadget_set_state(gadget, USB_STATE_NOTATTACHED);
 	udc->vbus = true;
@@ -1178,27 +1178,25 @@ int usb_add_gadget_udc_release(struct de
 	/* pick up one of pending gadget drivers */
 	ret = check_pending_gadget_drivers(udc);
 	if (ret)
-		goto err5;
+		goto err_del_udc;
 
 	mutex_unlock(&udc_lock);
 
 	return 0;
 
-err5:
+ err_del_udc:
 	device_del(&udc->dev);
 
-err4:
+ err_unlist_udc:
 	list_del(&udc->list);
 	mutex_unlock(&udc_lock);
 
-err3:
-	put_device(&udc->dev);
 	device_del(&gadget->dev);
 
-err2:
-	kfree(udc);
+ err_put_udc:
+	put_device(&udc->dev);
 
-err1:
+ err_put_gadget:
 	put_device(&gadget->dev);
 	return ret;
 }