diff mbox series

[02/12] x86-64: Add endbr64 to tst-quadmod[12].S

Message ID 20180721142035.21059-3-hjl.tools@gmail.com
State New
Headers show
Series x86/CET: The last 12 patches to enable Intel CET | expand

Commit Message

H.J. Lu July 21, 2018, 2:20 p.m. UTC 
Add endbr64 to tst-quadmod1.S and tst-quadmod2.S so that func and foo
can be called indirectly.

	* sysdeps/x86_64/tst-quadmod1.S (func): Add endbr64 if IBT is
	enabled.
	(foo): Likewise.
	* sysdeps/x86_64/tst-quadmod2.S (func) : Likewise.
	(foo): Likewise.
---
 sysdeps/x86_64/tst-quadmod1.S | 6 ++++++
 sysdeps/x86_64/tst-quadmod2.S | 6 ++++++
 2 files changed, 12 insertions(+)

Comments

Carlos O'Donell July 24, 2018, 2:53 a.m. UTC | #1 
On 07/21/2018 10:20 AM, H.J. Lu wrote:
> Add endbr64 to tst-quadmod1.S and tst-quadmod2.S so that func and foo
> can be called indirectly.
> 
> 	* sysdeps/x86_64/tst-quadmod1.S (func): Add endbr64 if IBT is
> 	enabled.
> 	(foo): Likewise.
> 	* sysdeps/x86_64/tst-quadmod2.S (func) : Likewise.
> 	(foo): Likewise.
> ---
>  sysdeps/x86_64/tst-quadmod1.S | 6 ++++++
>  sysdeps/x86_64/tst-quadmod2.S | 6 ++++++
>  2 files changed, 12 insertions(+)

This is OK as-is, but marking foo with enbr64 seems
beyond what is needed. Only foo calls func directly,
so I would expect only func needing markup. If you can
tighten this up it would be better.

Reviewed-by: Carlos O'Donell <carlos@redhat.com>

> 
> diff --git a/sysdeps/x86_64/tst-quadmod1.S b/sysdeps/x86_64/tst-quadmod1.S
> index 26f2f1b599..c60f9dc89d 100644
> --- a/sysdeps/x86_64/tst-quadmod1.S
> +++ b/sysdeps/x86_64/tst-quadmod1.S
> @@ -28,6 +28,9 @@
>  	.type	func, @function
>  func:
>  	.cfi_startproc
> +#if defined __CET__ && (__CET__ & 1) != 0
> +	endbr64
> +#endif

OK.

>  	xorl	%edi, %edi
>  	jmp	exit@PLT
>  	.cfi_endproc
> @@ -37,6 +40,9 @@ func:
>  foo:
>  	.cfi_startproc
>  	.cfi_def_cfa_register 6
> +#if defined __CET__ && (__CET__ & 1) != 0
> +	endbr64

OK.

> +#endif
>  	movq	.Ljmp(%rip), %rax
>  	subq	$BIAS, %rax
>  	jmp	*%rax
> diff --git a/sysdeps/x86_64/tst-quadmod2.S b/sysdeps/x86_64/tst-quadmod2.S
> index e923adf672..af03444d4f 100644
> --- a/sysdeps/x86_64/tst-quadmod2.S
> +++ b/sysdeps/x86_64/tst-quadmod2.S
> @@ -27,6 +27,9 @@
>  	.type	func, @function
>  func:
>  	.cfi_startproc
> +#if defined __CET__ && (__CET__ & 1) != 0
> +	endbr64

OK. Foo calls func directly.

> +#endif
>  	xorl	%edi, %edi
>  	jmp	exit@PLT
>  	.cfi_endproc
> @@ -36,6 +39,9 @@ func:
>  foo:
>  	.cfi_startproc
>  	.cfi_def_cfa_register 6
> +#if defined __CET__ && (__CET__ & 1) != 0
> +	endbr64
> +#endif
>  	movq	.Ljmp(%rip), %rax
>  	subq	$BIAS, %rax
>  	jmp	*%rax
> 

OK.

