diff mbox series

x86: Fixup some nits in longjmp asm implementation

Message ID 20240104221634.3612621-1-goldstein.w.n@gmail.com
State Superseded
Headers
Series x86: Fixup some nits in longjmp asm implementation |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed

Commit Message

Noah Goldstein Jan. 4, 2024, 10:16 p.m. UTC 
  1) Replace `jne` with `jb`. This is generally safer incase either
   r10/rcx are misaligned.

2) Replace a stray `nop` with a `.p2align` directive.
---
 sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S | 2 +-
 sysdeps/x86_64/__longjmp.S                       | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

H.J. Lu Jan. 4, 2024, 10:22 p.m. UTC | #1 
On Thu, Jan 4, 2024 at 2:16 PM Noah Goldstein <goldstein.w.n@gmail.com> wrote:
>
> 1) Replace `jne` with `jb`. This is generally safer incase either
>    r10/rcx are misaligned.
>
> 2) Replace a stray `nop` with a `.p2align` directive.
> ---
>  sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S | 2 +-
>  sysdeps/x86_64/__longjmp.S                       | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
> index 9aa24620b9..083ffb33f2 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
> @@ -57,7 +57,7 @@ longjmp_msg:
>         cfi_def_cfa_offset(16);                                         \
>         LOAD_MSG;                                                       \
>         call    HIDDEN_JUMPTARGET(__fortify_fail);                      \
> -       nop;                                                            \
> +       .p2align 3, 5;                                                          \

This looks good to me.

commit e451d22b22c959a4dbf86dbc9f125985601473ab
Author: Andreas Schwab <schwab@redhat.com>
Date:   Thu Apr 7 16:23:52 2011 -0400

    Maintain stack alignment in ____longjmp_chk on x86_64

had

 #ifdef PIC
-# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
-        call  __GI___fortify_fail
+# define CALL_FAIL  subq  $8, %rsp;                  \
+        cfi_remember_state;                 \
+        cfi_def_cfa_offset(16);                \
+        leaq  longjmp_msg(%rip), %rdi;            \
+        call  __GI___fortify_fail;             \
+        nop;                       \
+        cfi_restore_state

I don't think NOP is required.

>         cfi_restore_state;                                              \
>  .Lok2:                                                                 \
>         movq    %r10, %rdi;                                             \
> diff --git a/sysdeps/x86_64/__longjmp.S b/sysdeps/x86_64/__longjmp.S
> index 22fedc4997..d38ace7512 100644
> --- a/sysdeps/x86_64/__longjmp.S
> +++ b/sysdeps/x86_64/__longjmp.S
> @@ -89,7 +89,7 @@ L(find_restore_token_loop):
>         subq $8, %rcx
>         /* Stop if the current ssp is found.  */
>         cmpq %rcx, %r10
> -       jne L(find_restore_token_loop)
> +       jb L(find_restore_token_loop)

We should stop searching only if we find a match for
the current SSP.  We shouldn't stop searching for JA.

>         jmp L(no_shadow_stack_token)
>
>  L(restore_shadow_stack):
> --
> 2.34.1
>
Andreas Schwab Jan. 4, 2024, 10:40 p.m. UTC | #2 
On Jan 04 2024, H.J. Lu wrote:

>  #ifdef PIC
> -# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
> -        call  __GI___fortify_fail
> +# define CALL_FAIL  subq  $8, %rsp;                  \
> +        cfi_remember_state;                 \
> +        cfi_def_cfa_offset(16);                \
> +        leaq  longjmp_msg(%rip), %rdi;            \
> +        call  __GI___fortify_fail;             \
> +        nop;                       \
> +        cfi_restore_state
>
> I don't think NOP is required.

Is the unwind information correct without the nop?  Without it, the
return address points to the following label.  It is customary that a
noreturn call is followed by a nop to avoid that.
Noah Goldstein Jan. 4, 2024, 11:07 p.m. UTC | #3 
On Thu, Jan 4, 2024 at 2:40 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Jan 04 2024, H.J. Lu wrote:
>
> >  #ifdef PIC
> > -# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
> > -        call  __GI___fortify_fail
> > +# define CALL_FAIL  subq  $8, %rsp;                  \
> > +        cfi_remember_state;                 \
> > +        cfi_def_cfa_offset(16);                \
> > +        leaq  longjmp_msg(%rip), %rdi;            \
> > +        call  __GI___fortify_fail;             \
> > +        nop;                       \
> > +        cfi_restore_state
> >
> > I don't think NOP is required.
>
> Is the unwind information correct without the nop?  Without it, the
> return address points to the following label.  It is customary that a
> noreturn call is followed by a nop to avoid that.

