Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752009AbdHHBaH (ORCPT ); Mon, 7 Aug 2017 21:30:07 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:35339 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751828AbdHHBaF (ORCPT ); Mon, 7 Aug 2017 21:30:05 -0400 Subject: Re: [PATCH 4/4] selftests/seccomp: Test thread vs process killing To: Kees Cook , linux-kernel@vger.kernel.org Cc: Fabricio Voznika , Andy Lutomirski , Will Drewry , Shuah Khan , linux-kselftest@vger.kernel.org, linux-security-module@vger.kernel.org References: <1501730353-46840-1-git-send-email-keescook@chromium.org> <1501730353-46840-5-git-send-email-keescook@chromium.org> From: Tyler Hicks Message-ID: <2185ad3f-2a08-09b9-2df6-1fa5853506d7@canonical.com> Date: Mon, 7 Aug 2017 20:29:57 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <1501730353-46840-5-git-send-email-keescook@chromium.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="SX6mcQQ7j2U8jibHBWrW1QB8CUfLchHQD" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8988 Lines: 294 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --SX6mcQQ7j2U8jibHBWrW1QB8CUfLchHQD Content-Type: multipart/mixed; boundary="nwM0JwX6j5PDJoOH15VlxVjmosjr32H7K"; protected-headers="v1" From: Tyler Hicks To: Kees Cook , linux-kernel@vger.kernel.org Cc: Fabricio Voznika , Andy Lutomirski , Will Drewry , Shuah Khan , linux-kselftest@vger.kernel.org, linux-security-module@vger.kernel.org Message-ID: <2185ad3f-2a08-09b9-2df6-1fa5853506d7@canonical.com> Subject: Re: [PATCH 4/4] selftests/seccomp: Test thread vs process killing References: <1501730353-46840-1-git-send-email-keescook@chromium.org> <1501730353-46840-5-git-send-email-keescook@chromium.org> In-Reply-To: <1501730353-46840-5-git-send-email-keescook@chromium.org> --nwM0JwX6j5PDJoOH15VlxVjmosjr32H7K Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 08/02/2017 10:19 PM, Kees Cook wrote: > SECCOMP_RET_KILL is supposed to kill the current thread (and userspace > depends on this), so test for this, distinct from killing the entire > process. This also tests killing the entire process with the new > SECCOMP_FILTER_FLAG_KILL_PROCESS flag. (This also moves a bunch of > defines up earlier in the file to use them earlier.) >=20 > Signed-off-by: Kees Cook > --- > tools/testing/selftests/seccomp/seccomp_bpf.c | 182 ++++++++++++++++++= ++------ > 1 file changed, 141 insertions(+), 41 deletions(-) >=20 > diff --git a/tools/testing/selftests/seccomp/seccomp_bpf.c b/tools/test= ing/selftests/seccomp/seccomp_bpf.c > index ee78a53da5d1..68b9faf23ca6 100644 > --- a/tools/testing/selftests/seccomp/seccomp_bpf.c > +++ b/tools/testing/selftests/seccomp/seccomp_bpf.c > @@ -87,6 +87,51 @@ struct seccomp_data { > }; > #endif > =20 > +#ifndef __NR_seccomp > +# if defined(__i386__) > +# define __NR_seccomp 354 > +# elif defined(__x86_64__) > +# define __NR_seccomp 317 > +# elif defined(__arm__) > +# define __NR_seccomp 383 > +# elif defined(__aarch64__) > +# define __NR_seccomp 277 > +# elif defined(__hppa__) > +# define __NR_seccomp 338 > +# elif defined(__powerpc__) > +# define __NR_seccomp 358 > +# elif defined(__s390__) > +# define __NR_seccomp 348 > +# else > +# warning "seccomp syscall number unknown for this architecture" > +# define __NR_seccomp 0xffff > +# endif > +#endif > + > +#ifndef SECCOMP_SET_MODE_STRICT > +#define SECCOMP_SET_MODE_STRICT 0 > +#endif > + > +#ifndef SECCOMP_SET_MODE_FILTER > +#define SECCOMP_SET_MODE_FILTER 1 > +#endif > + > +#ifndef SECCOMP_FILTER_FLAG_TSYNC > +#define SECCOMP_FILTER_FLAG_TSYNC 1 > +#endif > + > +#ifndef SECCOMP_FILTER_FLAG_KILL_PROCESS > +#define SECCOMP_FILTER_FLAG_KILL_PROCESS 2 > +#endif > + > +#ifndef seccomp > +int seccomp(unsigned int op, unsigned int flags, void *args) > +{ > + errno =3D 0; > + return syscall(__NR_seccomp, op, flags, args); > +} > +#endif > + > #if __BYTE_ORDER =3D=3D __LITTLE_ENDIAN > #define syscall_arg(_n) (offsetof(struct seccomp_data, args[_n])) > #elif __BYTE_ORDER =3D=3D __BIG_ENDIAN > @@ -520,6 +565,102 @@ TEST_SIGNAL(KILL_one_arg_six, SIGSYS) > close(fd); > } > =20 > +/* This is a thread task to die via seccomp filter violation. */ > +void *kill_thread(void *data) > +{ > + bool die =3D (bool)data; > + > + if (die) { > + prctl(PR_GET_SECCOMP, 0, 0, 0, 0); > + return (void *)SIBLING_EXIT_FAILURE; > + } > + > + return (void *)SIBLING_EXIT_UNKILLED; > +} > + > +/* Prepare a thread that will kill itself or both of us. */ > +void kill_thread_or_group(struct __test_metadata *_metadata, bool kill= _process) > +{ > + pthread_t thread; > + void *status; > + unsigned int flags; > + /* Kill only when calling __NR_prctl. */ > + struct sock_filter filter[] =3D { > + BPF_STMT(BPF_LD|BPF_W|BPF_ABS, > + offsetof(struct seccomp_data, nr)), > + BPF_JUMP(BPF_JMP|BPF_JEQ|BPF_K, __NR_prctl, 0, 1), > + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_KILL), > + BPF_STMT(BPF_RET|BPF_K, SECCOMP_RET_ALLOW), > + }; > + struct sock_fprog prog =3D { > + .len =3D (unsigned short)ARRAY_SIZE(filter), > + .filter =3D filter, > + }; > + > + ASSERT_EQ(0, prctl(PR_SET_NO_NEW_PRIVS, 1, 0, 0, 0)) { > + TH_LOG("Kernel does not support PR_SET_NO_NEW_PRIVS!"); > + } > + > + flags =3D kill_process ? SECCOMP_FILTER_FLAG_KILL_PROCESS : 0; > + ASSERT_EQ(0, seccomp(SECCOMP_SET_MODE_FILTER, flags, &prog)) { > + if (kill_process) > + TH_LOG("Kernel does not support SECCOMP_FILTER_FLAG_KILL_PROCESS");= > + else > + TH_LOG("Kernel does not support seccomp syscall"); > + } > + > + /* Start a thread that will exit immediately. */ > + ASSERT_EQ(0, pthread_create(&thread, NULL, kill_thread, (void *)false= )); > + ASSERT_EQ(0, pthread_join(thread, &status)); > + ASSERT_EQ(SIBLING_EXIT_UNKILLED, (unsigned long)status); > + > + /* Start a thread that will die immediately. */ > + ASSERT_EQ(0, pthread_create(&thread, NULL, kill_thread, (void *)true)= ); > + ASSERT_EQ(0, pthread_join(thread, &status)); > + ASSERT_NE(SIBLING_EXIT_FAILURE, (unsigned long)status); > + > + /* Only the thread died. Let parent know this thread didn't die. */ This read a little odd to me. How about, "Only the created thread died. Let parent know the this creating thread didn't die."? > + exit(42); > +} > + > +TEST(KILL_thread) > +{ > + int status; > + pid_t child_pid; > + > + child_pid =3D fork(); > + ASSERT_LE(0, child_pid); > + if (child_pid =3D=3D 0) { > + kill_thread_or_group(_metadata, false); > + _exit(38); > + } > + > + ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0)); > + > + /* If only the thread was killed, we'll see exit 42. */ > + ASSERT_EQ(1, WIFEXITED(status)); This is probably nitpicky but, after reading the wait(2) man page, I feel like this should be ASSERT_TRUE(WIFEXITED(status)) instead of comparing to 1. There's no documented guarantee that 1 will be returned. > + ASSERT_EQ(42, WEXITSTATUS(status)); > +} > + > +TEST(KILL_process) > +{ > + int status; > + pid_t child_pid; > + > + child_pid =3D fork(); > + ASSERT_LE(0, child_pid); > + if (child_pid =3D=3D 0) { > + kill_thread_or_group(_metadata, true); > + _exit(38); > + } > + > + ASSERT_EQ(child_pid, waitpid(child_pid, &status, 0)); > + > + /* If the entire process was killed, we'll see SIGSYS. */ > + ASSERT_EQ(1, WIFSIGNALED(status)); Same here. ASSERT_TRUE(WIFSIGNALED(status)). With these small changes, Reviewed-by: Tyler Hicks Tyler > + ASSERT_EQ(SIGSYS, WTERMSIG(status)); > +} > + > /* TODO(wad) add 64-bit versus 32-bit arg tests. */ > TEST(arg_out_of_range) > { > @@ -1675,47 +1816,6 @@ TEST_F_SIGNAL(TRACE_syscall, kill_after_ptrace, = SIGSYS) > EXPECT_NE(self->mypid, syscall(__NR_getpid)); > } > =20 > -#ifndef __NR_seccomp > -# if defined(__i386__) > -# define __NR_seccomp 354 > -# elif defined(__x86_64__) > -# define __NR_seccomp 317 > -# elif defined(__arm__) > -# define __NR_seccomp 383 > -# elif defined(__aarch64__) > -# define __NR_seccomp 277 > -# elif defined(__hppa__) > -# define __NR_seccomp 338 > -# elif defined(__powerpc__) > -# define __NR_seccomp 358 > -# elif defined(__s390__) > -# define __NR_seccomp 348 > -# else > -# warning "seccomp syscall number unknown for this architecture" > -# define __NR_seccomp 0xffff > -# endif > -#endif > - > -#ifndef SECCOMP_SET_MODE_STRICT > -#define SECCOMP_SET_MODE_STRICT 0 > -#endif > - > -#ifndef SECCOMP_SET_MODE_FILTER > -#define SECCOMP_SET_MODE_FILTER 1 > -#endif > - > -#ifndef SECCOMP_FILTER_FLAG_TSYNC > -#define SECCOMP_FILTER_FLAG_TSYNC 1 > -#endif > - > -#ifndef seccomp > -int seccomp(unsigned int op, unsigned int flags, void *args) > -{ > - errno =3D 0; > - return syscall(__NR_seccomp, op, flags, args); > -} > -#endif > - > TEST(seccomp_syscall) > { > struct sock_filter filter[] =3D { >=20 --nwM0JwX6j5PDJoOH15VlxVjmosjr32H7K-- --SX6mcQQ7j2U8jibHBWrW1QB8CUfLchHQD Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJZiRQVAAoJENaSAD2qAscKwucP/i5VKkHOONcZGCKKiLCZ/iYN ZFA+aOx+YlmU+K0/cf4BvqOwhfFD359+o7HyhGYAPj+LcwO+Vht1Q66x4yA7i1nU mqEeqt4/KdXUEmXuz9coumstI7/cMNAdCo1+4+SUVQhqPnUZJ5lcACzr4DMZ0Es0 vyDBMGT4K6KxpGQIGLWXTXKOpiVASx7B76FQ7qQCyNRXv4Q9wIDcLuoCEWF6XZMS WQiAif7l2BE4JKz8SROtUEXkjrE1vgamQ42rszA+87mNnvSqMcB7k2LQFWsr3HiP 9FCQgSDqXFHThXIIIAjnL9fyKbNIjG83q0mze7uPKjsZO0WfxnF00hTtLD0NA4qZ khBbmfUV+72gp6D9oSGVB7MfBU7vYswnP6mIe709oqkD0Pv8SULtKUy9szpzThsb wTH5nGvs0Vi8JhXonfUFXmHH0/UI0gV3iMb+QzuvlVqYojgQa/6NX8BPfVBf46XS oSxKECdzMFkwkgRkq5RwI1IM96Kc27QfdMrkiKicz2yxcAgw2knKEE4siACUq2Bh AHHJNhf9+bdVOALqlj6ExwRI4wpcPTCmFKrnsSA8nSfhDWwKpNFoAqujyuLX7cq5 X2r4SC+30oHEvSWao86qK6EOytoVt6RT57JbkOT/pIWAnTmCvY7iC9x9WJkYiR7N dIdi51zz0XaBm8+ADOC+ =Vazn -----END PGP SIGNATURE----- --SX6mcQQ7j2U8jibHBWrW1QB8CUfLchHQD--