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

[PATCH v3 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 v3 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 v3 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(), cast the userspace
pointers to 'unsigned long __user *' before writing into them.
These pointers are originally '__u32 __user *' or '__u64 __user *',
causing sparse to complain when a userspace pointer is written into
them. The casts are okay because the pointers always point to
pointer-sized values.
Thanks to Luc Van Oostenryck and Al Viro for explaining this to me.

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..b419e1a1a0ce 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(fpstate, (unsigned long __user *)&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(restorer, (unsigned long __user *)&frame->pretcode);
 	} put_user_catch(err);
 
 	err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
-- 
2.21.0.392.gf8f6787159e-goog

[PATCH v3 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.

Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
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 v3 2/4] x86/microcode: Fix __user annotations around generic_load_microcode()

[Borislav Petkov]

On Fri, Mar 29, 2019 at 10:46:50PM +0100, Jann Horn wrote:
> 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))

Ok, how about something completely different?

This ->get_ucode_data() BIOS-code-like contraption has always bugged me
for being too ugly to live.

How about we vmalloc() a properly sized buffer - both
generic_load_microcode() callers have the size - and then hand that
buffer into generic_load_microcode() ?

That solves the __user annotation fun immediately and would simplify
generic_load_microcode() additionally.

The disadvantage would be having to vmalloc() a couple of... , I think
it is megabytes, with that old loading method request_microcode_user()
but then if vmalloc() fails, then it was clearly too big. I don't think
the blob can ever be that big though, to fail vmalloc(), but I'm not
going to bet on it...

Hmmm...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

[Jann Horn]

On Mon, Apr 1, 2019 at 7:30 PM Borislav Petkov <bp@alien8.de> wrote:
>
> On Fri, Mar 29, 2019 at 10:46:50PM +0100, Jann Horn wrote:
> > 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))
>
> Ok, how about something completely different?
>
> This ->get_ucode_data() BIOS-code-like contraption has always bugged me
> for being too ugly to live.
>
> How about we vmalloc() a properly sized buffer - both
> generic_load_microcode() callers have the size - and then hand that
> buffer into generic_load_microcode() ?
>
> That solves the __user annotation fun immediately and would simplify
> generic_load_microcode() additionally.
>
> The disadvantage would be having to vmalloc() a couple of... , I think
> it is megabytes, with that old loading method request_microcode_user()
> but then if vmalloc() fails, then it was clearly too big. I don't think
> the blob can ever be that big though, to fail vmalloc(), but I'm not
> going to bet on it...

Hm. request_microcode_fw() gets that buffer from
request_firmware_direct(), which does this:

        __module_get(THIS_MODULE);
        ret = _request_firmware(firmware_p, name, device, NULL, 0,
                                FW_OPT_UEVENT | FW_OPT_NO_WARN |
                                FW_OPT_NOFALLBACK);
        module_put(THIS_MODULE);
        return ret;

What is that module_get()/module_put() supposed to be good for? Are we
expecting that caller to do something ridiculous like calling
module_put() on us? This doesn't seem to make any sense.

And then _request_firmware() goes and ends up in places like
kernel_read_file(), which already use vmalloc().


Anyway, isn't this kind of thing exactly why we have that iov_iter
stuff? request_microcode_fw() can build an ITER_KVEC,
request_microcode_user() can build an ITER_IOVEC. And then
generic_load_microcode() can use something like copy_from_iter(). Does
that sound reasonable?

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

[David Laight]

