Re: [patch V2 00/14] x86/fpu: Mop up XSAVES and related damage

[Thomas Gleixner]

On Mon, Jun 07 2021 at 09:38, Dave Hansen wrote:
> On 6/7/21 7:08 AM, Thomas Gleixner wrote:
>>> By the way, are you talking specifically about the _error_ paths where
>>> the kernel is unable to XRSTOR the signal XSAVE buffer for some reason,
>>> and tries to apply either init_fpu or the hardware init state instead?
>> 
>> 1) Successful XRSTOR from user if the PKRU feature bit in the
>>    sigframe xsave.header.xfeatures is cleared. Both fast and slow path.
>
> It seems like the suggestion here is to inject 'init_pkru_value' in all
> cases where the kernel would be injecting the hardware init value.  I
> don't think we should go that far.
>
> If a signal handler sets xsave.header.xfeatures[PKRU]=0, I can't imagine
> any other intent than wanting the hardware init value.

Fine. But PKRU=0 is broken today...

T1 in user space
     wrpkru(0)

T1 -> kernel
     schedule()
       XSAVE(S) -> T1->xsave.header.xfeatures[PKRU] == 0
       T1->flags |= TIF_NEED_FPU_LOAD;
       
       wrpkru();

     schedule()
       ...
       pk = get_xsave_addr(&T1->fpu->state.xsave, XFEATURE_PKRU);
       if (pk)
         wrpkru(pk->pkru);
       else
         wrpkru(DEFAULT_PKRU);

But because PKRU of T1 was 0, the xfeatures bit is 0 and therefore the
value in the xsave storage is not valid. Which makes get_xsave_addr()
return NULL and switch_to() writes the default PKRU.

So that wreckages any copy_to/from_user() on the way back to user space
which hits memory which is protected by the default PKRU value.

Assumed that this does not fail (pure luck) then T1 goes back to user
space and because TIF_NEED_FPU_LOAD is set it ends up in

switch_fpu_return()
    __fpregs_load_activate()
      if (!fpregs_state_valid()) {
         load_XSTATE_from_task();
      }

But because nothing touched the FPU between schedule out and schedule in
of T1 the fpregs_state is valid so switch_fpu_return() does nothing and
just clears TIF_NEED_FPU_LOAD. Back to user space with DEFAULT_PKRU
loaded. FAIL!

XSTATE sucks.

Thanks,

        tglx