Re: [PATCH v6 3/6] x86/cet: Enable shadow stack during startup

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 12/29/2023 08:43, H.J. Lu wrote:
> Previously, CET was enabled by kernel before passing control to user
> space and the startup code must disable CET if applications or shared
> libraries aren't CET enabled.  Since the current kernel only supports
> shadow stack and won't enable shadow stack before passing control to
> user space, we need to enable shadow stack during startup if the
> application and all shared library are shadow stack enabled.

not all shared libraries are checked for shadow stack compat: ld.so
and vdso are not checked (i.e. kernel mapped dsos other than the
main.exe) and now static linking is another special case that is
handled differently.


_dl_process_property_note is called for property notes in dsos that
are loaded via _dl_map_object_from_fd and for the main.exe. this
happens early at phdr processing time:

- it is not called for ld.so or vdso.
- x86 libc start code calls it for static main.exe.
- it is called twice for main.exe when explicitly loaded via ld.so.
- there may be special link maps (e.g. fake ld.so one for dlmopen)
  those are not processed.

_rtld_main_check is called for main.exe and _dl_open_check is called
for a dlopened library after deps are loaded, but before relocation:

- these callbacks decide if loading the module is ok or not, so they
  have to iterate over dependencies and check the result of the
  previous property note processing. it's not clear what's the right
  way iterate over deps (l_initfini have redundant entries and it is
  missing in case of static linking, what's wrong with using
  l_searchlist.r_list instead?).
- deps include ld.so which is not processed (x86 currently special
  cases it during checks, but that means ld.so may be missing the
  property note so e.g. we would not catch toolchain inconsistency
  and one cannot force cet off by removing the marking from ld.so.)
- is it guaranteed that other than ld.so all deps are processed?
  if not then map->l_x86_feature_1_and may be wrong when checked.
- x86 libc start code does not call these for static main.exe
  instead replicates the logic (see my comment below).

i think cleaning this up would be useful: on aarch64 the note
processing has to do syscalls (to change BTI mappings) so if it
is called redundantly or missing that has significant impact.
and obviously it is useful to know which dsos are checked when
looking for security / reliability guarantees.


> --- a/sysdeps/x86/libc-start.h
> +++ b/sysdeps/x86/libc-start.h
> @@ -19,7 +19,56 @@
>  #ifndef SHARED
>  # define ARCH_SETUP_IREL() apply_irel ()
>  # define ARCH_APPLY_IREL()
> -# ifndef ARCH_SETUP_TLS
> -#  define ARCH_SETUP_TLS() __libc_setup_tls ()
> +# ifdef __CET__
> +/* Get CET features enabled in the static executable.  */
> +
> +static inline unsigned int
> +get_cet_feature (void)
> +{
> +  /* Check if CET is supported and not disabled by tunables.  */
> +  const struct cpu_features *cpu_features = __get_cpu_features ();
> +  unsigned int cet_feature = 0;
> +  if (CPU_FEATURE_USABLE_P (cpu_features, IBT))
> +    cet_feature |= GNU_PROPERTY_X86_FEATURE_1_IBT;
> +  if (CPU_FEATURE_USABLE_P (cpu_features, SHSTK))
> +    cet_feature |= GNU_PROPERTY_X86_FEATURE_1_SHSTK;
> +  if (!cet_feature)
> +    return cet_feature;
> +
> +  struct link_map *main_map = _dl_get_dl_main_map ();
> +
> +  /* Scan program headers backward to check PT_GNU_PROPERTY early for
> +     x86 feature bits on static executable.  */
> +  const ElfW(Phdr) *phdr = GL(dl_phdr);
> +  const ElfW(Phdr) *ph;
> +  for (ph = phdr + GL(dl_phnum); ph != phdr; ph--)
> +    if (ph[-1].p_type == PT_GNU_PROPERTY)
> +      {
> +	_dl_process_pt_gnu_property (main_map, -1, &ph[-1]);
> +	/* Enable IBT and SHSTK only if they are enabled on static
> +	   executable.  */
> +	cet_feature &= (main_map->l_x86_feature_1_and
> +			& (GNU_PROPERTY_X86_FEATURE_1_IBT
> +			   | GNU_PROPERTY_X86_FEATURE_1_SHSTK));
> +	/* Set GL(dl_x86_feature_1) to the enabled CET features.  */
> +	GL(dl_x86_feature_1) = cet_feature;

in theory this logic can be

_rtld_main_check (main_map, _dl_argv[0]);

except l_initfini is currently not set up for static main.exe
which may be an oversight.

> +	break;
> +      }
> +
> +  return cet_feature;
> +}