diff mbox series

[RFC] aarch64: Add support for memory protection keys

Message ID 20240612132242.848429-1-yury.khrustalev@arm.com
State New
Headers show
Series [RFC] aarch64: Add support for memory protection keys | expand

Commit Message

Yury Khrustalev June 12, 2024, 1:22 p.m. UTC 
From: Yury Khrustalev <yury.khrustalev@arm.com>

This patch adds support for memory protection keys on AArch64 targets
where it can be implemented by hardware with enabled Permission Overlay
Extension introduced in Armv8.9 / Armv9.4 [1].

 1. Internal functions "pkey_read" and "pkey_write" to access data
    associated with memory protection keys.
 2. Implementation of API functions "pkey_get" and "pkey_set" for
    the AArch64 target.
 3. New target-specific test that checks behaviour of pkeys on
    AArch64 targets.
 4. This patch also extends existing generic test for pkeys.
 5. HWCAP constant for Permission Overlay Extension feature.

Co-authored-by: Szabolcs Nagy <szabolcs.nagy@arm.com>

[1] DDI 0487K.a Arm Architecture Reference Manual for A-profile architecture

---
This patch is intended to work with and has been tested on a model using the
corresponding patch for Linux kernel [2] (latest version so far). Since the
Linux counterpart is still in progress, I'm posting this as an RFC to start
discussion of the changes on the side of Glibc.

Since I've added changes to the generic test "misc/tst-pkey" that runs on several
targets, I've tested this change on aarch64-linux-gnu and x86_64-linux-gnu using
native build and on the following targets using cross-build (build-many-glibcs.py):
 - aarch64_be-linux-gnu
 - arm-linux-gnueabi
 - armeb-linux-gnueabi
 - arm-linux-gnueabihf
 - armeb-linux-gnueabihf
 - powerpc-linux-gnu
 - powerpc64-linux-gnu
 - x86_64-linux-gnu
and no regressions have been found.

Any feedback welcome! Note, this would only be committed after the kernel API has
been fully agreed.

[2] https://lore.kernel.org/linux-arm-kernel/20240503130147.1154804-1-joey.gouly@arm.com/

---
 sysdeps/unix/sysv/linux/aarch64/Makefile      |  3 +
 sysdeps/unix/sysv/linux/aarch64/arch-pkey.h   | 51 +++++++++++
 sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h  |  1 +
 sysdeps/unix/sysv/linux/aarch64/pkey_get.c    | 53 +++++++++++
 sysdeps/unix/sysv/linux/aarch64/pkey_set.c    | 50 +++++++++++
 .../sysv/linux/aarch64/tst-aarch64-pkey.c     | 89 +++++++++++++++++++
 sysdeps/unix/sysv/linux/tst-pkey.c            | 31 ++++++-
 7 files changed, 277 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/arch-pkey.h
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/pkey_get.c
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/pkey_set.c
 create mode 100644 sysdeps/unix/sysv/linux/aarch64/tst-aarch64-pkey.c

Comments

Florian Weimer June 12, 2024, 3:04 p.m. UTC | #1 
* yury khrustalev:

> +  /* Only the following mapping between POR permission bits (4 bits)
> +     and PKEY flags is guaranteed:
> +
> +       -WXR
> +       0010       => PKEY_DISABLE_ACCESS (execute only)
> +       0011       => PKEY_DISABLE_WRITE (read-execute)
> +       0111       => 0 (no restrictions, read-write-execute)
> +       otherwise  => undefined behavior
> +
> +     Note that pkey_set and pkey_alloc would only set these specific
> +     values.  */
> +
> +  if (perm == S1POE_PERM_X)
> +    return PKEY_DISABLE_ACCESS;
> +  if (perm == S1POE_PERM_RX)
> +    return PKEY_DISABLE_WRITE;
> +  if (perm == S1POE_PERM_RWX)
> +    return 0; // no restrictions
> +  return PKEY_DISABLE_ACCESS; // undefined behavior

POWER has the same translation issue.  I suggest to get additional
constants defined by the kernel, whether it's PKEY_DISABLE_READ and
PKEY_DISABLE_EXEC or the full set of PKEY_ENABLE_* constant may not
matter.

PKEY_DISABLE_ACCESS really should have been PKEY_DISABLE_READ |
PKEY_DISABLE_WRITE, but unfortunately that's not how the x86 hardware
works.

There are also some concerns that pkey_set/pkey_get are impractical as
an interface because of the function call overhead, but that's a
separate matter.

