pkeys: Reserve PKEY_DISABLE_READ

pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

Would it be possible to reserve a bit for PKEY_DISABLE_READ?

I think the POWER implementation can disable read access at the hardware
level, but not write access, and that cannot be expressed with the
current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.

My upcoming POWER implementation of pkey_set and pkey_get in glibc would
benefit from this.

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Dave Hansen]

On 11/8/18 4:05 AM, Florian Weimer wrote:
> Would it be possible to reserve a bit for PKEY_DISABLE_READ?
> 
> I think the POWER implementation can disable read access at the hardware
> level, but not write access, and that cannot be expressed with the
> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.

Do you just mean in the syscall interfaces?  What would we need to do on
x86 if we see the bit?  Would we just say it's invalid on x86, or would
we make sure that PKEY_DISABLE_ACCESS==PKEY_DISABLE_READ?

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Dave Hansen:

> On 11/8/18 4:05 AM, Florian Weimer wrote:
>> Would it be possible to reserve a bit for PKEY_DISABLE_READ?
>> 
>> I think the POWER implementation can disable read access at the hardware
>> level, but not write access, and that cannot be expressed with the
>> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.
>
> Do you just mean in the syscall interfaces?  What would we need to do on
> x86 if we see the bit?  Would we just say it's invalid on x86, or would
> we make sure that PKEY_DISABLE_ACCESS==PKEY_DISABLE_READ?

Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ |
PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a
line PKEY_DISABLE_READ would result in an EINVAL failure.

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Dave Hansen]

On 11/8/18 7:01 AM, Florian Weimer wrote:
> Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ |
> PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a
> line PKEY_DISABLE_READ would result in an EINVAL failure.

Sounds reasonable to me.

I don't see any urgency to do this right now.  It could easily go in
alongside the ppc patches when those get merged.  The only thing I'd
suggest is that we make it something slightly higher than 0x4.  It'll
make the code easier to deal with in the kernel if we have the ABI and
the hardware mirror each other, and if we pick 0x4 in the ABI for
PKEY_DISABLE_READ, it might get messy if the harware choose 0x4 for
PKEY_DISABLE_EXECUTE or something.

So, let's make it 0x80 or something on x86 at least.

Also, I'll be happy to review and ack the patch to do this, but I'd
expect the ppc guys (hi Ram!) to actually put it together.

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Dave Hansen:

> On 11/8/18 7:01 AM, Florian Weimer wrote:
>> Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ |
>> PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a
>> line PKEY_DISABLE_READ would result in an EINVAL failure.
>
> Sounds reasonable to me.
>
> I don't see any urgency to do this right now.  It could easily go in
> alongside the ppc patches when those get merged.

POWER support has already been merged, so we need to do something here
now, before I can complete the userspace side.

> The only thing I'd suggest is that we make it something slightly
> higher than 0x4.  It'll make the code easier to deal with in the
> kernel if we have the ABI and the hardware mirror each other, and if
> we pick 0x4 in the ABI for PKEY_DISABLE_READ, it might get messy if
> the harware choose 0x4 for PKEY_DISABLE_EXECUTE or something.
> 
> So, let's make it 0x80 or something on x86 at least.

I don't have a problem with that if that's what it takes.

> Also, I'll be happy to review and ack the patch to do this, but I'd
> expect the ppc guys (hi Ram!) to actually put it together.

Ram, do you want to write a patch?

I'll promise I finish the glibc support for this. 8-)

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Thu, Nov 08, 2018 at 06:37:41PM +0100, Florian Weimer wrote:
> * Dave Hansen:
> 
> > On 11/8/18 7:01 AM, Florian Weimer wrote:
> >> Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ |
> >> PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a
> >> line PKEY_DISABLE_READ would result in an EINVAL failure.
> >
> > Sounds reasonable to me.
> >
> > I don't see any urgency to do this right now.  It could easily go in
> > alongside the ppc patches when those get merged.
> 
> POWER support has already been merged, so we need to do something here
> now, before I can complete the userspace side.
> 
> > The only thing I'd suggest is that we make it something slightly
> > higher than 0x4.  It'll make the code easier to deal with in the
> > kernel if we have the ABI and the hardware mirror each other, and if
> > we pick 0x4 in the ABI for PKEY_DISABLE_READ, it might get messy if
> > the harware choose 0x4 for PKEY_DISABLE_EXECUTE or something.
> > 
> > So, let's make it 0x80 or something on x86 at least.
> 
> I don't have a problem with that if that's what it takes.
> 
> > Also, I'll be happy to review and ack the patch to do this, but I'd
> > expect the ppc guys (hi Ram!) to actually put it together.
> 
> Ram, do you want to write a patch?


Florian,

	I can. But I am struggling to understand the requirement. Why is
	this needed?  Are we proposing a enhancement to the sys_pkey_alloc(),
	to be able to allocate keys that are initialied to disable-read
	only?

RP

> 
> I'll promise I finish the glibc support for this. 8-)
> 
> Thanks,
> Florian

-- 
Ram Pai

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> Florian,
>
> 	I can. But I am struggling to understand the requirement. Why is
> 	this needed?  Are we proposing a enhancement to the sys_pkey_alloc(),
> 	to be able to allocate keys that are initialied to disable-read
> 	only?

Yes, I think that would be a natural consequence.

However, my immediate need comes from the fact that the AMR register can
contain a flag combination that is not possible to represent with the
existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags.  User code
could write to AMR directly, so I cannot rule out that certain flag
combinations exist there.

So I came up with this:

int
pkey_get (int key)
{
  if (key < 0 || key > PKEY_MAX)
    {
      __set_errno (EINVAL);
      return -1;
    }
  unsigned int index = pkey_index (key);
  unsigned long int amr = pkey_read ();
  unsigned int bits = (amr >> index) & 3;

  /* Translate from AMR values.  PKEY_AMR_READ standing alone is not
     currently representable.  */
  if (bits & PKEY_AMR_READ)
    return PKEY_DISABLE_ACCESS;
  else if (bits == PKEY_AMR_WRITE)
    return PKEY_DISABLE_WRITE;
  return 0;
}

And this is not ideal.  I would prefer something like this instead:

  switch (bits)
    {
      case PKEY_AMR_READ | PKEY_AMR_WRITE:
        return PKEY_DISABLE_ACCESS;
      case PKEY_AMR_READ:
        return PKEY_DISABLE_READ;
      case PKEY_AMR_WRITE:
        return PKEY_DISABLE_WRITE;
      case 0:
        return 0;
    }

