From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1755296AbcAYErx (ORCPT <rfc822;w@1wt.eu>);
	Sun, 24 Jan 2016 23:47:53 -0500
Received: from mail.linuxfoundation.org ([140.211.169.12]:59935 "EHLO
	mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1754632AbcAYErt (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sun, 24 Jan 2016 23:47:49 -0500
Date: Sun, 24 Jan 2016 20:47:48 -0800
From: Greg KH <gregkh@linuxfoundation.org>
To: Dave Penkler <dpenkler@gmail.com>
Cc: peter.chen@freescale.com, teuniz@gmail.com, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 5/5] Add ioctls to enable and disable local controls
 on an instrument
Message-ID: <20160125044748.GC14247@kroah.com>
References: <20151129122251.GA7490@slacky>
 <20151129123743.GE7499@slacky>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20151129123743.GE7499@slacky>
User-Agent: Mutt/1.5.24 (2015-08-30)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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@gmail.com>
> ---
>  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