diff mbox

[2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS)

Message ID 1452014850-19354-2-git-send-email-jann@thejh.net (mailing list archive)
State New, archived
Headers show

Commit Message

Jann Horn Jan. 5, 2016, 5:27 p.m. UTC 
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@thejh.net>
---
 fs/compat_ioctl.c | 135 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 70 insertions(+), 65 deletions(-)

Comments

Kees Cook Jan. 5, 2016, 7:35 p.m. UTC | #1 
On Tue, Jan 5, 2016 at 9:27 AM, Jann Horn <jann@thejh.net> 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@thejh.net>

Acked-by: Kees Cook <keescook@chromium.org>

-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
>
diff mbox

Patch

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;