From b43417216e9ce55e1f1ab7c834c7ab43db0b53e1 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Tue, 5 Jan 2016 18:27:29 +0100 Subject: [PATCH 1/3] compat_ioctl: don't look up the fd twice 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). [AV: two missed sys_ioctl() taken care of] Signed-off-by: Jann Horn Signed-off-by: Al Viro --- fs/compat_ioctl.c | 122 ++++++++++++++++++++++++++-------------------- 1 file changed, 68 insertions(+), 54 deletions(-) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index dcf26537c935..06e60cab0c3b 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -115,15 +115,27 @@ #include #endif -static int w_long(unsigned int fd, unsigned int cmd, - compat_ulong_t __user *argp) +static int do_ioctl(struct file *file, unsigned int fd, + unsigned int cmd, unsigned long arg) +{ + int err; + + err = security_file_ioctl(file, cmd, arg); + if (err) + return err; + + return do_vfs_ioctl(file, fd, cmd, arg); +} + +static int w_long(struct file *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, 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(file, 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 *file, unsigned fd, + unsigned cmd, struct serial_struct32 __user *ss32) { typedef struct serial_struct32 SS32; int err; @@ -629,7 +642,7 @@ 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(file, fd, cmd, (unsigned long)&ss); set_fs(oldseg); if (cmd == TIOCGSERIAL && err >= 0) { if (!access_ok(VERIFY_WRITE, ss32, sizeof(SS32))) @@ -674,8 +687,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 *file, 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 +721,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(file, 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 *file, 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 +747,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(file, fd, cmd, (unsigned long)tdata); } #define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t) @@ -742,7 +755,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 *file, unsigned fd, + unsigned cmd, void __user *argp) { mm_segment_t oldfs = get_fs(); compat_ulong_t val32; @@ -753,7 +767,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(file, fd, (cmd == RTC_IRQP_READ32) ? RTC_IRQP_READ : RTC_EPOCH_READ, (unsigned long)&kval); set_fs(oldfs); @@ -762,9 +776,9 @@ static int rtc_ioctl(unsigned fd, unsigned cmd, void __user *argp) val32 = kval; return put_user(val32, (unsigned int __user *)argp); case RTC_IRQP_SET32: - return sys_ioctl(fd, RTC_IRQP_SET, (unsigned long)argp); + return do_ioctl(file, fd, RTC_IRQP_SET, (unsigned long)argp); case RTC_EPOCH_SET32: - return sys_ioctl(fd, RTC_EPOCH_SET, (unsigned long)argp); + return do_ioctl(file, fd, RTC_EPOCH_SET, (unsigned long)argp); } return -ENOIOCTLCMD; @@ -1443,46 +1457,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); } /* From 66cf191f3eae4582a83cb4251b75b43bee95a999 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Thu, 7 Jan 2016 09:53:30 -0500 Subject: [PATCH 2/3] compat_ioctl: don't pass fd around when not needed Signed-off-by: Al Viro --- fs/compat_ioctl.c | 103 +++++++++++++++++++++++---------------------- fs/internal.h | 7 +++ fs/ioctl.c | 4 +- include/linux/fs.h | 2 - 4 files changed, 61 insertions(+), 55 deletions(-) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index 06e60cab0c3b..908837cd2ac7 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -58,6 +58,8 @@ #include #include +#include "internal.h" + #include #include #include @@ -115,8 +117,7 @@ #include #endif -static int do_ioctl(struct file *file, unsigned int fd, - unsigned int cmd, unsigned long arg) +static int do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { int err; @@ -124,10 +125,10 @@ static int do_ioctl(struct file *file, unsigned int fd, if (err) return err; - return do_vfs_ioctl(file, fd, cmd, arg); + return vfs_ioctl(file, cmd, arg); } -static int w_long(struct file *file, unsigned int fd, +static int w_long(struct file *file, unsigned int cmd, compat_ulong_t __user *argp) { mm_segment_t old_fs = get_fs(); @@ -135,7 +136,7 @@ static int w_long(struct file *file, unsigned int fd, unsigned long val; set_fs (KERNEL_DS); - err = do_ioctl(file, fd, cmd, (unsigned long)&val); + err = do_ioctl(file, cmd, (unsigned long)&val); set_fs (old_fs); if (!err && put_user(val, argp)) return -EFAULT; @@ -151,7 +152,7 @@ struct compat_video_event { } u; }; -static int do_video_get_event(struct file *file, unsigned int fd, +static int do_video_get_event(struct file *file, unsigned int cmd, struct compat_video_event __user *up) { struct video_event kevent; @@ -159,7 +160,7 @@ static int do_video_get_event(struct file *file, unsigned int fd, int err; set_fs(KERNEL_DS); - err = do_ioctl(file, fd, cmd, (unsigned long) &kevent); + err = do_ioctl(file, cmd, (unsigned long) &kevent); set_fs(old_fs); if (!err) { @@ -181,7 +182,7 @@ struct compat_video_still_picture { int32_t size; }; -static int do_video_stillpicture(struct file *file, unsigned int fd, +static int do_video_stillpicture(struct file *file, unsigned int cmd, struct compat_video_still_picture __user *up) { struct video_still_picture __user *up_native; @@ -202,7 +203,7 @@ static int do_video_stillpicture(struct file *file, unsigned int fd, if (err) return -EFAULT; - err = do_ioctl(file, fd, cmd, (unsigned long) up_native); + err = do_ioctl(file, cmd, (unsigned long) up_native); return err; } @@ -212,7 +213,7 @@ struct compat_video_spu_palette { compat_uptr_t palette; }; -static int do_video_set_spu_palette(struct file *file, unsigned int fd, +static int do_video_set_spu_palette(struct file *file, unsigned int cmd, struct compat_video_spu_palette __user *up) { struct video_spu_palette __user *up_native; @@ -230,7 +231,7 @@ static int do_video_set_spu_palette(struct file *file, unsigned int fd, if (err) return -EFAULT; - err = do_ioctl(file, fd, cmd, (unsigned long) up_native); + err = do_ioctl(file, cmd, (unsigned long) up_native); return err; } @@ -288,7 +289,7 @@ static int sg_build_iovec(sg_io_hdr_t __user *sgio, void __user *dxferp, u16 iov return 0; } -static int sg_ioctl_trans(struct file *file, unsigned int fd, unsigned int cmd, +static int sg_ioctl_trans(struct file *file, unsigned int cmd, sg_io_hdr32_t __user *sgio32) { sg_io_hdr_t __user *sgio; @@ -301,7 +302,7 @@ static int sg_ioctl_trans(struct file *file, unsigned int fd, unsigned int cmd, if (get_user(interface_id, &sgio32->interface_id)) return -EFAULT; if (interface_id != 'S') - return do_ioctl(file, fd, cmd, (unsigned long)sgio32); + return do_ioctl(file, cmd, (unsigned long)sgio32); if (get_user(iovec_count, &sgio32->iovec_count)) return -EFAULT; @@ -361,7 +362,7 @@ static int sg_ioctl_trans(struct file *file, unsigned int fd, unsigned int cmd, if (put_user(compat_ptr(data), &sgio->usr_ptr)) return -EFAULT; - err = do_ioctl(file, fd, cmd, (unsigned long) sgio); + err = do_ioctl(file, cmd, (unsigned long) sgio); if (err >= 0) { void __user *datap; @@ -392,13 +393,13 @@ struct compat_sg_req_info { /* used by SG_GET_REQUEST_TABLE ioctl() */ int unused; }; -static int sg_grt_trans(struct file *file, unsigned int fd, +static int sg_grt_trans(struct file *file, 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 = do_ioctl(file, fd, cmd, (unsigned long)r); + err = do_ioctl(file, cmd, (unsigned long)r); if (err < 0) return err; for (i = 0; i < SG_MAX_QUEUE; i++) { @@ -424,7 +425,7 @@ 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(struct file *file, unsigned int fd, +static int ppp_sock_fprog_ioctl_trans(struct file *file, unsigned int cmd, struct sock_fprog32 __user *u_fprog32) { struct sock_fprog __user *u_fprog64 = compat_alloc_user_space(sizeof(struct sock_fprog)); @@ -447,7 +448,7 @@ static int ppp_sock_fprog_ioctl_trans(struct file *file, unsigned int fd, else cmd = PPPIOCSACTIVE; - return do_ioctl(file, fd, cmd, (unsigned long) u_fprog64); + return do_ioctl(file, cmd, (unsigned long) u_fprog64); } struct ppp_option_data32 { @@ -463,7 +464,7 @@ struct ppp_idle32 { }; #define PPPIOCGIDLE32 _IOR('t', 63, struct ppp_idle32) -static int ppp_gidle(struct file *file, unsigned int fd, unsigned int cmd, +static int ppp_gidle(struct file *file, unsigned int cmd, struct ppp_idle32 __user *idle32) { struct ppp_idle __user *idle; @@ -472,7 +473,7 @@ static int ppp_gidle(struct file *file, unsigned int fd, unsigned int cmd, idle = compat_alloc_user_space(sizeof(*idle)); - err = do_ioctl(file, fd, PPPIOCGIDLE, (unsigned long) idle); + err = do_ioctl(file, PPPIOCGIDLE, (unsigned long) idle); if (!err) { if (get_user(xmit, &idle->xmit_idle) || @@ -484,7 +485,7 @@ static int ppp_gidle(struct file *file, unsigned int fd, unsigned int cmd, return err; } -static int ppp_scompress(struct file *file, unsigned int fd, unsigned int cmd, +static int ppp_scompress(struct file *file, unsigned int cmd, struct ppp_option_data32 __user *odata32) { struct ppp_option_data __user *odata; @@ -504,7 +505,7 @@ static int ppp_scompress(struct file *file, unsigned int fd, unsigned int cmd, sizeof(__u32) + sizeof(int))) return -EFAULT; - return do_ioctl(file, fd, PPPIOCSCOMPRESS, (unsigned long) odata); + return do_ioctl(file, PPPIOCSCOMPRESS, (unsigned long) odata); } #ifdef CONFIG_BLOCK @@ -524,7 +525,7 @@ struct mtpos32 { }; #define MTIOCPOS32 _IOR('m', 3, struct mtpos32) -static int mt_ioctl_trans(struct file *file, unsigned int fd, +static int mt_ioctl_trans(struct file *file, unsigned int cmd, void __user *argp) { mm_segment_t old_fs = get_fs(); @@ -547,7 +548,7 @@ static int mt_ioctl_trans(struct file *file, unsigned int fd, break; } set_fs (KERNEL_DS); - err = do_ioctl(file, fd, kcmd, (unsigned long)karg); + err = do_ioctl(file, kcmd, (unsigned long)karg); set_fs (old_fs); if (err) return err; @@ -618,7 +619,7 @@ struct serial_struct32 { compat_int_t reserved[1]; }; -static int serial_struct_ioctl(struct file *file, unsigned fd, +static int serial_struct_ioctl(struct file *file, unsigned cmd, struct serial_struct32 __user *ss32) { typedef struct serial_struct32 SS32; @@ -642,7 +643,7 @@ static int serial_struct_ioctl(struct file *file, unsigned fd, ss.iomap_base = 0UL; } set_fs(KERNEL_DS); - err = do_ioctl(file, fd, cmd, (unsigned long)&ss); + err = do_ioctl(file, cmd, (unsigned long)&ss); set_fs(oldseg); if (cmd == TIOCGSERIAL && err >= 0) { if (!access_ok(VERIFY_WRITE, ss32, sizeof(SS32))) @@ -687,7 +688,7 @@ struct i2c_rdwr_aligned { struct i2c_msg msgs[0]; }; -static int do_i2c_rdwr_ioctl(struct file *file, unsigned int fd, +static int do_i2c_rdwr_ioctl(struct file *file, unsigned int cmd, struct i2c_rdwr_ioctl_data32 __user *udata) { struct i2c_rdwr_aligned __user *tdata; @@ -721,10 +722,10 @@ static int do_i2c_rdwr_ioctl(struct file *file, unsigned int fd, put_user(compat_ptr(datap), &tmsgs[i].buf)) return -EFAULT; } - return do_ioctl(file, fd, cmd, (unsigned long)tdata); + return do_ioctl(file, cmd, (unsigned long)tdata); } -static int do_i2c_smbus_ioctl(struct file *file, unsigned int fd, +static int do_i2c_smbus_ioctl(struct file *file, unsigned int cmd, struct i2c_smbus_ioctl_data32 __user *udata) { struct i2c_smbus_ioctl_data __user *tdata; @@ -747,7 +748,7 @@ static int do_i2c_smbus_ioctl(struct file *file, unsigned int fd, __put_user(compat_ptr(datap), &tdata->data)) return -EFAULT; - return do_ioctl(file, fd, cmd, (unsigned long)tdata); + return do_ioctl(file, cmd, (unsigned long)tdata); } #define RTC_IRQP_READ32 _IOR('p', 0x0b, compat_ulong_t) @@ -755,7 +756,7 @@ static int do_i2c_smbus_ioctl(struct file *file, unsigned int fd, #define RTC_EPOCH_READ32 _IOR('p', 0x0d, compat_ulong_t) #define RTC_EPOCH_SET32 _IOW('p', 0x0e, compat_ulong_t) -static int rtc_ioctl(struct file *file, unsigned fd, +static int rtc_ioctl(struct file *file, unsigned cmd, void __user *argp) { mm_segment_t oldfs = get_fs(); @@ -767,7 +768,7 @@ static int rtc_ioctl(struct file *file, unsigned fd, case RTC_IRQP_READ32: case RTC_EPOCH_READ32: set_fs(KERNEL_DS); - ret = do_ioctl(file, fd, (cmd == RTC_IRQP_READ32) ? + ret = do_ioctl(file, (cmd == RTC_IRQP_READ32) ? RTC_IRQP_READ : RTC_EPOCH_READ, (unsigned long)&kval); set_fs(oldfs); @@ -776,9 +777,9 @@ static int rtc_ioctl(struct file *file, unsigned fd, val32 = kval; return put_user(val32, (unsigned int __user *)argp); case RTC_IRQP_SET32: - return do_ioctl(file, fd, RTC_IRQP_SET, (unsigned long)argp); + return do_ioctl(file, RTC_IRQP_SET, (unsigned long)argp); case RTC_EPOCH_SET32: - return do_ioctl(file, fd, RTC_EPOCH_SET, (unsigned long)argp); + return do_ioctl(file, RTC_EPOCH_SET, (unsigned long)argp); } return -ENOIOCTLCMD; @@ -1450,53 +1451,53 @@ IGNORE_IOCTL(FBIOGCURSOR32) * a compat_ioctl operation in the place that handleѕ the * ioctl for the native case. */ -static long do_ioctl_trans(int fd, unsigned int cmd, +static long do_ioctl_trans(unsigned int cmd, unsigned long arg, struct file *file) { void __user *argp = compat_ptr(arg); switch (cmd) { case PPPIOCGIDLE32: - return ppp_gidle(file, fd, cmd, argp); + return ppp_gidle(file, cmd, argp); case PPPIOCSCOMPRESS32: - return ppp_scompress(file, fd, cmd, argp); + return ppp_scompress(file, cmd, argp); case PPPIOCSPASS32: case PPPIOCSACTIVE32: - return ppp_sock_fprog_ioctl_trans(file, fd, cmd, argp); + return ppp_sock_fprog_ioctl_trans(file, cmd, argp); #ifdef CONFIG_BLOCK case SG_IO: - return sg_ioctl_trans(file, fd, cmd, argp); + return sg_ioctl_trans(file, cmd, argp); case SG_GET_REQUEST_TABLE: - return sg_grt_trans(file, fd, cmd, argp); + return sg_grt_trans(file, cmd, argp); case MTIOCGET32: case MTIOCPOS32: - return mt_ioctl_trans(file, fd, cmd, argp); + return mt_ioctl_trans(file, cmd, argp); #endif /* Serial */ case TIOCGSERIAL: case TIOCSSERIAL: - return serial_struct_ioctl(file, fd, cmd, argp); + return serial_struct_ioctl(file, cmd, argp); /* i2c */ case I2C_FUNCS: - return w_long(file, fd, cmd, argp); + return w_long(file, cmd, argp); case I2C_RDWR: - return do_i2c_rdwr_ioctl(file, fd, cmd, argp); + return do_i2c_rdwr_ioctl(file, cmd, argp); case I2C_SMBUS: - return do_i2c_smbus_ioctl(file, fd, cmd, argp); + return do_i2c_smbus_ioctl(file, 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(file, fd, cmd, argp); + return rtc_ioctl(file, cmd, argp); /* dvb */ case VIDEO_GET_EVENT: - return do_video_get_event(file, fd, cmd, argp); + return do_video_get_event(file, cmd, argp); case VIDEO_STILLPICTURE: - return do_video_stillpicture(file, fd, cmd, argp); + return do_video_stillpicture(file, cmd, argp); case VIDEO_SET_SPU_PALETTE: - return do_video_set_spu_palette(file, fd, cmd, argp); + return do_video_set_spu_palette(file, cmd, argp); } /* @@ -1527,7 +1528,7 @@ static long do_ioctl_trans(int fd, unsigned int cmd, case NBD_SET_BLKSIZE: case NBD_SET_SIZE: case NBD_SET_SIZE_BLOCKS: - return do_vfs_ioctl(file, fd, cmd, arg); + return vfs_ioctl(file, cmd, arg); } return -ENOIOCTLCMD; @@ -1616,7 +1617,7 @@ COMPAT_SYSCALL_DEFINE3(ioctl, unsigned int, fd, unsigned int, cmd, if (compat_ioctl_check_table(XFORM(cmd))) goto found_handler; - error = do_ioctl_trans(fd, cmd, arg, f.file); + error = do_ioctl_trans(cmd, arg, f.file); if (error == -ENOIOCTLCMD) error = -ENOTTY; diff --git a/fs/internal.h b/fs/internal.h index 71859c4d0b41..e38c08ca437d 100644 --- a/fs/internal.h +++ b/fs/internal.h @@ -151,3 +151,10 @@ extern void mnt_pin_kill(struct mount *m); * fs/nsfs.c */ extern struct dentry_operations ns_dentry_operations; + +/* + * fs/ioctl.c + */ +extern int do_vfs_ioctl(struct file *file, unsigned int fd, unsigned int cmd, + unsigned long arg); +extern long vfs_ioctl(struct file *file, unsigned int cmd, unsigned long arg); diff --git a/fs/ioctl.c b/fs/ioctl.c index 5d01d2638ca5..41c352e81193 100644 --- a/fs/ioctl.c +++ b/fs/ioctl.c @@ -15,6 +15,7 @@ #include #include #include +#include "internal.h" #include @@ -32,8 +33,7 @@ * * Returns 0 on success, -errno on error. */ -static long vfs_ioctl(struct file *filp, unsigned int cmd, - unsigned long arg) +long vfs_ioctl(struct file *filp, unsigned int cmd, unsigned long arg) { int error = -ENOTTY; diff --git a/include/linux/fs.h b/include/linux/fs.h index 3aa514254161..51f9f8d93d4d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -2764,8 +2764,6 @@ extern int vfs_lstat(const char __user *, struct kstat *); extern int vfs_fstat(unsigned int, struct kstat *); extern int vfs_fstatat(int , const char __user *, struct kstat *, int); -extern int do_vfs_ioctl(struct file *filp, unsigned int fd, unsigned int cmd, - unsigned long arg); extern int __generic_block_fiemap(struct inode *inode, struct fiemap_extent_info *fieinfo, loff_t start, loff_t len, From a7f61e89af73e9bf760826b20dba4e637221fcb9 Mon Sep 17 00:00:00 2001 From: Jann Horn Date: Tue, 5 Jan 2016 18:27:30 +0100 Subject: [PATCH 3/3] compat_ioctl: don't call do_ioctl under set_fs(KERNEL_DS) 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. Signed-off-by: Jann Horn Signed-off-by: Al Viro --- fs/compat_ioctl.c | 130 ++++++++++++++++++++++++---------------------- 1 file changed, 68 insertions(+), 62 deletions(-) diff --git a/fs/compat_ioctl.c b/fs/compat_ioctl.c index 908837cd2ac7..9144b779d10e 100644 --- a/fs/compat_ioctl.c +++ b/fs/compat_ioctl.c @@ -117,6 +117,13 @@ #include #endif +#define convert_in_user(srcptr, dstptr) \ +({ \ + typeof(*srcptr) val; \ + \ + get_user(val, srcptr) || put_user(val, dstptr); \ +}) + static int do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { int err; @@ -131,16 +138,17 @@ static int do_ioctl(struct file *file, unsigned int cmd, unsigned long arg) static int w_long(struct file *file, 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(file, cmd, (unsigned long)&val); - set_fs (old_fs); - if (!err && put_user(val, argp)) + if (valp == NULL) return -EFAULT; - return err; + err = do_ioctl(file, cmd, (unsigned long)valp); + if (err) + return err; + if (convert_in_user(valp, argp)) + return -EFAULT; + return 0; } struct compat_video_event { @@ -155,20 +163,20 @@ struct compat_video_event { static int do_video_get_event(struct file *file, 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(file, cmd, (unsigned long) &kevent); - set_fs(old_fs); + if (kevent == NULL) + return -EFAULT; + err = do_ioctl(file, 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; @@ -528,10 +536,10 @@ struct mtpos32 { static int mt_ioctl_trans(struct file *file, 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; @@ -540,32 +548,34 @@ static int mt_ioctl_trans(struct file *file, 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(file, 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; @@ -624,37 +634,36 @@ static int serial_struct_ioctl(struct file *file, { 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(file, cmd, (unsigned long)&ss); - set_fs(oldseg); + err = do_ioctl(file, 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,23 +768,20 @@ static int do_i2c_smbus_ioctl(struct file *file, static int rtc_ioctl(struct file *file, 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(file, (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 do_ioctl(file, RTC_IRQP_SET, (unsigned long)argp); case RTC_EPOCH_SET32: