Re: [RFC v2.1 1/1] media: Support variable size IOCTL arguments

[Mauro Carvalho Chehab <mchehab@s-opensource.com>]

Em Mon, 26 Mar 2018 16:23:24 +0300
Sakari Ailus <sakari.ailus@linux.intel.com> escreveu:

> Maintain a list of supported IOCTL argument sizes and allow only those in
> the list.
> 
> As an additional bonus, IOCTL handlers will be able to check whether the
> caller actually set (using the argument size) the field vs. assigning it
> to zero. Separate macro can be provided for that.
> 
> This will be easier for applications as well since there is no longer the
> problem of setting the reserved fields zero, or at least it is a lesser
> problem.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
> ---
> Hi folks,
> 
> I've essentially addressed Mauro's comments on v2.
> 
> The code is only compile tested so far but the changes from the last
> tested version are not that big. There's still some uncertainty though.

You should test it... I guess there is a bug on this version :-)
(see below)

> 
> since v2:
> 
> - Rework is_valid_ioctl based on the comments
> 
> 	- Improved comments,
> 	
> 	- Rename cmd as user_cmd, as this comes from the user
> 	
> 	- Check whether there are alternative argument sizes before any
> 	  checks on IOCTL command if there is no exact match
> 	  
> 	- Use IOCSIZE_MASK macro instead of creating our own
> 
> - Add documentation for macros declaring IOCTLs
> 
> 
>  drivers/media/media-device.c | 98 +++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 92 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7..279d740 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -387,22 +387,65 @@ static long copy_arg_to_user(void __user *uarg, void *karg, unsigned int cmd)
>  /* Do acquire the graph mutex */
>  #define MEDIA_IOC_FL_GRAPH_MUTEX	BIT(0)
>  
> -#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +/**
> + * MEDIA_IOC_SZ_ARG - Declare a Media device IOCTL with alternative size and
> + *		      to_user/from_user callbacks
> + *
> + * @__cmd:	The IOCTL command suffix (without "MEDIA_IOC_")
> + * @func:	The handler function
> + * @fl:		Flags from @enum media_ioc_flags
> + * @alt_sz:	A 0-terminated list of alternative argument struct sizes.
> + * @from_user:	Function to copy argument struct from the user to the kernel
> + * @to_user:	Function to copy argument struct to the user from the kernel
> + */
> +#define MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz, from_user, to_user)	\
>  	[_IOC_NR(MEDIA_IOC_##__cmd)] = {				\
>  		.cmd = MEDIA_IOC_##__cmd,				\
>  		.fn = (long (*)(struct media_device *, void *))func,	\
>  		.flags = fl,						\
> +		.alt_arg_sizes = alt_sz,				\
>  		.arg_from_user = from_user,				\
>  		.arg_to_user = to_user,					\
>  	}
>  
> -#define MEDIA_IOC(__cmd, func, fl)					\
> -	MEDIA_IOC_ARG(__cmd, func, fl, copy_arg_from_user, copy_arg_to_user)
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL with to_user/from_user callbacks
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list.
> + */

Nitpick: either use:
	/*
	 *...
	 */

or add the arguments to the macro there, as /** ... */ expects
the arguments. Same for other comments below.

> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user)		\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> +
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL with alternative argument struct
> + *		   sizes
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the callbacks to copy the data from the
> + * user space and back to user space.
> + */
> +#define MEDIA_IOC_SZ(__cmd, func, fl, alt_sz)			\
> +	MEDIA_IOC_SZ_ARG(__cmd, func, fl, alt_sz,		\
> +			 copy_arg_from_user, copy_arg_to_user)
> +
> +/**
> + * MEDIA_IOC_ARG - Declare a Media device IOCTL
> + *
> + * Just as MEDIA_IOC_SZ_ARG but without the alternative size list or the
> + * callbacks to copy the data from the user space and back to user space.
> + */
> +#define MEDIA_IOC(__cmd, func, fl)				\
> +	MEDIA_IOC_ARG(__cmd, func, fl,				\
> +		      copy_arg_from_user, copy_arg_to_user)
>  
>  /* the table is indexed by _IOC_NR(cmd) */
>  struct media_ioctl_info {
>  	unsigned int cmd;
>  	unsigned short flags;
> +	/*
> +	 * Sizes of the alternative arguments. If there are none, this
> +	 * pointer is NULL.
> +	 */
> +	const unsigned short *alt_arg_sizes;
>  	long (*fn)(struct media_device *dev, void *arg);
>  	long (*arg_from_user)(void *karg, void __user *uarg, unsigned int cmd);
>  	long (*arg_to_user)(void __user *uarg, void *karg, unsigned int cmd);
> @@ -416,6 +459,46 @@ static const struct media_ioctl_info ioctl_info[] = {
>  	MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
>  };
>  
> +static inline long is_valid_ioctl(unsigned int user_cmd)
> +{
> +	const struct media_ioctl_info *info = ioctl_info;
> +	const unsigned short *alt_arg_sizes;
> +
> +	if (_IOC_NR(user_cmd) >= ARRAY_SIZE(ioctl_info))
> +		return -ENOIOCTLCMD;
> +
> +	info += _IOC_NR(user_cmd);
> +
> +	if (user_cmd == info->cmd)
> +		return 0;
> +
> +	/*
> +	 * There was no exact match between the user-passed IOCTL command and
> +	 * the definition. Are there earlier revisions of the argument struct
> +	 * available?
> +	 */
> +	if (!info->alt_arg_sizes)
> +		return -ENOIOCTLCMD;
> +
> +	/*
> +	 * Variable size IOCTL argument support allows using either the latest
> +	 * revision of the IOCTL argument struct or an earlier version. Check
> +	 * that the size-independent portions of the IOCTL command match and
> +	 * that the size matches with one of the alternative sizes that
> +	 * represent earlier revisions of the argument struct.
> +	 */
> +	if ((user_cmd & ~IOCSIZE_MASK) != (info->cmd & ~IOCSIZE_MASK)
> +	    || _IOC_SIZE(user_cmd) < _IOC_SIZE(info->cmd))
> +		return -ENOIOCTLCMD;

I guess it should be, instead:

	    || _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))

