Re: [PATCH 2/2] Add run-time chesk for indirect external access

[Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org>]

On 07/10/2021 14:10, H.J. Lu wrote:
> On Thu, Oct 7, 2021 at 9:58 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 03/08/2021 18:59, H.J. Lu via Libc-alpha wrote:
>>> When performing symbol lookup for references in executable without
>>> indirect external access:
>>>
>>> 1. Disallow copy relocations in executable against protected data symbols
>>> in a shared object with indirect external access.
>>> 2. Disallow non-zero symbol values of undefined function symbols in
>>> executable, which are used as the function pointer, against protected
>>> function symbols in a shared object with indirect external access.
>>
>> How hard would to add some testcases for both cases?  To simplify we may
>> want to build it iff binutins supports noindirect-extern-access.
> 
> I will submit followup patches with testcases from
> users/hjl/indirect/master branch:
> 
> https://gitlab.com/x86-glibc/glibc/-/tree/users/hjl/indirect/master
> 
> including adding LD_DEBUG=protected to check copy relocations against
> protected data and non-canonical reference to protected function.
> 
>> The rest LGTM, just a nit below due an ununsed variable.
>>
>> Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>
>>
>>> ---
>>>  elf/dl-lookup.c                |  5 ++++
>>>  sysdeps/generic/dl-protected.h | 54 ++++++++++++++++++++++++++++++++++
>>>  2 files changed, 59 insertions(+)
>>>  create mode 100644 sysdeps/generic/dl-protected.h
>>>
>>> diff --git a/elf/dl-lookup.c b/elf/dl-lookup.c
>>> index eea217eb28..430359af39 100644
>>> --- a/elf/dl-lookup.c
>>> +++ b/elf/dl-lookup.c
>>> @@ -24,6 +24,7 @@
>>>  #include <ldsodefs.h>
>>>  #include <dl-hash.h>
>>>  #include <dl-machine.h>
>>> +#include <dl-protected.h>
>>>  #include <sysdep-cancel.h>
>>>  #include <libc-lock.h>
>>>  #include <tls.h>
>>> @@ -527,6 +528,10 @@ do_lookup_x (const char *undef_name, uint_fast32_t new_hash,
>>>         if (__glibc_unlikely (dl_symbol_visibility_binds_local_p (sym)))
>>>           goto skip;
>>>
>>> +       if (ELFW(ST_VISIBILITY) (sym->st_other) == STV_PROTECTED)
>>> +         _dl_check_protected_symbol (undef_name, undef_map, ref, map,
>>> +                                     type_class);
>>> +
>>>         switch (ELFW(ST_BIND) (sym->st_info))
>>>           {
>>>           case STB_WEAK:
>>
>> Ok.
>>
>>> diff --git a/sysdeps/generic/dl-protected.h b/sysdeps/generic/dl-protected.h
>>> new file mode 100644
>>> index 0000000000..244d020dc4
>>> --- /dev/null
>>> +++ b/sysdeps/generic/dl-protected.h
>>> @@ -0,0 +1,54 @@
>>> +/* Support for STV_PROTECTED visibility.  Generic version.
>>> +   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/>.  */
>>> +
>>> +#ifndef _DL_PROTECTED_H
>>> +#define _DL_PROTECTED_H
>>> +
>>> +static inline void __attribute__ ((always_inline))
>>> +_dl_check_protected_symbol (const char *undef_name,
>>
>> This argument seems unused.
> 
> It is used in
> 
>        _dl_signal_error (0, map->l_name, undef_name,
>                           N_("non-canonical reference to canonical
> protected function"));

Indeed, you are right.

> 
>>> +                         const struct link_map *undef_map,
>>> +                         const ElfW(Sym) *ref,
>>> +                         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)
>>> +      && (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.  */
>>> +     _dl_signal_error (0, map->l_name, undef_name,
>>> +                       N_("copy relocation against non-copyable protected symbol"));
>>> +      else if (ref->st_value != 0
>>> +            && ref->st_shndx == SHN_UNDEF
>>> +            && (type_class & ELF_RTYPE_CLASS_PLT))
>>> +     /* Disallow non-zero symbol values of undefined symbols in
>>> +        executable, which are used as the function pointer, against
>>> +        protected function symbols in a shared object with indirect
>>> +        external access.  */
>>> +     _dl_signal_error (0, map->l_name, undef_name,
>>> +                       N_("non-canonical reference to canonical protected function"));
>>> +    }
>>> +}
>>> +
>>> +#endif /* _DL_PROTECTED_H */
>>>
>>
>> Ok.
> 
> I will check it in ASIS.
> 
> Thanks.
>