> +  /* Check that unallocated pkey is not accepted by the
> +     pkey_mprotect function.  */
> +  {
> +    int pkey = -1;
> +    pkey = pkey_alloc (0, PKEY_DISABLE_WRITE);
> +    pkey_free (pkey);
> +    int *page = xmmap (NULL, pagesize, PROT_NONE,
> +                       MAP_ANONYMOUS | MAP_PRIVATE, -1);
> +    TEST_COMPARE (pkey_mprotect (page, pagesize, PROT_READ, pkey), -1);
> +    TEST_COMPARE (errno, EINVAL);
> +    xmunmap (page, pagesize);
> +  }

Is this really something that the kernel API checks?

When you tested this on x86_64-linux-gnu, did you run it on a machine
with protection key hardware/kernel support?

Thanks,
Florian
Szabolcs Nagy June 12, 2024, 3:53 p.m. UTC | #2 
The 06/12/2024 17:04, Florian Weimer wrote:
> * yury khrustalev:
> 
> > +  /* Only the following mapping between POR permission bits (4 bits)
> > +     and PKEY flags is guaranteed:
> > +
> > +       -WXR
> > +       0010       => PKEY_DISABLE_ACCESS (execute only)
> > +       0011       => PKEY_DISABLE_WRITE (read-execute)
> > +       0111       => 0 (no restrictions, read-write-execute)
> > +       otherwise  => undefined behavior
> > +
> > +     Note that pkey_set and pkey_alloc would only set these specific
> > +     values.  */
> > +
> > +  if (perm == S1POE_PERM_X)
> > +    return PKEY_DISABLE_ACCESS;
> > +  if (perm == S1POE_PERM_RX)
> > +    return PKEY_DISABLE_WRITE;
> > +  if (perm == S1POE_PERM_RWX)
> > +    return 0; // no restrictions
> > +  return PKEY_DISABLE_ACCESS; // undefined behavior
> 
> POWER has the same translation issue.  I suggest to get additional
> constants defined by the kernel, whether it's PKEY_DISABLE_READ and
> PKEY_DISABLE_EXEC or the full set of PKEY_ENABLE_* constant may not
> matter.
> 
> PKEY_DISABLE_ACCESS really should have been PKEY_DISABLE_READ |
> PKEY_DISABLE_WRITE, but unfortunately that's not how the x86 hardware
> works.

i assume existing aarch64 binaries using pkey would always fail
so we don't have a linux abi requirement to use the constants as
defined today.

i'm not sure if linux generic code relies on the exact values,
but we can discuss changing the values with linux devs.

either way only the disable access/write macros will be portable.

> 
> There are also some concerns that pkey_set/pkey_get are impractical as
> an interface because of the function call overhead, but that's a
> separate matter.
> 

if user code is allowed to change the permission register directly
then pkey_get has a problem if the setting is an uknown value.
on aarch64, 0b1xxx permission values are reserved for future use
so this can happen.

i guess we can document that setting such a value is ub for now.
Yury Khrustalev June 13, 2024, 12:18 p.m. UTC | #3 
Hi Florian,

On Wed, Jun 12, 2024 at 05:04:57PM +0200, Florian Weimer wrote:
> * yury khrustalev:
> 
> > +  /* Only the following mapping between POR permission bits (4 bits)
> > +     and PKEY flags is guaranteed:
> > +
> > +       -WXR
> > +       0010       => PKEY_DISABLE_ACCESS (execute only)
> > +       0011       => PKEY_DISABLE_WRITE (read-execute)
> > +       0111       => 0 (no restrictions, read-write-execute)
> > +       otherwise  => undefined behavior
> > +
> > +     Note that pkey_set and pkey_alloc would only set these specific
> > +     values.  */
> > +
> > +  if (perm == S1POE_PERM_X)
> > +    return PKEY_DISABLE_ACCESS;
> > +  if (perm == S1POE_PERM_RX)
> > +    return PKEY_DISABLE_WRITE;
> > +  if (perm == S1POE_PERM_RWX)
> > +    return 0; // no restrictions
> > +  return PKEY_DISABLE_ACCESS; // undefined behavior
> 
> POWER has the same translation issue.  I suggest to get additional
> constants defined by the kernel, whether it's PKEY_DISABLE_READ and
> PKEY_DISABLE_EXEC or the full set of PKEY_ENABLE_* constant may not
> matter.
> 
> PKEY_DISABLE_ACCESS really should have been PKEY_DISABLE_READ |
> PKEY_DISABLE_WRITE, but unfortunately that's not how the x86 hardware
> works.

