[PATCH v4] usb: usbip: set buffer pointers to NULL after free

[PATCH v4] usb: usbip: set buffer pointers to NULL after free

[Michael Grzeschik]

The usbip stack dynamically allocates the transfer_buffer and
setup_packet of each urb that got generated by the tcp to usb stub code.
As these pointers are always used only once we will set them to NULL
after use. This is done likewise to the free_urb code in vudc_dev.c.
This patch fixes double kfree situations where the usbip remote side
added the URB_FREE_BUFFER.

Cc: stable@vger.kernel.org
Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
---
v1 -> v2: - rephrased patch subject from:
            "usb: usbip: avoid the usb layer to kfree our allocated buffer"
          - changed to always let urb_destoy remove the transfer_buffer
v2 -> v3: - added stable to cc
          - wrapped long line with over 80 chars
v3 -> v4: - rephrades patch subject from usb:
            "usbip: let urb_destroy kfree the transfer_buffer"
          - setting buffer pointers to NULL
            instead of omitting flag URB_FREE_BUFFER

 drivers/usb/usbip/stub_main.c | 4 ++++
 drivers/usb/usbip/stub_tx.c   | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
index 44ab43fc4fcc7..af10f7b131a49 100644
--- a/drivers/usb/usbip/stub_main.c
+++ b/drivers/usb/usbip/stub_main.c
@@ -262,7 +262,11 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
 		kmem_cache_free(stub_priv_cache, priv);
 
 		kfree(urb->transfer_buffer);
+		urb->transfer_buffer = NULL;
+
 		kfree(urb->setup_packet);
+		urb->setup_packet = NULL;
+
 		usb_free_urb(urb);
 	}
 }
diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index 6b1e8c3f0e4b2..be50cef645d8a 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -28,7 +28,11 @@ static void stub_free_priv_and_urb(struct stub_priv *priv)
 	struct urb *urb = priv->urb;
 
 	kfree(urb->setup_packet);
+	urb->setup_packet = NULL;
+
 	kfree(urb->transfer_buffer);
+	urb->transfer_buffer = NULL;
+
 	list_del(&priv->list);
 	kmem_cache_free(stub_priv_cache, priv);
 	usb_free_urb(urb);
-- 
2.11.0

Re: [PATCH v4] usb: usbip: set buffer pointers to NULL after free

[Michael Grzeschik]

Gentle Ping.

To verify this patch just test usbip with some USB Ethernet devices.

On Mon, May 22, 2017 at 01:02:44PM +0200, Michael Grzeschik wrote:
> The usbip stack dynamically allocates the transfer_buffer and
> setup_packet of each urb that got generated by the tcp to usb stub code.
> As these pointers are always used only once we will set them to NULL
> after use. This is done likewise to the free_urb code in vudc_dev.c.
> This patch fixes double kfree situations where the usbip remote side
> added the URB_FREE_BUFFER.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>
> ---
> v1 -> v2: - rephrased patch subject from:
>             "usb: usbip: avoid the usb layer to kfree our allocated buffer"
>           - changed to always let urb_destoy remove the transfer_buffer
> v2 -> v3: - added stable to cc
>           - wrapped long line with over 80 chars
> v3 -> v4: - rephrades patch subject from usb:
>             "usbip: let urb_destroy kfree the transfer_buffer"
>           - setting buffer pointers to NULL
>             instead of omitting flag URB_FREE_BUFFER
> 
>  drivers/usb/usbip/stub_main.c | 4 ++++
>  drivers/usb/usbip/stub_tx.c   | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 44ab43fc4fcc7..af10f7b131a49 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -262,7 +262,11 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
>  		kmem_cache_free(stub_priv_cache, priv);
>  
>  		kfree(urb->transfer_buffer);
> +		urb->transfer_buffer = NULL;
> +
>  		kfree(urb->setup_packet);
> +		urb->setup_packet = NULL;
> +
>  		usb_free_urb(urb);
>  	}
>  }
> diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> index 6b1e8c3f0e4b2..be50cef645d8a 100644
> --- a/drivers/usb/usbip/stub_tx.c
> +++ b/drivers/usb/usbip/stub_tx.c
> @@ -28,7 +28,11 @@ static void stub_free_priv_and_urb(struct stub_priv *priv)
>  	struct urb *urb = priv->urb;
>  
>  	kfree(urb->setup_packet);
> +	urb->setup_packet = NULL;
> +
>  	kfree(urb->transfer_buffer);
> +	urb->transfer_buffer = NULL;
> +
>  	list_del(&priv->list);
>  	kmem_cache_free(stub_priv_cache, priv);
>  	usb_free_urb(urb);
> -- 
> 2.11.0
> 
> --
> 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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v4] usb: usbip: set buffer pointers to NULL after free

