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

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

On Fri, May 18, 2018 at 01:32:51PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <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?
> 
> Ooops. It should be buffer = kmalloc(max_t(u16, 256, request.req.wLength),
> GFP_KERNEL);
> Then all values of request.req.wLength (0..65535) are ok.

Yes, that would make more sense.  But you should still reject
known-invalid values, and not just "silently fix them up".

> > 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.

Still wrong given the check above.

> > 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?
> 
> I did not define the USBTMC and VISA standard. However the API requires
> to send this generic control request. Otherwise we have to use the libusb
> again.
> 
> http://zone.ni.com/reference/en-XX/help/370131S-01/ni-visa/viusbcontrolin/

Ugh, that's horrid.  And a sign of "who knows what our devices will want
to support!" design by committee (note, I've worked on lots of spec
groups before, I know how they work...)

Ok, so that needs to be supported, fair enough, so let's get the
implementation correct :)

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