I agree with this. There is a separate discussion about this in:
https://lore.kernel.org/linux-arm-kernel/ZlnlQ%2FavUAuSum5R@arm.com/

> There are also some concerns that pkey_set/pkey_get are impractical as
> an interface because of the function call overhead, but that's a
> separate matter.
> 
> > +  /* Check that unallocated pkey is not accepted by the
> > +     pkey_mprotect function.  */
> > +  {
> > +    int pkey = -1;
> > +    pkey = pkey_alloc (0, PKEY_DISABLE_WRITE);
> > +    pkey_free (pkey);
> > +    int *page = xmmap (NULL, pagesize, PROT_NONE,
> > +                       MAP_ANONYMOUS | MAP_PRIVATE, -1);
> > +    TEST_COMPARE (pkey_mprotect (page, pagesize, PROT_READ, pkey), -1);
> > +    TEST_COMPARE (errno, EINVAL);
> > +    xmunmap (page, pagesize);
> > +  }
> 
> Is this really something that the kernel API checks?

Yes, at least this is what documented for errno set by pkey_mprotect
when the pkey used has not been allocated.

> When you tested this on x86_64-linux-gnu, did you run it on a machine
> with protection key hardware/kernel support?

Yes, the misc/tst-pkey test PASS-ed rather than UNSUPPORTED and cpuinfo
on this machine includes "pku".

> Thanks,
> Florian

Kind regards,
Yury
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/aarch64/Makefile b/sysdeps/unix/sysv/linux/aarch64/Makefile
index 40b9a2e5de..1fdad67fae 100644
--- a/sysdeps/unix/sysv/linux/aarch64/Makefile
+++ b/sysdeps/unix/sysv/linux/aarch64/Makefile
@@ -1,5 +1,8 @@ 
 ifeq ($(subdir),misc)
 sysdep_headers += sys/elf.h
+tests += \
+  tst-aarch64-pkey \
+  # tests
 endif
 
 ifeq ($(subdir),stdlib)
diff --git a/sysdeps/unix/sysv/linux/aarch64/arch-pkey.h b/sysdeps/unix/sysv/linux/aarch64/arch-pkey.h
new file mode 100644
index 0000000000..a507bf3608
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/arch-pkey.h
@@ -0,0 +1,51 @@ 
+/* Helper functions for manipulating memory protection keys.
+   Copyright (C) 2024 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
+   <https://www.gnu.org/licenses/>.  */
+
+#ifndef _ARCH_PKEY_H
+#define _ARCH_PKEY_H
+
+#define S1POE_PERM_NO_ACCESS    0b0000UL
+#define S1POE_PERM_R            0b0001UL
+#define S1POE_PERM_X            0b0010UL
+#define S1POE_PERM_RX           0b0011UL
+#define S1POE_PERM_W            0b0100UL
+#define S1POE_PERM_RW           0b0101UL
+#define S1POE_PERM_WX           0b0110UL
+#define S1POE_PERM_RWX          0b0111UL
+
+#define S1POE_PERM_MASK         0b1111UL
+
+#define S1POE_BITS_PER_POI  4UL
+
+/* Return the value of the POR_EL0 register.  */
+static inline unsigned long
+pkey_read (void)
+{
+  unsigned long r;
+  __asm__ volatile ("mrs %0, s3_3_c10_c2_4" : "=r" (r));
+  return r;
+}
+
+/* Overwrite the POR_EL0 register with VALUE.  */
+static inline void
+pkey_write (unsigned long value)
+{
+  __asm__ volatile ("msr s3_3_c10_c2_4, %0" : : "r" (value));
+}
+
+#endif /* _ARCH_PKEY_H */
diff --git a/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h b/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
index 8dceaa1a52..2fa158fcc0 100644
--- a/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
+++ b/sysdeps/unix/sysv/linux/aarch64/bits/hwcap.h
@@ -118,3 +118,4 @@ 
 #define HWCAP2_SME_SF8FMA	(1UL << 60)
 #define HWCAP2_SME_SF8DP4	(1UL << 61)
 #define HWCAP2_SME_SF8DP2	(1UL << 62)
