Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752048AbdHHBXS (ORCPT ); Mon, 7 Aug 2017 21:23:18 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:35169 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751677AbdHHBXO (ORCPT ); Mon, 7 Aug 2017 21:23:14 -0400 Subject: Re: [PATCH 2/4] seccomp: Add SECCOMP_FILTER_FLAG_KILL_PROCESS 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-3-git-send-email-keescook@chromium.org> From: Tyler Hicks Message-ID: <84366abd-bf72-83be-ce42-be5332300eb2@canonical.com> Date: Mon, 7 Aug 2017 20:23:00 -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-3-git-send-email-keescook@chromium.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="04t6iAspViI67dggpQMuXJxQ1de2qhB8J" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8444 Lines: 224 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --04t6iAspViI67dggpQMuXJxQ1de2qhB8J Content-Type: multipart/mixed; boundary="POIRRD6jiKNdSMKjaO4e7C3pcRGrJdcKB"; 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: <84366abd-bf72-83be-ce42-be5332300eb2@canonical.com> Subject: Re: [PATCH 2/4] seccomp: Add SECCOMP_FILTER_FLAG_KILL_PROCESS References: <1501730353-46840-1-git-send-email-keescook@chromium.org> <1501730353-46840-3-git-send-email-keescook@chromium.org> In-Reply-To: <1501730353-46840-3-git-send-email-keescook@chromium.org> --POIRRD6jiKNdSMKjaO4e7C3pcRGrJdcKB 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: > Right now, SECCOMP_RET_KILL kills the current thread. There have been > a few requests for RET_KILL to kill the entire process (the thread > group), but since seccomp's u32 return values are ABI, and ordered by > lowest value, with RET_KILL as 0, there isn't a trivial way to provide > an even smaller value that would mean the more restrictive action of > killing the thread group. >=20 > Instead, create a filter flag that indicates that a RET_KILL from this > filter must kill the process rather than the thread. This can be set > (and not cleared) via the new SECCOMP_FILTER_FLAG_KILL_PROCESS flag. >=20 > Pros: > - the logic for the filter action is contained in the filter. > - userspace can detect support for the feature since earlier kernels > will reject the new flag. > Cons: > - depends on adding an assignment to the seccomp_run_filters() loop > (previous patch). >=20 > Alternatives to this approach with pros/cons: >=20 > - Use a new test during seccomp_run_filters() that treats the RET_DATA > mask of a RET_KILL action as special. If a new bit is set in the data= , > then treat the return value as -1 (lower than 0). > Pros: > - the logic for the filter action is contained in the filter. > Cons: > - added complexity to time-sensitive seccomp_run_filters() loop. > - there isn't a trivial way for userspace to detect if the kernel > supports the feature (earlier kernels will silently ignore the > RET_DATA and only kill the thread). I prefer using a filter flag over a special RET_DATA mask but, for completeness, I wanted to mention that SECCOMP_GET_ACTION_AVAIL operation could be extended to validate special RET_DATA masks. However, I don't think that is a clean design. > - Have SECCOMP_FILTER_FLAG_KILL_PROCESS attach to the seccomp struct > rather than the filter. > Pros: > - no change needed to seccomp_run_filters() loop. > Cons: > - the change in behavior technically originates external to the > filter, which allows for later filters to "enhance" a previously > applied filter's RET_KILL to kill the entire process, which may > be unexpected. >=20 > Signed-off-by: Kees Cook > --- > include/linux/seccomp.h | 3 ++- > include/uapi/linux/seccomp.h | 3 ++- > kernel/seccomp.c | 12 +++++++++++- > 3 files changed, 15 insertions(+), 3 deletions(-) >=20 > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h > index ecc296c137cd..59d001ba655c 100644 > --- a/include/linux/seccomp.h > +++ b/include/linux/seccomp.h > @@ -3,7 +3,8 @@ > =20 > #include > =20 > -#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC) > +#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC | \ > + SECCOMP_FILTER_FLAG_KILL_PROCESS) > =20 > #ifdef CONFIG_SECCOMP > =20 > diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.= h > index 0f238a43ff1e..4b75d8c297b6 100644 > --- a/include/uapi/linux/seccomp.h > +++ b/include/uapi/linux/seccomp.h > @@ -15,7 +15,8 @@ > #define SECCOMP_SET_MODE_FILTER 1 > =20 > /* Valid flags for SECCOMP_SET_MODE_FILTER */ > -#define SECCOMP_FILTER_FLAG_TSYNC 1 > +#define SECCOMP_FILTER_FLAG_TSYNC 1 > +#define SECCOMP_FILTER_FLAG_KILL_PROCESS 2 > =20 > /* > * All BPF programs must return a 32-bit value. > diff --git a/kernel/seccomp.c b/kernel/seccomp.c > index 8bdcf01379e4..931eb9cbd093 100644 > --- a/kernel/seccomp.c > +++ b/kernel/seccomp.c > @@ -44,6 +44,7 @@ > * is only needed for handling filters shared across tasks. > * @prev: points to a previously installed, or inherited, filter > * @prog: the BPF program to evaluate > + * @kill_process: if true, RET_KILL will kill process rather than thre= ad. > * > * seccomp_filter objects are organized in a tree linked via the @prev= > * pointer. For any task, it appears to be a singly-linked list start= ing > @@ -59,6 +60,7 @@ struct seccomp_filter { > refcount_t usage; > struct seccomp_filter *prev; > struct bpf_prog *prog; > + bool kill_process; Just a reminder to move bool up to be the 2nd member of the struct for improved struct packing. (You already said you were going to this while you were reviewing my logging patches) > }; > =20 > /* Limit any path through the tree to 256KB worth of instructions. */ > @@ -448,6 +450,10 @@ static long seccomp_attach_filter(unsigned int fla= gs, > return ret; > } > =20 > + /* Set process-killing flag, if present. */ > + if (flags & SECCOMP_FILTER_FLAG_KILL_PROCESS) > + filter->kill_process =3D true; > + > /* > * If there is an existing filter, make it the prev and don't drop it= s > * task reference. > @@ -658,7 +664,11 @@ static int __seccomp_filter(int this_syscall, cons= t struct seccomp_data *sd, > seccomp_init_siginfo(&info, this_syscall, data); > do_coredump(&info); > } > - do_exit(SIGSYS); > + /* Kill entire thread group if requested (or went haywire). */ > + if (!match || match->kill_process) I found this conditional to be a little surprising. I would have expected= : if (match && match->kill_process) do_group_exit(SIGSYS); else do_exit(SIGSYS); A resulting NULL match pointer is unexpected but callers of seccomp_run_filters() are expected to handle such a situation so it is possible. Are you preferring to fail closed as much as possible (considering that killing the process is more restrictive than killing the thread)? I don't know the specific situations that would result in the match pointer not being set in seccomp_run_filters() but I'm curious if existing, old userspace code that only expected the thread to be killed could potentially tickle such a situation and result in the entire thread group being unexpectedly killed. FWIW, I like the way the conditional is written and am only thinking out loud about the possible side effects. Side note: I initially expected to see Documentation/userspace-api/seccomp_filter.rst being updated in this patch and then remembered that seccomp(2) isn't documented in that file and, therefore, it isn't trivial to add blurbs about filter flags in there. We should fix that after the dust settles on these two patch sets.= Tyler > + do_group_exit(SIGSYS); > + else > + do_exit(SIGSYS); > } > =20 > unreachable(); >=20 --POIRRD6jiKNdSMKjaO4e7C3pcRGrJdcKB-- --04t6iAspViI67dggpQMuXJxQ1de2qhB8J Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJZiRJ0AAoJENaSAD2qAscKDVUQAIFgkmTy2TYD8PMZXx9qTTsW WRMAt6FKdBYy6OgObcziVVmMTGq+o8ks0JnzG87zBRxBOVQe/bLbGGVLj0nfxOcA ZgDmbydvhC/RtziVhhvze+MPyyOYCirxLvvH5sZCd42lRSMXPRfc4gshkn06+Xy1 uN8Y1DazofR92oz8Gy2wRTDSL0poNCQHQJcUOY4DkEHshmNKALSICs9k4XP+kqju WyBlEOV60T4aaVf3v6KZ+R3+PG1vNU5HhbupPVZ3DIAInj76EFnPfKaYgVLOeU2U Efo8L50ORwUtjm2UnONlxwtIzx5f9T3Gz1M5gVLJB7ESXh4GXkvT3zM7nurjIXvY shSdnetjwUkcVKwIYyqCvnXUyKWf338w3xKljJVfCxJcrS/CZS3hTGuqNZTIDNKq XbeuhD+jzrjeEt8Mj50BCCkXe97C2x6oNhH2jaPxkeACg4Q9gE97tUH7EwlGyQQ1 joUYb+e7erSRqqL7N8JMmSuvd9w6Z43y+sjDSZGFjC5X/9v7a3mw1/+CKtahJbtF jv1igfST1Qb8ZW+uVNgWcaCsm3YfCvhCQvk1UPMR02d6N3beY0AR4ZF38q1UfxN+ YzC6VnJRIf2sm7zz2GhHBu9I319g7cKQ3teJoZf+ez21tIVSQYE9Xy6PsJyCJav9 xg33QQRL+l08n1izLxQN =XmPf -----END PGP SIGNATURE----- --04t6iAspViI67dggpQMuXJxQ1de2qhB8J--