[PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
instead of longjmp.  ____longjmp_chk compares the relative stack
values to decide if it is called from a stack frame which called
setjmp.  If not, ____longjmp_chk assumes that an alternate signal
stack is used.  Since comparing the relative stack values isn't
reliable with user context, when there is no signal, ____longjmp_chk
will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
user context test.
---
 stdlib/tst-setcontext10.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/stdlib/tst-setcontext10.c b/stdlib/tst-setcontext10.c
index 2926753cb1..d714563742 100644
--- a/stdlib/tst-setcontext10.c
+++ b/stdlib/tst-setcontext10.c
@@ -16,6 +16,15 @@
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
+/* When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
+   instead of longjmp.  ____longjmp_chk compares the relative stack
+   values to decide if it is called from a stack frame which called
+   setjmp.  If not, ____longjmp_chk assumes that an alternate signal
+   stack is used.  Since comparing the relative stack values isn't
+   reliable with user context, when there is no signal, ____longjmp_chk
+   will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk.  */
+#undef _FORTIFY_SOURCE
+
 #include <stdio.h>
 #include <stdlib.h>
 #include <setjmp.h>
-- 
2.43.0

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[Florian Weimer]

* H. J. Lu:

> When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
> instead of longjmp.  ____longjmp_chk compares the relative stack
> values to decide if it is called from a stack frame which called
> setjmp.  If not, ____longjmp_chk assumes that an alternate signal
> stack is used.  Since comparing the relative stack values isn't
> reliable with user context, when there is no signal, ____longjmp_chk
> will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
> user context test.

Doesn't shadow stack longjmp unwind the shadow stack and provides better
checking than ____longjmp_chk, that is, should we call shadow stack
longjmp from ____longjmp_chk (or select it via an IFUNC)?

Thanks,
Florian

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

On Tue, Dec 19, 2023 at 12:29 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
> > instead of longjmp.  ____longjmp_chk compares the relative stack
> > values to decide if it is called from a stack frame which called
> > setjmp.  If not, ____longjmp_chk assumes that an alternate signal
> > stack is used.  Since comparing the relative stack values isn't
> > reliable with user context, when there is no signal, ____longjmp_chk
> > will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
> > user context test.
>
> Doesn't shadow stack longjmp unwind the shadow stack and provides better
> checking than ____longjmp_chk, that is, should we call shadow stack
> longjmp from ____longjmp_chk (or select it via an IFUNC)?
>

shadow stack longjmp will be called.  But abort happens before it:

#ifdef _STACK_GROWS_DOWN
#define called_from(this, saved) ((this) < (saved))
#else
#define called_from(this, saved) ((this) > (saved))
#endif

_Noreturn extern void ____longjmp_chk (__jmp_buf __env, int __val);

void ____longjmp_chk (__jmp_buf env, int val)
{
  void *this_frame = __builtin_frame_address (0);
  void *saved_frame = JB_FRAME_ADDRESS (env);
  stack_t ss;

  /* If "env" is from a frame that called us, we're all set.  */
  if (called_from(this_frame, saved_frame))  <<< This is false for user context
    __longjmp (env, val);

  /* If we can't get the current stack state, give up and do the longjmp. */
  if (INTERNAL_SYSCALL_CALL (sigaltstack, NULL, &ss) != 0)
    __longjmp (env, val);

  /* If we we are executing on the alternate stack and within the
     bounds, do the longjmp.  */
  if (ss.ss_flags == SS_ONSTACK  <<< This is false since there is no signal.
      && (this_frame >= ss.ss_sp && this_frame < (ss.ss_sp + ss.ss_size)))
    __longjmp (env, val);

  __fortify_fail ("longjmp causes uninitialized stack frame");
}

The x86-64 version is in sysdeps/unix/sysv/linux/x86_64/____longjmp_chk.S

-- 
H.J.

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[Florian Weimer]

* H. J. Lu:

> void ____longjmp_chk (__jmp_buf env, int val)
> {
>   void *this_frame = __builtin_frame_address (0);
>   void *saved_frame = JB_FRAME_ADDRESS (env);
>   stack_t ss;
>
>   /* If "env" is from a frame that called us, we're all set.  */
>   if (called_from(this_frame, saved_frame))  <<< This is false for user context
>     __longjmp (env, val);

Why isn't this a problem without shadow stack?

But what I meant was: Do the longjmp checks really add value over the
checking that shadow stack longjmp inevitably does?

Thanks,
Florian

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

On Tue, Dec 19, 2023 at 11:57 PM Florian Weimer <fweimer@redhat.com> wrote:
>
> * H. J. Lu:
>
> > void ____longjmp_chk (__jmp_buf env, int val)
> > {
> >   void *this_frame = __builtin_frame_address (0);
> >   void *saved_frame = JB_FRAME_ADDRESS (env);
> >   stack_t ss;
> >
> >   /* If "env" is from a frame that called us, we're all set.  */
> >   if (called_from(this_frame, saved_frame))  <<< This is false for user context
> >     __longjmp (env, val);
>
> Why isn't this a problem without shadow stack?

It is a problem without shadow stack:

https://sourceware.org/pipermail/libc-alpha/2023-December/153409.html

> But what I meant was: Do the longjmp checks really add value over the
> checking that shadow stack longjmp inevitably does?
>

No, I don't think so.  But IFUNC can't be used since shadow stack may
be enabled after IFUNC resolver has been initialized.

-- 
H.J.

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

On Wed, Dec 20, 2023 at 4:54 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Tue, Dec 19, 2023 at 11:57 PM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * H. J. Lu:
> >
> > > void ____longjmp_chk (__jmp_buf env, int val)
> > > {
> > >   void *this_frame = __builtin_frame_address (0);
> > >   void *saved_frame = JB_FRAME_ADDRESS (env);
> > >   stack_t ss;
> > >
> > >   /* If "env" is from a frame that called us, we're all set.  */
> > >   if (called_from(this_frame, saved_frame))  <<< This is false for user context
> > >     __longjmp (env, val);
> >
> > Why isn't this a problem without shadow stack?
>
> It is a problem without shadow stack:
>
> https://sourceware.org/pipermail/libc-alpha/2023-December/153409.html
>
> > But what I meant was: Do the longjmp checks really add value over the
> > checking that shadow stack longjmp inevitably does?
> >
>
> No, I don't think so.  But IFUNC can't be used since shadow stack may
> be enabled after IFUNC resolver has been initialized.

____longjmp_chk checks:

static jmp_buf b;

static void
__attribute__ ((noinline))
f (void)
{
  char buf[1000];
  asm volatile ("" : "=m" (buf));

  if (setjmp (b) != 0)
    {
      puts ("second longjmp succeeded");
      exit (1);
    }
}

void
b (void)
{
  f ();
  longjmp (b, 1);
}

Shadow stack doesn't prevent it.


H.J.

-- 
H.J.

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[Siddhesh Poyarekar]

On 2023-12-19 13:02, H.J. Lu wrote:
> When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
> instead of longjmp.  ____longjmp_chk compares the relative stack
> values to decide if it is called from a stack frame which called
> setjmp.  If not, ____longjmp_chk assumes that an alternate signal
> stack is used.  Since comparing the relative stack values isn't
> reliable with user context, when there is no signal, ____longjmp_chk
> will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
> user context test.

I don't remember the details, but ____longjmp_chk seemed to be a source 
for false positives, enough for packages (ceph and python-cysignal IIRC) 
to disable fortification altogether as a result.

If this check is not reliable enough (I haven't had time to check) then 
does it make sense to disable it altogether until we can make this check 
more reliable?

Thanks,
Sid

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

On Tue, Dec 19, 2023 at 1:23 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 2023-12-19 13:02, H.J. Lu wrote:
> > When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
> > instead of longjmp.  ____longjmp_chk compares the relative stack
> > values to decide if it is called from a stack frame which called
> > setjmp.  If not, ____longjmp_chk assumes that an alternate signal
> > stack is used.  Since comparing the relative stack values isn't
> > reliable with user context, when there is no signal, ____longjmp_chk
> > will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
> > user context test.
>
> I don't remember the details, but ____longjmp_chk seemed to be a source
> for false positives, enough for packages (ceph and python-cysignal IIRC)

Do they use user context?

> to disable fortification altogether as a result.
>
> If this check is not reliable enough (I haven't had time to check) then
> does it make sense to disable it altogether until we can make this check
> more reliable?
>

____longjmp_chk doesn't work if user context is used.   In the meantime,
I'd like to undef _FORTIFY_SOURCE in tst-setcontext10.c.

-- 
H.J.

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[Siddhesh Poyarekar]

On 2023-12-19 16:31, H.J. Lu wrote:
> On Tue, Dec 19, 2023 at 1:23 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>>
>> On 2023-12-19 13:02, H.J. Lu wrote:
>>> When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
>>> instead of longjmp.  ____longjmp_chk compares the relative stack
>>> values to decide if it is called from a stack frame which called
>>> setjmp.  If not, ____longjmp_chk assumes that an alternate signal
>>> stack is used.  Since comparing the relative stack values isn't
>>> reliable with user context, when there is no signal, ____longjmp_chk
>>> will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
>>> user context test.
>>
>> I don't remember the details, but ____longjmp_chk seemed to be a source
>> for false positives, enough for packages (ceph and python-cysignal IIRC)
> 
> Do they use user context?

That's the bit I don't remember.  I should take a look at this again 
next year.

>> to disable fortification altogether as a result.
>>
>> If this check is not reliable enough (I haven't had time to check) then
>> does it make sense to disable it altogether until we can make this check
>> more reliable?
>>
> 
> ____longjmp_chk doesn't work if user context is used.   In the meantime,
> I'd like to undef _FORTIFY_SOURCE in tst-setcontext10.c.
> 

I think that is fine.

Thanks,
Sid

Re: [PATCH] tst-setcontext10.c: Undef _FORTIFY_SOURCE

[H.J. Lu]

On Tue, Dec 19, 2023 at 1:38 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
>
> On 2023-12-19 16:31, H.J. Lu wrote:
> > On Tue, Dec 19, 2023 at 1:23 PM Siddhesh Poyarekar <siddhesh@gotplt.org> wrote:
> >>
> >> On 2023-12-19 13:02, H.J. Lu wrote:
> >>> When _FORTIFY_SOURCE is defined to 2, ____longjmp_chk is called,
> >>> instead of longjmp.  ____longjmp_chk compares the relative stack
> >>> values to decide if it is called from a stack frame which called
> >>> setjmp.  If not, ____longjmp_chk assumes that an alternate signal
> >>> stack is used.  Since comparing the relative stack values isn't
> >>> reliable with user context, when there is no signal, ____longjmp_chk
> >>> will fail.  Undefine _FORTIFY_SOURCE to avoid ____longjmp_chk in
> >>> user context test.
> >>
> >> I don't remember the details, but ____longjmp_chk seemed to be a source
> >> for false positives, enough for packages (ceph and python-cysignal IIRC)
> >
> > Do they use user context?
>
> That's the bit I don't remember.  I should take a look at this again
> next year.
>
> >> to disable fortification altogether as a result.
> >>
> >> If this check is not reliable enough (I haven't had time to check) then
> >> does it make sense to disable it altogether until we can make this check
> >> more reliable?
> >>
> >
> > ____longjmp_chk doesn't work if user context is used.   In the meantime,
> > I'd like to undef _FORTIFY_SOURCE in tst-setcontext10.c.
> >
>
> I think that is fine.
>
> Thanks,
> Sid

Pushed.

Thanks.

-- 
H.J.