usb: usbtmc: Add ioctl for generic requests on control pipe
Greg Kroah-Hartman
maintainerPATCH 5/12 usb: usbtmc: Add ioctl for generic requests on control pipe
Quoted 31 lines · Guido Kiener
Add
USBTMC_IOCTL_CTRL_REQUESTto 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 <[email protected]>Reviewed-by Steve Bayless <[email protected]>--- drivers/usb/class/usbtmc.c | 61 ++++++++++++++++++++++++++++++++++++ include/uapi/linux/usb/tmc.h | 15 +++++++++ 2 files changed, 76 insertions(+)drivers/usb/class/usbtmc.c +5
-0@@ -5,6 +5,7 @@* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany * Copyright (C) 2008 Novell, Inc. * Copyright (C) 2008 Greg Kroah-Hartman <[email protected]> +* 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;
drivers/usb/class/usbtmc.c static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) · line 1280
Quoted 10 lines
drivers/usb/class/usbtmc.c +10
-0+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?
Go to the whiteboard nearby and write a the top of it:
ALL INPUT IS EVIL
Quoted 10 lines
drivers/usb/class/usbtmc.c +10
-0+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;
drivers/usb/class/usbtmc.c static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) · line 1300
Quoted 10 lines
drivers/usb/class/usbtmc.c +10
-0+ } + } + + 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.
drivers/usb/class/usbtmc.c static int usbtmc_ioctl_indicator_pulse(struct usbtmc_device_data *data) · line 1310
Quoted 10 lines
drivers/usb/class/usbtmc.c +10
-0+ +if (rv <0 ) { +dev_err (dev,"%s failed %d\n" , __func__, rv); + goto exit; + } +if ((request.req.bRequestType &USB_DIR_IN )) { +if (rv > request.req.wLength) { +dev_warn (dev,"%s returned too much data: %d\n" , + __func__, rv); + rv = request.req.wLength;
Why are you returning a value that is greater than the size asked for? That's going to make userspace confused.
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?
thanks,
greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html