On Tue, Jan 5, 2016 at 9:27 AM, Jann Horn <jann@xxxxxxxxx> wrote:
> This replaces all code in fs/compat_ioctl.c that translated
> ioctl arguments into a in-kernel structure, then performed
> do_ioctl under set_fs(KERNEL_DS), with code that allocates
> data on the user stack and can call the VFS ioctl handler
> under USER_DS.
>
> This is done as a hardening measure because the caller
> does not know what kind of ioctl handler will be invoked,
> only that no corresponding compat_ioctl handler exists and
> what the ioctl command number is. The accidental
> invocation of an unlocked_ioctl handler that unexpectedly
> calls copy_to_user could be a severe security issue.
>
> (This patch is only compile-tested so far.)
>
> Signed-off-by: Jann Horn <jann@xxxxxxxxx>
Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
-Kees
> ---
> fs/compat_ioctl.c | 135 ++++++++++++++++++++++++++++--------------------------
> 1 file changed, 70 insertions(+), 65 deletions(-)
>
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index c9b8d4e..6a795ba 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -115,6 +115,13 @@
> #include <asm/fbio.h>
> #endif
>
> +#define convert_in_user(srcptr, dstptr) \
> +({ \
> + typeof(*srcptr) val; \
> + \
> + get_user(val, srcptr) || put_user(val, dstptr); \
> +})
> +
> static int do_ioctl(struct file *filp, unsigned int fd,
> unsigned int cmd, unsigned long arg)
> {
> @@ -130,16 +137,17 @@ static int do_ioctl(struct file *filp, unsigned int fd,
> static int w_long(struct file *filp, unsigned int fd,
> unsigned int cmd, compat_ulong_t __user *argp)
> {
> - mm_segment_t old_fs = get_fs();
> int err;
> - unsigned long val;
> + unsigned long __user *valp = compat_alloc_user_space(sizeof(*valp));
>
> - set_fs (KERNEL_DS);
> - err = do_ioctl(filp, fd, cmd, (unsigned long)&val);
> - set_fs (old_fs);
> - if (!err && put_user(val, argp))
> + if (valp == NULL)
> return -EFAULT;
> - return err;
> + err = do_ioctl(filp, fd, cmd, (unsigned long)valp);
> + if (err)
> + return err;
> + if (convert_in_user(valp, argp))
> + return -EFAULT;
> + return 0;
> }
>
> struct compat_video_event {
> @@ -154,20 +162,20 @@ struct compat_video_event {
> static int do_video_get_event(struct file *filp, unsigned int fd,
> unsigned int cmd, struct compat_video_event __user *up)
> {
> - struct video_event kevent;
> - mm_segment_t old_fs = get_fs();
> + struct video_event __user *kevent =
> + compat_alloc_user_space(sizeof(*kevent));
> int err;
>
> - set_fs(KERNEL_DS);
> - err = do_ioctl(filp, fd, cmd, (unsigned long) &kevent);
> - set_fs(old_fs);
> + if (kevent == NULL)
> + return -EFAULT;
>
> + err = do_ioctl(filp, fd, cmd, (unsigned long)kevent);
> if (!err) {
> - err = put_user(kevent.type, &up->type);
> - err |= put_user(kevent.timestamp, &up->timestamp);
> - err |= put_user(kevent.u.size.w, &up->u.size.w);
> - err |= put_user(kevent.u.size.h, &up->u.size.h);
> - err |= put_user(kevent.u.size.aspect_ratio,
> + err = convert_in_user(&kevent->type, &up->type);
> + err |= convert_in_user(&kevent->timestamp, &up->timestamp);
> + err |= convert_in_user(&kevent->u.size.w, &up->u.size.w);
> + err |= convert_in_user(&kevent->u.size.h, &up->u.size.h);
> + err |= convert_in_user(&kevent->u.size.aspect_ratio,
> &up->u.size.aspect_ratio);
> if (err)
> err = -EFAULT;
> @@ -527,10 +535,10 @@ struct mtpos32 {
> static int mt_ioctl_trans(struct file *filp, unsigned int fd,
> unsigned int cmd, void __user *argp)
> {
> - mm_segment_t old_fs = get_fs();
> - struct mtget get;
> + /* NULL initialization to make gcc shut up */
> + struct mtget __user *get = NULL;
> struct mtget32 __user *umget32;
> - struct mtpos pos;
> + struct mtpos __user *pos = NULL;
> struct mtpos32 __user *upos32;
> unsigned long kcmd;
> void *karg;
> @@ -539,32 +547,34 @@ static int mt_ioctl_trans(struct file *filp, unsigned int fd,
> switch(cmd) {
> case MTIOCPOS32:
> kcmd = MTIOCPOS;
> - karg = &pos;
> + pos = compat_alloc_user_space(sizeof(*pos));
> + karg = pos;
> break;
> default: /* MTIOCGET32 */
> kcmd = MTIOCGET;
> - karg = &get;
> + get = compat_alloc_user_space(sizeof(*get));
> + karg = get;
> break;
> }
> - set_fs (KERNEL_DS);
> + if (karg == NULL)
> + return -EFAULT;
> err = do_ioctl(filp, fd, kcmd, (unsigned long)karg);
> - set_fs (old_fs);
> if (err)
> return err;
> switch (cmd) {
> case MTIOCPOS32:
> upos32 = argp;
> - err = __put_user(pos.mt_blkno, &upos32->mt_blkno);
> + err = convert_in_user(&pos->mt_blkno, &upos32->mt_blkno);
> break;
> case MTIOCGET32:
> umget32 = argp;
> - err = __put_user(get.mt_type, &umget32->mt_type);
> - err |= __put_user(get.mt_resid, &umget32->mt_resid);
> - err |= __put_user(get.mt_dsreg, &umget32->mt_dsreg);
> - err |= __put_user(get.mt_gstat, &umget32->mt_gstat);
> - err |= __put_user(get.mt_erreg, &umget32->mt_erreg);
> - err |= __put_user(get.mt_fileno, &umget32->mt_fileno);
> - err |= __put_user(get.mt_blkno, &umget32->mt_blkno);
> + err = convert_in_user(&get->mt_type, &umget32->mt_type);
> + err |= convert_in_user(&get->mt_resid, &umget32->mt_resid);
> + err |= convert_in_user(&get->mt_dsreg, &umget32->mt_dsreg);
> + err |= convert_in_user(&get->mt_gstat, &umget32->mt_gstat);
> + err |= convert_in_user(&get->mt_erreg, &umget32->mt_erreg);
> + err |= convert_in_user(&get->mt_fileno, &umget32->mt_fileno);
> + err |= convert_in_user(&get->mt_blkno, &umget32->mt_blkno);
> break;
> }
> return err ? -EFAULT: 0;
> @@ -623,38 +633,36 @@ static int serial_struct_ioctl(struct file *filp, unsigned fd,
> {
> typedef struct serial_struct32 SS32;
> int err;
> - struct serial_struct ss;
> - mm_segment_t oldseg = get_fs();
> + struct serial_struct __user *ss = compat_alloc_user_space(sizeof(*ss));
> __u32 udata;
> unsigned int base;
> + unsigned char *iomem_base;
>
> + if (ss == NULL)
> + return -EFAULT;
> if (cmd == TIOCSSERIAL) {
> - if (!access_ok(VERIFY_READ, ss32, sizeof(SS32)))
> - return -EFAULT;
> - if (__copy_from_user(&ss, ss32, offsetof(SS32, iomem_base)))
> + if (copy_in_user(ss, ss32, offsetof(SS32, iomem_base)) ||
> + get_user(udata, &ss32->iomem_base))
> return -EFAULT;
> - if (__get_user(udata, &ss32->iomem_base))
> + iomem_base = compat_ptr(udata);
> + if (put_user(iomem_base, &ss->iomem_base) ||
> + convert_in_user(&ss32->iomem_reg_shift,
> + &ss->iomem_reg_shift) ||
> + convert_in_user(&ss32->port_high, &ss->port_high) ||
> + put_user(0UL, &ss->iomap_base))
> return -EFAULT;
> - ss.iomem_base = compat_ptr(udata);
> - if (__get_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift) ||
> - __get_user(ss.port_high, &ss32->port_high))
> - return -EFAULT;
> - ss.iomap_base = 0UL;
> }
> - set_fs(KERNEL_DS);
> - err = do_ioctl(filp, fd, cmd,
> - (unsigned long)(&ss));
> - set_fs(oldseg);
> + err = do_ioctl(filp, fd, cmd, (unsigned long)ss);
> if (cmd == TIOCGSERIAL && err >= 0) {
> - if (!access_ok(VERIFY_WRITE, ss32, sizeof(SS32)))
> - return -EFAULT;
> - if (__copy_to_user(ss32,&ss,offsetof(SS32,iomem_base)))
> + if (copy_in_user(ss32, ss, offsetof(SS32, iomem_base)) ||
> + get_user(iomem_base, &ss->iomem_base))
> return -EFAULT;
> - base = (unsigned long)ss.iomem_base >> 32 ?
> - 0xffffffff : (unsigned)(unsigned long)ss.iomem_base;
> - if (__put_user(base, &ss32->iomem_base) ||
> - __put_user(ss.iomem_reg_shift, &ss32->iomem_reg_shift) ||
> - __put_user(ss.port_high, &ss32->port_high))
> + base = (unsigned long)iomem_base >> 32 ?
> + 0xffffffff : (unsigned)(unsigned long)iomem_base;
> + if (put_user(base, &ss32->iomem_base) ||
> + convert_in_user(&ss->iomem_reg_shift,
> + &ss32->iomem_reg_shift) ||
> + convert_in_user(&ss->port_high, &ss32->port_high))
> return -EFAULT;
> }
> return err;
> @@ -759,27 +767,24 @@ static int do_i2c_smbus_ioctl(struct file *filp, unsigned int fd,
> static int rtc_ioctl(struct file *filp, unsigned fd,
> unsigned cmd, void __user *argp)
> {
> - mm_segment_t oldfs = get_fs();
> - compat_ulong_t val32;
> - unsigned long kval;
> + unsigned long __user *valp = compat_alloc_user_space(sizeof(*valp));
> int ret;
>
> + if (valp == NULL)
> + return -EFAULT;
> switch (cmd) {
> case RTC_IRQP_READ32:
> case RTC_EPOCH_READ32:
> - set_fs(KERNEL_DS);
> ret = do_ioctl(filp, fd, (cmd == RTC_IRQP_READ32) ?
> RTC_IRQP_READ : RTC_EPOCH_READ,
> - (unsigned long)&kval);
> - set_fs(oldfs);
> + (unsigned long)valp);
> if (ret)
> return ret;
> - val32 = kval;
> - return put_user(val32, (unsigned int __user *)argp);
> + return convert_in_user(valp, (unsigned int __user *)argp);
> case RTC_IRQP_SET32:
> - return sys_ioctl(fd, RTC_IRQP_SET, (unsigned long)argp);
> + return do_ioctl(filp, fd, RTC_IRQP_SET, (unsigned long)argp);
> case RTC_EPOCH_SET32:
> - return sys_ioctl(fd, RTC_EPOCH_SET, (unsigned long)argp);
> + return do_ioctl(filp, fd, RTC_EPOCH_SET, (unsigned long)argp);
> }
>
> return -ENOIOCTLCMD;
> --
> 2.1.4
>
--
Kees Cook
Chrome OS & Brillo Security
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