+#define HWCAP2_POE		(1UL << 63)
diff --git a/sysdeps/unix/sysv/linux/aarch64/pkey_get.c b/sysdeps/unix/sysv/linux/aarch64/pkey_get.c
new file mode 100644
index 0000000000..444cd001dc
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/pkey_get.c
@@ -0,0 +1,53 @@ 
+/* Reading the per-thread memory protection key, AArch64 version.
+   Copyright (C) 2024 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <arch-pkey.h>
+#include <errno.h>
+#include <sys/mman.h>
+
+int
+pkey_get (int key)
+{
+  if (key < 0 || key > 15)
+    {
+      __set_errno (EINVAL);
+      return -1;
+    }
+  unsigned long por_el0 = pkey_read ();
+  unsigned long perm = (por_el0 >> (S1POE_BITS_PER_POI * key)) & S1POE_PERM_MASK;
+
+  /* Only the following mapping between POR permission bits (4 bits)
+     and PKEY flags is guaranteed:
+
+       -WXR
+       0010       => PKEY_DISABLE_ACCESS (execute only)
+       0011       => PKEY_DISABLE_WRITE (read-execute)
+       0111       => 0 (no restrictions, read-write-execute)
+       otherwise  => undefined behavior
+
+     Note that pkey_set and pkey_alloc would only set these specific
+     values.  */
+
+  if (perm == S1POE_PERM_X)
+    return PKEY_DISABLE_ACCESS;
+  if (perm == S1POE_PERM_RX)
+    return PKEY_DISABLE_WRITE;
+  if (perm == S1POE_PERM_RWX)
+    return 0; // no restrictions
+  return PKEY_DISABLE_ACCESS; // undefined behavior
+}
diff --git a/sysdeps/unix/sysv/linux/aarch64/pkey_set.c b/sysdeps/unix/sysv/linux/aarch64/pkey_set.c
new file mode 100644
index 0000000000..30b79fcdb2
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/pkey_set.c
@@ -0,0 +1,50 @@ 
+/* Changing the per-thread memory protection key, AArch64 version.
+   Copyright (C) 2024 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
+   <https://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 > 15 || rights > 3)
+    {
+      __set_errno (EINVAL);
+      return -1;
+    }
+  unsigned long mask = S1POE_PERM_MASK << (S1POE_BITS_PER_POI * key);
+  unsigned long por_el0 = pkey_read ();
+  unsigned long perm;
+
+  /* POR ot PKEY mapping:     -WXR
+     PKEY_DISABLE_ACCESS  =>  0010 (execute only)
+     PKEY_DISABLE_WRITE   =>  0011 (read-execute)
+     0, no restrictions   =>  0111 (read-write-execute) */
+
+  if (rights & PKEY_DISABLE_ACCESS)
+    perm = S1POE_PERM_X;
+  else if (rights & PKEY_DISABLE_WRITE)
+    perm = S1POE_PERM_RX;
+  else
+    perm = S1POE_PERM_RWX;
+
+  por_el0 = (por_el0 & ~mask) | (perm << (S1POE_BITS_PER_POI * key));
+  pkey_write (por_el0);
+  return 0;
+}
diff --git a/sysdeps/unix/sysv/linux/aarch64/tst-aarch64-pkey.c b/sysdeps/unix/sysv/linux/aarch64/tst-aarch64-pkey.c
new file mode 100644
index 0000000000..9fa13f7df5
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aarch64/tst-aarch64-pkey.c
@@ -0,0 +1,89 @@ 
+/* AArch64 tests for memory protection keys.
+   Copyright (C) 2024 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
+   <https://www.gnu.org/licenses/>.  */
+
+#include <errno.h>
+#include <inttypes.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <support/check.h>
+#include <support/support.h>
+#include <support/test-driver.h>
+#include <support/xsignal.h>
+#include <support/xunistd.h>
+#include <sys/mman.h>
+
+static volatile sig_atomic_t sigusr1_handler_ran;
+static volatile int pkey;
+
+/* On AArch64 access is revoked during signal handling.  */
+static void
+sigusr1_handler (int signum)
+{
+  TEST_COMPARE (signum, SIGUSR1);
+  TEST_COMPARE (pkey_get (pkey), PKEY_DISABLE_ACCESS);
+  sigusr1_handler_ran += 1;
+}
+
+static int
+do_test (void)
+{
+
+  pkey = pkey_alloc (0, 0);
+  if (pkey < 0)
+    {
+      if (errno == ENOSYS || errno == EINVAL)
+        FAIL_UNSUPPORTED
+          ("kernel or CPU does not support memory protection keys");
+      FAIL_EXIT1 ("pkey_alloc: %m");
+    }
+
+  long pagesize = xsysconf (_SC_PAGESIZE);
+
+  int *page = xmmap (NULL, pagesize, PROT_NONE, MAP_ANONYMOUS | MAP_PRIVATE, -1);
+  pkey = pkey_alloc (0, 0);
+
+  /* On AArch64 pkey == 0 is reserved and should never be accocated.  */
+  TEST_VERIFY (pkey > 0);
+  TEST_COMPARE (pkey_get(pkey), 0);
+
+  /* Check that access is revoked during signal handling
+     with initial rights being set to no restrictions.  */
+  TEST_COMPARE (pkey_mprotect ((void *) page, pagesize, PROT_READ | PROT_WRITE, pkey), 0);
+  xsignal (SIGUSR1, &sigusr1_handler);
+  xraise (SIGUSR1);
+  xsignal (SIGUSR1, SIG_DFL);
+  TEST_COMPARE (sigusr1_handler_ran, 1);
+
+  /* Check that access is revoked during signal handling
+     with initial rights being set to PKEY_DISABLE_WRITE.  */
+  TEST_COMPARE (pkey_set (pkey, PKEY_DISABLE_WRITE), 0);
+  xsignal (SIGUSR1, &sigusr1_handler);
+  xraise (SIGUSR1);
+  xsignal (SIGUSR1, SIG_DFL);
+  TEST_COMPARE (sigusr1_handler_ran, 2);
+
+  TEST_COMPARE (pkey_free(pkey), 0);
+
+  xmunmap ((void *) page, pagesize);
+
+  return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/tst-pkey.c b/sysdeps/unix/sysv/linux/tst-pkey.c
index d9083daab9..5106f2050e 100644
--- a/sysdeps/unix/sysv/linux/tst-pkey.c
+++ b/sysdeps/unix/sysv/linux/tst-pkey.c
@@ -185,6 +185,7 @@  do_test (void)
     xmunmap (page, pagesize);
   }
 
