On Fri, May 18, 2018 at 01:32:51PM +0000, guido@xxxxxxxxxxxxxxxxxx wrote:
>
> Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
>
> > 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@xxxxxxxxxxxxxxxxx>
> > > Reviewed-by: Steve Bayless <steve_bayless@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxx>
> > > + * 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
