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.
