On Tue, Jan 5, 2016 at 9:27 AM, Jann Horn <jann@xxxxxxxxx> 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@xxxxxxxxx>
Acked-by: Kees Cook <keescook@xxxxxxxxxxxx>
-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
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
