Messages in this thread | | | Date | Fri, 22 Mar 2024 08:40:56 -0700 | Subject | Re: [RFC PATCH v2 1/1] x86/pkeys: update PKRU to enable pkey 0 before XSAVE | From | Dave Hansen <> |
| |
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?
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.
| |