| Message ID | 20200916234503.3553822-1-hjl.tools@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | x86/CET: Update vfork to prevent child return | expand |
* 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.
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 --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