From: Jann Horn
> Sent: 01 April 2019 18:54
...
> > This ->get_ucode_data() BIOS-code-like contraption has always bugged me
> > for being too ugly to live.
> >
> > How about we vmalloc() a properly sized buffer - both
> > generic_load_microcode() callers have the size - and then hand that
> > buffer into generic_load_microcode() ?
> >
> > That solves the __user annotation fun immediately and would simplify
> > generic_load_microcode() additionally.
> >
> > The disadvantage would be having to vmalloc() a couple of... , I think
> > it is megabytes, with that old loading method request_microcode_user()
> > but then if vmalloc() fails, then it was clearly too big. I don't think
> > the blob can ever be that big though, to fail vmalloc(), but I'm not
> > going to bet on it...
> 
> Hm. request_microcode_fw() gets that buffer from
> request_firmware_direct(), which does this:
> 
>         __module_get(THIS_MODULE);
>         ret = _request_firmware(firmware_p, name, device, NULL, 0,
>                                 FW_OPT_UEVENT | FW_OPT_NO_WARN |
>                                 FW_OPT_NOFALLBACK);
>         module_put(THIS_MODULE);
>         return ret;
> 
> What is that module_get()/module_put() supposed to be good for? Are we
> expecting that caller to do something ridiculous like calling
> module_put() on us? This doesn't seem to make any sense.

At least it isn't doing a try_module_get(THIS_MODULE) :-)

> And then _request_firmware() goes and ends up in places like
> kernel_read_file(), which already use vmalloc().
> 
> 
> Anyway, isn't this kind of thing exactly why we have that iov_iter
> stuff? request_microcode_fw() can build an ITER_KVEC,
> request_microcode_user() can build an ITER_IOVEC. And then
> generic_load_microcode() can use something like copy_from_iter(). Does
> that sound reasonable?

That ought to allow the microcode be copied in chunks - removing the
need for a massive buffer?

The largest file we ever copy to PCIe cards is a 6MB fpga image.
But we do that by mmapping the PCIe registers directly into userspace.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

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

[Borislav Petkov]

On Mon, Apr 01, 2019 at 07:53:46PM +0200, Jann Horn wrote:
> Hm. request_microcode_fw() gets that buffer from
> request_firmware_direct(), which does this:
> 
>         __module_get(THIS_MODULE);
>         ret = _request_firmware(firmware_p, name, device, NULL, 0,
>                                 FW_OPT_UEVENT | FW_OPT_NO_WARN |
>                                 FW_OPT_NOFALLBACK);
>         module_put(THIS_MODULE);
>         return ret;
> 
> What is that module_get()/module_put() supposed to be good for? Are we
> expecting that caller to do something ridiculous like calling
> module_put() on us? This doesn't seem to make any sense.

Yah, the microcode thing used to be a module. Not anymore.

> And then _request_firmware() goes and ends up in places like
> kernel_read_file(), which already use vmalloc().
> 
> Anyway, isn't this kind of thing exactly why we have that iov_iter
> stuff? request_microcode_fw() can build an ITER_KVEC,
> request_microcode_user() can build an ITER_IOVEC. And then
> generic_load_microcode() can use something like copy_from_iter(). Does
> that sound reasonable?

/me doesn't know that interface, goes and looks...

You mean doing something like iov_iter_init() ... and then
copy_from_iter()? I'm looking at vhost_vsock_alloc_pkt() as an example
for a user of that interface.

The only thing I'm unsure about is the use case: that iov thing uses
a bunch of segments with separate lengths AFAICT which it copies
back'n'forth.

The loader does that sequentially in the sense that it parses the header
first, does some checks, then computes the size of the trailing patch
data which then copies again, see those ->get_ucode_data invocations.

Which would mean that the iov_* stuff would be always called with a
single segment of some length. Not that it is an issue, just saying.

That's why I suggested doing the trivial thing of copying the whole
buffer at once but I tend to prefer simple things so... :-)

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

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

[David Laight]

