* [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() @ 2019-03-28 21:23 Jann Horn 2019-03-28 21:23 ` [PATCH 2/2] x86: fix __user annotations Jann Horn 2019-03-29 5:59 ` [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Mukesh Ojha 0 siblings, 2 replies; 8+ messages in thread From: Jann Horn @ 2019-03-28 21:23 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, jannh Cc: x86, linux-kernel, Andrew Morton, Signed-off-by : Qiaowei Ren Use parentheses around uses of the argument in u64_to_user_ptr() to ensure that the cast doesn't apply to part of the argument. There are existing uses of the macro of the form `u64_to_user_ptr(A + B)`, which expands to `(void __user *)(uintptr_t)A + B` (the cast applies to the first operand of the addition, the addition is a pointer addition). This happens to still work as intended, the semantic difference doesn't cause a difference in behavior. But I want to use u64_to_user_ptr() with a ternary operator in the argument, like so: `u64_to_user_ptr(A ? B : C)`. This currently doesn't work as intended. Fixes: f09174c501f8 ("x86: add user_atomic_cmpxchg_inatomic at uaccess.h") Signed-off-by: Jann Horn <jannh@google.com> --- Can we take this patch through the x86 tree with the following one, or do we need to get this one through akpm's tree first? include/linux/kernel.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/linux/kernel.h b/include/linux/kernel.h index 34a5036debd3..2d14e21c16c0 100644 --- a/include/linux/kernel.h +++ b/include/linux/kernel.h @@ -47,8 +47,8 @@ #define u64_to_user_ptr(x) ( \ { \ - typecheck(u64, x); \ - (void __user *)(uintptr_t)x; \ + typecheck(u64, (x)); \ + (void __user *)(uintptr_t)(x); \ } \ ) -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] x86: fix __user annotations 2019-03-28 21:23 [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Jann Horn @ 2019-03-28 21:23 ` Jann Horn 2019-03-29 13:39 ` Borislav Petkov 2019-04-01 14:37 ` David Laight 2019-03-29 5:59 ` [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Mukesh Ojha 1 sibling, 2 replies; 8+ messages in thread From: Jann Horn @ 2019-03-28 21:23 UTC (permalink / raw) To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, jannh Cc: x86, linux-kernel, Andrew Morton, Signed-off-by : Qiaowei Ren Fix __user annotations in various places across the x86 tree: - cast to wrong pointer type in __user_atomic_cmpxchg_inatomic() - generic_load_microcode() deals with a pointer that can be either a kernel pointer or a user pointer; change the code to pass it around as a __user pointer, and add explicit casts to convert between __user and __kernel - save_xstate_epilog() has missing __user in explicit casts - setup_sigcontext() and x32_setup_rt_frame() rely on the cast performed by put_user_ex() on its first argument, but sparse requires __force for casting __user pointers to unsigned long - xen_hvm_config() has missing __user This patch removes all sparse warnings about the asn:1 address space (__user) in arch/x86/ for my kernel config. Signed-off-by: Jann Horn <jannh@google.com> --- This patch requires the previous one, "[PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr()", otherwise xen_hvm_config() breaks. Can we take both together through the x86 tree, or does the first one have to go through akpm's tree? arch/x86/include/asm/uaccess.h | 3 +-- arch/x86/include/asm/uaccess_64.h | 2 +- arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++-------- arch/x86/kernel/fpu/signal.c | 6 +++--- arch/x86/kernel/signal.c | 4 ++-- arch/x86/kvm/x86.c | 8 ++++---- arch/x86/lib/usercopy_64.c | 4 +++- 7 files changed, 26 insertions(+), 21 deletions(-) diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h index 1954dd5552a2..a21f2a2f17bf 100644 --- a/arch/x86/include/asm/uaccess.h +++ b/arch/x86/include/asm/uaccess.h @@ -585,7 +585,6 @@ extern void __cmpxchg_wrong_size(void) #define __user_atomic_cmpxchg_inatomic(uval, ptr, old, new, size) \ ({ \ int __ret = 0; \ - __typeof__(ptr) __uval = (uval); \ __typeof__(*(ptr)) __old = (old); \ __typeof__(*(ptr)) __new = (new); \ __uaccess_begin_nospec(); \ @@ -661,7 +660,7 @@ extern void __cmpxchg_wrong_size(void) __cmpxchg_wrong_size(); \ } \ __uaccess_end(); \ - *__uval = __old; \ + *(uval) = __old; \ __ret; \ }) diff --git a/arch/x86/include/asm/uaccess_64.h b/arch/x86/include/asm/uaccess_64.h index a9d637bc301d..cbca2cb28939 100644 --- a/arch/x86/include/asm/uaccess_64.h +++ b/arch/x86/include/asm/uaccess_64.h @@ -208,7 +208,7 @@ __copy_from_user_flushcache(void *dst, const void __user *src, unsigned size) } unsigned long -copy_user_handle_tail(char *to, char *from, unsigned len); +copy_user_handle_tail(char __user *to, char __user *from, unsigned len); unsigned long mcsafe_handle_tail(char *to, char *from, unsigned len); diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c index 16936a24795c..e8ef65c275c7 100644 --- a/arch/x86/kernel/cpu/microcode/intel.c +++ b/arch/x86/kernel/cpu/microcode/intel.c @@ -861,11 +861,13 @@ static enum ucode_state apply_microcode_intel(int cpu) return ret; } -static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size, - int (*get_ucode_data)(void *, const void *, size_t)) +static enum ucode_state generic_load_microcode(int cpu, + const void __user *data, size_t size, + int (*get_ucode_data)(void *, const void __user *, size_t)) { struct ucode_cpu_info *uci = ucode_cpu_info + cpu; - u8 *ucode_ptr = data, *new_mc = NULL, *mc = NULL; + const u8 __user *ucode_ptr = data; + u8 *new_mc = NULL, *mc = NULL; int new_rev = uci->cpu_sig.rev; unsigned int leftover = size; unsigned int curr_mc_size = 0, new_mc_size = 0; @@ -945,9 +947,10 @@ static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size, return ret; } -static int get_ucode_fw(void *to, const void *from, size_t n) +static int get_ucode_fw(void *to, const void __user *from, size_t n) { - memcpy(to, from, n); + /* cast paired with request_microcode_fw() */ + memcpy(to, (const void __force *)from, n); return 0; } @@ -993,7 +996,8 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, return UCODE_NFOUND; } - ret = generic_load_microcode(cpu, (void *)firmware->data, + /* cast paired with get_ucode_fw() */ + ret = generic_load_microcode(cpu, (void __force __user *)firmware->data, firmware->size, &get_ucode_fw); release_firmware(firmware); @@ -1001,7 +1005,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, return ret; } -static int get_ucode_user(void *to, const void *from, size_t n) +static int get_ucode_user(void *to, const void __user *from, size_t n) { return copy_from_user(to, from, n); } @@ -1012,7 +1016,7 @@ request_microcode_user(int cpu, const void __user *buf, size_t size) if (is_blacklisted(cpu)) return UCODE_NFOUND; - return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user); + return generic_load_microcode(cpu, buf, size, &get_ucode_user); } static struct microcode_ops microcode_intel_ops = { diff --git a/arch/x86/kernel/fpu/signal.c b/arch/x86/kernel/fpu/signal.c index f6a1d299627c..55b80de13ea5 100644 --- a/arch/x86/kernel/fpu/signal.c +++ b/arch/x86/kernel/fpu/signal.c @@ -92,13 +92,13 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame) return err; err |= __put_user(FP_XSTATE_MAGIC2, - (__u32 *)(buf + fpu_user_xstate_size)); + (__u32 __user *)(buf + fpu_user_xstate_size)); /* * Read the xfeatures which we copied (directly from the cpu or * from the state in task struct) to the user buffers. */ - err |= __get_user(xfeatures, (__u32 *)&x->header.xfeatures); + err |= __get_user(xfeatures, (__u32 __user *)&x->header.xfeatures); /* * For legacy compatible, we always set FP/SSE bits in the bit @@ -113,7 +113,7 @@ static inline int save_xstate_epilog(void __user *buf, int ia32_frame) */ xfeatures |= XFEATURE_MASK_FPSSE; - err |= __put_user(xfeatures, (__u32 *)&x->header.xfeatures); + err |= __put_user(xfeatures, (__u32 __user *)&x->header.xfeatures); return err; } diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c index 08dfd4c1a4f9..e13cd972f9af 100644 --- a/arch/x86/kernel/signal.c +++ b/arch/x86/kernel/signal.c @@ -206,7 +206,7 @@ int setup_sigcontext(struct sigcontext __user *sc, void __user *fpstate, put_user_ex(regs->ss, &sc->ss); #endif /* CONFIG_X86_32 */ - put_user_ex(fpstate, &sc->fpstate); + put_user_ex((unsigned long __force)fpstate, &sc->fpstate); /* non-iBCS2 extensions.. */ put_user_ex(mask, &sc->oldmask); @@ -569,7 +569,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig, restorer = NULL; err |= -EFAULT; } - put_user_ex(restorer, &frame->pretcode); + put_user_ex((unsigned long __force)restorer, &frame->pretcode); } put_user_catch(err); err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate, diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 65e4559eef2f..ca087debefbf 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -2317,11 +2317,11 @@ static int set_msr_mce(struct kvm_vcpu *vcpu, struct msr_data *msr_info) static int xen_hvm_config(struct kvm_vcpu *vcpu, u64 data) { struct kvm *kvm = vcpu->kvm; + struct kvm_xen_hvm_config *cfg = &kvm->arch.xen_hvm_config; int lm = is_long_mode(vcpu); - u8 *blob_addr = lm ? (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_64 - : (u8 *)(long)kvm->arch.xen_hvm_config.blob_addr_32; - u8 blob_size = lm ? kvm->arch.xen_hvm_config.blob_size_64 - : kvm->arch.xen_hvm_config.blob_size_32; + u8 __user *blob_addr = + u64_to_user_ptr(lm ? cfg->blob_addr_64 : cfg->blob_addr_32); + u8 blob_size = lm ? cfg->blob_size_64 : cfg->blob_size_32; u32 page_num = data & ~PAGE_MASK; u64 page_addr = data & PAGE_MASK; u8 *page; diff --git a/arch/x86/lib/usercopy_64.c b/arch/x86/lib/usercopy_64.c index ee42bb0cbeb3..cd8a1802adde 100644 --- a/arch/x86/lib/usercopy_64.c +++ b/arch/x86/lib/usercopy_64.c @@ -58,9 +58,11 @@ EXPORT_SYMBOL(clear_user); * Try to copy last bytes and clear the rest if needed. * Since protection fault in copy_from/to_user is not a normal situation, * it is not necessary to optimize tail handling. + * We don't know which side of the copy is in userspace; we treat both sides as + * user pointers. */ __visible unsigned long -copy_user_handle_tail(char *to, char *from, unsigned len) +copy_user_handle_tail(char __user *to, char __user *from, unsigned len) { for (; len; --len, to++) { char c; -- 2.21.0.392.gf8f6787159e-goog ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86: fix __user annotations 2019-03-28 21:23 ` [PATCH 2/2] x86: fix __user annotations Jann Horn @ 2019-03-29 13:39 ` Borislav Petkov 2019-03-29 15:24 ` Ben Dooks 2019-03-29 16:02 ` Jann Horn 2019-04-01 14:37 ` David Laight 1 sibling, 2 replies; 8+ messages in thread From: Borislav Petkov @ 2019-03-29 13:39 UTC (permalink / raw) To: Jann Horn Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Andrew Morton, Qiaowei Ren, Ben Dooks On Thu, Mar 28, 2019 at 10:23:21PM +0100, Jann Horn wrote: > Fix __user annotations in various places across the x86 tree: > > - cast to wrong pointer type in __user_atomic_cmpxchg_inatomic() > - generic_load_microcode() deals with a pointer that can be either a > kernel pointer or a user pointer; change the code to pass it around as > a __user pointer, and add explicit casts to convert between __user and > __kernel > - save_xstate_epilog() has missing __user in explicit casts > - setup_sigcontext() and x32_setup_rt_frame() rely on the cast performed > by put_user_ex() on its first argument, but sparse requires __force for > casting __user pointers to unsigned long > - xen_hvm_config() has missing __user > > This patch removes all sparse warnings about the asn:1 address space > (__user) in arch/x86/ for my kernel config. > > Signed-off-by: Jann Horn <jannh@google.com> > --- > This patch requires the previous one, "[PATCH 1/2] kernel.h: use > parentheses around argument in u64_to_user_ptr()", otherwise > xen_hvm_config() breaks. Can we take both together through the x86 tree, > or does the first one have to go through akpm's tree? I don't see why not, unless akpm has objections. However, > arch/x86/include/asm/uaccess.h | 3 +-- > arch/x86/include/asm/uaccess_64.h | 2 +- This chunk is being discussed here already: https://lkml.kernel.org/r/20190228185027.2480-1-ben.dooks@codethink.co.uk and I'd like to take Ben's v2 when Ben adds Linus' explanation. Then, it would be probably easier if you could split that patch into: > arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++-------- microcode > arch/x86/kernel/fpu/signal.c | 6 +++--- > arch/x86/kernel/signal.c | 4 ++-- fpu patch > arch/x86/kvm/x86.c | 8 ++++---- kvm patch which would make review/merging/etc considerably easier. Thx. -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86: fix __user annotations 2019-03-29 13:39 ` Borislav Petkov @ 2019-03-29 15:24 ` Ben Dooks 2019-03-29 16:02 ` Jann Horn 1 sibling, 0 replies; 8+ messages in thread From: Ben Dooks @ 2019-03-29 15:24 UTC (permalink / raw) To: Borislav Petkov Cc: Jann Horn, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, linux-kernel, Andrew Morton, Qiaowei Ren On 2019-03-29 13:39, Borislav Petkov wrote: > On Thu, Mar 28, 2019 at 10:23:21PM +0100, Jann Horn wrote: >> Fix __user annotations in various places across the x86 tree: >> >> - cast to wrong pointer type in __user_atomic_cmpxchg_inatomic() >> - generic_load_microcode() deals with a pointer that can be either a >> kernel pointer or a user pointer; change the code to pass it around >> as >> a __user pointer, and add explicit casts to convert between __user >> and >> __kernel >> - save_xstate_epilog() has missing __user in explicit casts >> - setup_sigcontext() and x32_setup_rt_frame() rely on the cast >> performed >> by put_user_ex() on its first argument, but sparse requires __force >> for >> casting __user pointers to unsigned long >> - xen_hvm_config() has missing __user >> >> This patch removes all sparse warnings about the asn:1 address space >> (__user) in arch/x86/ for my kernel config. >> >> Signed-off-by: Jann Horn <jannh@google.com> >> --- >> This patch requires the previous one, "[PATCH 1/2] kernel.h: use >> parentheses around argument in u64_to_user_ptr()", otherwise >> xen_hvm_config() breaks. Can we take both together through the x86 >> tree, >> or does the first one have to go through akpm's tree? > > I don't see why not, unless akpm has objections. > > However, > >> arch/x86/include/asm/uaccess.h | 3 +-- >> arch/x86/include/asm/uaccess_64.h | 2 +- > > This chunk is being discussed here already: > > https://lkml.kernel.org/r/20190228185027.2480-1-ben.dooks@codethink.co.uk > > and I'd like to take Ben's v2 when Ben adds Linus' explanation. I'll sort that out in a bit, thanks for reviewing. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86: fix __user annotations 2019-03-29 13:39 ` Borislav Petkov 2019-03-29 15:24 ` Ben Dooks @ 2019-03-29 16:02 ` Jann Horn 1 sibling, 0 replies; 8+ messages in thread From: Jann Horn @ 2019-03-29 16:02 UTC (permalink / raw) To: Borislav Petkov Cc: Thomas Gleixner, Ingo Molnar, H. Peter Anvin, the arch/x86 maintainers, kernel list, Andrew Morton, Qiaowei Ren, Ben Dooks On Fri, Mar 29, 2019 at 2:39 PM Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Mar 28, 2019 at 10:23:21PM +0100, Jann Horn wrote: > > Fix __user annotations in various places across the x86 tree: > > > > - cast to wrong pointer type in __user_atomic_cmpxchg_inatomic() > > - generic_load_microcode() deals with a pointer that can be either a > > kernel pointer or a user pointer; change the code to pass it around as > > a __user pointer, and add explicit casts to convert between __user and > > __kernel > > - save_xstate_epilog() has missing __user in explicit casts > > - setup_sigcontext() and x32_setup_rt_frame() rely on the cast performed > > by put_user_ex() on its first argument, but sparse requires __force for > > casting __user pointers to unsigned long > > - xen_hvm_config() has missing __user > > > > This patch removes all sparse warnings about the asn:1 address space > > (__user) in arch/x86/ for my kernel config. > > > > Signed-off-by: Jann Horn <jannh@google.com> > > --- > > This patch requires the previous one, "[PATCH 1/2] kernel.h: use > > parentheses around argument in u64_to_user_ptr()", otherwise > > xen_hvm_config() breaks. Can we take both together through the x86 tree, > > or does the first one have to go through akpm's tree? > > I don't see why not, unless akpm has objections. > > However, > > > arch/x86/include/asm/uaccess.h | 3 +-- > > arch/x86/include/asm/uaccess_64.h | 2 +- > > This chunk is being discussed here already: > > https://lkml.kernel.org/r/20190228185027.2480-1-ben.dooks@codethink.co.uk > > and I'd like to take Ben's v2 when Ben adds Linus' explanation. Alright, dropped the changes in: arch/x86/lib/usercopy_64.c arch/x86/include/asm/uaccess_64.h > Then, it would be probably easier if you could split that patch into: > > > arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++-------- > > microcode > > > arch/x86/kernel/fpu/signal.c | 6 +++--- > > arch/x86/kernel/signal.c | 4 ++-- > > fpu patch > > > arch/x86/kvm/x86.c | 8 ++++---- Split these out as suggested. Additionally, I've split out one commit for x86/uaccess (the change to arch/x86/include/asm/uaccess.h wasn't in Ben's patch). > kvm patch Actually, looking at the log of that file, and at MAINTAINERS, it looks like that should go through the KVM tree? There's also something I want to fix in virt/kvm/kvm_main.c, so I have to send some stuff to Paolo anyway. ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 2/2] x86: fix __user annotations 2019-03-28 21:23 ` [PATCH 2/2] x86: fix __user annotations Jann Horn 2019-03-29 13:39 ` Borislav Petkov @ 2019-04-01 14:37 ` David Laight 2019-04-01 15:04 ` Jann Horn 1 sibling, 1 reply; 8+ messages in thread From: David Laight @ 2019-04-01 14:37 UTC (permalink / raw) To: 'Jann Horn', Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin Cc: x86, linux-kernel, Andrew Morton, Signed-off-by : Qiaowei Ren From: Jann Horn > Sent: 28 March 2019 21:23 > Fix __user annotations in various places across the x86 tree: > ... > - generic_load_microcode() deals with a pointer that can be either a > kernel pointer or a user pointer; change the code to pass it around as > a __user pointer, and add explicit casts to convert between __user and > __kernel ... > -static int get_ucode_fw(void *to, const void *from, size_t n) > +static int get_ucode_fw(void *to, const void __user *from, size_t n) > { > - memcpy(to, from, n); > + /* cast paired with request_microcode_fw() */ > + memcpy(to, (const void __force *)from, n); > return 0; > } > > @@ -993,7 +996,8 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, > return UCODE_NFOUND; > } > > - ret = generic_load_microcode(cpu, (void *)firmware->data, > + /* cast paired with get_ucode_fw() */ > + ret = generic_load_microcode(cpu, (void __force __user *)firmware->data, > firmware->size, &get_ucode_fw); > > release_firmware(firmware); > @@ -1001,7 +1005,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, > return ret; > } > > -static int get_ucode_user(void *to, const void *from, size_t n) > +static int get_ucode_user(void *to, const void __user *from, size_t n) > { > return copy_from_user(to, from, n); > } > @@ -1012,7 +1016,7 @@ request_microcode_user(int cpu, const void __user *buf, size_t size) > if (is_blacklisted(cpu)) > return UCODE_NFOUND; > > - return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user); > + return generic_load_microcode(cpu, buf, size, &get_ucode_user); That is all an 'accident waiting to happen' ... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales) ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] x86: fix __user annotations 2019-04-01 14:37 ` David Laight @ 2019-04-01 15:04 ` Jann Horn 0 siblings, 0 replies; 8+ messages in thread From: Jann Horn @ 2019-04-01 15:04 UTC (permalink / raw) To: David Laight Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, x86, linux-kernel, Andrew Morton, Qiaowei Ren On Mon, Apr 1, 2019 at 4:36 PM David Laight <David.Laight@aculab.com> wrote: > > From: Jann Horn > > Sent: 28 March 2019 21:23 > > Fix __user annotations in various places across the x86 tree: > > > ... > > - generic_load_microcode() deals with a pointer that can be either a > > kernel pointer or a user pointer; change the code to pass it around as > > a __user pointer, and add explicit casts to convert between __user and > > __kernel > ... > > -static int get_ucode_fw(void *to, const void *from, size_t n) > > +static int get_ucode_fw(void *to, const void __user *from, size_t n) > > { > > - memcpy(to, from, n); > > + /* cast paired with request_microcode_fw() */ > > + memcpy(to, (const void __force *)from, n); > > return 0; > > } > > > > @@ -993,7 +996,8 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, > > return UCODE_NFOUND; > > } > > > > - ret = generic_load_microcode(cpu, (void *)firmware->data, > > + /* cast paired with get_ucode_fw() */ > > + ret = generic_load_microcode(cpu, (void __force __user *)firmware->data, > > firmware->size, &get_ucode_fw); > > > > release_firmware(firmware); > > @@ -1001,7 +1005,7 @@ static enum ucode_state request_microcode_fw(int cpu, struct device *device, > > return ret; > > } > > > > -static int get_ucode_user(void *to, const void *from, size_t n) > > +static int get_ucode_user(void *to, const void __user *from, size_t n) > > { > > return copy_from_user(to, from, n); > > } > > @@ -1012,7 +1016,7 @@ request_microcode_user(int cpu, const void __user *buf, size_t size) > > if (is_blacklisted(cpu)) > > return UCODE_NFOUND; > > > > - return generic_load_microcode(cpu, (void *)buf, size, &get_ucode_user); > > + return generic_load_microcode(cpu, buf, size, &get_ucode_user); > > That is all an 'accident waiting to happen' ... What's your suggestion? The code used to store user pointers in kernel-typed pointers. Now it only stores kernel pointers in user-typed pointers, which is much less hazardous. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() 2019-03-28 21:23 [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Jann Horn 2019-03-28 21:23 ` [PATCH 2/2] x86: fix __user annotations Jann Horn @ 2019-03-29 5:59 ` Mukesh Ojha 1 sibling, 0 replies; 8+ messages in thread From: Mukesh Ojha @ 2019-03-29 5:59 UTC (permalink / raw) To: Jann Horn, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin Cc: x86, linux-kernel, Andrew Morton, Signed-off-by : Qiaowei Ren On 3/29/2019 2:53 AM, Jann Horn wrote: > Use parentheses around uses of the argument in u64_to_user_ptr() to ensure > that the cast doesn't apply to part of the argument. > > There are existing uses of the macro of the form `u64_to_user_ptr(A + B)`, > which expands to `(void __user *)(uintptr_t)A + B` (the cast applies to the > first operand of the addition, the addition is a pointer addition). This > happens to still work as intended, the semantic difference doesn't cause a > difference in behavior. > But I want to use u64_to_user_ptr() with a ternary operator in the > argument, like so: `u64_to_user_ptr(A ? B : C)`. This currently doesn't > work as intended. > > Fixes: f09174c501f8 ("x86: add user_atomic_cmpxchg_inatomic at uaccess.h") > Signed-off-by: Jann Horn <jannh@google.com> Looks good to me. Reviewed-by: Mukesh Ojha <mojha@codeaurora.org> -Mukesh > --- > Can we take this patch through the x86 tree with the following one, or > do we need to get this one through akpm's tree first? > > include/linux/kernel.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/kernel.h b/include/linux/kernel.h > index 34a5036debd3..2d14e21c16c0 100644 > --- a/include/linux/kernel.h > +++ b/include/linux/kernel.h > @@ -47,8 +47,8 @@ > > #define u64_to_user_ptr(x) ( \ > { \ > - typecheck(u64, x); \ > - (void __user *)(uintptr_t)x; \ > + typecheck(u64, (x)); \ > + (void __user *)(uintptr_t)(x); \ > } \ > ) > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-04-01 15:04 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-28 21:23 [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Jann Horn 2019-03-28 21:23 ` [PATCH 2/2] x86: fix __user annotations Jann Horn 2019-03-29 13:39 ` Borislav Petkov 2019-03-29 15:24 ` Ben Dooks 2019-03-29 16:02 ` Jann Horn 2019-04-01 14:37 ` David Laight 2019-04-01 15:04 ` Jann Horn 2019-03-29 5:59 ` [PATCH 1/2] kernel.h: use parentheses around argument in u64_to_user_ptr() Mukesh Ojha
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).