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@xxxxxxxxx> 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.