[Greg KH]

On Tue, Jun 06, 2017 at 11:36:03AM +0200, Michael Grzeschik wrote:
> Gentle Ping.

I'm waiting for a usbip maintainer to review it :)

Re: [PATCH v4] usb: usbip: set buffer pointers to NULL after free

[Shuah Khan]

On 05/22/2017 05:02 AM, Michael Grzeschik wrote:
> The usbip stack dynamically allocates the transfer_buffer and
> setup_packet of each urb that got generated by the tcp to usb stub code.
> As these pointers are always used only once we will set them to NULL
> after use. This is done likewise to the free_urb code in vudc_dev.c.
> This patch fixes double kfree situations where the usbip remote side
> added the URB_FREE_BUFFER.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Michael Grzeschik <m.grzeschik@pengutronix.de>

Sorry for the delay. I was away last couple of weeks.

The change looks good. Thanks for the fix.

Acked-by: Shuah Khan <shuahkh@osg.samsung.com>

thanks,
-- Shuah

> ---
> v1 -> v2: - rephrased patch subject from:
>             "usb: usbip: avoid the usb layer to kfree our allocated buffer"
>           - changed to always let urb_destoy remove the transfer_buffer
> v2 -> v3: - added stable to cc
>           - wrapped long line with over 80 chars
> v3 -> v4: - rephrades patch subject from usb:
>             "usbip: let urb_destroy kfree the transfer_buffer"
>           - setting buffer pointers to NULL
>             instead of omitting flag URB_FREE_BUFFER
> 
>  drivers/usb/usbip/stub_main.c | 4 ++++
>  drivers/usb/usbip/stub_tx.c   | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/usb/usbip/stub_main.c b/drivers/usb/usbip/stub_main.c
> index 44ab43fc4fcc7..af10f7b131a49 100644
> --- a/drivers/usb/usbip/stub_main.c
> +++ b/drivers/usb/usbip/stub_main.c
> @@ -262,7 +262,11 @@ void stub_device_cleanup_urbs(struct stub_device *sdev)
>  		kmem_cache_free(stub_priv_cache, priv);
>  
>  		kfree(urb->transfer_buffer);
> +		urb->transfer_buffer = NULL;
> +
>  		kfree(urb->setup_packet);
> +		urb->setup_packet = NULL;
> +
>  		usb_free_urb(urb);
>  	}
>  }
> diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
> index 6b1e8c3f0e4b2..be50cef645d8a 100644
> --- a/drivers/usb/usbip/stub_tx.c
> +++ b/drivers/usb/usbip/stub_tx.c
> @@ -28,7 +28,11 @@ static void stub_free_priv_and_urb(struct stub_priv *priv)
>  	struct urb *urb = priv->urb;
>  
>  	kfree(urb->setup_packet);
> +	urb->setup_packet = NULL;
> +
>  	kfree(urb->transfer_buffer);
> +	urb->transfer_buffer = NULL;
> +
>  	list_del(&priv->list);
>  	kmem_cache_free(stub_priv_cache, priv);
>  	usb_free_urb(urb);
>