Messages in this thread | | | From | Aruna Ramakrishna <> | Subject | Re: [RFC PATCH v2 1/1] x86/pkeys: update PKRU to enable pkey 0 before XSAVE | Date | Fri, 22 Mar 2024 18:28:24 +0000 |
| |
> On Mar 22, 2024, at 8:40 AM, Dave Hansen <dave.hansen@intel.com> wrote: > > On 3/21/24 14:56, Aruna Ramakrishna wrote: >> +/* >> + * Ensure that the both the current stack and the alternate signal >> + * stack is writeable. The alternate stack must be accessible by the >> + * init PKRU value. >> + */ >> +static inline u32 sig_prepare_pkru(void) >> +{ >> + u32 current_pkru = read_pkru(); >> + u32 init_pkru_snapshot = pkru_get_init_value(); >> + >> + write_pkru(current_pkru & init_pkru_snapshot); >> + return current_pkru; >> +} > > That comment is quite misleading. This code has *ZERO* knowledge of the > permissions on either the current or alternate stack. It _assumes_ that > the current PKRU permissions allow writes to the current stack and > _assumes_ that the init PKRU value can write to the alternative stack. > > Those aren't bad assumptions, but they _are_ assumptions and need to be > clearly called out as such. > > The '&' operation looks rather random and needs an explanation. What is > that logically trying to do? It's trying to clear bits in the old > (pre-signal) PKRU value so that it gains write access to the alt stack. > Please say that. > > Which leads me to ask: Why bother with the '&'? It would be simpler to, > for instance, just wrpkru(0). What is being written to the old stack at > this point?
Right. This works only for the very specific use case where the alt stack is protected by init_pkru and the current execution stack is protected by the thread’s PKRU. If those assumptions do not hold for an application, then it would still run into the same issue.
I wasn’t sure if enabling all pkeys before XSAVE - i.e. wrpkru(0) - will be acceptable from a security standpoint. If it is, that seems like a more generic solution than what’s in this patch.
> > I also dislike something being called 'current_pkru' when it's clearly > the old value by the time it is returned. > >> +static inline void sig_restore_pkru(u32 pkru) >> +{ >> + write_pkru(pkru); >> +} > > This seems like unnecessary abstraction.
Yeah. Just trying to be consistent with the prep/restore…
I can remove this.
Thanks, Aruna
| |