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 <[email protected]>
Signed-off-by: Jann Horn <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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
+sparse list
On Fri, Mar 29, 2019 at 7:03 PM Luc Van Oostenryck
<[email protected]> 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.
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.
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 <[email protected]>
Looks good to me.
Reviewed-by: Mukesh Ojha <[email protected]>
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; \
> })
>