* [PATCHES] uaccess base
@ 2020-05-28 23:40 Al Viro
2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro
` (11 more replies)
0 siblings, 12 replies; 94+ messages in thread
From: Al Viro @ 2020-05-28 23:40 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel
Base of uaccess pile - series from Christophe Leroy adding
user_{read,write}_access_begin(). Sat in ppc tree all along,
in vfs.git it's #uaccess.base. No changes since the last time
it got posted, repeated for reference, based at v5.7-rc1
Christophe Leroy (3):
uaccess: Add user_read_access_begin/end and user_write_access_begin/end
uaccess: Selectively open read or write user access
drm/i915/gem: Replace user_access_begin by user_write_access_begin
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 5 +++--
fs/readdir.c | 12 ++++++------
include/linux/uaccess.h | 8 ++++++++
kernel/compat.c | 12 ++++++------
kernel/exit.c | 12 ++++++------
lib/strncpy_from_user.c | 4 ++--
lib/strnlen_user.c | 4 ++--
lib/usercopy.c | 6 +++---
8 files changed, 36 insertions(+), 27 deletions(-)
^ permalink raw reply [flat|nested] 94+ messages in thread* [PATCHES] uaccess readdir 2020-05-28 23:40 [PATCHES] uaccess base Al Viro @ 2020-05-28 23:48 ` Al Viro 2020-05-28 23:49 ` [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() Al Viro 2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro ` (10 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-28 23:48 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel readdir.c uaccess stuff. Lives in #uaccess.readdir, based at #uaccess.base, gets the rest of fs/readdir.c in sync with getdents()/getdents64(). Al Viro (3): switch readdir(2) to unsafe_copy_dirent_name() readdir.c: get compat_filldir() more or less in sync with filldir() readdir.c: get rid of the last __put_user(), drop now-useless access_ok() fs/readdir.c | 92 +++++++++++++++++++++++++++++------------------------------- 1 file changed, 44 insertions(+), 48 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() 2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro @ 2020-05-28 23:49 ` Al Viro 2020-05-28 23:49 ` [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() Al Viro 2020-05-28 23:49 ` [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() Al Viro 0 siblings, 2 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> ... and the same for its compat counterpart Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- 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; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() 2020-05-28 23:49 ` [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() Al Viro @ 2020-05-28 23:49 ` Al Viro 2020-05-28 23:49 ` [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- 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)) - 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)) + prev = (void __user *) dirent - prev_reclen; + if (!user_write_access_begin(prev, reclen + prev_reclen)) 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 -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() 2020-05-28 23:49 ` [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() Al Viro 2020-05-28 23:49 ` [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() Al Viro @ 2020-05-28 23:49 ` Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- 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; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end 2020-05-28 23:40 [PATCHES] uaccess base Al Viro 2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro @ 2020-05-28 23:49 ` Al Viro 2020-05-28 23:49 ` [PATCH 2/6] uaccess: Selectively open read or write user access Al Viro 2020-05-28 23:49 ` [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin Al Viro 2020-05-28 23:57 ` [PATCHES] uaccess __copy_from_user() Al Viro ` (9 subsequent siblings) 11 siblings, 2 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Christophe Leroy <christophe.leroy@c-s.fr> 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 <christophe.leroy@c-s.fr> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> 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, -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/6] uaccess: Selectively open read or write user access 2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro @ 2020-05-28 23:49 ` Al Viro 2020-05-28 23:49 ` [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Christophe Leroy <christophe.leroy@c-s.fr> 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 <christophe.leroy@c-s.fr> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> 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); -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin 2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro 2020-05-28 23:49 ` [PATCH 2/6] uaccess: Selectively open read or write user access Al Viro @ 2020-05-28 23:49 ` Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:49 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Christophe Leroy <christophe.leroy@c-s.fr> 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 <christophe.leroy@c-s.fr> Reviewed-by: Kees Cook <keescook@chromium.org> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> 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:; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCHES] uaccess __copy_from_user() 2020-05-28 23:40 [PATCHES] uaccess base Al Viro 2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro 2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro @ 2020-05-28 23:57 ` Al Viro 2020-05-28 23:58 ` [PATCH 1/2] firewire: switch ioctl_queue_iso to use of copy_from_user() Al Viro 2020-05-29 0:03 ` [PATCHES] uaccess __copy_to_user() Al Viro ` (8 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-28 23:57 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel A couple of __copy_from_user()-related patches. That's the stuff that didn't fit anywhere else. The end goal is to kill uses of that thing outside of arch/* and we are not far from getting there. Branch in uaccess.__copy_from_user, based at v5.7-rc1. Al Viro (2): firewire: switch ioctl_queue_iso to use of copy_from_user() pstore: switch to copy_from_user() drivers/firewire/core-cdev.c | 4 +--- fs/pstore/ram_core.c | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/2] firewire: switch ioctl_queue_iso to use of copy_from_user() 2020-05-28 23:57 ` [PATCHES] uaccess __copy_from_user() Al Viro @ 2020-05-28 23:58 ` Al Viro 2020-05-28 23:58 ` [PATCH 2/2] pstore: switch to copy_from_user() Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-28 23:58 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> no point trying to do access_ok() for all those __copy_from_user() at once. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/firewire/core-cdev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/firewire/core-cdev.c b/drivers/firewire/core-cdev.c index 6e291d8f3a27..c7ea4f2d5ca6 100644 --- a/drivers/firewire/core-cdev.c +++ b/drivers/firewire/core-cdev.c @@ -1081,8 +1081,6 @@ static int ioctl_queue_iso(struct client *client, union ioctl_arg *arg) return -EINVAL; p = (struct fw_cdev_iso_packet __user *)u64_to_uptr(a->packets); - if (!access_ok(p, a->size)) - return -EFAULT; end = (void __user *)p + a->size; count = 0; @@ -1120,7 +1118,7 @@ static int ioctl_queue_iso(struct client *client, union ioctl_arg *arg) &p->header[transmit_header_bytes / 4]; if (next > end) return -EINVAL; - if (__copy_from_user + if (copy_from_user (u.packet.header, p->header, transmit_header_bytes)) return -EFAULT; if (u.packet.skip && ctx->type == FW_ISO_CONTEXT_TRANSMIT && -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/2] pstore: switch to copy_from_user() 2020-05-28 23:58 ` [PATCH 1/2] firewire: switch ioctl_queue_iso to use of copy_from_user() Al Viro @ 2020-05-28 23:58 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-28 23:58 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> don't bother trying to do bulk access_ok() Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/pstore/ram_core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fs/pstore/ram_core.c b/fs/pstore/ram_core.c index c917c191e78c..aa8e0b65ff1a 100644 --- a/fs/pstore/ram_core.c +++ b/fs/pstore/ram_core.c @@ -283,7 +283,7 @@ static int notrace persistent_ram_update_user(struct persistent_ram_zone *prz, const void __user *s, unsigned int start, unsigned int count) { struct persistent_ram_buffer *buffer = prz->buffer; - int ret = unlikely(__copy_from_user(buffer->data + start, s, count)) ? + int ret = unlikely(copy_from_user(buffer->data + start, s, count)) ? -EFAULT : 0; persistent_ram_update_ecc(prz, start, count); return ret; @@ -348,8 +348,6 @@ int notrace persistent_ram_write_user(struct persistent_ram_zone *prz, int rem, ret = 0, c = count; size_t start; - if (unlikely(!access_ok(s, count))) - return -EFAULT; if (unlikely(c > prz->buffer_size)) { s += c - prz->buffer_size; c = prz->buffer_size; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCHES] uaccess __copy_to_user() 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (2 preceding siblings ...) 2020-05-28 23:57 ` [PATCHES] uaccess __copy_from_user() Al Viro @ 2020-05-29 0:03 ` Al Viro 2020-05-29 0:04 ` [PATCH 1/2] esas2r: don't bother with __copy_to_user() Al Viro 2020-05-29 0:09 ` [PATCHES] uaccess __put_user() Al Viro ` (7 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:03 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel Similar for __copy_to_user(); patches that didn't fit anywhere else (e.g. into readdir series). The goal, again, is to get rid of __copy_to_user() outside of arch/*. We are not quite there (e.g. there's regset crap, some KVM/vhost-related callers that want different predicate instead of access_ok()), but it's getting fairly close. Branch is #uaccess.__copy_to_user, based at v5.7-rc1. Al Viro (2): esas2r: don't bother with __copy_to_user() dlmfs: convert dlmfs_file_read() to copy_to_user() drivers/scsi/esas2r/esas2r_ioctl.c | 2 +- fs/ocfs2/dlmfs/dlmfs.c | 33 ++++++++++++++------------------- 2 files changed, 15 insertions(+), 20 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/2] esas2r: don't bother with __copy_to_user() 2020-05-29 0:03 ` [PATCHES] uaccess __copy_to_user() Al Viro @ 2020-05-29 0:04 ` Al Viro 2020-05-29 0:04 ` [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> sure, we'd done copy_from_user() on the same range, so we can skip access_ok()... and it's not worth bothering. Just use copy_to_user(). Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/esas2r/esas2r_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/esas2r/esas2r_ioctl.c b/drivers/scsi/esas2r/esas2r_ioctl.c index 442c5e70a7b4..cc620f10eabc 100644 --- a/drivers/scsi/esas2r/esas2r_ioctl.c +++ b/drivers/scsi/esas2r/esas2r_ioctl.c @@ -1510,7 +1510,7 @@ int esas2r_ioctl_handler(void *hostdata, unsigned int cmd, void __user *arg) } /* Always copy the buffer back, if only to pick up the status */ - err = __copy_to_user(arg, ioctl, sizeof(struct atto_express_ioctl)); + err = copy_to_user(arg, ioctl, sizeof(struct atto_express_ioctl)); if (err != 0) { esas2r_log(ESAS2R_LOG_WARN, "ioctl_handler copy_to_user didn't copy everything (err %d, cmd %u)", -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 0:04 ` [PATCH 1/2] esas2r: don't bother with __copy_to_user() Al Viro @ 2020-05-29 0:04 ` Al Viro 2020-05-29 1:27 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:04 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/ocfs2/dlmfs/dlmfs.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-) diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c index 8e4f1ace467c..92f0a3bc3ac5 100644 --- a/fs/ocfs2/dlmfs/dlmfs.c +++ b/fs/ocfs2/dlmfs/dlmfs.c @@ -227,7 +227,7 @@ static ssize_t dlmfs_file_read(struct file *filp, loff_t *ppos) { int bytes_left; - ssize_t readlen, got; + ssize_t got; char *lvb_buf; struct inode *inode = file_inode(filp); @@ -237,36 +237,31 @@ static ssize_t dlmfs_file_read(struct file *filp, if (*ppos >= i_size_read(inode)) return 0; + /* don't read past the lvb */ + if (count > i_size_read(inode) - *ppos) + count = i_size_read(inode) - *ppos; + if (!count) return 0; - if (!access_ok(buf, count)) - return -EFAULT; - - /* don't read past the lvb */ - if ((count + *ppos) > i_size_read(inode)) - readlen = i_size_read(inode) - *ppos; - else - readlen = count; - - lvb_buf = kmalloc(readlen, GFP_NOFS); + lvb_buf = kmalloc(count, GFP_NOFS); if (!lvb_buf) return -ENOMEM; - got = user_dlm_read_lvb(inode, lvb_buf, readlen); + got = user_dlm_read_lvb(inode, lvb_buf, count); if (got) { - BUG_ON(got != readlen); - bytes_left = __copy_to_user(buf, lvb_buf, readlen); - readlen -= bytes_left; + BUG_ON(got != count); + bytes_left = copy_to_user(buf, lvb_buf, count); + count -= bytes_left; } else - readlen = 0; + count = 0; kfree(lvb_buf); - *ppos = *ppos + readlen; + *ppos = *ppos + count; - mlog(0, "read %zd bytes\n", readlen); - return readlen; + mlog(0, "read %zu bytes\n", count); + return count; } static ssize_t dlmfs_file_write(struct file *filp, -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 0:04 ` [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() Al Viro @ 2020-05-29 1:27 ` Linus Torvalds 2020-05-29 1:47 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 1:27 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 5:04 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > if (*ppos >= i_size_read(inode)) > return 0; > > + /* don't read past the lvb */ > + if (count > i_size_read(inode) - *ppos) > + count = i_size_read(inode) - *ppos; This isn't a new problem, since you effectively just moved this code, but it's perhaps more obvious now.. "i_size_read()" is not necessarily stable - we do special things on 32-bit to make sure that we get _a_ stable value for it, but it's not necessarily guaranteed to be the same value when called twice. Think concurrent pread() with a write.. So the inode size could change in between those two accesses, and the subtraction might end up underflowing despite the check just above. This might not be an issue with ocfs2 (I didn't check locking), but .. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 1:27 ` Linus Torvalds @ 2020-05-29 1:47 ` Al Viro 2020-05-29 1:54 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 1:47 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 06:27:36PM -0700, Linus Torvalds wrote: > On Thu, May 28, 2020 at 5:04 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > if (*ppos >= i_size_read(inode)) > > return 0; > > > > + /* don't read past the lvb */ > > + if (count > i_size_read(inode) - *ppos) > > + count = i_size_read(inode) - *ppos; > > This isn't a new problem, since you effectively just moved this code, > but it's perhaps more obvious now.. > > "i_size_read()" is not necessarily stable - we do special things on > 32-bit to make sure that we get _a_ stable value for it, but it's not > necessarily guaranteed to be the same value when called twice. Think > concurrent pread() with a write.. > > So the inode size could change in between those two accesses, and the > subtraction might end up underflowing despite the check just above. > > This might not be an issue with ocfs2 (I didn't check locking), but .. case S_IFREG: inode->i_op = &dlmfs_file_inode_operations; inode->i_fop = &dlmfs_file_operations; i_size_write(inode, DLM_LVB_LEN); is the only thing that does anything to size of that sucker. IOW, that i_size_read() might as well had been an explicit 64. Actually, looking at that thing I would suggest simply static ssize_t dlmfs_file_read(struct file *filp, char __user *buf, size_t count, loff_t *ppos) { struct inode *inode = file_inode(filp); char lvb_buf[DLM_LVB_LEN]; if (!user_dlm_read_lvb(inode, lvb_buf, DLM_LVB_LEN)) return 0; return simple_read_from_buffer(buf, count, ppos, lvb_buf, DLM_LVB_LEN); } But that's belongs in a followup, IMO. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 1:47 ` Al Viro @ 2020-05-29 1:54 ` Linus Torvalds 2020-05-29 3:10 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 1:54 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 6:47 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > case S_IFREG: > inode->i_op = &dlmfs_file_inode_operations; > inode->i_fop = &dlmfs_file_operations; > > i_size_write(inode, DLM_LVB_LEN); > is the only thing that does anything to size of that sucker. IOW, that > i_size_read() might as well had been an explicit 64. Heh. Indeed. I did actually grep for i_size_write() use in ocfs2 and saw several. But I didn't realize to limit it to just the dlmfs part. So it does that crazy sequence number lock dance on 32-bit just to read a constant value. Oh well. It would be nice to get those follow-up cleanups eventually, but I guess the general user access cleanups are more important than this very odd special case silliness. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 1:54 ` Linus Torvalds @ 2020-05-29 3:10 ` Al Viro 2020-05-29 3:42 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 3:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 06:54:11PM -0700, Linus Torvalds wrote: > On Thu, May 28, 2020 at 6:47 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > case S_IFREG: > > inode->i_op = &dlmfs_file_inode_operations; > > inode->i_fop = &dlmfs_file_operations; > > > > i_size_write(inode, DLM_LVB_LEN); > > is the only thing that does anything to size of that sucker. IOW, that > > i_size_read() might as well had been an explicit 64. > > Heh. Indeed. I did actually grep for i_size_write() use in ocfs2 and > saw several. But I didn't realize to limit it to just the dlmfs part. > > So it does that crazy sequence number lock dance on 32-bit just to > read a constant value. > > Oh well. > > It would be nice to get those follow-up cleanups eventually, but I > guess the general user access cleanups are more important than this > very odd special case silliness. Not a problem - I'll put it into work.misc for the next cycle... BTW, regarding uaccess - how badly does the following offend your taste? Normally I'd just go for copy_from_user(), but these syscalls just might be hot enough for overhead to matter... commit 40f443f132306d724f43eaff5330b31c632455a6 Author: Al Viro <viro@zeniv.linux.org.uk> Date: Wed Feb 19 09:54:24 2020 -0500 pselect6() and friends: take handling the combined 6th/7th args into helper ... and use unsafe_get_user(), while we are at it. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> diff --git a/fs/select.c b/fs/select.c index 11d0285d46b7..ff0489c67e3f 100644 --- a/fs/select.c +++ b/fs/select.c @@ -766,6 +766,24 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, * which has a pointer to the sigset_t itself followed by a size_t containing * the sigset size. */ +static inline int unkludge_sigmask(void __user *sig, + sigset_t __user **up, + size_t *sigsetsize) +{ + if (sig) { + if (!user_read_access_begin(sig, sizeof(void *)+sizeof(size_t))) + return -EFAULT; + unsafe_get_user(*up, (sigset_t __user * __user *)sig, Efault); + unsafe_get_user(*sigsetsize, + (size_t __user *)(sig+sizeof(void *)), Efault); + user_read_access_end(); + } + return 0; +Efault: + user_access_end(); + return -EFAULT; +} + SYSCALL_DEFINE6(pselect6, int, n, fd_set __user *, inp, fd_set __user *, outp, fd_set __user *, exp, struct __kernel_timespec __user *, tsp, void __user *, sig) @@ -773,13 +791,8 @@ SYSCALL_DEFINE6(pselect6, int, n, fd_set __user *, inp, fd_set __user *, outp, size_t sigsetsize = 0; sigset_t __user *up = NULL; - if (sig) { - if (!access_ok(sig, sizeof(void *)+sizeof(size_t)) - || __get_user(up, (sigset_t __user * __user *)sig) - || __get_user(sigsetsize, - (size_t __user *)(sig+sizeof(void *)))) - return -EFAULT; - } + if (unkludge_sigmask(sig, &up, &sigsetsize)) + return -EFAULT; return do_pselect(n, inp, outp, exp, tsp, up, sigsetsize, PT_TIMESPEC); } @@ -793,13 +806,8 @@ SYSCALL_DEFINE6(pselect6_time32, int, n, fd_set __user *, inp, fd_set __user *, size_t sigsetsize = 0; sigset_t __user *up = NULL; - if (sig) { - if (!access_ok(sig, sizeof(void *)+sizeof(size_t)) - || __get_user(up, (sigset_t __user * __user *)sig) - || __get_user(sigsetsize, - (size_t __user *)(sig+sizeof(void *)))) - return -EFAULT; - } + if (unkludge_sigmask(sig, &up, &sigsetsize)) + return -EFAULT; return do_pselect(n, inp, outp, exp, tsp, up, sigsetsize, PT_OLD_TIMESPEC); } @@ -1325,6 +1333,25 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return poll_select_finish(&end_time, tsp, type, ret); } +static inline int unkludge_compat_sigmask(void __user *sig, + compat_uptr_t *up, + compat_size_t *sigsetsize) +{ + if (sig) { + if (!user_read_access_begin(sig, + sizeof(compat_uptr_t)+sizeof(compat_size_t))) + return -EFAULT; + unsafe_get_user(*up, (compat_uptr_t __user *)sig, Efault); + unsafe_get_user(*sigsetsize, + (compat_size_t __user *)(sig+sizeof(up)), Efault); + user_read_access_end(); + } + return 0; +Efault: + user_access_end(); + return -EFAULT; +} + COMPAT_SYSCALL_DEFINE6(pselect6_time64, int, n, compat_ulong_t __user *, inp, compat_ulong_t __user *, outp, compat_ulong_t __user *, exp, struct __kernel_timespec __user *, tsp, void __user *, sig) @@ -1332,14 +1359,8 @@ COMPAT_SYSCALL_DEFINE6(pselect6_time64, int, n, compat_ulong_t __user *, inp, compat_size_t sigsetsize = 0; compat_uptr_t up = 0; - if (sig) { - if (!access_ok(sig, - sizeof(compat_uptr_t)+sizeof(compat_size_t)) || - __get_user(up, (compat_uptr_t __user *)sig) || - __get_user(sigsetsize, - (compat_size_t __user *)(sig+sizeof(up)))) - return -EFAULT; - } + if (unkludge_compat_sigmask(sig, &up, &sigsetsize)) + return -EFAULT; return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(up), sigsetsize, PT_TIMESPEC); @@ -1354,14 +1375,8 @@ COMPAT_SYSCALL_DEFINE6(pselect6_time32, int, n, compat_ulong_t __user *, inp, compat_size_t sigsetsize = 0; compat_uptr_t up = 0; - if (sig) { - if (!access_ok(sig, - sizeof(compat_uptr_t)+sizeof(compat_size_t)) || - __get_user(up, (compat_uptr_t __user *)sig) || - __get_user(sigsetsize, - (compat_size_t __user *)(sig+sizeof(up)))) - return -EFAULT; - } + if (unkludge_compat_sigmask(sig, &up, &sigsetsize)) + return -EFAULT; return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(up), sigsetsize, PT_OLD_TIMESPEC); ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 3:10 ` Al Viro @ 2020-05-29 3:42 ` Linus Torvalds 2020-05-29 20:46 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 3:42 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 8:10 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > BTW, regarding uaccess - how badly does the following offend your taste? > Normally I'd just go for copy_from_user(), but these syscalls just might > be hot enough for overhead to matter... Hmm. So the code itself per se doesn't really offend me, but: > +static inline int unkludge_sigmask(void __user *sig, > + sigset_t __user **up, > + size_t *sigsetsize) That's a rather odd function, and if there's a reason for it I have no issue, but I dislike the combination of "odd semantics" together with "nondescriptive naming". "unkludge" really doesn't describe anything. Why is that "sig" pointer "void __user *" instead of being an actually descriptive structure pointer: struct sigset_argpack { sigset_t __user *sigset; size_t sigset_size; }; and then it would be "struct sigset_size_argpack __user *" instead? And same with compat_uptr_t */compat_size_t for the compat case? Yeah, yeah, maybe I got that struct definition wrong when writing it in the email, but wouldn't that make it much more understandable? Then the output arguments could be just a pointer to that struct too (except now in kernel space), and change that "unkludge" to "get_sigset_argpack()", and the end result would be static inline int get_sigset_argpack( struct sigset_argpack __user *uarg, struct sigset_argpack *out) and I think the implementation would be simpler and more understandable too when it didn't need those odd casts and "+sizeof" things etc.. So then the call-site would go from > size_t sigsetsize = 0; > sigset_t __user *up = NULL; > > if (unkludge_sigmask(sig, &up, &sigsetsize)) > return -EFAULT; to > struct sigset_argpack argpack = { NULL, 0 }; > > if (get_sigset_argpack(sig, &argpack)) > return -EFAULT; and now you can use "argpack.sigset" and "argpack.sigset_size". No? Same exact deal for the compat case, where you'd just need that compat struct (using "compat_uptr_t" and "compat_size_t"), and then > struct compat_sigset_argpack argpack = { 0, 0 }; > > + if (get_compat_sigset_argpack(sig, &argpack)) > + return -EFAULT; and then you use the result with "compat_ptr(argpack.sigset)" and "argpack.sigset_size". Or did I mis-read anything and get confused by that code in your patch? Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 3:42 ` Linus Torvalds @ 2020-05-29 20:46 ` Al Viro 2020-05-29 20:57 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 20:46 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, linux-fsdevel On Thu, May 28, 2020 at 08:42:25PM -0700, Linus Torvalds wrote: > > struct sigset_argpack argpack = { NULL, 0 }; > > > > if (get_sigset_argpack(sig, &argpack)) > > return -EFAULT; > > and now you can use "argpack.sigset" and "argpack.sigset_size". > > No? > > Same exact deal for the compat case, where you'd just need that compat > struct (using "compat_uptr_t" and "compat_size_t"), and then > > > struct compat_sigset_argpack argpack = { 0, 0 }; > > > > + if (get_compat_sigset_argpack(sig, &argpack)) > > + return -EFAULT; > > and then you use the result with "compat_ptr(argpack.sigset)" and > "argpack.sigset_size". > > Or did I mis-read anything and get confused by that code in your patch? Umm... I'd been concerned about code generation, but it actually gets split into a pair of scalars just fine... Al, trying to resist the temptation to call those struct bad_idea and struct bad_idea_32... All jokes aside, when had we (or anybody else, really) _not_ gotten into trouble when passing structs across the kernel boundary? Sure, sometimes you have to (stat, for example), but just look at the amount of PITA stat() has spawned... ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 20:46 ` Al Viro @ 2020-05-29 20:57 ` Linus Torvalds 2020-05-29 21:06 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 20:57 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Fri, May 29, 2020 at 1:46 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > Umm... I'd been concerned about code generation, but it actually gets > split into a pair of scalars just fine... We actually have depended on that for a long time: our 'pte_t' etc on 32-bit kernels were very much about "structs of two words are handled fairly well by gcc". IIrc, we (for a while) had a config option to switch between "long long" and the struct, but passing and returning two-word structs ends up working fine even when it's a function call, and when it's all inlined it ends up generating pretty good code on just two registers instead. > Al, trying to resist the temptation to call those struct bad_idea and > struct bad_idea_32... I'm sure you can contain yourself. > All jokes aside, when had we (or anybody else, really) _not_ gotten > into trouble when passing structs across the kernel boundary? Sure, > sometimes you have to (stat, for example), but just look at the amount > of PITA stat() has spawned... I'd rather see the struct than some ugly manual address calculations and casts... Because that's fundamentally what a struct _is_, after all. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() 2020-05-29 20:57 ` Linus Torvalds @ 2020-05-29 21:06 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 21:06 UTC (permalink / raw) To: Linus Torvalds; +Cc: Linux Kernel Mailing List, linux-fsdevel On Fri, May 29, 2020 at 01:57:36PM -0700, Linus Torvalds wrote: > > All jokes aside, when had we (or anybody else, really) _not_ gotten > > into trouble when passing structs across the kernel boundary? Sure, > > sometimes you have to (stat, for example), but just look at the amount > > of PITA stat() has spawned... > > I'd rather see the struct than some ugly manual address calculations > and casts... > > Because that's fundamentally what a struct _is_, after all. Sure; the bad idea I was refering to had been to pass the arguments from userland that way, not the syntax used for it. And it's obviously cast in stone by now - userland ABI and all such... ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCHES] uaccess __put_user() 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (3 preceding siblings ...) 2020-05-29 0:03 ` [PATCHES] uaccess __copy_to_user() Al Viro @ 2020-05-29 0:09 ` Al Viro 2020-05-29 0:10 ` [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout Al Viro 2020-05-29 0:34 ` [PATCHES] uaccess comedi compat Al Viro ` (6 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:09 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel Similar misc patches dealing with __put_user() eliminations not fitting into other series (e.g. quite a bit went into net-next already, then there's readdir series and comedi one - the last one is the single biggest pile of __put_user() outside of arch/*, etc.). Branch in #uaccess.__put_user, based at v5.7-rc1 Al Viro (3): compat sysinfo(2): don't bother with field-by-field copyout scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() pcm_native: result of put_user() needs to be checked drivers/scsi/scsi_ioctl.c | 20 ++++++++++---------- kernel/sys.c | 33 +++++++++++++++++---------------- sound/core/pcm_native.c | 12 ++++++++---- 3 files changed, 35 insertions(+), 30 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout 2020-05-29 0:09 ` [PATCHES] uaccess __put_user() Al Viro @ 2020-05-29 0:10 ` Al Viro 2020-05-29 0:10 ` [PATCH 2/3] scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() Al Viro 2020-05-29 0:10 ` [PATCH 3/3] pcm_native: result of put_user() needs to be checked Al Viro 0 siblings, 2 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- kernel/sys.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/kernel/sys.c b/kernel/sys.c index d325f3ab624a..b4a0324a0699 100644 --- a/kernel/sys.c +++ b/kernel/sys.c @@ -2634,6 +2634,7 @@ struct compat_sysinfo { COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info) { struct sysinfo s; + struct compat_sysinfo s_32; do_sysinfo(&s); @@ -2658,23 +2659,23 @@ COMPAT_SYSCALL_DEFINE1(sysinfo, struct compat_sysinfo __user *, info) s.freehigh >>= bitcount; } - if (!access_ok(info, sizeof(struct compat_sysinfo)) || - __put_user(s.uptime, &info->uptime) || - __put_user(s.loads[0], &info->loads[0]) || - __put_user(s.loads[1], &info->loads[1]) || - __put_user(s.loads[2], &info->loads[2]) || - __put_user(s.totalram, &info->totalram) || - __put_user(s.freeram, &info->freeram) || - __put_user(s.sharedram, &info->sharedram) || - __put_user(s.bufferram, &info->bufferram) || - __put_user(s.totalswap, &info->totalswap) || - __put_user(s.freeswap, &info->freeswap) || - __put_user(s.procs, &info->procs) || - __put_user(s.totalhigh, &info->totalhigh) || - __put_user(s.freehigh, &info->freehigh) || - __put_user(s.mem_unit, &info->mem_unit)) + memset(&s_32, 0, sizeof(s_32)); + s_32.uptime = s.uptime; + s_32.loads[0] = s.loads[0]; + s_32.loads[1] = s.loads[1]; + s_32.loads[2] = s.loads[2]; + s_32.totalram = s.totalram; + s_32.freeram = s.freeram; + s_32.sharedram = s.sharedram; + s_32.bufferram = s.bufferram; + s_32.totalswap = s.totalswap; + s_32.freeswap = s.freeswap; + s_32.procs = s.procs; + s_32.totalhigh = s.totalhigh; + s_32.freehigh = s.freehigh; + s_32.mem_unit = s.mem_unit; + if (copy_to_user(info, &s_32, sizeof(s_32))) return -EFAULT; - return 0; } #endif /* CONFIG_COMPAT */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/3] scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() 2020-05-29 0:10 ` [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout Al Viro @ 2020-05-29 0:10 ` Al Viro 2020-05-29 0:10 ` [PATCH 3/3] pcm_native: result of put_user() needs to be checked Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/scsi_ioctl.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c index 8f3af87b6bb0..45d04b7b2643 100644 --- a/drivers/scsi/scsi_ioctl.c +++ b/drivers/scsi/scsi_ioctl.c @@ -211,18 +211,18 @@ static int scsi_ioctl_common(struct scsi_device *sdev, int cmd, void __user *arg } switch (cmd) { - case SCSI_IOCTL_GET_IDLUN: - if (!access_ok(arg, sizeof(struct scsi_idlun))) + case SCSI_IOCTL_GET_IDLUN: { + struct scsi_idlun v = { + .dev_id = (sdev->id & 0xff) + + ((sdev->lun & 0xff) << 8) + + ((sdev->channel & 0xff) << 16) + + ((sdev->host->host_no & 0xff) << 24), + .host_unique_id = sdev->host->unique_id + }; + if (copy_to_user(arg, &v, sizeof(struct scsi_idlun))) return -EFAULT; - - __put_user((sdev->id & 0xff) - + ((sdev->lun & 0xff) << 8) - + ((sdev->channel & 0xff) << 16) - + ((sdev->host->host_no & 0xff) << 24), - &((struct scsi_idlun __user *)arg)->dev_id); - __put_user(sdev->host->unique_id, - &((struct scsi_idlun __user *)arg)->host_unique_id); return 0; + } case SCSI_IOCTL_GET_BUS_NUMBER: return put_user(sdev->host->host_no, (int __user *)arg); case SCSI_IOCTL_PROBE_HOST: -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 3/3] pcm_native: result of put_user() needs to be checked 2020-05-29 0:10 ` [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout Al Viro 2020-05-29 0:10 ` [PATCH 2/3] scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() Al Viro @ 2020-05-29 0:10 ` Al Viro 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:10 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> ... and no, __put_user() doesn't help here - skipping access_ok() on the second call does not remove the possibility of page having become unmapped or r/o in the meanwhile Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- sound/core/pcm_native.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index aef860256278..47838f57a647 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -3093,7 +3093,8 @@ static int snd_pcm_xferi_frames_ioctl(struct snd_pcm_substream *substream, result = snd_pcm_lib_write(substream, xferi.buf, xferi.frames); else result = snd_pcm_lib_read(substream, xferi.buf, xferi.frames); - __put_user(result, &_xferi->result); + if (put_user(result, &_xferi->result)) + return -EFAULT; return result < 0 ? result : 0; } @@ -3122,7 +3123,8 @@ static int snd_pcm_xfern_frames_ioctl(struct snd_pcm_substream *substream, else result = snd_pcm_lib_readv(substream, bufs, xfern.frames); kfree(bufs); - __put_user(result, &_xfern->result); + if (put_user(result, &_xfern->result)) + return -EFAULT; return result < 0 ? result : 0; } @@ -3137,7 +3139,8 @@ static int snd_pcm_rewind_ioctl(struct snd_pcm_substream *substream, if (put_user(0, _frames)) return -EFAULT; result = snd_pcm_rewind(substream, frames); - __put_user(result, _frames); + if (put_user(result, _frames)) + return -EFAULT; return result < 0 ? result : 0; } @@ -3152,7 +3155,8 @@ static int snd_pcm_forward_ioctl(struct snd_pcm_substream *substream, if (put_user(0, _frames)) return -EFAULT; result = snd_pcm_forward(substream, frames); - __put_user(result, _frames); + if (put_user(result, _frames)) + return -EFAULT; return result < 0 ? result : 0; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCHES] uaccess comedi compat 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (4 preceding siblings ...) 2020-05-29 0:09 ` [PATCHES] uaccess __put_user() Al Viro @ 2020-05-29 0:34 ` Al Viro 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro 2020-05-29 10:48 ` [PATCHES] uaccess comedi compat Ian Abbott 2020-05-29 0:40 ` [PATCHES] uaccess i915 Al Viro ` (5 subsequent siblings) 11 siblings, 2 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:34 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott The way comedi compat ioctls are done is wrong. Instead of having ->compat_ioctl() copying the 32bit stuff in, then passing the kernel copies to helpers shared with native ->ioctl() and doing copyout with conversion if needed, it's playing silly buggers with creating a 64bit copy on user stack, then calling native ioctl (which copies that copy into the kernel), then fetching it from user stack, converting to 32bit variant and copying that to user. Extra headache for no good reason. And the single largest remaining pile of __put_user()/__get_user() this side of arch/*. IMO compat_alloc_user_space() should die... NOTE: this is only compile-tested - I simply don't have the hardware in question. Anyway, the branch lives in #uaccess.comedi, based at v5.7-rc1 Al Viro (10): comedi: move compat ioctl handling to native fops comedi: get rid of indirection via translated_ioctl() comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller comedi: do_cmd_ioctl(): lift copyin/copyout into the caller comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat drivers/staging/comedi/Makefile | 1 - drivers/staging/comedi/comedi_compat32.c | 455 ------------------------- drivers/staging/comedi/comedi_compat32.h | 28 -- drivers/staging/comedi/comedi_fops.c | 563 +++++++++++++++++++++++++------ drivers/staging/comedi/comedi_internal.h | 2 +- drivers/staging/comedi/range.c | 17 +- 6 files changed, 466 insertions(+), 600 deletions(-) delete mode 100644 drivers/staging/comedi/comedi_compat32.c delete mode 100644 drivers/staging/comedi/comedi_compat32.h ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 01/10] comedi: move compat ioctl handling to native fops 2020-05-29 0:34 ` [PATCHES] uaccess comedi compat Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 0:35 ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro ` (8 more replies) 2020-05-29 10:48 ` [PATCHES] uaccess comedi compat Ian Abbott 1 sibling, 9 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> mechanical move Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/Makefile | 1 - drivers/staging/comedi/comedi_compat32.c | 455 ------------------------------- drivers/staging/comedi/comedi_compat32.h | 28 -- drivers/staging/comedi/comedi_fops.c | 451 +++++++++++++++++++++++++++++- 4 files changed, 448 insertions(+), 487 deletions(-) delete mode 100644 drivers/staging/comedi/comedi_compat32.c delete mode 100644 drivers/staging/comedi/comedi_compat32.h diff --git a/drivers/staging/comedi/Makefile b/drivers/staging/comedi/Makefile index 6af5da3b4315..072ed83a5a6a 100644 --- a/drivers/staging/comedi/Makefile +++ b/drivers/staging/comedi/Makefile @@ -4,7 +4,6 @@ ccflags-$(CONFIG_COMEDI_DEBUG) := -DDEBUG comedi-y := comedi_fops.o range.o drivers.o \ comedi_buf.o comedi-$(CONFIG_PROC_FS) += proc.o -comedi-$(CONFIG_COMPAT) += comedi_compat32.o obj-$(CONFIG_COMEDI_PCI_DRIVERS) += comedi_pci.o obj-$(CONFIG_COMEDI_PCMCIA_DRIVERS) += comedi_pcmcia.o diff --git a/drivers/staging/comedi/comedi_compat32.c b/drivers/staging/comedi/comedi_compat32.c deleted file mode 100644 index 36a3564ba1fb..000000000000 --- a/drivers/staging/comedi/comedi_compat32.c +++ /dev/null @@ -1,455 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0+ -/* - * comedi/comedi_compat32.c - * 32-bit ioctl compatibility for 64-bit comedi kernel module. - * - * Author: Ian Abbott, MEV Ltd. <abbotti@mev.co.uk> - * Copyright (C) 2007 MEV Ltd. <http://www.mev.co.uk/> - * - * COMEDI - Linux Control and Measurement Device Interface - * Copyright (C) 1997-2007 David A. Schleef <ds@schleef.org> - */ - -#include <linux/uaccess.h> -#include <linux/compat.h> -#include <linux/fs.h> -#include "comedi.h" -#include "comedi_compat32.h" - -#define COMEDI32_CHANINFO _IOR(CIO, 3, struct comedi32_chaninfo_struct) -#define COMEDI32_RANGEINFO _IOR(CIO, 8, struct comedi32_rangeinfo_struct) -/* - * N.B. COMEDI32_CMD and COMEDI_CMD ought to use _IOWR, not _IOR. - * It's too late to change it now, but it only affects the command number. - */ -#define COMEDI32_CMD _IOR(CIO, 9, struct comedi32_cmd_struct) -/* - * N.B. COMEDI32_CMDTEST and COMEDI_CMDTEST ought to use _IOWR, not _IOR. - * It's too late to change it now, but it only affects the command number. - */ -#define COMEDI32_CMDTEST _IOR(CIO, 10, struct comedi32_cmd_struct) -#define COMEDI32_INSNLIST _IOR(CIO, 11, struct comedi32_insnlist_struct) -#define COMEDI32_INSN _IOR(CIO, 12, struct comedi32_insn_struct) - -struct comedi32_chaninfo_struct { - unsigned int subdev; - compat_uptr_t maxdata_list; /* 32-bit 'unsigned int *' */ - compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */ - compat_uptr_t rangelist; /* 32-bit 'unsigned int *' */ - unsigned int unused[4]; -}; - -struct comedi32_rangeinfo_struct { - unsigned int range_type; - compat_uptr_t range_ptr; /* 32-bit 'void *' */ -}; - -struct comedi32_cmd_struct { - unsigned int subdev; - unsigned int flags; - unsigned int start_src; - unsigned int start_arg; - unsigned int scan_begin_src; - unsigned int scan_begin_arg; - unsigned int convert_src; - unsigned int convert_arg; - unsigned int scan_end_src; - unsigned int scan_end_arg; - unsigned int stop_src; - unsigned int stop_arg; - compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */ - unsigned int chanlist_len; - compat_uptr_t data; /* 32-bit 'short *' */ - unsigned int data_len; -}; - -struct comedi32_insn_struct { - unsigned int insn; - unsigned int n; - compat_uptr_t data; /* 32-bit 'unsigned int *' */ - unsigned int subdev; - unsigned int chanspec; - unsigned int unused[3]; -}; - -struct comedi32_insnlist_struct { - unsigned int n_insns; - compat_uptr_t insns; /* 32-bit 'struct comedi_insn *' */ -}; - -/* Handle translated ioctl. */ -static int translated_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) -{ - if (file->f_op->unlocked_ioctl) - return file->f_op->unlocked_ioctl(file, cmd, arg); - - return -ENOTTY; -} - -/* Handle 32-bit COMEDI_CHANINFO ioctl. */ -static int compat_chaninfo(struct file *file, unsigned long arg) -{ - struct comedi_chaninfo __user *chaninfo; - struct comedi32_chaninfo_struct __user *chaninfo32; - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - chaninfo32 = compat_ptr(arg); - chaninfo = compat_alloc_user_space(sizeof(*chaninfo)); - - /* Copy chaninfo structure. Ignore unused members. */ - if (!access_ok(chaninfo32, sizeof(*chaninfo32)) || - !access_ok(chaninfo, sizeof(*chaninfo))) - return -EFAULT; - - err = 0; - err |= __get_user(temp.uint, &chaninfo32->subdev); - err |= __put_user(temp.uint, &chaninfo->subdev); - err |= __get_user(temp.uptr, &chaninfo32->maxdata_list); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->maxdata_list); - err |= __get_user(temp.uptr, &chaninfo32->flaglist); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->flaglist); - err |= __get_user(temp.uptr, &chaninfo32->rangelist); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->rangelist); - if (err) - return -EFAULT; - - return translated_ioctl(file, COMEDI_CHANINFO, (unsigned long)chaninfo); -} - -/* Handle 32-bit COMEDI_RANGEINFO ioctl. */ -static int compat_rangeinfo(struct file *file, unsigned long arg) -{ - struct comedi_rangeinfo __user *rangeinfo; - struct comedi32_rangeinfo_struct __user *rangeinfo32; - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - rangeinfo32 = compat_ptr(arg); - rangeinfo = compat_alloc_user_space(sizeof(*rangeinfo)); - - /* Copy rangeinfo structure. */ - if (!access_ok(rangeinfo32, sizeof(*rangeinfo32)) || - !access_ok(rangeinfo, sizeof(*rangeinfo))) - return -EFAULT; - - err = 0; - err |= __get_user(temp.uint, &rangeinfo32->range_type); - err |= __put_user(temp.uint, &rangeinfo->range_type); - err |= __get_user(temp.uptr, &rangeinfo32->range_ptr); - err |= __put_user(compat_ptr(temp.uptr), &rangeinfo->range_ptr); - if (err) - return -EFAULT; - - return translated_ioctl(file, COMEDI_RANGEINFO, - (unsigned long)rangeinfo); -} - -/* Copy 32-bit cmd structure to native cmd structure. */ -static int get_compat_cmd(struct comedi_cmd __user *cmd, - struct comedi32_cmd_struct __user *cmd32) -{ - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - /* Copy cmd structure. */ - if (!access_ok(cmd32, sizeof(*cmd32)) || - !access_ok(cmd, sizeof(*cmd))) - return -EFAULT; - - err = 0; - err |= __get_user(temp.uint, &cmd32->subdev); - err |= __put_user(temp.uint, &cmd->subdev); - err |= __get_user(temp.uint, &cmd32->flags); - err |= __put_user(temp.uint, &cmd->flags); - err |= __get_user(temp.uint, &cmd32->start_src); - err |= __put_user(temp.uint, &cmd->start_src); - err |= __get_user(temp.uint, &cmd32->start_arg); - err |= __put_user(temp.uint, &cmd->start_arg); - err |= __get_user(temp.uint, &cmd32->scan_begin_src); - err |= __put_user(temp.uint, &cmd->scan_begin_src); - err |= __get_user(temp.uint, &cmd32->scan_begin_arg); - err |= __put_user(temp.uint, &cmd->scan_begin_arg); - err |= __get_user(temp.uint, &cmd32->convert_src); - err |= __put_user(temp.uint, &cmd->convert_src); - err |= __get_user(temp.uint, &cmd32->convert_arg); - err |= __put_user(temp.uint, &cmd->convert_arg); - err |= __get_user(temp.uint, &cmd32->scan_end_src); - err |= __put_user(temp.uint, &cmd->scan_end_src); - err |= __get_user(temp.uint, &cmd32->scan_end_arg); - err |= __put_user(temp.uint, &cmd->scan_end_arg); - err |= __get_user(temp.uint, &cmd32->stop_src); - err |= __put_user(temp.uint, &cmd->stop_src); - err |= __get_user(temp.uint, &cmd32->stop_arg); - err |= __put_user(temp.uint, &cmd->stop_arg); - err |= __get_user(temp.uptr, &cmd32->chanlist); - err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr), - &cmd->chanlist); - err |= __get_user(temp.uint, &cmd32->chanlist_len); - err |= __put_user(temp.uint, &cmd->chanlist_len); - err |= __get_user(temp.uptr, &cmd32->data); - err |= __put_user(compat_ptr(temp.uptr), &cmd->data); - err |= __get_user(temp.uint, &cmd32->data_len); - err |= __put_user(temp.uint, &cmd->data_len); - return err ? -EFAULT : 0; -} - -/* Copy native cmd structure to 32-bit cmd structure. */ -static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32, - struct comedi_cmd __user *cmd) -{ - int err; - unsigned int temp; - - /* - * Copy back most of cmd structure. - * - * Assume the pointer values are already valid. - * (Could use ptr_to_compat() to set them.) - */ - if (!access_ok(cmd, sizeof(*cmd)) || - !access_ok(cmd32, sizeof(*cmd32))) - return -EFAULT; - - err = 0; - err |= __get_user(temp, &cmd->subdev); - err |= __put_user(temp, &cmd32->subdev); - err |= __get_user(temp, &cmd->flags); - err |= __put_user(temp, &cmd32->flags); - err |= __get_user(temp, &cmd->start_src); - err |= __put_user(temp, &cmd32->start_src); - err |= __get_user(temp, &cmd->start_arg); - err |= __put_user(temp, &cmd32->start_arg); - err |= __get_user(temp, &cmd->scan_begin_src); - err |= __put_user(temp, &cmd32->scan_begin_src); - err |= __get_user(temp, &cmd->scan_begin_arg); - err |= __put_user(temp, &cmd32->scan_begin_arg); - err |= __get_user(temp, &cmd->convert_src); - err |= __put_user(temp, &cmd32->convert_src); - err |= __get_user(temp, &cmd->convert_arg); - err |= __put_user(temp, &cmd32->convert_arg); - err |= __get_user(temp, &cmd->scan_end_src); - err |= __put_user(temp, &cmd32->scan_end_src); - err |= __get_user(temp, &cmd->scan_end_arg); - err |= __put_user(temp, &cmd32->scan_end_arg); - err |= __get_user(temp, &cmd->stop_src); - err |= __put_user(temp, &cmd32->stop_src); - err |= __get_user(temp, &cmd->stop_arg); - err |= __put_user(temp, &cmd32->stop_arg); - /* Assume chanlist pointer is unchanged. */ - err |= __get_user(temp, &cmd->chanlist_len); - err |= __put_user(temp, &cmd32->chanlist_len); - /* Assume data pointer is unchanged. */ - err |= __get_user(temp, &cmd->data_len); - err |= __put_user(temp, &cmd32->data_len); - return err ? -EFAULT : 0; -} - -/* Handle 32-bit COMEDI_CMD ioctl. */ -static int compat_cmd(struct file *file, unsigned long arg) -{ - struct comedi_cmd __user *cmd; - struct comedi32_cmd_struct __user *cmd32; - int rc, err; - - cmd32 = compat_ptr(arg); - cmd = compat_alloc_user_space(sizeof(*cmd)); - - rc = get_compat_cmd(cmd, cmd32); - if (rc) - return rc; - - rc = translated_ioctl(file, COMEDI_CMD, (unsigned long)cmd); - if (rc == -EAGAIN) { - /* Special case: copy cmd back to user. */ - err = put_compat_cmd(cmd32, cmd); - if (err) - rc = err; - } - - return rc; -} - -/* Handle 32-bit COMEDI_CMDTEST ioctl. */ -static int compat_cmdtest(struct file *file, unsigned long arg) -{ - struct comedi_cmd __user *cmd; - struct comedi32_cmd_struct __user *cmd32; - int rc, err; - - cmd32 = compat_ptr(arg); - cmd = compat_alloc_user_space(sizeof(*cmd)); - - rc = get_compat_cmd(cmd, cmd32); - if (rc) - return rc; - - rc = translated_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd); - if (rc < 0) - return rc; - - err = put_compat_cmd(cmd32, cmd); - if (err) - rc = err; - - return rc; -} - -/* Copy 32-bit insn structure to native insn structure. */ -static int get_compat_insn(struct comedi_insn __user *insn, - struct comedi32_insn_struct __user *insn32) -{ - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - /* Copy insn structure. Ignore the unused members. */ - err = 0; - if (!access_ok(insn32, sizeof(*insn32)) || - !access_ok(insn, sizeof(*insn))) - return -EFAULT; - - err |= __get_user(temp.uint, &insn32->insn); - err |= __put_user(temp.uint, &insn->insn); - err |= __get_user(temp.uint, &insn32->n); - err |= __put_user(temp.uint, &insn->n); - err |= __get_user(temp.uptr, &insn32->data); - err |= __put_user(compat_ptr(temp.uptr), &insn->data); - err |= __get_user(temp.uint, &insn32->subdev); - err |= __put_user(temp.uint, &insn->subdev); - err |= __get_user(temp.uint, &insn32->chanspec); - err |= __put_user(temp.uint, &insn->chanspec); - return err ? -EFAULT : 0; -} - -/* Handle 32-bit COMEDI_INSNLIST ioctl. */ -static int compat_insnlist(struct file *file, unsigned long arg) -{ - struct combined_insnlist { - struct comedi_insnlist insnlist; - struct comedi_insn insn[1]; - } __user *s; - struct comedi32_insnlist_struct __user *insnlist32; - struct comedi32_insn_struct __user *insn32; - compat_uptr_t uptr; - unsigned int n_insns, n; - int err, rc; - - insnlist32 = compat_ptr(arg); - - /* Get 32-bit insnlist structure. */ - if (!access_ok(insnlist32, sizeof(*insnlist32))) - return -EFAULT; - - err = 0; - err |= __get_user(n_insns, &insnlist32->n_insns); - err |= __get_user(uptr, &insnlist32->insns); - insn32 = compat_ptr(uptr); - if (err) - return -EFAULT; - - /* Allocate user memory to copy insnlist and insns into. */ - s = compat_alloc_user_space(offsetof(struct combined_insnlist, - insn[n_insns])); - - /* Set native insnlist structure. */ - if (!access_ok(&s->insnlist, sizeof(s->insnlist))) - return -EFAULT; - - err |= __put_user(n_insns, &s->insnlist.n_insns); - err |= __put_user(&s->insn[0], &s->insnlist.insns); - if (err) - return -EFAULT; - - /* Copy insn structures. */ - for (n = 0; n < n_insns; n++) { - rc = get_compat_insn(&s->insn[n], &insn32[n]); - if (rc) - return rc; - } - - return translated_ioctl(file, COMEDI_INSNLIST, - (unsigned long)&s->insnlist); -} - -/* Handle 32-bit COMEDI_INSN ioctl. */ -static int compat_insn(struct file *file, unsigned long arg) -{ - struct comedi_insn __user *insn; - struct comedi32_insn_struct __user *insn32; - int rc; - - insn32 = compat_ptr(arg); - insn = compat_alloc_user_space(sizeof(*insn)); - - rc = get_compat_insn(insn, insn32); - if (rc) - return rc; - - return translated_ioctl(file, COMEDI_INSN, (unsigned long)insn); -} - -/* - * compat_ioctl file operation. - * - * Returns -ENOIOCTLCMD for unrecognised ioctl codes. - */ -long comedi_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) -{ - int rc; - - switch (cmd) { - case COMEDI_DEVCONFIG: - case COMEDI_DEVINFO: - case COMEDI_SUBDINFO: - case COMEDI_BUFCONFIG: - case COMEDI_BUFINFO: - /* Just need to translate the pointer argument. */ - arg = (unsigned long)compat_ptr(arg); - rc = translated_ioctl(file, cmd, arg); - break; - case COMEDI_LOCK: - case COMEDI_UNLOCK: - case COMEDI_CANCEL: - case COMEDI_POLL: - case COMEDI_SETRSUBD: - case COMEDI_SETWSUBD: - /* No translation needed. */ - rc = translated_ioctl(file, cmd, arg); - break; - case COMEDI32_CHANINFO: - rc = compat_chaninfo(file, arg); - break; - case COMEDI32_RANGEINFO: - rc = compat_rangeinfo(file, arg); - break; - case COMEDI32_CMD: - rc = compat_cmd(file, arg); - break; - case COMEDI32_CMDTEST: - rc = compat_cmdtest(file, arg); - break; - case COMEDI32_INSNLIST: - rc = compat_insnlist(file, arg); - break; - case COMEDI32_INSN: - rc = compat_insn(file, arg); - break; - default: - rc = -ENOIOCTLCMD; - break; - } - return rc; -} diff --git a/drivers/staging/comedi/comedi_compat32.h b/drivers/staging/comedi/comedi_compat32.h deleted file mode 100644 index dc3e2a9442c7..000000000000 --- a/drivers/staging/comedi/comedi_compat32.h +++ /dev/null @@ -1,28 +0,0 @@ -/* SPDX-License-Identifier: GPL-2.0+ */ -/* - * comedi/comedi_compat32.h - * 32-bit ioctl compatibility for 64-bit comedi kernel module. - * - * Author: Ian Abbott, MEV Ltd. <abbotti@mev.co.uk> - * Copyright (C) 2007 MEV Ltd. <http://www.mev.co.uk/> - * - * COMEDI - Linux Control and Measurement Device Interface - * Copyright (C) 1997-2007 David A. Schleef <ds@schleef.org> - */ - -#ifndef _COMEDI_COMPAT32_H -#define _COMEDI_COMPAT32_H - -#ifdef CONFIG_COMPAT - -struct file; -long comedi_compat_ioctl(struct file *file, unsigned int cmd, - unsigned long arg); - -#else /* CONFIG_COMPAT */ - -#define comedi_compat_ioctl NULL - -#endif /* CONFIG_COMPAT */ - -#endif /* _COMEDI_COMPAT32_H */ diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 08d1bbbebf2d..9dfb81dfe43c 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -4,13 +4,14 @@ * comedi kernel module * * COMEDI - Linux Control and Measurement Device Interface - * Copyright (C) 1997-2000 David A. Schleef <ds@schleef.org> + * Copyright (C) 1997-2007 David A. Schleef <ds@schleef.org> + * compat ioctls: + * Author: Ian Abbott, MEV Ltd. <abbotti@mev.co.uk> + * Copyright (C) 2007 MEV Ltd. <http://www.mev.co.uk/> */ #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt -#include "comedi_compat32.h" - #include <linux/module.h> #include <linux/errno.h> #include <linux/kernel.h> @@ -27,6 +28,7 @@ #include <linux/io.h> #include <linux/uaccess.h> +#include <linux/compat.h> #include "comedi_internal.h" @@ -2806,6 +2808,449 @@ static int comedi_close(struct inode *inode, struct file *file) return 0; } +#ifdef CONFIG_COMPAT + +#define COMEDI32_CHANINFO _IOR(CIO, 3, struct comedi32_chaninfo_struct) +#define COMEDI32_RANGEINFO _IOR(CIO, 8, struct comedi32_rangeinfo_struct) +/* + * N.B. COMEDI32_CMD and COMEDI_CMD ought to use _IOWR, not _IOR. + * It's too late to change it now, but it only affects the command number. + */ +#define COMEDI32_CMD _IOR(CIO, 9, struct comedi32_cmd_struct) +/* + * N.B. COMEDI32_CMDTEST and COMEDI_CMDTEST ought to use _IOWR, not _IOR. + * It's too late to change it now, but it only affects the command number. + */ +#define COMEDI32_CMDTEST _IOR(CIO, 10, struct comedi32_cmd_struct) +#define COMEDI32_INSNLIST _IOR(CIO, 11, struct comedi32_insnlist_struct) +#define COMEDI32_INSN _IOR(CIO, 12, struct comedi32_insn_struct) + +struct comedi32_chaninfo_struct { + unsigned int subdev; + compat_uptr_t maxdata_list; /* 32-bit 'unsigned int *' */ + compat_uptr_t flaglist; /* 32-bit 'unsigned int *' */ + compat_uptr_t rangelist; /* 32-bit 'unsigned int *' */ + unsigned int unused[4]; +}; + +struct comedi32_rangeinfo_struct { + unsigned int range_type; + compat_uptr_t range_ptr; /* 32-bit 'void *' */ +}; + +struct comedi32_cmd_struct { + unsigned int subdev; + unsigned int flags; + unsigned int start_src; + unsigned int start_arg; + unsigned int scan_begin_src; + unsigned int scan_begin_arg; + unsigned int convert_src; + unsigned int convert_arg; + unsigned int scan_end_src; + unsigned int scan_end_arg; + unsigned int stop_src; + unsigned int stop_arg; + compat_uptr_t chanlist; /* 32-bit 'unsigned int *' */ + unsigned int chanlist_len; + compat_uptr_t data; /* 32-bit 'short *' */ + unsigned int data_len; +}; + +struct comedi32_insn_struct { + unsigned int insn; + unsigned int n; + compat_uptr_t data; /* 32-bit 'unsigned int *' */ + unsigned int subdev; + unsigned int chanspec; + unsigned int unused[3]; +}; + +struct comedi32_insnlist_struct { + unsigned int n_insns; + compat_uptr_t insns; /* 32-bit 'struct comedi_insn *' */ +}; + +/* Handle translated ioctl. */ +static int translated_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + if (file->f_op->unlocked_ioctl) + return file->f_op->unlocked_ioctl(file, cmd, arg); + + return -ENOTTY; +} + +/* Handle 32-bit COMEDI_CHANINFO ioctl. */ +static int compat_chaninfo(struct file *file, unsigned long arg) +{ + struct comedi_chaninfo __user *chaninfo; + struct comedi32_chaninfo_struct __user *chaninfo32; + int err; + union { + unsigned int uint; + compat_uptr_t uptr; + } temp; + + chaninfo32 = compat_ptr(arg); + chaninfo = compat_alloc_user_space(sizeof(*chaninfo)); + + /* Copy chaninfo structure. Ignore unused members. */ + if (!access_ok(chaninfo32, sizeof(*chaninfo32)) || + !access_ok(chaninfo, sizeof(*chaninfo))) + return -EFAULT; + + err = 0; + err |= __get_user(temp.uint, &chaninfo32->subdev); + err |= __put_user(temp.uint, &chaninfo->subdev); + err |= __get_user(temp.uptr, &chaninfo32->maxdata_list); + err |= __put_user(compat_ptr(temp.uptr), &chaninfo->maxdata_list); + err |= __get_user(temp.uptr, &chaninfo32->flaglist); + err |= __put_user(compat_ptr(temp.uptr), &chaninfo->flaglist); + err |= __get_user(temp.uptr, &chaninfo32->rangelist); + err |= __put_user(compat_ptr(temp.uptr), &chaninfo->rangelist); + if (err) + return -EFAULT; + + return translated_ioctl(file, COMEDI_CHANINFO, (unsigned long)chaninfo); +} + +/* Handle 32-bit COMEDI_RANGEINFO ioctl. */ +static int compat_rangeinfo(struct file *file, unsigned long arg) +{ + struct comedi_rangeinfo __user *rangeinfo; + struct comedi32_rangeinfo_struct __user *rangeinfo32; + int err; + union { + unsigned int uint; + compat_uptr_t uptr; + } temp; + + rangeinfo32 = compat_ptr(arg); + rangeinfo = compat_alloc_user_space(sizeof(*rangeinfo)); + + /* Copy rangeinfo structure. */ + if (!access_ok(rangeinfo32, sizeof(*rangeinfo32)) || + !access_ok(rangeinfo, sizeof(*rangeinfo))) + return -EFAULT; + + err = 0; + err |= __get_user(temp.uint, &rangeinfo32->range_type); + err |= __put_user(temp.uint, &rangeinfo->range_type); + err |= __get_user(temp.uptr, &rangeinfo32->range_ptr); + err |= __put_user(compat_ptr(temp.uptr), &rangeinfo->range_ptr); + if (err) + return -EFAULT; + + return translated_ioctl(file, COMEDI_RANGEINFO, + (unsigned long)rangeinfo); +} + +/* Copy 32-bit cmd structure to native cmd structure. */ +static int get_compat_cmd(struct comedi_cmd __user *cmd, + struct comedi32_cmd_struct __user *cmd32) +{ + int err; + union { + unsigned int uint; + compat_uptr_t uptr; + } temp; + + /* Copy cmd structure. */ + if (!access_ok(cmd32, sizeof(*cmd32)) || + !access_ok(cmd, sizeof(*cmd))) + return -EFAULT; + + err = 0; + err |= __get_user(temp.uint, &cmd32->subdev); + err |= __put_user(temp.uint, &cmd->subdev); + err |= __get_user(temp.uint, &cmd32->flags); + err |= __put_user(temp.uint, &cmd->flags); + err |= __get_user(temp.uint, &cmd32->start_src); + err |= __put_user(temp.uint, &cmd->start_src); + err |= __get_user(temp.uint, &cmd32->start_arg); + err |= __put_user(temp.uint, &cmd->start_arg); + err |= __get_user(temp.uint, &cmd32->scan_begin_src); + err |= __put_user(temp.uint, &cmd->scan_begin_src); + err |= __get_user(temp.uint, &cmd32->scan_begin_arg); + err |= __put_user(temp.uint, &cmd->scan_begin_arg); + err |= __get_user(temp.uint, &cmd32->convert_src); + err |= __put_user(temp.uint, &cmd->convert_src); + err |= __get_user(temp.uint, &cmd32->convert_arg); + err |= __put_user(temp.uint, &cmd->convert_arg); + err |= __get_user(temp.uint, &cmd32->scan_end_src); + err |= __put_user(temp.uint, &cmd->scan_end_src); + err |= __get_user(temp.uint, &cmd32->scan_end_arg); + err |= __put_user(temp.uint, &cmd->scan_end_arg); + err |= __get_user(temp.uint, &cmd32->stop_src); + err |= __put_user(temp.uint, &cmd->stop_src); + err |= __get_user(temp.uint, &cmd32->stop_arg); + err |= __put_user(temp.uint, &cmd->stop_arg); + err |= __get_user(temp.uptr, &cmd32->chanlist); + err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr), + &cmd->chanlist); + err |= __get_user(temp.uint, &cmd32->chanlist_len); + err |= __put_user(temp.uint, &cmd->chanlist_len); + err |= __get_user(temp.uptr, &cmd32->data); + err |= __put_user(compat_ptr(temp.uptr), &cmd->data); + err |= __get_user(temp.uint, &cmd32->data_len); + err |= __put_user(temp.uint, &cmd->data_len); + return err ? -EFAULT : 0; +} + +/* Copy native cmd structure to 32-bit cmd structure. */ +static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32, + struct comedi_cmd __user *cmd) +{ + int err; + unsigned int temp; + + /* + * Copy back most of cmd structure. + * + * Assume the pointer values are already valid. + * (Could use ptr_to_compat() to set them.) + */ + if (!access_ok(cmd, sizeof(*cmd)) || + !access_ok(cmd32, sizeof(*cmd32))) + return -EFAULT; + + err = 0; + err |= __get_user(temp, &cmd->subdev); + err |= __put_user(temp, &cmd32->subdev); + err |= __get_user(temp, &cmd->flags); + err |= __put_user(temp, &cmd32->flags); + err |= __get_user(temp, &cmd->start_src); + err |= __put_user(temp, &cmd32->start_src); + err |= __get_user(temp, &cmd->start_arg); + err |= __put_user(temp, &cmd32->start_arg); + err |= __get_user(temp, &cmd->scan_begin_src); + err |= __put_user(temp, &cmd32->scan_begin_src); + err |= __get_user(temp, &cmd->scan_begin_arg); + err |= __put_user(temp, &cmd32->scan_begin_arg); + err |= __get_user(temp, &cmd->convert_src); + err |= __put_user(temp, &cmd32->convert_src); + err |= __get_user(temp, &cmd->convert_arg); + err |= __put_user(temp, &cmd32->convert_arg); + err |= __get_user(temp, &cmd->scan_end_src); + err |= __put_user(temp, &cmd32->scan_end_src); + err |= __get_user(temp, &cmd->scan_end_arg); + err |= __put_user(temp, &cmd32->scan_end_arg); + err |= __get_user(temp, &cmd->stop_src); + err |= __put_user(temp, &cmd32->stop_src); + err |= __get_user(temp, &cmd->stop_arg); + err |= __put_user(temp, &cmd32->stop_arg); + /* Assume chanlist pointer is unchanged. */ + err |= __get_user(temp, &cmd->chanlist_len); + err |= __put_user(temp, &cmd32->chanlist_len); + /* Assume data pointer is unchanged. */ + err |= __get_user(temp, &cmd->data_len); + err |= __put_user(temp, &cmd32->data_len); + return err ? -EFAULT : 0; +} + +/* Handle 32-bit COMEDI_CMD ioctl. */ +static int compat_cmd(struct file *file, unsigned long arg) +{ + struct comedi_cmd __user *cmd; + struct comedi32_cmd_struct __user *cmd32; + int rc, err; + + cmd32 = compat_ptr(arg); + cmd = compat_alloc_user_space(sizeof(*cmd)); + + rc = get_compat_cmd(cmd, cmd32); + if (rc) + return rc; + + rc = translated_ioctl(file, COMEDI_CMD, (unsigned long)cmd); + if (rc == -EAGAIN) { + /* Special case: copy cmd back to user. */ + err = put_compat_cmd(cmd32, cmd); + if (err) + rc = err; + } + + return rc; +} + +/* Handle 32-bit COMEDI_CMDTEST ioctl. */ +static int compat_cmdtest(struct file *file, unsigned long arg) +{ + struct comedi_cmd __user *cmd; + struct comedi32_cmd_struct __user *cmd32; + int rc, err; + + cmd32 = compat_ptr(arg); + cmd = compat_alloc_user_space(sizeof(*cmd)); + + rc = get_compat_cmd(cmd, cmd32); + if (rc) + return rc; + + rc = translated_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd); + if (rc < 0) + return rc; + + err = put_compat_cmd(cmd32, cmd); + if (err) + rc = err; + + return rc; +} + +/* Copy 32-bit insn structure to native insn structure. */ +static int get_compat_insn(struct comedi_insn __user *insn, + struct comedi32_insn_struct __user *insn32) +{ + int err; + union { + unsigned int uint; + compat_uptr_t uptr; + } temp; + + /* Copy insn structure. Ignore the unused members. */ + err = 0; + if (!access_ok(insn32, sizeof(*insn32)) || + !access_ok(insn, sizeof(*insn))) + return -EFAULT; + + err |= __get_user(temp.uint, &insn32->insn); + err |= __put_user(temp.uint, &insn->insn); + err |= __get_user(temp.uint, &insn32->n); + err |= __put_user(temp.uint, &insn->n); + err |= __get_user(temp.uptr, &insn32->data); + err |= __put_user(compat_ptr(temp.uptr), &insn->data); + err |= __get_user(temp.uint, &insn32->subdev); + err |= __put_user(temp.uint, &insn->subdev); + err |= __get_user(temp.uint, &insn32->chanspec); + err |= __put_user(temp.uint, &insn->chanspec); + return err ? -EFAULT : 0; +} + +/* Handle 32-bit COMEDI_INSNLIST ioctl. */ +static int compat_insnlist(struct file *file, unsigned long arg) +{ + struct combined_insnlist { + struct comedi_insnlist insnlist; + struct comedi_insn insn[1]; + } __user *s; + struct comedi32_insnlist_struct __user *insnlist32; + struct comedi32_insn_struct __user *insn32; + compat_uptr_t uptr; + unsigned int n_insns, n; + int err, rc; + + insnlist32 = compat_ptr(arg); + + /* Get 32-bit insnlist structure. */ + if (!access_ok(insnlist32, sizeof(*insnlist32))) + return -EFAULT; + + err = 0; + err |= __get_user(n_insns, &insnlist32->n_insns); + err |= __get_user(uptr, &insnlist32->insns); + insn32 = compat_ptr(uptr); + if (err) + return -EFAULT; + + /* Allocate user memory to copy insnlist and insns into. */ + s = compat_alloc_user_space(offsetof(struct combined_insnlist, + insn[n_insns])); + + /* Set native insnlist structure. */ + if (!access_ok(&s->insnlist, sizeof(s->insnlist))) + return -EFAULT; + + err |= __put_user(n_insns, &s->insnlist.n_insns); + err |= __put_user(&s->insn[0], &s->insnlist.insns); + if (err) + return -EFAULT; + + /* Copy insn structures. */ + for (n = 0; n < n_insns; n++) { + rc = get_compat_insn(&s->insn[n], &insn32[n]); + if (rc) + return rc; + } + + return translated_ioctl(file, COMEDI_INSNLIST, + (unsigned long)&s->insnlist); +} + +/* Handle 32-bit COMEDI_INSN ioctl. */ +static int compat_insn(struct file *file, unsigned long arg) +{ + struct comedi_insn __user *insn; + struct comedi32_insn_struct __user *insn32; + int rc; + + insn32 = compat_ptr(arg); + insn = compat_alloc_user_space(sizeof(*insn)); + + rc = get_compat_insn(insn, insn32); + if (rc) + return rc; + + return translated_ioctl(file, COMEDI_INSN, (unsigned long)insn); +} + +/* + * compat_ioctl file operation. + * + * Returns -ENOIOCTLCMD for unrecognised ioctl codes. + */ +static long comedi_compat_ioctl(struct file *file, unsigned int cmd, unsigned long arg) +{ + int rc; + + switch (cmd) { + case COMEDI_DEVCONFIG: + case COMEDI_DEVINFO: + case COMEDI_SUBDINFO: + case COMEDI_BUFCONFIG: + case COMEDI_BUFINFO: + /* Just need to translate the pointer argument. */ + arg = (unsigned long)compat_ptr(arg); + rc = translated_ioctl(file, cmd, arg); + break; + case COMEDI_LOCK: + case COMEDI_UNLOCK: + case COMEDI_CANCEL: + case COMEDI_POLL: + case COMEDI_SETRSUBD: + case COMEDI_SETWSUBD: + /* No translation needed. */ + rc = translated_ioctl(file, cmd, arg); + break; + case COMEDI32_CHANINFO: + rc = compat_chaninfo(file, arg); + break; + case COMEDI32_RANGEINFO: + rc = compat_rangeinfo(file, arg); + break; + case COMEDI32_CMD: + rc = compat_cmd(file, arg); + break; + case COMEDI32_CMDTEST: + rc = compat_cmdtest(file, arg); + break; + case COMEDI32_INSNLIST: + rc = compat_insnlist(file, arg); + break; + case COMEDI32_INSN: + rc = compat_insn(file, arg); + break; + default: + rc = -ENOIOCTLCMD; + break; + } + return rc; +} +#else +#define comedi_compat_ioctl NULL +#endif + static const struct file_operations comedi_fops = { .owner = THIS_MODULE, .unlocked_ioctl = comedi_unlocked_ioctl, -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:34 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat Al Viro ` (7 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index 9dfb81dfe43c..ecd29f28673c 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2871,16 +2871,6 @@ struct comedi32_insnlist_struct { compat_uptr_t insns; /* 32-bit 'struct comedi_insn *' */ }; -/* Handle translated ioctl. */ -static int translated_ioctl(struct file *file, unsigned int cmd, - unsigned long arg) -{ - if (file->f_op->unlocked_ioctl) - return file->f_op->unlocked_ioctl(file, cmd, arg); - - return -ENOTTY; -} - /* Handle 32-bit COMEDI_CHANINFO ioctl. */ static int compat_chaninfo(struct file *file, unsigned long arg) { @@ -2912,7 +2902,7 @@ static int compat_chaninfo(struct file *file, unsigned long arg) if (err) return -EFAULT; - return translated_ioctl(file, COMEDI_CHANINFO, (unsigned long)chaninfo); + return comedi_unlocked_ioctl(file, COMEDI_CHANINFO, (unsigned long)chaninfo); } /* Handle 32-bit COMEDI_RANGEINFO ioctl. */ @@ -2942,7 +2932,7 @@ static int compat_rangeinfo(struct file *file, unsigned long arg) if (err) return -EFAULT; - return translated_ioctl(file, COMEDI_RANGEINFO, + return comedi_unlocked_ioctl(file, COMEDI_RANGEINFO, (unsigned long)rangeinfo); } @@ -3063,7 +3053,7 @@ static int compat_cmd(struct file *file, unsigned long arg) if (rc) return rc; - rc = translated_ioctl(file, COMEDI_CMD, (unsigned long)cmd); + rc = comedi_unlocked_ioctl(file, COMEDI_CMD, (unsigned long)cmd); if (rc == -EAGAIN) { /* Special case: copy cmd back to user. */ err = put_compat_cmd(cmd32, cmd); @@ -3088,7 +3078,7 @@ static int compat_cmdtest(struct file *file, unsigned long arg) if (rc) return rc; - rc = translated_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd); + rc = comedi_unlocked_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd); if (rc < 0) return rc; @@ -3174,7 +3164,7 @@ static int compat_insnlist(struct file *file, unsigned long arg) return rc; } - return translated_ioctl(file, COMEDI_INSNLIST, + return comedi_unlocked_ioctl(file, COMEDI_INSNLIST, (unsigned long)&s->insnlist); } @@ -3192,7 +3182,7 @@ static int compat_insn(struct file *file, unsigned long arg) if (rc) return rc; - return translated_ioctl(file, COMEDI_INSN, (unsigned long)insn); + return comedi_unlocked_ioctl(file, COMEDI_INSN, (unsigned long)insn); } /* @@ -3212,7 +3202,7 @@ static long comedi_compat_ioctl(struct file *file, unsigned int cmd, unsigned lo case COMEDI_BUFINFO: /* Just need to translate the pointer argument. */ arg = (unsigned long)compat_ptr(arg); - rc = translated_ioctl(file, cmd, arg); + rc = comedi_unlocked_ioctl(file, cmd, arg); break; case COMEDI_LOCK: case COMEDI_UNLOCK: @@ -3221,7 +3211,7 @@ static long comedi_compat_ioctl(struct file *file, unsigned int cmd, unsigned lo case COMEDI_SETRSUBD: case COMEDI_SETWSUBD: /* No translation needed. */ - rc = translated_ioctl(file, cmd, arg); + rc = comedi_unlocked_ioctl(file, cmd, arg); break; case COMEDI32_CHANINFO: rc = compat_chaninfo(file, arg); -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() 2020-05-29 0:35 ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro @ 2020-05-29 10:34 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:34 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 26 ++++++++------------------ > 1 file changed, 8 insertions(+), 18 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro 2020-05-29 0:35 ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:35 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat Al Viro ` (6 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Just take copy_from_user() out of do_chaninfo_ioctl() into the caller and have compat_chaninfo() build a native version and pass it to do_chaninfo_ioctl() directly. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 68 ++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index ecd29f28673c..ab811735cd1b 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1049,31 +1049,28 @@ static int do_subdinfo_ioctl(struct comedi_device *dev, * array of range table lengths to chaninfo->range_table_list if requested */ static int do_chaninfo_ioctl(struct comedi_device *dev, - struct comedi_chaninfo __user *arg) + struct comedi_chaninfo *it) { struct comedi_subdevice *s; - struct comedi_chaninfo it; lockdep_assert_held(&dev->mutex); - if (copy_from_user(&it, arg, sizeof(it))) - return -EFAULT; - if (it.subdev >= dev->n_subdevices) + if (it->subdev >= dev->n_subdevices) return -EINVAL; - s = &dev->subdevices[it.subdev]; + s = &dev->subdevices[it->subdev]; - if (it.maxdata_list) { + if (it->maxdata_list) { if (s->maxdata || !s->maxdata_list) return -EINVAL; - if (copy_to_user(it.maxdata_list, s->maxdata_list, + if (copy_to_user(it->maxdata_list, s->maxdata_list, s->n_chan * sizeof(unsigned int))) return -EFAULT; } - if (it.flaglist) + if (it->flaglist) return -EINVAL; /* flaglist not supported */ - if (it.rangelist) { + if (it->rangelist) { int i; if (!s->range_table_list) @@ -1081,9 +1078,9 @@ static int do_chaninfo_ioctl(struct comedi_device *dev, for (i = 0; i < s->n_chan; i++) { int x; - x = (dev->minor << 28) | (it.subdev << 24) | (i << 16) | + x = (dev->minor << 28) | (it->subdev << 24) | (i << 16) | (s->range_table_list[i]->length); - if (put_user(x, it.rangelist + i)) + if (put_user(x, it->rangelist + i)) return -EFAULT; } } @@ -2205,9 +2202,14 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, (struct comedi_subdinfo __user *)arg, file); break; - case COMEDI_CHANINFO: - rc = do_chaninfo_ioctl(dev, (void __user *)arg); + case COMEDI_CHANINFO: { + struct comedi_chaninfo it; + if (copy_from_user(&it, (void __user *)arg, sizeof(it))) + rc = -EFAULT; + else + rc = do_chaninfo_ioctl(dev, &it); break; + } case COMEDI_RANGEINFO: rc = do_rangeinfo_ioctl(dev, (void __user *)arg); break; @@ -2874,35 +2876,25 @@ struct comedi32_insnlist_struct { /* Handle 32-bit COMEDI_CHANINFO ioctl. */ static int compat_chaninfo(struct file *file, unsigned long arg) { - struct comedi_chaninfo __user *chaninfo; - struct comedi32_chaninfo_struct __user *chaninfo32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi32_chaninfo_struct chaninfo32; + struct comedi_chaninfo chaninfo; int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - chaninfo32 = compat_ptr(arg); - chaninfo = compat_alloc_user_space(sizeof(*chaninfo)); - - /* Copy chaninfo structure. Ignore unused members. */ - if (!access_ok(chaninfo32, sizeof(*chaninfo32)) || - !access_ok(chaninfo, sizeof(*chaninfo))) + if (copy_from_user(&chaninfo32, compat_ptr(arg), sizeof(chaninfo32))) return -EFAULT; - err = 0; - err |= __get_user(temp.uint, &chaninfo32->subdev); - err |= __put_user(temp.uint, &chaninfo->subdev); - err |= __get_user(temp.uptr, &chaninfo32->maxdata_list); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->maxdata_list); - err |= __get_user(temp.uptr, &chaninfo32->flaglist); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->flaglist); - err |= __get_user(temp.uptr, &chaninfo32->rangelist); - err |= __put_user(compat_ptr(temp.uptr), &chaninfo->rangelist); - if (err) - return -EFAULT; + memset(&chaninfo, 0, sizeof(chaninfo)); + chaninfo.subdev = chaninfo32.subdev; + chaninfo.maxdata_list = compat_ptr(chaninfo32.maxdata_list); + chaninfo.flaglist = compat_ptr(chaninfo32.flaglist); + chaninfo.rangelist = compat_ptr(chaninfo32.rangelist); - return comedi_unlocked_ioctl(file, COMEDI_CHANINFO, (unsigned long)chaninfo); + mutex_lock(&dev->mutex); + err = do_chaninfo_ioctl(dev, &chaninfo); + mutex_unlock(&dev->mutex); + return err; } /* Handle 32-bit COMEDI_RANGEINFO ioctl. */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat 2020-05-29 0:35 ` [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat Al Viro @ 2020-05-29 10:35 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:35 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Just take copy_from_user() out of do_chaninfo_ioctl() into the caller and > have compat_chaninfo() build a native version and pass it to do_chaninfo_ioctl() > directly. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 68 ++++++++++++++++-------------------- > 1 file changed, 30 insertions(+), 38 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro 2020-05-29 0:35 ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro 2020-05-29 0:35 ` [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:35 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat Al Viro ` (5 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Just take copy_from_user() out of do_rangeing_ioctl() into the caller and have compat_rangeinfo() build a native version and pass it to do_rangeinfo_ioctl() directly. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 43 ++++++++++++++------------------ drivers/staging/comedi/comedi_internal.h | 2 +- drivers/staging/comedi/range.c | 17 ++++++------- 3 files changed, 27 insertions(+), 35 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index ab811735cd1b..d96dc85d8a98 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2210,9 +2210,14 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, rc = do_chaninfo_ioctl(dev, &it); break; } - case COMEDI_RANGEINFO: - rc = do_rangeinfo_ioctl(dev, (void __user *)arg); + case COMEDI_RANGEINFO: { + struct comedi_rangeinfo it; + if (copy_from_user(&it, (void __user *)arg, sizeof(it))) + rc = -EFAULT; + else + rc = do_rangeinfo_ioctl(dev, &it); break; + } case COMEDI_BUFINFO: rc = do_bufinfo_ioctl(dev, (struct comedi_bufinfo __user *)arg, @@ -2900,32 +2905,22 @@ static int compat_chaninfo(struct file *file, unsigned long arg) /* Handle 32-bit COMEDI_RANGEINFO ioctl. */ static int compat_rangeinfo(struct file *file, unsigned long arg) { - struct comedi_rangeinfo __user *rangeinfo; - struct comedi32_rangeinfo_struct __user *rangeinfo32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi32_rangeinfo_struct rangeinfo32; + struct comedi_rangeinfo rangeinfo; int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - rangeinfo32 = compat_ptr(arg); - rangeinfo = compat_alloc_user_space(sizeof(*rangeinfo)); - /* Copy rangeinfo structure. */ - if (!access_ok(rangeinfo32, sizeof(*rangeinfo32)) || - !access_ok(rangeinfo, sizeof(*rangeinfo))) + if (copy_from_user(&rangeinfo32, compat_ptr(arg), sizeof(rangeinfo32))) return -EFAULT; + memset(&rangeinfo, 0, sizeof(rangeinfo)); + rangeinfo.range_type = rangeinfo32.range_type; + rangeinfo.range_ptr = compat_ptr(rangeinfo32.range_ptr); - err = 0; - err |= __get_user(temp.uint, &rangeinfo32->range_type); - err |= __put_user(temp.uint, &rangeinfo->range_type); - err |= __get_user(temp.uptr, &rangeinfo32->range_ptr); - err |= __put_user(compat_ptr(temp.uptr), &rangeinfo->range_ptr); - if (err) - return -EFAULT; - - return comedi_unlocked_ioctl(file, COMEDI_RANGEINFO, - (unsigned long)rangeinfo); + mutex_lock(&dev->mutex); + err = do_rangeinfo_ioctl(dev, &rangeinfo); + mutex_unlock(&dev->mutex); + return err; } /* Copy 32-bit cmd structure to native cmd structure. */ diff --git a/drivers/staging/comedi/comedi_internal.h b/drivers/staging/comedi/comedi_internal.h index 515f293a5d26..7c8f18f55122 100644 --- a/drivers/staging/comedi/comedi_internal.h +++ b/drivers/staging/comedi/comedi_internal.h @@ -18,7 +18,7 @@ struct comedi_subdevice; struct device; int do_rangeinfo_ioctl(struct comedi_device *dev, - struct comedi_rangeinfo __user *arg); + struct comedi_rangeinfo *it); struct comedi_device *comedi_alloc_board_minor(struct device *hardware_device); void comedi_release_hardware_device(struct device *hardware_device); int comedi_alloc_subdevice_minor(struct comedi_subdevice *s); diff --git a/drivers/staging/comedi/range.c b/drivers/staging/comedi/range.c index 89d599877445..a4e6fe0fb729 100644 --- a/drivers/staging/comedi/range.c +++ b/drivers/staging/comedi/range.c @@ -46,17 +46,14 @@ EXPORT_SYMBOL_GPL(range_unknown); * array of comedi_krange structures to rangeinfo->range_ptr pointer */ int do_rangeinfo_ioctl(struct comedi_device *dev, - struct comedi_rangeinfo __user *arg) + struct comedi_rangeinfo *it) { - struct comedi_rangeinfo it; int subd, chan; const struct comedi_lrange *lr; struct comedi_subdevice *s; - if (copy_from_user(&it, arg, sizeof(struct comedi_rangeinfo))) - return -EFAULT; - subd = (it.range_type >> 24) & 0xf; - chan = (it.range_type >> 16) & 0xff; + subd = (it->range_type >> 24) & 0xf; + chan = (it->range_type >> 16) & 0xff; if (!dev->attached) return -EINVAL; @@ -73,15 +70,15 @@ int do_rangeinfo_ioctl(struct comedi_device *dev, return -EINVAL; } - if (RANGE_LENGTH(it.range_type) != lr->length) { + if (RANGE_LENGTH(it->range_type) != lr->length) { dev_dbg(dev->class_dev, "wrong length %d should be %d (0x%08x)\n", - RANGE_LENGTH(it.range_type), - lr->length, it.range_type); + RANGE_LENGTH(it->range_type), + lr->length, it->range_type); return -EINVAL; } - if (copy_to_user(it.range_ptr, lr->range, + if (copy_to_user(it->range_ptr, lr->range, sizeof(struct comedi_krange) * lr->length)) return -EFAULT; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat 2020-05-29 0:35 ` [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat Al Viro @ 2020-05-29 10:35 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:35 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Just take copy_from_user() out of do_rangeing_ioctl() into the caller and > have compat_rangeinfo() build a native version and pass it to do_rangeinfo_ioctl() > directly. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 43 ++++++++++++++------------------ > drivers/staging/comedi/comedi_internal.h | 2 +- > drivers/staging/comedi/range.c | 17 ++++++------- > 3 files changed, 27 insertions(+), 35 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (2 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:05 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat Al Viro ` (4 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Just take copy_from_user() out of do_insn_ioctl() into the caller and have compat_insn() build a native version and pass it to do_insn_ioctl() directly. One difference from the previous commits is that the helper used to convert 32bit variant to native has two users - compat_insn() and compat_insnlist(). The latter will be converted in next commit; for now we simply split the helper in two variants - "userland 32bit to kernel native" and "userland 32bit to userland native". The latter is renamed old get_compat_insn(); it will be gone in the next commit. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 73 +++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 27 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index d96dc85d8a98..ae0067ab5ead 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1615,22 +1615,19 @@ static int do_insnlist_ioctl(struct comedi_device *dev, * data (for reads) to insn->data pointer */ static int do_insn_ioctl(struct comedi_device *dev, - struct comedi_insn __user *arg, void *file) + struct comedi_insn *insn, void *file) { - struct comedi_insn insn; unsigned int *data = NULL; unsigned int n_data = MIN_SAMPLES; int ret = 0; lockdep_assert_held(&dev->mutex); - if (copy_from_user(&insn, arg, sizeof(insn))) - return -EFAULT; - n_data = max(n_data, insn.n); + n_data = max(n_data, insn->n); /* This is where the behavior of insn and insnlist deviate. */ - if (insn.n > MAX_SAMPLES) { - insn.n = MAX_SAMPLES; + if (insn->n > MAX_SAMPLES) { + insn->n = MAX_SAMPLES; n_data = MAX_SAMPLES; } @@ -1640,26 +1637,26 @@ static int do_insn_ioctl(struct comedi_device *dev, goto error; } - if (insn.insn & INSN_MASK_WRITE) { + if (insn->insn & INSN_MASK_WRITE) { if (copy_from_user(data, - insn.data, - insn.n * sizeof(unsigned int))) { + insn->data, + insn->n * sizeof(unsigned int))) { ret = -EFAULT; goto error; } } - ret = parse_insn(dev, &insn, data, file); + ret = parse_insn(dev, insn, data, file); if (ret < 0) goto error; - if (insn.insn & INSN_MASK_READ) { - if (copy_to_user(insn.data, + if (insn->insn & INSN_MASK_READ) { + if (copy_to_user(insn->data, data, - insn.n * sizeof(unsigned int))) { + insn->n * sizeof(unsigned int))) { ret = -EFAULT; goto error; } } - ret = insn.n; + ret = insn->n; error: kfree(data); @@ -2244,10 +2241,13 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, (struct comedi_insnlist __user *)arg, file); break; - case COMEDI_INSN: - rc = do_insn_ioctl(dev, (struct comedi_insn __user *)arg, - file); + case COMEDI_INSN: { + struct comedi_insn insn; + if (copy_from_user(&insn, (void __user *)arg, sizeof(insn))) + rc = -EFAULT; + rc = do_insn_ioctl(dev, &insn, file); break; + } case COMEDI_POLL: rc = do_poll_ioctl(dev, arg, file); break; @@ -3077,7 +3077,25 @@ static int compat_cmdtest(struct file *file, unsigned long arg) } /* Copy 32-bit insn structure to native insn structure. */ -static int get_compat_insn(struct comedi_insn __user *insn, +static int get_compat_insn(struct comedi_insn *insn, + struct comedi32_insn_struct __user *insn32) +{ + struct comedi32_insn_struct v32; + + /* Copy insn structure. Ignore the unused members. */ + if (copy_from_user(&v32, insn32, sizeof(v32))) + return -EFAULT; + memset(insn, 0, sizeof(*insn)); + insn->insn = v32.insn; + insn->n = v32.n; + insn->data = compat_ptr(v32.data); + insn->subdev = v32.subdev; + insn->chanspec = v32.chanspec; + return 0; +} + +/* Copy 32-bit insn structure to native insn structure. */ +static int __get_compat_insn(struct comedi_insn __user *insn, struct comedi32_insn_struct __user *insn32) { int err; @@ -3146,7 +3164,7 @@ static int compat_insnlist(struct file *file, unsigned long arg) /* Copy insn structures. */ for (n = 0; n < n_insns; n++) { - rc = get_compat_insn(&s->insn[n], &insn32[n]); + rc = __get_compat_insn(&s->insn[n], &insn32[n]); if (rc) return rc; } @@ -3158,18 +3176,19 @@ static int compat_insnlist(struct file *file, unsigned long arg) /* Handle 32-bit COMEDI_INSN ioctl. */ static int compat_insn(struct file *file, unsigned long arg) { - struct comedi_insn __user *insn; - struct comedi32_insn_struct __user *insn32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi_insn insn; int rc; - insn32 = compat_ptr(arg); - insn = compat_alloc_user_space(sizeof(*insn)); - - rc = get_compat_insn(insn, insn32); + rc = get_compat_insn(&insn, (void __user *)arg); if (rc) return rc; - return comedi_unlocked_ioctl(file, COMEDI_INSN, (unsigned long)insn); + mutex_lock(&dev->mutex); + rc = do_insn_ioctl(dev, &insn, file); + mutex_unlock(&dev->mutex); + return rc; } /* -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat 2020-05-29 0:35 ` [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat Al Viro @ 2020-05-29 10:05 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:05 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Just take copy_from_user() out of do_insn_ioctl() into the caller and > have compat_insn() build a native version and pass it to do_insn_ioctl() > directly. > > One difference from the previous commits is that the helper used to > convert 32bit variant to native has two users - compat_insn() and > compat_insnlist(). The latter will be converted in next commit; > for now we simply split the helper in two variants - "userland 32bit > to kernel native" and "userland 32bit to userland native". The latter > is renamed old get_compat_insn(); it will be gone in the next commit. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 73 +++++++++++++++++++++++------------- > 1 file changed, 46 insertions(+), 27 deletions(-) > > diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c > index d96dc85d8a98..ae0067ab5ead 100644 [snip] > @@ -2244,10 +2241,13 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, > (struct comedi_insnlist __user *)arg, > file); > break; > - case COMEDI_INSN: > - rc = do_insn_ioctl(dev, (struct comedi_insn __user *)arg, > - file); > + case COMEDI_INSN: { > + struct comedi_insn insn; > + if (copy_from_user(&insn, (void __user *)arg, sizeof(insn))) > + rc = -EFAULT; Missing an 'else' here: > + rc = do_insn_ioctl(dev, &insn, file); > break; > + } > case COMEDI_POLL: -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (3 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:36 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() Al Viro ` (3 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 138 ++++++++++++----------------------- 1 file changed, 48 insertions(+), 90 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index ae0067ab5ead..d80a416e70b2 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1520,34 +1520,19 @@ static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn, #define MIN_SAMPLES 16 #define MAX_SAMPLES 65536 static int do_insnlist_ioctl(struct comedi_device *dev, - struct comedi_insnlist __user *arg, void *file) + struct comedi_insn *insns, + unsigned int n_insns, + void *file) { - struct comedi_insnlist insnlist; - struct comedi_insn *insns = NULL; unsigned int *data = NULL; unsigned int max_n_data_required = MIN_SAMPLES; int i = 0; int ret = 0; lockdep_assert_held(&dev->mutex); - if (copy_from_user(&insnlist, arg, sizeof(insnlist))) - return -EFAULT; - - insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL); - if (!insns) { - ret = -ENOMEM; - goto error; - } - - if (copy_from_user(insns, insnlist.insns, - sizeof(*insns) * insnlist.n_insns)) { - dev_dbg(dev->class_dev, "copy_from_user failed\n"); - ret = -EFAULT; - goto error; - } /* Determine maximum memory needed for all instructions. */ - for (i = 0; i < insnlist.n_insns; ++i) { + for (i = 0; i < n_insns; ++i) { if (insns[i].n > MAX_SAMPLES) { dev_dbg(dev->class_dev, "number of samples too large\n"); @@ -1565,7 +1550,7 @@ static int do_insnlist_ioctl(struct comedi_device *dev, goto error; } - for (i = 0; i < insnlist.n_insns; ++i) { + for (i = 0; i < n_insns; ++i) { if (insns[i].insn & INSN_MASK_WRITE) { if (copy_from_user(data, insns[i].data, insns[i].n * sizeof(unsigned int))) { @@ -1592,7 +1577,6 @@ static int do_insnlist_ioctl(struct comedi_device *dev, } error: - kfree(insns); kfree(data); if (ret < 0) @@ -2236,11 +2220,30 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, rc = do_cmdtest_ioctl(dev, (struct comedi_cmd __user *)arg, file); break; - case COMEDI_INSNLIST: - rc = do_insnlist_ioctl(dev, - (struct comedi_insnlist __user *)arg, - file); + case COMEDI_INSNLIST: { + struct comedi_insnlist insnlist; + struct comedi_insn *insns = NULL; + + if (copy_from_user(&insnlist, (void __user *)arg, + sizeof(insnlist))) { + rc = -EFAULT; + break; + } + insns = kcalloc(insnlist.n_insns, sizeof(*insns), GFP_KERNEL); + if (!insns) { + rc = -ENOMEM; + break; + } + if (copy_from_user(insns, insnlist.insns, + sizeof(*insns) * insnlist.n_insns)) { + rc = -EFAULT; + kfree(insns); + break; + } + rc = do_insnlist_ioctl(dev, insns, insnlist.n_insns, file); + kfree(insns); break; + } case COMEDI_INSN: { struct comedi_insn insn; if (copy_from_user(&insn, (void __user *)arg, sizeof(insn))) @@ -3094,83 +3097,38 @@ static int get_compat_insn(struct comedi_insn *insn, return 0; } -/* Copy 32-bit insn structure to native insn structure. */ -static int __get_compat_insn(struct comedi_insn __user *insn, - struct comedi32_insn_struct __user *insn32) -{ - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - /* Copy insn structure. Ignore the unused members. */ - err = 0; - if (!access_ok(insn32, sizeof(*insn32)) || - !access_ok(insn, sizeof(*insn))) - return -EFAULT; - - err |= __get_user(temp.uint, &insn32->insn); - err |= __put_user(temp.uint, &insn->insn); - err |= __get_user(temp.uint, &insn32->n); - err |= __put_user(temp.uint, &insn->n); - err |= __get_user(temp.uptr, &insn32->data); - err |= __put_user(compat_ptr(temp.uptr), &insn->data); - err |= __get_user(temp.uint, &insn32->subdev); - err |= __put_user(temp.uint, &insn->subdev); - err |= __get_user(temp.uint, &insn32->chanspec); - err |= __put_user(temp.uint, &insn->chanspec); - return err ? -EFAULT : 0; -} - /* Handle 32-bit COMEDI_INSNLIST ioctl. */ static int compat_insnlist(struct file *file, unsigned long arg) { - struct combined_insnlist { - struct comedi_insnlist insnlist; - struct comedi_insn insn[1]; - } __user *s; - struct comedi32_insnlist_struct __user *insnlist32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi32_insnlist_struct insnlist32; struct comedi32_insn_struct __user *insn32; - compat_uptr_t uptr; - unsigned int n_insns, n; - int err, rc; - - insnlist32 = compat_ptr(arg); - - /* Get 32-bit insnlist structure. */ - if (!access_ok(insnlist32, sizeof(*insnlist32))) - return -EFAULT; - - err = 0; - err |= __get_user(n_insns, &insnlist32->n_insns); - err |= __get_user(uptr, &insnlist32->insns); - insn32 = compat_ptr(uptr); - if (err) - return -EFAULT; - - /* Allocate user memory to copy insnlist and insns into. */ - s = compat_alloc_user_space(offsetof(struct combined_insnlist, - insn[n_insns])); + struct comedi_insn *insns; + unsigned int n; + int rc; - /* Set native insnlist structure. */ - if (!access_ok(&s->insnlist, sizeof(s->insnlist))) + if (copy_from_user(&insnlist32, compat_ptr(arg), sizeof(insnlist32))) return -EFAULT; - err |= __put_user(n_insns, &s->insnlist.n_insns); - err |= __put_user(&s->insn[0], &s->insnlist.insns); - if (err) - return -EFAULT; + insns = kcalloc(insnlist32.n_insns, sizeof(*insns), GFP_KERNEL); + if (!insns) + return -ENOMEM; /* Copy insn structures. */ - for (n = 0; n < n_insns; n++) { - rc = __get_compat_insn(&s->insn[n], &insn32[n]); - if (rc) + insn32 = compat_ptr(insnlist32.insns); + for (n = 0; n < insnlist32.n_insns; n++) { + rc = get_compat_insn(insns + n, insn32 + n); + if (rc) { + kfree(insns); return rc; + } } - return comedi_unlocked_ioctl(file, COMEDI_INSNLIST, - (unsigned long)&s->insnlist); + mutex_lock(&dev->mutex); + rc = do_insnlist_ioctl(dev, insns, insnlist32.n_insns, file); + mutex_unlock(&dev->mutex); + return rc; } /* Handle 32-bit COMEDI_INSN ioctl. */ -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat 2020-05-29 0:35 ` [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat Al Viro @ 2020-05-29 10:36 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:36 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 138 ++++++++++++----------------------- > 1 file changed, 48 insertions(+), 90 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (4 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:37 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller Al Viro ` (2 subsequent siblings) 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index d80a416e70b2..e85a143057a1 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1649,17 +1649,11 @@ static int do_insn_ioctl(struct comedi_device *dev, } static int __comedi_get_user_cmd(struct comedi_device *dev, - struct comedi_cmd __user *arg, struct comedi_cmd *cmd) { struct comedi_subdevice *s; lockdep_assert_held(&dev->mutex); - if (copy_from_user(cmd, arg, sizeof(*cmd))) { - dev_dbg(dev->class_dev, "bad cmd address\n"); - return -EFAULT; - } - if (cmd->subdev >= dev->n_subdevices) { dev_dbg(dev->class_dev, "%d no such subdevice\n", cmd->subdev); return -ENODEV; @@ -1757,8 +1751,13 @@ static int do_cmd_ioctl(struct comedi_device *dev, lockdep_assert_held(&dev->mutex); + if (copy_from_user(&cmd, arg, sizeof(cmd))) { + dev_dbg(dev->class_dev, "bad cmd address\n"); + return -EFAULT; + } + /* get the user's cmd and do some simple validation */ - ret = __comedi_get_user_cmd(dev, arg, &cmd); + ret = __comedi_get_user_cmd(dev, &cmd); if (ret) return ret; @@ -1866,8 +1865,13 @@ static int do_cmdtest_ioctl(struct comedi_device *dev, lockdep_assert_held(&dev->mutex); + if (copy_from_user(&cmd, arg, sizeof(cmd))) { + dev_dbg(dev->class_dev, "bad cmd address\n"); + return -EFAULT; + } + /* get the user's cmd and do some simple validation */ - ret = __comedi_get_user_cmd(dev, arg, &cmd); + ret = __comedi_get_user_cmd(dev, &cmd); if (ret) return ret; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() 2020-05-29 0:35 ` [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() Al Viro @ 2020-05-29 10:37 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:37 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (5 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:37 ` Ian Abbott 2020-05-29 0:35 ` [PATCH 09/10] comedi: do_cmd_ioctl(): " Al Viro 2020-05-29 10:34 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Ian Abbott 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 45 ++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 23 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index e85a143057a1..a40a865ed45c 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1856,49 +1856,39 @@ static int do_cmd_ioctl(struct comedi_device *dev, * possibly modified comedi_cmd structure */ static int do_cmdtest_ioctl(struct comedi_device *dev, - struct comedi_cmd __user *arg, void *file) + struct comedi_cmd *cmd, bool *copy, void *file) { - struct comedi_cmd cmd; struct comedi_subdevice *s; unsigned int __user *user_chanlist; int ret; lockdep_assert_held(&dev->mutex); - if (copy_from_user(&cmd, arg, sizeof(cmd))) { - dev_dbg(dev->class_dev, "bad cmd address\n"); - return -EFAULT; - } - - /* get the user's cmd and do some simple validation */ - ret = __comedi_get_user_cmd(dev, &cmd); + /* do some simple cmd validation */ + ret = __comedi_get_user_cmd(dev, cmd); if (ret) return ret; /* save user's chanlist pointer so it can be restored later */ - user_chanlist = (unsigned int __user *)cmd.chanlist; + user_chanlist = (unsigned int __user *)cmd->chanlist; - s = &dev->subdevices[cmd.subdev]; + s = &dev->subdevices[cmd->subdev]; /* user_chanlist can be NULL for COMEDI_CMDTEST ioctl */ if (user_chanlist) { /* load channel/gain list */ - ret = __comedi_get_user_chanlist(dev, s, user_chanlist, &cmd); + ret = __comedi_get_user_chanlist(dev, s, user_chanlist, cmd); if (ret) return ret; } - ret = s->do_cmdtest(dev, s, &cmd); + ret = s->do_cmdtest(dev, s, cmd); - kfree(cmd.chanlist); /* free kernel copy of user chanlist */ + kfree(cmd->chanlist); /* free kernel copy of user chanlist */ /* restore chanlist pointer before copying back */ - cmd.chanlist = (unsigned int __force *)user_chanlist; - - if (copy_to_user(arg, &cmd, sizeof(cmd))) { - dev_dbg(dev->class_dev, "bad cmd address\n"); - ret = -EFAULT; - } + cmd->chanlist = (unsigned int __force *)user_chanlist; + *copy = true; return ret; } @@ -2220,10 +2210,19 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, case COMEDI_CMD: rc = do_cmd_ioctl(dev, (struct comedi_cmd __user *)arg, file); break; - case COMEDI_CMDTEST: - rc = do_cmdtest_ioctl(dev, (struct comedi_cmd __user *)arg, - file); + case COMEDI_CMDTEST: { + struct comedi_cmd cmd; + bool copy = false; + + if (copy_from_user(&cmd, (void __user *)arg, sizeof(cmd))) { + rc = -EFAULT; + break; + } + rc = do_cmdtest_ioctl(dev, &cmd, ©, file); + if (copy && copy_to_user((void __user *)arg, &cmd, sizeof(cmd))) + rc = -EFAULT; break; + } case COMEDI_INSNLIST: { struct comedi_insnlist insnlist; struct comedi_insn *insns = NULL; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller 2020-05-29 0:35 ` [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller Al Viro @ 2020-05-29 10:37 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:37 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 45 ++++++++++++++++++------------------ > 1 file changed, 22 insertions(+), 23 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 09/10] comedi: do_cmd_ioctl(): lift copyin/copyout into the caller 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (6 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller Al Viro @ 2020-05-29 0:35 ` Al Viro 2020-05-29 10:38 ` Ian Abbott 2020-05-29 10:34 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Ian Abbott 8 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:35 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Ian Abbott From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index a40a865ed45c..f5ecfbfcdaf5 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -1741,9 +1741,8 @@ static int __comedi_get_user_chanlist(struct comedi_device *dev, * possibly modified comedi_cmd structure (when -EAGAIN returned) */ static int do_cmd_ioctl(struct comedi_device *dev, - struct comedi_cmd __user *arg, void *file) + struct comedi_cmd *cmd, bool *copy, void *file) { - struct comedi_cmd cmd; struct comedi_subdevice *s; struct comedi_async *async; unsigned int __user *user_chanlist; @@ -1751,20 +1750,15 @@ static int do_cmd_ioctl(struct comedi_device *dev, lockdep_assert_held(&dev->mutex); - if (copy_from_user(&cmd, arg, sizeof(cmd))) { - dev_dbg(dev->class_dev, "bad cmd address\n"); - return -EFAULT; - } - - /* get the user's cmd and do some simple validation */ - ret = __comedi_get_user_cmd(dev, &cmd); + /* do some simple cmd validation */ + ret = __comedi_get_user_cmd(dev, cmd); if (ret) return ret; /* save user's chanlist pointer so it can be restored later */ - user_chanlist = (unsigned int __user *)cmd.chanlist; + user_chanlist = (unsigned int __user *)cmd->chanlist; - s = &dev->subdevices[cmd.subdev]; + s = &dev->subdevices[cmd->subdev]; async = s->async; /* are we locked? (ioctl lock) */ @@ -1780,13 +1774,13 @@ static int do_cmd_ioctl(struct comedi_device *dev, } /* make sure channel/gain list isn't too short */ - if (cmd.chanlist_len < 1) { + if (cmd->chanlist_len < 1) { dev_dbg(dev->class_dev, "channel/gain list too short %u < 1\n", - cmd.chanlist_len); + cmd->chanlist_len); return -EINVAL; } - async->cmd = cmd; + async->cmd = *cmd; async->cmd.data = NULL; /* load channel/gain list */ @@ -1798,15 +1792,11 @@ static int do_cmd_ioctl(struct comedi_device *dev, if (async->cmd.flags & CMDF_BOGUS || ret) { dev_dbg(dev->class_dev, "test returned %d\n", ret); - cmd = async->cmd; + *cmd = async->cmd; /* restore chanlist pointer before copying back */ - cmd.chanlist = (unsigned int __force *)user_chanlist; - cmd.data = NULL; - if (copy_to_user(arg, &cmd, sizeof(cmd))) { - dev_dbg(dev->class_dev, "fault writing cmd\n"); - ret = -EFAULT; - goto cleanup; - } + cmd->chanlist = (unsigned int __force *)user_chanlist; + cmd->data = NULL; + *copy = true; ret = -EAGAIN; goto cleanup; } @@ -2207,9 +2197,19 @@ static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, case COMEDI_CANCEL: rc = do_cancel_ioctl(dev, arg, file); break; - case COMEDI_CMD: - rc = do_cmd_ioctl(dev, (struct comedi_cmd __user *)arg, file); + case COMEDI_CMD: { + struct comedi_cmd cmd; + bool copy = false; + + if (copy_from_user(&cmd, (void __user *)arg, sizeof(cmd))) { + rc = -EFAULT; + break; + } + rc = do_cmd_ioctl(dev, &cmd, ©, file); + if (copy && copy_to_user((void __user *)arg, &cmd, sizeof(cmd))) + rc = -EFAULT; break; + } case COMEDI_CMDTEST: { struct comedi_cmd cmd; bool copy = false; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 09/10] comedi: do_cmd_ioctl(): lift copyin/copyout into the caller 2020-05-29 0:35 ` [PATCH 09/10] comedi: do_cmd_ioctl(): " Al Viro @ 2020-05-29 10:38 ` Ian Abbott 0 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:38 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++------------------ > 1 file changed, 24 insertions(+), 24 deletions(-) Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 01/10] comedi: move compat ioctl handling to native fops 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro ` (7 preceding siblings ...) 2020-05-29 0:35 ` [PATCH 09/10] comedi: do_cmd_ioctl(): " Al Viro @ 2020-05-29 10:34 ` Ian Abbott 8 siblings, 0 replies; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:34 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:35, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > mechanical move > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > drivers/staging/comedi/Makefile | 1 - > drivers/staging/comedi/comedi_compat32.c | 455 ------------------------------- > drivers/staging/comedi/comedi_compat32.h | 28 -- > drivers/staging/comedi/comedi_fops.c | 451 +++++++++++++++++++++++++++++- > 4 files changed, 448 insertions(+), 487 deletions(-) > delete mode 100644 drivers/staging/comedi/comedi_compat32.c > delete mode 100644 drivers/staging/comedi/comedi_compat32.h > Signed-off-by: Ian Abbott <abbotti@mev.co.uk> -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess comedi compat 2020-05-29 0:34 ` [PATCHES] uaccess comedi compat Al Viro 2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro @ 2020-05-29 10:48 ` Ian Abbott 2020-05-29 14:15 ` Al Viro 1 sibling, 1 reply; 94+ messages in thread From: Ian Abbott @ 2020-05-29 10:48 UTC (permalink / raw) To: Al Viro, Linus Torvalds; +Cc: linux-kernel, linux-fsdevel On 29/05/2020 01:34, Al Viro wrote: > The way comedi compat ioctls are done is wrong. > Instead of having ->compat_ioctl() copying the 32bit > stuff in, then passing the kernel copies to helpers shared > with native ->ioctl() and doing copyout with conversion if > needed, it's playing silly buggers with creating a 64bit > copy on user stack, then calling native ioctl (which copies > that copy into the kernel), then fetching it from user stack, > converting to 32bit variant and copying that to user. > Extra headache for no good reason. And the single > largest remaining pile of __put_user()/__get_user() this side > of arch/*. IMO compat_alloc_user_space() should die... > > NOTE: this is only compile-tested - I simply don't > have the hardware in question. > > Anyway, the branch lives in #uaccess.comedi, based > at v5.7-rc1 > > Al Viro (10): > comedi: move compat ioctl handling to native fops > comedi: get rid of indirection via translated_ioctl() > comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat > comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat > comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat > comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat > comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() > comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller > comedi: do_cmd_ioctl(): lift copyin/copyout into the caller > comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat There is a bug in patch 05. Patch 10 doesn't seem to have been sent yet (I didn't receive it and I can't see it in the thread in the LKML archives). I've signed off on 01-04, 06-09. These should be Cc'd to Greg KH and to devel@driverdev.osuosl.org. Cheers, Ian -- -=( Ian Abbott <abbotti@mev.co.uk> || Web: www.mev.co.uk )=- -=( MEV Ltd. is a company registered in England & Wales. )=- -=( Registered number: 02862268. Registered address: )=- -=( 15 West Park Road, Bramhall, STOCKPORT, SK7 3JZ, UK. )=- ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess comedi compat 2020-05-29 10:48 ` [PATCHES] uaccess comedi compat Ian Abbott @ 2020-05-29 14:15 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 14:15 UTC (permalink / raw) To: Ian Abbott; +Cc: Linus Torvalds, linux-kernel, linux-fsdevel On Fri, May 29, 2020 at 11:48:51AM +0100, Ian Abbott wrote: > > Al Viro (10): > > comedi: move compat ioctl handling to native fops > > comedi: get rid of indirection via translated_ioctl() > > comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat > > comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat > > comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat > > comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat > > comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() > > comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller > > comedi: do_cmd_ioctl(): lift copyin/copyout into the caller > > comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat > > There is a bug in patch 05. Patch 10 doesn't seem to have been sent yet (I > didn't receive it and I can't see it in the thread in the LKML archives). > I've signed off on 01-04, 06-09. #5 fixed, force-pushed to the same branch. As for s-o-b... are you sure that's the header you have in mind? Normally it's for the chain of transmission... Do you offer to take that series through comedi (or staging, or...) git tree? In that case s-o-b would make sense and I'd be happy to have it taken off my hands. Otherwise it probably should be Acked-by: or Reviewed-by: or Read-through-and-managed-not-to-throw-up: - up to you... > These should be Cc'd to Greg KH and to devel@driverdev.osuosl.org. FWIW, 10/10 seems to have been really lost; follows here: From 88833127a8f00da422ddef03425ad9b19eb65558 Mon Sep 17 00:00:00 2001 From: Al Viro <viro@zeniv.linux.org.uk> Date: Sun, 26 Apr 2020 09:27:23 -0400 Subject: [PATCH 10/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CMD{,TEST} compat Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/staging/comedi/comedi_fops.c | 181 +++++++++++++---------------------- 1 file changed, 66 insertions(+), 115 deletions(-) diff --git a/drivers/staging/comedi/comedi_fops.c b/drivers/staging/comedi/comedi_fops.c index f5ecfbfcdaf5..bcdb059e6bb6 100644 --- a/drivers/staging/comedi/comedi_fops.c +++ b/drivers/staging/comedi/comedi_fops.c @@ -2930,155 +2930,106 @@ static int compat_rangeinfo(struct file *file, unsigned long arg) } /* Copy 32-bit cmd structure to native cmd structure. */ -static int get_compat_cmd(struct comedi_cmd __user *cmd, +static int get_compat_cmd(struct comedi_cmd *cmd, struct comedi32_cmd_struct __user *cmd32) { - int err; - union { - unsigned int uint; - compat_uptr_t uptr; - } temp; - - /* Copy cmd structure. */ - if (!access_ok(cmd32, sizeof(*cmd32)) || - !access_ok(cmd, sizeof(*cmd))) + struct comedi32_cmd_struct v32; + + if (copy_from_user(&v32, cmd32, sizeof(v32))) return -EFAULT; - err = 0; - err |= __get_user(temp.uint, &cmd32->subdev); - err |= __put_user(temp.uint, &cmd->subdev); - err |= __get_user(temp.uint, &cmd32->flags); - err |= __put_user(temp.uint, &cmd->flags); - err |= __get_user(temp.uint, &cmd32->start_src); - err |= __put_user(temp.uint, &cmd->start_src); - err |= __get_user(temp.uint, &cmd32->start_arg); - err |= __put_user(temp.uint, &cmd->start_arg); - err |= __get_user(temp.uint, &cmd32->scan_begin_src); - err |= __put_user(temp.uint, &cmd->scan_begin_src); - err |= __get_user(temp.uint, &cmd32->scan_begin_arg); - err |= __put_user(temp.uint, &cmd->scan_begin_arg); - err |= __get_user(temp.uint, &cmd32->convert_src); - err |= __put_user(temp.uint, &cmd->convert_src); - err |= __get_user(temp.uint, &cmd32->convert_arg); - err |= __put_user(temp.uint, &cmd->convert_arg); - err |= __get_user(temp.uint, &cmd32->scan_end_src); - err |= __put_user(temp.uint, &cmd->scan_end_src); - err |= __get_user(temp.uint, &cmd32->scan_end_arg); - err |= __put_user(temp.uint, &cmd->scan_end_arg); - err |= __get_user(temp.uint, &cmd32->stop_src); - err |= __put_user(temp.uint, &cmd->stop_src); - err |= __get_user(temp.uint, &cmd32->stop_arg); - err |= __put_user(temp.uint, &cmd->stop_arg); - err |= __get_user(temp.uptr, &cmd32->chanlist); - err |= __put_user((unsigned int __force *)compat_ptr(temp.uptr), - &cmd->chanlist); - err |= __get_user(temp.uint, &cmd32->chanlist_len); - err |= __put_user(temp.uint, &cmd->chanlist_len); - err |= __get_user(temp.uptr, &cmd32->data); - err |= __put_user(compat_ptr(temp.uptr), &cmd->data); - err |= __get_user(temp.uint, &cmd32->data_len); - err |= __put_user(temp.uint, &cmd->data_len); - return err ? -EFAULT : 0; + cmd->subdev = v32.subdev; + cmd->flags = v32.flags; + cmd->start_src = v32.start_src; + cmd->start_arg = v32.start_arg; + cmd->scan_begin_src = v32.scan_begin_src; + cmd->scan_begin_arg = v32.scan_begin_arg; + cmd->convert_src = v32.convert_src; + cmd->convert_arg = v32.convert_arg; + cmd->scan_end_src = v32.scan_end_src; + cmd->scan_end_arg = v32.scan_end_arg; + cmd->stop_src = v32.stop_src; + cmd->stop_arg = v32.stop_arg; + cmd->chanlist = compat_ptr(v32.chanlist); + cmd->chanlist_len = v32.chanlist_len; + cmd->data = compat_ptr(v32.data); + cmd->data_len = v32.data_len; + return 0; } /* Copy native cmd structure to 32-bit cmd structure. */ static int put_compat_cmd(struct comedi32_cmd_struct __user *cmd32, - struct comedi_cmd __user *cmd) -{ - int err; - unsigned int temp; - - /* - * Copy back most of cmd structure. - * - * Assume the pointer values are already valid. - * (Could use ptr_to_compat() to set them.) - */ - if (!access_ok(cmd, sizeof(*cmd)) || - !access_ok(cmd32, sizeof(*cmd32))) - return -EFAULT; - - err = 0; - err |= __get_user(temp, &cmd->subdev); - err |= __put_user(temp, &cmd32->subdev); - err |= __get_user(temp, &cmd->flags); - err |= __put_user(temp, &cmd32->flags); - err |= __get_user(temp, &cmd->start_src); - err |= __put_user(temp, &cmd32->start_src); - err |= __get_user(temp, &cmd->start_arg); - err |= __put_user(temp, &cmd32->start_arg); - err |= __get_user(temp, &cmd->scan_begin_src); - err |= __put_user(temp, &cmd32->scan_begin_src); - err |= __get_user(temp, &cmd->scan_begin_arg); - err |= __put_user(temp, &cmd32->scan_begin_arg); - err |= __get_user(temp, &cmd->convert_src); - err |= __put_user(temp, &cmd32->convert_src); - err |= __get_user(temp, &cmd->convert_arg); - err |= __put_user(temp, &cmd32->convert_arg); - err |= __get_user(temp, &cmd->scan_end_src); - err |= __put_user(temp, &cmd32->scan_end_src); - err |= __get_user(temp, &cmd->scan_end_arg); - err |= __put_user(temp, &cmd32->scan_end_arg); - err |= __get_user(temp, &cmd->stop_src); - err |= __put_user(temp, &cmd32->stop_src); - err |= __get_user(temp, &cmd->stop_arg); - err |= __put_user(temp, &cmd32->stop_arg); + struct comedi_cmd *cmd) +{ + struct comedi32_cmd_struct v32; + + memset(&v32, 0, sizeof(v32)); + v32.subdev = cmd->subdev; + v32.flags = cmd->flags; + v32.start_src = cmd->start_src; + v32.start_arg = cmd->start_arg; + v32.scan_begin_src = cmd->scan_begin_src; + v32.scan_begin_arg = cmd->scan_begin_arg; + v32.convert_src = cmd->convert_src; + v32.convert_arg = cmd->convert_arg; + v32.scan_end_src = cmd->scan_end_src; + v32.scan_end_arg = cmd->scan_end_arg; + v32.stop_src = cmd->stop_src; + v32.stop_arg = cmd->stop_arg; /* Assume chanlist pointer is unchanged. */ - err |= __get_user(temp, &cmd->chanlist_len); - err |= __put_user(temp, &cmd32->chanlist_len); - /* Assume data pointer is unchanged. */ - err |= __get_user(temp, &cmd->data_len); - err |= __put_user(temp, &cmd32->data_len); - return err ? -EFAULT : 0; + v32.chanlist = ptr_to_compat(cmd->chanlist); + v32.chanlist_len = cmd->chanlist_len; + v32.data = ptr_to_compat(cmd->data); + v32.data_len = cmd->data_len; + return copy_to_user(cmd32, &v32, sizeof(v32)); } /* Handle 32-bit COMEDI_CMD ioctl. */ static int compat_cmd(struct file *file, unsigned long arg) { - struct comedi_cmd __user *cmd; - struct comedi32_cmd_struct __user *cmd32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi_cmd cmd; + bool copy = false; int rc, err; - cmd32 = compat_ptr(arg); - cmd = compat_alloc_user_space(sizeof(*cmd)); - - rc = get_compat_cmd(cmd, cmd32); + rc = get_compat_cmd(&cmd, compat_ptr(arg)); if (rc) return rc; - rc = comedi_unlocked_ioctl(file, COMEDI_CMD, (unsigned long)cmd); - if (rc == -EAGAIN) { + mutex_lock(&dev->mutex); + rc = do_cmd_ioctl(dev, &cmd, ©, file); + mutex_unlock(&dev->mutex); + if (copy) { /* Special case: copy cmd back to user. */ - err = put_compat_cmd(cmd32, cmd); + err = put_compat_cmd(compat_ptr(arg), &cmd); if (err) rc = err; } - return rc; } /* Handle 32-bit COMEDI_CMDTEST ioctl. */ static int compat_cmdtest(struct file *file, unsigned long arg) { - struct comedi_cmd __user *cmd; - struct comedi32_cmd_struct __user *cmd32; + struct comedi_file *cfp = file->private_data; + struct comedi_device *dev = cfp->dev; + struct comedi_cmd cmd; + bool copy = false; int rc, err; - cmd32 = compat_ptr(arg); - cmd = compat_alloc_user_space(sizeof(*cmd)); - - rc = get_compat_cmd(cmd, cmd32); + rc = get_compat_cmd(&cmd, compat_ptr(arg)); if (rc) return rc; - rc = comedi_unlocked_ioctl(file, COMEDI_CMDTEST, (unsigned long)cmd); - if (rc < 0) - return rc; - - err = put_compat_cmd(cmd32, cmd); - if (err) - rc = err; - + mutex_lock(&dev->mutex); + rc = do_cmdtest_ioctl(dev, &cmd, ©, file); + mutex_unlock(&dev->mutex); + if (copy) { + err = put_compat_cmd(compat_ptr(arg), &cmd); + if (err) + rc = err; + } return rc; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCHES] uaccess i915 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (5 preceding siblings ...) 2020-05-29 0:34 ` [PATCHES] uaccess comedi compat Al Viro @ 2020-05-29 0:40 ` Al Viro 2020-05-29 5:06 ` [Intel-gfx] " Jani Nikula 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro ` (4 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 0:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula Low-hanging fruit in i915 uaccess-related stuff. There's some subtler stuff remaining after that; these are the simple ones. Branch in uaccess.i915, based at uaccess.base. Al Viro (5): i915: switch query_{topology,engine}_info() to copy_to_user() i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() i915 compat ioctl(): just use drm_ioctl_kernel() i915: alloc_oa_regs(): get rid of pointless access_ok() i915:get_engines(): get rid of pointless access_ok() drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 --- drivers/gpu/drm/i915/i915_ioc32.c | 14 +++---- drivers/gpu/drm/i915/i915_perf.c | 3 -- drivers/gpu/drm/i915/i915_query.c | 62 ++++++++++------------------- drivers/gpu/drm/i915/i915_reg.h | 2 +- 5 files changed, 28 insertions(+), 58 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess i915 2020-05-29 0:40 ` [PATCHES] uaccess i915 Al Viro @ 2020-05-29 5:06 ` Jani Nikula 0 siblings, 0 replies; 94+ messages in thread From: Jani Nikula @ 2020-05-29 5:06 UTC (permalink / raw) To: Al Viro, Linus Torvalds Cc: linux-kernel, linux-fsdevel, intel-gfx, Chris Wilson On Fri, 29 May 2020, Al Viro <viro@zeniv.linux.org.uk> wrote: > Low-hanging fruit in i915 uaccess-related stuff. > There's some subtler stuff remaining after that; these > are the simple ones. Please Cc: intel-gfx@lists.freedesktop.org for i915 changes. Also added Chris who I believe will be able to best review the changes. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Intel-gfx] [PATCHES] uaccess i915 @ 2020-05-29 5:06 ` Jani Nikula 0 siblings, 0 replies; 94+ messages in thread From: Jani Nikula @ 2020-05-29 5:06 UTC (permalink / raw) To: Al Viro, Linus Torvalds Cc: linux-fsdevel, intel-gfx, linux-kernel, Chris Wilson On Fri, 29 May 2020, Al Viro <viro@zeniv.linux.org.uk> wrote: > Low-hanging fruit in i915 uaccess-related stuff. > There's some subtler stuff remaining after that; these > are the simple ones. Please Cc: intel-gfx@lists.freedesktop.org for i915 changes. Also added Chris who I believe will be able to best review the changes. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess i915 2020-05-29 5:06 ` [Intel-gfx] " Jani Nikula @ 2020-05-29 14:17 ` Al Viro -1 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 14:17 UTC (permalink / raw) To: Jani Nikula Cc: Linus Torvalds, linux-kernel, linux-fsdevel, intel-gfx, Chris Wilson On Fri, May 29, 2020 at 08:06:14AM +0300, Jani Nikula wrote: > On Fri, 29 May 2020, Al Viro <viro@zeniv.linux.org.uk> wrote: > > Low-hanging fruit in i915 uaccess-related stuff. > > There's some subtler stuff remaining after that; these > > are the simple ones. > > Please Cc: intel-gfx@lists.freedesktop.org for i915 changes. Will do. Do you want me to resend those (or post them there separately, or...)? > Also added Chris who I believe will be able to best review the changes. FWIW, the branch is in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #uaccess.i915 ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [Intel-gfx] [PATCHES] uaccess i915 @ 2020-05-29 14:17 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 14:17 UTC (permalink / raw) To: Jani Nikula Cc: linux-fsdevel, intel-gfx, Linus Torvalds, linux-kernel, Chris Wilson On Fri, May 29, 2020 at 08:06:14AM +0300, Jani Nikula wrote: > On Fri, 29 May 2020, Al Viro <viro@zeniv.linux.org.uk> wrote: > > Low-hanging fruit in i915 uaccess-related stuff. > > There's some subtler stuff remaining after that; these > > are the simple ones. > > Please Cc: intel-gfx@lists.freedesktop.org for i915 changes. Will do. Do you want me to resend those (or post them there separately, or...)? > Also added Chris who I believe will be able to best review the changes. FWIW, the branch is in git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git #uaccess.i915 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (6 preceding siblings ...) 2020-05-29 0:40 ` [PATCHES] uaccess i915 Al Viro @ 2020-05-29 0:41 ` Al Viro 2020-05-29 0:41 ` [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() Al Viro ` (3 more replies) 2020-05-29 23:26 ` [PATCHES] uaccess misc Al Viro ` (3 subsequent siblings) 11 siblings, 4 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/i915/i915_query.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index ef25ce6e395e..ad8e55fe1e59 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -25,10 +25,6 @@ static int copy_query_item(void *query_hdr, size_t query_sz, query_sz)) return -EFAULT; - if (!access_ok(u64_to_user_ptr(query_item->data_ptr), - total_length)) - return -EFAULT; - return 0; } @@ -72,20 +68,20 @@ static int query_topology_info(struct drm_i915_private *dev_priv, topo.eu_offset = slice_length + subslice_length; topo.eu_stride = sseu->eu_stride; - if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr), + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr), &topo, sizeof(topo))) return -EFAULT; - if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)), + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo)), &sseu->slice_mask, slice_length)) return -EFAULT; - if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo) + slice_length), sseu->subslice_mask, subslice_length)) return -EFAULT; - if (__copy_to_user(u64_to_user_ptr(query_item->data_ptr + + if (copy_to_user(u64_to_user_ptr(query_item->data_ptr + sizeof(topo) + slice_length + subslice_length), sseu->eu_mask, eu_length)) @@ -131,14 +127,14 @@ query_engine_info(struct drm_i915_private *i915, info.engine.engine_instance = engine->uabi_instance; info.capabilities = engine->uabi_capabilities; - if (__copy_to_user(info_ptr, &info, sizeof(info))) + if (copy_to_user(info_ptr, &info, sizeof(info))) return -EFAULT; query.num_engines++; info_ptr++; } - if (__copy_to_user(query_ptr, &query, sizeof(query))) + if (copy_to_user(query_ptr, &query, sizeof(query))) return -EFAULT; return len; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro @ 2020-05-29 0:41 ` Al Viro 2020-05-29 0:41 ` [PATCH 3/5] i915 compat ioctl(): just use drm_ioctl_kernel() Al Viro ` (2 subsequent siblings) 3 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula From: Al Viro <viro@zeniv.linux.org.uk> ... and the rest of query_perf_config_data() to normal uaccess primitives Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/i915/i915_query.c | 46 ++++++++++++++------------------------- drivers/gpu/drm/i915/i915_reg.h | 2 +- 2 files changed, 17 insertions(+), 31 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_query.c b/drivers/gpu/drm/i915/i915_query.c index ad8e55fe1e59..e75c528ebbe0 100644 --- a/drivers/gpu/drm/i915/i915_query.c +++ b/drivers/gpu/drm/i915/i915_query.c @@ -154,10 +154,6 @@ static int can_copy_perf_config_registers_or_number(u32 user_n_regs, if (user_n_regs < kernel_n_regs) return -EINVAL; - if (!access_ok(u64_to_user_ptr(user_regs_ptr), - 2 * sizeof(u32) * kernel_n_regs)) - return -EFAULT; - return 0; } @@ -166,6 +162,7 @@ static int copy_perf_config_registers_or_number(const struct i915_oa_reg *kernel u64 user_regs_ptr, u32 *user_n_regs) { + u32 __user *p = u64_to_user_ptr(user_regs_ptr); u32 r; if (*user_n_regs == 0) { @@ -175,25 +172,19 @@ static int copy_perf_config_registers_or_number(const struct i915_oa_reg *kernel *user_n_regs = kernel_n_regs; - for (r = 0; r < kernel_n_regs; r++) { - u32 __user *user_reg_ptr = - u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2); - u32 __user *user_val_ptr = - u64_to_user_ptr(user_regs_ptr + sizeof(u32) * r * 2 + - sizeof(u32)); - int ret; - - ret = __put_user(i915_mmio_reg_offset(kernel_regs[r].addr), - user_reg_ptr); - if (ret) - return -EFAULT; + if (!user_write_access_begin(p, 2 * sizeof(u32) * kernel_n_regs)) + return -EFAULT; - ret = __put_user(kernel_regs[r].value, user_val_ptr); - if (ret) - return -EFAULT; + for (r = 0; r < kernel_n_regs; r++, p += 2) { + unsafe_put_user(i915_mmio_reg_offset(kernel_regs[r].addr), + p, Efault); + unsafe_put_user(kernel_regs[r].value, p + 1, Efault); } - + user_write_access_end(); return 0; +Efault: + user_write_access_end(); + return -EFAULT; } static int query_perf_config_data(struct drm_i915_private *i915, @@ -229,10 +220,7 @@ static int query_perf_config_data(struct drm_i915_private *i915, return -EINVAL; } - if (!access_ok(user_query_config_ptr, total_size)) - return -EFAULT; - - if (__get_user(flags, &user_query_config_ptr->flags)) + if (get_user(flags, &user_query_config_ptr->flags)) return -EFAULT; if (flags != 0) @@ -245,7 +233,7 @@ static int query_perf_config_data(struct drm_i915_private *i915, BUILD_BUG_ON(sizeof(user_query_config_ptr->uuid) >= sizeof(uuid)); memset(&uuid, 0, sizeof(uuid)); - if (__copy_from_user(uuid, user_query_config_ptr->uuid, + if (copy_from_user(uuid, user_query_config_ptr->uuid, sizeof(user_query_config_ptr->uuid))) return -EFAULT; @@ -259,7 +247,7 @@ static int query_perf_config_data(struct drm_i915_private *i915, } rcu_read_unlock(); } else { - if (__get_user(config_id, &user_query_config_ptr->config)) + if (get_user(config_id, &user_query_config_ptr->config)) return -EFAULT; oa_config = i915_perf_get_oa_config(perf, config_id); @@ -267,8 +255,7 @@ static int query_perf_config_data(struct drm_i915_private *i915, if (!oa_config) return -ENOENT; - if (__copy_from_user(&user_config, user_config_ptr, - sizeof(user_config))) { + if (copy_from_user(&user_config, user_config_ptr, sizeof(user_config))) { ret = -EFAULT; goto out; } @@ -314,8 +301,7 @@ static int query_perf_config_data(struct drm_i915_private *i915, memcpy(user_config.uuid, oa_config->uuid, sizeof(user_config.uuid)); - if (__copy_to_user(user_config_ptr, &user_config, - sizeof(user_config))) { + if (copy_to_user(user_config_ptr, &user_config, sizeof(user_config))) { ret = -EFAULT; goto out; } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 59e64acc2c56..3733b9e20976 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -186,7 +186,7 @@ typedef struct { #define INVALID_MMIO_REG _MMIO(0) -static inline u32 i915_mmio_reg_offset(i915_reg_t reg) +static __always_inline u32 i915_mmio_reg_offset(i915_reg_t reg) { return reg.reg; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 3/5] i915 compat ioctl(): just use drm_ioctl_kernel() 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro 2020-05-29 0:41 ` [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() Al Viro @ 2020-05-29 0:41 ` Al Viro 2020-05-29 0:41 ` [PATCH 4/5] i915: alloc_oa_regs(): get rid of pointless access_ok() Al Viro 2020-05-29 0:41 ` [PATCH 5/5] i915:get_engines(): " Al Viro 3 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula From: Al Viro <viro@zeniv.linux.org.uk> compat_alloc_user_space() is a bad kludge; the sooner it goes, the better... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/i915/i915_ioc32.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_ioc32.c b/drivers/gpu/drm/i915/i915_ioc32.c index 8e45ca3d2ede..55b97c3a3dde 100644 --- a/drivers/gpu/drm/i915/i915_ioc32.c +++ b/drivers/gpu/drm/i915/i915_ioc32.c @@ -47,20 +47,16 @@ static int compat_i915_getparam(struct file *file, unsigned int cmd, unsigned long arg) { struct drm_i915_getparam32 req32; - drm_i915_getparam_t __user *request; + struct drm_i915_getparam req; if (copy_from_user(&req32, (void __user *)arg, sizeof(req32))) return -EFAULT; - request = compat_alloc_user_space(sizeof(*request)); - if (!access_ok(request, sizeof(*request)) || - __put_user(req32.param, &request->param) || - __put_user((void __user *)(unsigned long)req32.value, - &request->value)) - return -EFAULT; + req.param = req32.param; + req.value = compat_ptr(req32.value); - return drm_ioctl(file, DRM_IOCTL_I915_GETPARAM, - (unsigned long)request); + return drm_ioctl_kernel(file, i915_getparam_ioctl, &req, + DRM_RENDER_ALLOW); } static drm_ioctl_compat_t *i915_compat_ioctls[] = { -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 4/5] i915: alloc_oa_regs(): get rid of pointless access_ok() 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro 2020-05-29 0:41 ` [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() Al Viro 2020-05-29 0:41 ` [PATCH 3/5] i915 compat ioctl(): just use drm_ioctl_kernel() Al Viro @ 2020-05-29 0:41 ` Al Viro 2020-05-29 0:41 ` [PATCH 5/5] i915:get_engines(): " Al Viro 3 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/i915/i915_perf.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 551be589d6f4..a52461016935 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -3949,9 +3949,6 @@ static struct i915_oa_reg *alloc_oa_regs(struct i915_perf *perf, if (!n_regs) return NULL; - if (!access_ok(regs, n_regs * sizeof(u32) * 2)) - return ERR_PTR(-EFAULT); - /* No is_valid function means we're not allowing any register to be programmed. */ GEM_BUG_ON(!is_valid); if (!is_valid) -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 5/5] i915:get_engines(): get rid of pointless access_ok() 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro ` (2 preceding siblings ...) 2020-05-29 0:41 ` [PATCH 4/5] i915: alloc_oa_regs(): get rid of pointless access_ok() Al Viro @ 2020-05-29 0:41 ` Al Viro 3 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 0:41 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Jani Nikula From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 ----- 1 file changed, 5 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_context.c b/drivers/gpu/drm/i915/gem/i915_gem_context.c index 68326ad3b2e0..55cb6dbfe61f 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_context.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_context.c @@ -1924,11 +1924,6 @@ get_engines(struct i915_gem_context *ctx, } user = u64_to_user_ptr(args->value); - if (!access_ok(user, size)) { - err = -EFAULT; - goto err_free; - } - if (put_user(0, &user->extensions)) { err = -EFAULT; goto err_free; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCHES] uaccess misc 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (7 preceding siblings ...) 2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro @ 2020-05-29 23:26 ` Al Viro 2020-05-29 23:54 ` Linus Torvalds 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro ` (2 subsequent siblings) 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 23:26 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel The stuff that doesn't fit anywhere else. Hopefully saner marshalling for weird 7-argument syscalls (pselect6()), low-hanging fruit in several binfmt, unsafe_put_user-based x86 cp_stat64(), etc. - there's really no common topic here. BTW, after that series there's no more __clear_user() callers outside of arch/* and damn few in arch/*, other than clear_user() instances themselves... Branch is uaccess.misc, based at uaccess.base. Al Viro (9): pselect6() and friends: take handling the combined 6th/7th args into helper binfmt_elf: don't bother with __{put,copy_to}_user() binfmt_elf_fdpic: don't use __... uaccess primitives binfmt_flat: don't use __put_user() x86: switch cp_stat64() to unsafe_put_user() TEST_ACCESS_OK _never_ had been checked anywhere user_regset_copyout_zero(): use clear_user() x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() arch/x86/include/asm/pgtable_32.h | 7 --- arch/x86/kernel/sys_ia32.c | 40 ++++++++------ arch/x86/kvm/hyperv.c | 2 +- fs/binfmt_elf.c | 14 ++--- fs/binfmt_elf_fdpic.c | 31 +++++++---- fs/binfmt_flat.c | 22 +++++--- fs/select.c | 112 ++++++++++++++++++++++---------------- include/linux/regset.h | 2 +- kernel/bpf/syscall.c | 25 ++------- 9 files changed, 134 insertions(+), 121 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess misc 2020-05-29 23:26 ` [PATCHES] uaccess misc Al Viro @ 2020-05-29 23:54 ` Linus Torvalds 2020-05-29 23:57 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 23:54 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Fri, May 29, 2020 at 4:26 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > The stuff that doesn't fit anywhere else. Hopefully > saner marshalling for weird 7-argument syscalls (pselect6()), That looked fine to me, btw. Looks like an improvement even outside the "avoid __get_user()" and double STAC/CLAC issue. > low-hanging fruit in several binfmt, unsafe_put_user-based > x86 cp_stat64(), etc. - there's really no common topic here. My only complaint was that kvm thing that I think should have gone even further. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess misc 2020-05-29 23:54 ` Linus Torvalds @ 2020-05-29 23:57 ` Linus Torvalds 0 siblings, 0 replies; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 23:57 UTC (permalink / raw) To: Al Viro; +Cc: Linux Kernel Mailing List, linux-fsdevel On Fri, May 29, 2020 at 4:54 PM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > My only complaint was that kvm thing that I think should have gone even further. Oh... And the cc list looks a bit odd. You cc'd fsdevel, but none of the patches were really to any filesystem code (ok, the pselect and binfmt thing is in fs/, but that's kind of incidental and more of a "we split core system calls up by area too" than any "it's filesystem code"). The kvm patch didn't have anybody from kvm-land cc'd, for example. I added some to my "this shouldn't use double underscores" reply, but.. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (8 preceding siblings ...) 2020-05-29 23:26 ` [PATCHES] uaccess misc Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() Al Viro ` (6 more replies) 2020-05-29 23:28 ` [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() Al Viro 2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro 11 siblings, 7 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> ... and use unsafe_get_user(), while we are at it. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/select.c | 112 ++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 64 insertions(+), 48 deletions(-) diff --git a/fs/select.c b/fs/select.c index 11d0285d46b7..7aef49552d4c 100644 --- a/fs/select.c +++ b/fs/select.c @@ -766,22 +766,38 @@ static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, * which has a pointer to the sigset_t itself followed by a size_t containing * the sigset size. */ +struct sigset_argpack { + sigset_t __user *p; + size_t size; +}; + +static inline int get_sigset_argpack(struct sigset_argpack *to, + struct sigset_argpack __user *from) +{ + // the path is hot enough for overhead of copy_from_user() to matter + if (from) { + if (!user_read_access_begin(from, sizeof(*from))) + return -EFAULT; + unsafe_get_user(to->p, &from->p, Efault); + unsafe_get_user(to->size, &from->size, Efault); + user_read_access_end(); + } + return 0; +Efault: + user_access_end(); + return -EFAULT; +} + SYSCALL_DEFINE6(pselect6, int, n, fd_set __user *, inp, fd_set __user *, outp, fd_set __user *, exp, struct __kernel_timespec __user *, tsp, void __user *, sig) { - size_t sigsetsize = 0; - sigset_t __user *up = NULL; - - if (sig) { - if (!access_ok(sig, sizeof(void *)+sizeof(size_t)) - || __get_user(up, (sigset_t __user * __user *)sig) - || __get_user(sigsetsize, - (size_t __user *)(sig+sizeof(void *)))) - return -EFAULT; - } + struct sigset_argpack x = {NULL, 0}; + + if (get_sigset_argpack(&x, sig)) + return -EFAULT; - return do_pselect(n, inp, outp, exp, tsp, up, sigsetsize, PT_TIMESPEC); + return do_pselect(n, inp, outp, exp, tsp, x.p, x.size, PT_TIMESPEC); } #if defined(CONFIG_COMPAT_32BIT_TIME) && !defined(CONFIG_64BIT) @@ -790,18 +806,12 @@ SYSCALL_DEFINE6(pselect6_time32, int, n, fd_set __user *, inp, fd_set __user *, fd_set __user *, exp, struct old_timespec32 __user *, tsp, void __user *, sig) { - size_t sigsetsize = 0; - sigset_t __user *up = NULL; - - if (sig) { - if (!access_ok(sig, sizeof(void *)+sizeof(size_t)) - || __get_user(up, (sigset_t __user * __user *)sig) - || __get_user(sigsetsize, - (size_t __user *)(sig+sizeof(void *)))) - return -EFAULT; - } + struct sigset_argpack x = {NULL, 0}; + + if (get_sigset_argpack(&x, sig)) + return -EFAULT; - return do_pselect(n, inp, outp, exp, tsp, up, sigsetsize, PT_OLD_TIMESPEC); + return do_pselect(n, inp, outp, exp, tsp, x.p, x.size, PT_OLD_TIMESPEC); } #endif @@ -1325,24 +1335,37 @@ static long do_compat_pselect(int n, compat_ulong_t __user *inp, return poll_select_finish(&end_time, tsp, type, ret); } +struct compat_sigset_argpack { + compat_uptr_t p; + compat_size_t size; +}; +static inline int get_compat_sigset_argpack(struct compat_sigset_argpack *to, + struct compat_sigset_argpack __user *from) +{ + if (from) { + if (!user_read_access_begin(from, sizeof(*from))) + return -EFAULT; + unsafe_get_user(to->p, &from->p, Efault); + unsafe_get_user(to->size, &from->size, Efault); + user_read_access_end(); + } + return 0; +Efault: + user_access_end(); + return -EFAULT; +} + COMPAT_SYSCALL_DEFINE6(pselect6_time64, int, n, compat_ulong_t __user *, inp, compat_ulong_t __user *, outp, compat_ulong_t __user *, exp, struct __kernel_timespec __user *, tsp, void __user *, sig) { - compat_size_t sigsetsize = 0; - compat_uptr_t up = 0; - - if (sig) { - if (!access_ok(sig, - sizeof(compat_uptr_t)+sizeof(compat_size_t)) || - __get_user(up, (compat_uptr_t __user *)sig) || - __get_user(sigsetsize, - (compat_size_t __user *)(sig+sizeof(up)))) - return -EFAULT; - } + struct compat_sigset_argpack x = {0, 0}; + + if (get_compat_sigset_argpack(&x, sig)) + return -EFAULT; - return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(up), - sigsetsize, PT_TIMESPEC); + return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(x.p), + x.size, PT_TIMESPEC); } #if defined(CONFIG_COMPAT_32BIT_TIME) @@ -1351,20 +1374,13 @@ COMPAT_SYSCALL_DEFINE6(pselect6_time32, int, n, compat_ulong_t __user *, inp, compat_ulong_t __user *, outp, compat_ulong_t __user *, exp, struct old_timespec32 __user *, tsp, void __user *, sig) { - compat_size_t sigsetsize = 0; - compat_uptr_t up = 0; - - if (sig) { - if (!access_ok(sig, - sizeof(compat_uptr_t)+sizeof(compat_size_t)) || - __get_user(up, (compat_uptr_t __user *)sig) || - __get_user(sigsetsize, - (compat_size_t __user *)(sig+sizeof(up)))) - return -EFAULT; - } + struct compat_sigset_argpack x = {0, 0}; + + if (get_compat_sigset_argpack(&x, sig)) + return -EFAULT; - return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(up), - sigsetsize, PT_OLD_TIMESPEC); + return do_compat_pselect(n, inp, outp, exp, tsp, compat_ptr(x.p), + x.size, PT_OLD_TIMESPEC); } #endif -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 3/9] binfmt_elf_fdpic: don't use __... uaccess primitives Al Viro ` (5 subsequent siblings) 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/binfmt_elf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 13f25e241ac4..782d218c3bb9 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -202,7 +202,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, size_t len = strlen(k_platform) + 1; u_platform = (elf_addr_t __user *)STACK_ALLOC(p, len); - if (__copy_to_user(u_platform, k_platform, len)) + if (copy_to_user(u_platform, k_platform, len)) return -EFAULT; } @@ -215,7 +215,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, size_t len = strlen(k_base_platform) + 1; u_base_platform = (elf_addr_t __user *)STACK_ALLOC(p, len); - if (__copy_to_user(u_base_platform, k_base_platform, len)) + if (copy_to_user(u_base_platform, k_base_platform, len)) return -EFAULT; } @@ -225,7 +225,7 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, get_random_bytes(k_rand_bytes, sizeof(k_rand_bytes)); u_rand_bytes = (elf_addr_t __user *) STACK_ALLOC(p, sizeof(k_rand_bytes)); - if (__copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes))) + if (copy_to_user(u_rand_bytes, k_rand_bytes, sizeof(k_rand_bytes))) return -EFAULT; /* Create the ELF interpreter info */ @@ -308,14 +308,14 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, return -EFAULT; /* Now, let's put argc (and argv, envp if appropriate) on the stack */ - if (__put_user(argc, sp++)) + if (put_user(argc, sp++)) return -EFAULT; /* Populate list of argv pointers back to argv strings. */ p = mm->arg_end = mm->arg_start; while (argc-- > 0) { size_t len; - if (__put_user((elf_addr_t)p, sp++)) + if (put_user((elf_addr_t)p, sp++)) return -EFAULT; len = strnlen_user((void __user *)p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) @@ -330,14 +330,14 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec, mm->env_end = mm->env_start = p; while (envc-- > 0) { size_t len; - if (__put_user((elf_addr_t)p, sp++)) + if (put_user((elf_addr_t)p, sp++)) return -EFAULT; len = strnlen_user((void __user *)p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; p += len; } - if (__put_user(0, sp++)) + if (put_user(0, sp++)) return -EFAULT; mm->env_end = p; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 3/9] binfmt_elf_fdpic: don't use __... uaccess primitives 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro 2020-05-29 23:27 ` [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 4/9] binfmt_flat: don't use __put_user() Al Viro ` (4 subsequent siblings) 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/binfmt_elf_fdpic.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 240f66663543..ab6fb117b33c 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -537,7 +537,7 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, platform_len = strlen(k_platform) + 1; sp -= platform_len; u_platform = (char __user *) sp; - if (__copy_to_user(u_platform, k_platform, platform_len) != 0) + if (copy_to_user(u_platform, k_platform, platform_len) != 0) return -EFAULT; } @@ -552,7 +552,7 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, platform_len = strlen(k_base_platform) + 1; sp -= platform_len; u_base_platform = (char __user *) sp; - if (__copy_to_user(u_base_platform, k_base_platform, platform_len) != 0) + if (copy_to_user(u_base_platform, k_base_platform, platform_len) != 0) return -EFAULT; } @@ -604,11 +604,13 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, /* put the ELF interpreter info on the stack */ #define NEW_AUX_ENT(id, val) \ do { \ - struct { unsigned long _id, _val; } __user *ent; \ + struct { unsigned long _id, _val; } __user *ent, v; \ \ ent = (void __user *) csp; \ - __put_user((id), &ent[nr]._id); \ - __put_user((val), &ent[nr]._val); \ + v._id = (id); \ + v._val = (val); \ + if (copy_to_user(ent + nr, &v)) \ + return -EFAULT; \ nr++; \ } while (0) @@ -675,7 +677,8 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, /* stack argc */ csp -= sizeof(unsigned long); - __put_user(bprm->argc, (unsigned long __user *) csp); + if (put_user(bprm->argc, (unsigned long __user *) csp)) + return -EFAULT; BUG_ON(csp != sp); @@ -689,25 +692,29 @@ static int create_elf_fdpic_tables(struct linux_binprm *bprm, p = (char __user *) current->mm->arg_start; for (loop = bprm->argc; loop > 0; loop--) { - __put_user((elf_caddr_t) p, argv++); + if (put_user((elf_caddr_t) p, argv++)) + return -EFAULT; len = strnlen_user(p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; p += len; } - __put_user(NULL, argv); + if (put_user(NULL, argv)) + return -EFAULT; current->mm->arg_end = (unsigned long) p; /* fill in the envv[] array */ current->mm->env_start = (unsigned long) p; for (loop = bprm->envc; loop > 0; loop--) { - __put_user((elf_caddr_t)(unsigned long) p, envp++); + if (put_user((elf_caddr_t)(unsigned long) p, envp++)) + return -EFAULT; len = strnlen_user(p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; p += len; } - __put_user(NULL, envp); + if (put_user(NULL, envp)) + return -EFAULT; current->mm->env_end = (unsigned long) p; mm->start_stack = (unsigned long) sp; @@ -849,8 +856,8 @@ static int elf_fdpic_map_file(struct elf_fdpic_params *params, tmp = phdr->p_memsz / sizeof(Elf32_Dyn); dyn = (Elf32_Dyn __user *)params->dynamic_addr; - __get_user(d_tag, &dyn[tmp - 1].d_tag); - if (d_tag != 0) + if (get_user(d_tag, &dyn[tmp - 1].d_tag) || + d_tag != 0) goto dynamic_error; break; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 4/9] binfmt_flat: don't use __put_user() 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro 2020-05-29 23:27 ` [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() Al Viro 2020-05-29 23:27 ` [PATCH 3/9] binfmt_elf_fdpic: don't use __... uaccess primitives Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 5/9] x86: switch cp_stat64() to unsafe_put_user() Al Viro ` (3 subsequent siblings) 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> ... and check the return value Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- fs/binfmt_flat.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-) diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 831a2b25ba79..7b663ed5247b 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -138,35 +138,40 @@ static int create_flat_tables(struct linux_binprm *bprm, unsigned long arg_start current->mm->start_stack = (unsigned long)sp & -FLAT_STACK_ALIGN; sp = (unsigned long __user *)current->mm->start_stack; - __put_user(bprm->argc, sp++); + if (put_user(bprm->argc, sp++)) + return -EFAULT; if (IS_ENABLED(CONFIG_BINFMT_FLAT_ARGVP_ENVP_ON_STACK)) { unsigned long argv, envp; argv = (unsigned long)(sp + 2); envp = (unsigned long)(sp + 2 + bprm->argc + 1); - __put_user(argv, sp++); - __put_user(envp, sp++); + if (put_user(argv, sp++) || put_user(envp, sp++)) + return -EFAULT; } current->mm->arg_start = (unsigned long)p; for (i = bprm->argc; i > 0; i--) { - __put_user((unsigned long)p, sp++); + if (put_user((unsigned long)p, sp++)) + return -EFAULT; len = strnlen_user(p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; p += len; } - __put_user(0, sp++); + if (put_user(0, sp++)) + return -EFAULT; current->mm->arg_end = (unsigned long)p; current->mm->env_start = (unsigned long) p; for (i = bprm->envc; i > 0; i--) { - __put_user((unsigned long)p, sp++); + if (put_user((unsigned long)p, sp++)) + return -EFAULT; len = strnlen_user(p, MAX_ARG_STRLEN); if (!len || len > MAX_ARG_STRLEN) return -EINVAL; p += len; } - __put_user(0, sp++); + if (put_user(0, sp++)) + return -EFAULT; current->mm->env_end = (unsigned long)p; return 0; @@ -998,7 +1003,8 @@ static int load_flat_binary(struct linux_binprm *bprm) unsigned long __user *sp; current->mm->start_stack -= sizeof(unsigned long); sp = (unsigned long __user *)current->mm->start_stack; - __put_user(start_addr, sp); + if (put_user(start_addr, sp)) + return -EFAULT; start_addr = libinfo.lib_list[i].entry; } } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 5/9] x86: switch cp_stat64() to unsafe_put_user() 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro ` (2 preceding siblings ...) 2020-05-29 23:27 ` [PATCH 4/9] binfmt_flat: don't use __put_user() Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 6/9] TEST_ACCESS_OK _never_ had been checked anywhere Al Viro ` (2 subsequent siblings) 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- arch/x86/kernel/sys_ia32.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/arch/x86/kernel/sys_ia32.c b/arch/x86/kernel/sys_ia32.c index ab03fede1422..f8d65c99feb8 100644 --- a/arch/x86/kernel/sys_ia32.c +++ b/arch/x86/kernel/sys_ia32.c @@ -135,26 +135,30 @@ static int cp_stat64(struct stat64 __user *ubuf, struct kstat *stat) typeof(ubuf->st_gid) gid = 0; SET_UID(uid, from_kuid_munged(current_user_ns(), stat->uid)); SET_GID(gid, from_kgid_munged(current_user_ns(), stat->gid)); - if (!access_ok(ubuf, sizeof(struct stat64)) || - __put_user(huge_encode_dev(stat->dev), &ubuf->st_dev) || - __put_user(stat->ino, &ubuf->__st_ino) || - __put_user(stat->ino, &ubuf->st_ino) || - __put_user(stat->mode, &ubuf->st_mode) || - __put_user(stat->nlink, &ubuf->st_nlink) || - __put_user(uid, &ubuf->st_uid) || - __put_user(gid, &ubuf->st_gid) || - __put_user(huge_encode_dev(stat->rdev), &ubuf->st_rdev) || - __put_user(stat->size, &ubuf->st_size) || - __put_user(stat->atime.tv_sec, &ubuf->st_atime) || - __put_user(stat->atime.tv_nsec, &ubuf->st_atime_nsec) || - __put_user(stat->mtime.tv_sec, &ubuf->st_mtime) || - __put_user(stat->mtime.tv_nsec, &ubuf->st_mtime_nsec) || - __put_user(stat->ctime.tv_sec, &ubuf->st_ctime) || - __put_user(stat->ctime.tv_nsec, &ubuf->st_ctime_nsec) || - __put_user(stat->blksize, &ubuf->st_blksize) || - __put_user(stat->blocks, &ubuf->st_blocks)) + if (!user_write_access_begin(ubuf, sizeof(struct stat64))) return -EFAULT; + unsafe_put_user(huge_encode_dev(stat->dev), &ubuf->st_dev, Efault); + unsafe_put_user(stat->ino, &ubuf->__st_ino, Efault); + unsafe_put_user(stat->ino, &ubuf->st_ino, Efault); + unsafe_put_user(stat->mode, &ubuf->st_mode, Efault); + unsafe_put_user(stat->nlink, &ubuf->st_nlink, Efault); + unsafe_put_user(uid, &ubuf->st_uid, Efault); + unsafe_put_user(gid, &ubuf->st_gid, Efault); + unsafe_put_user(huge_encode_dev(stat->rdev), &ubuf->st_rdev, Efault); + unsafe_put_user(stat->size, &ubuf->st_size, Efault); + unsafe_put_user(stat->atime.tv_sec, &ubuf->st_atime, Efault); + unsafe_put_user(stat->atime.tv_nsec, &ubuf->st_atime_nsec, Efault); + unsafe_put_user(stat->mtime.tv_sec, &ubuf->st_mtime, Efault); + unsafe_put_user(stat->mtime.tv_nsec, &ubuf->st_mtime_nsec, Efault); + unsafe_put_user(stat->ctime.tv_sec, &ubuf->st_ctime, Efault); + unsafe_put_user(stat->ctime.tv_nsec, &ubuf->st_ctime_nsec, Efault); + unsafe_put_user(stat->blksize, &ubuf->st_blksize, Efault); + unsafe_put_user(stat->blocks, &ubuf->st_blocks, Efault); + user_access_end(); return 0; +Efault: + user_write_access_end(); + return -EFAULT; } COMPAT_SYSCALL_DEFINE2(ia32_stat64, const char __user *, filename, -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 6/9] TEST_ACCESS_OK _never_ had been checked anywhere 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro ` (3 preceding siblings ...) 2020-05-29 23:27 ` [PATCH 5/9] x86: switch cp_stat64() to unsafe_put_user() Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 7/9] user_regset_copyout_zero(): use clear_user() Al Viro 2020-05-29 23:27 ` [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() Al Viro 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Once upon a time the predecessor of that thing (TEST_VERIFY_AREA) used to be. However, that had been gone for years now (and the patch that introduced TEST_ACCESS_OK has not touched any ifdefs - they got gradually removed later). Just bury it... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- arch/x86/include/asm/pgtable_32.h | 7 ------- 1 file changed, 7 deletions(-) diff --git a/arch/x86/include/asm/pgtable_32.h b/arch/x86/include/asm/pgtable_32.h index 0dca7f7aeff2..fb10f2f8f4f0 100644 --- a/arch/x86/include/asm/pgtable_32.h +++ b/arch/x86/include/asm/pgtable_32.h @@ -32,13 +32,6 @@ extern pmd_t initial_pg_pmd[]; void paging_init(void); void sync_initial_page_table(void); -/* - * Define this if things work differently on an i386 and an i486: - * it will (on an i486) warn about kernel memory accesses that are - * done without a 'access_ok( ..)' - */ -#undef TEST_ACCESS_OK - #ifdef CONFIG_X86_PAE # include <asm/pgtable-3level.h> #else -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 7/9] user_regset_copyout_zero(): use clear_user() 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro ` (4 preceding siblings ...) 2020-05-29 23:27 ` [PATCH 6/9] TEST_ACCESS_OK _never_ had been checked anywhere Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:27 ` [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() Al Viro 6 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> that's the only caller of __clear_user() in generic code, and it's not hot enough to bother with skipping access_ok(). Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- include/linux/regset.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/linux/regset.h b/include/linux/regset.h index bf0243779738..46d6ae68c455 100644 --- a/include/linux/regset.h +++ b/include/linux/regset.h @@ -320,7 +320,7 @@ static inline int user_regset_copyout_zero(unsigned int *pos, if (*kbuf) { memset(*kbuf, 0, copy); *kbuf += copy; - } else if (__clear_user(*ubuf, copy)) + } else if (clear_user(*ubuf, copy)) return -EFAULT; else *ubuf += copy; -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro ` (5 preceding siblings ...) 2020-05-29 23:27 ` [PATCH 7/9] user_regset_copyout_zero(): use clear_user() Al Viro @ 2020-05-29 23:27 ` Al Viro 2020-05-29 23:52 ` Linus Torvalds 6 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 23:27 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- arch/x86/kvm/hyperv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c index bcefa9d4e57e..b85b211d4676 100644 --- a/arch/x86/kvm/hyperv.c +++ b/arch/x86/kvm/hyperv.c @@ -1129,7 +1129,7 @@ static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host) * only, there can be valuable data in the rest which needs * to be preserved e.g. on migration. */ - if (__clear_user((void __user *)addr, sizeof(u32))) + if (__put_user(0, (u32 __user *)addr)) return 1; hv_vcpu->hv_vapic = data; kvm_vcpu_mark_page_dirty(vcpu, gfn); -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-29 23:27 ` [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() Al Viro @ 2020-05-29 23:52 ` Linus Torvalds 2020-05-30 14:31 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-29 23:52 UTC (permalink / raw) To: Al Viro, Paolo Bonzini; +Cc: Linux Kernel Mailing List, linux-fsdevel, KVM list On Fri, May 29, 2020 at 4:27 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > a/arch/x86/kvm/hyperv.c > - if (__clear_user((void __user *)addr, sizeof(u32))) > + if (__put_user(0, (u32 __user *)addr)) I'm not doubting that this is a correct transformation and an improvement, but why is it using that double-underscore version in the first place? There's a __copy_to_user() in kvm_hv_set_msr_pw() in addition to this one in kvm_hv_set_msr(). Both go back to 2011 and commit 8b0cedff040b ("KVM: use __copy_to_user/__clear_user to write guest page") and both look purely like "pointlessly avoid the access_ok". All these KVM "optimizations" seem entirely pointless, since access_ok() isn't the problem. And the address _claims_ to be verified, but I'm not seeing it. There is not a single 'access_ok()' anywhere in arch/x86/kvm/ that I can see. It looks like the argument for the address being validated is that it comes from "gfn_to_hva()", which should only return host-virtual-addresses. That may be true. But "should" is not "does", and honestly, the cost of gfn_to_hva() is high enough that then using that as an argument for removing "access_ok()" smells. So I would suggest just removing all these completely bogus double-underscore versions. It's pointless, it's wrong, and it's unsafe. This isn't even some critical path, but even if it was, that user address check isn't where the problems are. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-29 23:52 ` Linus Torvalds @ 2020-05-30 14:31 ` Al Viro 2020-05-30 14:52 ` Al Viro 2020-05-30 16:20 ` Paolo Bonzini 0 siblings, 2 replies; 94+ messages in thread From: Al Viro @ 2020-05-30 14:31 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Fri, May 29, 2020 at 04:52:59PM -0700, Linus Torvalds wrote: > On Fri, May 29, 2020 at 4:27 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > a/arch/x86/kvm/hyperv.c > > - if (__clear_user((void __user *)addr, sizeof(u32))) > > + if (__put_user(0, (u32 __user *)addr)) > > I'm not doubting that this is a correct transformation and an > improvement, but why is it using that double-underscore version in the > first place? > > There's a __copy_to_user() in kvm_hv_set_msr_pw() in addition to this > one in kvm_hv_set_msr(). Both go back to 2011 and commit 8b0cedff040b > ("KVM: use __copy_to_user/__clear_user to write guest page") and both > look purely like "pointlessly avoid the access_ok". > > All these KVM "optimizations" seem entirely pointless, since > access_ok() isn't the problem. And the address _claims_ to be > verified, but I'm not seeing it. There is not a single 'access_ok()' > anywhere in arch/x86/kvm/ that I can see. > > It looks like the argument for the address being validated is that it > comes from "gfn_to_hva()", which should only return > host-virtual-addresses. That may be true. > > But "should" is not "does", and honestly, the cost of gfn_to_hva() is > high enough that then using that as an argument for removing > "access_ok()" smells. > > So I would suggest just removing all these completely bogus > double-underscore versions. It's pointless, it's wrong, and it's > unsafe. It's a bit trickier than that, but I want to deal with that at the same time as the rest of kvm/vhost stuff. So for this series I just went for minimal change. There's quite a pile of vhost and kvm stuff, but it's not ready yet - wait for the next cycle. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 14:31 ` Al Viro @ 2020-05-30 14:52 ` Al Viro 2020-05-30 16:20 ` Paolo Bonzini 1 sibling, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-30 14:52 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 03:31:47PM +0100, Al Viro wrote: > It's a bit trickier than that, but I want to deal with that at the same > time as the rest of kvm/vhost stuff. So for this series I just went > for minimal change. There's quite a pile of vhost and kvm stuff, > but it's not ready yet - wait for the next cycle. BTW, regarding uaccess plans for the next cycle: * regset mess (at least the ->get() side) * killing more compat_alloc_user_space() call sites (_maybe_ all of it, if we are lucky enough; v4l2 is a bitch in that respect, but I've some ideas on how to deal with that - need to discuss with mchehab) * sorting the remaining (harder) parts of i915 out * kvm/vhost * fault_in_pages_...() series That should get rid of almost all __... ones outside of arch/*; might actually kill copy_in_user() off as well. * finally lifting stac/clac out of raw_copy_{to,from}_user(). ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 14:31 ` Al Viro 2020-05-30 14:52 ` Al Viro @ 2020-05-30 16:20 ` Paolo Bonzini 2020-05-30 17:57 ` Linus Torvalds 1 sibling, 1 reply; 94+ messages in thread From: Paolo Bonzini @ 2020-05-30 16:20 UTC (permalink / raw) To: Al Viro, Linus Torvalds Cc: Linux Kernel Mailing List, linux-fsdevel, KVM list > On Fri, May 29, 2020 at 04:52:59PM -0700, Linus Torvalds wrote: >> It looks like the argument for the address being validated is that it >> comes from "gfn_to_hva()", which should only return >> host-virtual-addresses. That may be true. Yes, the access_ok is done in __kvm_set_memory_region and gfn_to_hva() returns a page-aligned address so it's obviously ok for a u32. But I have no objections to removing the __ because if a read or write is in the hot path it will use kvm_write_guest_cached and similar. Paolo >> But "should" is not "does", and honestly, the cost of gfn_to_hva() is >> high enough that then using that as an argument for removing >> "access_ok()" smells. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 16:20 ` Paolo Bonzini @ 2020-05-30 17:57 ` Linus Torvalds 2020-05-30 18:38 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-30 17:57 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Al Viro, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 9:20 AM Paolo Bonzini <pbonzini@redhat.com> wrote: > > Yes, the access_ok is done in __kvm_set_memory_region and gfn_to_hva() > returns a page-aligned address so it's obviously ok for a u32. It's not that it's "obviously ok for an u32". It is _not_ obviously ok for a user address. There's actually no access_ok() done in the lookup path at all, and what gfn_to_hva() actually ends up doing in the end is __gfn_to_hva_memslot(), which has zero overflow protection at all, and just does slot->userspace_addr + (gfn - slot->base_gfn) * PAGE_SIZE; without us having _ever_ checked that 'gfn' parameter. Yes, at some point in the very very distant past, __kvm_set_memory_region() has validated mem->{userspace_addr,memory_size}. But even that validation is actually very questionable, since it's not even done for all of the memory slots, only the "user" ones. So if at any point we have a non-user slot, of it at any point the gfn thing was mis-calculated and {over,under}flows, there are no protections what-so-ever. In other words, it really looks like kvm is entirely dependent on magic and luck and a desperate hope that there are no other bugs to keep the end result as a user address. Because if _any_ bug or oversight in that kvm_memory_slot handling ever happens, you end up with random crap. So no. I disagree. There is absolutely nothing "obviously ok" about any of that kvm code. Quite the reverse. I'd argue that it's very much obviously *NOT* ok, even while it might just happen to work. That double underscore needs to go away. It's either actively buggy right now and I see no proof it isn't, or it's a bug just waiting to happen in the future. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 17:57 ` Linus Torvalds @ 2020-05-30 18:38 ` Al Viro 2020-05-30 18:52 ` Linus Torvalds 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-30 18:38 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 10:57:24AM -0700, Linus Torvalds wrote: > So no. I disagree. There is absolutely nothing "obviously ok" about > any of that kvm code. Quite the reverse. > > I'd argue that it's very much obviously *NOT* ok, even while it might > just happen to work. Actually, it's somewhat less brittle than you think (on non-mips, at least) and not due to those long-ago access_ok(). > That double underscore needs to go away. It's either actively buggy > right now and I see no proof it isn't, or it's a bug just waiting to > happen in the future. FWIW, the kvm side of things (vhost is yet another pile of fun) is [x86] kvm_hv_set_msr_pw(): arch/x86/kvm/hyperv.c:1027: if (__copy_to_user((void __user *)addr, instructions, 4)) HV_X64_MSR_HYPERCALL arch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32))) HV_X64_MSR_VP_ASSIST_PAGE in both cases addr comes from gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT; addr = kvm_vcpu_gfn_to_hva(vcpu, gfn); if (kvm_is_error_hva(addr)) return 1; [x86] FNAME(walk_addr_generic), very hot: arch/x86/kvm/mmu/paging_tmpl.h:403: if (unlikely(__get_user(pte, ptep_user))) index = PT_INDEX(addr, walker->level); ... offset = index * sizeof(pt_element_t); ... host_addr = kvm_vcpu_gfn_to_hva_prot(vcpu, real_gfn, &walker->pte_writable[walker->level - 1]); if (unlikely(kvm_is_error_hva(host_addr))) goto error; ptep_user = (pt_element_t __user *)((void *)host_addr + offset); __kvm_read_guest_page(): virt/kvm/kvm_main.c:2252: r = __copy_from_user(data, (void __user *)addr + offset, len); addr = gfn_to_hva_memslot_prot(slot, gfn, NULL); if (kvm_is_error_hva(addr)) return -EFAULT; __kvm_read_guest_atomic(): virt/kvm/kvm_main.c:2326: r = __copy_from_user_inatomic(data, (void __user *)addr + offset, len); addr = gfn_to_hva_memslot_prot(slot, gfn, NULL); if (kvm_is_error_hva(addr)) return -EFAULT; __kvm_write_guest_page(): virt/kvm/kvm_main.c:2353: r = __copy_to_user((void __user *)addr + offset, data, len); addr = gfn_to_hva_memslot(memslot, gfn); if (kvm_is_error_hva(addr)) return -EFAULT; kvm_write_guest_offset_cached(): virt/kvm/kvm_main.c:2490: r = __copy_to_user((void __user *)ghc->hva + offset, data, len); if (kvm_is_error_hva(ghc->hva)) return -EFAULT; kvm_read_guest_cached(): virt/kvm/kvm_main.c:2525: r = __copy_from_user(data, (void __user *)ghc->hva, len); if (kvm_is_error_hva(ghc->hva)) return -EFAULT; default kvm_is_error_hva() is addr >= PAGE_OFFSET; however, on mips and s390 it's IS_ERR_VALUE(). Sure, we can use non-__ variants, but is access_ok() the right primitive here? We want userland memory, set_fs() be damned. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 18:38 ` Al Viro @ 2020-05-30 18:52 ` Linus Torvalds 2020-05-30 19:14 ` Al Viro 2020-05-30 19:19 ` Al Viro 0 siblings, 2 replies; 94+ messages in thread From: Linus Torvalds @ 2020-05-30 18:52 UTC (permalink / raw) To: Al Viro; +Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 11:39 AM Al Viro <viro@zeniv.linux.org.uk> wrote: > > Actually, it's somewhat less brittle than you think (on non-mips, at least) > and not due to those long-ago access_ok(). It really isn't. Your very first statement shows how broken it is: > FWIW, the kvm side of things (vhost is yet another pile of fun) is > > [x86] kvm_hv_set_msr_pw(): > arch/x86/kvm/hyperv.c:1027: if (__copy_to_user((void __user *)addr, instructions, 4)) > HV_X64_MSR_HYPERCALL > arch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32))) > HV_X64_MSR_VP_ASSIST_PAGE > in both cases addr comes from > gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT; > addr = kvm_vcpu_gfn_to_hva(vcpu, gfn); > if (kvm_is_error_hva(addr)) > return 1; Just look at that. You have _zero_ indication that 'adds" is a user space address. It could be a kernel address. That kvm_vcpu_gfn_to_hva() function is a complicated mess that first looks for the right 'memslot', and basically uses a search with a default slot to try to figure it out. It doesn't even use locking for any of it, but assumes the arrays are stable, and that it can use atomics to reliably read and set the last successfully found slot. And none of that code verifies that the end result is a user address. It _literally_ all depends on this optimistically lock-free code being bug-free, and never using a slot that isn't a user slot. And as mentioned, there _are_ non-user memslots. It's fragile as hell. And it's all completely and utterly pointless. ALL of the above is incredibly much more expensive than just checking the damn address range. So the optimization is completely bogus to begin with, and all it results in is that any bug in this _incredibly_ subtle code will be a security proiblem. And I don't understand why you mention set_fs() vs access_ok(). None of this code has anything that messes with set_fs(). The access_ok() is garbage and shouldn't exist, and those user accesses should all use the checking versions and the double underscores are wrong. I have no idea why you think the double underscores could _possibly_ be worth defending. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 18:52 ` Linus Torvalds @ 2020-05-30 19:14 ` Al Viro 2020-05-30 19:20 ` Linus Torvalds 2020-05-30 19:19 ` Al Viro 1 sibling, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-30 19:14 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 11:52:44AM -0700, Linus Torvalds wrote: > > It really isn't. > > Your very first statement shows how broken it is: > > > FWIW, the kvm side of things (vhost is yet another pile of fun) is > > > > [x86] kvm_hv_set_msr_pw(): > > arch/x86/kvm/hyperv.c:1027: if (__copy_to_user((void __user *)addr, instructions, 4)) > > HV_X64_MSR_HYPERCALL > > arch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32))) > > HV_X64_MSR_VP_ASSIST_PAGE > > in both cases addr comes from > > gfn = data >> HV_X64_MSR_VP_ASSIST_PAGE_ADDRESS_SHIFT; > > addr = kvm_vcpu_gfn_to_hva(vcpu, gfn); > > if (kvm_is_error_hva(addr)) > > return 1; > > Just look at that. You have _zero_ indication that 'adds" is a user > space address. It could be a kernel address. > > That kvm_vcpu_gfn_to_hva() function is a complicated mess that first > looks for the right 'memslot', and basically uses a search with a > default slot to try to figure it out. It doesn't even use locking for > any of it, but assumes the arrays are stable, and that it can use > atomics to reliably read and set the last successfully found slot. > > And none of that code verifies that the end result is a user address. kvm_is_error_hva() is return addr >= PAGE_OFFSET; ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 19:14 ` Al Viro @ 2020-05-30 19:20 ` Linus Torvalds 2020-05-30 19:42 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Linus Torvalds @ 2020-05-30 19:20 UTC (permalink / raw) To: Al Viro; +Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 12:14 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > And none of that code verifies that the end result is a user address. > > kvm_is_error_hva() is > return addr >= PAGE_OFFSET; Ahh, that's what I missed. It won't work on other architectures, but within x86 it's fine. Linus ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 19:20 ` Linus Torvalds @ 2020-05-30 19:42 ` Al Viro 2020-05-30 20:43 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-30 19:42 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 12:20:54PM -0700, Linus Torvalds wrote: > On Sat, May 30, 2020 at 12:14 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > And none of that code verifies that the end result is a user address. > > > > kvm_is_error_hva() is > > return addr >= PAGE_OFFSET; > > Ahh, that's what I missed. It won't work on other architectures, but > within x86 it's fine. FWIW, we use virt/kvm on x86, powerpc, mips, s390 and arm64. For x86 and powerpc the check is, AFAICS, OK (ppc kernel might start higher than PAGE_OFFSET, but not lower than it). For arm64... not sure - I'm not familiar with the virtual address space layout we use there. mips does *NOT* get that protection at all - there kvm_is_error_hva() is IS_ERR_VALUE() (thus the "at least on non-mips" upthread). And for s390 it's also IS_ERR_VALUE(), but that's an separate can of worms - there access_ok() is constant true; if we ever hit any of that code in virt/kvm while under KERNEL_DS, we are well and truly fucked there. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 19:42 ` Al Viro @ 2020-05-30 20:43 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-30 20:43 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 08:42:32PM +0100, Al Viro wrote: > On Sat, May 30, 2020 at 12:20:54PM -0700, Linus Torvalds wrote: > > On Sat, May 30, 2020 at 12:14 PM Al Viro <viro@zeniv.linux.org.uk> wrote: > > > > > > > And none of that code verifies that the end result is a user address. > > > > > > kvm_is_error_hva() is > > > return addr >= PAGE_OFFSET; > > > > Ahh, that's what I missed. It won't work on other architectures, but > > within x86 it's fine. > > FWIW, we use virt/kvm on x86, powerpc, mips, s390 and arm64. > > For x86 and powerpc the check is, AFAICS, OK (ppc kernel might start > higher than PAGE_OFFSET, but not lower than it). For arm64... not > sure - I'm not familiar with the virtual address space layout we use > there. mips does *NOT* get that protection at all - there kvm_is_error_hva() > is IS_ERR_VALUE() (thus the "at least on non-mips" upthread). And > for s390 it's also IS_ERR_VALUE(), but that's an separate can of worms - > there access_ok() is constant true; if we ever hit any of that code in > virt/kvm while under KERNEL_DS, we are well and truly fucked there. Anyway, I really think it's too big to handle this cycle, what with the amount of other stuff already in queue. If anything, that __put_user() is a useful marker of the things that will need attention. That's arch/x86 and the test excluding the kernel space is just upstream of that call, so IMO that's robust enough for now. Crossing the limit just into the beginning of kernel space is theoretically possible, but that would depend upon slot->userspace_addr not being page-aligned (and would attempt to zero up to 3 bytes past the PAGE_OFFSET in any case). If we get memory corruption going on, we have much worse problems than that. And it would have to be memory corruption - ->userspace_addr is assign-once, there's only one place doing the assignments and alignment check is shortly upstream of it, so all instances must have that field page-aligned all the time. We'll need to sort the kvm-related issues out, but let's leave it for the next cycle. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 18:52 ` Linus Torvalds 2020-05-30 19:14 ` Al Viro @ 2020-05-30 19:19 ` Al Viro 2020-05-30 19:27 ` Al Viro 1 sibling, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-30 19:19 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 11:52:44AM -0700, Linus Torvalds wrote: > And I don't understand why you mention set_fs() vs access_ok(). None > of this code has anything that messes with set_fs(). The access_ok() > is garbage and shouldn't exist, and those user accesses should all use > the checking versions and the double underscores are wrong. > > I have no idea why you think the double underscores could _possibly_ > be worth defending. I do not. What I'm saying is that this just might be a beast different from *both* __... and the normal ones. I'm not saying that this __put_user() (or __clear_user(), etc.) is the right primitive here. If anything, it's closer to the situation for (x86) copy_stack_trace(). ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() 2020-05-30 19:19 ` Al Viro @ 2020-05-30 19:27 ` Al Viro 0 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-30 19:27 UTC (permalink / raw) To: Linus Torvalds Cc: Paolo Bonzini, Linux Kernel Mailing List, linux-fsdevel, KVM list On Sat, May 30, 2020 at 08:19:40PM +0100, Al Viro wrote: > On Sat, May 30, 2020 at 11:52:44AM -0700, Linus Torvalds wrote: > > > And I don't understand why you mention set_fs() vs access_ok(). None > > of this code has anything that messes with set_fs(). The access_ok() > > is garbage and shouldn't exist, and those user accesses should all use > > the checking versions and the double underscores are wrong. > > > > I have no idea why you think the double underscores could _possibly_ > > be worth defending. > > I do not. What I'm saying is that this just might be a beast different > from *both* __... and the normal ones. I'm not saying that this > __put_user() (or __clear_user(), etc.) is the right primitive here. > If anything, it's closer to the situation for (x86) copy_stack_trace(). ... and no, I'm not saying that copy_stack_trace() should stay with __get_user() either. It feels like we are lacking primitives needed to express that cleanly and copy_stack_trace() currently cobbles something up out of what we have. Which works for arch-specific code, but yes, that kind of thing is brittle for arch-independent places like virt/kvm; I wonder if e.g. s390 is really OK there. ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (9 preceding siblings ...) 2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro @ 2020-05-29 23:28 ` Al Viro 2020-05-31 16:35 ` Alexei Starovoitov 2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro 11 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-05-29 23:28 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Alexei Starovoitov, bpf From: Al Viro <viro@zeniv.linux.org.uk> ... rather than open-coding it, and badly, at that. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- kernel/bpf/syscall.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c index 64783da34202..41ba746ecbc2 100644 --- a/kernel/bpf/syscall.c +++ b/kernel/bpf/syscall.c @@ -67,32 +67,19 @@ int bpf_check_uarg_tail_zero(void __user *uaddr, size_t expected_size, size_t actual_size) { - unsigned char __user *addr; - unsigned char __user *end; - unsigned char val; - int err; + unsigned char __user *addr = uaddr + expected_size; + int res; if (unlikely(actual_size > PAGE_SIZE)) /* silly large */ return -E2BIG; - if (unlikely(!access_ok(uaddr, actual_size))) - return -EFAULT; - if (actual_size <= expected_size) return 0; - addr = uaddr + expected_size; - end = uaddr + actual_size; - - for (; addr < end; addr++) { - err = get_user(val, addr); - if (err) - return err; - if (val) - return -E2BIG; - } - - return 0; + res = check_zeroed_user(addr, actual_size - expected_size); + if (res < 0) + return res; + return res ? 0 : -E2BIG; } const struct bpf_map_ops bpf_map_offload_ops = { -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() 2020-05-29 23:28 ` [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() Al Viro @ 2020-05-31 16:35 ` Alexei Starovoitov 0 siblings, 0 replies; 94+ messages in thread From: Alexei Starovoitov @ 2020-05-31 16:35 UTC (permalink / raw) To: Al Viro Cc: Linus Torvalds, linux-kernel, linux-fsdevel, Alexei Starovoitov, bpf On Sat, May 30, 2020 at 12:28:14AM +0100, Al Viro wrote: > From: Al Viro <viro@zeniv.linux.org.uk> > > ... rather than open-coding it, and badly, at that. > > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- > kernel/bpf/syscall.c | 25 ++++++------------------- > 1 file changed, 6 insertions(+), 19 deletions(-) lgtm Acked-by: Alexei Starovoitov <ast@kernel.org> ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCHES] uaccess hpsa 2020-05-28 23:40 [PATCHES] uaccess base Al Viro ` (10 preceding siblings ...) 2020-05-29 23:28 ` [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() Al Viro @ 2020-05-29 23:39 ` Al Viro 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro ` (2 more replies) 11 siblings, 3 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:39 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Don Brace, linux-scsi hpsa compat ioctl done (hopefully) saner. I really want to kill compat_alloc_user_space() off - it's always trouble and for a driver-private ioctls it's absolutely pointless. Note that this is only compile-tested - I don't have the hardware to test it on *or* userland to issue the ioctls in question. So this series definitely needs a review and testing from hpsa maintainers before it might go anywhere. The series is in vfs.git #uaccess.hpsa, based at v5.7-rc1 Al Viro (4): hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct hpsa: get rid of compat_alloc_user_space() hpsa_ioctl(): tidy up a bit drivers/scsi/hpsa.c | 199 ++++++++++++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 109 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() 2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro @ 2020-05-29 23:40 ` Al Viro 2020-05-29 23:40 ` [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct Al Viro ` (2 more replies) 2020-06-03 1:57 ` [PATCHES] uaccess hpsa Martin K. Petersen 2020-06-03 18:37 ` Don.Brace 2 siblings, 3 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Don Brace, linux-scsi From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/hpsa.c | 116 +++++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 60 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 1e9302e99d05..3344a06c938e 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -6358,37 +6358,33 @@ static int hpsa_getdrivver_ioctl(struct ctlr_info *h, void __user *argp) return 0; } -static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp) +static int hpsa_passthru_ioctl(struct ctlr_info *h, + IOCTL_Command_struct *iocommand) { - IOCTL_Command_struct iocommand; struct CommandList *c; char *buff = NULL; u64 temp64; int rc = 0; - if (!argp) - return -EINVAL; if (!capable(CAP_SYS_RAWIO)) return -EPERM; - if (copy_from_user(&iocommand, argp, sizeof(iocommand))) - return -EFAULT; - if ((iocommand.buf_size < 1) && - (iocommand.Request.Type.Direction != XFER_NONE)) { + if ((iocommand->buf_size < 1) && + (iocommand->Request.Type.Direction != XFER_NONE)) { return -EINVAL; } - if (iocommand.buf_size > 0) { - buff = kmalloc(iocommand.buf_size, GFP_KERNEL); + if (iocommand->buf_size > 0) { + buff = kmalloc(iocommand->buf_size, GFP_KERNEL); if (buff == NULL) return -ENOMEM; - if (iocommand.Request.Type.Direction & XFER_WRITE) { + if (iocommand->Request.Type.Direction & XFER_WRITE) { /* Copy the data into the buffer we created */ - if (copy_from_user(buff, iocommand.buf, - iocommand.buf_size)) { + if (copy_from_user(buff, iocommand->buf, + iocommand->buf_size)) { rc = -EFAULT; goto out_kfree; } } else { - memset(buff, 0, iocommand.buf_size); + memset(buff, 0, iocommand->buf_size); } } c = cmd_alloc(h); @@ -6398,23 +6394,23 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp) c->scsi_cmd = SCSI_CMD_BUSY; /* Fill in Command Header */ c->Header.ReplyQueue = 0; /* unused in simple mode */ - if (iocommand.buf_size > 0) { /* buffer to fill */ + if (iocommand->buf_size > 0) { /* buffer to fill */ c->Header.SGList = 1; c->Header.SGTotal = cpu_to_le16(1); } else { /* no buffers to fill */ c->Header.SGList = 0; c->Header.SGTotal = cpu_to_le16(0); } - memcpy(&c->Header.LUN, &iocommand.LUN_info, sizeof(c->Header.LUN)); + memcpy(&c->Header.LUN, &iocommand->LUN_info, sizeof(c->Header.LUN)); /* Fill in Request block */ - memcpy(&c->Request, &iocommand.Request, + memcpy(&c->Request, &iocommand->Request, sizeof(c->Request)); /* Fill in the scatter gather information */ - if (iocommand.buf_size > 0) { + if (iocommand->buf_size > 0) { temp64 = dma_map_single(&h->pdev->dev, buff, - iocommand.buf_size, DMA_BIDIRECTIONAL); + iocommand->buf_size, DMA_BIDIRECTIONAL); if (dma_mapping_error(&h->pdev->dev, (dma_addr_t) temp64)) { c->SG[0].Addr = cpu_to_le64(0); c->SG[0].Len = cpu_to_le32(0); @@ -6422,12 +6418,12 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp) goto out; } c->SG[0].Addr = cpu_to_le64(temp64); - c->SG[0].Len = cpu_to_le32(iocommand.buf_size); + c->SG[0].Len = cpu_to_le32(iocommand->buf_size); c->SG[0].Ext = cpu_to_le32(HPSA_SG_LAST); /* not chaining */ } rc = hpsa_scsi_do_simple_cmd(h, c, DEFAULT_REPLY_QUEUE, NO_TIMEOUT); - if (iocommand.buf_size > 0) + if (iocommand->buf_size > 0) hpsa_pci_unmap(h->pdev, c, 1, DMA_BIDIRECTIONAL); check_ioctl_unit_attention(h, c); if (rc) { @@ -6436,16 +6432,12 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp) } /* Copy the error information out */ - memcpy(&iocommand.error_info, c->err_info, - sizeof(iocommand.error_info)); - if (copy_to_user(argp, &iocommand, sizeof(iocommand))) { - rc = -EFAULT; - goto out; - } - if ((iocommand.Request.Type.Direction & XFER_READ) && - iocommand.buf_size > 0) { + memcpy(&iocommand->error_info, c->err_info, + sizeof(iocommand->error_info)); + if ((iocommand->Request.Type.Direction & XFER_READ) && + iocommand->buf_size > 0) { /* Copy the data out of the buffer we created */ - if (copy_to_user(iocommand.buf, buff, iocommand.buf_size)) { + if (copy_to_user(iocommand->buf, buff, iocommand->buf_size)) { rc = -EFAULT; goto out; } @@ -6457,9 +6449,9 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h, void __user *argp) return rc; } -static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp) +static int hpsa_big_passthru_ioctl(struct ctlr_info *h, + BIG_IOCTL_Command_struct *ioc) { - BIG_IOCTL_Command_struct *ioc; struct CommandList *c; unsigned char **buff = NULL; int *buff_size = NULL; @@ -6470,29 +6462,17 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp) u32 sz; BYTE __user *data_ptr; - if (!argp) - return -EINVAL; if (!capable(CAP_SYS_RAWIO)) return -EPERM; - ioc = vmemdup_user(argp, sizeof(*ioc)); - if (IS_ERR(ioc)) { - status = PTR_ERR(ioc); - goto cleanup1; - } + if ((ioc->buf_size < 1) && - (ioc->Request.Type.Direction != XFER_NONE)) { - status = -EINVAL; - goto cleanup1; - } + (ioc->Request.Type.Direction != XFER_NONE)) + return -EINVAL; /* Check kmalloc limits using all SGs */ - if (ioc->malloc_size > MAX_KMALLOC_SIZE) { - status = -EINVAL; - goto cleanup1; - } - if (ioc->buf_size > ioc->malloc_size * SG_ENTRIES_IN_CMD) { - status = -EINVAL; - goto cleanup1; - } + if (ioc->malloc_size > MAX_KMALLOC_SIZE) + return -EINVAL; + if (ioc->buf_size > ioc->malloc_size * SG_ENTRIES_IN_CMD) + return -EINVAL; buff = kcalloc(SG_ENTRIES_IN_CMD, sizeof(char *), GFP_KERNEL); if (!buff) { status = -ENOMEM; @@ -6565,10 +6545,6 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp) /* Copy the error information out */ memcpy(&ioc->error_info, c->err_info, sizeof(ioc->error_info)); - if (copy_to_user(argp, ioc, sizeof(*ioc))) { - status = -EFAULT; - goto cleanup0; - } if ((ioc->Request.Type.Direction & XFER_READ) && ioc->buf_size > 0) { int i; @@ -6594,7 +6570,6 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp) kfree(buff); } kfree(buff_size); - kvfree(ioc); return status; } @@ -6628,18 +6603,39 @@ static int hpsa_ioctl(struct scsi_device *dev, unsigned int cmd, return hpsa_getpciinfo_ioctl(h, argp); case CCISS_GETDRIVVER: return hpsa_getdrivver_ioctl(h, argp); - case CCISS_PASSTHRU: + case CCISS_PASSTHRU: { + IOCTL_Command_struct iocommand; + + if (!argp) + return -EINVAL; + if (copy_from_user(&iocommand, argp, sizeof(iocommand))) + return -EFAULT; if (atomic_dec_if_positive(&h->passthru_cmds_avail) < 0) return -EAGAIN; - rc = hpsa_passthru_ioctl(h, argp); + rc = hpsa_passthru_ioctl(h, &iocommand); atomic_inc(&h->passthru_cmds_avail); + if (!rc && copy_to_user(argp, &iocommand, sizeof(iocommand))) + rc = -EFAULT; return rc; - case CCISS_BIG_PASSTHRU: + } + case CCISS_BIG_PASSTHRU: { + BIG_IOCTL_Command_struct *ioc; + if (!argp) + return -EINVAL; if (atomic_dec_if_positive(&h->passthru_cmds_avail) < 0) return -EAGAIN; - rc = hpsa_big_passthru_ioctl(h, argp); + ioc = vmemdup_user(argp, sizeof(*ioc)); + if (IS_ERR(ioc)) { + atomic_inc(&h->passthru_cmds_avail); + return PTR_ERR(ioc); + } + rc = hpsa_big_passthru_ioctl(h, ioc); atomic_inc(&h->passthru_cmds_avail); + if (!rc && copy_to_user(argp, ioc, sizeof(*ioc))) + rc = -EFAULT; + kvfree(ioc); return rc; + } default: return -ENOTTY; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro @ 2020-05-29 23:40 ` Al Viro 2020-05-29 23:40 ` [PATCH 3/4] hpsa: get rid of compat_alloc_user_space() Al Viro 2020-05-29 23:40 ` [PATCH 4/4] hpsa_ioctl(): tidy up a bit Al Viro 2 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Don Brace, linux-scsi From: Al Viro <viro@zeniv.linux.org.uk> "BIG" in the name refers to the amount of data being transferred, _not_ the size of structure itself; it's 140 or 144 bytes (for 32bit and 64bit hosts resp.). IOCTL_Command_struct is 136 or 144 bytes large... No point whatsoever turning that into dynamic allocation, let alone vmalloc one. Just keep it as local variable... Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/hpsa.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 3344a06c938e..64fd97272109 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -6619,21 +6619,17 @@ static int hpsa_ioctl(struct scsi_device *dev, unsigned int cmd, return rc; } case CCISS_BIG_PASSTHRU: { - BIG_IOCTL_Command_struct *ioc; + BIG_IOCTL_Command_struct ioc; if (!argp) return -EINVAL; + if (copy_from_user(&ioc, argp, sizeof(ioc))) + return -EFAULT; if (atomic_dec_if_positive(&h->passthru_cmds_avail) < 0) return -EAGAIN; - ioc = vmemdup_user(argp, sizeof(*ioc)); - if (IS_ERR(ioc)) { - atomic_inc(&h->passthru_cmds_avail); - return PTR_ERR(ioc); - } - rc = hpsa_big_passthru_ioctl(h, ioc); + rc = hpsa_big_passthru_ioctl(h, &ioc); atomic_inc(&h->passthru_cmds_avail); - if (!rc && copy_to_user(argp, ioc, sizeof(*ioc))) + if (!rc && copy_to_user(argp, &ioc, sizeof(ioc))) rc = -EFAULT; - kvfree(ioc); return rc; } default: -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 3/4] hpsa: get rid of compat_alloc_user_space() 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro 2020-05-29 23:40 ` [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct Al Viro @ 2020-05-29 23:40 ` Al Viro 2020-05-29 23:40 ` [PATCH 4/4] hpsa_ioctl(): tidy up a bit Al Viro 2 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Don Brace, linux-scsi From: Al Viro <viro@zeniv.linux.org.uk> no need for building a native struct on kernel stack, copying it to userland one, then calling hpsa_ioctl() which copies it back into _another_ instance of the same struct. Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/hpsa.c | 80 ++++++++++++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 44 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 64fd97272109..c7fbe56891ef 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -254,6 +254,10 @@ static irqreturn_t do_hpsa_intr_intx(int irq, void *dev_id); static irqreturn_t do_hpsa_intr_msi(int irq, void *dev_id); static int hpsa_ioctl(struct scsi_device *dev, unsigned int cmd, void __user *arg); +static int hpsa_passthru_ioctl(struct ctlr_info *h, + IOCTL_Command_struct *iocommand); +static int hpsa_big_passthru_ioctl(struct ctlr_info *h, + BIG_IOCTL_Command_struct *ioc); #ifdef CONFIG_COMPAT static int hpsa_compat_ioctl(struct scsi_device *dev, unsigned int cmd, @@ -6217,75 +6221,63 @@ static void cmd_free(struct ctlr_info *h, struct CommandList *c) static int hpsa_ioctl32_passthru(struct scsi_device *dev, unsigned int cmd, void __user *arg) { - IOCTL32_Command_struct __user *arg32 = - (IOCTL32_Command_struct __user *) arg; + struct ctlr_info *h = sdev_to_hba(dev); + IOCTL32_Command_struct __user *arg32 = arg; IOCTL_Command_struct arg64; - IOCTL_Command_struct __user *p = compat_alloc_user_space(sizeof(arg64)); int err; u32 cp; - memset(&arg64, 0, sizeof(arg64)); - err = 0; - err |= copy_from_user(&arg64.LUN_info, &arg32->LUN_info, - sizeof(arg64.LUN_info)); - err |= copy_from_user(&arg64.Request, &arg32->Request, - sizeof(arg64.Request)); - err |= copy_from_user(&arg64.error_info, &arg32->error_info, - sizeof(arg64.error_info)); - err |= get_user(arg64.buf_size, &arg32->buf_size); - err |= get_user(cp, &arg32->buf); - arg64.buf = compat_ptr(cp); - err |= copy_to_user(p, &arg64, sizeof(arg64)); + if (!arg) + return -EINVAL; - if (err) + memset(&arg64, 0, sizeof(arg64)); + if (copy_from_user(&arg64, arg32, offsetof(IOCTL_Command_struct, buf))) + return -EFAULT; + if (get_user(cp, &arg32->buf)) return -EFAULT; + arg64.buf = compat_ptr(cp); - err = hpsa_ioctl(dev, CCISS_PASSTHRU, p); + if (atomic_dec_if_positive(&h->passthru_cmds_avail) < 0) + return -EAGAIN; + err = hpsa_passthru_ioctl(h, &arg64); + atomic_inc(&h->passthru_cmds_avail); if (err) return err; - err |= copy_in_user(&arg32->error_info, &p->error_info, - sizeof(arg32->error_info)); - if (err) + if (copy_to_user(&arg32->error_info, &arg64.error_info, + sizeof(arg32->error_info))) return -EFAULT; - return err; + return 0; } static int hpsa_ioctl32_big_passthru(struct scsi_device *dev, unsigned int cmd, void __user *arg) { - BIG_IOCTL32_Command_struct __user *arg32 = - (BIG_IOCTL32_Command_struct __user *) arg; + struct ctlr_info *h = sdev_to_hba(dev); + BIG_IOCTL32_Command_struct __user *arg32 = arg; BIG_IOCTL_Command_struct arg64; - BIG_IOCTL_Command_struct __user *p = - compat_alloc_user_space(sizeof(arg64)); int err; u32 cp; + if (!arg) + return -EINVAL; memset(&arg64, 0, sizeof(arg64)); - err = 0; - err |= copy_from_user(&arg64.LUN_info, &arg32->LUN_info, - sizeof(arg64.LUN_info)); - err |= copy_from_user(&arg64.Request, &arg32->Request, - sizeof(arg64.Request)); - err |= copy_from_user(&arg64.error_info, &arg32->error_info, - sizeof(arg64.error_info)); - err |= get_user(arg64.buf_size, &arg32->buf_size); - err |= get_user(arg64.malloc_size, &arg32->malloc_size); - err |= get_user(cp, &arg32->buf); - arg64.buf = compat_ptr(cp); - err |= copy_to_user(p, &arg64, sizeof(arg64)); - - if (err) + if (copy_from_user(&arg64, arg32, + offsetof(BIG_IOCTL32_Command_struct, buf))) return -EFAULT; + if (get_user(cp, &arg32->buf)) + return -EFAULT; + arg64.buf = compat_ptr(cp); - err = hpsa_ioctl(dev, CCISS_BIG_PASSTHRU, p); + if (atomic_dec_if_positive(&h->passthru_cmds_avail) < 0) + return -EAGAIN; + err = hpsa_big_passthru_ioctl(h, &arg64); + atomic_inc(&h->passthru_cmds_avail); if (err) return err; - err |= copy_in_user(&arg32->error_info, &p->error_info, - sizeof(arg32->error_info)); - if (err) + if (copy_to_user(&arg32->error_info, &arg64.error_info, + sizeof(arg32->error_info))) return -EFAULT; - return err; + return 0; } static int hpsa_compat_ioctl(struct scsi_device *dev, unsigned int cmd, -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* [PATCH 4/4] hpsa_ioctl(): tidy up a bit 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro 2020-05-29 23:40 ` [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct Al Viro 2020-05-29 23:40 ` [PATCH 3/4] hpsa: get rid of compat_alloc_user_space() Al Viro @ 2020-05-29 23:40 ` Al Viro 2 siblings, 0 replies; 94+ messages in thread From: Al Viro @ 2020-05-29 23:40 UTC (permalink / raw) To: Linus Torvalds; +Cc: linux-kernel, linux-fsdevel, Don Brace, linux-scsi From: Al Viro <viro@zeniv.linux.org.uk> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- drivers/scsi/hpsa.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index c7fbe56891ef..81d0414e2117 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -6577,14 +6577,11 @@ static void check_ioctl_unit_attention(struct ctlr_info *h, * ioctl */ static int hpsa_ioctl(struct scsi_device *dev, unsigned int cmd, - void __user *arg) + void __user *argp) { - struct ctlr_info *h; - void __user *argp = (void __user *)arg; + struct ctlr_info *h = sdev_to_hba(dev); int rc; - h = sdev_to_hba(dev); - switch (cmd) { case CCISS_DEREGDISK: case CCISS_REGNEWDISK: -- 2.11.0 ^ permalink raw reply related [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess hpsa 2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro @ 2020-06-03 1:57 ` Martin K. Petersen 2020-06-03 18:37 ` Don.Brace 2 siblings, 0 replies; 94+ messages in thread From: Martin K. Petersen @ 2020-06-03 1:57 UTC (permalink / raw) To: Don Brace Cc: Al Viro, Linus Torvalds, linux-kernel, linux-fsdevel, linux-scsi > hpsa compat ioctl done (hopefully) saner. I really want > to kill compat_alloc_user_space() off - it's always trouble and > for a driver-private ioctls it's absolutely pointless. > > Note that this is only compile-tested - I don't have the > hardware to test it on *or* userland to issue the ioctls in > question. So this series definitely needs a review and testing > from hpsa maintainers before it might go anywhere. Don: Please test and review. Thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 94+ messages in thread
* RE: [PATCHES] uaccess hpsa 2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro 2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro 2020-06-03 1:57 ` [PATCHES] uaccess hpsa Martin K. Petersen @ 2020-06-03 18:37 ` Don.Brace 2020-06-03 19:17 ` Al Viro 2 siblings, 1 reply; 94+ messages in thread From: Don.Brace @ 2020-06-03 18:37 UTC (permalink / raw) To: viro, torvalds; +Cc: linux-kernel, linux-fsdevel, don.brace, linux-scsi -----Original Message----- From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Al Viro Sent: Friday, May 29, 2020 6:39 PM To: Linus Torvalds <torvalds@linux-foundation.org> Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; Don Brace <don.brace@microsemi.com>; linux-scsi@vger.kernel.org Subject: [PATCHES] uaccess hpsa hpsa compat ioctl done (hopefully) saner. I really want to kill compat_alloc_user_space() off - it's always trouble and for a driver-private ioctls it's absolutely pointless. The series is in vfs.git #uaccess.hpsa, based at v5.7-rc1 Al Viro (4): hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct hpsa: get rid of compat_alloc_user_space() hpsa_ioctl(): tidy up a bit Acked-by: Don Brace <don.brace@microsemi.com> Tested-by: Don Brace <don.brace@microsemi.com> drivers/scsi/hpsa.c | 199 ++++++++++++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 109 deletions(-) ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess hpsa 2020-06-03 18:37 ` Don.Brace @ 2020-06-03 19:17 ` Al Viro 2020-06-03 20:53 ` Martin K. Petersen 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-06-03 19:17 UTC (permalink / raw) To: Don.Brace; +Cc: torvalds, linux-kernel, linux-fsdevel, don.brace, linux-scsi On Wed, Jun 03, 2020 at 06:37:11PM +0000, Don.Brace@microchip.com wrote: > -----Original Message----- > From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-owner@vger.kernel.org] On Behalf Of Al Viro > Sent: Friday, May 29, 2020 6:39 PM > To: Linus Torvalds <torvalds@linux-foundation.org> > Cc: linux-kernel@vger.kernel.org; linux-fsdevel@vger.kernel.org; Don Brace <don.brace@microsemi.com>; linux-scsi@vger.kernel.org > Subject: [PATCHES] uaccess hpsa > > hpsa compat ioctl done (hopefully) saner. I really want to kill compat_alloc_user_space() off - it's always trouble and for a driver-private ioctls it's absolutely pointless. > > The series is in vfs.git #uaccess.hpsa, based at v5.7-rc1 > > Al Viro (4): > hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() > hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct > hpsa: get rid of compat_alloc_user_space() > hpsa_ioctl(): tidy up a bit > > Acked-by: Don Brace <don.brace@microsemi.com> > Tested-by: Don Brace <don.brace@microsemi.com> OK... Acked-by/Tested-by added, branch re-pushed (commits are otherwise identical). Which tree would you prefer that to go through - vfs.git, scsi.git, something else? ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess hpsa 2020-06-03 19:17 ` Al Viro @ 2020-06-03 20:53 ` Martin K. Petersen 2020-06-03 20:54 ` Al Viro 0 siblings, 1 reply; 94+ messages in thread From: Martin K. Petersen @ 2020-06-03 20:53 UTC (permalink / raw) To: Al Viro Cc: Don.Brace, torvalds, linux-kernel, linux-fsdevel, don.brace, linux-scsi Hi Al! > OK... Acked-by/Tested-by added, branch re-pushed (commits are otherwise > identical). Which tree would you prefer that to go through - vfs.git, > scsi.git, something else? I don't have anything queued for 5.8 for hpsa so there shouldn't be any conflicts if it goes through vfs.git. But I'm perfectly happy to take the changes through SCSI if that's your preference. -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess hpsa 2020-06-03 20:53 ` Martin K. Petersen @ 2020-06-03 20:54 ` Al Viro 2020-06-04 14:18 ` Martin K. Petersen 0 siblings, 1 reply; 94+ messages in thread From: Al Viro @ 2020-06-03 20:54 UTC (permalink / raw) To: Martin K. Petersen Cc: Don.Brace, torvalds, linux-kernel, linux-fsdevel, don.brace, linux-scsi On Wed, Jun 03, 2020 at 04:53:11PM -0400, Martin K. Petersen wrote: > > Hi Al! > > > OK... Acked-by/Tested-by added, branch re-pushed (commits are otherwise > > identical). Which tree would you prefer that to go through - vfs.git, > > scsi.git, something else? > > I don't have anything queued for 5.8 for hpsa so there shouldn't be any > conflicts if it goes through vfs.git. But I'm perfectly happy to take > the changes through SCSI if that's your preference. Up to you; if you need a pull request, just say so. ^ permalink raw reply [flat|nested] 94+ messages in thread
* Re: [PATCHES] uaccess hpsa 2020-06-03 20:54 ` Al Viro @ 2020-06-04 14:18 ` Martin K. Petersen 0 siblings, 0 replies; 94+ messages in thread From: Martin K. Petersen @ 2020-06-04 14:18 UTC (permalink / raw) To: Al Viro Cc: Martin K. Petersen, Don.Brace, torvalds, linux-kernel, linux-fsdevel, don.brace, linux-scsi Al, >> I don't have anything queued for 5.8 for hpsa so there shouldn't be any >> conflicts if it goes through vfs.git. But I'm perfectly happy to take >> the changes through SCSI if that's your preference. > > Up to you; if you need a pull request, just say so. OK, I queued these up for 5.8. Thanks! -- Martin K. Petersen Oracle Linux Engineering ^ permalink raw reply [flat|nested] 94+ messages in thread
end of thread, other threads:[~2020-06-04 14:18 UTC | newest]
Thread overview: 94+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-28 23:40 [PATCHES] uaccess base Al Viro
2020-05-28 23:48 ` [PATCHES] uaccess readdir Al Viro
2020-05-28 23:49 ` [PATCH 4/6] switch readdir(2) to unsafe_copy_dirent_name() Al Viro
2020-05-28 23:49 ` [PATCH 5/6] readdir.c: get compat_filldir() more or less in sync with filldir() Al Viro
2020-05-28 23:49 ` [PATCH 6/6] readdir.c: get rid of the last __put_user(), drop now-useless access_ok() Al Viro
2020-05-28 23:49 ` [PATCH 1/6] uaccess: Add user_read_access_begin/end and user_write_access_begin/end Al Viro
2020-05-28 23:49 ` [PATCH 2/6] uaccess: Selectively open read or write user access Al Viro
2020-05-28 23:49 ` [PATCH 3/6] drm/i915/gem: Replace user_access_begin by user_write_access_begin Al Viro
2020-05-28 23:57 ` [PATCHES] uaccess __copy_from_user() Al Viro
2020-05-28 23:58 ` [PATCH 1/2] firewire: switch ioctl_queue_iso to use of copy_from_user() Al Viro
2020-05-28 23:58 ` [PATCH 2/2] pstore: switch to copy_from_user() Al Viro
2020-05-29 0:03 ` [PATCHES] uaccess __copy_to_user() Al Viro
2020-05-29 0:04 ` [PATCH 1/2] esas2r: don't bother with __copy_to_user() Al Viro
2020-05-29 0:04 ` [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user() Al Viro
2020-05-29 1:27 ` Linus Torvalds
2020-05-29 1:47 ` Al Viro
2020-05-29 1:54 ` Linus Torvalds
2020-05-29 3:10 ` Al Viro
2020-05-29 3:42 ` Linus Torvalds
2020-05-29 20:46 ` Al Viro
2020-05-29 20:57 ` Linus Torvalds
2020-05-29 21:06 ` Al Viro
2020-05-29 0:09 ` [PATCHES] uaccess __put_user() Al Viro
2020-05-29 0:10 ` [PATCH 1/3] compat sysinfo(2): don't bother with field-by-field copyout Al Viro
2020-05-29 0:10 ` [PATCH 2/3] scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user() Al Viro
2020-05-29 0:10 ` [PATCH 3/3] pcm_native: result of put_user() needs to be checked Al Viro
2020-05-29 0:34 ` [PATCHES] uaccess comedi compat Al Viro
2020-05-29 0:35 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Al Viro
2020-05-29 0:35 ` [PATCH 02/10] comedi: get rid of indirection via translated_ioctl() Al Viro
2020-05-29 10:34 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat Al Viro
2020-05-29 10:35 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat Al Viro
2020-05-29 10:35 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat Al Viro
2020-05-29 10:05 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat Al Viro
2020-05-29 10:36 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd() Al Viro
2020-05-29 10:37 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller Al Viro
2020-05-29 10:37 ` Ian Abbott
2020-05-29 0:35 ` [PATCH 09/10] comedi: do_cmd_ioctl(): " Al Viro
2020-05-29 10:38 ` Ian Abbott
2020-05-29 10:34 ` [PATCH 01/10] comedi: move compat ioctl handling to native fops Ian Abbott
2020-05-29 10:48 ` [PATCHES] uaccess comedi compat Ian Abbott
2020-05-29 14:15 ` Al Viro
2020-05-29 0:40 ` [PATCHES] uaccess i915 Al Viro
2020-05-29 5:06 ` Jani Nikula
2020-05-29 5:06 ` [Intel-gfx] " Jani Nikula
2020-05-29 14:17 ` Al Viro
2020-05-29 14:17 ` [Intel-gfx] " Al Viro
2020-05-29 0:41 ` [PATCH 1/5] i915: switch query_{topology,engine}_info() to copy_to_user() Al Viro
2020-05-29 0:41 ` [PATCH 2/5] i915: switch copy_perf_config_registers_or_number() to unsafe_put_user() Al Viro
2020-05-29 0:41 ` [PATCH 3/5] i915 compat ioctl(): just use drm_ioctl_kernel() Al Viro
2020-05-29 0:41 ` [PATCH 4/5] i915: alloc_oa_regs(): get rid of pointless access_ok() Al Viro
2020-05-29 0:41 ` [PATCH 5/5] i915:get_engines(): " Al Viro
2020-05-29 23:26 ` [PATCHES] uaccess misc Al Viro
2020-05-29 23:54 ` Linus Torvalds
2020-05-29 23:57 ` Linus Torvalds
2020-05-29 23:27 ` [PATCH 1/9] pselect6() and friends: take handling the combined 6th/7th args into helper Al Viro
2020-05-29 23:27 ` [PATCH 2/9] binfmt_elf: don't bother with __{put,copy_to}_user() Al Viro
2020-05-29 23:27 ` [PATCH 3/9] binfmt_elf_fdpic: don't use __... uaccess primitives Al Viro
2020-05-29 23:27 ` [PATCH 4/9] binfmt_flat: don't use __put_user() Al Viro
2020-05-29 23:27 ` [PATCH 5/9] x86: switch cp_stat64() to unsafe_put_user() Al Viro
2020-05-29 23:27 ` [PATCH 6/9] TEST_ACCESS_OK _never_ had been checked anywhere Al Viro
2020-05-29 23:27 ` [PATCH 7/9] user_regset_copyout_zero(): use clear_user() Al Viro
2020-05-29 23:27 ` [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user() Al Viro
2020-05-29 23:52 ` Linus Torvalds
2020-05-30 14:31 ` Al Viro
2020-05-30 14:52 ` Al Viro
2020-05-30 16:20 ` Paolo Bonzini
2020-05-30 17:57 ` Linus Torvalds
2020-05-30 18:38 ` Al Viro
2020-05-30 18:52 ` Linus Torvalds
2020-05-30 19:14 ` Al Viro
2020-05-30 19:20 ` Linus Torvalds
2020-05-30 19:42 ` Al Viro
2020-05-30 20:43 ` Al Viro
2020-05-30 19:19 ` Al Viro
2020-05-30 19:27 ` Al Viro
2020-05-29 23:28 ` [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user() Al Viro
2020-05-31 16:35 ` Alexei Starovoitov
2020-05-29 23:39 ` [PATCHES] uaccess hpsa Al Viro
2020-05-29 23:40 ` [PATCH 1/4] hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl() Al Viro
2020-05-29 23:40 ` [PATCH 2/4] hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct Al Viro
2020-05-29 23:40 ` [PATCH 3/4] hpsa: get rid of compat_alloc_user_space() Al Viro
2020-05-29 23:40 ` [PATCH 4/4] hpsa_ioctl(): tidy up a bit Al Viro
2020-06-03 1:57 ` [PATCHES] uaccess hpsa Martin K. Petersen
2020-06-03 18:37 ` Don.Brace
2020-06-03 19:17 ` Al Viro
2020-06-03 20:53 ` Martin K. Petersen
2020-06-03 20:54 ` Al Viro
2020-06-04 14:18 ` Martin K. Petersen
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.