From: Borislav Petkov
> Sent: 02 April 2019 11:02
> 
> On Mon, Apr 01, 2019 at 07:53:46PM +0200, Jann Horn wrote:
> > Hm. request_microcode_fw() gets that buffer from
> > request_firmware_direct(), which does this:
> >
> >         __module_get(THIS_MODULE);
> >         ret = _request_firmware(firmware_p, name, device, NULL, 0,
> >                                 FW_OPT_UEVENT | FW_OPT_NO_WARN |
> >                                 FW_OPT_NOFALLBACK);
> >         module_put(THIS_MODULE);
> >         return ret;
> >
> > What is that module_get()/module_put() supposed to be good for? Are we
> > expecting that caller to do something ridiculous like calling
> > module_put() on us? This doesn't seem to make any sense.
> 
> Yah, the microcode thing used to be a module. Not anymore.

That probably makes diddly-squit difference.
There has to be a 'hold' on THIS_MODULE in order to be executing
code from it.
You only (usually) need an extra reference for a kernel thread,
and that needs to exit with module_put_and_exit().

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

[tip:core/urgent] linux/kernel.h: Use parentheses around argument in u64_to_user_ptr()

[tip-bot for Jann Horn]

Commit-ID:  a0fe2c6479aab5723239b315ef1b552673f434a3
Gitweb:     https://git.kernel.org/tip/a0fe2c6479aab5723239b315ef1b552673f434a3
Author:     Jann Horn <jannh@google.com>
AuthorDate: Fri, 29 Mar 2019 22:46:49 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Wed, 3 Apr 2019 11:43:49 +0200

linux/kernel.h: Use parentheses around argument in u64_to_user_ptr()

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.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cc: Andrei Vagin <avagin@openvz.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Jani Nikula <jani.nikula@intel.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: NeilBrown <neilb@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Qiaowei Ren <qiaowei.ren@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190329214652.258477-1-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);	\
 }					\
 )
 

[tip:x86/fpu] x86/fpu: Fix __user annotations

[tip-bot for Jann Horn]

Commit-ID:  89833fab15d6017ba006a45b5af68caa067171a7
Gitweb:     https://git.kernel.org/tip/89833fab15d6017ba006a45b5af68caa067171a7
Author:     Jann Horn <jannh@google.com>
AuthorDate: Fri, 29 Mar 2019 22:46:51 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Wed, 3 Apr 2019 14:12:40 +0200

x86/fpu: Fix __user annotations

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

In setup_sigcontext() and x32_setup_rt_frame(), cast the userspace
pointers to 'unsigned long __user *' before writing into them. These
pointers are originally '__u32 __user *' or '__u64 __user *', causing
sparse to complain when a userspace pointer is written into them. The
casts are okay because the pointers always point to pointer-sized
values.

Thanks to Luc Van Oostenryck and Al Viro for explaining this to me.

Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Mukesh Ojha <mojha@codeaurora.org>
Cc: Qiaowei Ren <qiaowei.ren@intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190329214652.258477-3-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..b419e1a1a0ce 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(fpstate, (unsigned long __user *)&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(restorer, (unsigned long __user *)&frame->pretcode);
 	} put_user_catch(err);
 
 	err |= setup_sigcontext(&frame->uc.uc_mcontext, fpstate,

[tip:x86/asm] x86/uaccess: Fix implicit cast of __user pointer

[tip-bot for Jann Horn]

Commit-ID:  a6cbfbe6677efb5ca47bb7958c2718236c25126e
Gitweb:     https://git.kernel.org/tip/a6cbfbe6677efb5ca47bb7958c2718236c25126e
Author:     Jann Horn <jannh@google.com>
AuthorDate: Fri, 29 Mar 2019 22:46:52 +0100
Committer:  Borislav Petkov <bp@suse.de>
CommitDate: Wed, 3 Apr 2019 16:26:17 +0200

x86/uaccess: Fix implicit cast of __user pointer

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>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Mike Rapoport <rppt@linux.ibm.com>
Cc: Qiaowei Ren <qiaowei.ren@intel.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Will Deacon <will.deacon@arm.com>
Cc: x86-ml <x86@kernel.org>
Link: https://lkml.kernel.org/r/20190329214652.258477-4-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;								\
 })