diff mbox series

x86/CET: Update vfork to prevent child return

Message ID 20200916234503.3553822-1-hjl.tools@gmail.com
State Superseded
Headers show
Series x86/CET: Update vfork to prevent child return | expand

Commit Message

H.J. Lu Sept. 16, 2020, 11:45 p.m. UTC
Child of vfork should either call _exit or one of the exec family of
functions.  But normally there is nothing to prevent child of vfork from
returning to caller of vfork's caller.  With shadow stack enabled, we
can introduce mismatched shadow stack in child of vfork.  When the child
returns from the function in which vfork was called, mismatched shadow
stack will trigger SIGSEGV.
---
 sysdeps/unix/sysv/linux/i386/vfork.S          |  6 +++
 sysdeps/unix/sysv/linux/x86/Makefile          |  5 ++
 sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c | 54 +++++++++++++++++++
 sysdeps/unix/sysv/linux/x86_64/vfork.S        |  6 +++
 4 files changed, 71 insertions(+)
 create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c

Comments

Florian Weimer Sept. 17, 2020, 12:46 a.m. UTC | #1
* H. J. Lu via Libc-alpha:

> Child of vfork should either call _exit or one of the exec family of
> functions.  But normally there is nothing to prevent child of vfork from
> returning to caller of vfork's caller.  With shadow stack enabled, we
> can introduce mismatched shadow stack in child of vfork.  When the child
> returns from the function in which vfork was called, mismatched shadow
> stack will trigger SIGSEGV.
> ---
>  sysdeps/unix/sysv/linux/i386/vfork.S          |  6 +++
>  sysdeps/unix/sysv/linux/x86/Makefile          |  5 ++
>  sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c | 54 +++++++++++++++++++
>  sysdeps/unix/sysv/linux/x86_64/vfork.S        |  6 +++
>  4 files changed, 71 insertions(+)
>  create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
>
> diff --git a/sysdeps/unix/sysv/linux/i386/vfork.S b/sysdeps/unix/sysv/linux/i386/vfork.S
> index ceb41db0bd..e54fdb7e4c 100644
> --- a/sysdeps/unix/sysv/linux/i386/vfork.S
> +++ b/sysdeps/unix/sysv/linux/i386/vfork.S
> @@ -91,6 +91,12 @@ ENTRY (__vfork)
>  	/* Normal return if shadow stack isn't in use.  */
>  	je	L(no_shstk)
>  
> +	testl	%eax, %eax
> +	jnz	2f
> +	/* NB: Jump back to caller directly with mismatched shadow stack
> +	   to prevent child return.  */
> +	jmp	*%ecx
> +2:

Doesn't the jmp need a notrack prefix?  Or does GCC generate special
code for returns_twice functions?

The comment should say that the *function* calling vfork cannot return
in the subprocess.

> diff --git a/sysdeps/unix/sysv/linux/x86/Makefile b/sysdeps/unix/sysv/linux/x86/Makefile
> index 920edd8948..f3fae85c1e 100644
> --- a/sysdeps/unix/sysv/linux/x86/Makefile
> +++ b/sysdeps/unix/sysv/linux/x86/Makefile
> @@ -47,6 +47,11 @@ $(objpfx)tst-cet-property-2.out: $(objpfx)tst-cet-property-2 \
>  	  $(evaluate-test)
>  endif
>  
> +ifeq ($(subdir),posix)
> +tests += tst-cet-vfork-1
> +CFLAGS-tst-cet-vfork-1.c += -mshstk
> +endif

Does -mshstk alter the ISA?  Then I think you can't test for the
presence of support if you build the whole translation unit with
-mshstk.

> diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
> new file mode 100644
> index 0000000000..9ca148e857
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
> @@ -0,0 +1,54 @@
> +/* Verify that child of vfork can't return with shadow stack.

Likewise: It's the vfork-calling function that must not return.

> +__attribute__ ((noclone, noinline))
> +static pid_t
> +do_test_1 (void)
> +{
> +  pid_t pid;
> +
> +  pid = vfork ();
> +  if (pid == 0)
> +    {
> +      /* Child return should trigger SIGSEGV.  */
> +      return 0;
> +    }
> +  _exit (EXIT_SUCCESS);
> +
> +  return pid;

The return statement immediately above is unreachable.

> +static int
> +do_test (void)
> +{
> +  /* NB: This test should trigger SIGSEGV with shadow stack enabled.  */
> +  if (_get_ssp () == 0)
> +    return EXIT_UNSUPPORTED;
> +  return do_test_1 () ? EXIT_SUCCESS : EXIT_FAILURE;
> +}
> +
> +#define EXPECTED_SIGNAL (_get_ssp () == 0 ? 0 : SIGSEGV)
> +#include <support/test-driver.c>

I'm surprised EXPECTED_SIGNAL works here.  I would expect that the
original test process would have to wait using xwaitpid and check for
the signal in the subprocess.

I think it would also be good to add a check that the subprocess
actually returned from vfork without crashing, say using a pipe and a
write before the return statement.

> diff --git a/sysdeps/unix/sysv/linux/x86_64/vfork.S b/sysdeps/unix/sysv/linux/x86_64/vfork.S
> index 776d2fc610..5dd5cb714c 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/vfork.S
> +++ b/sysdeps/unix/sysv/linux/x86_64/vfork.S
> @@ -71,6 +71,12 @@ ENTRY (__vfork)
>  	/* Normal return if shadow stack isn't in use.  */
>  	je	L(no_shstk)
>  
> +	testl	%eax, %eax
> +	jnz	2f
> +	/* NB: Jump back to caller directly with mismatched shadow stack
> +	   to prevent child return.  */
> +	jmp	*%rdi
> +2:
>  	/* Pop return address from shadow stack and jump back to caller
>  	   directly.  */
>  	movl	$1, %esi

See above.
H.J. Lu Sept. 17, 2020, 12:40 p.m. UTC | #2
On Wed, Sep 16, 2020 at 5:46 PM Florian Weimer <fw@deneb.enyo.de> wrote:
>
> * H. J. Lu via Libc-alpha:
>
> > Child of vfork should either call _exit or one of the exec family of
> > functions.  But normally there is nothing to prevent child of vfork from
> > returning to caller of vfork's caller.  With shadow stack enabled, we
> > can introduce mismatched shadow stack in child of vfork.  When the child
> > returns from the function in which vfork was called, mismatched shadow
> > stack will trigger SIGSEGV.
> > ---
> >  sysdeps/unix/sysv/linux/i386/vfork.S          |  6 +++
> >  sysdeps/unix/sysv/linux/x86/Makefile          |  5 ++
> >  sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c | 54 +++++++++++++++++++
> >  sysdeps/unix/sysv/linux/x86_64/vfork.S        |  6 +++
> >  4 files changed, 71 insertions(+)
> >  create mode 100644 sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
> >
> > diff --git a/sysdeps/unix/sysv/linux/i386/vfork.S b/sysdeps/unix/sysv/linux/i386/vfork.S
> > index ceb41db0bd..e54fdb7e4c 100644
> > --- a/sysdeps/unix/sysv/linux/i386/vfork.S
> > +++ b/sysdeps/unix/sysv/linux/i386/vfork.S
> > @@ -91,6 +91,12 @@ ENTRY (__vfork)
> >       /* Normal return if shadow stack isn't in use.  */
> >       je      L(no_shstk)
> >
> > +     testl   %eax, %eax
> > +     jnz     2f
> > +     /* NB: Jump back to caller directly with mismatched shadow stack
> > +        to prevent child return.  */
> > +     jmp     *%ecx
> > +2:
>
> Doesn't the jmp need a notrack prefix?  Or does GCC generate special
> code for returns_twice functions?

