Zitat von Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
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".
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. A device
should always be able to deal with all possible 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.
> 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.
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.
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.
> 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