Any link about why it wouldn't be. The only related mention I could find
was:
https://stackoverflow.com/questions/44854497/why-does-64-bit-vc-compiler-add-nop-instruction-after-function-calls
which seems to argue its potentially an unwind optimization, but not
for correctness.
>
> --
> Andreas Schwab, schwab@linux-m68k.org
> GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
> "And now for something completely different."
H.J. Lu Jan. 4, 2024, 11:42 p.m. UTC | #4 
On Thu, Jan 4, 2024 at 2:40 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Jan 04 2024, H.J. Lu wrote:
>
> >  #ifdef PIC
> > -# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
> > -        call  __GI___fortify_fail
> > +# define CALL_FAIL  subq  $8, %rsp;                  \
> > +        cfi_remember_state;                 \
> > +        cfi_def_cfa_offset(16);                \
> > +        leaq  longjmp_msg(%rip), %rdi;            \
> > +        call  __GI___fortify_fail;             \
> > +        nop;                       \
> > +        cfi_restore_state
> >
> > I don't think NOP is required.
>
> Is the unwind information correct without the nop?  Without it, the
> return address points to the following label.  It is customary that a
> noreturn call is followed by a nop to avoid that.
>

Without the return address is the next instruction,

movq %r10, %rdi

What difference does NOP here make?  Are there any visible
impacts without NOP?
Andreas Schwab Jan. 5, 2024, 11:18 a.m. UTC | #5 
On Jan 04 2024, H.J. Lu wrote:

> On Thu, Jan 4, 2024 at 2:40 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
>>
>> On Jan 04 2024, H.J. Lu wrote:
>>
>> >  #ifdef PIC
>> > -# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
>> > -        call  __GI___fortify_fail
>> > +# define CALL_FAIL  subq  $8, %rsp;                  \
>> > +        cfi_remember_state;                 \
>> > +        cfi_def_cfa_offset(16);                \
>> > +        leaq  longjmp_msg(%rip), %rdi;            \
>> > +        call  __GI___fortify_fail;             \
>> > +        nop;                       \
>> > +        cfi_restore_state
>> >
>> > I don't think NOP is required.
>>
>> Is the unwind information correct without the nop?  Without it, the
>> return address points to the following label.  It is customary that a
>> noreturn call is followed by a nop to avoid that.
>>
>
> Without the return address is the next instruction,
>
> movq %r10, %rdi
>
> What difference does NOP here make?  Are there any visible
> impacts without NOP?

Does backtrace work correctly in gdb at each and every point?
H.J. Lu Jan. 5, 2024, 3:22 p.m. UTC | #6 
On Fri, Jan 5, 2024 at 3:18 AM Andreas Schwab <schwab@linux-m68k.org> wrote:
>
> On Jan 04 2024, H.J. Lu wrote:
>
> > On Thu, Jan 4, 2024 at 2:40 PM Andreas Schwab <schwab@linux-m68k.org> wrote:
> >>
> >> On Jan 04 2024, H.J. Lu wrote:
> >>
> >> >  #ifdef PIC
> >> > -# define CALL_FAIL  leaq  longjmp_msg(%rip), %rdi;            \
> >> > -        call  __GI___fortify_fail
> >> > +# define CALL_FAIL  subq  $8, %rsp;                  \
> >> > +        cfi_remember_state;                 \
> >> > +        cfi_def_cfa_offset(16);                \
> >> > +        leaq  longjmp_msg(%rip), %rdi;            \
> >> > +        call  __GI___fortify_fail;             \
> >> > +        nop;                       \
> >> > +        cfi_restore_state
> >> >
> >> > I don't think NOP is required.
> >>
> >> Is the unwind information correct without the nop?  Without it, the
> >> return address points to the following label.  It is customary that a
> >> noreturn call is followed by a nop to avoid that.
> >>
> >
> > Without the return address is the next instruction,
> >
> > movq %r10, %rdi
> >
> > What difference does NOP here make?  Are there any visible
> > impacts without NOP?
>
> Does backtrace work correctly in gdb at each and every point?
>