The notrack prefix isn't needed ince GCC has

     /* ECF_RETURNS_TWICE is safe even for -ffreestanding.  */
      if (! strcmp (tname, "setjmp")
          || ! strcmp (tname, "sigsetjmp")
          || ! strcmp (name, "savectx")
          || ! strcmp (name, "vfork")
          || ! strcmp (name, "getcontext"))
        flags |= ECF_RETURNS_TWICE;

> The comment should say that the *function* calling vfork cannot return
> in the subprocess.

Fixed.

> > diff --git a/sysdeps/unix/sysv/linux/x86/Makefile b/sysdeps/unix/sysv/linux/x86/Makefile
> > index 920edd8948..f3fae85c1e 100644
> > --- a/sysdeps/unix/sysv/linux/x86/Makefile
> > +++ b/sysdeps/unix/sysv/linux/x86/Makefile
> > @@ -47,6 +47,11 @@ $(objpfx)tst-cet-property-2.out: $(objpfx)tst-cet-property-2 \
> >         $(evaluate-test)
> >  endif
> >
> > +ifeq ($(subdir),posix)
> > +tests += tst-cet-vfork-1
> > +CFLAGS-tst-cet-vfork-1.c += -mshstk
> > +endif
>
> Does -mshstk alter the ISA?  Then I think you can't test for the
> presence of support if you build the whole translation unit with
> -mshstk.

The only thing -mshstk does is to enable _get_ssp ().

> > diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
> > new file mode 100644
> > index 0000000000..9ca148e857
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
> > @@ -0,0 +1,54 @@
> > +/* Verify that child of vfork can't return with shadow stack.
>
> Likewise: It's the vfork-calling function that must not return.

Fixed.

> > +__attribute__ ((noclone, noinline))
> > +static pid_t
> > +do_test_1 (void)
> > +{
> > +  pid_t pid;
> > +
> > +  pid = vfork ();
> > +  if (pid == 0)
> > +    {
> > +      /* Child return should trigger SIGSEGV.  */
> > +      return 0;
> > +    }
> > +  _exit (EXIT_SUCCESS);
> > +
> > +  return pid;
>
> The return statement immediately above is unreachable.

Fixed.

> > +static int
> > +do_test (void)
> > +{
> > +  /* NB: This test should trigger SIGSEGV with shadow stack enabled.  */
> > +  if (_get_ssp () == 0)
> > +    return EXIT_UNSUPPORTED;
> > +  return do_test_1 () ? EXIT_SUCCESS : EXIT_FAILURE;
> > +}
> > +
> > +#define EXPECTED_SIGNAL (_get_ssp () == 0 ? 0 : SIGSEGV)
> > +#include <support/test-driver.c>
>
> I'm surprised EXPECTED_SIGNAL works here.  I would expect that the
> original test process would have to wait using xwaitpid and check for
> the signal in the subprocess.
>
> I think it would also be good to add a check that the subprocess
> actually returned from vfork without crashing, say using a pipe and a
> write before the return statement.

Fixed.

> > diff --git a/sysdeps/unix/sysv/linux/x86_64/vfork.S b/sysdeps/unix/sysv/linux/x86_64/vfork.S
> > index 776d2fc610..5dd5cb714c 100644
> > --- a/sysdeps/unix/sysv/linux/x86_64/vfork.S
> > +++ b/sysdeps/unix/sysv/linux/x86_64/vfork.S
> > @@ -71,6 +71,12 @@ ENTRY (__vfork)
> >       /* Normal return if shadow stack isn't in use.  */
> >       je      L(no_shstk)
> >
> > +     testl   %eax, %eax
> > +     jnz     2f
> > +     /* NB: Jump back to caller directly with mismatched shadow stack
> > +        to prevent child return.  */
> > +     jmp     *%rdi
> > +2:
> >       /* Pop return address from shadow stack and jump back to caller
> >          directly.  */
> >       movl    $1, %esi
>
> See above.