By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> * Ram Pai:
> 
> > Florian,
> >
> > 	I can. But I am struggling to understand the requirement. Why is
> > 	this needed?  Are we proposing a enhancement to the sys_pkey_alloc(),
> > 	to be able to allocate keys that are initialied to disable-read
> > 	only?
> 
> Yes, I think that would be a natural consequence.
> 
> However, my immediate need comes from the fact that the AMR register can
> contain a flag combination that is not possible to represent with the
> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags.  User code
> could write to AMR directly, so I cannot rule out that certain flag
> combinations exist there.
> 
> So I came up with this:
> 
> int
> pkey_get (int key)
> {
>   if (key < 0 || key > PKEY_MAX)
>     {
>       __set_errno (EINVAL);
>       return -1;
>     }
>   unsigned int index = pkey_index (key);
>   unsigned long int amr = pkey_read ();
>   unsigned int bits = (amr >> index) & 3;
> 
>   /* Translate from AMR values.  PKEY_AMR_READ standing alone is not
>      currently representable.  */
>   if (bits & PKEY_AMR_READ)

this should be
   if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))


>     return PKEY_DISABLE_ACCESS;


>   else if (bits == PKEY_AMR_WRITE)
>     return PKEY_DISABLE_WRITE;
>   return 0;
> }
> 
> And this is not ideal.  I would prefer something like this instead:
> 
>   switch (bits)
>     {
>       case PKEY_AMR_READ | PKEY_AMR_WRITE:
>         return PKEY_DISABLE_ACCESS;
>       case PKEY_AMR_READ:
>         return PKEY_DISABLE_READ;
>       case PKEY_AMR_WRITE:
>         return PKEY_DISABLE_WRITE;
>       case 0:
>         return 0;
>     }

yes.
 and on x86 it will be something like:
   switch (bits)
     {
       case PKEY_PKRU_ACCESS :
         return PKEY_DISABLE_ACCESS;
       case PKEY_AMR_WRITE:
         return PKEY_DISABLE_WRITE;
       case 0:
         return 0;
     }

But for this to work, why do you need to enhance the sys_pkey_alloc()
interface?  Not that I am against it. Trying to understand if the
enhancement is really needed.

> 
> By the way, is the AMR register 64-bit or 32-bit on 32-bit POWER?

It is 64-bit.

RP

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
>> * Ram Pai:
>> 
>> > Florian,
>> >
>> > 	I can. But I am struggling to understand the requirement. Why is
>> > 	this needed?  Are we proposing a enhancement to the sys_pkey_alloc(),
>> > 	to be able to allocate keys that are initialied to disable-read
>> > 	only?
>> 
>> Yes, I think that would be a natural consequence.
>> 
>> However, my immediate need comes from the fact that the AMR register can
>> contain a flag combination that is not possible to represent with the
>> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags.  User code
>> could write to AMR directly, so I cannot rule out that certain flag
>> combinations exist there.
>> 
>> So I came up with this:
>> 
>> int
>> pkey_get (int key)
>> {
>>   if (key < 0 || key > PKEY_MAX)
>>     {
>>       __set_errno (EINVAL);
>>       return -1;
>>     }
>>   unsigned int index = pkey_index (key);
>>   unsigned long int amr = pkey_read ();
>>   unsigned int bits = (amr >> index) & 3;
>> 
>>   /* Translate from AMR values.  PKEY_AMR_READ standing alone is not
>>      currently representable.  */
>>   if (bits & PKEY_AMR_READ)
>
> this should be
>    if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))

This would return zero for PKEY_AMR_READ alone.

>>     return PKEY_DISABLE_ACCESS;
>
>
>>   else if (bits == PKEY_AMR_WRITE)
>>     return PKEY_DISABLE_WRITE;
>>   return 0;
>> }

It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
Which is why I want PKEY_DISABLE_READ.

>> And this is not ideal.  I would prefer something like this instead:
>> 
>>   switch (bits)
>>     {
>>       case PKEY_AMR_READ | PKEY_AMR_WRITE:
>>         return PKEY_DISABLE_ACCESS;
>>       case PKEY_AMR_READ:
>>         return PKEY_DISABLE_READ;
>>       case PKEY_AMR_WRITE:
>>         return PKEY_DISABLE_WRITE;
>>       case 0:
>>         return 0;
>>     }
>
> yes.
>  and on x86 it will be something like:
>    switch (bits)
>      {
>        case PKEY_PKRU_ACCESS :
>          return PKEY_DISABLE_ACCESS;
>        case PKEY_AMR_WRITE:
>          return PKEY_DISABLE_WRITE;
>        case 0:
>          return 0;
>      }

x86 returns the PKRU bits directly, including the nonsensical case
(PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).

> But for this to work, why do you need to enhance the sys_pkey_alloc()
> interface?  Not that I am against it. Trying to understand if the
> enhancement is really needed.

sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
(that is, it updates the PKRU/AMR register).  It makes sense to match
the behavior of the userspace implementation.

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Mon, Nov 12, 2018 at 01:00:19PM +0100, Florian Weimer wrote:
> * Ram Pai:
> 
> > On Thu, Nov 08, 2018 at 09:23:35PM +0100, Florian Weimer wrote:
> >> * Ram Pai:
> >> 
> >> > Florian,
> >> >
> >> > 	I can. But I am struggling to understand the requirement. Why is
> >> > 	this needed?  Are we proposing a enhancement to the sys_pkey_alloc(),
> >> > 	to be able to allocate keys that are initialied to disable-read
> >> > 	only?
> >> 
> >> Yes, I think that would be a natural consequence.
> >> 
> >> However, my immediate need comes from the fact that the AMR register can
> >> contain a flag combination that is not possible to represent with the
> >> existing PKEY_DISABLE_WRITE and PKEY_DISABLE_ACCESS flags.  User code
> >> could write to AMR directly, so I cannot rule out that certain flag
> >> combinations exist there.
> >> 
> >> So I came up with this:
> >> 
> >> int
> >> pkey_get (int key)
> >> {
> >>   if (key < 0 || key > PKEY_MAX)
> >>     {
> >>       __set_errno (EINVAL);
> >>       return -1;
> >>     }
> >>   unsigned int index = pkey_index (key);
> >>   unsigned long int amr = pkey_read ();
> >>   unsigned int bits = (amr >> index) & 3;
> >> 
> >>   /* Translate from AMR values.  PKEY_AMR_READ standing alone is not
> >>      currently representable.  */
> >>   if (bits & PKEY_AMR_READ)
> >
> > this should be
> >    if (bits & (PKEY_AMR_READ|PKEY_AMR_WRITE))
> 
> This would return zero for PKEY_AMR_READ alone.
> 
> >>     return PKEY_DISABLE_ACCESS;
> >
> >
> >>   else if (bits == PKEY_AMR_WRITE)
> >>     return PKEY_DISABLE_WRITE;
> >>   return 0;
> >> }
> 
> It's hard to tell whether PKEY_DISABLE_ACCESS is better in this case.
> Which is why I want PKEY_DISABLE_READ.
> 
> >> And this is not ideal.  I would prefer something like this instead:
> >> 
> >>   switch (bits)
> >>     {
> >>       case PKEY_AMR_READ | PKEY_AMR_WRITE:
> >>         return PKEY_DISABLE_ACCESS;
> >>       case PKEY_AMR_READ:
> >>         return PKEY_DISABLE_READ;
> >>       case PKEY_AMR_WRITE:
> >>         return PKEY_DISABLE_WRITE;
> >>       case 0:
> >>         return 0;
> >>     }
> >
> > yes.
> >  and on x86 it will be something like:
> >    switch (bits)
> >      {
> >        case PKEY_PKRU_ACCESS :
> >          return PKEY_DISABLE_ACCESS;
> >        case PKEY_AMR_WRITE:
> >          return PKEY_DISABLE_WRITE;
> >        case 0:
> >          return 0;
> >      }
> 
> x86 returns the PKRU bits directly, including the nonsensical case
> (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE).
> 
> > But for this to work, why do you need to enhance the sys_pkey_alloc()
> > interface?  Not that I am against it. Trying to understand if the
> > enhancement is really needed.
> 
> sys_pkey_alloc performs an implicit pkey_set for the newly allocated key
> (that is, it updates the PKRU/AMR register).  It makes sense to match
> the behavior of the userspace implementation.

Here is a untested patch. Does this meet your needs?
It defines the new flags. Each architecture will than define the set of flags
it supports through PKEY_ACCESS_MASK.


Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 92a9962..724ef43 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -21,11 +21,6 @@
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 			    VM_PKEY_BIT3 | VM_PKEY_BIT4)
 
-/* Override any generic PKEY permission defines */
-#define PKEY_DISABLE_EXECUTE   0x4
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS | \
-				PKEY_DISABLE_WRITE  | \
-				PKEY_DISABLE_EXECUTE)
 
 static inline u64 pkey_to_vmflag_bits(u16 pkey)
 {
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..76237b3 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -31,9 +31,9 @@
 #define MAP_HUGETLB	0x40000		/* create a huge page mapping */
 
 /* Override any generic PKEY permission defines */
-#define PKEY_DISABLE_EXECUTE   0x4
 #undef PKEY_ACCESS_MASK
 #define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
 				PKEY_DISABLE_WRITE  |\
+				PKEY_DISABLE_READ  |\
 				PKEY_DISABLE_EXECUTE)
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 4860acd..c8b2540 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -62,14 +62,6 @@ int pkey_initialize(void)
 	int os_reserved, i;
 
 	/*
-	 * We define PKEY_DISABLE_EXECUTE in addition to the arch-neutral
-	 * generic defines for PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE.
-	 * Ensure that the bits a distinct.
-	 */
-	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
-		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
-
-	/*
 	 * pkey_to_vmflag_bits() assumes that the pkey bits are contiguous
 	 * in the vmaflag. Make sure that is really the case.
 	 */
@@ -259,6 +251,8 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		new_amr_bits |= AMR_RD_BIT | AMR_WR_BIT;
 	else if (init_val & PKEY_DISABLE_WRITE)
 		new_amr_bits |= AMR_WR_BIT;
+	else if (init_val & PKEY_DISABLE_READ)
+		new_amr_bits |= AMR_RD_BIT;
 
 	init_amr(pkey, new_amr_bits);
 	return 0;
diff --git a/arch/x86/include/uapi/asm/mman.h b/arch/x86/include/uapi/asm/mman.h
index d4a8d04..e9b121b 100644
--- a/arch/x86/include/uapi/asm/mman.h
+++ b/arch/x86/include/uapi/asm/mman.h
@@ -24,6 +24,11 @@
 		((key) & 0x2 ? VM_PKEY_BIT1 : 0) |      \
 		((key) & 0x4 ? VM_PKEY_BIT2 : 0) |      \
 		((key) & 0x8 ? VM_PKEY_BIT3 : 0))
+
+/* Override any generic PKEY permission defines */
+#undef PKEY_ACCESS_MASK
+#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
+				PKEY_DISABLE_WRITE)
 #endif
 
 #include <asm-generic/mman.h>
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index e7ee328..61168e4 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -71,7 +71,8 @@
 
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
-#define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
-				 PKEY_DISABLE_WRITE)
-
+#define PKEY_DISABLE_EXECUTE	0x4
+#define PKEY_DISABLE_READ	0x8
+#define PKEY_ACCESS_MASK	0x0	/* arch can override and define its own
+					   mask bits */
 #endif /* __ASM_GENERIC_MMAN_COMMON_H */

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> diff --git a/arch/x86/include/uapi/asm/mman.h b/arch/x86/include/uapi/asm/mman.h
> index d4a8d04..e9b121b 100644
> --- a/arch/x86/include/uapi/asm/mman.h
> +++ b/arch/x86/include/uapi/asm/mman.h
> @@ -24,6 +24,11 @@
>  		((key) & 0x2 ? VM_PKEY_BIT1 : 0) |      \
>  		((key) & 0x4 ? VM_PKEY_BIT2 : 0) |      \
>  		((key) & 0x8 ? VM_PKEY_BIT3 : 0))
> +
> +/* Override any generic PKEY permission defines */
> +#undef PKEY_ACCESS_MASK
> +#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
> +				PKEY_DISABLE_WRITE)
>  #endif

I would have expected something that translates PKEY_DISABLE_WRITE |
PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.

(My understanding is that PKEY_DISABLE_ACCESS does not disable all
access, but produces execute-only memory.)

> diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
> index e7ee328..61168e4 100644
> --- a/include/uapi/asm-generic/mman-common.h
> +++ b/include/uapi/asm-generic/mman-common.h
> @@ -71,7 +71,8 @@
>  
>  #define PKEY_DISABLE_ACCESS	0x1
>  #define PKEY_DISABLE_WRITE	0x2
> -#define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
> -				 PKEY_DISABLE_WRITE)
> -
> +#define PKEY_DISABLE_EXECUTE	0x4
> +#define PKEY_DISABLE_READ	0x8
> +#define PKEY_ACCESS_MASK	0x0	/* arch can override and define its own
> +					   mask bits */
>  #endif /* __ASM_GENERIC_MMAN_COMMON_H */

I think Dave requested a value for PKEY_DISABLE_READ which is further
away from the existing bits.

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Dave Hansen]

On 11/27/18 3:57 AM, Florian Weimer wrote:
> I would have expected something that translates PKEY_DISABLE_WRITE |
> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
> 
> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
> access, but produces execute-only memory.)

Correct, it disables all data access, but not execution.

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Dave Hansen:

> On 11/27/18 3:57 AM, Florian Weimer wrote:
>> I would have expected something that translates PKEY_DISABLE_WRITE |
>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
>> 
>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
>> access, but produces execute-only memory.)
>
> Correct, it disables all data access, but not execution.

So I would expect something like this (completely untested, I did not
even compile this):

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf153c871..bed23f9e8336 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -199,6 +199,11 @@ static inline bool arch_pkeys_enabled(void)
 	return !static_branch_likely(&pkey_disabled);
 }
 
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	return (rights & ~(unsigned long)PKEY_ACCESS_MASK) == 0;
+}
+
 extern void pkey_mm_init(struct mm_struct *mm);
 extern bool arch_supports_pkeys(int cap);
 extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f1b3be..e3e1d5a316e8 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -14,6 +14,17 @@ static inline bool arch_pkeys_enabled(void)
 	return boot_cpu_has(X86_FEATURE_OSPKE);
 }
 
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	if (rights & ~(unsigned long)PKEY_ACCESS_MASK)
+		return false;
+	if (rights & PKEY_DISABLE_READ) {
+		/* x86 can only disable read access along with write access. */
+		return rights & (PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS);
+	}
+	return true;
+}
+
 /*
  * Try to dedicate one of the protection keys to be used as an
  * execute-only protection key.
diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
index 87a57b7642d3..b9b78145017f 100644
--- a/arch/x86/kernel/fpu/xstate.c
+++ b/arch/x86/kernel/fpu/xstate.c
@@ -928,7 +928,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		return -EINVAL;
 
 	/* Set the bits we need in PKRU:  */
-	if (init_val & PKEY_DISABLE_ACCESS)
+	if (init_val & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ))
+		/*
+		 * arch_pkey_access_rights_valid checked that
+		 * PKEY_DISABLE_READ is actually representable on x86
+		 * (that is, it comes with PKEY_DISABLE_ACCESS or
+		 * PKEY_DISABLE_WRITE).
+		 */
 		new_pkru_bits |= PKRU_AD_BIT;
 	if (init_val & PKEY_DISABLE_WRITE)
 		new_pkru_bits |= PKRU_WD_BIT;
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 2955ba976048..2c330fabbe55 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
 {
 }
 
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	return false;
+}
+
 #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
 #endif /* _LINUX_PKEYS_H */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6d331620b9e5..f4cefc3540df 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -597,7 +597,7 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
 	if (flags)
 		return -EINVAL;
 	/* check for unsupported init values */
-	if (init_val & ~PKEY_ACCESS_MASK)
+	if (!arch_pkey_access_rights_valid(init_val))
 		return -EINVAL;
 
 	down_write(&current->mm->mmap_sem);

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
> * Dave Hansen:
> 
> > On 11/27/18 3:57 AM, Florian Weimer wrote:
> >> I would have expected something that translates PKEY_DISABLE_WRITE |
> >> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
> >> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
> >> 
> >> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
> >> access, but produces execute-only memory.)
> >
> > Correct, it disables all data access, but not execution.
> 
> So I would expect something like this (completely untested, I did not
> even compile this):


Ok. I re-read through the entire email thread to understand the problem and
the proposed solution. Let me summarize it below. Lets see if we are on the same
plate.

So the problem is as follows:

Currently the kernel supports  'disable-write'  and 'disable-access'.

On x86, cpu supports 'disable-write' and 'disable-access'. This
matches with what the kernel supports. All good.

However on power, cpu supports 'disable-read' too. Since userspace can
program the cpu directly, userspace has the ability to set
'disable-read' too.  This can lead to inconsistency between the kernel
and the userspace.

We want the kernel to match userspace on all architectures.

Proposed Solution:

Enhance the kernel to understand 'disable-read', and facilitate architectures
that understand 'disable-read' to allow it.

Also explicitly define the semantics of disable-access  as 
'disable-read and disable-write'

Did I get this right?  Assuming I did, the implementation has to do
the following --
  
	On power, sys_pkey_alloc() should succeed if the init_val
	is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
	or any combination of the three.

	On x86, sys_pkey_alloc() should succeed if the init_val is
	PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
	or any combination of the three, except  PKEY_DISABLE_READ
      	specified all by itself.

	On all other arches, none of the flags are supported.


Are we on the same plate?
RP


