Re: [PATCH v4 18/29] arm64: add POE signal support

[Joey Gouly <joey.gouly@arm.com>]

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