Here is the updated patch to use indirect branch only for child.
diff mbox series

Patch

diff --git a/sysdeps/unix/sysv/linux/i386/vfork.S b/sysdeps/unix/sysv/linux/i386/vfork.S
index ceb41db0bd..e54fdb7e4c 100644
--- a/sysdeps/unix/sysv/linux/i386/vfork.S
+++ b/sysdeps/unix/sysv/linux/i386/vfork.S
@@ -91,6 +91,12 @@  ENTRY (__vfork)
 	/* Normal return if shadow stack isn't in use.  */
 	je	L(no_shstk)
 
+	testl	%eax, %eax
+	jnz	2f
+	/* NB: Jump back to caller directly with mismatched shadow stack
+	   to prevent child return.  */
+	jmp	*%ecx
+2:
 	/* Pop return address from shadow stack and jump back to caller
 	   directly.  */
 	movl	$1, %edx
diff --git a/sysdeps/unix/sysv/linux/x86/Makefile b/sysdeps/unix/sysv/linux/x86/Makefile
index 920edd8948..f3fae85c1e 100644
--- a/sysdeps/unix/sysv/linux/x86/Makefile
+++ b/sysdeps/unix/sysv/linux/x86/Makefile
@@ -47,6 +47,11 @@  $(objpfx)tst-cet-property-2.out: $(objpfx)tst-cet-property-2 \
 	  $(evaluate-test)
 endif
 
+ifeq ($(subdir),posix)
+tests += tst-cet-vfork-1
+CFLAGS-tst-cet-vfork-1.c += -mshstk
+endif
+
 ifeq ($(subdir),stdlib)
 tests += tst-cet-setcontext-1
 CFLAGS-tst-cet-setcontext-1.c += -mshstk
diff --git a/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
new file mode 100644
index 0000000000..9ca148e857
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/x86/tst-cet-vfork-1.c
@@ -0,0 +1,54 @@ 
+/* Verify that child of vfork can't return with shadow stack.
+   Copyright (C) 2020 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <https://www.gnu.org/licenses/>.  */
+
+#include <stdlib.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <x86intrin.h>
+#include <support/test-driver.h>
+#include <support/xsignal.h>
+#include <support/xunistd.h>
+
+__attribute__ ((noclone, noinline))
+static pid_t
+do_test_1 (void)
+{
+  pid_t pid;
+
+  pid = vfork ();
+  if (pid == 0)
+    {
+      /* Child return should trigger SIGSEGV.  */
+      return 0;
+    }
+  _exit (EXIT_SUCCESS);
+
+  return pid;
+}
+
+static int
+do_test (void)
+{
+  /* NB: This test should trigger SIGSEGV with shadow stack enabled.  */
+  if (_get_ssp () == 0)
+    return EXIT_UNSUPPORTED;
+  return do_test_1 () ? EXIT_SUCCESS : EXIT_FAILURE;
+}
+
+#define EXPECTED_SIGNAL (_get_ssp () == 0 ? 0 : SIGSEGV)
+#include <support/test-driver.c>
diff --git a/sysdeps/unix/sysv/linux/x86_64/vfork.S b/sysdeps/unix/sysv/linux/x86_64/vfork.S
index 776d2fc610..5dd5cb714c 100644
--- a/sysdeps/unix/sysv/linux/x86_64/vfork.S
+++ b/sysdeps/unix/sysv/linux/x86_64/vfork.S
@@ -71,6 +71,12 @@  ENTRY (__vfork)
 	/* Normal return if shadow stack isn't in use.  */
 	je	L(no_shstk)
 
+	testl	%eax, %eax
+	jnz	2f
+	/* NB: Jump back to caller directly with mismatched shadow stack
+	   to prevent child return.  */
+	jmp	*%rdi
+2:
 	/* Pop return address from shadow stack and jump back to caller
 	   directly.  */
 	movl	$1, %esi