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

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

On Mon, May 21, 2018 at 10:06:57PM +0000, guido@kiener-muenchen.de wrote:
> 
> Zitat von Greg KH <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".
> 
> When I start here to refuse (current) unknown settings or flags, then I fear
> that people always want us to change or allow new flags.

That doesn't make sense, you always need to reject unknown values, or
people will put crap in them and then get mad in the future when you
want to use those fields/values for something else.

> A device should always be able to deal with all possible values.

But the kernel has to be sane and properly validate userspace values.

> We
> cannot prevent a
> user from sending crazy messages to the device and I do not want to develop
> something like a firewall here.

Sure, I'm not saying to do that, you are not validating the other
fields, just the length.  The kernel will validate the other fields when
you submit the urb.

> > > > > +	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.
> 
> I'm missing something. I do not see the error. The size of buffer is just
> at least 256 bytes. It doesn't matter when request.req.wLength < 256.

If wLength is set to 10000 but buffer really isn't that big, bad things
happen...

> I thought this helps the memory management, since we need not to deal with
> different and odd memory sizes. Maybe we should just alloc always a size of
> request.req.wLength.

Again, you have to validate that number, if you want to bound it to 256,
great, then bound it.  Don't half-way do it please.

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