Re: [PATCH] ld.so: Check protected symbols

[Carlos O'Donell <carlos@redhat.com>]

On 10/7/21 15:56, H.J. Lu via Libc-alpha wrote:
> Add LD_DEBUG=protected to check copy relocations against protected data
> and non-canonical reference to protected function.

This review is part of my backlog patch review (SLI-driven patch review).

This patch doesn't apply anymore due to changes in dl-protected.h.

This is because of 7374c02b683b7110b853a32496a619410364d70b and the
addition of similar diagnostics.

I'm marking this at Changes Requested in patchwork.

If some of this still applies we should rework the diagnostics.

Thank you!

> ---
>  elf/rtld.c                      |  2 ++
>  sysdeps/generic/dl-protected.h  | 24 +++++++++++++++++------
>  sysdeps/generic/ldsodefs.h      |  1 +
>  sysdeps/x86/Makefile            | 13 +++++++++++++
>  sysdeps/x86/tst-protected3.c    | 34 +++++++++++++++++++++++++++++++++
>  sysdeps/x86/tst-protected3mod.c | 25 ++++++++++++++++++++++++
>  6 files changed, 93 insertions(+), 6 deletions(-)
>  create mode 100644 sysdeps/x86/tst-protected3.c
>  create mode 100644 sysdeps/x86/tst-protected3mod.c
> 
> diff --git a/elf/rtld.c b/elf/rtld.c
> index 5eee9e1091..9d7d7533a9 100644
> --- a/elf/rtld.c
> +++ b/elf/rtld.c
> @@ -2560,6 +2560,8 @@ process_dl_debug (struct dl_main_state *state, const char *dl_debug)
>  	DL_DEBUG_STATISTICS },
>        { LEN_AND_STR ("unused"), "determined unused DSOs",
>  	DL_DEBUG_UNUSED },
> +      { LEN_AND_STR ("protected"), "check protected symbols",
> +	DL_DEBUG_PROTECTED },
>        { LEN_AND_STR ("help"), "display this help message and exit",
>  	DL_DEBUG_HELP },
>      };
> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h
> index 244d020dc4..c6cf46e434 100644
> --- a/sysdeps/generic/dl-protected.h
> +++ b/sysdeps/generic/dl-protected.h
> @@ -26,17 +26,18 @@ _dl_check_protected_symbol (const char *undef_name,
>  			    const struct link_map *map,
>  			    int type_class)
>  {
> -  if (undef_map != NULL
> -      && undef_map->l_type == lt_executable
> -      && !(undef_map->l_1_needed
> -	   & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)
> +  if (undef_map == NULL || undef_map->l_type != lt_executable)
> +    return;
> +
> +  if (!(undef_map->l_1_needed
> +	& GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS)
>        && (map->l_1_needed
>  	  & GNU_PROPERTY_1_NEEDED_INDIRECT_EXTERN_ACCESS))
>      {
>        if ((type_class & ELF_RTYPE_CLASS_COPY))
>  	/* Disallow copy relocations in executable against protected
> -	   data symbols in a shared object which needs indirect external
> -	   access.  */
> +	   data symbols in a shared object which needs indirect
> +	   external access.  */
>  	_dl_signal_error (0, map->l_name, undef_name,
>  			  N_("copy relocation against non-copyable protected symbol"));
>        else if (ref->st_value != 0
> @@ -49,6 +50,17 @@ _dl_check_protected_symbol (const char *undef_name,
>  	_dl_signal_error (0, map->l_name, undef_name,
>  			  N_("non-canonical reference to canonical protected function"));
>      }
> +  else if (__glibc_unlikely (GLRO(dl_debug_mask) & DL_DEBUG_PROTECTED))
> +    {
> +      if ((type_class & ELF_RTYPE_CLASS_COPY))
> +	_dl_debug_printf ("%s: copy relocation against protected symbol `%s' in %s\n",
> +			  RTLD_PROGNAME, undef_name, map->l_name);
> +      else if (ref->st_value != 0
> +	       && ref->st_shndx == SHN_UNDEF
> +	       && (type_class & ELF_RTYPE_CLASS_PLT))
> +	_dl_debug_printf ("%s: non-canonical reference to protected function `%s' in %s\n",
> +			  RTLD_PROGNAME, undef_name, map->l_name);
> +    }
>  }
>  
>  #endif /* _DL_PROTECTED_H */
> diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
> index 9ec1511bb0..89ad7b5099 100644
> --- a/sysdeps/generic/ldsodefs.h
> +++ b/sysdeps/generic/ldsodefs.h
> @@ -545,6 +545,7 @@ struct rtld_global_ro
>  /* These two are used only internally.  */
>  #define DL_DEBUG_HELP       (1 << 10)
>  #define DL_DEBUG_PRELINK    (1 << 11)
> +#define DL_DEBUG_PROTECTED  (1 << 12)
>  
>    /* OS version.  */
>    EXTERN unsigned int _dl_osversion;
> diff --git a/sysdeps/x86/Makefile b/sysdeps/x86/Makefile
> index 402986ff68..f4f3a0fc73 100644
> --- a/sysdeps/x86/Makefile
> +++ b/sysdeps/x86/Makefile
> @@ -68,6 +68,19 @@ ifneq ($(have-tunables),no)
>  tst-ifunc-isa-2-ENV = GLIBC_TUNABLES=glibc.cpu.hwcaps=-SSE4_2,-AVX,-AVX2,-AVX512F
>  tst-ifunc-isa-2-static-ENV = $(tst-ifunc-isa-2-ENV)
>  endif
> +
> +ifeq (yes,$(build-shared))
> +tests += tst-protected3
> +modules-names += tst-protected3mod
> +
> +$(objpfx)tst-protected3: $(objpfx)tst-protected3mod.so
> +
> +tst-protected3-ENV = LD_DEBUG=protected LD_DEBUG_OUTPUT=$(objpfx)tst-protected3.debug.out
> +
> +ifeq (yes,$(have-fno-direct-extern-access))
> +CFLAGS-tst-protected3.c += -fdirect-extern-access
> +endif
> +endif
>  endif
>  
>  ifeq ($(subdir),math)
> diff --git a/sysdeps/x86/tst-protected3.c b/sysdeps/x86/tst-protected3.c
> new file mode 100644
> index 0000000000..87d3cd2a45
> --- /dev/null
> +++ b/sysdeps/x86/tst-protected3.c
> @@ -0,0 +1,34 @@
> +/* Test warnings on protected function and data symbols.
> +   Copyright (C) 2021 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 <stddef.h>
> +#include <support/check.h>
> +
> +extern int protected_data;
> +extern int protected_func (void) __attribute__((weak));
> +
> +static int
> +do_test (void)
> +{
> +  TEST_COMPARE (protected_data, 30);
> +  TEST_VERIFY_EXIT (protected_func != NULL);
> +  protected_func ();
> +  return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/x86/tst-protected3mod.c b/sysdeps/x86/tst-protected3mod.c
> new file mode 100644
> index 0000000000..b9588e9015
> --- /dev/null
> +++ b/sysdeps/x86/tst-protected3mod.c
> @@ -0,0 +1,25 @@
> +/* Test warnings on protected function and data symbols.
> +   Copyright (C) 2021 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/>.  */
> +
> +int protected_data __attribute__ ((visibility("protected"))) = 30;
> +
> +__attribute__ ((visibility("protected")))
> +void
> +protected_func (void)
> +{
> +}

-- 
Cheers,
Carlos.