Re: [PATCH 2/2] rtc: move compat_ioctl handling into rtc-dev.c

[Al Viro <viro@ZenIV.linux.org.uk>]

On Mon, Aug 27, 2018 at 10:03:09PM +0200, Arnd Bergmann wrote:

> +#ifdef CONFIG_COMPAT
> +#define RTC_IRQP_READ32		_IOR('p', 0x0b, compat_ulong_t)
> +#define RTC_IRQP_SET32		_IOW('p', 0x0c, compat_ulong_t)
> +#define RTC_EPOCH_READ32	_IOR('p', 0x0d, compat_ulong_t)
> +#define RTC_EPOCH_SET32		_IOW('p', 0x0e, compat_ulong_t)
> +
> +static long rtc_dev_compat_ioctl(struct file *file, unsigned cmd, unsigned long arg)
> +{
> +	unsigned long __user *valp;
> +	compat_ulong_t __user *argp = compat_ptr(arg);
> +	unsigned long val;
> +	int ret;
> +
> +	switch (cmd) {
> +	/* pointer to 'long' needs translation. */
> +	case RTC_IRQP_READ32:
> +	case RTC_EPOCH_READ32: {
> +		valp = compat_alloc_user_space(sizeof(*valp));
> +		if (valp == NULL)
> +			return -EFAULT;
> +		ret = rtc_dev_ioctl(file, (cmd == RTC_IRQP_READ32) ?
> +					RTC_IRQP_READ : RTC_EPOCH_READ,
> +					(unsigned long)valp);
> +		if (ret)
> +			return ret;
> +		if (get_user(val, valp) || put_user(val, argp))
> +			return -EFAULT;

	No.  With the side of Hell, No.  This is bloody ridiculous - take a look
at rtc_dev_ioctl().  Seriously.  Relevant bits:
        struct rtc_device *rtc = file->private_data;
        const struct rtc_class_ops *ops = rtc->ops;
        struct rtc_time tm;
        struct rtc_wkalrm alarm;
        void __user *uarg = (void __user *) arg;

        err = mutex_lock_interruptible(&rtc->ops_lock);
        if (err)
                return err;

                err = put_user(rtc->irq_freq, (unsigned long __user *)uarg);

        mutex_unlock(&rtc->ops_lock);
        return err;

That's RTC_IRQP_READ.  IOW, all that song and dance starting with
compat_alloc_user_space() is just a way to get to rtc->irq_freq value.


RTC_EPOCH_READ is in the bowels of individual implementations, but it's
otherwise very similar; add a method returning the damn thing (e.g.
rtc->ops->get_epoch(rtc)) and lift its call into rtc_dev_ioctl(), then
this nonsense will go away as well.

> +		return 0;
> +	}
> +
> +	/* arguments are compatible, command number is not */
> +	case RTC_IRQP_SET32:
> +		return rtc_dev_ioctl(file, RTC_IRQP_SET, arg);
ITYM
		cmd = RTC_IRQP_SET;
		break;
> +	case RTC_EPOCH_SET32:
... and
		cmd = RTC_EPOCH_SET;
		break;
here.
> +		return rtc_dev_ioctl(file, RTC_EPOCH_SET, arg);
> +	}
> +
> +	/*
> +	 * all other rtc ioctls are compatible, or only
> +	 * exist on architectures without compat mode
> +	 */
> +
> +	return rtc_dev_ioctl(file, cmd, (unsigned long)argp);
> +}

compat_alloc_user_space() is usually papering over bogosities like
that; if you are moving something out to individual drivers, it's
always a red flag - most of the time it'll turn out to be pointless.