> 
> diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
> index 20ebf153c871..bed23f9e8336 100644
> --- a/arch/powerpc/include/asm/pkeys.h
> +++ b/arch/powerpc/include/asm/pkeys.h
> @@ -199,6 +199,11 @@ static inline bool arch_pkeys_enabled(void)
>  	return !static_branch_likely(&pkey_disabled);
>  }
> 
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> +	return (rights & ~(unsigned long)PKEY_ACCESS_MASK) == 0;
> +}
> +
>  extern void pkey_mm_init(struct mm_struct *mm);
>  extern bool arch_supports_pkeys(int cap);
>  extern unsigned int arch_usable_pkeys(void);
> diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
> index 19b137f1b3be..e3e1d5a316e8 100644
> --- a/arch/x86/include/asm/pkeys.h
> +++ b/arch/x86/include/asm/pkeys.h
> @@ -14,6 +14,17 @@ static inline bool arch_pkeys_enabled(void)
>  	return boot_cpu_has(X86_FEATURE_OSPKE);
>  }
> 
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> +	if (rights & ~(unsigned long)PKEY_ACCESS_MASK)
> +		return false;
> +	if (rights & PKEY_DISABLE_READ) {
> +		/* x86 can only disable read access along with write access. */
> +		return rights & (PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS);
> +	}
> +	return true;
> +}
> +
>  /*
>   * Try to dedicate one of the protection keys to be used as an
>   * execute-only protection key.
> diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> index 87a57b7642d3..b9b78145017f 100644
> --- a/arch/x86/kernel/fpu/xstate.c
> +++ b/arch/x86/kernel/fpu/xstate.c
> @@ -928,7 +928,13 @@ int arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
>  		return -EINVAL;
> 
>  	/* Set the bits we need in PKRU:  */
> -	if (init_val & PKEY_DISABLE_ACCESS)
> +	if (init_val & (PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ))
> +		/*
> +		 * arch_pkey_access_rights_valid checked that
> +		 * PKEY_DISABLE_READ is actually representable on x86
> +		 * (that is, it comes with PKEY_DISABLE_ACCESS or
> +		 * PKEY_DISABLE_WRITE).
> +		 */
>  		new_pkru_bits |= PKRU_AD_BIT;
>  	if (init_val & PKEY_DISABLE_WRITE)
>  		new_pkru_bits |= PKRU_WD_BIT;
> diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
> index 2955ba976048..2c330fabbe55 100644
> --- a/include/linux/pkeys.h
> +++ b/include/linux/pkeys.h
> @@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
>  {
>  }
> 
> +static inline bool arch_pkey_access_rights_valid(unsigned long rights)
> +{
> +	return false;
> +}
> +
>  #endif /* ! CONFIG_ARCH_HAS_PKEYS */
> 
>  #endif /* _LINUX_PKEYS_H */
> diff --git a/mm/mprotect.c b/mm/mprotect.c
> index 6d331620b9e5..f4cefc3540df 100644
> --- a/mm/mprotect.c
> +++ b/mm/mprotect.c
> @@ -597,7 +597,7 @@ SYSCALL_DEFINE2(pkey_alloc, unsigned long, flags, unsigned long, init_val)
>  	if (flags)
>  		return -EINVAL;
>  	/* check for unsupported init values */
> -	if (init_val & ~PKEY_ACCESS_MASK)
> +	if (!arch_pkey_access_rights_valid(init_val))
>  		return -EINVAL;
> 
>  	down_write(&current->mm->mmap_sem);
> 
> Thanks,
> Florian

-- 
Ram Pai

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> So the problem is as follows:
>
> Currently the kernel supports  'disable-write'  and 'disable-access'.
>
> On x86, cpu supports 'disable-write' and 'disable-access'. This
> matches with what the kernel supports. All good.
>
> However on power, cpu supports 'disable-read' too. Since userspace can
> program the cpu directly, userspace has the ability to set
> 'disable-read' too.  This can lead to inconsistency between the kernel
> and the userspace.
>
> We want the kernel to match userspace on all architectures.

Correct.

> Proposed Solution:
>
> Enhance the kernel to understand 'disable-read', and facilitate architectures
> that understand 'disable-read' to allow it.
>
> Also explicitly define the semantics of disable-access  as 
> 'disable-read and disable-write'
>
> Did I get this right?  Assuming I did, the implementation has to do
> the following --
>   
> 	On power, sys_pkey_alloc() should succeed if the init_val
> 	is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> 	or any combination of the three.

Agreed.

> 	On x86, sys_pkey_alloc() should succeed if the init_val is
> 	PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> 	or any combination of the three, except  PKEY_DISABLE_READ
>       	specified all by itself.

Again agreed.  That's a clever way of phrasing it actually.

> 	On all other arches, none of the flags are supported.
>
>
> Are we on the same plate?

I think so, thanks.

Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Mon, Dec 03, 2018 at 04:52:02PM +0100, Florian Weimer wrote:
> * Ram Pai:
> 
> > So the problem is as follows:
> >
> > Currently the kernel supports  'disable-write'  and 'disable-access'.
> >
> > On x86, cpu supports 'disable-write' and 'disable-access'. This
> > matches with what the kernel supports. All good.
> >
> > However on power, cpu supports 'disable-read' too. Since userspace can
> > program the cpu directly, userspace has the ability to set
> > 'disable-read' too.  This can lead to inconsistency between the kernel
> > and the userspace.
> >
> > We want the kernel to match userspace on all architectures.
> 
> Correct.
> 
> > Proposed Solution:
> >
> > Enhance the kernel to understand 'disable-read', and facilitate architectures
> > that understand 'disable-read' to allow it.
> >
> > Also explicitly define the semantics of disable-access  as 
> > 'disable-read and disable-write'
> >
> > Did I get this right?  Assuming I did, the implementation has to do
> > the following --
> >   
> > 	On power, sys_pkey_alloc() should succeed if the init_val
> > 	is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> > 	or any combination of the three.
> 
> Agreed.
> 
> > 	On x86, sys_pkey_alloc() should succeed if the init_val is
> > 	PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> > 	or any combination of the three, except  PKEY_DISABLE_READ
> >       	specified all by itself.
> 
> Again agreed.  That's a clever way of phrasing it actually.
> 
> > 	On all other arches, none of the flags are supported.
> >
> >
> > Are we on the same plate?
> 
> I think so, thanks.
> 
> Florian

Ok. here is a patch, compiled but not tested. See if this meets the
specifications.

-----------------------------------------------------------------------------------

commit 3dc06e73f3795921265d5d1d935e428deab01616
Author: Ram Pai <linuxram@us.ibm.com>
Date:   Tue Dec 4 00:04:11 2018 -0500

    pkeys: add support of PKEY_DISABLE_READ
    
    Kernel supports  'disable-write'  and 'disable-access'.
    
    x86 cpu supports 'disable-write' and 'disable-access'. This
    matches with the kernel support.
    
    However POWER cpu supports 'disable-read' too. Since userspace can
    program the cpu directly, userspace has the ability to set
    'disable-read' too.  This can lead to inconsistency between the kernel
    and the userspace.
    
    Make kernel match userspace on all architectures.
    
    Enhance the kernel to understand 'disable-read', and facilitate architectures
    that understand 'disable-read' to allow it.
    
    Define the semantics of disable-access  as 'disable-read and disable-write'
    
    Signed-off-by: Ram Pai <linuxram@us.ibm.com>

diff --git a/arch/powerpc/include/asm/pkeys.h b/arch/powerpc/include/asm/pkeys.h
index 20ebf15..4bd09d0 100644
--- a/arch/powerpc/include/asm/pkeys.h
+++ b/arch/powerpc/include/asm/pkeys.h
@@ -19,11 +19,7 @@
 #define ARCH_VM_PKEY_FLAGS (VM_PKEY_BIT0 | VM_PKEY_BIT1 | VM_PKEY_BIT2 | \
 			    VM_PKEY_BIT3 | VM_PKEY_BIT4)
 
