Re: V4 [PATCH 04/12] x86/CET: Extend arch_prctl syscall for CET control

[Carlos O'Donell <carlos at redhat dot com>]

On 07/24/2018 12:13 PM, H.J. Lu wrote:
> On Tue, Jul 24, 2018 at 8:56 AM, Joseph Myers <joseph@codesourcery.com> wrote:
>> On Tue, 24 Jul 2018, H.J. Lu wrote:
>>
>>> CET arch_prctl bits should be defined in <asm/prctl.h> from Linux kernel
>>> header files.   Here is the updated patch to add x86 <include/asm/prctl.h>.
>>>
>>> OK for master?
>> I don't think the relevant condition for removing such a wrapper is "after
>> the CET kernel interface has been committed into the public kernel".  It's
>> after we require kernel headers recent enough to have that interface,
>> which may well be several years away.  Once the interface has been
>> committed into the public kernel, the wrapper should be updated to name
>> the kernel version that has it and to say it should be removed once we
>> require kernel headers at least that recent.
> Like this?
> 
> 
> -- H.J.
> 
> 
> 0001-x86-CET-Extend-arch_prctl-syscall-for-CET-control.patch
> 
> 
> From 8244c8532d22976c3d92ec25bcefeb3a7854123f Mon Sep 17 00:00:00 2001
> From: "H.J. Lu" <hjl.tools@gmail.com>
> Date: Sat, 7 Jul 2018 07:33:04 -0700
> Subject: [PATCH] x86/CET: Extend arch_prctl syscall for CET control
> 
> CET arch_prctl bits should be defined in <asm/prctl.h> from Linux kernel
> header files.  Add x86 <include/asm/prctl.h> for pre-CET kernel header
> files.
> 
> Note: sysdeps/unix/sysv/linux/x86/include/asm/prctl.h should be removed
> if <asm/prctl.h> from the required kernel header files contains CET
> arch_prctl bits.
> 
>  /* CET features:
>     IBT:   GNU_PROPERTY_X86_FEATURE_1_IBT
>     SHSTK: GNU_PROPERTY_X86_FEATURE_1_SHSTK
>   */
> 
>  /* Return CET features in unsigned long long *addr:
>       features: addr[0].
>       shadow stack base address: addr[1].
>       shadow stack size: addr[2].
>   */
>  # define ARCH_CET_STATUS		0x3001
>  /* Disable CET features in unsigned int features.  */
>  # define ARCH_CET_DISABLE		0x3002
>  /* Lock all CET features.  */
>  # define ARCH_CET_LOCK			0x3003
>  /* Allocate a new shadow stack with unsigned long long *addr:
>       IN: requested shadow stack size: *addr.
>       OUT: allocated shadow stack address: *addr.
>   */
>  # define ARCH_CET_ALLOC_SHSTK		0x3004
>  /* Return legacy region bitmap info in unsigned long long *addr:
>      address: addr[0].
>      size: addr[1].
>   */
>  # define ARCH_CET_LEGACY_BITMAP	0x3005
> 
> 	* sysdeps/unix/sysv/linux/x86/include/asm/prctl.h: New file.
> 	* sysdeps/unix/sysv/linux/x86/cpu-features.c: Include
> 	<sys/prctl.h> and <asm/prctl.h>.
> 	(get_cet_status): Call arch_prctl with ARCH_CET_STATUS.
> 	* sysdeps/unix/sysv/linux/x86/dl-cet.h: Include <sys/prctl.h>
> 	and <asm/prctl.h>.
> 	(dl_cet_allocate_legacy_bitmap): Call arch_prctl with
> 	ARCH_CET_LEGACY_BITMAP.
> 	(dl_cet_disable_cet): Call arch_prctl with ARCH_CET_DISABLE.
> 	(dl_cet_lock_cet): Call arch_prctl with ARCH_CET_LOCK.
> 	* sysdeps/x86/libc-start.c: Include <startup.h>.

OK for 2.28.

This follows the review from Joseph:
* Do not use bits/ for internal headers.
* Expects sys/prctl.h to provide the values.
* Provides an internal asm/prctl.h that mirrors the kernel-provided
  file and which will be deleted when the constants appear upstream.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> ---
>  sysdeps/unix/sysv/linux/x86/cpu-features.c    |  8 +++++
>  sysdeps/unix/sysv/linux/x86/dl-cet.h          | 30 +++++++++++++----
>  .../unix/sysv/linux/x86/include/asm/prctl.h   | 32 +++++++++++++++++++
>  sysdeps/x86/libc-start.c                      |  3 ++
>  4 files changed, 67 insertions(+), 6 deletions(-)
>  create mode 100644 sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
> 
> diff --git a/sysdeps/unix/sysv/linux/x86/cpu-features.c b/sysdeps/unix/sysv/linux/x86/cpu-features.c
> index 7c9df9b794..8566a265b8 100644
> --- a/sysdeps/unix/sysv/linux/x86/cpu-features.c
> +++ b/sysdeps/unix/sysv/linux/x86/cpu-features.c
> @@ -17,9 +17,17 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #if CET_ENABLED
> +# include <sys/prctl.h>
> +# include <asm/prctl.h>

OK.

> +
>  static inline int __attribute__ ((always_inline))
>  get_cet_status (void)
>  {
> +  unsigned long long cet_status[3];
> +  INTERNAL_SYSCALL_DECL (err);
> +  if (INTERNAL_SYSCALL (arch_prctl, err, 2, ARCH_CET_STATUS,
> +			cet_status) == 0)
> +    return cet_status[0];

OK.

>    return 0;
>  }
>  
> diff --git a/sysdeps/unix/sysv/linux/x86/dl-cet.h b/sysdeps/unix/sysv/linux/x86/dl-cet.h
> index ae81e2f2ca..3fbcfebed5 100644
> --- a/sysdeps/unix/sysv/linux/x86/dl-cet.h
> +++ b/sysdeps/unix/sysv/linux/x86/dl-cet.h
> @@ -15,23 +15,41 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> +#include <sys/prctl.h>
> +#include <asm/prctl.h>

OK. Include additional asm/prctl.h which can be removed later.

> +
>  static inline int __attribute__ ((always_inline))
>  dl_cet_allocate_legacy_bitmap (unsigned long *legacy_bitmap)
>  {
> -  /* FIXME: Need syscall support.  */
> -  return -1;
> +  /* Allocate legacy bitmap.  */
> +  INTERNAL_SYSCALL_DECL (err);
> +#ifdef __LP64__
> +  return (int) INTERNAL_SYSCALL (arch_prctl, err, 2,
> +				 ARCH_CET_LEGACY_BITMAP, legacy_bitmap);
> +#else
> +  unsigned long long legacy_bitmap_u64[2];
> +  int res = INTERNAL_SYSCALL (arch_prctl, err, 2,
> +			      ARCH_CET_LEGACY_BITMAP, legacy_bitmap_u64);
> +  if (res == 0)
> +    {
> +      legacy_bitmap[0] = legacy_bitmap_u64[0];
> +      legacy_bitmap[1] = legacy_bitmap_u64[1];
> +    }
> +  return res;
> +#endif

OK.

>  }
>  
>  static inline int __attribute__ ((always_inline))
>  dl_cet_disable_cet (unsigned int cet_feature)
>  {
> -  /* FIXME: Need syscall support.  */
> -  return -1;
> +  INTERNAL_SYSCALL_DECL (err);
> +  return (int) INTERNAL_SYSCALL (arch_prctl, err, 2, ARCH_CET_DISABLE,
> +				 cet_feature);

OK.

>  }
>  
>  static inline int __attribute__ ((always_inline))
>  dl_cet_lock_cet (void)
>  {
> -  /* FIXME: Need syscall support.  */
> -  return -1;
> +  INTERNAL_SYSCALL_DECL (err);
> +  return (int) INTERNAL_SYSCALL (arch_prctl, err, 2, ARCH_CET_LOCK, 0);

OK.

>  }
> diff --git a/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h b/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h
> new file mode 100644
> index 0000000000..f67f3299b9
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/include/asm/prctl.h

OK, not a delivered public header.

> @@ -0,0 +1,32 @@
> +/* FIXME: CET arch_prctl bits should come from the kernel header files.
> +   This file should be removed if <asm/prctl.h> from the required kernel
> +   header files contains CET arch_prctl bits.  */
> +
> +#include_next <asm/prctl.h>
> +
> +#ifndef ARCH_CET_STATUS
> +/* CET features:
> +   IBT:   GNU_PROPERTY_X86_FEATURE_1_IBT
> +   SHSTK: GNU_PROPERTY_X86_FEATURE_1_SHSTK
> + */
> +/* Return CET features in unsigned long long *addr:
> +     features: addr[0].
> +     shadow stack base address: addr[1].
> +     shadow stack size: addr[2].
> + */
> +# define ARCH_CET_STATUS	0x3001
> +/* Disable CET features in unsigned int features.  */
> +# define ARCH_CET_DISABLE	0x3002
> +/* Lock all CET features.  */
> +# define ARCH_CET_LOCK		0x3003
> +/* Allocate a new shadow stack with unsigned long long *addr:
> +     IN: requested shadow stack size: *addr.
> +     OUT: allocated shadow stack address: *addr.
> + */
> +# define ARCH_CET_ALLOC_SHSTK	0x3004
> +/* Return legacy region bitmap info in unsigned long long *addr:
> +     address: addr[0].
> +     size: addr[1].
> + */
> +# define ARCH_CET_LEGACY_BITMAP	0x3005
> +#endif /* ARCH_CET_STATUS */

OK.

> diff --git a/sysdeps/x86/libc-start.c b/sysdeps/x86/libc-start.c
> index 43aba9d061..eb5335c154 100644
> --- a/sysdeps/x86/libc-start.c
> +++ b/sysdeps/x86/libc-start.c
> @@ -16,6 +16,9 @@
>     <http://www.gnu.org/licenses/>.  */
>  
>  #ifndef SHARED
> +/* Define I386_USE_SYSENTER to support syscall during startup in static
> +   PIE.  */
> +# include <startup.h>

OK.

>  # include <ldsodefs.h>
>  # include <cpu-features.h>
>  # include <cpu-features.c>
> -- 2.17.1