linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [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 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

* 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

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).