-/* Override any generic PKEY permission defines */
 #define PKEY_DISABLE_EXECUTE   0x4
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS | \
-				PKEY_DISABLE_WRITE  | \
-				PKEY_DISABLE_EXECUTE)
 
 static inline u64 pkey_to_vmflag_bits(u16 pkey)
 {
@@ -199,6 +195,16 @@ static inline bool arch_pkeys_enabled(void)
 	return !static_branch_likely(&pkey_disabled);
 }
 
+extern bool __arch_pkey_access_rights_valid(unsigned long rights);
+
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	if (static_branch_likely(&pkey_disabled))
+		return false;
+
+	return __arch_pkey_access_rights_valid(rights);
+}
+
 extern void pkey_mm_init(struct mm_struct *mm);
 extern bool arch_supports_pkeys(int cap);
 extern unsigned int arch_usable_pkeys(void);
diff --git a/arch/powerpc/include/uapi/asm/mman.h b/arch/powerpc/include/uapi/asm/mman.h
index 65065ce..e63bc37 100644
--- a/arch/powerpc/include/uapi/asm/mman.h
+++ b/arch/powerpc/include/uapi/asm/mman.h
@@ -30,10 +30,4 @@
 #define MAP_STACK	0x20000		/* give out an address that is best suited for process/thread stacks */
 #define MAP_HUGETLB	0x40000		/* create a huge page mapping */
 
-/* Override any generic PKEY permission defines */
-#define PKEY_DISABLE_EXECUTE   0x4
-#undef PKEY_ACCESS_MASK
-#define PKEY_ACCESS_MASK       (PKEY_DISABLE_ACCESS |\
-				PKEY_DISABLE_WRITE  |\
-				PKEY_DISABLE_EXECUTE)
 #endif /* _UAPI_ASM_POWERPC_MMAN_H */
diff --git a/arch/powerpc/mm/pkeys.c b/arch/powerpc/mm/pkeys.c
index 5d65c47..a4f288b 100644
--- a/arch/powerpc/mm/pkeys.c
+++ b/arch/powerpc/mm/pkeys.c
@@ -67,7 +67,7 @@ int pkey_initialize(void)
 	 * Ensure that the bits a distinct.
 	 */
 	BUILD_BUG_ON(PKEY_DISABLE_EXECUTE &
-		     (PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE));
+		(PKEY_DISABLE_READ | PKEY_DISABLE_WRITE | PKEY_DISABLE_ACCESS));
 
 	/*
 	 * pkey_to_vmflag_bits() assumes that the pkey bits are contiguous
@@ -259,11 +259,20 @@ int __arch_set_user_pkey_access(struct task_struct *tsk, int pkey,
 		new_amr_bits |= AMR_RD_BIT | AMR_WR_BIT;
 	else if (init_val & PKEY_DISABLE_WRITE)
 		new_amr_bits |= AMR_WR_BIT;
+	else if (init_val & PKEY_DISABLE_READ)
+		new_amr_bits |= AMR_RD_BIT;
 
 	init_amr(pkey, new_amr_bits);
 	return 0;
 }
 
+bool __arch_pkey_access_rights_valid(unsigned long rights)
+{
+	unsigned long mask = PKEY_DISABLE_READ | PKEY_DISABLE_WRITE |\
+			     PKEY_DISABLE_ACCESS;
+	return (rights & mask);
+}
+
 void thread_pkey_regs_save(struct thread_struct *thread)
 {
 	if (static_branch_likely(&pkey_disabled))
diff --git a/arch/x86/include/asm/pkeys.h b/arch/x86/include/asm/pkeys.h
index 19b137f..4f36a7e 100644
--- a/arch/x86/include/asm/pkeys.h
+++ b/arch/x86/include/asm/pkeys.h
@@ -14,6 +14,15 @@ static inline bool arch_pkeys_enabled(void)
 	return boot_cpu_has(X86_FEATURE_OSPKE);
 }
 
+extern bool __arch_pkey_access_rights_valid(unsigned long rights);
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	if (!boot_cpu_has(X86_FEATURE_OSPKE))
+		return false;
+
+	return __arch_pkey_access_rights_valid(rights);
+}
+
 /*
  * Try to dedicate one of the protection keys to be used as an
  * execute-only protection key.
diff --git a/arch/x86/mm/pkeys.c b/arch/x86/mm/pkeys.c
index 6e98e0a..fcfe1b2 100644
--- a/arch/x86/mm/pkeys.c
+++ b/arch/x86/mm/pkeys.c
@@ -72,6 +72,17 @@ int __execute_only_pkey(struct mm_struct *mm)
 	return execute_only_pkey;
 }
 
+bool __arch_pkey_access_rights_valid(unsigned long rights)
+{
+	unsigned long mask = PKEY_DISABLE_READ | PKEY_DISABLE_WRITE |\
+				PKEY_DISABLE_ACCESS;
+	if (!(rights & mask))
+		return false;
+
+	/* return failure if only PKEY_DISABLE_READ is specified */
+	return ((rights & mask) != PKEY_DISABLE_READ);
+}
+
 static inline bool vma_is_pkey_exec_only(struct vm_area_struct *vma)
 {
 	/* Do this check first since the vm_flags should be hot */
diff --git a/include/linux/pkeys.h b/include/linux/pkeys.h
index 2955ba97..2c330fa 100644
--- a/include/linux/pkeys.h
+++ b/include/linux/pkeys.h
@@ -48,6 +48,11 @@ static inline void copy_init_pkru_to_fpregs(void)
 {
 }
 
+static inline bool arch_pkey_access_rights_valid(unsigned long rights)
+{
+	return false;
+}
+
 #endif /* ! CONFIG_ARCH_HAS_PKEYS */
 
 #endif /* _LINUX_PKEYS_H */
diff --git a/include/uapi/asm-generic/mman-common.h b/include/uapi/asm-generic/mman-common.h
index e7ee328..d2e1a5e 100644
--- a/include/uapi/asm-generic/mman-common.h
+++ b/include/uapi/asm-generic/mman-common.h
@@ -71,7 +71,5 @@
 
 #define PKEY_DISABLE_ACCESS	0x1
 #define PKEY_DISABLE_WRITE	0x2
-#define PKEY_ACCESS_MASK	(PKEY_DISABLE_ACCESS |\
-				 PKEY_DISABLE_WRITE)
-
+#define PKEY_DISABLE_READ	0x10
 #endif /* __ASM_GENERIC_MMAN_COMMON_H */
