Fix BZ 23606 -- Missing ENDBR32 in sysdeps/i386/start.S

  Hello,

The patch is to fix this issue by wrapping the _start function with
ENTRY and END just like what we did for 64bit target. Tested with
glibc 32bit build, there is no regression. OK for trunk?

BR,
Terry


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

        [BZ #23606]
        * sysdeps/i386/start.S: Wrap _start function with ENTRY and END.
  

On Fri, Sep 7, 2018 at 12:06 AM, Terry Guo <terry.xpguo@gmail.com> wrote:
> Hello,
>
> The patch is to fix this issue by wrapping the _start function with
> ENTRY and END just like what we did for 64bit target. Tested with
> glibc 32bit build, there is no regression. OK for trunk?
>
> BR,
> Terry
>
>
> 2018-09-07  H.J. Lu  <hongjiu.lu@intel.com>
>             Xuepeng Guo  <xuepeng.guo@intel.com>
>
>         [BZ #23606]
>         * sysdeps/i386/start.S: Wrap _start function with ENTRY and END.
>
>
> 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.  */

LGTM.  Please check it in.

If you don't have an account, please send me an attachment of
"git format-patch".  I will check it in for you.

Thanks.
  

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?

The ChangeLog or the commit message should mention ENDBR32 (“Use ENTRY 
to insert ENDBR32 when building for CET” or something like that).

Thanks,
Florian
  

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.

> The ChangeLog or the commit message should mention ENDBR32 (“Use ENTRY to
> insert ENDBR32 when building for CET” or something like that).
>

Please also write ChangeLog as

         * sysdeps/i386/start.S (_start): ...
  

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.

Thanks,
Florian