* [RFC][PATCH 02/22] x86 kvm page table walks: switch to explicit __get_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
@ 2020-03-23 18:37 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 03/22] x86: switch sigframe sigset handling to explict __get_user()/__put_user() Al Viro
` (21 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:37 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kvm/mmu/paging_tmpl.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kvm/mmu/paging_tmpl.h b/arch/x86/kvm/mmu/paging_tmpl.h
index 4e1ef0473663..5bea4cfe8a15 100644
--- a/arch/x86/kvm/mmu/paging_tmpl.h
+++ b/arch/x86/kvm/mmu/paging_tmpl.h
@@ -400,7 +400,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
goto error;
ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
- if (unlikely(__copy_from_user(&pte, ptep_user, sizeof(pte))))
+ if (unlikely(__get_user(pte, ptep_user)))
goto error;
walker->ptep_user[walker->level - 1] = ptep_user;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 03/22] x86: switch sigframe sigset handling to explict __get_user()/__put_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
2020-03-23 18:37 ` [RFC][PATCH 02/22] x86 kvm page table walks: " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 04/22] x86: get rid of small constant size cases in raw_copy_{to,from}_user() Al Viro
` (20 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
... and consolidate the definition of sigframe_ia32->extramask - it's
always a 1-element array of 32bit unsigned.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 16 +++++-----------
arch/x86/include/asm/sigframe.h | 6 +-----
arch/x86/kernel/signal.c | 20 ++++++++------------
3 files changed, 14 insertions(+), 28 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index a3aefe9b9401..c72025d615f8 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -126,10 +126,7 @@ COMPAT_SYSCALL_DEFINE0(sigreturn)
if (!access_ok(frame, sizeof(*frame)))
goto badframe;
if (__get_user(set.sig[0], &frame->sc.oldmask)
- || (_COMPAT_NSIG_WORDS > 1
- && __copy_from_user((((char *) &set.sig) + 4),
- &frame->extramask,
- sizeof(frame->extramask))))
+ || __get_user(((__u32 *)&set)[1], &frame->extramask[0]))
goto badframe;
set_current_blocked(&set);
@@ -153,7 +150,7 @@ COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
if (!access_ok(frame, sizeof(*frame)))
goto badframe;
- if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
+ if (__get_user(set.sig[0], (__u64 __user *)&frame->uc.uc_sigmask))
goto badframe;
set_current_blocked(&set);
@@ -277,11 +274,8 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
return -EFAULT;
- if (_COMPAT_NSIG_WORDS > 1) {
- if (__copy_to_user(frame->extramask, &set->sig[1],
- sizeof(frame->extramask)))
- return -EFAULT;
- }
+ if (__put_user(set->sig[1], &frame->extramask[0]))
+ return -EFAULT;
if (ksig->ka.sa.sa_flags & SA_RESTORER) {
restorer = ksig->ka.sa.sa_restorer;
@@ -381,7 +375,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]);
- err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
+ err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
if (err)
return -EFAULT;
diff --git a/arch/x86/include/asm/sigframe.h b/arch/x86/include/asm/sigframe.h
index f176114c04d4..84eab2724875 100644
--- a/arch/x86/include/asm/sigframe.h
+++ b/arch/x86/include/asm/sigframe.h
@@ -33,11 +33,7 @@ struct sigframe_ia32 {
* legacy application accessing/modifying it.
*/
struct _fpstate_32 fpstate_unused;
-#ifdef CONFIG_IA32_EMULATION
- unsigned int extramask[_COMPAT_NSIG_WORDS-1];
-#else /* !CONFIG_IA32_EMULATION */
- unsigned long extramask[_NSIG_WORDS-1];
-#endif /* CONFIG_IA32_EMULATION */
+ unsigned int extramask[1];
char retcode[8];
/* fp state follows here */
};
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 8a29573851a3..53ac66b3fd9b 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -326,11 +326,8 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
return -EFAULT;
- if (_NSIG_WORDS > 1) {
- if (__copy_to_user(&frame->extramask, &set->sig[1],
- sizeof(frame->extramask)))
- return -EFAULT;
- }
+ if (__put_user(set->sig[1], &frame->extramask[0]))
+ return -EFAULT;
if (current->mm->context.vdso)
restorer = current->mm->context.vdso +
@@ -489,7 +486,7 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
} put_user_catch(err);
err |= setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]);
- err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
+ err |= __put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0]);
if (err)
return -EFAULT;
@@ -575,7 +572,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]);
- err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
+ err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
if (err)
return -EFAULT;
@@ -613,9 +610,8 @@ SYSCALL_DEFINE0(sigreturn)
if (!access_ok(frame, sizeof(*frame)))
goto badframe;
- if (__get_user(set.sig[0], &frame->sc.oldmask) || (_NSIG_WORDS > 1
- && __copy_from_user(&set.sig[1], &frame->extramask,
- sizeof(frame->extramask))))
+ if (__get_user(set.sig[0], &frame->sc.oldmask) ||
+ __get_user(set.sig[1], &frame->extramask[0]))
goto badframe;
set_current_blocked(&set);
@@ -645,7 +641,7 @@ SYSCALL_DEFINE0(rt_sigreturn)
frame = (struct rt_sigframe __user *)(regs->sp - sizeof(long));
if (!access_ok(frame, sizeof(*frame)))
goto badframe;
- if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
+ if (__get_user(*(__u64 *)&set, (__u64 __user *)&frame->uc.uc_sigmask))
goto badframe;
if (__get_user(uc_flags, &frame->uc.uc_flags))
goto badframe;
@@ -870,7 +866,7 @@ asmlinkage long sys32_x32_rt_sigreturn(void)
if (!access_ok(frame, sizeof(*frame)))
goto badframe;
- if (__copy_from_user(&set, &frame->uc.uc_sigmask, sizeof(set)))
+ if (__get_user(set.sig[0], (__u64 __user *)&frame->uc.uc_sigmask))
goto badframe;
if (__get_user(uc_flags, &frame->uc.uc_flags))
goto badframe;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 04/22] x86: get rid of small constant size cases in raw_copy_{to,from}_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
2020-03-23 18:37 ` [RFC][PATCH 02/22] x86 kvm page table walks: " Al Viro
2020-03-23 18:38 ` [RFC][PATCH 03/22] x86: switch sigframe sigset handling to explict __get_user()/__put_user() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 05/22] vm86: get rid of get_user_ex() use Al Viro
` (19 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Very few call sites where that would be triggered remain, and none
of those is anywhere near hot enough to bother.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/include/asm/uaccess.h | 12 -----
arch/x86/include/asm/uaccess_32.h | 27 ----------
arch/x86/include/asm/uaccess_64.h | 108 +-------------------------------------
3 files changed, 2 insertions(+), 145 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index ab8eab43a8a2..1cfa33b94a1a 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -378,18 +378,6 @@ do { \
: "=r" (err), ltype(x) \
: "m" (__m(addr)), "i" (errret), "0" (err))
-#define __get_user_asm_nozero(x, addr, err, itype, rtype, ltype, errret) \
- asm volatile("\n" \
- "1: mov"itype" %2,%"rtype"1\n" \
- "2:\n" \
- ".section .fixup,\"ax\"\n" \
- "3: mov %3,%0\n" \
- " jmp 2b\n" \
- ".previous\n" \
- _ASM_EXTABLE_UA(1b, 3b) \
- : "=r" (err), ltype(x) \
- : "m" (__m(addr)), "i" (errret), "0" (err))
-
/*
* This doesn't do __uaccess_begin/end - the exception handling
* around it must do that.
diff --git a/arch/x86/include/asm/uaccess_32.h b/arch/x86/include/asm/uaccess_32.h
index ba2dc1930630..388a40660c7b 100644
--- a/arch/x86/include/asm/uaccess_32.h
+++ b/arch/x86/include/asm/uaccess_32.h
@@ -23,33 +23,6 @@ raw_copy_to_user(void __user *to, const void *from, unsigned long n)
static __always_inline unsigned long
raw_copy_from_user(void *to, const void __user *from, unsigned long n)
{
- if (__builtin_constant_p(n)) {
- unsigned long ret;
-
- switch (n) {
- case 1:
- ret = 0;
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u8 *)to, from, ret,
- "b", "b", "=q", 1);
- __uaccess_end();
- return ret;
- case 2:
- ret = 0;
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u16 *)to, from, ret,
- "w", "w", "=r", 2);
- __uaccess_end();
- return ret;
- case 4:
- ret = 0;
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u32 *)to, from, ret,
- "l", "k", "=r", 4);
- __uaccess_end();
- return ret;
- }
- }
return __copy_user_ll(to, (__force const void *)from, n);
}
diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h
index 5cd1caa8bc65..bc10e3dc64fe 100644
--- a/arch/x86/include/asm/uaccess_64.h
+++ b/arch/x86/include/asm/uaccess_64.h
@@ -65,117 +65,13 @@ copy_to_user_mcsafe(void *to, const void *from, unsigned len)
static __always_inline __must_check unsigned long
raw_copy_from_user(void *dst, const void __user *src, unsigned long size)
{
- int ret = 0;
-
- if (!__builtin_constant_p(size))
- return copy_user_generic(dst, (__force void *)src, size);
- switch (size) {
- case 1:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u8 *)dst, (u8 __user *)src,
- ret, "b", "b", "=q", 1);
- __uaccess_end();
- return ret;
- case 2:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u16 *)dst, (u16 __user *)src,
- ret, "w", "w", "=r", 2);
- __uaccess_end();
- return ret;
- case 4:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u32 *)dst, (u32 __user *)src,
- ret, "l", "k", "=r", 4);
- __uaccess_end();
- return ret;
- case 8:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u64 *)dst, (u64 __user *)src,
- ret, "q", "", "=r", 8);
- __uaccess_end();
- return ret;
- case 10:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u64 *)dst, (u64 __user *)src,
- ret, "q", "", "=r", 10);
- if (likely(!ret))
- __get_user_asm_nozero(*(u16 *)(8 + (char *)dst),
- (u16 __user *)(8 + (char __user *)src),
- ret, "w", "w", "=r", 2);
- __uaccess_end();
- return ret;
- case 16:
- __uaccess_begin_nospec();
- __get_user_asm_nozero(*(u64 *)dst, (u64 __user *)src,
- ret, "q", "", "=r", 16);
- if (likely(!ret))
- __get_user_asm_nozero(*(u64 *)(8 + (char *)dst),
- (u64 __user *)(8 + (char __user *)src),
- ret, "q", "", "=r", 8);
- __uaccess_end();
- return ret;
- default:
- return copy_user_generic(dst, (__force void *)src, size);
- }
+ return copy_user_generic(dst, (__force void *)src, size);
}
static __always_inline __must_check unsigned long
raw_copy_to_user(void __user *dst, const void *src, unsigned long size)
{
- int ret = 0;
-
- if (!__builtin_constant_p(size))
- return copy_user_generic((__force void *)dst, src, size);
- switch (size) {
- case 1:
- __uaccess_begin();
- __put_user_asm(*(u8 *)src, (u8 __user *)dst,
- ret, "b", "b", "iq", 1);
- __uaccess_end();
- return ret;
- case 2:
- __uaccess_begin();
- __put_user_asm(*(u16 *)src, (u16 __user *)dst,
- ret, "w", "w", "ir", 2);
- __uaccess_end();
- return ret;
- case 4:
- __uaccess_begin();
- __put_user_asm(*(u32 *)src, (u32 __user *)dst,
- ret, "l", "k", "ir", 4);
- __uaccess_end();
- return ret;
- case 8:
- __uaccess_begin();
- __put_user_asm(*(u64 *)src, (u64 __user *)dst,
- ret, "q", "", "er", 8);
- __uaccess_end();
- return ret;
- case 10:
- __uaccess_begin();
- __put_user_asm(*(u64 *)src, (u64 __user *)dst,
- ret, "q", "", "er", 10);
- if (likely(!ret)) {
- asm("":::"memory");
- __put_user_asm(4[(u16 *)src], 4 + (u16 __user *)dst,
- ret, "w", "w", "ir", 2);
- }
- __uaccess_end();
- return ret;
- case 16:
- __uaccess_begin();
- __put_user_asm(*(u64 *)src, (u64 __user *)dst,
- ret, "q", "", "er", 16);
- if (likely(!ret)) {
- asm("":::"memory");
- __put_user_asm(1[(u64 *)src], 1 + (u64 __user *)dst,
- ret, "q", "", "er", 8);
- }
- __uaccess_end();
- return ret;
- default:
- return copy_user_generic((__force void *)dst, src, size);
- }
+ return copy_user_generic((__force void *)dst, src, size);
}
static __always_inline __must_check
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 05/22] vm86: get rid of get_user_ex() use
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (2 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 04/22] x86: get rid of small constant size cases in raw_copy_{to,from}_user() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 06/22] x86: get rid of get_user_ex() in ia32_restore_sigcontext() Al Viro
` (18 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Just do a copyin of what we want into a local variable and
be done with that. We are guaranteed to be on shallow stack
here...
Note that conditional expression for range passed to access_ok()
in mainline had been pointless all along - the only difference
between vm86plus_struct and vm86_struct is that the former has
one extra field in the end and when we get to copyin of that
field (conditional upon 'plus' argument), we use copy_from_user().
Moreover, all fields starting with ->int_revectored are copied
that way, so we only need that check (be it done by access_ok()
or by user_access_begin()) only on the beginning of the structure -
the fields that used to be covered by that get_user_try() block.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/vm86_32.c | 54 +++++++++++++++++++++--------------------------
1 file changed, 24 insertions(+), 30 deletions(-)
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 91d55454e702..49b37eb01e99 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -243,6 +243,7 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
struct kernel_vm86_regs vm86regs;
struct pt_regs *regs = current_pt_regs();
unsigned long err = 0;
+ struct vm86_struct v;
err = security_mmap_addr(0);
if (err) {
@@ -278,39 +279,32 @@ static long do_sys_vm86(struct vm86plus_struct __user *user_vm86, bool plus)
if (vm86->saved_sp0)
return -EPERM;
- if (!access_ok(user_vm86, plus ?
- sizeof(struct vm86_struct) :
- sizeof(struct vm86plus_struct)))
+ if (copy_from_user(&v, user_vm86,
+ offsetof(struct vm86_struct, int_revectored)))
return -EFAULT;
memset(&vm86regs, 0, sizeof(vm86regs));
- get_user_try {
- unsigned short seg;
- get_user_ex(vm86regs.pt.bx, &user_vm86->regs.ebx);
- get_user_ex(vm86regs.pt.cx, &user_vm86->regs.ecx);
- get_user_ex(vm86regs.pt.dx, &user_vm86->regs.edx);
- get_user_ex(vm86regs.pt.si, &user_vm86->regs.esi);
- get_user_ex(vm86regs.pt.di, &user_vm86->regs.edi);
- get_user_ex(vm86regs.pt.bp, &user_vm86->regs.ebp);
- get_user_ex(vm86regs.pt.ax, &user_vm86->regs.eax);
- get_user_ex(vm86regs.pt.ip, &user_vm86->regs.eip);
- get_user_ex(seg, &user_vm86->regs.cs);
- vm86regs.pt.cs = seg;
- get_user_ex(vm86regs.pt.flags, &user_vm86->regs.eflags);
- get_user_ex(vm86regs.pt.sp, &user_vm86->regs.esp);
- get_user_ex(seg, &user_vm86->regs.ss);
- vm86regs.pt.ss = seg;
- get_user_ex(vm86regs.es, &user_vm86->regs.es);
- get_user_ex(vm86regs.ds, &user_vm86->regs.ds);
- get_user_ex(vm86regs.fs, &user_vm86->regs.fs);
- get_user_ex(vm86regs.gs, &user_vm86->regs.gs);
-
- get_user_ex(vm86->flags, &user_vm86->flags);
- get_user_ex(vm86->screen_bitmap, &user_vm86->screen_bitmap);
- get_user_ex(vm86->cpu_type, &user_vm86->cpu_type);
- } get_user_catch(err);
- if (err)
- return err;
+
+ vm86regs.pt.bx = v.regs.ebx;
+ vm86regs.pt.cx = v.regs.ecx;
+ vm86regs.pt.dx = v.regs.edx;
+ vm86regs.pt.si = v.regs.esi;
+ vm86regs.pt.di = v.regs.edi;
+ vm86regs.pt.bp = v.regs.ebp;
+ vm86regs.pt.ax = v.regs.eax;
+ vm86regs.pt.ip = v.regs.eip;
+ vm86regs.pt.cs = v.regs.cs;
+ vm86regs.pt.flags = v.regs.eflags;
+ vm86regs.pt.sp = v.regs.esp;
+ vm86regs.pt.ss = v.regs.ss;
+ vm86regs.es = v.regs.es;
+ vm86regs.ds = v.regs.ds;
+ vm86regs.fs = v.regs.fs;
+ vm86regs.gs = v.regs.gs;
+
+ vm86->flags = v.flags;
+ vm86->screen_bitmap = v.screen_bitmap;
+ vm86->cpu_type = v.cpu_type;
if (copy_from_user(&vm86->int_revectored,
&user_vm86->int_revectored,
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 06/22] x86: get rid of get_user_ex() in ia32_restore_sigcontext()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (3 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 05/22] vm86: get rid of get_user_ex() use Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 07/22] x86: get rid of get_user_ex() in restore_sigcontext() Al Viro
` (17 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Just do copyin into a local struct and be done with that - we are
on a shallow stack here.
[reworked by tglx, removing the macro horrors while we are touching that]
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 106 ++++++++++++++++++--------------------------
1 file changed, 44 insertions(+), 62 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index c72025d615f8..23e2c55d8a59 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -36,70 +36,56 @@
#include <asm/sighandling.h>
#include <asm/smap.h>
+static inline void reload_segments(struct sigcontext_32 *sc)
+{
+ unsigned int cur;
+
+ savesegment(gs, cur);
+ if ((sc->gs | 0x03) != cur)
+ load_gs_index(sc->gs | 0x03);
+ savesegment(fs, cur);
+ if ((sc->fs | 0x03) != cur)
+ loadsegment(fs, sc->fs | 0x03);
+ savesegment(ds, cur);
+ if ((sc->ds | 0x03) != cur)
+ loadsegment(ds, sc->ds | 0x03);
+ savesegment(es, cur);
+ if ((sc->es | 0x03) != cur)
+ loadsegment(es, sc->es | 0x03);
+}
+
/*
* Do a signal return; undo the signal stack.
*/
-#define loadsegment_gs(v) load_gs_index(v)
-#define loadsegment_fs(v) loadsegment(fs, v)
-#define loadsegment_ds(v) loadsegment(ds, v)
-#define loadsegment_es(v) loadsegment(es, v)
-
-#define get_user_seg(seg) ({ unsigned int v; savesegment(seg, v); v; })
-#define set_user_seg(seg, v) loadsegment_##seg(v)
-
-#define COPY(x) { \
- get_user_ex(regs->x, &sc->x); \
-}
-
-#define GET_SEG(seg) ({ \
- unsigned short tmp; \
- get_user_ex(tmp, &sc->seg); \
- tmp; \
-})
-
-#define COPY_SEG_CPL3(seg) do { \
- regs->seg = GET_SEG(seg) | 3; \
-} while (0)
-
-#define RELOAD_SEG(seg) { \
- unsigned int pre = (seg) | 3; \
- unsigned int cur = get_user_seg(seg); \
- if (pre != cur) \
- set_user_seg(seg, pre); \
-}
-
static int ia32_restore_sigcontext(struct pt_regs *regs,
- struct sigcontext_32 __user *sc)
+ struct sigcontext_32 __user *usc)
{
- unsigned int tmpflags, err = 0;
- u16 gs, fs, es, ds;
- void __user *buf;
- u32 tmp;
+ struct sigcontext_32 sc;
/* Always make any pending restarted system calls return -EINTR */
current->restart_block.fn = do_no_restart_syscall;
- get_user_try {
- gs = GET_SEG(gs);
- fs = GET_SEG(fs);
- ds = GET_SEG(ds);
- es = GET_SEG(es);
-
- COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
- COPY(dx); COPY(cx); COPY(ip); COPY(ax);
- /* Don't touch extended registers */
-
- COPY_SEG_CPL3(cs);
- COPY_SEG_CPL3(ss);
-
- get_user_ex(tmpflags, &sc->flags);
- regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
- /* disable syscall checks */
- regs->orig_ax = -1;
+ if (unlikely(copy_from_user(&sc, usc, sizeof(sc))))
+ return -EFAULT;
- get_user_ex(tmp, &sc->fpstate);
- buf = compat_ptr(tmp);
- } get_user_catch(err);
+ /* Get only the ia32 registers. */
+ regs->bx = sc.bx;
+ regs->cx = sc.cx;
+ regs->dx = sc.dx;
+ regs->si = sc.si;
+ regs->di = sc.di;
+ regs->bp = sc.bp;
+ regs->ax = sc.ax;
+ regs->sp = sc.sp;
+ regs->ip = sc.ip;
+
+ /* Get CS/SS and force CPL3 */
+ regs->cs = sc.cs | 0x03;
+ regs->ss = sc.ss | 0x03;
+
+ regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
+ /* disable syscall checks */
+ regs->orig_ax = -1;
/*
* Reload fs and gs if they have changed in the signal
@@ -107,14 +93,8 @@ static int ia32_restore_sigcontext(struct pt_regs *regs,
* the handler, but does not clobber them at least in the
* normal case.
*/
- RELOAD_SEG(gs);
- RELOAD_SEG(fs);
- RELOAD_SEG(ds);
- RELOAD_SEG(es);
-
- err |= fpu__restore_sig(buf, 1);
-
- return err;
+ reload_segments(&sc);
+ return fpu__restore_sig(compat_ptr(sc.fpstate), 1);
}
COMPAT_SYSCALL_DEFINE0(sigreturn)
@@ -172,6 +152,8 @@ COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
* Set up a signal frame.
*/
+#define get_user_seg(seg) ({ unsigned int v; savesegment(seg, v); v; })
+
static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
void __user *fpstate,
struct pt_regs *regs, unsigned int mask)
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 07/22] x86: get rid of get_user_ex() in restore_sigcontext()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (4 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 06/22] x86: get rid of get_user_ex() in ia32_restore_sigcontext() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 08/22] x86: kill get_user_{try,catch,ex} Al Viro
` (16 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Just do copyin into a local struct and be done with that - we are
on a shallow stack here.
[reworked by tglx, removing the macro horrors while we are touching that]
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 86 ++++++++++++++++++++----------------------------
1 file changed, 36 insertions(+), 50 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 53ac66b3fd9b..83563e98f0be 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -47,24 +47,6 @@
#include <asm/sigframe.h>
#include <asm/signal.h>
-#define COPY(x) do { \
- get_user_ex(regs->x, &sc->x); \
-} while (0)
-
-#define GET_SEG(seg) ({ \
- unsigned short tmp; \
- get_user_ex(tmp, &sc->seg); \
- tmp; \
-})
-
-#define COPY_SEG(seg) do { \
- regs->seg = GET_SEG(seg); \
-} while (0)
-
-#define COPY_SEG_CPL3(seg) do { \
- regs->seg = GET_SEG(seg) | 3; \
-} while (0)
-
#ifdef CONFIG_X86_64
/*
* If regs->ss will cause an IRET fault, change it. Otherwise leave it
@@ -92,53 +74,58 @@ static void force_valid_ss(struct pt_regs *regs)
ar != (AR_DPL3 | AR_S | AR_P | AR_TYPE_RWDATA_EXPDOWN))
regs->ss = __USER_DS;
}
+# define CONTEXT_COPY_SIZE offsetof(struct sigcontext, reserved1)
+#else
+# define CONTEXT_COPY_SIZE sizeof(struct sigcontext)
#endif
static int restore_sigcontext(struct pt_regs *regs,
- struct sigcontext __user *sc,
+ struct sigcontext __user *usc,
unsigned long uc_flags)
{
- unsigned long buf_val;
- void __user *buf;
- unsigned int tmpflags;
- unsigned int err = 0;
+ struct sigcontext sc;
/* Always make any pending restarted system calls return -EINTR */
current->restart_block.fn = do_no_restart_syscall;
- get_user_try {
+ if (copy_from_user(&sc, usc, CONTEXT_COPY_SIZE))
+ return -EFAULT;
#ifdef CONFIG_X86_32
- set_user_gs(regs, GET_SEG(gs));
- COPY_SEG(fs);
- COPY_SEG(es);
- COPY_SEG(ds);
+ set_user_gs(regs, sc.gs);
+ regs->fs = sc.fs;
+ regs->es = sc.es;
+ regs->ds = sc.ds;
#endif /* CONFIG_X86_32 */
- COPY(di); COPY(si); COPY(bp); COPY(sp); COPY(bx);
- COPY(dx); COPY(cx); COPY(ip); COPY(ax);
+ regs->bx = sc.bx;
+ regs->cx = sc.cx;
+ regs->dx = sc.dx;
+ regs->si = sc.si;
+ regs->di = sc.di;
+ regs->bp = sc.bp;
+ regs->ax = sc.ax;
+ regs->sp = sc.sp;
+ regs->ip = sc.ip;
#ifdef CONFIG_X86_64
- COPY(r8);
- COPY(r9);
- COPY(r10);
- COPY(r11);
- COPY(r12);
- COPY(r13);
- COPY(r14);
- COPY(r15);
+ regs->r8 = sc.r8;
+ regs->r9 = sc.r9;
+ regs->r10 = sc.r10;
+ regs->r11 = sc.r11;
+ regs->r12 = sc.r12;
+ regs->r13 = sc.r13;
+ regs->r14 = sc.r14;
+ regs->r15 = sc.r15;
#endif /* CONFIG_X86_64 */
- COPY_SEG_CPL3(cs);
- COPY_SEG_CPL3(ss);
-
- get_user_ex(tmpflags, &sc->flags);
- regs->flags = (regs->flags & ~FIX_EFLAGS) | (tmpflags & FIX_EFLAGS);
- regs->orig_ax = -1; /* disable syscall checks */
+ /* Get CS/SS and force CPL3 */
+ regs->cs = sc.cs | 0x03;
+ regs->ss = sc.ss | 0x03;
- get_user_ex(buf_val, &sc->fpstate);
- buf = (void __user *)buf_val;
- } get_user_catch(err);
+ regs->flags = (regs->flags & ~FIX_EFLAGS) | (sc.flags & FIX_EFLAGS);
+ /* disable syscall checks */
+ regs->orig_ax = -1;
#ifdef CONFIG_X86_64
/*
@@ -149,9 +136,8 @@ static int restore_sigcontext(struct pt_regs *regs,
force_valid_ss(regs);
#endif
- err |= fpu__restore_sig(buf, IS_ENABLED(CONFIG_X86_32));
-
- return err;
+ return fpu__restore_sig((void __user *)sc.fpstate,
+ IS_ENABLED(CONFIG_X86_32));
}
int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 08/22] x86: kill get_user_{try,catch,ex}
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (5 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 07/22] x86: get rid of get_user_ex() in restore_sigcontext() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 09/22] x86: switch save_v86_state() to unsafe_put_user() Al Viro
` (15 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
no users left
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/include/asm/uaccess.h | 54 ------------------------------------------
1 file changed, 54 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 1cfa33b94a1a..4dc5accdd826 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -335,12 +335,9 @@ do { \
"i" (errret), "0" (retval)); \
})
-#define __get_user_asm_ex_u64(x, ptr) (x) = __get_user_bad()
#else
#define __get_user_asm_u64(x, ptr, retval, errret) \
__get_user_asm(x, ptr, retval, "q", "", "=r", errret)
-#define __get_user_asm_ex_u64(x, ptr) \
- __get_user_asm_ex(x, ptr, "q", "", "=r")
#endif
#define __get_user_size(x, ptr, size, retval, errret) \
@@ -378,41 +375,6 @@ do { \
: "=r" (err), ltype(x) \
: "m" (__m(addr)), "i" (errret), "0" (err))
-/*
- * This doesn't do __uaccess_begin/end - the exception handling
- * around it must do that.
- */
-#define __get_user_size_ex(x, ptr, size) \
-do { \
- __chk_user_ptr(ptr); \
- switch (size) { \
- case 1: \
- __get_user_asm_ex(x, ptr, "b", "b", "=q"); \
- break; \
- case 2: \
- __get_user_asm_ex(x, ptr, "w", "w", "=r"); \
- break; \
- case 4: \
- __get_user_asm_ex(x, ptr, "l", "k", "=r"); \
- break; \
- case 8: \
- __get_user_asm_ex_u64(x, ptr); \
- break; \
- default: \
- (x) = __get_user_bad(); \
- } \
-} while (0)
-
-#define __get_user_asm_ex(x, addr, itype, rtype, ltype) \
- asm volatile("1: mov"itype" %1,%"rtype"0\n" \
- "2:\n" \
- ".section .fixup,\"ax\"\n" \
- "3:xor"itype" %"rtype"0,%"rtype"0\n" \
- " jmp 2b\n" \
- ".previous\n" \
- _ASM_EXTABLE_EX(1b, 3b) \
- : ltype(x) : "m" (__m(addr)))
-
#define __put_user_nocheck(x, ptr, size) \
({ \
__label__ __pu_label; \
@@ -540,22 +502,6 @@ struct __large_struct { unsigned long buf[100]; };
#define __put_user(x, ptr) \
__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-/*
- * {get|put}_user_try and catch
- *
- * get_user_try {
- * get_user_ex(...);
- * } get_user_catch(err)
- */
-#define get_user_try uaccess_try_nospec
-#define get_user_catch(err) uaccess_catch(err)
-
-#define get_user_ex(x, ptr) do { \
- unsigned long __gue_val; \
- __get_user_size_ex((__gue_val), (ptr), (sizeof(*(ptr)))); \
- (x) = (__force __typeof__(*(ptr)))__gue_val; \
-} while (0)
-
#define put_user_try uaccess_try
#define put_user_catch(err) uaccess_catch(err)
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 09/22] x86: switch save_v86_state() to unsafe_put_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (6 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 08/22] x86: kill get_user_{try,catch,ex} Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 10/22] x86: switch setup_sigcontext() " Al Viro
` (14 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/vm86_32.c | 61 +++++++++++++++++++++++------------------------
1 file changed, 30 insertions(+), 31 deletions(-)
diff --git a/arch/x86/kernel/vm86_32.c b/arch/x86/kernel/vm86_32.c
index 49b37eb01e99..47a8676c7395 100644
--- a/arch/x86/kernel/vm86_32.c
+++ b/arch/x86/kernel/vm86_32.c
@@ -98,7 +98,6 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
struct task_struct *tsk = current;
struct vm86plus_struct __user *user;
struct vm86 *vm86 = current->thread.vm86;
- long err = 0;
/*
* This gets called from entry.S with interrupts disabled, but
@@ -114,37 +113,30 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
set_flags(regs->pt.flags, VEFLAGS, X86_EFLAGS_VIF | vm86->veflags_mask);
user = vm86->user_vm86;
- if (!access_ok(user, vm86->vm86plus.is_vm86pus ?
+ if (!user_access_begin(user, vm86->vm86plus.is_vm86pus ?
sizeof(struct vm86plus_struct) :
- sizeof(struct vm86_struct))) {
- pr_alert("could not access userspace vm86 info\n");
- do_exit(SIGSEGV);
- }
-
- put_user_try {
- put_user_ex(regs->pt.bx, &user->regs.ebx);
- put_user_ex(regs->pt.cx, &user->regs.ecx);
- put_user_ex(regs->pt.dx, &user->regs.edx);
- put_user_ex(regs->pt.si, &user->regs.esi);
- put_user_ex(regs->pt.di, &user->regs.edi);
- put_user_ex(regs->pt.bp, &user->regs.ebp);
- put_user_ex(regs->pt.ax, &user->regs.eax);
- put_user_ex(regs->pt.ip, &user->regs.eip);
- put_user_ex(regs->pt.cs, &user->regs.cs);
- put_user_ex(regs->pt.flags, &user->regs.eflags);
- put_user_ex(regs->pt.sp, &user->regs.esp);
- put_user_ex(regs->pt.ss, &user->regs.ss);
- put_user_ex(regs->es, &user->regs.es);
- put_user_ex(regs->ds, &user->regs.ds);
- put_user_ex(regs->fs, &user->regs.fs);
- put_user_ex(regs->gs, &user->regs.gs);
-
- put_user_ex(vm86->screen_bitmap, &user->screen_bitmap);
- } put_user_catch(err);
- if (err) {
- pr_alert("could not access userspace vm86 info\n");
- do_exit(SIGSEGV);
- }
+ sizeof(struct vm86_struct)))
+ goto Efault;
+
+ unsafe_put_user(regs->pt.bx, &user->regs.ebx, Efault_end);
+ unsafe_put_user(regs->pt.cx, &user->regs.ecx, Efault_end);
+ unsafe_put_user(regs->pt.dx, &user->regs.edx, Efault_end);
+ unsafe_put_user(regs->pt.si, &user->regs.esi, Efault_end);
+ unsafe_put_user(regs->pt.di, &user->regs.edi, Efault_end);
+ unsafe_put_user(regs->pt.bp, &user->regs.ebp, Efault_end);
+ unsafe_put_user(regs->pt.ax, &user->regs.eax, Efault_end);
+ unsafe_put_user(regs->pt.ip, &user->regs.eip, Efault_end);
+ unsafe_put_user(regs->pt.cs, &user->regs.cs, Efault_end);
+ unsafe_put_user(regs->pt.flags, &user->regs.eflags, Efault_end);
+ unsafe_put_user(regs->pt.sp, &user->regs.esp, Efault_end);
+ unsafe_put_user(regs->pt.ss, &user->regs.ss, Efault_end);
+ unsafe_put_user(regs->es, &user->regs.es, Efault_end);
+ unsafe_put_user(regs->ds, &user->regs.ds, Efault_end);
+ unsafe_put_user(regs->fs, &user->regs.fs, Efault_end);
+ unsafe_put_user(regs->gs, &user->regs.gs, Efault_end);
+ unsafe_put_user(vm86->screen_bitmap, &user->screen_bitmap, Efault_end);
+
+ user_access_end();
preempt_disable();
tsk->thread.sp0 = vm86->saved_sp0;
@@ -159,6 +151,13 @@ void save_v86_state(struct kernel_vm86_regs *regs, int retval)
lazy_load_gs(vm86->regs32.gs);
regs->pt.ax = retval;
+ return;
+
+Efault_end:
+ user_access_end();
+Efault:
+ pr_alert("could not access userspace vm86 info\n");
+ do_exit(SIGSEGV);
}
static void mark_screen_rdonly(struct mm_struct *mm)
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 10/22] x86: switch setup_sigcontext() to unsafe_put_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (7 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 09/22] x86: switch save_v86_state() to unsafe_put_user() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 11/22] x86: switch ia32_setup_sigcontext() " Al Viro
` (13 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/include/asm/sighandling.h | 3 --
arch/x86/kernel/signal.c | 88 +++++++++++++++++++-------------------
2 files changed, 45 insertions(+), 46 deletions(-)
diff --git a/arch/x86/include/asm/sighandling.h b/arch/x86/include/asm/sighandling.h
index 2fcbd6f33ef7..35e0b579ffcb 100644
--- a/arch/x86/include/asm/sighandling.h
+++ b/arch/x86/include/asm/sighandling.h
@@ -14,9 +14,6 @@
X86_EFLAGS_CF | X86_EFLAGS_RF)
void signal_fault(struct pt_regs *regs, void __user *frame, char *where);
-int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
- struct pt_regs *regs, unsigned long mask);
-
#ifdef CONFIG_X86_X32_ABI
asmlinkage long sys32_x32_rt_sigreturn(void);
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 83563e98f0be..3b4ca484cfc2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -140,63 +140,65 @@ static int restore_sigcontext(struct pt_regs *regs,
IS_ENABLED(CONFIG_X86_32));
}
-int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
+static int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
struct pt_regs *regs, unsigned long mask)
{
- int err = 0;
-
- put_user_try {
+ if (!user_access_begin(sc, sizeof(struct sigcontext)))
+ return -EFAULT;
#ifdef CONFIG_X86_32
- put_user_ex(get_user_gs(regs), (unsigned int __user *)&sc->gs);
- put_user_ex(regs->fs, (unsigned int __user *)&sc->fs);
- put_user_ex(regs->es, (unsigned int __user *)&sc->es);
- put_user_ex(regs->ds, (unsigned int __user *)&sc->ds);
+ unsafe_put_user(get_user_gs(regs),
+ (unsigned int __user *)&sc->gs, Efault);
+ unsafe_put_user(regs->fs, (unsigned int __user *)&sc->fs, Efault);
+ unsafe_put_user(regs->es, (unsigned int __user *)&sc->es, Efault);
+ unsafe_put_user(regs->ds, (unsigned int __user *)&sc->ds, Efault);
#endif /* CONFIG_X86_32 */
- put_user_ex(regs->di, &sc->di);
- put_user_ex(regs->si, &sc->si);
- put_user_ex(regs->bp, &sc->bp);
- put_user_ex(regs->sp, &sc->sp);
- put_user_ex(regs->bx, &sc->bx);
- put_user_ex(regs->dx, &sc->dx);
- put_user_ex(regs->cx, &sc->cx);
- put_user_ex(regs->ax, &sc->ax);
+ unsafe_put_user(regs->di, &sc->di, Efault);
+ unsafe_put_user(regs->si, &sc->si, Efault);
+ unsafe_put_user(regs->bp, &sc->bp, Efault);
+ unsafe_put_user(regs->sp, &sc->sp, Efault);
+ unsafe_put_user(regs->bx, &sc->bx, Efault);
+ unsafe_put_user(regs->dx, &sc->dx, Efault);
+ unsafe_put_user(regs->cx, &sc->cx, Efault);
+ unsafe_put_user(regs->ax, &sc->ax, Efault);
#ifdef CONFIG_X86_64
- put_user_ex(regs->r8, &sc->r8);
- put_user_ex(regs->r9, &sc->r9);
- put_user_ex(regs->r10, &sc->r10);
- put_user_ex(regs->r11, &sc->r11);
- put_user_ex(regs->r12, &sc->r12);
- put_user_ex(regs->r13, &sc->r13);
- put_user_ex(regs->r14, &sc->r14);
- put_user_ex(regs->r15, &sc->r15);
+ unsafe_put_user(regs->r8, &sc->r8, Efault);
+ unsafe_put_user(regs->r9, &sc->r9, Efault);
+ unsafe_put_user(regs->r10, &sc->r10, Efault);
+ unsafe_put_user(regs->r11, &sc->r11, Efault);
+ unsafe_put_user(regs->r12, &sc->r12, Efault);
+ unsafe_put_user(regs->r13, &sc->r13, Efault);
+ unsafe_put_user(regs->r14, &sc->r14, Efault);
+ unsafe_put_user(regs->r15, &sc->r15, Efault);
#endif /* CONFIG_X86_64 */
- put_user_ex(current->thread.trap_nr, &sc->trapno);
- put_user_ex(current->thread.error_code, &sc->err);
- put_user_ex(regs->ip, &sc->ip);
+ unsafe_put_user(current->thread.trap_nr, &sc->trapno, Efault);
+ unsafe_put_user(current->thread.error_code, &sc->err, Efault);
+ unsafe_put_user(regs->ip, &sc->ip, Efault);
#ifdef CONFIG_X86_32
- put_user_ex(regs->cs, (unsigned int __user *)&sc->cs);
- put_user_ex(regs->flags, &sc->flags);
- put_user_ex(regs->sp, &sc->sp_at_signal);
- put_user_ex(regs->ss, (unsigned int __user *)&sc->ss);
+ unsafe_put_user(regs->cs, (unsigned int __user *)&sc->cs, Efault);
+ unsafe_put_user(regs->flags, &sc->flags, Efault);
+ unsafe_put_user(regs->sp, &sc->sp_at_signal, Efault);
+ unsafe_put_user(regs->ss, (unsigned int __user *)&sc->ss, Efault);
#else /* !CONFIG_X86_32 */
- put_user_ex(regs->flags, &sc->flags);
- put_user_ex(regs->cs, &sc->cs);
- put_user_ex(0, &sc->gs);
- put_user_ex(0, &sc->fs);
- put_user_ex(regs->ss, &sc->ss);
+ unsafe_put_user(regs->flags, &sc->flags, Efault);
+ unsafe_put_user(regs->cs, &sc->cs, Efault);
+ unsafe_put_user(0, &sc->gs, Efault);
+ unsafe_put_user(0, &sc->fs, Efault);
+ unsafe_put_user(regs->ss, &sc->ss, Efault);
#endif /* CONFIG_X86_32 */
- put_user_ex(fpstate, (unsigned long __user *)&sc->fpstate);
+ unsafe_put_user(fpstate, (unsigned long __user *)&sc->fpstate, Efault);
- /* non-iBCS2 extensions.. */
- put_user_ex(mask, &sc->oldmask);
- put_user_ex(current->thread.cr2, &sc->cr2);
- } put_user_catch(err);
-
- return err;
+ /* non-iBCS2 extensions.. */
+ unsafe_put_user(mask, &sc->oldmask, Efault);
+ unsafe_put_user(current->thread.cr2, &sc->cr2, Efault);
+ user_access_end();
+ return 0;
+Efault:
+ user_access_end();
+ return -EFAULT;
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 11/22] x86: switch ia32_setup_sigcontext() to unsafe_put_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (8 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 10/22] x86: switch setup_sigcontext() " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 12/22] x86: get rid of put_user_try in {ia32,x32}_setup_rt_frame() Al Viro
` (12 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 64 +++++++++++++++++++++++----------------------
1 file changed, 33 insertions(+), 31 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 23e2c55d8a59..af673ec23a2d 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -158,38 +158,40 @@ static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
void __user *fpstate,
struct pt_regs *regs, unsigned int mask)
{
- int err = 0;
-
- put_user_try {
- put_user_ex(get_user_seg(gs), (unsigned int __user *)&sc->gs);
- put_user_ex(get_user_seg(fs), (unsigned int __user *)&sc->fs);
- put_user_ex(get_user_seg(ds), (unsigned int __user *)&sc->ds);
- put_user_ex(get_user_seg(es), (unsigned int __user *)&sc->es);
-
- put_user_ex(regs->di, &sc->di);
- put_user_ex(regs->si, &sc->si);
- put_user_ex(regs->bp, &sc->bp);
- put_user_ex(regs->sp, &sc->sp);
- put_user_ex(regs->bx, &sc->bx);
- put_user_ex(regs->dx, &sc->dx);
- put_user_ex(regs->cx, &sc->cx);
- put_user_ex(regs->ax, &sc->ax);
- put_user_ex(current->thread.trap_nr, &sc->trapno);
- put_user_ex(current->thread.error_code, &sc->err);
- put_user_ex(regs->ip, &sc->ip);
- put_user_ex(regs->cs, (unsigned int __user *)&sc->cs);
- put_user_ex(regs->flags, &sc->flags);
- put_user_ex(regs->sp, &sc->sp_at_signal);
- put_user_ex(regs->ss, (unsigned int __user *)&sc->ss);
-
- put_user_ex(ptr_to_compat(fpstate), &sc->fpstate);
-
- /* non-iBCS2 extensions.. */
- put_user_ex(mask, &sc->oldmask);
- put_user_ex(current->thread.cr2, &sc->cr2);
- } put_user_catch(err);
+ if (!user_access_begin(sc, sizeof(struct sigcontext_32)))
+ return -EFAULT;
- return err;
+ unsafe_put_user(get_user_seg(gs), (unsigned int __user *)&sc->gs, Efault);
+ unsafe_put_user(get_user_seg(fs), (unsigned int __user *)&sc->fs, Efault);
+ unsafe_put_user(get_user_seg(ds), (unsigned int __user *)&sc->ds, Efault);
+ unsafe_put_user(get_user_seg(es), (unsigned int __user *)&sc->es, Efault);
+
+ unsafe_put_user(regs->di, &sc->di, Efault);
+ unsafe_put_user(regs->si, &sc->si, Efault);
+ unsafe_put_user(regs->bp, &sc->bp, Efault);
+ unsafe_put_user(regs->sp, &sc->sp, Efault);
+ unsafe_put_user(regs->bx, &sc->bx, Efault);
+ unsafe_put_user(regs->dx, &sc->dx, Efault);
+ unsafe_put_user(regs->cx, &sc->cx, Efault);
+ unsafe_put_user(regs->ax, &sc->ax, Efault);
+ unsafe_put_user(current->thread.trap_nr, &sc->trapno, Efault);
+ unsafe_put_user(current->thread.error_code, &sc->err, Efault);
+ unsafe_put_user(regs->ip, &sc->ip, Efault);
+ unsafe_put_user(regs->cs, (unsigned int __user *)&sc->cs, Efault);
+ unsafe_put_user(regs->flags, &sc->flags, Efault);
+ unsafe_put_user(regs->sp, &sc->sp_at_signal, Efault);
+ unsafe_put_user(regs->ss, (unsigned int __user *)&sc->ss, Efault);
+
+ unsafe_put_user(ptr_to_compat(fpstate), &sc->fpstate, Efault);
+
+ /* non-iBCS2 extensions.. */
+ unsafe_put_user(mask, &sc->oldmask, Efault);
+ unsafe_put_user(current->thread.cr2, &sc->cr2, Efault);
+ user_access_end();
+ return 0;
+Efault:
+ user_access_end();
+ return -EFAULT;
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 12/22] x86: get rid of put_user_try in {ia32,x32}_setup_rt_frame()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (9 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 11/22] x86: switch ia32_setup_sigcontext() " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers Al Viro
` (11 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Straightforward, except for compat_save_altstack_ex() stuck in those.
Replace that thing with an analogue that would use unsafe_put_user()
instead of put_user_ex() (called unsafe_compat_save_altstack()) and
be done with that...
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 50 +++++++++++++++++++++++----------------------
arch/x86/kernel/signal.c | 33 ++++++++++++++++--------------
include/linux/compat.h | 9 ++++----
3 files changed, 49 insertions(+), 43 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index af673ec23a2d..a96995aa23da 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -326,35 +326,34 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
frame = get_sigframe(ksig, regs, sizeof(*frame), &fpstate);
- if (!access_ok(frame, sizeof(*frame)))
+ if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
- put_user_try {
- put_user_ex(sig, &frame->sig);
- put_user_ex(ptr_to_compat(&frame->info), &frame->pinfo);
- put_user_ex(ptr_to_compat(&frame->uc), &frame->puc);
+ unsafe_put_user(sig, &frame->sig, Efault);
+ unsafe_put_user(ptr_to_compat(&frame->info), &frame->pinfo, Efault);
+ unsafe_put_user(ptr_to_compat(&frame->uc), &frame->puc, Efault);
- /* Create the ucontext. */
- if (static_cpu_has(X86_FEATURE_XSAVE))
- put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
- else
- put_user_ex(0, &frame->uc.uc_flags);
- put_user_ex(0, &frame->uc.uc_link);
- compat_save_altstack_ex(&frame->uc.uc_stack, regs->sp);
+ /* Create the ucontext. */
+ if (static_cpu_has(X86_FEATURE_XSAVE))
+ unsafe_put_user(UC_FP_XSTATE, &frame->uc.uc_flags, Efault);
+ else
+ unsafe_put_user(0, &frame->uc.uc_flags, Efault);
+ unsafe_put_user(0, &frame->uc.uc_link, Efault);
+ unsafe_compat_save_altstack(&frame->uc.uc_stack, regs->sp, Efault);
- if (ksig->ka.sa.sa_flags & SA_RESTORER)
- restorer = ksig->ka.sa.sa_restorer;
- else
- restorer = current->mm->context.vdso +
- vdso_image_32.sym___kernel_rt_sigreturn;
- put_user_ex(ptr_to_compat(restorer), &frame->pretcode);
+ if (ksig->ka.sa.sa_flags & SA_RESTORER)
+ restorer = ksig->ka.sa.sa_restorer;
+ else
+ restorer = current->mm->context.vdso +
+ vdso_image_32.sym___kernel_rt_sigreturn;
+ unsafe_put_user(ptr_to_compat(restorer), &frame->pretcode, Efault);
- /*
- * Not actually used anymore, but left because some gdb
- * versions need it.
- */
- put_user_ex(*((u64 *)&code), (u64 __user *)frame->retcode);
- } put_user_catch(err);
+ /*
+ * Not actually used anymore, but left because some gdb
+ * versions need it.
+ */
+ unsafe_put_user(*((u64 *)&code), (u64 __user *)frame->retcode, Efault);
+ user_access_end();
err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
@@ -380,4 +379,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
regs->ss = __USER32_DS;
return 0;
+Efault:
+ user_access_end();
+ return -EFAULT;
}
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 3b4ca484cfc2..29abad29aaa1 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -529,6 +529,9 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
int err = 0;
void __user *fpstate = NULL;
+ if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
+ return -EFAULT;
+
frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
if (!access_ok(frame, sizeof(*frame)))
@@ -541,22 +544,17 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
uc_flags = frame_uc_flags(regs);
- put_user_try {
- /* Create the ucontext. */
- put_user_ex(uc_flags, &frame->uc.uc_flags);
- put_user_ex(0, &frame->uc.uc_link);
- compat_save_altstack_ex(&frame->uc.uc_stack, regs->sp);
- put_user_ex(0, &frame->uc.uc__pad0);
+ if (!user_access_begin(frame, sizeof(*frame)))
+ return -EFAULT;
- if (ksig->ka.sa.sa_flags & SA_RESTORER) {
- restorer = ksig->ka.sa.sa_restorer;
- } else {
- /* could use a vstub here */
- restorer = NULL;
- err |= -EFAULT;
- }
- put_user_ex(restorer, (unsigned long __user *)&frame->pretcode);
- } put_user_catch(err);
+ /* Create the ucontext. */
+ unsafe_put_user(uc_flags, &frame->uc.uc_flags, Efault);
+ unsafe_put_user(0, &frame->uc.uc_link, Efault);
+ unsafe_compat_save_altstack(&frame->uc.uc_stack, regs->sp, Efault);
+ unsafe_put_user(0, &frame->uc.uc__pad0, Efault);
+ restorer = ksig->ka.sa.sa_restorer;
+ unsafe_put_user(restorer, (unsigned long __user *)&frame->pretcode, Efault);
+ user_access_end();
err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]);
@@ -582,6 +580,11 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
#endif /* CONFIG_X86_X32_ABI */
return 0;
+#ifdef CONFIG_X86_X32_ABI
+Efault:
+ user_access_end();
+ return -EFAULT;
+#endif
}
/*
diff --git a/include/linux/compat.h b/include/linux/compat.h
index 11083d84eb23..224ecb4fffd4 100644
--- a/include/linux/compat.h
+++ b/include/linux/compat.h
@@ -483,12 +483,13 @@ extern void __user *compat_alloc_user_space(unsigned long len);
int compat_restore_altstack(const compat_stack_t __user *uss);
int __compat_save_altstack(compat_stack_t __user *, unsigned long);
-#define compat_save_altstack_ex(uss, sp) do { \
+#define unsafe_compat_save_altstack(uss, sp, label) do { \
compat_stack_t __user *__uss = uss; \
struct task_struct *t = current; \
- put_user_ex(ptr_to_compat((void __user *)t->sas_ss_sp), &__uss->ss_sp); \
- put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
- put_user_ex(t->sas_ss_size, &__uss->ss_size); \
+ unsafe_put_user(ptr_to_compat((void __user *)t->sas_ss_sp), \
+ &__uss->ss_sp, label); \
+ unsafe_put_user(t->sas_ss_flags, &__uss->ss_flags, label); \
+ unsafe_put_user(t->sas_ss_size, &__uss->ss_size, label); \
if (t->sas_ss_flags & SS_AUTODISARM) \
sas_ss_reset(t); \
} while (0);
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (10 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 12/22] x86: get rid of put_user_try in {ia32,x32}_setup_rt_frame() Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:53 ` Linus Torvalds
2020-03-23 18:38 ` [RFC][PATCH 14/22] x86: ia32_setup_frame(): consolidate uaccess areas Al Viro
` (10 subsequent siblings)
22 siblings, 1 reply; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
What's left is just a sequence of stores to userland addresses, with all
error handling, etc. done out of line. Calling that from user_access block
is safe, but rather than teaching objtool to recognize it as such we can
just make it always_inline - it is small enough and has few enough callers,
for the space savings not to be an issue.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index a96995aa23da..aedf9929f2b9 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -154,13 +154,10 @@ COMPAT_SYSCALL_DEFINE0(rt_sigreturn)
#define get_user_seg(seg) ({ unsigned int v; savesegment(seg, v); v; })
-static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
+static __always_inline int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
void __user *fpstate,
struct pt_regs *regs, unsigned int mask)
{
- if (!user_access_begin(sc, sizeof(struct sigcontext_32)))
- return -EFAULT;
-
unsafe_put_user(get_user_seg(gs), (unsigned int __user *)&sc->gs, Efault);
unsafe_put_user(get_user_seg(fs), (unsigned int __user *)&sc->fs, Efault);
unsafe_put_user(get_user_seg(ds), (unsigned int __user *)&sc->ds, Efault);
@@ -187,10 +184,9 @@ static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
/* non-iBCS2 extensions.. */
unsafe_put_user(mask, &sc->oldmask, Efault);
unsafe_put_user(current->thread.cr2, &sc->cr2, Efault);
- user_access_end();
return 0;
+
Efault:
- user_access_end();
return -EFAULT;
}
@@ -255,9 +251,13 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
if (__put_user(sig, &frame->sig))
return -EFAULT;
- if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
+ if (!user_access_begin(&frame->sc, sizeof(struct sigcontext_32)))
return -EFAULT;
+ if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
+ goto Efault;
+ user_access_end();
+
if (__put_user(set->sig[1], &frame->extramask[0]))
return -EFAULT;
@@ -301,6 +301,9 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
regs->ss = __USER32_DS;
return 0;
+Efault:
+ user_access_end();
+ return -EFAULT;
}
int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
@@ -356,8 +359,11 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
user_access_end();
err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
+ if (!user_access_begin(&frame->uc.uc_mcontext, sizeof(struct sigcontext_32)))
+ return -EFAULT;
err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]);
+ user_access_end();
err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
if (err)
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers
2020-03-23 18:38 ` [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers Al Viro
@ 2020-03-23 18:53 ` Linus Torvalds
2020-03-23 21:42 ` Al Viro
0 siblings, 1 reply; 71+ messages in thread
From: Linus Torvalds @ 2020-03-23 18:53 UTC (permalink / raw)
To: Al Viro
Cc: Thomas Gleixner, the arch/x86 maintainers, Linux Kernel Mailing List
On Mon, Mar 23, 2020 at 11:39 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> -static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> +static __always_inline int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
Please rename this at the same time (to "unsafe_ia32_setup_sigcontext()").
I absolutely _hate_ how we have historically split the "__get_user()"
calls from the "access_ok()" calls, and then have had bugs when we had
ways to reach the user access without checking it.
Yes, we have static checking for the unsafe stuff in objtool now, but
I still want this to be explicit on the source level too: if you do
unsafe user accesses, you make it very very explicit in the naming, so
that you can't possibly even by mistake have a "let's call this
function withou having done the user_access_begin()" calls.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers
2020-03-23 18:53 ` Linus Torvalds
@ 2020-03-23 21:42 ` Al Viro
0 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 21:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Gleixner, the arch/x86 maintainers, Linux Kernel Mailing List
On Mon, Mar 23, 2020 at 11:53:39AM -0700, Linus Torvalds wrote:
> On Mon, Mar 23, 2020 at 11:39 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > -static int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
> > +static __always_inline int ia32_setup_sigcontext(struct sigcontext_32 __user *sc,
>
> Please rename this at the same time (to "unsafe_ia32_setup_sigcontext()").
>
> I absolutely _hate_ how we have historically split the "__get_user()"
> calls from the "access_ok()" calls, and then have had bugs when we had
> ways to reach the user access without checking it.
>
> Yes, we have static checking for the unsafe stuff in objtool now, but
> I still want this to be explicit on the source level too: if you do
> unsafe user accesses, you make it very very explicit in the naming, so
> that you can't possibly even by mistake have a "let's call this
> function withou having done the user_access_begin()" calls.
Umm... OK, but I wonder if unsafe_... makes the right naming conventions
for such cases. Note that towards the end of that series we get
#define unsafe_put_sigcontext(sc, fpstate, regs, set, label) \
do { \
if (setup_sigcontext(sc, fpstate, regs, set->sig[0])) \
goto label; \
} while(0);
and that's not an uncommon pattern. We generally have unsafe_...
mean "doesn't return anything, takes a label, needs to be called
from under user_access_begin" and I suspect that it would make sense
to have another recognizable naming pattern for "it must be called
from under user_access_begin() and you need to look at return value".
In this case we could grit teeth and turn that sucker into a macro.
But what about e.g. lifting user_access_{begin,end}() out of
raw_copy_from_user()? unsafe_copy_from_user() would imply
"all or nothing" kind of calling conventions, like e.g.
unsafe_copy_to_user() currently does. Which is fine in some
situations, and it's a good helper to have, but we definitely
want a "how much is left to copy" variant as well.
Hmm... raw_setup_sigcontext(), perhaps, along with the
macro above for unsafe_put_sigcontext()?
^ permalink raw reply [flat|nested] 71+ messages in thread
* [RFC][PATCH 14/22] x86: ia32_setup_frame(): consolidate uaccess areas
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (11 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 13/22] x86: ia32_setup_sigcontext(): lift user_access_{begin,end}() into the callers Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 15/22] x86: ia32_setup_rt_frame(): " Al Viro
` (9 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Currently we have user_access block, followed by __put_user(),
deciding what the restorer will be and finally a put_user_try
block.
Moving the calculation of restorer first allows the rest
(actual copyout work) to coalesce into a single user_access block.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 41 +++++++++++++----------------------------
1 file changed, 13 insertions(+), 28 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index aedf9929f2b9..4dae24f42e1a 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -229,7 +229,6 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
{
struct sigframe_ia32 __user *frame;
void __user *restorer;
- int err = 0;
void __user *fpstate = NULL;
/* copy_to_user optimizes that into a single 8 byte store */
@@ -245,22 +244,6 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
frame = get_sigframe(ksig, regs, sizeof(*frame), &fpstate);
- if (!access_ok(frame, sizeof(*frame)))
- return -EFAULT;
-
- if (__put_user(sig, &frame->sig))
- return -EFAULT;
-
- if (!user_access_begin(&frame->sc, sizeof(struct sigcontext_32)))
- return -EFAULT;
-
- if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
- goto Efault;
- user_access_end();
-
- if (__put_user(set->sig[1], &frame->extramask[0]))
- return -EFAULT;
-
if (ksig->ka.sa.sa_flags & SA_RESTORER) {
restorer = ksig->ka.sa.sa_restorer;
} else {
@@ -272,19 +255,21 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
restorer = &frame->retcode;
}
- put_user_try {
- put_user_ex(ptr_to_compat(restorer), &frame->pretcode);
-
- /*
- * These are actually not used anymore, but left because some
- * gdb versions depend on them as a marker.
- */
- put_user_ex(*((u64 *)&code), (u64 __user *)frame->retcode);
- } put_user_catch(err);
-
- if (err)
+ if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
+ unsafe_put_user(sig, &frame->sig, Efault);
+ if (ia32_setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
+ goto Efault;
+ unsafe_put_user(set->sig[1], &frame->extramask[0], Efault);
+ unsafe_put_user(ptr_to_compat(restorer), &frame->pretcode, Efault);
+ /*
+ * These are actually not used anymore, but left because some
+ * gdb versions depend on them as a marker.
+ */
+ unsafe_put_user(*((u64 *)&code), (u64 __user *)frame->retcode, Efault);
+ user_access_end();
+
/* Set up registers for signal handler */
regs->sp = (unsigned long) frame;
regs->ip = (unsigned long) ksig->ka.sa.sa_handler;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 15/22] x86: ia32_setup_rt_frame(): consolidate uaccess areas
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (12 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 14/22] x86: ia32_setup_frame(): consolidate uaccess areas Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 16/22] x86: get rid of put_user_try in __setup_rt_frame() (both 32bit and 64bit) Al Viro
` (8 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
__copy_siginfo_to_user32() call reordered a bit. The rest folds
nicely.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/ia32/ia32_signal.c | 17 ++++++-----------
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 4dae24f42e1a..b97087812d26 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -296,10 +296,9 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
{
struct rt_sigframe_ia32 __user *frame;
void __user *restorer;
- int err = 0;
void __user *fpstate = NULL;
- /* __copy_to_user optimizes that into a single 8 byte store */
+ /* unsafe_put_user optimizes that into a single 8 byte store */
static const struct {
u8 movl;
u32 val;
@@ -341,17 +340,13 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
* versions need it.
*/
unsafe_put_user(*((u64 *)&code), (u64 __user *)frame->retcode, Efault);
+ if (ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
+ regs, set->sig[0]))
+ goto Efault;
+ unsafe_put_user(*(__u64 *)set, (__u64 *)&frame->uc.uc_sigmask, Efault);
user_access_end();
- err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
- if (!user_access_begin(&frame->uc.uc_mcontext, sizeof(struct sigcontext_32)))
- return -EFAULT;
- err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
- regs, set->sig[0]);
- user_access_end();
- err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
-
- if (err)
+ if (__copy_siginfo_to_user32(&frame->info, &ksig->info, false))
return -EFAULT;
/* Set up registers for signal handler */
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 16/22] x86: get rid of put_user_try in __setup_rt_frame() (both 32bit and 64bit)
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (13 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 15/22] x86: ia32_setup_rt_frame(): " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 17/22] x86: setup_sigcontext(): list user_access_{begin,end}() into callers Al Viro
` (7 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Straightforward, except for save_altstack_ex() stuck in those.
Replace that thing with an analogue that would use unsafe_put_user()
instead of put_user_ex() (called compat_save_altstack()) and be done
with that.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 91 +++++++++++++++++++++++++-----------------------
include/linux/signal.h | 8 ++---
2 files changed, 52 insertions(+), 47 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 29abad29aaa1..8b879fdc214c 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -365,38 +365,37 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
- if (!access_ok(frame, sizeof(*frame)))
+ if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
- put_user_try {
- put_user_ex(sig, &frame->sig);
- put_user_ex(&frame->info, &frame->pinfo);
- put_user_ex(&frame->uc, &frame->puc);
+ unsafe_put_user(sig, &frame->sig, Efault);
+ unsafe_put_user(&frame->info, &frame->pinfo, Efault);
+ unsafe_put_user(&frame->uc, &frame->puc, Efault);
- /* Create the ucontext. */
- if (static_cpu_has(X86_FEATURE_XSAVE))
- put_user_ex(UC_FP_XSTATE, &frame->uc.uc_flags);
- else
- put_user_ex(0, &frame->uc.uc_flags);
- put_user_ex(0, &frame->uc.uc_link);
- save_altstack_ex(&frame->uc.uc_stack, regs->sp);
+ /* Create the ucontext. */
+ if (static_cpu_has(X86_FEATURE_XSAVE))
+ unsafe_put_user(UC_FP_XSTATE, &frame->uc.uc_flags, Efault);
+ else
+ unsafe_put_user(0, &frame->uc.uc_flags, Efault);
+ unsafe_put_user(0, &frame->uc.uc_link, Efault);
+ unsafe_save_altstack(&frame->uc.uc_stack, regs->sp, Efault);
- /* Set up to return from userspace. */
- restorer = current->mm->context.vdso +
- vdso_image_32.sym___kernel_rt_sigreturn;
- if (ksig->ka.sa.sa_flags & SA_RESTORER)
- restorer = ksig->ka.sa.sa_restorer;
- put_user_ex(restorer, &frame->pretcode);
+ /* Set up to return from userspace. */
+ restorer = current->mm->context.vdso +
+ vdso_image_32.sym___kernel_rt_sigreturn;
+ if (ksig->ka.sa.sa_flags & SA_RESTORER)
+ restorer = ksig->ka.sa.sa_restorer;
+ unsafe_put_user(restorer, &frame->pretcode, Efault);
- /*
- * This is movl $__NR_rt_sigreturn, %ax ; int $0x80
- *
- * WE DO NOT USE IT ANY MORE! It's only left here for historical
- * reasons and because gdb uses it as a signature to notice
- * signal handler stack frames.
- */
- put_user_ex(*((u64 *)&rt_retcode), (u64 *)frame->retcode);
- } put_user_catch(err);
+ /*
+ * This is movl $__NR_rt_sigreturn, %ax ; int $0x80
+ *
+ * WE DO NOT USE IT ANY MORE! It's only left here for historical
+ * reasons and because gdb uses it as a signature to notice
+ * signal handler stack frames.
+ */
+ unsafe_put_user(*((u64 *)&rt_retcode), (u64 *)frame->retcode, Efault);
+ user_access_end();
err |= copy_siginfo_to_user(&frame->info, &ksig->info);
err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
@@ -419,6 +418,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
regs->cs = __USER_CS;
return 0;
+Efault:
+ user_access_end();
+ return -EFAULT;
}
#else /* !CONFIG_X86_32 */
static unsigned long frame_uc_flags(struct pt_regs *regs)
@@ -444,6 +446,10 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
unsigned long uc_flags;
int err = 0;
+ /* x86-64 should always use SA_RESTORER. */
+ if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
+ return -EFAULT;
+
frame = get_sigframe(&ksig->ka, regs, sizeof(struct rt_sigframe), &fp);
if (!access_ok(frame, sizeof(*frame)))
@@ -455,23 +461,18 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
}
uc_flags = frame_uc_flags(regs);
+ if (!user_access_begin(frame, sizeof(*frame)))
+ return -EFAULT;
- put_user_try {
- /* Create the ucontext. */
- put_user_ex(uc_flags, &frame->uc.uc_flags);
- put_user_ex(0, &frame->uc.uc_link);
- save_altstack_ex(&frame->uc.uc_stack, regs->sp);
-
- /* Set up to return from userspace. If provided, use a stub
- already in userspace. */
- /* x86-64 should always use SA_RESTORER. */
- if (ksig->ka.sa.sa_flags & SA_RESTORER) {
- put_user_ex(ksig->ka.sa.sa_restorer, &frame->pretcode);
- } else {
- /* could use a vstub here */
- err |= -EFAULT;
- }
- } put_user_catch(err);
+ /* Create the ucontext. */
+ unsafe_put_user(uc_flags, &frame->uc.uc_flags, Efault);
+ unsafe_put_user(0, &frame->uc.uc_link, Efault);
+ unsafe_save_altstack(&frame->uc.uc_stack, regs->sp, Efault);
+
+ /* Set up to return from userspace. If provided, use a stub
+ already in userspace. */
+ unsafe_put_user(ksig->ka.sa.sa_restorer, &frame->pretcode, Efault);
+ user_access_end();
err |= setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]);
err |= __put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0]);
@@ -515,6 +516,10 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
force_valid_ss(regs);
return 0;
+
+Efault:
+ user_access_end();
+ return -EFAULT;
}
#endif /* CONFIG_X86_32 */
diff --git a/include/linux/signal.h b/include/linux/signal.h
index 1a5f88316b08..05bacd2ab135 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -444,12 +444,12 @@ void signals_init(void);
int restore_altstack(const stack_t __user *);
int __save_altstack(stack_t __user *, unsigned long);
-#define save_altstack_ex(uss, sp) do { \
+#define unsafe_save_altstack(uss, sp, label) do { \
stack_t __user *__uss = uss; \
struct task_struct *t = current; \
- put_user_ex((void __user *)t->sas_ss_sp, &__uss->ss_sp); \
- put_user_ex(t->sas_ss_flags, &__uss->ss_flags); \
- put_user_ex(t->sas_ss_size, &__uss->ss_size); \
+ unsafe_put_user((void __user *)t->sas_ss_sp, &__uss->ss_sp, label); \
+ unsafe_put_user(t->sas_ss_flags, &__uss->ss_flags, label); \
+ unsafe_put_user(t->sas_ss_size, &__uss->ss_size, label); \
if (t->sas_ss_flags & SS_AUTODISARM) \
sas_ss_reset(t); \
} while (0);
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 17/22] x86: setup_sigcontext(): list user_access_{begin,end}() into callers
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (14 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 16/22] x86: get rid of put_user_try in __setup_rt_frame() (both 32bit and 64bit) Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:56 ` Linus Torvalds
2020-03-23 18:38 ` [RFC][PATCH 18/22] x86: __setup_frame(): consolidate uaccess areas Al Viro
` (6 subsequent siblings)
22 siblings, 1 reply; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Similar to ia32_setup_sigcontext() change several commits ago, make it
__always_inline. In cases when there is a user_access_{begin,end}()
section nearby, just move the call over there.
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 32 +++++++++++++++++---------------
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 8b879fdc214c..a5a898c3c574 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -140,12 +140,9 @@ static int restore_sigcontext(struct pt_regs *regs,
IS_ENABLED(CONFIG_X86_32));
}
-static int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
+static __always_inline int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
struct pt_regs *regs, unsigned long mask)
{
- if (!user_access_begin(sc, sizeof(struct sigcontext)))
- return -EFAULT;
-
#ifdef CONFIG_X86_32
unsafe_put_user(get_user_gs(regs),
(unsigned int __user *)&sc->gs, Efault);
@@ -194,10 +191,8 @@ static int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
/* non-iBCS2 extensions.. */
unsafe_put_user(mask, &sc->oldmask, Efault);
unsafe_put_user(current->thread.cr2, &sc->cr2, Efault);
- user_access_end();
return 0;
Efault:
- user_access_end();
return -EFAULT;
}
@@ -311,8 +306,11 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
if (__put_user(sig, &frame->sig))
return -EFAULT;
- if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
+ if (!user_access_begin(&frame->sc, sizeof(struct sigcontext)))
return -EFAULT;
+ if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
+ goto Efault;
+ user_access_end();
if (__put_user(set->sig[1], &frame->extramask[0]))
return -EFAULT;
@@ -353,6 +351,10 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
regs->cs = __USER_CS;
return 0;
+
+Efault:
+ user_access_end();
+ return -EFAULT;
}
static int __setup_rt_frame(int sig, struct ksignal *ksig,
@@ -395,13 +397,13 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
* signal handler stack frames.
*/
unsafe_put_user(*((u64 *)&rt_retcode), (u64 *)frame->retcode, Efault);
+ if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
+ regs, set->sig[0]))
+ goto Efault;
user_access_end();
err |= copy_siginfo_to_user(&frame->info, &ksig->info);
- err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
- regs, set->sig[0]);
err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
-
if (err)
return -EFAULT;
@@ -472,9 +474,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
/* Set up to return from userspace. If provided, use a stub
already in userspace. */
unsafe_put_user(ksig->ka.sa.sa_restorer, &frame->pretcode, Efault);
+ if (setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]))
+ goto Efault;
user_access_end();
-
- err |= setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]);
err |= __put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0]);
if (err)
@@ -559,10 +561,10 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
unsafe_put_user(0, &frame->uc.uc__pad0, Efault);
restorer = ksig->ka.sa.sa_restorer;
unsafe_put_user(restorer, (unsigned long __user *)&frame->pretcode, Efault);
+ if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
+ regs, set->sig[0]))
+ goto Efault;
user_access_end();
-
- err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
- regs, set->sig[0]);
err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
if (err)
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 17/22] x86: setup_sigcontext(): list user_access_{begin,end}() into callers
2020-03-23 18:38 ` [RFC][PATCH 17/22] x86: setup_sigcontext(): list user_access_{begin,end}() into callers Al Viro
@ 2020-03-23 18:56 ` Linus Torvalds
0 siblings, 0 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-23 18:56 UTC (permalink / raw)
To: Al Viro
Cc: Thomas Gleixner, the arch/x86 maintainers, Linux Kernel Mailing List
On Mon, Mar 23, 2020 at 11:40 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> -static int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
> +static __always_inline int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate,
Same deal here: just marking it __always_inline is not enough - make
the naming show that this does the unsafe accesses.
So call it "unsafe_setup_sigcontext()" if you're doing unsafe
accesses, so that *EVERYTHING* that gets done in between the
"user_access_begin/end()" is clearly marked as being special.
Not just for objdump, but for people too.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread
* [RFC][PATCH 18/22] x86: __setup_frame(): consolidate uaccess areas
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (15 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 17/22] x86: setup_sigcontext(): list user_access_{begin,end}() into callers Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 19/22] x86: __setup_rt_frame(): " Al Viro
` (5 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 23 ++++++-----------------
1 file changed, 6 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index a5a898c3c574..02b81784acc7 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -295,26 +295,17 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
{
struct sigframe __user *frame;
void __user *restorer;
- int err = 0;
void __user *fpstate = NULL;
frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
- if (!access_ok(frame, sizeof(*frame)))
- return -EFAULT;
-
- if (__put_user(sig, &frame->sig))
+ if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
- if (!user_access_begin(&frame->sc, sizeof(struct sigcontext)))
- return -EFAULT;
+ unsafe_put_user(sig, &frame->sig, Efault);
if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
goto Efault;
- user_access_end();
-
- if (__put_user(set->sig[1], &frame->extramask[0]))
- return -EFAULT;
-
+ unsafe_put_user(set->sig[1], &frame->extramask[0], Efault);
if (current->mm->context.vdso)
restorer = current->mm->context.vdso +
vdso_image_32.sym___kernel_sigreturn;
@@ -324,7 +315,7 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
restorer = ksig->ka.sa.sa_restorer;
/* Set up to return from userspace. */
- err |= __put_user(restorer, &frame->pretcode);
+ unsafe_put_user(restorer, &frame->pretcode, Efault);
/*
* This is popl %eax ; movl $__NR_sigreturn, %eax ; int $0x80
@@ -333,10 +324,8 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
* reasons and because gdb uses it as a signature to notice
* signal handler stack frames.
*/
- err |= __put_user(*((u64 *)&retcode), (u64 *)frame->retcode);
-
- if (err)
- return -EFAULT;
+ unsafe_put_user(*((u64 *)&retcode), (u64 *)frame->retcode, Efault);
+ user_access_end();
/* Set up registers for signal handler */
regs->sp = (unsigned long)frame;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 19/22] x86: __setup_rt_frame(): consolidate uaccess areas
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (16 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 18/22] x86: __setup_frame(): consolidate uaccess areas Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 20/22] x86: x32_setup_rt_frame(): " Al Viro
` (4 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
reorder copy_siginfo_to_user() calls a bit
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 26 +++++++++-----------------
1 file changed, 9 insertions(+), 17 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 02b81784acc7..66bcb5539ae7 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -351,7 +351,6 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
{
struct rt_sigframe __user *frame;
void __user *restorer;
- int err = 0;
void __user *fpstate = NULL;
frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
@@ -389,11 +388,11 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]))
goto Efault;
+ unsafe_put_user(*(__u64 *)set,
+ (__u64 __user *)&frame->uc.uc_sigmask, Efault);
user_access_end();
- err |= copy_siginfo_to_user(&frame->info, &ksig->info);
- err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
- if (err)
+ if (copy_siginfo_to_user(&frame->info, &ksig->info))
return -EFAULT;
/* Set up registers for signal handler */
@@ -435,23 +434,14 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
struct rt_sigframe __user *frame;
void __user *fp = NULL;
unsigned long uc_flags;
- int err = 0;
/* x86-64 should always use SA_RESTORER. */
if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
return -EFAULT;
frame = get_sigframe(&ksig->ka, regs, sizeof(struct rt_sigframe), &fp);
-
- if (!access_ok(frame, sizeof(*frame)))
- return -EFAULT;
-
- if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
- if (copy_siginfo_to_user(&frame->info, &ksig->info))
- return -EFAULT;
- }
-
uc_flags = frame_uc_flags(regs);
+
if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
@@ -465,11 +455,13 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
unsafe_put_user(ksig->ka.sa.sa_restorer, &frame->pretcode, Efault);
if (setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]))
goto Efault;
+ unsafe_put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0], Efault);
user_access_end();
- err |= __put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0]);
- if (err)
- return -EFAULT;
+ if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
+ if (copy_siginfo_to_user(&frame->info, &ksig->info))
+ return -EFAULT;
+ }
/* Set up registers for signal handler */
regs->di = sig;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 20/22] x86: x32_setup_rt_frame(): consolidate uaccess areas
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (17 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 19/22] x86: __setup_rt_frame(): " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 21/22] x86: unsafe_put_... macros for sigcontext and sigmask Al Viro
` (3 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 66bcb5539ae7..50679e8f42d7 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -514,7 +514,6 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
struct rt_sigframe_x32 __user *frame;
unsigned long uc_flags;
void __user *restorer;
- int err = 0;
void __user *fpstate = NULL;
if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
@@ -522,14 +521,6 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
- if (!access_ok(frame, sizeof(*frame)))
- return -EFAULT;
-
- if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
- if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true))
- return -EFAULT;
- }
-
uc_flags = frame_uc_flags(regs);
if (!user_access_begin(frame, sizeof(*frame)))
@@ -545,11 +536,13 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]))
goto Efault;
+ unsafe_put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask, Efault);
user_access_end();
- err |= __put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask);
- if (err)
- return -EFAULT;
+ if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
+ if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true))
+ return -EFAULT;
+ }
/* Set up registers for signal handler */
regs->sp = (unsigned long) frame;
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 21/22] x86: unsafe_put_... macros for sigcontext and sigmask
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (18 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 20/22] x86: x32_setup_rt_frame(): " Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-23 18:38 ` [RFC][PATCH 22/22] kill uaccess_try() Al Viro
` (2 subsequent siblings)
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
regularizes things a bit
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
arch/x86/kernel/signal.c | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 50679e8f42d7..52edadca5723 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -196,6 +196,17 @@ static __always_inline int setup_sigcontext(struct sigcontext __user *sc, void _
return -EFAULT;
}
+#define unsafe_put_sigcontext(sc, fpstate, regs, set, label) \
+do { \
+ if (setup_sigcontext(sc, fpstate, regs, set->sig[0])) \
+ goto label; \
+} while(0);
+
+#define unsafe_put_sigmask(set, frame, label) \
+ unsafe_put_user(*(__u64 *)(set), \
+ (__u64 __user *)&(frame)->uc.uc_sigmask, \
+ label)
+
/*
* Set up a signal frame.
*/
@@ -303,8 +314,7 @@ __setup_frame(int sig, struct ksignal *ksig, sigset_t *set,
return -EFAULT;
unsafe_put_user(sig, &frame->sig, Efault);
- if (setup_sigcontext(&frame->sc, fpstate, regs, set->sig[0]))
- goto Efault;
+ unsafe_put_sigcontext(&frame->sc, fpstate, regs, set, Efault);
unsafe_put_user(set->sig[1], &frame->extramask[0], Efault);
if (current->mm->context.vdso)
restorer = current->mm->context.vdso +
@@ -351,9 +361,9 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
{
struct rt_sigframe __user *frame;
void __user *restorer;
- void __user *fpstate = NULL;
+ void __user *fp = NULL;
- frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
+ frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fp);
if (!user_access_begin(frame, sizeof(*frame)))
return -EFAULT;
@@ -385,11 +395,8 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
* signal handler stack frames.
*/
unsafe_put_user(*((u64 *)&rt_retcode), (u64 *)frame->retcode, Efault);
- if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
- regs, set->sig[0]))
- goto Efault;
- unsafe_put_user(*(__u64 *)set,
- (__u64 __user *)&frame->uc.uc_sigmask, Efault);
+ unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set, Efault);
+ unsafe_put_sigmask(set, frame, Efault);
user_access_end();
if (copy_siginfo_to_user(&frame->info, &ksig->info))
@@ -453,9 +460,8 @@ static int __setup_rt_frame(int sig, struct ksignal *ksig,
/* Set up to return from userspace. If provided, use a stub
already in userspace. */
unsafe_put_user(ksig->ka.sa.sa_restorer, &frame->pretcode, Efault);
- if (setup_sigcontext(&frame->uc.uc_mcontext, fp, regs, set->sig[0]))
- goto Efault;
- unsafe_put_user(set->sig[0], &frame->uc.uc_sigmask.sig[0], Efault);
+ unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set, Efault);
+ unsafe_put_sigmask(set, frame, Efault);
user_access_end();
if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
@@ -514,12 +520,12 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
struct rt_sigframe_x32 __user *frame;
unsigned long uc_flags;
void __user *restorer;
- void __user *fpstate = NULL;
+ void __user *fp = NULL;
if (!(ksig->ka.sa.sa_flags & SA_RESTORER))
return -EFAULT;
- frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fpstate);
+ frame = get_sigframe(&ksig->ka, regs, sizeof(*frame), &fp);
uc_flags = frame_uc_flags(regs);
@@ -533,10 +539,8 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
unsafe_put_user(0, &frame->uc.uc__pad0, Efault);
restorer = ksig->ka.sa.sa_restorer;
unsafe_put_user(restorer, (unsigned long __user *)&frame->pretcode, Efault);
- if (setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
- regs, set->sig[0]))
- goto Efault;
- unsafe_put_user(*(__u64 *)set, (__u64 __user *)&frame->uc.uc_sigmask, Efault);
+ unsafe_put_sigcontext(&frame->uc.uc_mcontext, fp, regs, set, Efault);
+ unsafe_put_sigmask(set, frame, Efault);
user_access_end();
if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* [RFC][PATCH 22/22] kill uaccess_try()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (19 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 21/22] x86: unsafe_put_... macros for sigcontext and sigmask Al Viro
@ 2020-03-23 18:38 ` Al Viro
2020-03-24 15:15 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Peter Zijlstra
2020-03-28 10:48 ` Ingo Molnar
22 siblings, 0 replies; 71+ messages in thread
From: Al Viro @ 2020-03-23 18:38 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Thomas Gleixner, x86, linux-kernel
From: Al Viro <viro@zeniv.linux.org.uk>
finally
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
Documentation/x86/exception-tables.rst | 6 ----
arch/x86/include/asm/asm.h | 6 ----
arch/x86/include/asm/processor.h | 1 -
arch/x86/include/asm/uaccess.h | 65 ----------------------------------
arch/x86/mm/extable.c | 12 -------
5 files changed, 90 deletions(-)
diff --git a/Documentation/x86/exception-tables.rst b/Documentation/x86/exception-tables.rst
index ed6d4b0cf62c..514f51829da7 100644
--- a/Documentation/x86/exception-tables.rst
+++ b/Documentation/x86/exception-tables.rst
@@ -337,10 +337,4 @@ pointer which points to one of:
entry->insn. It is used to distinguish page faults from machine
check.
-3) ``int ex_handler_ext(const struct exception_table_entry *fixup)``
- This case is used for uaccess_err ... we need to set a flag
- in the task structure. Before the handler functions existed this
- case was handled by adding a large offset to the fixup to tag
- it as special.
-
More functions can easily be added.
diff --git a/arch/x86/include/asm/asm.h b/arch/x86/include/asm/asm.h
index cd339b88d5d4..0f63585edf5f 100644
--- a/arch/x86/include/asm/asm.h
+++ b/arch/x86/include/asm/asm.h
@@ -138,9 +138,6 @@
# define _ASM_EXTABLE_FAULT(from, to) \
_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
-# define _ASM_EXTABLE_EX(from, to) \
- _ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
-
# define _ASM_NOKPROBE(entry) \
.pushsection "_kprobe_blacklist","aw" ; \
_ASM_ALIGN ; \
@@ -166,9 +163,6 @@
# define _ASM_EXTABLE_FAULT(from, to) \
_ASM_EXTABLE_HANDLE(from, to, ex_handler_fault)
-# define _ASM_EXTABLE_EX(from, to) \
- _ASM_EXTABLE_HANDLE(from, to, ex_handler_ext)
-
/* For C file, we already have NOKPROBE_SYMBOL macro */
#endif
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 09705ccc393c..19718fcfdd6a 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -541,7 +541,6 @@ struct thread_struct {
mm_segment_t addr_limit;
unsigned int sig_on_uaccess_err:1;
- unsigned int uaccess_err:1; /* uaccess failed */
/* Floating point and extended processor state */
struct fpu fpu;
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 4dc5accdd826..d11662f753d2 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -193,23 +193,12 @@ __typeof__(__builtin_choose_expr(sizeof(x) > sizeof(0UL), 0ULL, 0UL))
: : "A" (x), "r" (addr) \
: : label)
-#define __put_user_asm_ex_u64(x, addr) \
- asm volatile("\n" \
- "1: movl %%eax,0(%1)\n" \
- "2: movl %%edx,4(%1)\n" \
- "3:" \
- _ASM_EXTABLE_EX(1b, 2b) \
- _ASM_EXTABLE_EX(2b, 3b) \
- : : "A" (x), "r" (addr))
-
#define __put_user_x8(x, ptr, __ret_pu) \
asm volatile("call __put_user_8" : "=a" (__ret_pu) \
: "A" ((typeof(*(ptr)))(x)), "c" (ptr) : "ebx")
#else
#define __put_user_goto_u64(x, ptr, label) \
__put_user_goto(x, ptr, "q", "", "er", label)
-#define __put_user_asm_ex_u64(x, addr) \
- __put_user_asm_ex(x, addr, "q", "", "er")
#define __put_user_x8(x, ptr, __ret_pu) __put_user_x(8, x, ptr, __ret_pu)
#endif
@@ -289,31 +278,6 @@ do { \
} \
} while (0)
-/*
- * This doesn't do __uaccess_begin/end - the exception handling
- * around it must do that.
- */
-#define __put_user_size_ex(x, ptr, size) \
-do { \
- __chk_user_ptr(ptr); \
- switch (size) { \
- case 1: \
- __put_user_asm_ex(x, ptr, "b", "b", "iq"); \
- break; \
- case 2: \
- __put_user_asm_ex(x, ptr, "w", "w", "ir"); \
- break; \
- case 4: \
- __put_user_asm_ex(x, ptr, "l", "k", "ir"); \
- break; \
- case 8: \
- __put_user_asm_ex_u64((__typeof__(*ptr))(x), ptr); \
- break; \
- default: \
- __put_user_bad(); \
- } \
-} while (0)
-
#ifdef CONFIG_X86_32
#define __get_user_asm_u64(x, ptr, retval, errret) \
({ \
@@ -430,29 +394,6 @@ struct __large_struct { unsigned long buf[100]; };
retval = __put_user_failed(x, addr, itype, rtype, ltype, errret); \
} while (0)
-#define __put_user_asm_ex(x, addr, itype, rtype, ltype) \
- asm volatile("1: mov"itype" %"rtype"0,%1\n" \
- "2:\n" \
- _ASM_EXTABLE_EX(1b, 2b) \
- : : ltype(x), "m" (__m(addr)))
-
-/*
- * uaccess_try and catch
- */
-#define uaccess_try do { \
- current->thread.uaccess_err = 0; \
- __uaccess_begin(); \
- barrier();
-
-#define uaccess_try_nospec do { \
- current->thread.uaccess_err = 0; \
- __uaccess_begin_nospec(); \
-
-#define uaccess_catch(err) \
- __uaccess_end(); \
- (err) |= (current->thread.uaccess_err ? -EFAULT : 0); \
-} while (0)
-
/**
* __get_user - Get a simple variable from user space, with less checking.
* @x: Variable to store result.
@@ -502,12 +443,6 @@ struct __large_struct { unsigned long buf[100]; };
#define __put_user(x, ptr) \
__put_user_nocheck((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-#define put_user_try uaccess_try
-#define put_user_catch(err) uaccess_catch(err)
-
-#define put_user_ex(x, ptr) \
- __put_user_size_ex((__typeof__(*(ptr)))(x), (ptr), sizeof(*(ptr)))
-
extern unsigned long
copy_from_user_nmi(void *to, const void __user *from, unsigned long n);
extern __must_check long
diff --git a/arch/x86/mm/extable.c b/arch/x86/mm/extable.c
index 30bb0bd3b1b8..b991aa4bdfae 100644
--- a/arch/x86/mm/extable.c
+++ b/arch/x86/mm/extable.c
@@ -80,18 +80,6 @@ __visible bool ex_handler_uaccess(const struct exception_table_entry *fixup,
}
EXPORT_SYMBOL(ex_handler_uaccess);
-__visible bool ex_handler_ext(const struct exception_table_entry *fixup,
- struct pt_regs *regs, int trapnr,
- unsigned long error_code,
- unsigned long fault_addr)
-{
- /* Special hack for uaccess_err */
- current->thread.uaccess_err = 1;
- regs->ip = ex_fixup_addr(fixup);
- return true;
-}
-EXPORT_SYMBOL(ex_handler_ext);
-
__visible bool ex_handler_rdmsr_unsafe(const struct exception_table_entry *fixup,
struct pt_regs *regs, int trapnr,
unsigned long error_code,
--
2.11.0
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (20 preceding siblings ...)
2020-03-23 18:38 ` [RFC][PATCH 22/22] kill uaccess_try() Al Viro
@ 2020-03-24 15:15 ` Peter Zijlstra
2020-03-28 10:48 ` Ingo Molnar
22 siblings, 0 replies; 71+ messages in thread
From: Peter Zijlstra @ 2020-03-24 15:15 UTC (permalink / raw)
To: Al Viro; +Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel
On Mon, Mar 23, 2020 at 06:37:58PM +0000, Al Viro wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> rather than relying upon the magic in raw_copy_from_user()
>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 61d93f062a36..ab8eab43a8a2 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -695,15 +695,6 @@ extern struct movsl_mask {
> #endif
>
> /*
> - * We rely on the nested NMI work to allow atomic faults from the NMI path; the
> - * nested NMI paths are careful to preserve CR2.
> - *
> - * Caller must use pagefault_enable/disable, or run in interrupt context,
> - * and also do a uaccess_ok() check
> - */
> -#define __copy_from_user_nmi __copy_from_user_inatomic
> -
> -/*
> * The "unsafe" user accesses aren't really "unsafe", but the naming
> * is a big fat warning: you have to not only do the access_ok()
> * checking before using them, but you have to surround them with the
Thanks for killing that remnant!
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-23 18:37 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Al Viro
` (21 preceding siblings ...)
2020-03-24 15:15 ` [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user() Peter Zijlstra
@ 2020-03-28 10:48 ` Ingo Molnar
2020-03-28 11:59 ` Al Viro
22 siblings, 1 reply; 71+ messages in thread
From: Ingo Molnar @ 2020-03-28 10:48 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel, Borislav Petkov
* Al Viro <viro@ZenIV.linux.org.uk> wrote:
> From: Al Viro <viro@zeniv.linux.org.uk>
>
> rather than relying upon the magic in raw_copy_from_user()
> - bytes = __copy_from_user_nmi(&frame.next_frame, fp, 4);
> - if (bytes != 0)
> + if (__get_user(frame.next_frame, &fp->next_frame))
> break;
> - bytes = __copy_from_user_nmi(&frame.return_address, fp+4, 4);
> - if (bytes != 0)
> + if (__get_user(frame.return_address, &fp->return_address))
> break;
Just wondering about the long term plan here: we have unsafe_get_user()
as a wrapper around __get_user(), but the __get_user() API doesn't carry
the 'unsafe' tag yet.
Should we add an __unsafe_get_user() alias to it perhaps, and use it in
all code that adds it, like the chunk above? Or rename it to
__unsafe_get_user() outright? No change to the logic, but it would be
more obvious what code has inherited old __get_user() uses and which code
uses __unsafe_get_user() intentionally.
Even after your series there's 700 uses of __get_user(), so it would make
sense to make a distinction in name at least and tag all unsafe APIs with
an 'unsafe_' prefix.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-28 10:48 ` Ingo Molnar
@ 2020-03-28 11:59 ` Al Viro
2020-03-29 9:26 ` Ingo Molnar
0 siblings, 1 reply; 71+ messages in thread
From: Al Viro @ 2020-03-28 11:59 UTC (permalink / raw)
To: Ingo Molnar
Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel, Borislav Petkov
On Sat, Mar 28, 2020 at 11:48:57AM +0100, Ingo Molnar wrote:
>
> * Al Viro <viro@ZenIV.linux.org.uk> wrote:
>
> > From: Al Viro <viro@zeniv.linux.org.uk>
> >
> > rather than relying upon the magic in raw_copy_from_user()
>
> > - bytes = __copy_from_user_nmi(&frame.next_frame, fp, 4);
> > - if (bytes != 0)
> > + if (__get_user(frame.next_frame, &fp->next_frame))
> > break;
> > - bytes = __copy_from_user_nmi(&frame.return_address, fp+4, 4);
> > - if (bytes != 0)
> > + if (__get_user(frame.return_address, &fp->return_address))
> > break;
>
> Just wondering about the long term plan here: we have unsafe_get_user()
> as a wrapper around __get_user(),
Not on x86; that wrapper is the fallback for architectures without
non-trivial user_access_begin/user_access_end
> but the __get_user() API doesn't carry
> the 'unsafe' tag yet.
>
> Should we add an __unsafe_get_user() alias to it perhaps, and use it in
> all code that adds it, like the chunk above? Or rename it to
> __unsafe_get_user() outright? No change to the logic, but it would be
> more obvious what code has inherited old __get_user() uses and which code
> uses __unsafe_get_user() intentionally.
>
> Even after your series there's 700 uses of __get_user(), so it would make
> sense to make a distinction in name at least and tag all unsafe APIs with
> an 'unsafe_' prefix.
"unsafe" != "lacks access_ok", it's "done under user_access_begin".
And this series is just a part of much bigger pile.
FWIW, with the currently linearized part I see 26 users in arch/x86 and
108 - outside of arch/*. With 43 of the latter supplied by the sodding
comedi_compat32.c, which needs to be rewritten anyway (or git rm'ed,
for that matter)...
We'll get there; the tricky part is the ones that come in pair with
something other than access_ok() in the first place (many of those
are KVM-related, but not all such are).
This part had been more about untangling uaccess_try stuff,,,
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-28 11:59 ` Al Viro
@ 2020-03-29 9:26 ` Ingo Molnar
2020-03-29 16:50 ` Andy Lutomirski
2020-03-29 17:57 ` Al Viro
0 siblings, 2 replies; 71+ messages in thread
From: Ingo Molnar @ 2020-03-29 9:26 UTC (permalink / raw)
To: Al Viro
Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel, Borislav Petkov
* Al Viro <viro@zeniv.linux.org.uk> wrote:
> > but the __get_user() API doesn't carry the 'unsafe' tag yet.
> >
> > Should we add an __unsafe_get_user() alias to it perhaps, and use it
> > in all code that adds it, like the chunk above? Or rename it to
> > __unsafe_get_user() outright? No change to the logic, but it would be
> > more obvious what code has inherited old __get_user() uses and which
> > code uses __unsafe_get_user() intentionally.
> >
> > Even after your series there's 700 uses of __get_user(), so it would
> > make sense to make a distinction in name at least and tag all unsafe
> > APIs with an 'unsafe_' prefix.
>
> "unsafe" != "lacks access_ok", it's "done under user_access_begin".
Well, I thought the principle was that we'd mark generic APIs that had
*either* a missing access_ok() check or a missing
user_access_begin()/end() wrapping marked unsafe_*(), right?
__get_user() has __uaccess_begin()/end() on the inside, but doesn't have
the access_ok() check, so those calls are 'unsafe' with regard to not
being safe to untrusted (ptr,size) ranges.
I agree that all of these topics need equal attention:
- leaking of cleared SMAP state (CLAC), which results in a silent
failure.
- running user accesses without STAC, which results in a crash.
- not doing an access_ok() check on untrusted (pointer,size) ranges,
which results in a silent failure as well.
I just think that any API that doesn't guarantee all of these are handled
right probably needs to be unsafe_*() tagged.
> FWIW, with the currently linearized part I see 26 users in arch/x86 and
> 108 - outside of arch/*. With 43 of the latter supplied by the sodding
> comedi_compat32.c, which needs to be rewritten anyway (or git rm'ed,
> for that matter)...
>
> We'll get there; the tricky part is the ones that come in pair with
> something other than access_ok() in the first place (many of those are
> KVM-related, but not all such are).
>
> This part had been more about untangling uaccess_try stuff,,,
It's much appreciated! In my previous mail I just wanted to inquire about
the long term plan, whether we are going to get rid of all uses of
__get_user() - to which the answer appears to be "yes". :-)
Thanks,
Ingo
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 9:26 ` Ingo Molnar
@ 2020-03-29 16:50 ` Andy Lutomirski
2020-03-29 17:05 ` Linus Torvalds
2020-03-29 17:41 ` David Laight
2020-03-29 17:57 ` Al Viro
1 sibling, 2 replies; 71+ messages in thread
From: Andy Lutomirski @ 2020-03-29 16:50 UTC (permalink / raw)
To: Ingo Molnar
Cc: Al Viro, Linus Torvalds, Thomas Gleixner, X86 ML, LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 2:26 AM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> > > but the __get_user() API doesn't carry the 'unsafe' tag yet.
> > >
> > > Should we add an __unsafe_get_user() alias to it perhaps, and use it
> > > in all code that adds it, like the chunk above? Or rename it to
> > > __unsafe_get_user() outright? No change to the logic, but it would be
> > > more obvious what code has inherited old __get_user() uses and which
> > > code uses __unsafe_get_user() intentionally.
> > >
> > > Even after your series there's 700 uses of __get_user(), so it would
> > > make sense to make a distinction in name at least and tag all unsafe
> > > APIs with an 'unsafe_' prefix.
> >
> > "unsafe" != "lacks access_ok", it's "done under user_access_begin".
>
> Well, I thought the principle was that we'd mark generic APIs that had
> *either* a missing access_ok() check or a missing
> user_access_begin()/end() wrapping marked unsafe_*(), right?
>
> __get_user() has __uaccess_begin()/end() on the inside, but doesn't have
> the access_ok() check, so those calls are 'unsafe' with regard to not
> being safe to untrusted (ptr,size) ranges.
>
> I agree that all of these topics need equal attention:
>
> - leaking of cleared SMAP state (CLAC), which results in a silent
> failure.
>
> - running user accesses without STAC, which results in a crash.
>
> - not doing an access_ok() check on untrusted (pointer,size) ranges,
> which results in a silent failure as well.
My incliniation is to just get rid of the __get_user()-style APIs.
There shouldn't be any __get_user() calls that can't be directly
replaced by get_user(), and a single integer comparison is not that
expensive. On SMAP systems, the speedup of __get_user vs get_user is
negligible.
(It's possible that some arch code somewhere uses __get_user as a way
to say "access user or kernel memory -- I know what I'm doing". This
is crap if it exists. It better not happen in generic code because of
sane architectures like s390x.)
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 16:50 ` Andy Lutomirski
@ 2020-03-29 17:05 ` Linus Torvalds
2020-03-29 17:41 ` David Laight
1 sibling, 0 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 17:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML, LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 9:50 AM Andy Lutomirski <luto@amacapital.net> wrote:
>
> My incliniation is to just get rid of the __get_user()-style APIs.
That's definitely the direction we're going in, and Al has already
been moving that way.
There is basically zero advantage of __get_user() over get_user() these days.
Historically the advantage used to be quite noticeable (one could be
inlined and generated nice dense code for repeated single accesses),
but with CLAC/STAC that simply isn't the case any more.
> (It's possible that some arch code somewhere uses __get_user as a way
> to say "access user or kernel memory -- I know what I'm doing".
Not just possible - it was what was literally happening in tracing.
Except for the "I know what I'm doing" part, where tracing code used a
pointer that could be user pointer or kernel pointer interchangeably.
Which isn't even possible on some architectures (it just happens to
work on the common ones), because the same pointer bit pattern can be
either or.
But that got fixed, and hopefully there aren't other cases around any
more. But slowly converting away from __get_user() and friends should
end up fixing them all, since objtool will then verify that you do the
right user_access_begin() etc.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread* RE: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 16:50 ` Andy Lutomirski
2020-03-29 17:05 ` Linus Torvalds
@ 2020-03-29 17:41 ` David Laight
2020-03-29 17:56 ` Linus Torvalds
1 sibling, 1 reply; 71+ messages in thread
From: David Laight @ 2020-03-29 17:41 UTC (permalink / raw)
To: 'Andy Lutomirski', Ingo Molnar
Cc: Al Viro, Linus Torvalds, Thomas Gleixner, X86 ML, LKML, Borislav Petkov
From: Andy Lutomirski
> Sent: 29 March 2020 17:51
..
> My incliniation is to just get rid of the __get_user()-style APIs.
> There shouldn't be any __get_user() calls that can't be directly
> replaced by get_user(), and a single integer comparison is not that
> expensive. On SMAP systems, the speedup of __get_user vs get_user is
> negligible.
On x86-64 (at least) __get_user() is inlined but get_user() isn't.
Since get_user() has to return two values, one will always be
a (usually) on-stack real memory location rather than a register.
For frequently use code paths this may be measurable.
I'm thinking of things like epoll_wait() writing out events.
(although that is a put_user() loop...)
It may be worth implementing get_user() as an inline
function that writes the result of access_ok() to a
'by reference' parameter and then returns the value
from an 'real' __get_user() function.
The compiler will then optimise away the memory reference.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 17:41 ` David Laight
@ 2020-03-29 17:56 ` Linus Torvalds
2020-03-29 18:03 ` David Laight
2020-03-29 18:16 ` Al Viro
0 siblings, 2 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 17:56 UTC (permalink / raw)
To: David Laight
Cc: Andy Lutomirski, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 10:41 AM David Laight <David.Laight@aculab.com> wrote:
>
> It may be worth implementing get_user() as an inline
> function that writes the result of access_ok() to a
> 'by reference' parameter and then returns the value
> from an 'real' __get_user() function.
That's how get_user() already works.
It is a polymorphic function (done using macros, sizeof() and ugly
compiler tricks) that generates a call, yes. But it's not a normal C
call. On x86-64, it returns the error code in %rax, and the value in
%rdx
So "get_user()" is already basically optimal. It's likely *faster*
than __get_user(), because it has a smaller I$ footprint if you do
multiple ones.
But, if you have lots of performance-critical get_user() calls, just use
if (user_access_begin(..))
goto efault;
.. multiple "unsafe_get_user(x,ptr,efault);" ..
user_access_end();
...
efault:
user_access_end();
return -EFAULT;
and be done with it.
Yes, the above sequence looks cumbersome, but it's designed for doing
multiple accesses together efficiently. It's basically the "I actually
had a good reason to use __get_user(), but it sucks now, so this is
the new interface"
It's designed for multiple accesses, because as mentioned, if you only
have one, then "get_user()" is already optimal.
And yes, the interface (with that "label for error cases") is
optimized for a (future) world where the compiler can do "asm goto"
together with outputs. Any exception on the access doesn't actually
generate a test at all, the exception will branch directly to the
error label instead.
That already works for "unsafe_put_user()", but for
"unsafe_get_user()" you need a compiler that can do that kind of "asm
goto".
If you use a modern clang version (ie build clang from git), I can
send you a patch for the kernel to try (and a patch for clang to fix a
bug, unless it's been already merged, I didn't check).
The above will generate basically _optimal_ code with my patch and
that modern clang version.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread* RE: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 17:56 ` Linus Torvalds
@ 2020-03-29 18:03 ` David Laight
2020-03-29 18:16 ` Linus Torvalds
2020-03-29 18:16 ` Al Viro
1 sibling, 1 reply; 71+ messages in thread
From: David Laight @ 2020-03-29 18:03 UTC (permalink / raw)
To: 'Linus Torvalds'
Cc: Andy Lutomirski, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
From: Linus Torvalds
> Sent: 29 March 2020 18:57
> On Sun, Mar 29, 2020 at 10:41 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > It may be worth implementing get_user() as an inline
> > function that writes the result of access_ok() to a
> > 'by reference' parameter and then returns the value
> > from an 'real' __get_user() function.
>
> That's how get_user() already works.
>
> It is a polymorphic function (done using macros, sizeof() and ugly
> compiler tricks) that generates a call, yes. But it's not a normal C
> call. On x86-64, it returns the error code in %rax, and the value in
> %rdx
I must be mis-remembering the object code from last time
I looked at it.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 18:03 ` David Laight
@ 2020-03-29 18:16 ` Linus Torvalds
2020-03-29 18:32 ` David Laight
2020-03-29 21:21 ` Andy Lutomirski
0 siblings, 2 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 18:16 UTC (permalink / raw)
To: David Laight
Cc: Andy Lutomirski, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 11:03 AM David Laight <David.Laight@aculab.com> wrote:
>
> > That's how get_user() already works.
> >
> > It is a polymorphic function (done using macros, sizeof() and ugly
> > compiler tricks) that generates a call, yes. But it's not a normal C
> > call. On x86-64, it returns the error code in %rax, and the value in
> > %rdx
>
> I must be mis-remembering the object code from last time
> I looked at it.
On an object code level, the end result actually almost looks like a
normal call, until you start looking at the exact register passing
details.
On a source level, it's anything but.
This is "get_user()" on x86:
#define get_user(x, ptr) \
({ \
int __ret_gu; \
register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
__chk_user_ptr(ptr); \
might_fault(); \
asm volatile("call __get_user_%P4" \
: "=a" (__ret_gu), "=r" (__val_gu), \
ASM_CALL_CONSTRAINT \
: "0" (ptr), "i" (sizeof(*(ptr)))); \
(x) = (__force __typeof__(*(ptr))) __val_gu; \
__builtin_expect(__ret_gu, 0); \
})
and all it actually *generates* is that single "call" instruction, so
the only code you see from the above mess in the object file (assuming
you don't have debugging enabled that expands "might_fault()" into a
lot of checking code) is something like this:
call __get_user_4
with the input address being in %rax (which is also the returning
error code), and the value of 'x' being in %rdx.
The above macro looks a bit messy, but that's actually hiding some of
the real complexity (that "__inttype()" thing is a mess of compiler
tricks in itself, as is the magical ASM_CALL_CONSTRAINT thing, along
with the magic that goes into a 64-bit return value on x86-32 which is
implicit in the magical register asm() definition).
The uaccess code is a lot of complex macros and inline functions to
make it all work well.
Al's uaccess cleanups (which should be in the next merge window) help
a _lot_. Not with the get_user() above (that is already about as
simple as it can get), but with a lot of the other crazy special cases
that we had grown over the years.
My current "unsafe_get_user() using clang and 'asm goto' with outpus"
patch is on top of Al's patches exactly because it was cleaner to do
with all of his cleanups.
But that magical asm-goto-with-outputs patch probably won't land
upstream for a couple of years.
I had the unsafe_put_user() patches to use 'asm goto' in a local
branch for almost three years before I made them official. See commit
a959dc88f9c8 ("Use __put_user_goto in __put_user_size() and
unsafe_put_user()") and notice how it has an author date of May, 2016,
but was only committed January 2019.
I basically waited until "asm goto" was something we could rely on
(and used for other reasons).
I suspect something very similar will happen with unsafe_get_user(). I
have a patch if people want to play with it, but right now it's a
"this relies on an unreleased version of clang to even work".
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread* RE: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 18:16 ` Linus Torvalds
@ 2020-03-29 18:32 ` David Laight
2020-03-29 18:55 ` Linus Torvalds
2020-03-29 21:21 ` Andy Lutomirski
1 sibling, 1 reply; 71+ messages in thread
From: David Laight @ 2020-03-29 18:32 UTC (permalink / raw)
To: 'Linus Torvalds'
Cc: Andy Lutomirski, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
From: Linus Torvalds
> Sent: 29 March 2020 19:16
> On Sun, Mar 29, 2020 at 11:03 AM David Laight <David.Laight@aculab.com> wrote:
> >
> > > That's how get_user() already works.
> > >
> > > It is a polymorphic function (done using macros, sizeof() and ugly
> > > compiler tricks) that generates a call, yes. But it's not a normal C
> > > call. On x86-64, it returns the error code in %rax, and the value in
> > > %rdx
> >
> > I must be mis-remembering the object code from last time
> > I looked at it.
>
> On an object code level, the end result actually almost looks like a
> normal call, until you start looking at the exact register passing
> details.
>
> On a source level, it's anything but.
>
> This is "get_user()" on x86:
>
> #define get_user(x, ptr) \
> ({ \
> int __ret_gu; \
> register __inttype(*(ptr)) __val_gu asm("%"_ASM_DX); \
> __chk_user_ptr(ptr); \
> might_fault(); \
> asm volatile("call __get_user_%P4" \
> : "=a" (__ret_gu), "=r" (__val_gu), \
> ASM_CALL_CONSTRAINT \
> : "0" (ptr), "i" (sizeof(*(ptr)))); \
> (x) = (__force __typeof__(*(ptr))) __val_gu; \
> __builtin_expect(__ret_gu, 0); \
> })
Can't you simplify that by using the =d constraint rather
than relying on a asm register variable.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 18:32 ` David Laight
@ 2020-03-29 18:55 ` Linus Torvalds
0 siblings, 0 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 18:55 UTC (permalink / raw)
To: David Laight
Cc: Andy Lutomirski, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 11:32 AM David Laight <David.Laight@aculab.com> wrote:
>
> Can't you simplify that by using the =d constraint rather
> than relying on a asm register variable.
No, because that asm register variable can be 64-bit.
Which on x86-32 isn't "=d". It's "%edx:%ecx".
The asm register variable thing handles that automatically, but "=d" would not.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 18:16 ` Linus Torvalds
2020-03-29 18:32 ` David Laight
@ 2020-03-29 21:21 ` Andy Lutomirski
2020-03-29 22:06 ` Linus Torvalds
1 sibling, 1 reply; 71+ messages in thread
From: Andy Lutomirski @ 2020-03-29 21:21 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Laight, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 11:16 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> But that magical asm-goto-with-outputs patch probably won't land
> upstream for a couple of years.
I'm not that familiar with gcc politics, but what's the delay? ISTM
having an actual upstream Linux asm-goto-with-outputs that works on
clang might help light a fire under some butts and/or convince someone
to pay a gcc developer to implement it on gcc.
--Andy
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 21:21 ` Andy Lutomirski
@ 2020-03-29 22:06 ` Linus Torvalds
2020-03-29 22:12 ` Linus Torvalds
0 siblings, 1 reply; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 22:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Laight, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
[-- Attachment #1: Type: text/plain, Size: 1941 bytes --]
On Sun, Mar 29, 2020 at 2:22 PM Andy Lutomirski <luto@kernel.org> wrote:
>
> On Sun, Mar 29, 2020 at 11:16 AM Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
> > But that magical asm-goto-with-outputs patch probably won't land
> > upstream for a couple of years.
>
> I'm not that familiar with gcc politics, but what's the delay?
No, I mean that _my_ magical patch for the kernel won't land upstream,
simply because even if both gcc and clang supported it today, nobody
would effectively have those compilers for a couple of years..
> ISTM
> having an actual upstream Linux asm-goto-with-outputs that works on
> clang might help light a fire under some butts and/or convince someone
> to pay a gcc developer to implement it on gcc.
Yes, but even for clang, it needs a version that isn't even released yet.
And right now my patch is unconditional. It started out that way
because the whole x86 uaccess.h files were such a mess, and I couldn't
be bothered to fix all the small cases to then have *two* cases (one
for asm goto with outputs, one without).
These days my patch is much simpler (thanks to Al's simplifications),
and I think making it handle both cases would likely not be too
painful.
And in that case I might just commit it, even if effectively nobody
has the clang version installed to make use of it.
Anyway, just in case people want to see it, I'm attaching my current
unconditional patch.
Note that it requires Al's uaccess cleanups, but I do want to point
out how it actually makes for simpler code:
arch/x86/include/asm/uaccess.h | 72 +++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 40 deletions(-)
and not only does it delete more lines than it adds, the lines it adds
are shorter and simpler than the ones it deletes.
But that "deletes more lines than it adds" is only because it doesn't
even try to build without that "asm goto with outputs" support..
Linus
[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4489 bytes --]
Use "asm goto" with outputs for clang testing
---
arch/x86/include/asm/uaccess.h | 72 +++++++++++++++++++-----------------------
1 file changed, 32 insertions(+), 40 deletions(-)
diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index c8247a84244b..9e8b04d4560a 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -279,65 +279,52 @@ do { \
} while (0)
#ifdef CONFIG_X86_32
-#define __get_user_asm_u64(x, ptr, retval, errret) \
+#define __get_user_asm_u64(x, ptr, label) \
({ \
__typeof__(ptr) __ptr = (ptr); \
- asm volatile("\n" \
- "1: movl %2,%%eax\n" \
- "2: movl %3,%%edx\n" \
- "3:\n" \
- ".section .fixup,\"ax\"\n" \
- "4: mov %4,%0\n" \
- " xorl %%eax,%%eax\n" \
- " xorl %%edx,%%edx\n" \
- " jmp 3b\n" \
- ".previous\n" \
- _ASM_EXTABLE_UA(1b, 4b) \
- _ASM_EXTABLE_UA(2b, 4b) \
- : "=r" (retval), "=&A"(x) \
- : "m" (__m(__ptr)), "m" __m(((u32 __user *)(__ptr)) + 1), \
- "i" (errret), "0" (retval)); \
+ asm_volatile_goto("\n" \
+ "1: movl %1,%%eax\n" \
+ "2: movl %2,%%edx\n" \
+ _ASM_EXTABLE_UA(1b, %l3) \
+ _ASM_EXTABLE_UA(2b, %l3) \
+ : "=&A"(x) \
+ : "m" (__m(__ptr)), "m" __m(((u32 __user *)(__ptr)) + 1) \
+ : : label); \
})
#else
-#define __get_user_asm_u64(x, ptr, retval, errret) \
- __get_user_asm(x, ptr, retval, "q", "", "=r", errret)
+#define __get_user_asm_u64(x, ptr, label) \
+ __get_user_asm(x, ptr, "q", "", "=r", label)
#endif
-#define __get_user_size(x, ptr, size, retval, errret) \
+#define __get_user_size(x, ptr, size, label) \
do { \
- retval = 0; \
__chk_user_ptr(ptr); \
switch (size) { \
case 1: \
- __get_user_asm(x, ptr, retval, "b", "b", "=q", errret); \
+ __get_user_asm(x, ptr, "b", "b", "=q", label); \
break; \
case 2: \
- __get_user_asm(x, ptr, retval, "w", "w", "=r", errret); \
+ __get_user_asm(x, ptr, "w", "w", "=r", label); \
break; \
case 4: \
- __get_user_asm(x, ptr, retval, "l", "k", "=r", errret); \
+ __get_user_asm(x, ptr, "l", "k", "=r", label); \
break; \
case 8: \
- __get_user_asm_u64(x, ptr, retval, errret); \
+ __get_user_asm_u64(x, ptr, label); \
break; \
default: \
(x) = __get_user_bad(); \
} \
} while (0)
-#define __get_user_asm(x, addr, err, itype, rtype, ltype, errret) \
- asm volatile("\n" \
- "1: mov"itype" %2,%"rtype"1\n" \
- "2:\n" \
- ".section .fixup,\"ax\"\n" \
- "3: mov %3,%0\n" \
- " xor"itype" %"rtype"1,%"rtype"1\n" \
- " jmp 2b\n" \
- ".previous\n" \
- _ASM_EXTABLE_UA(1b, 3b) \
- : "=r" (err), ltype(x) \
- : "m" (__m(addr)), "i" (errret), "0" (err))
+#define __get_user_asm(x, addr, itype, rtype, ltype, label) \
+ asm_volatile_goto("\n" \
+ "1: mov"itype" %1,%"rtype"0\n" \
+ _ASM_EXTABLE_UA(1b, %l2) \
+ : ltype(x) \
+ : "m" (__m(addr)) \
+ : : label)
#define __put_user_nocheck(x, ptr, size) \
({ \
@@ -356,14 +343,21 @@ __pu_label: \
#define __get_user_nocheck(x, ptr, size) \
({ \
+ __label__ __gu_label; \
int __gu_err; \
__inttype(*(ptr)) __gu_val; \
__typeof__(ptr) __gu_ptr = (ptr); \
__typeof__(size) __gu_size = (size); \
__uaccess_begin_nospec(); \
- __get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_err, -EFAULT); \
+ __get_user_size(__gu_val, __gu_ptr, __gu_size, __gu_label); \
__uaccess_end(); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
+ __gu_err = 0; \
+ if (0) { \
+__gu_label: \
+ __uaccess_end(); \
+ __gu_err = -EFAULT; \
+ } \
__builtin_expect(__gu_err, 0); \
})
@@ -494,11 +488,9 @@ static __must_check __always_inline bool user_access_begin(const void __user *pt
#define unsafe_get_user(x, ptr, err_label) \
do { \
- int __gu_err; \
__inttype(*(ptr)) __gu_val; \
- __get_user_size(__gu_val, (ptr), sizeof(*(ptr)), __gu_err, -EFAULT); \
+ __get_user_size(__gu_val, (ptr), sizeof(*(ptr)), err_label); \
(x) = (__force __typeof__(*(ptr)))__gu_val; \
- if (unlikely(__gu_err)) goto err_label; \
} while (0)
/*
^ permalink raw reply related [flat|nested] 71+ messages in thread* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 22:06 ` Linus Torvalds
@ 2020-03-29 22:12 ` Linus Torvalds
0 siblings, 0 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 22:12 UTC (permalink / raw)
To: Andy Lutomirski
Cc: David Laight, Ingo Molnar, Al Viro, Thomas Gleixner, X86 ML,
LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 3:06 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Anyway, just in case people want to see it, I'm attaching my current
> unconditional patch.
Side note: I've actually been running variations of this patch for a
couple of months now, so it's even tested, but I debugged one of the
problems with the clang asm goto support it exposed only this last
week. I thought it was a problem with my patch for the longest time,
and couldn't figure it out.
So to work it needs tip-of-tree clang *and* an additional llvm bugfix
from Nick Desaulniers for that problem I reported that hasn't landed
yet.
So I included that patch just for people to look at. If you actually
want to test it, I can send you Nick's llvm fix too, and you need to
have the ability to build your own clang.
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 17:56 ` Linus Torvalds
2020-03-29 18:03 ` David Laight
@ 2020-03-29 18:16 ` Al Viro
2020-03-29 18:19 ` Linus Torvalds
1 sibling, 1 reply; 71+ messages in thread
From: Al Viro @ 2020-03-29 18:16 UTC (permalink / raw)
To: Linus Torvalds
Cc: David Laight, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
X86 ML, LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 10:56:59AM -0700, Linus Torvalds wrote:
> But, if you have lots of performance-critical get_user() calls, just use
>
> if (user_access_begin(..))
> goto efault;
>
> .. multiple "unsafe_get_user(x,ptr,efault);" ..
>
> user_access_end();
> ...
>
> efault:
> user_access_end();
> return -EFAULT;
>
> and be done with it.
Except that you'd better make that
if (!user_access_begin(...))
return -EFAULT;
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 18:16 ` Al Viro
@ 2020-03-29 18:19 ` Linus Torvalds
0 siblings, 0 replies; 71+ messages in thread
From: Linus Torvalds @ 2020-03-29 18:19 UTC (permalink / raw)
To: Al Viro
Cc: David Laight, Andy Lutomirski, Ingo Molnar, Thomas Gleixner,
X86 ML, LKML, Borislav Petkov
On Sun, Mar 29, 2020 at 11:18 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Except that you'd better make that
> if (!user_access_begin(...))
> return -EFAULT;
Yeah, that should teach me not to just write random code snippets in
the mail reader rather than just cutting-and-pasting actual working
code ;)
Linus
^ permalink raw reply [flat|nested] 71+ messages in thread
* Re: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 9:26 ` Ingo Molnar
2020-03-29 16:50 ` Andy Lutomirski
@ 2020-03-29 17:57 ` Al Viro
2020-03-30 15:54 ` David Laight
1 sibling, 1 reply; 71+ messages in thread
From: Al Viro @ 2020-03-29 17:57 UTC (permalink / raw)
To: Ingo Molnar
Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel, Borislav Petkov
On Sun, Mar 29, 2020 at 11:26:02AM +0200, Ingo Molnar wrote:
> > We'll get there; the tricky part is the ones that come in pair with
> > something other than access_ok() in the first place (many of those are
> > KVM-related, but not all such are).
> >
> > This part had been more about untangling uaccess_try stuff,,,
>
> It's much appreciated! In my previous mail I just wanted to inquire about
> the long term plan, whether we are going to get rid of all uses of
> __get_user() - to which the answer appears to be "yes". :-)
It is. FWIW, __get_user() and friends are not good interfaces; that goes
for __copy_from_user_inatomic(), etc. - the whole pile should eventually
go away.
The reason why we can't just do a blanket "convert the entire pile to
get_user() et.al." is that in some cases access_ok() is *not* what
we want. Currently they are very hard to distinguish - access_ok()
might've been done much earlier, so locating it can be tricky. And
we do have such beasts - look for example at KVM-related code.
Another fun issue is that e.g. ppc will need at some point (preferably -
over the next cycle) get their analogue of stac/clac lifted into
user_access_begin/user_access_end. The same goes for several other
architectures. And we are almost certainly will need to change
the user_access_begin()/user_access_end() calling conventions to
cover that; there had been several subthreads discussing the ways
to do it, but those will need to be resurrected and brought to some
conclusion. Until that's done I really don't want to do bulk conversions
of __get_user() to unsafe_get_user() - right now we have relatively
few user_access_begin/user_access_end blocks, but if we get a lot
of those from the stuff like e.g. comedi crap[*], any changes of calling
conventions will be a lot more noisy and painful.
[*] IMO compat_alloc_user_space() should die; this "grab some space on
user stack, copy the 32bit data structure into 64bit equivalent there,
complete with pointer chasing and creating 64bit equivalents of everything
that's referenced from that struct, then call native ioctl, then do the
reverse conversion" is just plain wrong. That native ioctl is going to
bring the structures we'd constructed back into the kernel space and
work with them there; we might as well separate the function that work
with the copied struct (usually we do have those anyway) and call those
instead the native ioctl. And skip the damn "copy the structures we'd
built into temp allocation on user stack, then have it copied back"
part. We have relatively few callers, thankfully.
I mean, take a look at compat get_mempolicy(2).
COMPAT_SYSCALL_DEFINE5(get_mempolicy, int __user *, policy,
compat_ulong_t __user *, nmask,
compat_ulong_t, maxnode,
compat_ulong_t, addr, compat_ulong_t, flags)
{
long err;
unsigned long __user *nm = NULL;
unsigned long nr_bits, alloc_size;
DECLARE_BITMAP(bm, MAX_NUMNODES);
nr_bits = min_t(unsigned long, maxnode-1, nr_node_ids);
alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8;
if (nmask)
nm = compat_alloc_user_space(alloc_size);
Allocated the one on userland stack.
err = kernel_get_mempolicy(policy, nm, nr_bits+1, addr, flags);
Called native variant, asking it to put its result in that temp object.
if (!err && nmask) {
... and if it hasn't failed, copy the sucker back into the kernel space
unsigned long copy_size;
copy_size = min_t(unsigned long, sizeof(bm), alloc_size);
err = copy_from_user(bm, nm, copy_size);
... and convert-and-copy it where the user asked.
/* ensure entire bitmap is zeroed */
err |= clear_user(nmask, ALIGN(maxnode-1, 8) / 8);
err |= compat_put_bitmap(nmask, bm, nr_bits);
}
return err;
}
OK, but kernel_get_mempolicy() only touches its second argument in the
very end:
if (nmask)
err = copy_nodes_to_user(nmask, maxnode, &nodes);
return err;
with nodes being a local variable and copy_nodes_to_user() being
{
unsigned long copy = ALIGN(maxnode-1, 64) / 8;
unsigned int nbytes = BITS_TO_LONGS(nr_node_ids) * sizeof(long);
if (copy > nbytes) {
if (copy > PAGE_SIZE)
return -EINVAL;
if (clear_user((char __user *)mask + nbytes, copy - nbytes))
return -EFAULT;
copy = nbytes;
}
return copy_to_user(mask, nodes_addr(*nodes), copy) ? -EFAULT : 0;
}
So we have local variable in kernel_get_mempolicy() filled (by call of
do_get_mempolicy()), then copied out to temp userland allocation, then
copied back into the local variable in the caller of kernel_get_mempolicy(),
then copied-with-conversion... I don't know about you, but I really
don't like that kind of code. And untangling it is not that hard, really -
something like (completely untested) delta below would deal with that one:
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 977c641f78cf..2901462da099 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1611,28 +1611,29 @@ COMPAT_SYSCALL_DEFINE5(get_mempolicy, int __user *, policy,
compat_ulong_t, maxnode,
compat_ulong_t, addr, compat_ulong_t, flags)
{
- long err;
- unsigned long __user *nm = NULL;
- unsigned long nr_bits, alloc_size;
- DECLARE_BITMAP(bm, MAX_NUMNODES);
+ int err;
+ int uninitialized_var(pval);
+ nodemask_t nodes;
- nr_bits = min_t(unsigned long, maxnode-1, nr_node_ids);
- alloc_size = ALIGN(nr_bits, BITS_PER_LONG) / 8;
+ if (nmask != NULL && maxnode < nr_node_ids)
+ return -EINVAL;
- if (nmask)
- nm = compat_alloc_user_space(alloc_size);
+ addr = untagged_addr(addr);
+
+ err = do_get_mempolicy(&pval, &nodes, addr, flags);
+ if (err)
+ return err;
- err = kernel_get_mempolicy(policy, nm, nr_bits+1, addr, flags);
+ if (policy && put_user(pval, policy))
+ return -EFAULT;
- if (!err && nmask) {
- unsigned long copy_size;
- copy_size = min_t(unsigned long, sizeof(bm), alloc_size);
- err = copy_from_user(bm, nm, copy_size);
+ if (nmask) {
+ unsigned long nr_bits;
/* ensure entire bitmap is zeroed */
+ nr_bits = min_t(unsigned long, maxnode-1, nr_node_ids);
err |= clear_user(nmask, ALIGN(maxnode-1, 8) / 8);
- err |= compat_put_bitmap(nmask, bm, nr_bits);
+ err |= compat_put_bitmap(nmask, nodes_addr(nodes), nr_bits);
}
-
return err;
}
^ permalink raw reply related [flat|nested] 71+ messages in thread* RE: [RFC][PATCH 01/22] x86 user stack frame reads: switch to explicit __get_user()
2020-03-29 17:57 ` Al Viro
@ 2020-03-30 15:54 ` David Laight
0 siblings, 0 replies; 71+ messages in thread
From: David Laight @ 2020-03-30 15:54 UTC (permalink / raw)
To: 'Al Viro', Ingo Molnar
Cc: Linus Torvalds, Thomas Gleixner, x86, linux-kernel, Borislav Petkov
From: Al Viro
> Sent: 29 March 2020 18:58
...
> [*] IMO compat_alloc_user_space() should die; this "grab some space on
> user stack, copy the 32bit data structure into 64bit equivalent there,
> complete with pointer chasing and creating 64bit equivalents of everything
> that's referenced from that struct, then call native ioctl, then do the
> reverse conversion" is just plain wrong. That native ioctl is going to
> bring the structures we'd constructed back into the kernel space and
> work with them there; we might as well separate the function that work
> with the copied struct (usually we do have those anyway) and call those
> instead the native ioctl. And skip the damn "copy the structures we'd
> built into temp allocation on user stack, then have it copied back"
> part. We have relatively few callers, thankfully.
I helped rip the same 'stackgap' code out of netbsd many years ago.
No only was it being used for system call compatibility, but
also for security checks and rewriting filenames.
Completely hopeless in a threaded program.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply [flat|nested] 71+ messages in thread