Re: [2.28 patch] Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S

Re: [2.28 patch] Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S

[Florian Weimer]

* H. J. Lu:

> I will backport it to 2.28 branch by the end of this week.

Feel free to backport it sooner. 8-)

Thanks,
Florian

[2.28 patch] Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S

[H.J. Lu]

[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]

I will backport it to 2.28 branch by the end of this week.


H.J.
---------- Forwarded message ----------
From: H.J. Lu <hjl.tools@gmail.com>
Date: Wed, Sep 12, 2018 at 8:20 AM
Subject: Re: [patch] Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S
To: Florian Weimer <fweimer@redhat.com>
Cc: Terry Guo <terry.xpguo@gmail.com>, GNU C Library <libc-alpha@sourceware.org>


On Wed, Sep 12, 2018 at 4:59 AM, Florian Weimer <fweimer@redhat.com> wrote:
> On 09/12/2018 01:57 PM, H.J. Lu wrote:
>>
>> On Wed, Sep 12, 2018 at 12:43 AM, Florian Weimer <fweimer@redhat.com>
>> wrote:
>>>
>>> On 09/07/2018 09:06 AM, Terry Guo wrote:
>>>>
>>>>
>>>> +       /* Clearing frame pointer is insufficient, use CFI.  */
>>>> +       cfi_undefined (eip)
>>>
>>>
>>>
>>> Isn't this a separate fix?
>>
>>
>> Since _start now includes CFI, without "cfi_undefined (eip)",  unwinder
>> may not
>> terminate at _start and one unwind test will fail.
>
>
> Ah!  Please include this information in the commit message or ChangeLog
> entry, too.

This is the patch I am going to check in.


--
H.J.


-- 
H.J.

[-- Attachment #2: 0001-i386-Use-ENTRY-and-END-in-start.S-BZ-23606.patch --]
[-- Type: text/x-patch, Size: 3184 bytes --]

From 402ea0545c9efa49be1596522e517b196d1397a8 Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <hjl.tools@gmail.com>
Date: Wed, 5 Sep 2018 05:58:45 -0700
Subject: [PATCH] i386: Use ENTRY and END in start.S [BZ #23606]

Wrapping the _start function with ENTRY and END to insert ENDBR32 at
function entry when CET is enabled.  Since _start now includes CFI,
without "cfi_undefined (eip)", unwinder may not terminate at _start
and we will get

Program received signal SIGSEGV, Segmentation fault.
0xf7dc661e in ?? () from /lib/libgcc_s.so.1
Missing separate debuginfos, use: dnf debuginfo-install libgcc-8.2.1-3.0.fc28.i686
(gdb) bt
 #0  0xf7dc661e in ?? () from /lib/libgcc_s.so.1
 #1  0xf7dc7c18 in _Unwind_Backtrace () from /lib/libgcc_s.so.1
 #2  0xf7f0d809 in __GI___backtrace (array=array@entry=0xffffc7d0,
    size=size@entry=20) at ../sysdeps/i386/backtrace.c:127
 #3  0x08049254 in compare (p1=p1@entry=0xffffcad0, p2=p2@entry=0xffffcad4)
    at backtrace-tst.c:12
 #4  0xf7e2a28c in msort_with_tmp (p=p@entry=0xffffca5c, b=b@entry=0xffffcad0,
    n=n@entry=2) at msort.c:65
 #5  0xf7e29f64 in msort_with_tmp (n=2, b=0xffffcad0, p=0xffffca5c)
    at msort.c:53
 #6  msort_with_tmp (p=p@entry=0xffffca5c, b=b@entry=0xffffcad0, n=n@entry=5)
    at msort.c:53
 #7  0xf7e29f64 in msort_with_tmp (n=5, b=0xffffcad0, p=0xffffca5c)
    at msort.c:53
 #8  msort_with_tmp (p=p@entry=0xffffca5c, b=b@entry=0xffffcad0, n=n@entry=10)
    at msort.c:53
 #9  0xf7e29f64 in msort_with_tmp (n=10, b=0xffffcad0, p=0xffffca5c)
    at msort.c:53
 #10 msort_with_tmp (p=p@entry=0xffffca5c, b=b@entry=0xffffcad0, n=n@entry=20)
    at msort.c:53
 #11 0xf7e2a5b6 in msort_with_tmp (n=20, b=0xffffcad0, p=0xffffca5c)
    at msort.c:297
 #12 __GI___qsort_r (b=b@entry=0xffffcad0, n=n@entry=20, s=s@entry=4,
    cmp=cmp@entry=0x8049230 <compare>, arg=arg@entry=0x0) at msort.c:297
 #13 0xf7e2a84d in __GI_qsort (b=b@entry=0xffffcad0, n=n@entry=20, s=s@entry=4,
    cmp=cmp@entry=0x8049230 <compare>) at msort.c:308
 #14 0x080490f6 in main (argc=2, argv=0xffffcbd4) at backtrace-tst.c:39

FAIL: debug/backtrace-tst

2018-09-12  H.J. Lu  <hongjiu.lu@intel.com>
	    Xuepeng Guo  <xuepeng.guo@intel.com>

	[BZ #23606]
	* sysdeps/i386/start.S: Include <sysdep.h>
	(_start): Use ENTRY/END to insert ENDBR32 at entry when CET is
	enabled.  Add cfi_undefined (eip).

Signed-off-by: H.J. Lu <hjl.tools@gmail.com>
---
 sysdeps/i386/start.S | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sysdeps/i386/start.S b/sysdeps/i386/start.S
index 91035fa83f..e35e9bd31b 100644
--- a/sysdeps/i386/start.S
+++ b/sysdeps/i386/start.S
@@ -52,10 +52,11 @@
 					NULL
 */
 
-	.text
-	.globl _start
-	.type _start,@function
-_start:
+#include <sysdep.h>
+
+ENTRY (_start)
+	/* Clearing frame pointer is insufficient, use CFI.  */
+	cfi_undefined (eip)
 	/* Clear the frame pointer.  The ABI suggests this be done, to mark
 	   the outermost frame obviously.  */
 	xorl %ebp, %ebp
@@ -131,6 +132,7 @@ _start:
 1:	movl	(%esp), %ebx
 	ret
 #endif
+END (_start)
 
 /* To fulfill the System V/i386 ABI we need this symbol.  Yuck, it's so
    meaningless since we don't support machines < 80386.  */
-- 
2.17.1