Re: [PATCH v6 5/5] Add ioctls to enable and disable local controls on an instrument

From: Greg KH
Date: Sun Jan 24 2016 - 23:47:55 EST


On Sun, Nov 29, 2015 at 01:37:43PM +0100, Dave Penkler wrote:
> These ioctls provide support for the USBTMC-USB488 control requests
> for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
>
> Signed-off-by: Dave Penkler <dpenkler@xxxxxxxxx>
> ---
> drivers/usb/class/usbtmc.c | 71 ++++++++++++++++++++++++++++++++++++++++++++
> include/uapi/linux/usb/tmc.h | 6 ++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 3a3264c..f04a086 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -473,6 +473,62 @@ static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
> return rv;
> }
>
> +static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
> + unsigned long arg,

Same thing here with arg, so you don't have to cast it later in this
function.

> + unsigned int cmd)
> +{
> + struct device *dev = &data->intf->dev;
> + unsigned int val;
> + u8 *buffer;
> + u16 wValue;
> + int rv;
> +
> + if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
> + return -EINVAL;
> +
> + buffer = kmalloc(8, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
> + rv = copy_from_user(&val, (void __user *)arg, sizeof(val));

unsigned int is different sizes on different user/kernel implementations
(32bit user, 64bit kernel.) You need to always copy an explicit size
across this boundry, you just made this ioctl really hard to implement
correctly in userspace :(

And it doesn't match the size you told userspace to use:

> +#define USBTMC488_IOCTL_REN_CONTROL _IOW(USBTMC_IOC_NR, 19, unsigned char)

sizeof(unsigned char) != (unsigned int)

Please fix this up, using the correct types (hint, you want __u8 here),
and for the other one, and resend the series.

thanks,

greg k-h