c.
H.J. Lu July 24, 2018, 11:58 a.m. UTC | #2 
On Mon, Jul 23, 2018 at 7:53 PM, Carlos O'Donell <carlos@redhat.com> wrote:
> On 07/21/2018 10:20 AM, H.J. Lu wrote:
>> Add endbr64 to tst-quadmod1.S and tst-quadmod2.S so that func and foo
>> can be called indirectly.
>>
>>       * sysdeps/x86_64/tst-quadmod1.S (func): Add endbr64 if IBT is
>>       enabled.
>>       (foo): Likewise.
>>       * sysdeps/x86_64/tst-quadmod2.S (func) : Likewise.
>>       (foo): Likewise.
>> ---
>>  sysdeps/x86_64/tst-quadmod1.S | 6 ++++++
>>  sysdeps/x86_64/tst-quadmod2.S | 6 ++++++
>>  2 files changed, 12 insertions(+)
>
> This is OK as-is, but marking foo with enbr64 seems
> beyond what is needed. Only foo calls func directly,

Both func and foo needs ENDBR64.   All global functions
may be called indirectly via PLT:

[hjl@gnu-cet-2 build-x86_64-linux]$ readelf -rW elf/tst-quad1

Relocation section '.rela.dyn' at offset 0x7f0 contains 2 entries:
    Offset             Info             Type               Symbol's
Value  Symbol's Name + Addend
0000000000403ff0  0000000100000006 R_X86_64_GLOB_DAT
0000000000000000 __libc_start_main@GLIBC_2.2.5 + 0
0000000000403ff8  0000000200000006 R_X86_64_GLOB_DAT
0000000000000000 __gmon_start__ + 0

Relocation section '.rela.plt' at offset 0x820 contains 1 entry:
    Offset             Info             Type               Symbol's
Value  Symbol's Name + Addend
0000000000404018  0000000300000007 R_X86_64_JUMP_SLOT
0000000000000000 foo + 0
[hjl@gnu-cet-2 build-x86_64-linux]$

Without ENDBR64, I got

(gdb) r --direct
Starting program:
/export/build/gnu/tools-build/glibc-cet/build-x86_64-linux/elf/tst-quad1
--direct

Program received signal SIGSEGV, Segmentation fault.
foo () at ../sysdeps/x86_64/tst-quadmod1.S:43
43 movq .Ljmp(%rip), %rax
(gdb)

I will check it as-is.