+  /* Create thread before setting up key in the current thread.  */
   xpthread_barrier_init (&barrier, NULL, 2);
   bool delayed_thread_check_access = true;
   pthread_t delayed_thread = xpthread_create
@@ -206,8 +207,36 @@  do_test (void)
       FAIL_EXIT1 ("pkey_alloc: %m");
     }
   TEST_COMPARE (pkey_get (keys[0]), 0);
+
+  /* Check that initial rights that are set via pkey_alloc
+     can be accessed via pkey_get.  */
+  {
+    int pkey = -1;
+    pkey = pkey_alloc (0, PKEY_DISABLE_ACCESS);
+    TEST_COMPARE (pkey_get (pkey), PKEY_DISABLE_ACCESS);
+    pkey_free(pkey);
+    pkey = pkey_alloc (0, PKEY_DISABLE_WRITE);
+    TEST_COMPARE (pkey_get (pkey), PKEY_DISABLE_WRITE);
+    pkey_free(pkey);
+  }
+
+  /* Check that unallocated pkey is not accepted by the
+     pkey_mprotect function.  */
+  {
+    int pkey = -1;
+    pkey = pkey_alloc (0, PKEY_DISABLE_WRITE);
+    pkey_free (pkey);
+    int *page = xmmap (NULL, pagesize, PROT_NONE,
+                       MAP_ANONYMOUS | MAP_PRIVATE, -1);
+    TEST_COMPARE (pkey_mprotect (page, pagesize, PROT_READ, pkey), -1);
+    TEST_COMPARE (errno, EINVAL);
+    xmunmap (page, pagesize);
+  }
+
   for (int i = 1; i < key_count; ++i)
     {
+      /* i == 1 corresponds to PKEY_DISABLE_ACCESS
+         i == 2 corresponds to PKEY_DISABLE_WRITE  */
       keys[i] = pkey_alloc (0, i);
       if (keys[i] < 0)
         FAIL_EXIT1 ("pkey_alloc (0, %d): %m", i);
@@ -349,7 +378,7 @@  do_test (void)
      not what happens in practice.  */
   {
     /* The limit is in place to avoid running indefinitely in case
-       there many keys available.  */
+       there are many keys available.  */
     int *keys_array = xcalloc (100000, sizeof (*keys_array));
     int keys_allocated = 0;
     while (keys_allocated < 100000)