usb: usbtmc: Add ioctl for generic requests on control pipe

Zitat von Greg KH <[email protected]>:

Quoted 29 lines · Guido Kiener

Quoted 28 lines

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 <[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 +1 -0

index 152e2daa9644..00c2e51a23a7 100644
@@ -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;
 }
 

Quoted 16 lines

Quoted 4 lines

drivers/usb/class/usbtmc.c +4 -0

+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 · line 1281

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?

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.

Quoted 18 lines

Go to the whiteboard nearby and write a the top of it:
ALL INPUT IS EVIL

Quoted 15 lines

drivers/usb/class/usbtmc.c +15 -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;
+		}
+	}
+
+	rv = usb_control_msg(data->usb_dev,
+			usb_rcvctrlpipe(data->usb_dev, 0),

Quoted 8 lines

drivers/usb/class/usbtmc.c static int · line 1301

Quoted 5 lines

drivers/usb/class/usbtmc.c +5 -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.

Quoted 8 lines

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.)

Quoted 1 line

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/