Re: [RFC v2 1/2] x86/fpu: Fix state corruption in __fpu__restore_sig()

[Dave Hansen <dave.hansen@intel.com>]

> +static void sigusr1(int sig, siginfo_t *info, void *uc_void)
> +{
> +       ucontext_t *uc = uc_void;
> +       uint8_t *fpstate = (uint8_t *)uc->uc_mcontext.fpregs;
> +       uint64_t *xfeatures = (uint64_t *)(fpstate + 512);
> +
> +       printf("\tOriginal XFEATURES = 0x%lx\n", *xfeatures);
> +       *(xfeatures+2) = 0xfffffff;
> +       //*xfeatures = ~(uint64_t)0;
> +}

I was trying to think of something which might be more durable than
this, like if a new feature started using bytes 16->23 for something useful.

How about a memset() of the entire 64-byte header to 0xff?

On to

	[PATCH v3 2/5] x86/fpu: Fix state corruption in ...

> Instead of trying to give sensible semantics to the fpu clear functions
> in the presence of XSAVE header corruption, simply avoid corrupting the
> header in the first place by using copy_user_to_xstate().

Should we mention that the verbatim copying via __copy_from_user()
copies the (bad) reserved bit contents and how copy_user_to_xstate()
avoids it?

Maybe:

__copy_from_user() copies the entire user buffer into the kernel buffer,
verbatim.  This means that the kernel buffer may now contain entirely
invalid state on which XRSTOR will #GP.  validate_user_xstate_header()
can detect some of that corruption, but that leaves the onus on callers
to clear the buffer.

Avoid corruption of the kernel XSAVE buffer.  Use copy_user_to_xstate()
which validates the XSAVE header contents *BEFORE* copying the buffer
into the kernel.

Note: copy_user_to_xstate() was previously only called for
compacted-format kernel buffers.  It functions for both compacted and
non-compacted forms.  Using it for the non-compacted form will be slower
because of multiple __copy_from_user() operations in the
compacted-format code, but that cost is less important than simpler code
in an already slow path.

> --- a/arch/x86/kernel/fpu/signal.c
> +++ b/arch/x86/kernel/fpu/signal.c
> @@ -405,14 +405,7 @@ static int __fpu__restore_sig(void __user *buf, void __user *buf_fx, int size)
>         if (use_xsave() && !fx_only) {
>                 u64 init_bv = xfeatures_mask_user() & ~user_xfeatures;
>  
> -               if (using_compacted_format()) {
> -                       ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
> -               } else {
> -                       ret = __copy_from_user(&fpu->state.xsave, buf_fx, state_size);
> -
> -                       if (!ret && state_size > offsetof(struct xregs_state, header))
> -                               ret = validate_user_xstate_header(&fpu->state.xsave.header);
> -               }
> +               ret = copy_user_to_xstate(&fpu->state.xsave, buf_fx);
>                 if (ret)
>                         goto err_out;
>