From 999a22890cb183b918e4372395d24426a755cef2 Mon Sep 17 00:00:00 2001 From: Christophe Leroy Date: Fri, 3 Apr 2020 07:20:50 +0000 Subject: [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Some architectures like powerpc64 have the capability to separate read access and write access protection. For get_user() and copy_from_user(), powerpc64 only open read access. For put_user() and copy_to_user(), powerpc64 only open write access. But when using unsafe_get_user() or unsafe_put_user(), user_access_begin open both read and write. Other architectures like powerpc book3s 32 bits only allow write access protection. And on this architecture protection is an heavy operation as it requires locking/unlocking per segment of 256Mbytes. On those architecture it is therefore desirable to do the unlocking only for write access. (Note that book3s/32 ranges from very old powermac from the 90's with powerpc 601 processor, till modern ADSL boxes with PowerQuicc II processors for instance so it is still worth considering.) In order to avoid any risk based of hacking some variable parameters passed to user_access_begin/end that would allow hacking and leaving user access open or opening too much, it is preferable to use dedicated static functions that can't be overridden. Add a user_read_access_begin and user_read_access_end to only open read access. Add a user_write_access_begin and user_write_access_end to only open write access. By default, when undefined, those new access helpers default on the existing user_access_begin and user_access_end. Signed-off-by: Christophe Leroy Reviewed-by: Kees Cook Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/36e43241c7f043a24b5069e78c6a7edd11043be5.1585898438.git.christophe.leroy@c-s.fr --- include/linux/uaccess.h | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 67f016010aad..9861c89f93be 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -378,6 +378,14 @@ extern long strnlen_unsafe_user(const void __user *unsafe_addr, long count); static inline unsigned long user_access_save(void) { return 0UL; } static inline void user_access_restore(unsigned long flags) { } #endif +#ifndef user_write_access_begin +#define user_write_access_begin user_access_begin +#define user_write_access_end user_access_end +#endif +#ifndef user_read_access_begin +#define user_read_access_begin user_access_begin +#define user_read_access_end user_access_end +#endif #ifdef CONFIG_HARDENED_USERCOPY void usercopy_warn(const char *name, const char *detail, bool to_user, From 41cd780524674082b037e7c8461f90c5e42103f0 Mon Sep 17 00:00:00 2001 From: Christophe Leroy Date: Fri, 3 Apr 2020 07:20:51 +0000 Subject: [PATCH 2/6] uaccess: Selectively open read or write user access When opening user access to only perform reads, only open read access. When opening user access to only perform writes, only open write access. Signed-off-by: Christophe Leroy Reviewed-by: Kees Cook Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/2e73bc57125c2c6ab12a587586a4eed3a47105fc.1585898438.git.christophe.leroy@c-s.fr --- fs/readdir.c | 12 ++++++------ kernel/compat.c | 12 ++++++------ kernel/exit.c | 12 ++++++------ lib/strncpy_from_user.c | 4 ++-- lib/strnlen_user.c | 4 ++-- lib/usercopy.c | 6 +++--- 6 files changed, 25 insertions(+), 25 deletions(-) diff --git a/fs/readdir.c b/fs/readdir.c index de2eceffdee8..ed6aaad451aa 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -242,7 +242,7 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen, return -EINTR; dirent = buf->current_dir; prev = (void __user *) dirent - prev_reclen; - if (!user_access_begin(prev, reclen + prev_reclen)) + if (!user_write_access_begin(prev, reclen + prev_reclen)) goto efault; /* This might be 'dirent->d_off', but if so it will get overwritten */ @@ -251,14 +251,14 @@ static int filldir(struct dir_context *ctx, const char *name, int namlen, unsafe_put_user(reclen, &dirent->d_reclen, efault_end); unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end); unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end); - user_access_end(); + user_write_access_end(); buf->current_dir = (void __user *)dirent + reclen; buf->prev_reclen = reclen; buf->count -= reclen; return 0; efault_end: - user_access_end(); + user_write_access_end(); efault: buf->error = -EFAULT; return -EFAULT; @@ -327,7 +327,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen, return -EINTR; dirent = buf->current_dir; prev = (void __user *)dirent - prev_reclen; - if (!user_access_begin(prev, reclen + prev_reclen)) + if (!user_write_access_begin(prev, reclen + prev_reclen)) goto efault; /* This might be 'dirent->d_off', but if so it will get overwritten */ @@ -336,7 +336,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen, unsafe_put_user(reclen, &dirent->d_reclen, efault_end); unsafe_put_user(d_type, &dirent->d_type, efault_end); unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end); - user_access_end(); + user_write_access_end(); buf->prev_reclen = reclen; buf->current_dir = (void __user *)dirent + reclen; @@ -344,7 +344,7 @@ static int filldir64(struct dir_context *ctx, const char *name, int namlen, return 0; efault_end: - user_access_end(); + user_write_access_end(); efault: buf->error = -EFAULT; return -EFAULT; diff --git a/kernel/compat.c b/kernel/compat.c index 843dd17e6078..b8d2800bb4b7 100644 --- a/kernel/compat.c +++ b/kernel/compat.c @@ -199,7 +199,7 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG); nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size); - if (!user_access_begin(umask, bitmap_size / 8)) + if (!user_read_access_begin(umask, bitmap_size / 8)) return -EFAULT; while (nr_compat_longs > 1) { @@ -211,11 +211,11 @@ long compat_get_bitmap(unsigned long *mask, const compat_ulong_t __user *umask, } if (nr_compat_longs) unsafe_get_user(*mask, umask++, Efault); - user_access_end(); + user_read_access_end(); return 0; Efault: - user_access_end(); + user_read_access_end(); return -EFAULT; } @@ -228,7 +228,7 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, bitmap_size = ALIGN(bitmap_size, BITS_PER_COMPAT_LONG); nr_compat_longs = BITS_TO_COMPAT_LONGS(bitmap_size); - if (!user_access_begin(umask, bitmap_size / 8)) + if (!user_write_access_begin(umask, bitmap_size / 8)) return -EFAULT; while (nr_compat_longs > 1) { @@ -239,10 +239,10 @@ long compat_put_bitmap(compat_ulong_t __user *umask, unsigned long *mask, } if (nr_compat_longs) unsafe_put_user((compat_ulong_t)*mask, umask++, Efault); - user_access_end(); + user_write_access_end(); return 0; Efault: - user_access_end(); + user_write_access_end(); return -EFAULT; } diff --git a/kernel/exit.c b/kernel/exit.c index 389a88cb3081..2d97cbba512d 100644 --- a/kernel/exit.c +++ b/kernel/exit.c @@ -1557,7 +1557,7 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *, if (!infop) return err; - if (!user_access_begin(infop, sizeof(*infop))) + if (!user_write_access_begin(infop, sizeof(*infop))) return -EFAULT; unsafe_put_user(signo, &infop->si_signo, Efault); @@ -1566,10 +1566,10 @@ SYSCALL_DEFINE5(waitid, int, which, pid_t, upid, struct siginfo __user *, unsafe_put_user(info.pid, &infop->si_pid, Efault); unsafe_put_user(info.uid, &infop->si_uid, Efault); unsafe_put_user(info.status, &infop->si_status, Efault); - user_access_end(); + user_write_access_end(); return err; Efault: - user_access_end(); + user_write_access_end(); return -EFAULT; } @@ -1684,7 +1684,7 @@ COMPAT_SYSCALL_DEFINE5(waitid, if (!infop) return err; - if (!user_access_begin(infop, sizeof(*infop))) + if (!user_write_access_begin(infop, sizeof(*infop))) return -EFAULT; unsafe_put_user(signo, &infop->si_signo, Efault); @@ -1693,10 +1693,10 @@ COMPAT_SYSCALL_DEFINE5(waitid, unsafe_put_user(info.pid, &infop->si_pid, Efault); unsafe_put_user(info.uid, &infop->si_uid, Efault); unsafe_put_user(info.status, &infop->si_status, Efault); - user_access_end(); + user_write_access_end(); return err; Efault: - user_access_end(); + user_write_access_end(); return -EFAULT; } #endif diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c index 706020b06617..b90ec550183a 100644 --- a/lib/strncpy_from_user.c +++ b/lib/strncpy_from_user.c @@ -116,9 +116,9 @@ long strncpy_from_user(char *dst, const char __user *src, long count) kasan_check_write(dst, count); check_object_size(dst, count, false); - if (user_access_begin(src, max)) { + if (user_read_access_begin(src, max)) { retval = do_strncpy_from_user(dst, src, count, max); - user_access_end(); + user_read_access_end(); return retval; } } diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c index 41670d4a5816..1616710b8a82 100644 --- a/lib/strnlen_user.c +++ b/lib/strnlen_user.c @@ -109,9 +109,9 @@ long strnlen_user(const char __user *str, long count) if (max > count) max = count; - if (user_access_begin(str, max)) { + if (user_read_access_begin(str, max)) { retval = do_strnlen_user(str, count, max); - user_access_end(); + user_read_access_end(); return retval; } } diff --git a/lib/usercopy.c b/lib/usercopy.c index cbb4d9ec00f2..ca2a697a2061 100644 --- a/lib/usercopy.c +++ b/lib/usercopy.c @@ -58,7 +58,7 @@ int check_zeroed_user(const void __user *from, size_t size) from -= align; size += align; - if (!user_access_begin(from, size)) + if (!user_read_access_begin(from, size)) return -EFAULT; unsafe_get_user(val, (unsigned long __user *) from, err_fault); @@ -79,10 +79,10 @@ int check_zeroed_user(const void __user *from, size_t size) val &= aligned_byte_mask(size); done: - user_access_end(); + user_read_access_end(); return (val == 0); err_fault: - user_access_end(); + user_read_access_end(); return -EFAULT; } EXPORT_SYMBOL(check_zeroed_user); From b44f687386875b714dae2afa768e73401e45c21c Mon Sep 17 00:00:00 2001 From: Christophe Leroy Date: Fri, 3 Apr 2020 07:20:52 +0000 Subject: [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin When i915_gem_execbuffer2_ioctl() is using user_access_begin(), that's only to perform unsafe_put_user() so use user_write_access_begin() in order to only open write access. Signed-off-by: Christophe Leroy Reviewed-by: Kees Cook Signed-off-by: Michael Ellerman Link: https://lore.kernel.org/r/ebf1250b6d4f351469fb339e5399d8b92aa8a1c1.1585898438.git.christophe.leroy@c-s.fr --- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index b7440f06c5e2..8a4e9c1cbf6c 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -2794,7 +2794,8 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data, * And this range already got effectively checked earlier * when we did the "copy_from_user()" above. */ - if (!user_access_begin(user_exec_list, count * sizeof(*user_exec_list))) + if (!user_write_access_begin(user_exec_list, + count * sizeof(*user_exec_list))) goto end; for (i = 0; i < args->buffer_count; i++) { @@ -2808,7 +2809,7 @@ i915_gem_execbuffer2_ioctl(struct drm_device *dev, void *data, end_user); } end_user: - user_access_end(); + user_write_access_end(); end:; } From 391b7461d4a14e8dbe1b815ec15e3ee0daf00342 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 18 Feb 2020 14:39:56 -0500 Subject: [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() ... and the same for its compat counterpart Signed-off-by: Al Viro --- fs/readdir.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/fs/readdir.c b/fs/readdir.c index ed6aaad451aa..a9085016a619 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -157,17 +157,18 @@ static int fillonedir(struct dir_context *ctx, const char *name, int namlen, } buf->result++; dirent = buf->dirent; - if (!access_ok(dirent, + if (!user_write_access_begin(dirent, (unsigned long)(dirent->d_name + namlen + 1) - (unsigned long)dirent)) goto efault; - if ( __put_user(d_ino, &dirent->d_ino) || - __put_user(offset, &dirent->d_offset) || - __put_user(namlen, &dirent->d_namlen) || - __copy_to_user(dirent->d_name, name, namlen) || - __put_user(0, dirent->d_name + namlen)) - goto efault; + unsafe_put_user(d_ino, &dirent->d_ino, efault_end); + unsafe_put_user(offset, &dirent->d_offset, efault_end); + unsafe_put_user(namlen, &dirent->d_namlen, efault_end); + unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end); + user_write_access_end(); return 0; +efault_end: + user_write_access_end(); efault: buf->result = -EFAULT; return -EFAULT; @@ -424,17 +425,18 @@ static int compat_fillonedir(struct dir_context *ctx, const char *name, } buf->result++; dirent = buf->dirent; - if (!access_ok(dirent, + if (!user_write_access_begin(dirent, (unsigned long)(dirent->d_name + namlen + 1) - (unsigned long)dirent)) goto efault; - if ( __put_user(d_ino, &dirent->d_ino) || - __put_user(offset, &dirent->d_offset) || - __put_user(namlen, &dirent->d_namlen) || - __copy_to_user(dirent->d_name, name, namlen) || - __put_user(0, dirent->d_name + namlen)) - goto efault; + unsafe_put_user(d_ino, &dirent->d_ino, efault_end); + unsafe_put_user(offset, &dirent->d_offset, efault_end); + unsafe_put_user(namlen, &dirent->d_namlen, efault_end); + unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end); + user_write_access_end(); return 0; +efault_end: + user_write_access_end(); efault: buf->result = -EFAULT; return -EFAULT; From 82af599b7036587d4921e9576ba6975910f1397d Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 18 Feb 2020 22:33:09 -0500 Subject: [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() Signed-off-by: Al Viro --- fs/readdir.c | 51 +++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-) diff --git a/fs/readdir.c b/fs/readdir.c index a9085016a619..9534675880ce 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -473,7 +473,7 @@ struct compat_linux_dirent { struct compat_getdents_callback { struct dir_context ctx; struct compat_linux_dirent __user *current_dir; - struct compat_linux_dirent __user *previous; + int prev_reclen; int count; int error; }; @@ -481,13 +481,17 @@ struct compat_getdents_callback { static int compat_filldir(struct dir_context *ctx, const char *name, int namlen, loff_t offset, u64 ino, unsigned int d_type) { - struct compat_linux_dirent __user * dirent; + struct compat_linux_dirent __user *dirent, *prev; struct compat_getdents_callback *buf = container_of(ctx, struct compat_getdents_callback, ctx); compat_ulong_t d_ino; int reclen = ALIGN(offsetof(struct compat_linux_dirent, d_name) + namlen + 2, sizeof(compat_long_t)); + int prev_reclen; + buf->error = verify_dirent_name(name, namlen); + if (unlikely(buf->error)) + return buf->error; buf->error = -EINVAL; /* only used if we fail.. */ if (reclen > buf->count) return -EINVAL; @@ -496,29 +500,27 @@ static int compat_filldir(struct dir_context *ctx, const char *name, int namlen, buf->error = -EOVERFLOW; return -EOVERFLOW; } - dirent = buf->previous; - if (dirent) { - if (signal_pending(current)) - return -EINTR; - if (__put_user(offset, &dirent->d_off)) - goto efault; - } + prev_reclen = buf->prev_reclen; + if (prev_reclen && signal_pending(current)) + return -EINTR; dirent = buf->current_dir; - if (__put_user(d_ino, &dirent->d_ino)) + prev = (void __user *) dirent - prev_reclen; + if (!user_write_access_begin(prev, reclen + prev_reclen)) goto efault; - if (__put_user(reclen, &dirent->d_reclen)) - goto efault; - if (copy_to_user(dirent->d_name, name, namlen)) - goto efault; - if (__put_user(0, dirent->d_name + namlen)) - goto efault; - if (__put_user(d_type, (char __user *) dirent + reclen - 1)) - goto efault; - buf->previous = dirent; - dirent = (void __user *)dirent + reclen; - buf->current_dir = dirent; + + unsafe_put_user(offset, &prev->d_off, efault_end); + unsafe_put_user(d_ino, &dirent->d_ino, efault_end); + unsafe_put_user(reclen, &dirent->d_reclen, efault_end); + unsafe_put_user(d_type, (char __user *) dirent + reclen - 1, efault_end); + unsafe_copy_dirent_name(dirent->d_name, name, namlen, efault_end); + user_write_access_end(); + + buf->prev_reclen = reclen; + buf->current_dir = (void __user *)dirent + reclen; buf->count -= reclen; return 0; +efault_end: + user_write_access_end(); efault: buf->error = -EFAULT; return -EFAULT; @@ -528,7 +530,6 @@ COMPAT_SYSCALL_DEFINE3(getdents, unsigned int, fd, struct compat_linux_dirent __user *, dirent, unsigned int, count) { struct fd f; - struct compat_linux_dirent __user * lastdirent; struct compat_getdents_callback buf = { .ctx.actor = compat_filldir, .current_dir = dirent, @@ -546,8 +547,10 @@ COMPAT_SYSCALL_DEFINE3(getdents, unsigned int, fd, error = iterate_dir(f.file, &buf.ctx); if (error >= 0) error = buf.error; - lastdirent = buf.previous; - if (lastdirent) { + if (buf.prev_reclen) { + struct compat_linux_dirent __user * lastdirent; + lastdirent = (void __user *)buf.current_dir - buf.prev_reclen; + if (put_user(buf.ctx.pos, &lastdirent->d_off)) error = -EFAULT; else From 5fb1514164de20ddb21886ffceda4cb54d6538f6 Mon Sep 17 00:00:00 2001 From: Al Viro Date: Tue, 18 Feb 2020 22:34:07 -0500 Subject: [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() Signed-off-by: Al Viro --- fs/readdir.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/fs/readdir.c b/fs/readdir.c index 9534675880ce..a49f07c11cfb 100644 --- a/fs/readdir.c +++ b/fs/readdir.c @@ -276,9 +276,6 @@ SYSCALL_DEFINE3(getdents, unsigned int, fd, }; int error; - if (!access_ok(dirent, count)) - return -EFAULT; - f = fdget_pos(fd); if (!f.file) return -EBADF; @@ -362,9 +359,6 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent, }; int error; - if (!access_ok(dirent, count)) - return -EFAULT; - f = fdget_pos(fd); if (!f.file) return -EBADF; @@ -377,7 +371,7 @@ int ksys_getdents64(unsigned int fd, struct linux_dirent64 __user *dirent, typeof(lastdirent->d_off) d_off = buf.ctx.pos; lastdirent = (void __user *) buf.current_dir - buf.prev_reclen; - if (__put_user(d_off, &lastdirent->d_off)) + if (put_user(d_off, &lastdirent->d_off)) error = -EFAULT; else error = count - buf.count; @@ -537,9 +531,6 @@ COMPAT_SYSCALL_DEFINE3(getdents, unsigned int, fd, }; int error; - if (!access_ok(dirent, count)) - return -EFAULT; - f = fdget_pos(fd); if (!f.file) return -EBADF;