Em Fri, 23 Mar 2018 23:17:35 +0200
Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx> 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.
Patch makes sense to me, but I have a few comments on it.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Acked-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/media-device.c | 65 ++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 59 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
> index 35e81f7..da63da1 100644
> --- a/drivers/media/media-device.c
> +++ b/drivers/media/media-device.c
> @@ -387,22 +387,36 @@ 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) \
> +#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)
> +#define MEDIA_IOC_ARG(__cmd, func, fl, from_user, to_user) \
> + MEDIA_IOC_SZ_ARG(__cmd, func, fl, NULL, from_user, to_user)
> +
> +#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)
> +
> +#define MEDIA_IOC(__cmd, func, fl) \
> + MEDIA_IOC_ARG(__cmd, func, fl, \
> + copy_arg_from_user, copy_arg_to_user)
Please add some comments to those macros (specially the first one,
as the names of the values are too short to help identifying what
they are.
>
> /* 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 +430,42 @@ static const struct media_ioctl_info ioctl_info[] = {
> MEDIA_IOC(G_TOPOLOGY, media_device_get_topology, MEDIA_IOC_FL_GRAPH_MUTEX),
> };
>
> +#define MASK_IOC_SIZE(cmd) \
> + ((cmd) & ~(_IOC_SIZEMASK << _IOC_SIZESHIFT))
This should be, instead at:
include/uapi/asm-generic/ioctl.h
The patch series adding it there should also touch the other usecases
for _IOC_SIZEMASK (evdev.c, phantom.c, v4l2-ioctl.c).
> +
> +static inline long is_valid_ioctl(unsigned int cmd)
Please rename from "cmd" to "user_cmd", in order to make it
clearer that it contains the value passed by userspace.
> +{
> + const struct media_ioctl_info *info = ioctl_info;
> + const unsigned short *alt_arg_sizes;
> +
> + if (_IOC_NR(cmd) >= ARRAY_SIZE(ioctl_info))
> + return -ENOIOCTLCMD;
> +
> + info += _IOC_NR(cmd);
> +
> + if (info->cmd == cmd)
> + return 0;
Please revert it:
if (user_cmd == info->cmd)
return 0
As we're validating if the userspace ioctl code makes sense,
and not the reverse.
Please add the check if alt_arg_sizes is defined here:
alt_arg_sizes = info->alt_arg_sizes;
if (!alt_arg_sizes)
return -ENOIOCTLCMD;
As the remaining code is not needed if user_cmd != info_cmd.
> +
> + /*
> + * Verify that the size-dependent patch of the IOCTL command
> + * matches and that the size does not exceed the principal
> + * argument size.
> + */
what do you mean by "principal argument size"? I guess what you're
meaning here is the "argument size of the latest version" with is
always bigger than the previous version. If so, make it clear.
I would write it as something like:
/*
* As the ioctl passed by userspace doesn't match the current
* one, and there are alternate sizes for thsi ioctl,
* we need to check if the ioctl code is correct.
*
* Validate that the ioctl code passed by userspace matches the
* Kernel definition with regards to its number, type and dir.
* Also checks if the size is not bigger than the one defined
* by the latest version of the ioctl.
*/
> + if (MASK_IOC_SIZE(info->cmd) != MASK_IOC_SIZE(cmd)
> + || _IOC_SIZE(info->cmd) < _IOC_SIZE(cmd))
> + return -ENOIOCTLCMD;
I would invert the check: what we want do to is to check if whatever
userspace passes is valid. So, better to place user_cmd as the first
arguments at the check, e. g.:
if (MASK_IOC_SIZE(user_cmd) != MASK_IOC_SIZE(info->cmd)
|| _IOC_SIZE(user_cmd) > _IOC_SIZE(info->cmd))
return -ENOIOCTLCMD;
> +
> + alt_arg_sizes = info->alt_arg_sizes;
> + if (!alt_arg_sizes)
> + return -ENOIOCTLCMD;
As said before, this check should happen earlier.
> +
> + for (; *alt_arg_sizes; alt_arg_sizes++)
> + if (_IOC_SIZE(cmd) == *alt_arg_sizes)
> + return 0;
> +
> + return -ENOIOCTLCMD;
> +}
> +
> static long media_device_ioctl(struct file *filp, unsigned int cmd,
> unsigned long __arg)
> {
> @@ -426,9 +476,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 +494,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);
>
Thanks,
Mauro
