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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Looks good to me.
Reviewed-by: Mukesh Ojha <[email protected]>
-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); \
> } \
> )
>
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 <[email protected]>
> ---
> 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/[email protected]
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.
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 <[email protected]>
>> ---
>> 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/[email protected]
>
> 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.
On Fri, Mar 29, 2019 at 2:39 PM Borislav Petkov <[email protected]> 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 <[email protected]>
> > ---
> > 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/[email protected]
>
> 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.
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)
On Mon, Apr 1, 2019 at 4:36 PM David Laight <[email protected]> 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.