diff --git a/mm/mprotect.c b/mm/mprotect.c
index 6d33162..f4cefc3 100644
--- a/mm/mprotect.c
+++ b/mm/mprotect.c
@@ -597,7 +597,7 @@ static int do_mprotect_pkey(unsigned long start, size_t len,
 	if (flags)
 		return -EINVAL;
 	/* check for unsupported init values */
-	if (init_val & ~PKEY_ACCESS_MASK)
+	if (!arch_pkey_access_rights_valid(init_val))
 		return -EINVAL;
 
 	down_write(&current->mm->mmap_sem);

-----------------------------------------------------------------------------

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> Ok. here is a patch, compiled but not tested. See if this meets the
> specifications.
>
> -----------------------------------------------------------------------------------
>
> commit 3dc06e73f3795921265d5d1d935e428deab01616
> Author: Ram Pai <linuxram@us.ibm.com>
> Date:   Tue Dec 4 00:04:11 2018 -0500
>
>     pkeys: add support of PKEY_DISABLE_READ

Thanks.  In the x86 code, the translation of PKEY_DISABLE_READ |
PKEY_DISABLE_WRITE to PKEY_DISABLE_ACCESS appears to be missing.  I
believe the existing code produces PKEY_DISABLE_WRITE, which is wrong.

Rest looks okay to me (again not tested).

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Wed, Dec 05, 2018 at 02:00:59PM +0100, Florian Weimer wrote:
> * Ram Pai:
> 
> > Ok. here is a patch, compiled but not tested. See if this meets the
> > specifications.
> >
> > -----------------------------------------------------------------------------------
> >
> > commit 3dc06e73f3795921265d5d1d935e428deab01616
> > Author: Ram Pai <linuxram@us.ibm.com>
> > Date:   Tue Dec 4 00:04:11 2018 -0500
> >
> >     pkeys: add support of PKEY_DISABLE_READ
> 
> Thanks.  In the x86 code, the translation of PKEY_DISABLE_READ |
> PKEY_DISABLE_WRITE to PKEY_DISABLE_ACCESS appears to be missing.  I
> believe the existing code produces PKEY_DISABLE_WRITE, which is wrong.

ah. yes. good point.

> 
> Rest looks okay to me (again not tested).

thanks,
RP

Re: pkeys: Reserve PKEY_DISABLE_READ

[Andy Lutomirski]

> On Dec 2, 2018, at 8:02 PM, Ram Pai <linuxram@us.ibm.com> wrote:
>
>> On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
>> * Dave Hansen:
>>
>>>> On 11/27/18 3:57 AM, Florian Weimer wrote:
>>>> I would have expected something that translates PKEY_DISABLE_WRITE |
>>>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
>>>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
>>>>
>>>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
>>>> access, but produces execute-only memory.)
>>>
>>> Correct, it disables all data access, but not execution.
>>
>> So I would expect something like this (completely untested, I did not
>> even compile this):
>
>
> Ok. I re-read through the entire email thread to understand the problem and
> the proposed solution. Let me summarize it below. Lets see if we are on the same
> plate.
>
> So the problem is as follows:
>
> Currently the kernel supports  'disable-write'  and 'disable-access'.
>
> On x86, cpu supports 'disable-write' and 'disable-access'. This
> matches with what the kernel supports. All good.
>
> However on power, cpu supports 'disable-read' too. Since userspace can
> program the cpu directly, userspace has the ability to set
> 'disable-read' too.  This can lead to inconsistency between the kernel
> and the userspace.
>
> We want the kernel to match userspace on all architectures.
>
> Proposed Solution:
>
> Enhance the kernel to understand 'disable-read', and facilitate architectures
> that understand 'disable-read' to allow it.
>
> Also explicitly define the semantics of disable-access  as
> 'disable-read and disable-write'
>
> Did I get this right?  Assuming I did, the implementation has to do
> the following --
>
>    On power, sys_pkey_alloc() should succeed if the init_val
>    is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
>    or any combination of the three.
>
>    On x86, sys_pkey_alloc() should succeed if the init_val is
>    PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
>    or any combination of the three, except  PKEY_DISABLE_READ
>          specified all by itself.
>
>    On all other arches, none of the flags are supported.

I don’t really love having a situation where you can use different
flag combinations to refer to the same mode.

Also, we should document the effect these flags have on execute permission.

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Wed, Dec 05, 2018 at 08:21:02AM -0800, Andy Lutomirski wrote:
> > On Dec 2, 2018, at 8:02 PM, Ram Pai <linuxram@us.ibm.com> wrote:
> >
> >> On Thu, Nov 29, 2018 at 12:37:15PM +0100, Florian Weimer wrote:
> >> * Dave Hansen:
> >>
> >>>> On 11/27/18 3:57 AM, Florian Weimer wrote:
> >>>> I would have expected something that translates PKEY_DISABLE_WRITE |
> >>>> PKEY_DISABLE_READ into PKEY_DISABLE_ACCESS, and also accepts
> >>>> PKEY_DISABLE_ACCESS | PKEY_DISABLE_READ, for consistency with POWER.
> >>>>
> >>>> (My understanding is that PKEY_DISABLE_ACCESS does not disable all
> >>>> access, but produces execute-only memory.)
> >>>
> >>> Correct, it disables all data access, but not execution.
> >>
> >> So I would expect something like this (completely untested, I did not
> >> even compile this):
> >
> >
> > Ok. I re-read through the entire email thread to understand the problem and
> > the proposed solution. Let me summarize it below. Lets see if we are on the same
> > plate.
> >
> > So the problem is as follows:
> >
> > Currently the kernel supports  'disable-write'  and 'disable-access'.
> >
> > On x86, cpu supports 'disable-write' and 'disable-access'. This
> > matches with what the kernel supports. All good.
> >
> > However on power, cpu supports 'disable-read' too. Since userspace can
> > program the cpu directly, userspace has the ability to set
> > 'disable-read' too.  This can lead to inconsistency between the kernel
> > and the userspace.
> >
> > We want the kernel to match userspace on all architectures.
> >
> > Proposed Solution:
> >
> > Enhance the kernel to understand 'disable-read', and facilitate architectures
> > that understand 'disable-read' to allow it.
> >
> > Also explicitly define the semantics of disable-access  as
> > 'disable-read and disable-write'
> >
> > Did I get this right?  Assuming I did, the implementation has to do
> > the following --
> >
> >    On power, sys_pkey_alloc() should succeed if the init_val
> >    is PKEY_DISABLE_READ, PKEY_DISABLE_WRITE, PKEY_DISABLE_ACCESS
> >    or any combination of the three.
> >
> >    On x86, sys_pkey_alloc() should succeed if the init_val is
> >    PKEY_DISABLE_WRITE or PKEY_DISABLE_ACCESS or PKEY_DISABLE_READ
> >    or any combination of the three, except  PKEY_DISABLE_READ
> >          specified all by itself.
> >
> >    On all other arches, none of the flags are supported.
> 
> I don’t really love having a situation where you can use different
> flag combinations to refer to the same mode.

true. But it is a side-effect of x86 cpu implicitly defining
'disable-access' as a combination of 'disable-read' and 'disable_write'.
In other words, if you disable-access on a pte on x86, you are
automatically disabling read and disabling write on that page.
The software/kernel just happens to explicitly capture that implicit
behavior.

> 
> Also, we should document the effect these flags have on execute permission.

Actually none of the above flags, interact with execute permission. They
operate independently; both on x86 and on POWER.  But yes, this
statement needs to be documented somewhere.

RP

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Thu, Nov 08, 2018 at 09:14:54AM -0800, Dave Hansen wrote:
> On 11/8/18 7:01 AM, Florian Weimer wrote:
> > Ideally, PKEY_DISABLE_READ | PKEY_DISABLE_WRITE and PKEY_DISABLE_READ |
> > PKEY_DISABLE_ACCESS would be treated as PKEY_DISABLE_ACCESS both, and a
> > line PKEY_DISABLE_READ would result in an EINVAL failure.
> 
> Sounds reasonable to me.
> 
> I don't see any urgency to do this right now.  It could easily go in
> alongside the ppc patches when those get merged.  The only thing I'd
> suggest is that we make it something slightly higher than 0x4.  It'll
> make the code easier to deal with in the kernel if we have the ABI and
> the hardware mirror each other, and if we pick 0x4 in the ABI for
> PKEY_DISABLE_READ, it might get messy if the harware choose 0x4 for
> PKEY_DISABLE_EXECUTE or something.

The hardware bits have to be decoupled from the software bits. Otherwise
we will get too constrainted and will conflict with the bit
configuration of some hardware. Powerpc implementation can deal with 0x4
or any other value.

> 
> So, let's make it 0x80 or something on x86 at least.
> 
> Also, I'll be happy to review and ack the patch to do this, but I'd
> expect the ppc guys (hi Ram!) to actually put it together.

Hi Dave! :) So what is needed? Support a new flag PKEY_DISABLE_READ, and make it
return error for all architectures?  

Or are we enhancing the symantics of pkey_alloc() to allocate keys with
just disable-read permissions.? And if so, will x86 be able to support
that semantics?


-- 
Ram Pai

Re: pkeys: Reserve PKEY_DISABLE_READ

[Dave Hansen]

On 11/8/18 12:08 PM, Ram Pai wrote:
>> Also, I'll be happy to review and ack the patch to do this, but I'd
>> expect the ppc guys (hi Ram!) to actually put it together.
> Hi Dave! :) So what is needed? Support a new flag PKEY_DISABLE_READ, and make it
> return error for all architectures?  
Florian proposed some semantics further up in the thread.  Basically if
someone asks for PKEY_DISABLE_READ, allow it to be accepted as long as
its compatible with the other flags.

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> Hi Dave! :) So what is needed? Support a new flag PKEY_DISABLE_READ,
> and make it return error for all architectures?

PKEY_DISABLE_READ | PKEY_DISABLE_WRITE should be equivalent to
PKEY_DISABLE_ACCESS.  PKEY_DISABLE_READ without any other flag on x86
should return EINVAL (as for other invalid access rights specified for
pkey_alloc).

> Or are we enhancing the symantics of pkey_alloc() to allocate keys with
> just disable-read permissions.? And if so, will x86 be able to support
> that semantics?

I think x86 cannot do this, but POWER can, but it's currently not
possible to express this via pkey_alloc.  That could be fixed, too.

Thanks,
Florian

Re: pkeys: Reserve PKEY_DISABLE_READ

[Ram Pai]

On Thu, Nov 08, 2018 at 01:05:09PM +0100, Florian Weimer wrote:
> Would it be possible to reserve a bit for PKEY_DISABLE_READ?
> 
> I think the POWER implementation can disable read access at the hardware
> level, but not write access, and that cannot be expressed with the
> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.

POWER hardware can disable-read and can **also disable-write**
at the hardware level. It can disable-execute aswell at the
hardware level.   For example if the key bits for a given key in the AMR
register is  
	0b01  it is read-disable
	0b10  it is write-disable

To support access-disable, we make the key value 0b11.

So in case if you want to know if the key is read-disable 'bitwise-and' it
against 0x1.  i.e  (x & 0x1)

RP

Re: pkeys: Reserve PKEY_DISABLE_READ

[Florian Weimer]

* Ram Pai:

> On Thu, Nov 08, 2018 at 01:05:09PM +0100, Florian Weimer wrote:
>> Would it be possible to reserve a bit for PKEY_DISABLE_READ?
>> 
>> I think the POWER implementation can disable read access at the hardware
>> level, but not write access, and that cannot be expressed with the
>> current PKEY_DISABLE_ACCESS and PKEY_DISABLE_WRITE bits.
>
> POWER hardware can disable-read and can **also disable-write**
> at the hardware level. It can disable-execute aswell at the
> hardware level.   For example if the key bits for a given key in the AMR
> register is  
> 	0b01  it is read-disable
> 	0b10  it is write-disable
>
> To support access-disable, we make the key value 0b11.
>
> So in case if you want to know if the key is read-disable 'bitwise-and' it
> against 0x1.  i.e  (x & 0x1)

Not sure if we covered that alreay, but my problem is that I cannot
translate a 0b01 mask to a PKEY_DISABLE_* flag combination with the
current flags.  0b10 and 0b11 are fine.

POWER also loses the distinction between PKEY_DISABLE_ACCESS and
PKEY_DISABLE_ACCESS | PKEY_DISABLE_WRITE, but that's fine.  This breaks
the current glibc test case, but I have a patch for that.  Arguably, the
test is wrong or at least overly strict in what it accepts.

Thanks,
Florian