With this change:

diff --git a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
index 9aa24620b9..0da7c0133e 100644
--- a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
+++ b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
@@ -57,7 +57,6 @@ longjmp_msg:
   cfi_def_cfa_offset(16);                \
   LOAD_MSG;                     \
   call  HIDDEN_JUMPTARGET(__fortify_fail);        \
-  nop;                       \
   cfi_restore_state;                  \
 .Lok2:                          \
   movq  %r10, %rdi;

on debug/tst-longjmp_chk, I got

   0x000055555532334c <+124>: call   0x555555324420 <__GI___fortify_fail>

(gdb) c
Continuing.

Breakpoint 3, 0x000055555532334c in ____longjmp_chk ()
    at ../sysdeps/x86_64/__longjmp.S:57
57 CHECK_INVALID_LONGJMP
(gdb) bt
#0  0x000055555532334c in ____longjmp_chk ()
    at ../sysdeps/x86_64/__longjmp.S:57
#1  0x0000555555324c9f in __GI___longjmp_chk (
    env=env@entry=0x55555555a200 <b>, val=val@entry=1)
    at ../setjmp/longjmp.c:41
#2  0x0000555555556a00 in do_test () at tst-longjmp_chk.c:70
#3  0x0000555555557312 in support_test_main (argc=1, argv=0x7fffffffdcd0,
    config=config@entry=0x7fffffffdb50) at support_test_main.c:413
#4  0x000055555555673f in main (argc=<optimized out>, argv=<optimized out>)
    at ../support/test-driver.c:170
(gdb) si
__GI___fortify_fail (
    msg=0x5555553b712b <longjmp_msg> "longjmp causes uninitialized
stack frame") at fortify_fail.c:23
23 {
(gdb) bt
#0  __GI___fortify_fail (
    msg=0x5555553b712b <longjmp_msg> "longjmp causes uninitialized
stack frame") at fortify_fail.c:23
#1  0x0000555555323351 in ____longjmp_chk ()
    at ../sysdeps/x86_64/__longjmp.S:57
#2  0x0000555555324c9f in __GI___longjmp_chk (
    env=env@entry=0x55555555a200 <b>, val=val@entry=1)
    at ../setjmp/longjmp.c:41
#3  0x0000555555556a00 in do_test () at tst-longjmp_chk.c:70
#4  0x0000555555557312 in support_test_main (argc=1, argv=0x7fffffffdcd0,
    config=config@entry=0x7fffffffdb50) at support_test_main.c:413
#5  0x000055555555673f in main (argc=<optimized out>, argv=<optimized out>)
    at ../support/test-driver.c:170
(gdb)

Does it look OK?
Andreas Schwab Jan. 5, 2024, 4:42 p.m. UTC | #7 
Seems to be ok.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
index 9aa24620b9..083ffb33f2 100644
--- a/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
+++ b/sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S
@@ -57,7 +57,7 @@  longjmp_msg:
 	cfi_def_cfa_offset(16);						\
 	LOAD_MSG;							\
 	call	HIDDEN_JUMPTARGET(__fortify_fail);			\
-	nop;								\
+	.p2align 3, 5;								\
 	cfi_restore_state;						\
 .Lok2:									\
 	movq	%r10, %rdi;						\
diff --git a/sysdeps/x86_64/__longjmp.S b/sysdeps/x86_64/__longjmp.S
index 22fedc4997..d38ace7512 100644
--- a/sysdeps/x86_64/__longjmp.S
+++ b/sysdeps/x86_64/__longjmp.S
@@ -89,7 +89,7 @@  L(find_restore_token_loop):
 	subq $8, %rcx
 	/* Stop if the current ssp is found.  */
 	cmpq %rcx, %r10
-	jne L(find_restore_token_loop)
+	jb L(find_restore_token_loop)
 	jmp L(no_shadow_stack_token)
 
 L(restore_shadow_stack):