[05/12] usb: usbtmc: Add ioctl for generic requests on control pipe

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, May 17, 2018 at 07:03:29PM +0200, Guido Kiener wrote:
> Add USBTMC_IOCTL_CTRL_REQUEST to send arbitrary requests on the
> control pipe.  Used by specific applications of IVI Foundation,
> Inc. to implement VISA API functions: viUsbControlIn/Out.
> 
> Signed-off-by: Guido Kiener <guido.kiener@rohde-schwarz.com>
> Reviewed-by: Steve Bayless <steve_bayless@keysight.com>
> ---
>  drivers/usb/class/usbtmc.c   | 61 ++++++++++++++++++++++++++++++++++++
>  include/uapi/linux/usb/tmc.h | 15 +++++++++
>  2 files changed, 76 insertions(+)
> 
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 152e2daa9644..00c2e51a23a7 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
>   * Copyright (C) 2008 Novell, Inc.
>   * Copyright (C) 2008 Greg Kroah-Hartman <gregkh@suse.de>
> + * Copyright (C) 2018, IVI Foundation, Inc.
>   */
>  
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> @@ -1263,6 +1264,62 @@ static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data)
>  	return rv;
>  }
>  
> +static int usbtmc_ioctl_request(struct usbtmc_device_data *data,
> +				void __user *arg)
> +{
> +	struct device *dev = &data->intf->dev;
> +	struct usbtmc_ctrlrequest request;
> +	u8 *buffer = NULL;
> +	int rv;
> +	unsigned long res;
> +
> +	res = copy_from_user(&request, arg, sizeof(struct usbtmc_ctrlrequest));
> +	if (res)
> +		return -EFAULT;
> +
> +	buffer = kmalloc(min_t(u16, 256, request.req.wLength), GFP_KERNEL);

No validation that wLength is a sane number?

Go to the whiteboard nearby and write a the top of it:
	ALL INPUT IS EVIL

> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	if ((request.req.bRequestType & USB_DIR_IN) == 0
> +	    && request.req.wLength) {
> +		res = copy_from_user(buffer, request.data,
> +				     request.req.wLength);
> +		if (res) {
> +			rv = -EFAULT;
> +			goto exit;
> +		}
> +	}
> +
> +	rv = usb_control_msg(data->usb_dev,
> +			usb_rcvctrlpipe(data->usb_dev, 0),
> +			request.req.bRequest,
> +			request.req.bRequestType,
> +			request.req.wValue,
> +			request.req.wIndex,
> +			buffer, request.req.wLength, USB_CTRL_GET_TIMEOUT);

request.req.wLength might not be the actual size of buffer here due to
your above min_t() check.

> +
> +	if (rv < 0) {
> +		dev_err(dev, "%s failed %d\n", __func__, rv);
> +		goto exit;
> +	}
> +	if ((request.req.bRequestType & USB_DIR_IN)) {
> +		if (rv > request.req.wLength) {
> +			dev_warn(dev, "%s returned too much data: %d\n",
> +				 __func__, rv);
> +			rv = request.req.wLength;

Why are you returning a value that is greater than the size asked for?
That's going to make userspace confused.

Then there is the generic question of "why are you allowing arbitrary
USB control messages to be sent to devices" here.  That feels like a
very odd way for a device that is supposed to be following a standard to
be working.  You are circumventing the standard here by allowing any
message to be sent to the device.  While nice for that one type of
device, you are breaking interoperability in horrible ways (now apps
only work with one type of device.)

Why did you all agree to allow this to happen?

thanks,

greg k-h
---
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