Hi Catalin, On Wed, Aug 14, 2024 at 04:03:47PM +0100, Catalin Marinas wrote: > Hi Joey, > > On Tue, Aug 06, 2024 at 03:31:03PM +0100, Joey Gouly wrote: > > diff --git arch/arm64/kernel/signal.c arch/arm64/kernel/signal.c > > index 561986947530..ca7d4e0be275 100644 > > --- arch/arm64/kernel/signal.c > > +++ arch/arm64/kernel/signal.c > > @@ -1024,7 +1025,10 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user, > > return err; > > } > > > > - if (system_supports_poe()) { > > + if (system_supports_poe() && > > + (add_all || > > + mm_pkey_allocation_map(current->mm) != 0x1 || > > + read_sysreg_s(SYS_POR_EL0) != POR_EL0_INIT)) { > > err = sigframe_alloc(user, &user->poe_offset, > > sizeof(struct poe_context)); > > if (err) > > > > > > That is, we only save the POR_EL0 value if any pkeys have been allocated (other > > than pkey 0) *or* if POR_EL0 is a non-default value. > > I had a chat with Dave as well on this and, in principle, we don't want > to add stuff to the signal frame unnecessarily, especially for old > binaries that have no clue of pkeys. OTOH, it looks like too complicated > for just 16 bytes. Also POR_EL0 all RWX is a valid combination, I don't > think we should exclude it. > > If no pkey has been allocated, I guess we could skip this and it also > matches the x86 description of the PKRU being guaranteed to be preserved > only for the allocated keys. Do we reserve pkey 0 for arm64? I thought > that's only an x86 thing to emulate execute-only mappings. To make it less complicated, I could drop the POR_EL0 check and just do: - if (system_supports_poe()) { + if (system_supports_poe() && + (add_all || + mm_pkey_allocation_map(current->mm) != 0x1) { This wouldn't preserve the value of POR_EL0 if no pkeys had been allocated, but that is fine, as you said / the man pages say. We don't preserve pkey 0, but it is the default for mappings and defaults to RWX. So changing it probably will lead to unexpected things. > > Another corner case would be the signal handler doing a pkey_alloc() and > willing to populate POR_EL0 on sigreturn. It will have to find room in > the signal handler, though I don't think that's a problem. pkey_alloc() doesn't appear in the signal safety man page, but that might just be an omission due to permission keys being newer, than actually saying pkey_alloc() can't be used. If POR_EL0 isn't in the sig context, I think the signal handler could just write the POR_EL0 system register directly? The kernel wouldn't restore POR_EL0 in that case, so the value set in the signal handler would just be preserved. The reason that trying to preserve the value of POR_EL0 without any pkeys allocated (like in the patch in my previous e-mail had) doesn't really make sense, is that when you do pkey_alloc() you have to pass an initial value for the pkey, so that will overwite what you may have manually written into POR_EL0. Also you can't pass an unallocated pkey value to pkey_mprotect(). That's a lot of words to say, or ask, do you agree with the approach of only saving POR_EL0 in the signal frame if num_allocated_pkeys() > 1? Thanks, Joey