[PATCH v2 1/4] kernel.h: use parentheses around argument in u64_to_user_ptr()

[PATCH v2 1/4] kernel.h: use parentheses around argument in u64_to_user_ptr()

[Jann Horn]

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.

Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Signed-off-by: Jann Horn <jannh@google.com>
---
 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

[PATCH v2 2/4] x86/microcode: Fix __user annotations around generic_load_microcode()

[Jann Horn]

generic_load_microcode() deals with a pointer that can be either a kernel
pointer or a user pointer. Pass it around as a __user pointer so that it
can't be dereferenced accidentally while its address space is unknown.
Use explicit casts to convert between __user and __kernel to inform the
checker that these address space conversions are intentional.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

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 = {
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v2 3/4] x86/fpu: Fix __user annotations

[Jann Horn]

In save_xstate_epilog(), use __user when type-casting userspace pointers.

In setup_sigcontext() and x32_setup_rt_frame(), perform explicit __force
casts for converting userspace pointers to unsigned long; put_user_ex()
already performs a cast, but without __force, which is required by sparse
for conversions from userspace pointers to numbers.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/kernel/fpu/signal.c | 6 +++---
 arch/x86/kernel/signal.c     | 4 ++--
 2 files changed, 5 insertions(+), 5 deletions(-)

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,
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v2 4/4] x86/uaccess: Fix implicit cast of __user pointer

[Jann Horn]

The first two arguments of __user_atomic_cmpxchg_inatomic() are:

 - `uval` is a kernel pointer into which the old value should be stored
 - `ptr` is the user pointer on which the cmpxchg should operate

This means that casting `uval` to `__typeof__(ptr)` is wrong. Since `uval`
is only used once inside the macro, just get rid of __uval and use `(uval)`
directly.

Signed-off-by: Jann Horn <jannh@google.com>
---
 arch/x86/include/asm/uaccess.h | 3 +--
 1 file changed, 1 insertion(+), 2 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;								\
 })
 
-- 
2.21.0.392.gf8f6787159e-goog

Re: [PATCH v2 3/4] x86/fpu: Fix __user annotations

[Luc Van Oostenryck]

On Fri, Mar 29, 2019 at 05:30:46PM +0100, Jann Horn wrote:
> In save_xstate_epilog(), use __user when type-casting userspace pointers.
> 
> In setup_sigcontext() and x32_setup_rt_frame(), perform explicit __force
> casts for converting userspace pointers to unsigned long; put_user_ex()
> already performs a cast, but without __force, which is required by sparse
> for conversions from userspace pointers to numbers.

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

The __force here is not needed and in fact meaningless as the address
space annotations and checks only concern pointers. By casting a
pointer to an unsigned long, all type info is lost anyway and thus
no address-space checks are performed. It's a bit like such casts
always have an implicit __force already included.

> @@ -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);

Same here.

Best regards,
-- Luc Van Oostenryck

Re: [PATCH v2 3/4] x86/fpu: Fix __user annotations

[Jann Horn]

+sparse list

On Fri, Mar 29, 2019 at 7:03 PM Luc Van Oostenryck
<luc.vanoostenryck@gmail.com> wrote:
>
> On Fri, Mar 29, 2019 at 05:30:46PM +0100, Jann Horn wrote:
> > In save_xstate_epilog(), use __user when type-casting userspace pointers.
> >
> > In setup_sigcontext() and x32_setup_rt_frame(), perform explicit __force
> > casts for converting userspace pointers to unsigned long; put_user_ex()
> > already performs a cast, but without __force, which is required by sparse
> > for conversions from userspace pointers to numbers.
>
> ...
>
> > 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);
>
> The __force here is not needed and in fact meaningless as the address
> space annotations and checks only concern pointers. By casting a
> pointer to an unsigned long, all type info is lost anyway and thus
> no address-space checks are performed. It's a bit like such casts
> always have an implicit __force already included.

Oooh, it's a sparse bug.

So, without this, sparse complains:

  CHECK   arch/x86/kernel/signal.c
arch/x86/kernel/signal.c:209:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:209:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:209:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:209:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:572:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:572:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:572:17: warning: cast removes address space
'<asn:1>' of expression
arch/x86/kernel/signal.c:572:17: warning: cast removes address space
'<asn:1>' of expression

Apparently it's significant that the user pointer is stored as a
__u64, and __u64 is defined as unsigned long long.

By reducing this to a small testcase, I arrived at this:

sparse-master$ nl jannh-typeof-number.c
     1 #define __user __attribute__((noderef, address_space(1)))
     2 static unsigned long a(void __user *fpstate) {
     3   return (unsigned long long)fpstate;
     4 }
     5 static unsigned long b(void __user *fpstate) {
     6   return (unsigned long)fpstate;
     7 }
     8 static unsigned long c(void __user *fpstate) {
     9   return (unsigned int)fpstate;
    10 }
sparse-master$ ./sparse -Wall jannh-typeof-number.c
jannh-typeof-number.c:3:11: warning: cast removes address space
'<asn:1>' of expression
jannh-typeof-number.c:9:11: warning: cast removes address space
'<asn:1>' of expression
sparse-master$

I'll have a look at sparse and try to come up with a patch if I can
figure out what's going wrong.

Re: [PATCH v2 3/4] x86/fpu: Fix __user annotations

[Al Viro]

On Fri, Mar 29, 2019 at 08:25:25PM +0100, Jann Horn wrote:
> Oooh, it's a sparse bug.

It's *not* a bug.  

> Apparently it's significant that the user pointer is stored as a
> __u64, and __u64 is defined as unsigned long long.

Yes, it is.  Casts to uintptr_t (== unsigned long on all targets)
are OK; any other arithmetical type gives a warning, and quite
deliberately so.

Don't do it.  If you want to say "I'm converting it to integer,
all traces of its origin are gone", use an idiomatic cast.

Re: [PATCH v2 4/4] x86/uaccess: Fix implicit cast of __user pointer

[Mukesh Ojha]

On 3/29/2019 10:00 PM, Jann Horn wrote:
> The first two arguments of __user_atomic_cmpxchg_inatomic() are:
>
>   - `uval` is a kernel pointer into which the old value should be stored
>   - `ptr` is the user pointer on which the cmpxchg should operate
>
> This means that casting `uval` to `__typeof__(ptr)` is wrong. Since `uval`
> is only used once inside the macro, just get rid of __uval and use `(uval)`
> directly.
>
> Signed-off-by: Jann Horn <jannh@google.com>

Looks good to me.
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>

Cheers,
-Mukesh

> ---
>   arch/x86/include/asm/uaccess.h | 3 +--
>   1 file changed, 1 insertion(+), 2 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;								\
>   })
>