diff mbox series

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

Message ID 20210803215914.4170913-2-hjl.tools@gmail.com
State Committed
Commit 349b0441dab375099b1d7f6909c1742286a67da9
Delegated to: Adhemerval Zanella Netto
Headers show
Series [1/2] Initial support for GNU_PROPERTY_1_NEEDED | expand

Checks

Context Check Description
dj/TryBot-apply_patch success Patch applied to master at the time it was sent
dj/TryBot-32bit success Build for i686

Commit Message

H.J. Lu Aug. 3, 2021, 9:59 p.m. UTC
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.
---
 elf/dl-lookup.c                |  5 ++++
 sysdeps/generic/dl-protected.h | 54 ++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)
 create mode 100644 sysdeps/generic/dl-protected.h

Comments

H.J. Lu Sept. 27, 2021, 2:10 a.m. UTC | #1
On Tue, Aug 3, 2021 at 2:59 PM H.J. Lu <hjl.tools@gmail.com> 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.

PING:

https://sourceware.org/pipermail/libc-alpha/2021-August/129798.html
https://sourceware.org/pipermail/libc-alpha/2021-August/129799.html
Adhemerval Zanella Oct. 7, 2021, 4:58 p.m. UTC | #2
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.

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.

> +			    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.
H.J. Lu Oct. 7, 2021, 5:10 p.m. UTC | #3
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"));

> > +                         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.
Adhemerval Zanella Oct. 7, 2021, 5:45 p.m. UTC | #4
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.
>
diff mbox series

Patch

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:
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,
+			    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 */