Re: [PATCH v4 17/29] arm64: implement PKEYS support

[Joey Gouly <joey.gouly@xxxxxxx>]

On Mon, Jul 08, 2024 at 06:53:18PM +0100, Catalin Marinas wrote:
> Hi Szabolcs,
> 
> On Mon, Jun 17, 2024 at 03:51:35PM +0100, Szabolcs Nagy wrote:
> > The 06/17/2024 15:40, Florian Weimer wrote:
> > > >> A user can still set it by interacting with the register directly, but I guess
> > > >> we want something for the glibc interface..
> > > >> 
> > > >> Dave, any thoughts here?
> > > >
> > > > adding Florian too, since i found an old thread of his that tried
> > > > to add separate PKEY_DISABLE_READ and PKEY_DISABLE_EXECUTE, but
> > > > it did not seem to end up upstream. (this makes more sense to me
> > > > as libc api than the weird disable access semantics)
> > > 
> > > I still think it makes sense to have a full complenent of PKEY_* flags
> > > complementing the PROT_* flags, in a somewhat abstract fashion for
> > > pkey_alloc only.  The internal protection mask register encoding will
> > > differ from architecture to architecture, but the abstract glibc
> > > functions pkey_set and pkey_get could use them (if we are a bit
> > > careful).
> > 
> > to me it makes sense to have abstract
> > 
> > PKEY_DISABLE_READ
> > PKEY_DISABLE_WRITE
> > PKEY_DISABLE_EXECUTE
> > PKEY_DISABLE_ACCESS
> > 
> > where access is handled like
> > 
> > if (flags&PKEY_DISABLE_ACCESS)
> > 	flags |= PKEY_DISABLE_READ|PKEY_DISABLE_WRITE;
> > disable_read = flags&PKEY_DISABLE_READ;
> > disable_write = flags&PKEY_DISABLE_WRITE;
> > disable_exec = flags&PKEY_DISABLE_EXECUTE;
> > 
> > if there are unsupported combinations like
> > disable_read&&!disable_write then those are rejected
> > by pkey_alloc and pkey_set.
> > 
> > this allows portable use of pkey apis.
> > (the flags could be target specific, but don't have to be)
> 
> On powerpc, PKEY_DISABLE_ACCESS also disables execution. AFAICT, the
> kernel doesn't define a PKEY_DISABLE_READ, only PKEY_DISABLE_ACCESS so
> for powerpc there's no way to to set an execute-only permission via this
> interface. I wouldn't like to diverge from powerpc.

I think this is wrong, look at this code from powerpc:

arch/powerpc/mm/book3s64/pkeys.c: __arch_set_user_pkey_access

        if (init_val & PKEY_DISABLE_EXECUTE) {
                if (!pkey_execute_disable_supported)
                        return -EINVAL;
                new_iamr_bits |= IAMR_EX_BIT;
        }
        init_iamr(pkey, new_iamr_bits);

        /* Set the bits we need in AMR: */
        if (init_val & PKEY_DISABLE_ACCESS)
                new_amr_bits |= AMR_RD_BIT | AMR_WR_BIT;
        else if (init_val & PKEY_DISABLE_WRITE)
                new_amr_bits |= AMR_WR_BIT;

        init_amr(pkey, new_amr_bits);

Seems to me that PKEY_DISABLE_ACCESS leaves exec permissions as-is.

Here is the patch I am planning to include in the next version of the series.
This should support all PKEY_DISABLE_* combinations. Any comments? 

commit ba51371a544f6b0a4a0f03df62ad894d53f5039b
Author: Joey Gouly <joey.gouly@xxxxxxx>
Date:   Thu Jul 4 11:29:20 2024 +0100

    arm64: add PKEY_DISABLE_READ and PKEY_DISABLE_EXEC
    
    TODO
    
    Signed-off-by: Joey Gouly <joey.gouly@xxxxxxx>

diff --git arch/arm64/include/uapi/asm/mman.h arch/arm64/include/uapi/asm/mman.h
index 1e6482a838e1..e7e0c8216243 100644
--- arch/arm64/include/uapi/asm/mman.h
+++ arch/arm64/include/uapi/asm/mman.h
@@ -7,4 +7,13 @@
 #define PROT_BTI       0x10            /* BTI guarded page */
 #define PROT_MTE       0x20            /* Normal Tagged mapping */
 
+/* Override any generic PKEY permission defines */
+#define PKEY_DISABLE_EXECUTE   0x4
+#define PKEY_DISABLE_READ      0x8
+#undef PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+                               PKEY_DISABLE_WRITE  |\
+                               PKEY_DISABLE_READ   |\
+                               PKEY_DISABLE_EXECUTE)
+
 #endif /* ! _UAPI__ASM_MMAN_H */
diff --git arch/arm64/mm/mmu.c arch/arm64/mm/mmu.c
index 68afe5fc3071..ce4cc6bdee4e 100644
--- arch/arm64/mm/mmu.c
+++ arch/arm64/mm/mmu.c
@@ -1570,10 +1570,15 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey, unsigned long i
                return -EINVAL;
 
        /* Set the bits we need in POR:  */
+       new_por = POE_RXW;
+       if (init_val & PKEY_DISABLE_WRITE)
+               new_por &= ~POE_W;
        if (init_val & PKEY_DISABLE_ACCESS)
-               new_por = POE_X;
-       else if (init_val & PKEY_DISABLE_WRITE)
-               new_por = POE_RX;
+               new_por &= ~POE_RW;
+       if (init_val & PKEY_DISABLE_READ)
+               new_por &= ~POE_R;
+       if (init_val & PKEY_DISABLE_EXECUTE)
+               new_por &= ~POE_X;
 
        /* Shift the bits in to the correct place in POR for pkey: */
        pkey_shift = pkey * POR_BITS_PER_PKEY;



Thanks,
Joey