Re: [PATCH v3 3/4] x86/pkeys: Update PKRU to enable all pkeys before XSAVE

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, May 07 2024 at 17:34, Aruna Ramakrishna wrote:
>> On May 7, 2024, at 9:47 AM, Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> Also this lacks any justification why this enables all pkeys and how
>> that is the right thing to do instead of using init_pkru_value which
>> is what is set by fpu__clear_user_states() before going back to user
>> space. For signal handling this can be the only valid PKEY state unless
>> I'm missing something here.
>
> If the alt sig stack is protected by a different pkey (other than pkey 0), then
> this flow would need to enable that, along with the pkey for the thread’s 
> stack. Since the code has no way of knowing what pkey the altstack needs,
> it enables all for this brief window.

Again. The flow here is:

handle_signal()
  enable_access_to_altstack()

  ....

  fpu__clear_user_states()
    restore_fpregs_from_init_fpstate(XFEATURE_MASK_USER_RESTORE)
    os_xrstor(&init_fpstate, features_mask)
    pkru_write_default()
      write_pkru(init_pkru_value);  <- Loads the default PKRU value
           
return_to_user_space()

User space resumes with the default PKRU value and the first thing user
space does when entering the signal handler is to push stuff on the
signal stack.

If the signal stack is protected by a key which is not contained in
init_pkru_value then the application segfaults in a non recoverable way,
no?

So arguably it is sufficient to ensure that PKRU has the keys in
init_pkru_value enabled:

    sigpkru = read_pkru() & init_pkru_value;

If user space protects the task stack or the sigalt stack with a key
which is not in init_pkru_value then it does not matter at all whether
it dies in handle_signal() or later when returning to user space, no?

I'm not fundamentaly opposed to enable all keys, but I don't buy this
without a proper explanation why this has been chosen over enabling only
the absolute minimum access rights.

Thanks,

        tglx