> so I would expect only func needing markup. If you can
> tighten this up it would be better.
>
> Reviewed-by: Carlos O'Donell <carlos@redhat.com>
>
>>
>> diff --git a/sysdeps/x86_64/tst-quadmod1.S b/sysdeps/x86_64/tst-quadmod1.S
>> index 26f2f1b599..c60f9dc89d 100644
>> --- a/sysdeps/x86_64/tst-quadmod1.S
>> +++ b/sysdeps/x86_64/tst-quadmod1.S
>> @@ -28,6 +28,9 @@
>>       .type   func, @function
>>  func:
>>       .cfi_startproc
>> +#if defined __CET__ && (__CET__ & 1) != 0
>> +     endbr64
>> +#endif
>
> OK.
>
>>       xorl    %edi, %edi
>>       jmp     exit@PLT
>>       .cfi_endproc
>> @@ -37,6 +40,9 @@ func:
>>  foo:
>>       .cfi_startproc
>>       .cfi_def_cfa_register 6
>> +#if defined __CET__ && (__CET__ & 1) != 0
>> +     endbr64
>
> OK.
>
>> +#endif
>>       movq    .Ljmp(%rip), %rax
>>       subq    $BIAS, %rax
>>       jmp     *%rax
>> diff --git a/sysdeps/x86_64/tst-quadmod2.S b/sysdeps/x86_64/tst-quadmod2.S
>> index e923adf672..af03444d4f 100644
>> --- a/sysdeps/x86_64/tst-quadmod2.S
>> +++ b/sysdeps/x86_64/tst-quadmod2.S
>> @@ -27,6 +27,9 @@
>>       .type   func, @function
>>  func:
>>       .cfi_startproc
>> +#if defined __CET__ && (__CET__ & 1) != 0
>> +     endbr64
>
> OK. Foo calls func directly.
>
>> +#endif
>>       xorl    %edi, %edi
>>       jmp     exit@PLT
>>       .cfi_endproc
>> @@ -36,6 +39,9 @@ func:
>>  foo:
>>       .cfi_startproc
>>       .cfi_def_cfa_register 6
>> +#if defined __CET__ && (__CET__ & 1) != 0
>> +     endbr64
>> +#endif
>>       movq    .Ljmp(%rip), %rax
>>       subq    $BIAS, %rax
>>       jmp     *%rax
>>
>
> OK.
>
> c.
Carlos O'Donell July 24, 2018, 12:27 p.m. UTC | #3 
On 07/24/2018 07:58 AM, H.J. Lu wrote:
> On Mon, Jul 23, 2018 at 7:53 PM, Carlos O'Donell <carlos@redhat.com> wrote:
>> On 07/21/2018 10:20 AM, H.J. Lu wrote:
>>> Add endbr64 to tst-quadmod1.S and tst-quadmod2.S so that func and foo
>>> can be called indirectly.
>>>
>>>       * sysdeps/x86_64/tst-quadmod1.S (func): Add endbr64 if IBT is
>>>       enabled.
>>>       (foo): Likewise.
>>>       * sysdeps/x86_64/tst-quadmod2.S (func) : Likewise.
>>>       (foo): Likewise.
>>> ---
>>>  sysdeps/x86_64/tst-quadmod1.S | 6 ++++++
>>>  sysdeps/x86_64/tst-quadmod2.S | 6 ++++++
>>>  2 files changed, 12 insertions(+)
>>
>> This is OK as-is, but marking foo with enbr64 seems
>> beyond what is needed. Only foo calls func directly,
> 
> Both func and foo needs ENDBR64.   All global functions
> may be called indirectly via PLT:

Sorry, you are correct. I wasn't thinking about the fact that the
main program calls via the PLT and we need an ENBR to continue the
CET state machine or the process will be terminated.

Cheers,
Carlos.
diff mbox series

Patch

diff --git a/sysdeps/x86_64/tst-quadmod1.S b/sysdeps/x86_64/tst-quadmod1.S
index 26f2f1b599..c60f9dc89d 100644
--- a/sysdeps/x86_64/tst-quadmod1.S
+++ b/sysdeps/x86_64/tst-quadmod1.S
@@ -28,6 +28,9 @@ 
 	.type	func, @function
 func:
 	.cfi_startproc
+#if defined __CET__ && (__CET__ & 1) != 0
+	endbr64
+#endif
 	xorl	%edi, %edi
 	jmp	exit@PLT
 	.cfi_endproc
@@ -37,6 +40,9 @@  func:
 foo:
 	.cfi_startproc
 	.cfi_def_cfa_register 6
+#if defined __CET__ && (__CET__ & 1) != 0
+	endbr64
+#endif
 	movq	.Ljmp(%rip), %rax
 	subq	$BIAS, %rax
 	jmp	*%rax
diff --git a/sysdeps/x86_64/tst-quadmod2.S b/sysdeps/x86_64/tst-quadmod2.S
index e923adf672..af03444d4f 100644
--- a/sysdeps/x86_64/tst-quadmod2.S
+++ b/sysdeps/x86_64/tst-quadmod2.S
@@ -27,6 +27,9 @@ 
 	.type	func, @function
 func:
 	.cfi_startproc
+#if defined __CET__ && (__CET__ & 1) != 0
+	endbr64
+#endif
 	xorl	%edi, %edi
 	jmp	exit@PLT
 	.cfi_endproc
@@ -36,6 +39,9 @@  func:
 foo:
 	.cfi_startproc
 	.cfi_def_cfa_register 6
+#if defined __CET__ && (__CET__ & 1) != 0
+	endbr64
+#endif
 	movq	.Ljmp(%rip), %rax
 	subq	$BIAS, %rax
 	jmp	*%rax