The hole idea is that the struct sizes used by ioctls can monotonically
increase as newer fields become needed, but never decrease.

Assuming that, _IOC_SIZE(MEDIA_IOC_foo) will give the size of the
latest version of an ioctl supported by a given Kernel version,
while alt_arg_sizes will list smaller sizes from previous
Kernel versions that will also be accepted, in order to make it
backward-compatible with apps compiled against older Kernel headers.

However, if an application is compiled with a kernel newer than
the current one, it should fail, as an older Kernel doesn't know
how to handle the newer fields. So, it should be up to the userspace
app to add backward-compatible code if it needs to support older
Kernels.

(perhaps it should be worth adding a comment like the above
somewhere).

> +
> +	for (alt_arg_sizes = info->alt_arg_sizes; *alt_arg_sizes;
> +	     alt_arg_sizes++)
> +		if (_IOC_SIZE(user_cmd) == *alt_arg_sizes)
> +			return 0;
> +
> +	return -ENOIOCTLCMD;
> +}
> +
>  static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			       unsigned long __arg)
>  {
> @@ -426,9 +509,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  	char __karg[256], *karg = __karg;
>  	long ret;
>  
> -	if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info)
> -	    || ioctl_info[_IOC_NR(cmd)].cmd != cmd)
> -		return -ENOIOCTLCMD;
> +	ret = is_valid_ioctl(cmd);
> +	if (ret)
> +		return ret;
>  
>  	info = &ioctl_info[_IOC_NR(cmd)];
>  
> @@ -444,6 +527,9 @@ static long media_device_ioctl(struct file *filp, unsigned int cmd,
>  			goto out_free;
>  	}
>  
> +	/* Set the rest of the argument struct to zero */
> +	memset(karg + _IOC_SIZE(cmd), 0, _IOC_SIZE(info->cmd) - _IOC_SIZE(cmd));
> +
>  	if (info->flags & MEDIA_IOC_FL_GRAPH_MUTEX)
>  		mutex_lock(&dev->graph_mutex);
>  

Regards,
Mauro