uaccess base
Subsystems (20)
Al Viro
maintainerPATCH 1/1 uaccess base
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(-)
Al Viro
maintainerPATCH 1/1 uaccess readdir
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(-)
Al Viro
maintainerPATCH 4/6 switch readdir(2) to unsafe_copy_dirent_name()
From: Al Viro <[email protected]>
... and the same for its compat counterpart
--- fs/readdir.c | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-)
fs/readdir.c +16 -14
@@ -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 ;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 5/6 readdir.c: get compat_filldir() more or less in sync with filldir()
From: Al Viro <[email protected]>
--- fs/readdir.c | 51 +++++++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 24 deletions(-)
fs/readdir.c +27 -24
@@ -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
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 6/6 readdir.c: get rid of the last __put_user(), drop now-useless access_ok()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/readdir.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-)
fs/readdir.c +1 -10
@@ -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 ;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/6 uaccess: Add user_read_access_begin/end and user_write_access_begin/end
In reply to Al ViroFrom: Christophe Leroy <[email protected]>
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.
--- include/linux/uaccess.h | 8 ++++++++ 1 file changed, 8 insertions(+)
include/linux/uaccess.h +8 -0
@@ -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,
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/6 uaccess: Selectively open read or write user access
From: Christophe Leroy <[email protected]>
When opening user access to only perform reads, only open read access. When opening user access to only perform writes, only open write access.
--- 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(-)
fs/readdir.c +6 -6
@@ -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 ;
kernel/compat.c +6 -6
@@ -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 ; }
kernel/exit.c +6 -6
@@ -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
lib/strncpy_from_user.c +2 -2
@@ -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; } }
lib/strnlen_user.c +2 -2
@@ -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; } }
lib/usercopy.c +3 -3
@@ -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);
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/6 drm/i915/gem: Replace user_access_begin by user_write_access_begin
In reply to Al ViroFrom: Christophe Leroy <[email protected]>
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.
--- drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +3 -2
@@ -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:; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/1 uaccess __copy_from_user()
In reply to Al Viro 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(-)
Al Viro
maintainerPATCH 1/2 firewire: switch ioctl_queue_iso to use of copy_from_user()
From: Al Viro <[email protected]>
no point trying to do access_ok() for all those __copy_from_user() at once.
--- drivers/firewire/core-cdev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/firewire/core-cdev.c +1 -3
@@ -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 &&
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/2 pstore: switch to copy_from_user()
From: Al Viro <[email protected]>
don't bother trying to do bulk access_ok()
--- fs/pstore/ram_core.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
fs/pstore/ram_core.c +1 -3
@@ -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;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/1 uaccess __copy_to_user()
In reply to Al ViroSimilar 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(-)
Al Viro
maintainerPATCH 1/2 esas2r: don't bother with __copy_to_user()
From: Al Viro <[email protected]>
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().
--- drivers/scsi/esas2r/esas2r_ioctl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/esas2r/esas2r_ioctl.c +1 -1
@@ -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)",
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/2 dlmfs: convert dlmfs_file_read() to copy_to_user()
From: Al Viro <[email protected]>
--- fs/ocfs2/dlmfs/dlmfs.c | 33 ++++++++++++++------------------- 1 file changed, 14 insertions(+), 19 deletions(-)
fs/ocfs2/dlmfs/dlmfs.c +14 -19
@@ -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,
Signature · 1 line
2.11.0
Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
fs/ocfs2/dlmfs/dlmfs.c static ssize_t dlmfs_file_read(struct file *filp, · line 242
Quoted 7 lines · Al Viro
if (*ppos >= i_size_read(inode)) return 0;fs/ocfs2/dlmfs/dlmfs.c +3
-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
Al Viro
maintainerRe: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 17 lines · Linus Torvalds
fs/ocfs2/dlmfs/dlmfs.c static ssize_t dlmfs_file_read(struct file *filp, · line 242
Quoted 7 lines · Al Viro
if (*ppos >= i_size_read(inode)) return 0;fs/ocfs2/dlmfs/dlmfs.c +3
-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 concurrentpread()with a write..
Quoted 4 lines
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.
Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 8 lines · Al Viro
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
Al Viro
maintainerRe: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 13 lines · Linus Torvalds
Quoted 8 lines · Al Viro
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.
Quoted 8 lines
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 <[email protected]> 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.
fs/select.c +45 -30
@@ -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 );
Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 4 lines · Al Viro
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:
fs/select.c static long do_pselect(int n, fd_set __user *inp, fd_set __user *outp, · line 771
Quoted 3 lines
fs/select.c +3
-0+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
Quoted 5 lines
size_t sigsetsize = 0;
sigset_t __user *up = NULL;
if (unkludge_sigmask(sig, &up, &sigsetsize))
return -EFAULT;to
Quoted 4 lines
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
Review
Quoted 4 lines
struct compat_sigset_argpack argpack = { 0, 0 };(quoted diff) +2
-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
Al Viro
maintainerRe: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 14 lines · Linus Torvalds
Quoted 4 lines
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 thenQuoted 2 lines
struct compat_sigset_argpack argpack = { 0, 0 };
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-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...
Re: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 3 lines · Al Viro
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.
Quoted 2 lines
Al, trying to resist the temptation to call those struct
bad_ideaand structbad_idea_32...
I'm sure you can contain yourself.
Quoted 4 lines
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
Al Viro
maintainerRe: [PATCH 2/2] dlmfs: convert dlmfs_file_read() to copy_to_user()
Quoted 9 lines · Linus Torvalds
Quoted 4 lines
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...
Al Viro
maintainerPATCH 1/1 uaccess __put_user()
In reply to Al Viro 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(-)
Al Viro
maintainerPATCH 1/3 compat sysinfo(2): don't bother with field-by-field copyout
From: Al Viro <[email protected]>
--- kernel/sys.c | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-)
kernel/sys.c +17 -16
@@ -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 */
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/3 scsi_ioctl.c: switch SCSI_IOCTL_GET_IDLUN to copy_to_user()
From: Al Viro <[email protected]>
--- drivers/scsi/scsi_ioctl.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
drivers/scsi/scsi_ioctl.c +10 -10
@@ -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 :
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/3 pcm_native: result of put_user() needs to be checked
In reply to Al ViroFrom: Al Viro <[email protected]>
... 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
--- sound/core/pcm_native.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)
sound/core/pcm_native.c +8 -4
@@ -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 ; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/1 uaccess comedi compat
In reply to Al Viro 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
Al Viro
maintainerPATCH 1/10 comedi: move compat ioctl handling to native fops
From: Al Viro <[email protected]>
mechanical move
--- 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
drivers/staging/comedi/Makefile +0 -1
@@ -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
drivers/staging/comedi/comedi_compat32.c deleted +0 -455
455 lines not on this page · raw message
drivers/staging/comedi/comedi_compat32.h deleted +0 -28
@@ -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. <[email protected]> -* Copyright (C) 2007 MEV Ltd. <http://www.mev.co.uk/> -* -* COMEDI - Linux Control and Measurement Device Interface -* Copyright (C) 1997-2007 David A. Schleef <[email protected]> -*/ - -#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 */
drivers/staging/comedi/comedi_fops.c +448 -3
473 lines not on this page · raw message
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/10 comedi: get rid of indirection via translated_ioctl()
From: Al Viro <[email protected]>
--- drivers/staging/comedi/comedi_fops.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
drivers/staging/comedi/comedi_fops.c +8 -18
@@ -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 locase 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 locase 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);
Signature · 1 line
2.11.0
Re: [PATCH 02/10] comedi: get rid of indirection via translated_ioctl()
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 26 ++++++++------------------ 1 file changed, 8 insertions(+), 18 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 3/10 comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat
In reply to Al ViroFrom: Al Viro <[email protected]>
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.
--- drivers/staging/comedi/comedi_fops.c | 68 ++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
drivers/staging/comedi/comedi_fops.c +30 -38
@@ -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. */
Signature · 1 line
2.11.0
Re: [PATCH 03/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_CHANINFO compat
Quoted 10 lines · Al Viro
From: Al Viro <[email protected]>
Just take
copy_from_user()out ofdo_chaninfo_ioctl()into the caller and havecompat_chaninfo()build a native version and pass it todo_chaninfo_ioctl()directly.Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 68 ++++++++++++++++-------------------- 1 file changed, 30 insertions(+), 38 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 4/10 comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat
In reply to Al ViroFrom: Al Viro <[email protected]>
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.
--- 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(-)
drivers/staging/comedi/comedi_fops.c +19 -24
@@ -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. */
drivers/staging/comedi/comedi_internal.h +1 -1
@@ -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);
drivers/staging/comedi/range.c +7 -10
@@ -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 ;
Signature · 1 line
2.11.0
Re: [PATCH 04/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_RANGEINFO compat
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Just take
copy_from_user()out ofdo_rangeing_ioctl()into the caller and havecompat_rangeinfo()build a native version and pass it todo_rangeinfo_ioctl()directly.
Quoted 6 lines
Signed-off-by Al Viro <[email protected]>--- 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(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 5/10 comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
In reply to Al ViroFrom: Al Viro <[email protected]>
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.
--- drivers/staging/comedi/comedi_fops.c | 73 +++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 27 deletions(-)
drivers/staging/comedi/comedi_fops.c +46 -27
@@ -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; }/*
Signature · 1 line
2.11.0
Re: [PATCH 05/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSN compat
Quoted 20 lines · Al Viro
From: Al Viro <[email protected]>
Just take
copy_from_user()out ofdo_insn_ioctl()into the caller and havecompat_insn()build a native version and pass it todo_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()andcompat_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 oldget_compat_insn(); it will be gone in the next commit.Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 73 +++++++++++++++++++++++------------- 1 file changed, 46 insertions(+), 27 deletions(-)drivers/staging/comedi/comedi_fops.c +0
-0
[snip]
drivers/staging/comedi/comedi_fops.c static long comedi_unlocked_ioctl(struct file *file, unsigned int cmd, · line 2247
Quoted 11 lines
drivers/staging/comedi/comedi_fops.c +4
-3@@ -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:
Quoted 4 lines
drivers/staging/comedi/comedi_fops.c +2
-0+ rc =do_insn_ioctl (dev, &insn, file);break ; + }case COMEDI_POLL :
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 6/10 comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/staging/comedi/comedi_fops.c | 138 ++++++++++++----------------------- 1 file changed, 48 insertions(+), 90 deletions(-)
drivers/staging/comedi/comedi_fops.c +48 -90
@@ -1520,34 +1520,19 @@ static int parse_insn(struct comedi_device *dev, struct comedi_insn *insn,#define MIN_SAMPLES 16#define MAX_SAMPLES 65536static 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. */
Signature · 1 line
2.11.0
Re: [PATCH 06/10] comedi: get rid of compat_alloc_user_space() mess in COMEDI_INSNLIST compat
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 138 ++++++++++++----------------------- 1 file changed, 48 insertions(+), 90 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 7/10 comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/staging/comedi/comedi_fops.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
drivers/staging/comedi/comedi_fops.c +12 -8
@@ -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;
Signature · 1 line
2.11.0
Re: [PATCH 07/10] comedi: lift copy_from_user() into callers of __comedi_get_user_cmd()
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 8/10 comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/staging/comedi/comedi_fops.c | 45 ++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 23 deletions(-)
drivers/staging/comedi/comedi_fops.c +22 -23
@@ -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 &©_to_user ((void __user *)arg, &cmd,sizeof (cmd))) + rc = -EFAULT ;break ; + }case COMEDI_INSNLIST : {struct comedi_insnlist insnlist;struct comedi_insn *insns =NULL ;
Signature · 1 line
2.11.0
Re: [PATCH 08/10] comedi: do_cmdtest_ioctl(): lift copyin/copyout into the caller
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 45 ++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 23 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerPATCH 9/10 comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 24 deletions(-)
drivers/staging/comedi/comedi_fops.c +24 -24
@@ -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 &©_to_user ((void __user *)arg, &cmd,sizeof (cmd))) + rc = -EFAULT ;break ; + }case COMEDI_CMDTEST : {struct comedi_cmd cmd;bool copy = false;
Signature · 1 line
2.11.0
Re: [PATCH 09/10] comedi: do_cmd_ioctl(): lift copyin/copyout into the caller
Quoted 6 lines · Al Viro
From: Al Viro <[email protected]>
Signed-off-by Al Viro <[email protected]>--- drivers/staging/comedi/comedi_fops.c | 48 ++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 24 deletions(-)
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Re: [PATCH 01/10] comedi: move compat ioctl handling to native fops
In reply to Al ViroQuoted 5 lines · Al Viro
From: Al Viro <[email protected]>
mechanical move
Signed-off-by Al Viro <[email protected]>
Quoted 9 lines
--- 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.cdelete mode 100644drivers/staging/comedi/comedi_compat32.h
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Re: [PATCHES] uaccess comedi compat
In reply to Al ViroQuoted 18 lines · Al Viro
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 ofarch/*. IMOcompat_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
Quoted 11 lines
Al Viro (10):
comedi: move compat ioctl handling to native fops
comedi: get rid of indirection viatranslated_ioctl()
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_CHANINFOcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_RANGEINFOcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_INSNcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_INSNLISTcompat
comedi: liftcopy_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 ofcompat_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 [email protected].
Cheers,
Ian
Signature · 4 lines
-=( Ian Abbott <[email protected]> || 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. )=-
Al Viro
maintainerRe: [PATCHES] uaccess comedi compat
Quoted 12 lines · Ian Abbott
Quoted 11 lines
Al Viro (10):
comedi: move compat ioctl handling to native fops
comedi: get rid of indirection viatranslated_ioctl()
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_CHANINFOcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_RANGEINFOcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_INSNcompat
comedi: get rid ofcompat_alloc_user_space()mess inCOMEDI_INSNLISTcompat
comedi: liftcopy_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 ofcompat_alloc_user_space()mess in COMEDI_CMD{,TEST} compat
Quoted 3 lines
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...
Quoted 1 line
These should be Cc'd to Greg KH and to [email protected].
FWIW, 10/10 seems to have been really lost; follows here:
From 88833127a8f00da422ddef03425ad9b19eb65558 Mon Sep 17 00:00:00 2001
From: Al Viro <[email protected]>
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
--- drivers/staging/comedi/comedi_fops.c | 181 +++++++++++++---------------------- 1 file changed, 66 insertions(+), 115 deletions(-)
drivers/staging/comedi/comedi_fops.c +66 -115
@@ -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; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/1 uaccess i915
In reply to Al Viro 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(-)
Jani Nikula
maintainerRe: [PATCHES] uaccess i915
Quoted 3 lines · Al Viro
Low-hanging fruit in i915 uaccess-related stuff.
There's some subtler stuff remaining after that; these are the simple ones.
Please Cc: [email protected] for i915 changes.
Also added Chris who I believe will be able to best review the changes.
BR,
Jani.
Signature · 1 line
Jani Nikula, Intel Open Source Graphics Center
Al Viro
maintainerRe: [PATCHES] uaccess i915
Quoted 6 lines · Jani Nikula
Quoted 3 lines · Al Viro
Low-hanging fruit in i915 uaccess-related stuff.
There's some subtler stuff remaining after that; these are the simple ones.Please Cc: [email protected] for i915 changes.
Will do. Do you want me to resend those (or post them there separately, or...)?
Quoted 1 line
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
Al Viro
maintainerPATCH 1/5 i915: switch query_{topology,engine}_info() to copy_to_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/gpu/drm/i915/i915_query.c | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-)
drivers/gpu/drm/i915/i915_query.c +6 -10
@@ -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;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/5 i915: switch copy_perf_config_registers_or_number() to unsafe_put_user()
From: Al Viro <[email protected]>
... and the rest of query_perf_config_data() to normal uaccess primitives
--- drivers/gpu/drm/i915/i915_query.c | 46 ++++++++++++++------------------------- drivers/gpu/drm/i915/i915_reg.h | 2 +- 2 files changed, 17 insertions(+), 31 deletions(-)
drivers/gpu/drm/i915/i915_query.c +16 -30
@@ -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 *kernelu64 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; }
drivers/gpu/drm/i915/i915_reg.h +1 -1
@@ -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; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/5 i915 compat ioctl(): just use drm_ioctl_kernel()
In reply to Al ViroFrom: Al Viro <[email protected]>
compat_alloc_user_space() is a bad kludge; the sooner it goes, the better...
--- drivers/gpu/drm/i915/i915_ioc32.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/gpu/drm/i915/i915_ioc32.c +5 -9
@@ -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[] = {
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 4/5 i915: alloc_oa_regs(): get rid of pointless access_ok()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/gpu/drm/i915/i915_perf.c | 3 --- 1 file changed, 3 deletions(-)
drivers/gpu/drm/i915/i915_perf.c +0 -3
@@ -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)
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 5/5 i915:get_engines(): get rid of pointless access_ok()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/gpu/drm/i915/gem/i915_gem_context.c | 5 ----- 1 file changed, 5 deletions(-)
drivers/gpu/drm/i915/gem/i915_gem_context.c +0 -5
@@ -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;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 1/1 uaccess misc
In reply to Al Viro 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(-)
Re: [PATCHES] uaccess misc
Quoted 3 lines · Al Viro
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.
Quoted 2 lines
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
Re: [PATCHES] uaccess misc
Quoted 2 lines · Linus Torvalds
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
Al Viro
maintainerPATCH 1/9 pselect6() and friends: take handling the combined 6th/7th args into helper
In reply to Al ViroFrom: Al Viro <[email protected]>
... and use unsafe_get_user(), while we are at it.
--- fs/select.c | 112 ++++++++++++++++++++++++++++++++++-------------------------- 1 file changed, 64 insertions(+), 48 deletions(-)
fs/select.c +64 -48
@@ -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
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/9 binfmt_elf: don't bother with __{put,copy_to}_user()
From: Al Viro <[email protected]>
--- fs/binfmt_elf.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
fs/binfmt_elf.c +7 -7
@@ -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;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/9 binfmt_elf_fdpic: don't use __... uaccess primitives
In reply to Al ViroFrom: Al Viro <[email protected]>
--- fs/binfmt_elf_fdpic.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-)
fs/binfmt_elf_fdpic.c +19 -12
@@ -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 ; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 4/9 binfmt_flat: don't use __put_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
... and check the return value
--- fs/binfmt_flat.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
fs/binfmt_flat.c +14 -8
@@ -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; } }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 5/9 x86: switch cp_stat64() to unsafe_put_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- arch/x86/kernel/sys_ia32.c | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-)
arch/x86/kernel/sys_ia32.c +22 -18
@@ -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,
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 6/9 TEST_ACCESS_OK _never_ had been checked anywhere
In reply to Al ViroFrom: Al Viro <[email protected]>
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...
--- arch/x86/include/asm/pgtable_32.h | 7 ------- 1 file changed, 7 deletions(-)
arch/x86/include/asm/pgtable_32.h +0 -7
@@ -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
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 7/9 user_regset_copyout_zero(): use clear_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
that's the only caller of __clear_user() in generic code, and it's not hot enough to bother with skipping access_ok().
--- include/linux/regset.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/regset.h +1 -1
@@ -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;
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 8/9 x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
--- arch/x86/kvm/hyperv.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/x86/kvm/hyperv.c +1 -1
@@ -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);
Signature · 1 line
2.11.0
Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
arch/x86/kvm/hyperv.c static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host) · line 1132
Quoted 3 lines · Al Viro
a/arch/x86/kvm/hyperv.carch/x86/kvm/hyperv.c +1
-1-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
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 23 lines · Linus Torvalds
arch/x86/kvm/hyperv.c static int kvm_hv_set_msr(struct kvm_vcpu *vcpu, u32 msr, u64 data, bool host) · line 1132
Quoted 3 lines · Al Viro
a/arch/x86/kvm/hyperv.carch/x86/kvm/hyperv.c +1
-1-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()inkvm_hv_set_msr_pw()in addition to this one inkvm_hv_set_msr(). Both go back to 2011 and commit8b0cedff040b("KVM: use __copy_to_user/__clear_user to write guest page") and both look purely like "pointlessly avoid theaccess_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 inarch/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.
Quoted 7 lines
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.
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 4 lines · Al Viro
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().
Paolo Bonzini
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
In reply to Al ViroQuoted 4 lines · Linus Torvalds
Quoted 3 lines
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
Quoted 3 lines
Quoted 3 lines
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.
Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 3 lines · Paolo Bonzini
Yes, the
access_okis done in__kvm_set_memory_regionandgfn_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
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 5 lines · Linus Torvalds
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().
Quoted 3 lines
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_HYPERCALLarch/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.
Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 3 lines · Al Viro
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:
Quoted 6 lines
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_HYPERCALLarch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32)))
Quoted 6 lines
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
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 21 lines · Linus Torvalds
It really isn't.
Your very first statement shows how broken it is:
Quoted 6 lines
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_HYPERCALLarch/x86/kvm/hyperv.c:1132: if (__clear_user((void __user *)addr, sizeof(u32)))Quoted 6 lines
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.
Quoted 7 lines
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;
Re: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 5 lines · Al Viro
Quoted 2 lines
And none of that code verifies that the end result is a user address.
kvm_is_error_hva()isreturn addr >= PAGE_OFFSET;
Ahh, that's what I missed. It won't work on other architectures, but within x86 it's fine.
Linus
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 9 lines · Linus Torvalds
Quoted 5 lines · Al Viro
Quoted 2 lines
And none of that code verifies that the end result is a user address.
kvm_is_error_hva()isreturn 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.
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 13 lines · Al Viro
Quoted 9 lines · Linus Torvalds
Quoted 5 lines · Al Viro
Quoted 2 lines
And none of that code verifies that the end result is a user address.
kvm_is_error_hva()isreturn 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/kvmon x86, powerpc, mips, s390 and arm64.
Quoted 8 lines
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 - therekvm_is_error_hva()isIS_ERR_VALUE()(thus the "at least on non-mips" upthread). And for s390 it's alsoIS_ERR_VALUE(), but that's an separate can of worms - thereaccess_ok()is constant true; if we ever hit any of that code invirt/kvmwhile underKERNEL_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.
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
In reply to Linus TorvaldsQuoted 7 lines · Linus Torvalds
And I don't understand why you mention
set_fs()vsaccess_ok(). None of this code has anything that messes withset_fs(). Theaccess_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().
Al Viro
maintainerRe: [PATCH 8/9] x86: kvm_hv_set_msr(): use __put_user() instead of 32bit __clear_user()
Quoted 7 lines · Al Viro
Quoted 5 lines · Linus Torvalds
And I don't understand why you mention
set_fs()vsaccess_ok(). None of this code has anything that messes withset_fs(). Theaccess_ok()is garbage and shouldn't exist, and those user accesses should all use the checking versions and the double underscores are wrong.
Quoted 7 lines
Quoted 2 lines
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.
Al Viro
maintainerPATCH 9/9 bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user()
In reply to Al ViroFrom: Al Viro <[email protected]>
... rather than open-coding it, and badly, at that.
--- kernel/bpf/syscall.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
kernel/bpf/syscall.c +6 -19
@@ -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 = {
Signature · 1 line
2.11.0
Re: [PATCH 9/9] bpf: make bpf_check_uarg_tail_zero() use check_zeroed_user()
Quoted 8 lines · Al Viro
From: Al Viro <[email protected]>
... rather than open-coding it, and badly, at that.
Signed-off-by Al Viro <[email protected]>--- kernel/bpf/syscall.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-)
lgtm
Al Viro
maintainerPATCH 1/1 uaccess hpsa
In reply to Al Viro 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(-)
Al Viro
maintainerPATCH 1/4 hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} into hpsa_ioctl()
From: Al Viro <[email protected]>
--- drivers/scsi/hpsa.c | 116 +++++++++++++++++++++++++--------------------------- 1 file changed, 56 insertions(+), 60 deletions(-)
drivers/scsi/hpsa.c +56 -60
@@ -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 &©_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 &©_to_user (argp, ioc,sizeof (*ioc))) + rc = -EFAULT ; +kvfree (ioc);return rc; + }default :return -ENOTTY ; }
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 2/4 hpsa: don't bother with vmalloc for BIG_IOCTL_Command_struct
From: Al Viro <[email protected]>
"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...
--- drivers/scsi/hpsa.c | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-)
drivers/scsi/hpsa.c +5 -9
@@ -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 &©_to_user (argp, ioc,sizeof (*ioc))) +if (!rc &©_to_user (argp, &ioc,sizeof (ioc))) rc = -EFAULT ; -kvfree (ioc);return rc; }default :
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 3/4 hpsa: get rid of compat_alloc_user_space()
In reply to Al ViroFrom: Al Viro <[email protected]>
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.
--- drivers/scsi/hpsa.c | 80 ++++++++++++++++++++++++----------------------------- 1 file changed, 36 insertions(+), 44 deletions(-)
drivers/scsi/hpsa.c +36 -44
@@ -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,
Signature · 1 line
2.11.0
Al Viro
maintainerPATCH 4/4 hpsa_ioctl(): tidy up a bit
In reply to Al ViroFrom: Al Viro <[email protected]>
--- drivers/scsi/hpsa.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/scsi/hpsa.c +2 -5
@@ -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 :
Signature · 1 line
2.11.0
Re: [PATCHES] uaccess hpsa
In reply to Al ViroQuoted 9 lines
hpsa compat ioctl done (hopefully) saner. I really want
to killcompat_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!
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
RE: [PATCHES] uaccess hpsa
In reply to Al Viro-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Al Viro
Sent: Friday, May 29, 2020 6:39 PM
To: Linus Torvalds <[email protected]>
Cc: [email protected]; [email protected]; Don Brace <[email protected]>; [email protected]
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
drivers/scsi/hpsa.c | 199 ++++++++++++++++++++++++---------------------------- 1 file changed, 90 insertions(+), 109 deletions(-)
Al Viro
maintainerRe: [PATCHES] uaccess hpsa
Quoted 11 lines · On Wed, Jun 03, 2020 at 06:37:11PM +0000, [email protected] wrote:
-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Al Viro
Sent: Friday, May 29, 2020 6:39 PM
To: Linus Torvalds <[email protected]>
Cc: [email protected]; [email protected]; Don Brace <[email protected]>; [email protected]
Subject: [PATCHES] uaccess hpsahpsa 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
Quoted 8 lines
Al Viro (4):
hpsa passthrough: lift {BIG_,}IOCTL_Command_struct copy{in,out} intohpsa_ioctl()
hpsa: don't bother with vmalloc forBIG_IOCTL_Command_struct
hpsa: get rid ofcompat_alloc_user_space()
hpsa_ioctl(): tidy up a bitAcked-by Don Brace <[email protected]>Tested-by Don Brace <[email protected]>
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?
Re: [PATCHES] uaccess hpsa
Hi Al!
Quoted 3 lines
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.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Al Viro
maintainerRe: [PATCHES] uaccess hpsa
Quoted 10 lines · Martin K. Petersen
Hi Al!
Quoted 3 lines
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.
Re: [PATCHES] uaccess hpsa
Al,
Quoted 5 lines
Quoted 3 lines
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!
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering