Implement protection key support for POWER

  This still does not address the misc/tst-pkeys failure because there are 
kernel bugs.

I'm not entirely sure if the 32-bit AMR mapping is correct because the 
kernel allocates keys starting with the MSB position.  If in doubt, we 
could make this specific to powerpc64.

Thanks,
Florian
  

On 05/23/2018 11:14 AM, Florian Weimer wrote:
> This still does not address the misc/tst-pkeys failure because there
> are kernel bugs.
> 
> I'm not entirely sure if the 32-bit AMR mapping is correct because
> the kernel allocates keys starting with the MSB position.  If in
> doubt, we could make this specific to powerpc64.
It's nice to see support for this going in early, and I particularly like
the direction this is going with respect to signal handler support you
are discussing upstream in linux. Involving userspace developers will yield
semantics that make more sense for library and application developers rather
than singular use cases driving the hardware e.g. just the needs of a database.

Is this more of an RFC than PATCH?

Regarding this specific patch, what happens when you are not on an ISA3.0
hardware that supports AMR/IAMR/UAMOR registers?

Does tst-pkeys test everything that is required to be tested here?

> Subject: [PATCH] powerpc: Implement memory protection key support
> To: libc-alpha@sourceware.org
> 
> This commit adds the architecture-specific implementations for
> pkey_set and pkey_get.
> 
> Execute-only keys are not yet supported.
> 
> 2018-05-23  Florian Weimer  <fweimer@redhat.com>
> 
> 	* sysdeps/unix/sysv/linux/powerpc/arch-pkey.h: New file.
> 	* sysdeps/unix/sysv/linux/powerpc/pkey_set.c: Likewise.
> 	* sysdeps/unix/sysv/linux/powerpc/pkey_get.c: Likewise.
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/arch-pkey.h b/sysdeps/unix/sysv/linux/powerpc/arch-pkey.h
> new file mode 100644
> index 0000000000..9431826a68
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/arch-pkey.h
> @@ -0,0 +1,55 @@
> +/* Helper functions for manipulating memory protection keys.
> +   Copyright (C) 2017-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#ifndef _ARCH_PKEY_H
> +#define _ARCH_PKEY_H
> +
> +/* Read and write access bits in the AMR register.  Needs to be
> +   translated from and to PKEY_DISABLE_* flags.  */
> +#define PKEY_AMR_READ 1UL
> +#define PKEY_AMR_WRITE 2UL
> +
> +/* Return the value of the AMR register.  */
> +static inline unsigned long int
> +pkey_read (void)
> +{
> +  unsigned long int result;
> +  __asm__ volatile ("mfspr %0, 13" : "=r" (result));
> +  return result;
> +}
> +
> +/* Overwrite the AMR register with VALUE.  */
> +static inline void
> +pkey_write (unsigned long int value)
> +{
> +  __asm__ volatile ("mtspr 13, %0" : : "r" (value));
> +}
> +
> +/* Number of the largest supported key.  This depends on the width of
> +   the AMR register.  */
> +#define PKEY_MAX (sizeof (unsigned long int) * 8 / 2 - 1)
> +_Static_assert (PKEY_MAX == 15 || PKEY_MAX == 31, "PKEY_MAX value");
> +
> +/* Translate key number into AMR index position.  */
> +static inline int
> +pkey_index (int key)
> +{
> +  return 2 * (PKEY_MAX - key);
> +}
> +
> +#endif /* _ARCH_PKEY_H */
> diff --git a/sysdeps/unix/sysv/linux/powerpc/pkey_get.c b/sysdeps/unix/sysv/linux/powerpc/pkey_get.c
> new file mode 100644
> index 0000000000..9c315a4766
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/pkey_get.c
> @@ -0,0 +1,42 @@
> +/* Reading the per-thread memory protection key, x86_64 version.
> +   Copyright (C) 2017-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <arch-pkey.h>
> +#include <errno.h>
> +#include <sys/mman.h>
> +
> +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;
> +}
> diff --git a/sysdeps/unix/sysv/linux/powerpc/pkey_set.c b/sysdeps/unix/sysv/linux/powerpc/pkey_set.c
> new file mode 100644
> index 0000000000..f57a46f1ed
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/powerpc/pkey_set.c
> @@ -0,0 +1,48 @@
> +/* Changing the per-thread memory protection key, powerpc64 version.
> +   Copyright (C) 2017-2018 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public
> +   License as published by the Free Software Foundation; either
> +   version 2.1 of the License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library; if not, see
> +   <http://www.gnu.org/licenses/>.  */
> +
> +#include <arch-pkey.h>
> +#include <errno.h>
> +#include <sys/mman.h>
> +
> +int
> +pkey_set (int key, unsigned int rights)
> +{
> +  if (key < 0 || key > PKEY_MAX || rights > 3)
> +    {
> +      __set_errno (EINVAL);
> +      return -1;
> +    }
> +
> +  /* Translate to AMR bit values.  */
> +  unsigned long int bits;
> +  if (rights & PKEY_DISABLE_ACCESS)
> +    /* The PKEY_DISABLE_WRITE bit does not matter.  */
> +    bits = PKEY_AMR_READ | PKEY_AMR_WRITE;
> +  else if (rights == PKEY_DISABLE_WRITE)
> +    bits = PKEY_AMR_WRITE;
> +  else
> +    bits = 0;
> +
> +  unsigned int index = pkey_index (key);
> +  unsigned long int mask = 3UL << index;
> +  unsigned long int amr = pkey_read ();
> +  amr = (amr & ~mask) | (bits << index);
> +  pkey_write (amr);
> +  return 0;
> +}
  

On 05/23/2018 05:25 PM, Carlos O'Donell wrote:
> On 05/23/2018 11:14 AM, Florian Weimer wrote:
>> This still does not address the misc/tst-pkeys failure because there
>> are kernel bugs.
>>
>> I'm not entirely sure if the 32-bit AMR mapping is correct because
>> the kernel allocates keys starting with the MSB position.  If in
>> doubt, we could make this specific to powerpc64.

> It's nice to see support for this going in early, and I particularly like
> the direction this is going with respect to signal handler support you
> are discussing upstream in linux. Involving userspace developers will yield
> semantics that make more sense for library and application developers rather
> than singular use cases driving the hardware e.g. just the needs of a database.
> 
> Is this more of an RFC than PATCH?

No, I believe this is the correct userspace change to enable support for 
this feature.  The rest has to happen in the kernel.

> Regarding this specific patch, what happens when you are not on an ISA3.0
> hardware that supports AMR/IAMR/UAMOR registers?

POWER silicon has supported this for ages.  The SPR is described in the 
2.06 ABI, and probably has been around way longer.  binutils GAS still 
does not support the mnemonics for AMR access, but the patch does not 
use them.

The userspace interface contract is that you have to call pkey_alloc, 
and if that succeeds, you know that you an call pkey_set and pkey_get. 
pkey_alloc is the only way to get a key number that is usable.

> Does tst-pkeys test everything that is required to be tested here?

It does not cover the current fork and execve kernel bugs, no.  But it 
is kind of hard to write tests now because of the other bugs (the 
incorrect UAMOR value, the unfortunate AMR default).

Thanks,
Florian
  

On 05/23/2018 12:33 PM, Florian Weimer wrote:
> The userspace interface contract is that you have to call pkey_alloc,
> and if that succeeds, you know that you an call pkey_set and
> pkey_get. pkey_alloc is the only way to get a key number that is
> usable.

That makes perfect sense, thanks for explaining that. So on pre-ISA3.0
hardware you can't get a value from pkey_alloc() because the kernel
would handle that side of the equation.

Are we interested in adding this support right know given the bugs
and potential future changes? I know that your patch implies that you
think it's going to be useful, but I want to call this out explicitly
here.
  

On 05/23/2018 06:44 PM, Carlos O'Donell wrote:
> That makes perfect sense, thanks for explaining that. So on pre-ISA3.0
> hardware you can't get a value from pkey_alloc() because the kernel
> would handle that side of the equation.

Ahem.  I already said that this has nothing to do with ISA 3.0.  The 
feature is available on the Fedora builders, for example, which are not 
ISA 3.0.

> Are we interested in adding this support right know given the bugs
> and potential future changes?

Yes, I think even with the current set of kernel bugs, the whole thing 
is still good enough for protecting the GOT and flip access rights in 
_dl_fixup.

Thanks,
Florian
  

On 05/23/2018 12:59 PM, Florian Weimer wrote:
> On 05/23/2018 06:44 PM, Carlos O'Donell wrote:
>> That makes perfect sense, thanks for explaining that. So on pre-ISA3.0
>> hardware you can't get a value from pkey_alloc() because the kernel
>> would handle that side of the equation.
> 
> Ahem.  I already said that this has nothing to do with ISA 3.0.  The feature is available on the Fedora builders, for example, which are not ISA 3.0.

My apologies, let me explain in more detail.

I thought userspace AMR access was added on ISA v3.0.

AMR access in userspace was added in ISA v2.06 Rev B
(via the non-privileged SPR number used in your implementation).

I expect the kernel pkey_alloc() syscall to fail on hardware that
does not support the Authority Mask Register, or some fallback
mechanism.

In Fedora we support quite old POWER hardware, therefore on these
systems the above pkey_alloc() will fail and we won't use this
feature, and will never call any of the functions you add which
might be using a reserved/undefined SPR number on that old hardware.

Does that clarify the point I wanted to make?

>> Are we interested in adding this support right know given the bugs
>> and potential future changes?
> 
> Yes, I think even with the current set of kernel bugs, the whole
> thing is still good enough for protecting the GOT and flip access
> rights in _dl_fixup.
Your patch looks good to me then.
  

On 05/23/2018 07:44 PM, Carlos O'Donell wrote:
>> Yes, I think even with the current set of kernel bugs, the whole
>> thing is still good enough for protecting the GOT and flip access
>> rights in _dl_fixup.

> Your patch looks good to me then.

Thanks.  I'll wait for feedback from the architecture maintainers before 
committing it.

Florian