* [PATCH 1/2] compat_ioctl: don't look up the fd twice
@ 2016-01-05 17:27 Jann Horn
2016-01-05 17:27 ` [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS) Jann Horn
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Jann Horn @ 2016-01-05 17:27 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel; +Cc: security, Kees Cook, Andy Lutomirski
In code in fs/compat_ioctl.c that translates ioctl arguments
into a in-kernel structure, then performs sys_ioctl, possibly
under set_fs(KERNEL_DS), this commit changes the sys_ioctl
calls to do_ioctl calls. do_ioctl is a new function that does
the same thing as sys_ioctl, but doesn't look up the fd again.
This change is made to avoid (potential) security issues
because of ioctl handlers that accept one of the ioctl
commands I2C_FUNCS, VIDEO_GET_EVENT, MTIOCPOS, MTIOCGET,
TIOCGSERIAL, TIOCSSERIAL, RTC_IRQP_READ, RTC_EPOCH_READ.
This can happen for multiple reasons:
- The ioctl command number could be reused.
- The ioctl handler might not check the full ioctl
command. This is e.g. true for drm_ioctl.
- The ioctl handler is very special, e.g. cuse_file_ioctl
The real issue is that set_fs(KERNEL_DS) is used here,
but that's fixed in a separate commit
"compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS)".
This change mitigates potential security issues by
preventing a race that permits invocation of
unlocked_ioctl handlers under KERNEL_DS through compat
code even if a corresponding compat_ioctl handler exists.
So far, no way has been identified to use this to damage
kernel memory without having CAP_SYS_ADMIN in the init ns
(with the capability, doing reads/writes at arbitrary
kernel addresses should be easy through CUSE's ioctl
handler with FUSE_IOCTL_UNRESTRICTED set).
(This patch is only compile-tested so far.)
Signed-off-by: Jann Horn <jann@thejh.net>
---
fs/compat_ioctl.c | 119 ++++++++++++++++++++++++++++++------------------------
1 file changed, 67 insertions(+), 52 deletions(-)
diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
index dcf2653..c9b8d4e 100644
--- a/fs/compat_ioctl.c
+++ b/fs/compat_ioctl.c
@@ -115,15 +115,27 @@
#include <asm/fbio.h>
#endif
-static int w_long(unsigned int fd, unsigned int cmd,
- compat_ulong_t __user *argp)
+static int do_ioctl(struct file *filp, unsigned int fd,
+ unsigned int cmd, unsigned long arg)
+{
+ int err;
+
+ err = security_file_ioctl(filp, cmd, arg);
+ if (err)
+ return err;
+
+ return do_vfs_ioctl(filp, fd, cmd, arg);
+}
+
+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;
set_fs (KERNEL_DS);
- err = sys_ioctl(fd, cmd, (unsigned long)&val);
+ err = do_ioctl(filp, fd, cmd, (unsigned long)&val);
set_fs (old_fs);
if (!err && put_user(val, argp))
return -EFAULT;
@@ -139,15 +151,15 @@ struct compat_video_event {
} u;
};
-static int do_video_get_event(unsigned int fd, unsigned int cmd,
- struct compat_video_event __user *up)
+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();
int err;
set_fs(KERNEL_DS);
- err = sys_ioctl(fd, cmd, (unsigned long) &kevent);
+ err = do_ioctl(filp, fd, cmd, (unsigned long) &kevent);
set_fs(old_fs);
if (!err) {
@@ -169,8 +181,8 @@ struct compat_video_still_picture {
int32_t size;
};
-static int do_video_stillpicture(unsigned int fd, unsigned int cmd,
- struct compat_video_still_picture __user *up)
+static int do_video_stillpicture(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct compat_video_still_picture __user *up)
{
struct video_still_picture __user *up_native;
compat_uptr_t fp;
@@ -190,7 +202,7 @@ static int do_video_stillpicture(unsigned int fd, unsigned int cmd,
if (err)
return -EFAULT;
- err = sys_ioctl(fd, cmd, (unsigned long) up_native);
+ err = do_ioctl(filp, fd, cmd, (unsigned long) up_native);
return err;
}
@@ -200,8 +212,8 @@ struct compat_video_spu_palette {
compat_uptr_t palette;
};
-static int do_video_set_spu_palette(unsigned int fd, unsigned int cmd,
- struct compat_video_spu_palette __user *up)
+static int do_video_set_spu_palette(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct compat_video_spu_palette __user *up)
{
struct video_spu_palette __user *up_native;
compat_uptr_t palp;
@@ -218,7 +230,7 @@ static int do_video_set_spu_palette(unsigned int fd, unsigned int cmd,
if (err)
return -EFAULT;
- err = sys_ioctl(fd, cmd, (unsigned long) up_native);
+ err = do_ioctl(filp, fd, cmd, (unsigned long) up_native);
return err;
}
@@ -276,7 +288,7 @@ static int sg_build_iovec(sg_io_hdr_t __user *sgio, void __user *dxferp, u16 iov
return 0;
}
-static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
+static int sg_ioctl_trans(struct file *filp, unsigned int fd, unsigned int cmd,
sg_io_hdr32_t __user *sgio32)
{
sg_io_hdr_t __user *sgio;
@@ -289,7 +301,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
if (get_user(interface_id, &sgio32->interface_id))
return -EFAULT;
if (interface_id != 'S')
- return sys_ioctl(fd, cmd, (unsigned long)sgio32);
+ return do_ioctl(filp, fd, cmd, (unsigned long)sgio32);
if (get_user(iovec_count, &sgio32->iovec_count))
return -EFAULT;
@@ -349,7 +361,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
if (put_user(compat_ptr(data), &sgio->usr_ptr))
return -EFAULT;
- err = sys_ioctl(fd, cmd, (unsigned long) sgio);
+ err = do_ioctl(filp, fd, cmd, (unsigned long) sgio);
if (err >= 0) {
void __user *datap;
@@ -380,13 +392,13 @@ struct compat_sg_req_info { /* used by SG_GET_REQUEST_TABLE ioctl() */
int unused;
};
-static int sg_grt_trans(unsigned int fd, unsigned int cmd, struct
- compat_sg_req_info __user *o)
+static int sg_grt_trans(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct compat_sg_req_info __user *o)
{
int err, i;
sg_req_info_t __user *r;
r = compat_alloc_user_space(sizeof(sg_req_info_t)*SG_MAX_QUEUE);
- err = sys_ioctl(fd,cmd,(unsigned long)r);
+ err = do_ioctl(filp, fd, cmd, (unsigned long)r);
if (err < 0)
return err;
for (i = 0; i < SG_MAX_QUEUE; i++) {
@@ -412,8 +424,8 @@ struct sock_fprog32 {
#define PPPIOCSPASS32 _IOW('t', 71, struct sock_fprog32)
#define PPPIOCSACTIVE32 _IOW('t', 70, struct sock_fprog32)
-static int ppp_sock_fprog_ioctl_trans(unsigned int fd, unsigned int cmd,
- struct sock_fprog32 __user *u_fprog32)
+static int ppp_sock_fprog_ioctl_trans(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct sock_fprog32 __user *u_fprog32)
{
struct sock_fprog __user *u_fprog64 = compat_alloc_user_space(sizeof(struct sock_fprog));
void __user *fptr64;
@@ -435,7 +447,7 @@ static int ppp_sock_fprog_ioctl_trans(unsigned int fd, unsigned int cmd,
else
cmd = PPPIOCSACTIVE;
- return sys_ioctl(fd, cmd, (unsigned long) u_fprog64);
+ return do_ioctl(filp, fd, cmd, (unsigned long) u_fprog64);
}
struct ppp_option_data32 {
@@ -451,7 +463,7 @@ struct ppp_idle32 {
};
#define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32)
-static int ppp_gidle(unsigned int fd, unsigned int cmd,
+static int ppp_gidle(struct file *filp, unsigned int fd, unsigned int cmd,
struct ppp_idle32 __user *idle32)
{
struct ppp_idle __user *idle;
@@ -460,7 +472,7 @@ static int ppp_gidle(unsigned int fd, unsigned int cmd,
idle = compat_alloc_user_space(sizeof(*idle));
- err = sys_ioctl(fd, PPPIOCGIDLE, (unsigned long) idle);
+ err = do_ioctl(filp, fd, PPPIOCGIDLE, (unsigned long) idle);
if (!err) {
if (get_user(xmit, &idle->xmit_idle) ||
@@ -472,7 +484,7 @@ static int ppp_gidle(unsigned int fd, unsigned int cmd,
return err;
}
-static int ppp_scompress(unsigned int fd, unsigned int cmd,
+static int ppp_scompress(struct file *filp, unsigned int fd, unsigned int cmd,
struct ppp_option_data32 __user *odata32)
{
struct ppp_option_data __user *odata;
@@ -492,7 +504,7 @@ static int ppp_scompress(unsigned int fd, unsigned int cmd,
sizeof(__u32) + sizeof(int)))
return -EFAULT;
- return sys_ioctl(fd, PPPIOCSCOMPRESS, (unsigned long) odata);
+ return do_ioctl(filp, fd, PPPIOCSCOMPRESS, (unsigned long) odata);
}
#ifdef CONFIG_BLOCK
@@ -512,7 +524,8 @@ struct mtpos32 {
};
#define MTIOCPOS32 _IOR('m', 3, struct mtpos32)
-static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
+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;
@@ -534,7 +547,7 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
break;
}
set_fs (KERNEL_DS);
- err = sys_ioctl (fd, kcmd, (unsigned long)karg);
+ err = do_ioctl(filp, fd, kcmd, (unsigned long)karg);
set_fs (old_fs);
if (err)
return err;
@@ -605,8 +618,8 @@ struct serial_struct32 {
compat_int_t reserved[1];
};
-static int serial_struct_ioctl(unsigned fd, unsigned cmd,
- struct serial_struct32 __user *ss32)
+static int serial_struct_ioctl(struct file *filp, unsigned fd,
+ unsigned cmd, struct serial_struct32 __user *ss32)
{
typedef struct serial_struct32 SS32;
int err;
@@ -629,7 +642,8 @@ static int serial_struct_ioctl(unsigned fd, unsigned cmd,
ss.iomap_base = 0UL;
}
set_fs(KERNEL_DS);
- err = sys_ioctl(fd,cmd,(unsigned long)(&ss));
+ err = do_ioctl(filp, fd, cmd,
+ (unsigned long)(&ss));
set_fs(oldseg);
if (cmd == TIOCGSERIAL && err >= 0) {
if (!access_ok(VERIFY_WRITE, ss32, sizeof(SS32)))
@@ -674,8 +688,8 @@ struct i2c_rdwr_aligned {
struct i2c_msg msgs[0];
};
-static int do_i2c_rdwr_ioctl(unsigned int fd, unsigned int cmd,
- struct i2c_rdwr_ioctl_data32 __user *udata)
+static int do_i2c_rdwr_ioctl(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct i2c_rdwr_ioctl_data32 __user *udata)
{
struct i2c_rdwr_aligned __user *tdata;
struct i2c_msg __user *tmsgs;
@@ -708,11 +722,11 @@ static int do_i2c_rdwr_ioctl(unsigned int fd, unsigned int cmd,
put_user(compat_ptr(datap), &tmsgs[i].buf))
return -EFAULT;
}
- return sys_ioctl(fd, cmd, (unsigned long)tdata);
+ return do_ioctl(filp, fd, cmd, (unsigned long)tdata);
}
-static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
- struct i2c_smbus_ioctl_data32 __user *udata)
+static int do_i2c_smbus_ioctl(struct file *filp, unsigned int fd,
+ unsigned int cmd, struct i2c_smbus_ioctl_data32 __user *udata)
{
struct i2c_smbus_ioctl_data __user *tdata;
compat_caddr_t datap;
@@ -734,7 +748,7 @@ static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
__put_user(compat_ptr(datap), &tdata->data))
return -EFAULT;
- return sys_ioctl(fd, cmd, (unsigned long)tdata);
+ return do_ioctl(filp, fd, cmd, (unsigned long)tdata);
}
#define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t)
@@ -742,7 +756,8 @@ static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
#define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t)
#define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t)
-static int rtc_ioctl(unsigned fd, unsigned cmd, void __user *argp)
+static int rtc_ioctl(struct file *filp, unsigned fd,
+ unsigned cmd, void __user *argp)
{
mm_segment_t oldfs = get_fs();
compat_ulong_t val32;
@@ -753,7 +768,7 @@ static int rtc_ioctl(unsigned fd, unsigned cmd, void __user *argp)
case RTC_IRQP_READ32:
case RTC_EPOCH_READ32:
set_fs(KERNEL_DS);
- ret = sys_ioctl(fd, (cmd == RTC_IRQP_READ32) ?
+ ret = do_ioctl(filp, fd, (cmd == RTC_IRQP_READ32) ?
RTC_IRQP_READ : RTC_EPOCH_READ,
(unsigned long)&kval);
set_fs(oldfs);
@@ -1443,46 +1458,46 @@ static long do_ioctl_trans(int fd, unsigned int cmd,
switch (cmd) {
case PPPIOCGIDLE32:
- return ppp_gidle(fd, cmd, argp);
+ return ppp_gidle(file, fd, cmd, argp);
case PPPIOCSCOMPRESS32:
- return ppp_scompress(fd, cmd, argp);
+ return ppp_scompress(file, fd, cmd, argp);
case PPPIOCSPASS32:
case PPPIOCSACTIVE32:
- return ppp_sock_fprog_ioctl_trans(fd, cmd, argp);
+ return ppp_sock_fprog_ioctl_trans(file, fd, cmd, argp);
#ifdef CONFIG_BLOCK
case SG_IO:
- return sg_ioctl_trans(fd, cmd, argp);
+ return sg_ioctl_trans(file, fd, cmd, argp);
case SG_GET_REQUEST_TABLE:
- return sg_grt_trans(fd, cmd, argp);
+ return sg_grt_trans(file, fd, cmd, argp);
case MTIOCGET32:
case MTIOCPOS32:
- return mt_ioctl_trans(fd, cmd, argp);
+ return mt_ioctl_trans(file, fd, cmd, argp);
#endif
/* Serial */
case TIOCGSERIAL:
case TIOCSSERIAL:
- return serial_struct_ioctl(fd, cmd, argp);
+ return serial_struct_ioctl(file, fd, cmd, argp);
/* i2c */
case I2C_FUNCS:
- return w_long(fd, cmd, argp);
+ return w_long(file, fd, cmd, argp);
case I2C_RDWR:
- return do_i2c_rdwr_ioctl(fd, cmd, argp);
+ return do_i2c_rdwr_ioctl(file, fd, cmd, argp);
case I2C_SMBUS:
- return do_i2c_smbus_ioctl(fd, cmd, argp);
+ return do_i2c_smbus_ioctl(file, fd, cmd, argp);
/* Not implemented in the native kernel */
case RTC_IRQP_READ32:
case RTC_IRQP_SET32:
case RTC_EPOCH_READ32:
case RTC_EPOCH_SET32:
- return rtc_ioctl(fd, cmd, argp);
+ return rtc_ioctl(file, fd, cmd, argp);
/* dvb */
case VIDEO_GET_EVENT:
- return do_video_get_event(fd, cmd, argp);
+ return do_video_get_event(file, fd, cmd, argp);
case VIDEO_STILLPICTURE:
- return do_video_stillpicture(fd, cmd, argp);
+ return do_video_stillpicture(file, fd, cmd, argp);
case VIDEO_SET_SPU_PALETTE:
- return do_video_set_spu_palette(fd, cmd, argp);
+ return do_video_set_spu_palette(file, fd, cmd, argp);
}
/*
--
2.1.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS)
2016-01-05 17:27 [PATCH 1/2] compat_ioctl: don't look up the fd twice Jann Horn
@ 2016-01-05 17:27 ` Jann Horn
2016-01-05 19:35 ` Kees Cook
2016-01-05 19:27 ` [PATCH 1/2] compat_ioctl: don't look up the fd twice Kees Cook
[not found] ` <CA+55aFzX9Gm=t=0HSnMxDeZmHkiZDMRKrBBvBw4CwPK1EbNbUQ@mail.gmail.com>
2 siblings, 1 reply; 7+ messages in thread
From: Jann Horn @ 2016-01-05 17:27 UTC (permalink / raw)
To: Alexander Viro, linux-fsdevel; +Cc: security, Kees Cook, Andy Lutomirski
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(-)
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] compat_ioctl: don't look up the fd twice
2016-01-05 17:27 [PATCH 1/2] compat_ioctl: don't look up the fd twice Jann Horn
2016-01-05 17:27 ` [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS) Jann Horn
@ 2016-01-05 19:27 ` Kees Cook
[not found] ` <CA+55aFzX9Gm=t=0HSnMxDeZmHkiZDMRKrBBvBw4CwPK1EbNbUQ@mail.gmail.com>
2 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2016-01-05 19:27 UTC (permalink / raw)
To: Jann Horn; +Cc: Alexander Viro, linux-fsdevel, security, Andy Lutomirski
On Tue, Jan 5, 2016 at 9:27 AM, Jann Horn <jann@thejh.net> wrote:
> In code in fs/compat_ioctl.c that translates ioctl arguments
> into a in-kernel structure, then performs sys_ioctl, possibly
> under set_fs(KERNEL_DS), this commit changes the sys_ioctl
> calls to do_ioctl calls. do_ioctl is a new function that does
> the same thing as sys_ioctl, but doesn't look up the fd again.
>
> This change is made to avoid (potential) security issues
> because of ioctl handlers that accept one of the ioctl
> commands I2C_FUNCS, VIDEO_GET_EVENT, MTIOCPOS, MTIOCGET,
> TIOCGSERIAL, TIOCSSERIAL, RTC_IRQP_READ, RTC_EPOCH_READ.
> This can happen for multiple reasons:
>
> - The ioctl command number could be reused.
> - The ioctl handler might not check the full ioctl
> command. This is e.g. true for drm_ioctl.
> - The ioctl handler is very special, e.g. cuse_file_ioctl
>
> The real issue is that set_fs(KERNEL_DS) is used here,
> but that's fixed in a separate commit
> "compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS)".
>
> This change mitigates potential security issues by
> preventing a race that permits invocation of
> unlocked_ioctl handlers under KERNEL_DS through compat
> code even if a corresponding compat_ioctl handler exists.
>
> So far, no way has been identified to use this to damage
> kernel memory without having CAP_SYS_ADMIN in the init ns
> (with the capability, doing reads/writes at arbitrary
> kernel addresses should be easy through CUSE's ioctl
> handler with FUSE_IOCTL_UNRESTRICTED set).
>
> (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 | 119 ++++++++++++++++++++++++++++++------------------------
> 1 file changed, 67 insertions(+), 52 deletions(-)
>
> diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c
> index dcf2653..c9b8d4e 100644
> --- a/fs/compat_ioctl.c
> +++ b/fs/compat_ioctl.c
> @@ -115,15 +115,27 @@
> #include <asm/fbio.h>
> #endif
>
> -static int w_long(unsigned int fd, unsigned int cmd,
> - compat_ulong_t __user *argp)
> +static int do_ioctl(struct file *filp, unsigned int fd,
> + unsigned int cmd, unsigned long arg)
> +{
> + int err;
> +
> + err = security_file_ioctl(filp, cmd, arg);
> + if (err)
> + return err;
> +
> + return do_vfs_ioctl(filp, fd, cmd, arg);
> +}
> +
> +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;
>
> set_fs (KERNEL_DS);
> - err = sys_ioctl(fd, cmd, (unsigned long)&val);
> + err = do_ioctl(filp, fd, cmd, (unsigned long)&val);
> set_fs (old_fs);
> if (!err && put_user(val, argp))
> return -EFAULT;
> @@ -139,15 +151,15 @@ struct compat_video_event {
> } u;
> };
>
> -static int do_video_get_event(unsigned int fd, unsigned int cmd,
> - struct compat_video_event __user *up)
> +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();
> int err;
>
> set_fs(KERNEL_DS);
> - err = sys_ioctl(fd, cmd, (unsigned long) &kevent);
> + err = do_ioctl(filp, fd, cmd, (unsigned long) &kevent);
> set_fs(old_fs);
>
> if (!err) {
> @@ -169,8 +181,8 @@ struct compat_video_still_picture {
> int32_t size;
> };
>
> -static int do_video_stillpicture(unsigned int fd, unsigned int cmd,
> - struct compat_video_still_picture __user *up)
> +static int do_video_stillpicture(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct compat_video_still_picture __user *up)
> {
> struct video_still_picture __user *up_native;
> compat_uptr_t fp;
> @@ -190,7 +202,7 @@ static int do_video_stillpicture(unsigned int fd, unsigned int cmd,
> if (err)
> return -EFAULT;
>
> - err = sys_ioctl(fd, cmd, (unsigned long) up_native);
> + err = do_ioctl(filp, fd, cmd, (unsigned long) up_native);
>
> return err;
> }
> @@ -200,8 +212,8 @@ struct compat_video_spu_palette {
> compat_uptr_t palette;
> };
>
> -static int do_video_set_spu_palette(unsigned int fd, unsigned int cmd,
> - struct compat_video_spu_palette __user *up)
> +static int do_video_set_spu_palette(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct compat_video_spu_palette __user *up)
> {
> struct video_spu_palette __user *up_native;
> compat_uptr_t palp;
> @@ -218,7 +230,7 @@ static int do_video_set_spu_palette(unsigned int fd, unsigned int cmd,
> if (err)
> return -EFAULT;
>
> - err = sys_ioctl(fd, cmd, (unsigned long) up_native);
> + err = do_ioctl(filp, fd, cmd, (unsigned long) up_native);
>
> return err;
> }
> @@ -276,7 +288,7 @@ static int sg_build_iovec(sg_io_hdr_t __user *sgio, void __user *dxferp, u16 iov
> return 0;
> }
>
> -static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
> +static int sg_ioctl_trans(struct file *filp, unsigned int fd, unsigned int cmd,
> sg_io_hdr32_t __user *sgio32)
> {
> sg_io_hdr_t __user *sgio;
> @@ -289,7 +301,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
> if (get_user(interface_id, &sgio32->interface_id))
> return -EFAULT;
> if (interface_id != 'S')
> - return sys_ioctl(fd, cmd, (unsigned long)sgio32);
> + return do_ioctl(filp, fd, cmd, (unsigned long)sgio32);
>
> if (get_user(iovec_count, &sgio32->iovec_count))
> return -EFAULT;
> @@ -349,7 +361,7 @@ static int sg_ioctl_trans(unsigned int fd, unsigned int cmd,
> if (put_user(compat_ptr(data), &sgio->usr_ptr))
> return -EFAULT;
>
> - err = sys_ioctl(fd, cmd, (unsigned long) sgio);
> + err = do_ioctl(filp, fd, cmd, (unsigned long) sgio);
>
> if (err >= 0) {
> void __user *datap;
> @@ -380,13 +392,13 @@ struct compat_sg_req_info { /* used by SG_GET_REQUEST_TABLE ioctl() */
> int unused;
> };
>
> -static int sg_grt_trans(unsigned int fd, unsigned int cmd, struct
> - compat_sg_req_info __user *o)
> +static int sg_grt_trans(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct compat_sg_req_info __user *o)
> {
> int err, i;
> sg_req_info_t __user *r;
> r = compat_alloc_user_space(sizeof(sg_req_info_t)*SG_MAX_QUEUE);
> - err = sys_ioctl(fd,cmd,(unsigned long)r);
> + err = do_ioctl(filp, fd, cmd, (unsigned long)r);
> if (err < 0)
> return err;
> for (i = 0; i < SG_MAX_QUEUE; i++) {
> @@ -412,8 +424,8 @@ struct sock_fprog32 {
> #define PPPIOCSPASS32 _IOW('t', 71, struct sock_fprog32)
> #define PPPIOCSACTIVE32 _IOW('t', 70, struct sock_fprog32)
>
> -static int ppp_sock_fprog_ioctl_trans(unsigned int fd, unsigned int cmd,
> - struct sock_fprog32 __user *u_fprog32)
> +static int ppp_sock_fprog_ioctl_trans(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct sock_fprog32 __user *u_fprog32)
> {
> struct sock_fprog __user *u_fprog64 = compat_alloc_user_space(sizeof(struct sock_fprog));
> void __user *fptr64;
> @@ -435,7 +447,7 @@ static int ppp_sock_fprog_ioctl_trans(unsigned int fd, unsigned int cmd,
> else
> cmd = PPPIOCSACTIVE;
>
> - return sys_ioctl(fd, cmd, (unsigned long) u_fprog64);
> + return do_ioctl(filp, fd, cmd, (unsigned long) u_fprog64);
> }
>
> struct ppp_option_data32 {
> @@ -451,7 +463,7 @@ struct ppp_idle32 {
> };
> #define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32)
>
> -static int ppp_gidle(unsigned int fd, unsigned int cmd,
> +static int ppp_gidle(struct file *filp, unsigned int fd, unsigned int cmd,
> struct ppp_idle32 __user *idle32)
> {
> struct ppp_idle __user *idle;
> @@ -460,7 +472,7 @@ static int ppp_gidle(unsigned int fd, unsigned int cmd,
>
> idle = compat_alloc_user_space(sizeof(*idle));
>
> - err = sys_ioctl(fd, PPPIOCGIDLE, (unsigned long) idle);
> + err = do_ioctl(filp, fd, PPPIOCGIDLE, (unsigned long) idle);
>
> if (!err) {
> if (get_user(xmit, &idle->xmit_idle) ||
> @@ -472,7 +484,7 @@ static int ppp_gidle(unsigned int fd, unsigned int cmd,
> return err;
> }
>
> -static int ppp_scompress(unsigned int fd, unsigned int cmd,
> +static int ppp_scompress(struct file *filp, unsigned int fd, unsigned int cmd,
> struct ppp_option_data32 __user *odata32)
> {
> struct ppp_option_data __user *odata;
> @@ -492,7 +504,7 @@ static int ppp_scompress(unsigned int fd, unsigned int cmd,
> sizeof(__u32) + sizeof(int)))
> return -EFAULT;
>
> - return sys_ioctl(fd, PPPIOCSCOMPRESS, (unsigned long) odata);
> + return do_ioctl(filp, fd, PPPIOCSCOMPRESS, (unsigned long) odata);
> }
>
> #ifdef CONFIG_BLOCK
> @@ -512,7 +524,8 @@ struct mtpos32 {
> };
> #define MTIOCPOS32 _IOR('m', 3, struct mtpos32)
>
> -static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
> +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;
> @@ -534,7 +547,7 @@ static int mt_ioctl_trans(unsigned int fd, unsigned int cmd, void __user *argp)
> break;
> }
> set_fs (KERNEL_DS);
> - err = sys_ioctl (fd, kcmd, (unsigned long)karg);
> + err = do_ioctl(filp, fd, kcmd, (unsigned long)karg);
> set_fs (old_fs);
> if (err)
> return err;
> @@ -605,8 +618,8 @@ struct serial_struct32 {
> compat_int_t reserved[1];
> };
>
> -static int serial_struct_ioctl(unsigned fd, unsigned cmd,
> - struct serial_struct32 __user *ss32)
> +static int serial_struct_ioctl(struct file *filp, unsigned fd,
> + unsigned cmd, struct serial_struct32 __user *ss32)
> {
> typedef struct serial_struct32 SS32;
> int err;
> @@ -629,7 +642,8 @@ static int serial_struct_ioctl(unsigned fd, unsigned cmd,
> ss.iomap_base = 0UL;
> }
> set_fs(KERNEL_DS);
> - err = sys_ioctl(fd,cmd,(unsigned long)(&ss));
> + err = do_ioctl(filp, fd, cmd,
> + (unsigned long)(&ss));
> set_fs(oldseg);
> if (cmd == TIOCGSERIAL && err >= 0) {
> if (!access_ok(VERIFY_WRITE, ss32, sizeof(SS32)))
> @@ -674,8 +688,8 @@ struct i2c_rdwr_aligned {
> struct i2c_msg msgs[0];
> };
>
> -static int do_i2c_rdwr_ioctl(unsigned int fd, unsigned int cmd,
> - struct i2c_rdwr_ioctl_data32 __user *udata)
> +static int do_i2c_rdwr_ioctl(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct i2c_rdwr_ioctl_data32 __user *udata)
> {
> struct i2c_rdwr_aligned __user *tdata;
> struct i2c_msg __user *tmsgs;
> @@ -708,11 +722,11 @@ static int do_i2c_rdwr_ioctl(unsigned int fd, unsigned int cmd,
> put_user(compat_ptr(datap), &tmsgs[i].buf))
> return -EFAULT;
> }
> - return sys_ioctl(fd, cmd, (unsigned long)tdata);
> + return do_ioctl(filp, fd, cmd, (unsigned long)tdata);
> }
>
> -static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
> - struct i2c_smbus_ioctl_data32 __user *udata)
> +static int do_i2c_smbus_ioctl(struct file *filp, unsigned int fd,
> + unsigned int cmd, struct i2c_smbus_ioctl_data32 __user *udata)
> {
> struct i2c_smbus_ioctl_data __user *tdata;
> compat_caddr_t datap;
> @@ -734,7 +748,7 @@ static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
> __put_user(compat_ptr(datap), &tdata->data))
> return -EFAULT;
>
> - return sys_ioctl(fd, cmd, (unsigned long)tdata);
> + return do_ioctl(filp, fd, cmd, (unsigned long)tdata);
> }
>
> #define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t)
> @@ -742,7 +756,8 @@ static int do_i2c_smbus_ioctl(unsigned int fd, unsigned int cmd,
> #define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t)
> #define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t)
>
> -static int rtc_ioctl(unsigned fd, unsigned cmd, void __user *argp)
> +static int rtc_ioctl(struct file *filp, unsigned fd,
> + unsigned cmd, void __user *argp)
> {
> mm_segment_t oldfs = get_fs();
> compat_ulong_t val32;
> @@ -753,7 +768,7 @@ static int rtc_ioctl(unsigned fd, unsigned cmd, void __user *argp)
> case RTC_IRQP_READ32:
> case RTC_EPOCH_READ32:
> set_fs(KERNEL_DS);
> - ret = sys_ioctl(fd, (cmd == RTC_IRQP_READ32) ?
> + ret = do_ioctl(filp, fd, (cmd == RTC_IRQP_READ32) ?
> RTC_IRQP_READ : RTC_EPOCH_READ,
> (unsigned long)&kval);
> set_fs(oldfs);
> @@ -1443,46 +1458,46 @@ static long do_ioctl_trans(int fd, unsigned int cmd,
>
> switch (cmd) {
> case PPPIOCGIDLE32:
> - return ppp_gidle(fd, cmd, argp);
> + return ppp_gidle(file, fd, cmd, argp);
> case PPPIOCSCOMPRESS32:
> - return ppp_scompress(fd, cmd, argp);
> + return ppp_scompress(file, fd, cmd, argp);
> case PPPIOCSPASS32:
> case PPPIOCSACTIVE32:
> - return ppp_sock_fprog_ioctl_trans(fd, cmd, argp);
> + return ppp_sock_fprog_ioctl_trans(file, fd, cmd, argp);
> #ifdef CONFIG_BLOCK
> case SG_IO:
> - return sg_ioctl_trans(fd, cmd, argp);
> + return sg_ioctl_trans(file, fd, cmd, argp);
> case SG_GET_REQUEST_TABLE:
> - return sg_grt_trans(fd, cmd, argp);
> + return sg_grt_trans(file, fd, cmd, argp);
> case MTIOCGET32:
> case MTIOCPOS32:
> - return mt_ioctl_trans(fd, cmd, argp);
> + return mt_ioctl_trans(file, fd, cmd, argp);
> #endif
> /* Serial */
> case TIOCGSERIAL:
> case TIOCSSERIAL:
> - return serial_struct_ioctl(fd, cmd, argp);
> + return serial_struct_ioctl(file, fd, cmd, argp);
> /* i2c */
> case I2C_FUNCS:
> - return w_long(fd, cmd, argp);
> + return w_long(file, fd, cmd, argp);
> case I2C_RDWR:
> - return do_i2c_rdwr_ioctl(fd, cmd, argp);
> + return do_i2c_rdwr_ioctl(file, fd, cmd, argp);
> case I2C_SMBUS:
> - return do_i2c_smbus_ioctl(fd, cmd, argp);
> + return do_i2c_smbus_ioctl(file, fd, cmd, argp);
> /* Not implemented in the native kernel */
> case RTC_IRQP_READ32:
> case RTC_IRQP_SET32:
> case RTC_EPOCH_READ32:
> case RTC_EPOCH_SET32:
> - return rtc_ioctl(fd, cmd, argp);
> + return rtc_ioctl(file, fd, cmd, argp);
>
> /* dvb */
> case VIDEO_GET_EVENT:
> - return do_video_get_event(fd, cmd, argp);
> + return do_video_get_event(file, fd, cmd, argp);
> case VIDEO_STILLPICTURE:
> - return do_video_stillpicture(fd, cmd, argp);
> + return do_video_stillpicture(file, fd, cmd, argp);
> case VIDEO_SET_SPU_PALETTE:
> - return do_video_set_spu_palette(fd, cmd, argp);
> + return do_video_set_spu_palette(file, fd, cmd, argp);
> }
>
> /*
> --
> 2.1.4
>
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS)
2016-01-05 17:27 ` [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS) Jann Horn
@ 2016-01-05 19:35 ` Kees Cook
0 siblings, 0 replies; 7+ messages in thread
From: Kees Cook @ 2016-01-05 19:35 UTC (permalink / raw)
To: Jann Horn; +Cc: Alexander Viro, linux-fsdevel, security, Andy Lutomirski
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
>
--
Kees Cook
Chrome OS & Brillo Security
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] compat_ioctl: don't look up the fd twice
[not found] ` <CA+55aFzX9Gm=t=0HSnMxDeZmHkiZDMRKrBBvBw4CwPK1EbNbUQ@mail.gmail.com>
@ 2016-01-06 14:50 ` Al Viro
2016-01-06 20:25 ` Al Viro
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2016-01-06 14:50 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, linux-fsdevel, Kees Cook, Andy Lutomirski, security
On Tue, Jan 05, 2016 at 11:33:53AM -0800, Linus Torvalds wrote:
> On Jan 5, 2016 9:27 AM, "Jann Horn" <jann@thejh.net> wrote:
> >
> > This change mitigates potential security issues by
> > preventing a race that permits invocation of
> > unlocked_ioctl handlers under KERNEL_DS through compat
> > code even if a corresponding compat_ioctl handler exists.
>
> Yeah, these two patches are much more legible now. Thanks.
>
> Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
>
> Al, I'm assuming I'll get these through the vfs tree during the 4.5 merge
> window. Correct?
I'll need to finish reviewing, but yes, that's 4.5 vfs.git fodder.
So far it looks sane...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] compat_ioctl: don't look up the fd twice
2016-01-06 14:50 ` Al Viro
@ 2016-01-06 20:25 ` Al Viro
2016-01-07 0:13 ` Linus Torvalds
0 siblings, 1 reply; 7+ messages in thread
From: Al Viro @ 2016-01-06 20:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jann Horn, linux-fsdevel, Kees Cook, Andy Lutomirski, security
On Wed, Jan 06, 2016 at 02:50:08PM +0000, Al Viro wrote:
> On Tue, Jan 05, 2016 at 11:33:53AM -0800, Linus Torvalds wrote:
> > On Jan 5, 2016 9:27 AM, "Jann Horn" <jann@thejh.net> wrote:
> > >
> > > This change mitigates potential security issues by
> > > preventing a race that permits invocation of
> > > unlocked_ioctl handlers under KERNEL_DS through compat
> > > code even if a corresponding compat_ioctl handler exists.
> >
> > Yeah, these two patches are much more legible now. Thanks.
> >
> > Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
> >
> > Al, I'm assuming I'll get these through the vfs tree during the 4.5 merge
> > window. Correct?
>
> I'll need to finish reviewing, but yes, that's 4.5 vfs.git fodder.
> So far it looks sane...
Said that, I'm not sure it's the best approach. E.g. I2C_* stuff is
defined only in one driver and if we are touching that code at all,
we might as well take it out of fs/compat_ioctl.c and be done with
that - just add ->compat_ioctl() to drivers/i2c/i2c-dev.c and move
that crap over there (and to hell with compat_alloc_user_space(), while
we are at it).
Oh, well - that can be done in followups, anyway. Applied, with s/filp/file/
through the entire thing - I really don't like that naming convention.
BTW, am I right assuming that it had been adopted from OS Design and
Implementation way back when?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] compat_ioctl: don't look up the fd twice
2016-01-06 20:25 ` Al Viro
@ 2016-01-07 0:13 ` Linus Torvalds
0 siblings, 0 replies; 7+ messages in thread
From: Linus Torvalds @ 2016-01-07 0:13 UTC (permalink / raw)
To: Al Viro; +Cc: Jann Horn, linux-fsdevel, Kees Cook, Andy Lutomirski, security
On Wed, Jan 6, 2016 at 12:25 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Said that, I'm not sure it's the best approach. E.g. I2C_* stuff is
> defined only in one driver and if we are touching that code at all,
> we might as well take it out of fs/compat_ioctl.c and be done with
> that - just add ->compat_ioctl() to drivers/i2c/i2c-dev.c and move
> that crap over there
Yeah, that seems a sane thing to do as an alternative if somebody is
willing and able to actually test it..
> Oh, well - that can be done in followups, anyway. Applied, with s/filp/file/
> through the entire thing - I really don't like that naming convention.
>
> BTW, am I right assuming that it had been adopted from OS Design and
> Implementation way back when?
Examples of 'filp' for "struct file *" variable naming do seem to go
all the way back to 0.01. And yes, very old naming like that is
presumably influenced by either that or by Bach's unix design book.
Linus
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-01-07 0:13 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-01-05 17:27 [PATCH 1/2] compat_ioctl: don't look up the fd twice Jann Horn
2016-01-05 17:27 ` [PATCH 2/2] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS) Jann Horn
2016-01-05 19:35 ` Kees Cook
2016-01-05 19:27 ` [PATCH 1/2] compat_ioctl: don't look up the fd twice Kees Cook
[not found] ` <CA+55aFzX9Gm=t=0HSnMxDeZmHkiZDMRKrBBvBw4CwPK1EbNbUQ@mail.gmail.com>
2016-01-06 14:50 ` Al Viro
2016-01-06 20:25 ` Al Viro
2016-01-07 0:13 ` Linus Torvalds
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.