On Fri, May 18, 2018 at 01:32:51PM +0000, gu...@kiener-muenchen.de wrote: > > Zitat von Greg KH <gre...@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.kie...@rohde-schwarz.com> > > > Reviewed-by: Steve Bayless <steve_bayl...@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 <gre...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html