i386: Use _dl_runtime_[resolve|profile]_shstk for SHSTK [BZ #23716]

Message ID 20180926171711.29435-1-hjl.tools@gmail.com
State New
Headers show
Series
  • i386: Use _dl_runtime_[resolve|profile]_shstk for SHSTK [BZ #23716]
Related show

Commit Message

H.J. Lu Sept. 26, 2018, 5:17 p.m.
When elf_machine_runtime_setup is called to set up resolver, it should
use _dl_runtime_resolve_shstk or _dl_runtime_profile_shstk if SHSTK is
enabled by kernel.

Tested on i686 with and without --enable-cet using the following patch:

Comments

Florian Weimer Sept. 26, 2018, 5:23 p.m. | #1
* H. J. Lu:

> diff --git a/sysdeps/i386/dl-trampoline.S b/sysdeps/i386/dl-trampoline.S
> index 6dc0319216..9734f9c981 100644
> --- a/sysdeps/i386/dl-trampoline.S
> +++ b/sysdeps/i386/dl-trampoline.S
> @@ -33,6 +33,7 @@
>  _dl_runtime_resolve:
>  	cfi_adjust_cfa_offset (8)
>  	_CET_ENDBR
> +	hlt
>  	pushl %eax		# Preserve registers otherwise clobbered.
>  	cfi_adjust_cfa_offset (4)
>  	pushl %ecx

That doesn't look right. 8-)

Florian
H.J. Lu Sept. 26, 2018, 5:30 p.m. | #2
On Wed, Sep 26, 2018 at 10:23 AM, Florian Weimer <fweimer@redhat.com> wrote:
> * H. J. Lu:
>
>> diff --git a/sysdeps/i386/dl-trampoline.S b/sysdeps/i386/dl-trampoline.S
>> index 6dc0319216..9734f9c981 100644
>> --- a/sysdeps/i386/dl-trampoline.S
>> +++ b/sysdeps/i386/dl-trampoline.S
>> @@ -33,6 +33,7 @@
>>  _dl_runtime_resolve:
>>       cfi_adjust_cfa_offset (8)
>>       _CET_ENDBR
>> +     hlt
>>       pushl %eax              # Preserve registers otherwise clobbered.
>>       cfi_adjust_cfa_offset (4)
>>       pushl %ecx
>
> That doesn't look right. 8-)
>

This is the change I used to test my fix to verify that the SHSTK resolver
is used if SHSTK is enabled by kernel.  It isn't the part of the fix.
H.J. Lu Sept. 28, 2018, 2:33 p.m. | #3
On Wed, Sep 26, 2018 at 10:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Sep 26, 2018 at 10:23 AM, Florian Weimer <fweimer@redhat.com> wrote:
>> * H. J. Lu:
>>
>>> diff --git a/sysdeps/i386/dl-trampoline.S b/sysdeps/i386/dl-trampoline.S
>>> index 6dc0319216..9734f9c981 100644
>>> --- a/sysdeps/i386/dl-trampoline.S
>>> +++ b/sysdeps/i386/dl-trampoline.S
>>> @@ -33,6 +33,7 @@
>>>  _dl_runtime_resolve:
>>>       cfi_adjust_cfa_offset (8)
>>>       _CET_ENDBR
>>> +     hlt
>>>       pushl %eax              # Preserve registers otherwise clobbered.
>>>       cfi_adjust_cfa_offset (4)
>>>       pushl %ecx
>>
>> That doesn't look right. 8-)
>>
>
> This is the change I used to test my fix to verify that the SHSTK resolver
> is used if SHSTK is enabled by kernel.  It isn't the part of the fix.
>

We verified that the fix worked on CET simulator.  If there is no objection,
I will check it later today.

Thanks.
H.J. Lu Sept. 28, 2018, 8:25 p.m. | #4
On Fri, Sep 28, 2018 at 7:33 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Wed, Sep 26, 2018 at 10:30 AM, H.J. Lu <hjl.tools@gmail.com> wrote:
>> On Wed, Sep 26, 2018 at 10:23 AM, Florian Weimer <fweimer@redhat.com> wrote:
>>> * H. J. Lu:
>>>
>>>> diff --git a/sysdeps/i386/dl-trampoline.S b/sysdeps/i386/dl-trampoline.S
>>>> index 6dc0319216..9734f9c981 100644
>>>> --- a/sysdeps/i386/dl-trampoline.S
>>>> +++ b/sysdeps/i386/dl-trampoline.S
>>>> @@ -33,6 +33,7 @@
>>>>  _dl_runtime_resolve:
>>>>       cfi_adjust_cfa_offset (8)
>>>>       _CET_ENDBR
>>>> +     hlt
>>>>       pushl %eax              # Preserve registers otherwise clobbered.
>>>>       cfi_adjust_cfa_offset (4)
>>>>       pushl %ecx
>>>
>>> That doesn't look right. 8-)
>>>
>>
>> This is the change I used to test my fix to verify that the SHSTK resolver
>> is used if SHSTK is enabled by kernel.  It isn't the part of the fix.
>>
>
> We verified that the fix worked on CET simulator.  If there is no objection,
> I will check it later today.
>
> Thanks.

This is the patch I am checking in now.

Patch

diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index f6cfb90e21..d4a737ba0d 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -73,6 +73,7 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
   bool shstk_enabled
     = (GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;

+  shstk_enabled = true;
   if (l->l_info[DT_JMPREL] && lazy)
     {
       /* The GOT entries for functions in the PLT have not yet been filled
diff --git a/sysdeps/i386/dl-trampoline.S b/sysdeps/i386/dl-trampoline.S
index 6dc0319216..9734f9c981 100644
--- a/sysdeps/i386/dl-trampoline.S
+++ b/sysdeps/i386/dl-trampoline.S
@@ -33,6 +33,7 @@ 
 _dl_runtime_resolve:
 	cfi_adjust_cfa_offset (8)
 	_CET_ENDBR
+	hlt
 	pushl %eax		# Preserve registers otherwise clobbered.
 	cfi_adjust_cfa_offset (4)
 	pushl %ecx
@@ -130,6 +131,7 @@  _dl_runtime_profile_shstk:
 _dl_runtime_profile:
 	cfi_adjust_cfa_offset (8)
 	_CET_ENDBR
+	hlt
 	pushl %esp
 	cfi_adjust_cfa_offset (4)
 	addl $8, (%esp)		# Account for the pushed PLT data

	[BZ #23716]
	* sysdeps/i386/dl-cet.c: Removed.
	* sysdeps/i386/dl-machine.h (_dl_runtime_resolve_shstk): New
	prototype.
	(_dl_runtime_profile_shstk): Likewise.
	(elf_machine_runtime_setup): Use _dl_runtime_profile_shstk or
	_dl_runtime_resolve_shstk if SHSTK is enabled by kernel.

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 sysdeps/i386/dl-cet.c     | 67 ---------------------------------------
 sysdeps/i386/dl-machine.h | 13 ++++++--
 2 files changed, 11 insertions(+), 69 deletions(-)
 delete mode 100644 sysdeps/i386/dl-cet.c

diff --git a/sysdeps/i386/dl-cet.c b/sysdeps/i386/dl-cet.c
deleted file mode 100644
index 5d9a4e8d51..0000000000
--- a/sysdeps/i386/dl-cet.c
+++ /dev/null
@@ -1,67 +0,0 @@ 
-/* Linux/i386 CET initializers function.
-   Copyright (C) 2018 Free Software Foundation, Inc.
-
-   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
-   <http://www.gnu.org/licenses/>.  */
-
-
-#define LINKAGE static inline
-#define _dl_cet_check cet_check
-#include <sysdeps/x86/dl-cet.c>
-#undef _dl_cet_check
-
-#ifdef SHARED
-void
-_dl_cet_check (struct link_map *main_map, const char *program)
-{
-  cet_check (main_map, program);
-
-  if ((GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_SHSTK))
-    {
-      /* Replace _dl_runtime_resolve and _dl_runtime_profile with
-         _dl_runtime_resolve_shstk and _dl_runtime_profile_shstk,
-	 respectively if SHSTK is enabled.  */
-      extern void _dl_runtime_resolve (Elf32_Word) attribute_hidden;
-      extern void _dl_runtime_resolve_shstk (Elf32_Word) attribute_hidden;
-      extern void _dl_runtime_profile (Elf32_Word) attribute_hidden;
-      extern void _dl_runtime_profile_shstk (Elf32_Word) attribute_hidden;
-      unsigned int i;
-      struct link_map *l;
-      Elf32_Addr *got;
-
-      if (main_map->l_info[DT_JMPREL])
-	{
-	  got = (Elf32_Addr *) D_PTR (main_map, l_info[DT_PLTGOT]);
-	  if (got[2] == (Elf32_Addr) &_dl_runtime_resolve)
-	    got[2] = (Elf32_Addr) &_dl_runtime_resolve_shstk;
-	  else if (got[2] == (Elf32_Addr) &_dl_runtime_profile)
-	    got[2] = (Elf32_Addr) &_dl_runtime_profile_shstk;
-	}
-
-      i = main_map->l_searchlist.r_nlist;
-      while (i-- > 0)
-	{
-	  l = main_map->l_initfini[i];
-	  if (l->l_info[DT_JMPREL])
-	    {
-	      got = (Elf32_Addr *) D_PTR (l, l_info[DT_PLTGOT]);
-	      if (got[2] == (Elf32_Addr) &_dl_runtime_resolve)
-		got[2] = (Elf32_Addr) &_dl_runtime_resolve_shstk;
-	      else if (got[2] == (Elf32_Addr) &_dl_runtime_profile)
-		got[2] = (Elf32_Addr) &_dl_runtime_profile_shstk;
-	    }
-	}
-    }
-}
-#endif
diff --git a/sysdeps/i386/dl-machine.h b/sysdeps/i386/dl-machine.h
index 1afdcbd9ea..f6cfb90e21 100644
--- a/sysdeps/i386/dl-machine.h
+++ b/sysdeps/i386/dl-machine.h
@@ -67,6 +67,11 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
   Elf32_Addr *got;
   extern void _dl_runtime_resolve (Elf32_Word) attribute_hidden;
   extern void _dl_runtime_profile (Elf32_Word) attribute_hidden;
+  extern void _dl_runtime_resolve_shstk (Elf32_Word) attribute_hidden;
+  extern void _dl_runtime_profile_shstk (Elf32_Word) attribute_hidden;
+  /* Check if SHSTK is enabled by kernel.  */
+  bool shstk_enabled
+    = (GL(dl_x86_feature_1)[0] & GNU_PROPERTY_X86_FEATURE_1_SHSTK) != 0;
 
   if (l->l_info[DT_JMPREL] && lazy)
     {
@@ -93,7 +98,9 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
 	 end in this function.  */
       if (__glibc_unlikely (profile))
 	{
-	  got[2] = (Elf32_Addr) &_dl_runtime_profile;
+	  got[2] = (shstk_enabled
+		    ? (Elf32_Addr) &_dl_runtime_profile_shstk
+		    : (Elf32_Addr) &_dl_runtime_profile);
 
 	  if (GLRO(dl_profile) != NULL
 	      && _dl_name_match_p (GLRO(dl_profile), l))
@@ -104,7 +111,9 @@  elf_machine_runtime_setup (struct link_map *l, int lazy, int profile)
       else
 	/* This function will get called to fix up the GOT entry indicated by
 	   the offset on the stack, and then jump to the resolved address.  */
-	got[2] = (Elf32_Addr) &_dl_runtime_resolve;
+	got[2] = (shstk_enabled
+		  ? (Elf32_Addr) &_dl_runtime_resolve_shstk
+		  : (Elf32_Addr) &_dl_runtime_resolve);
     }
 
   return lazy;