Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752016AbdHHCFI (ORCPT ); Mon, 7 Aug 2017 22:05:08 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:36045 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751639AbdHHCFG (ORCPT ); Mon, 7 Aug 2017 22:05:06 -0400 Subject: Re: [PATCH v2 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: <1502157548-111843-1-git-send-email-keescook@chromium.org> <1502157548-111843-3-git-send-email-keescook@chromium.org> From: Tyler Hicks Message-ID: <81683131-c0cc-9dd4-0a68-174b392b3ec3@canonical.com> Date: Mon, 7 Aug 2017 21:04:59 -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: <1502157548-111843-3-git-send-email-keescook@chromium.org> Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="pJgtX6EKMat5X98IDChoKk1blO16xSwQ0" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7265 Lines: 199 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --pJgtX6EKMat5X98IDChoKk1blO16xSwQ0 Content-Type: multipart/mixed; boundary="QrpFg7kclWARF2RU3saOHNGO7Q6iujf1j"; 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: <81683131-c0cc-9dd4-0a68-174b392b3ec3@canonical.com> Subject: Re: [PATCH v2 2/4] seccomp: Add SECCOMP_FILTER_FLAG_KILL_PROCESS References: <1502157548-111843-1-git-send-email-keescook@chromium.org> <1502157548-111843-3-git-send-email-keescook@chromium.org> In-Reply-To: <1502157548-111843-3-git-send-email-keescook@chromium.org> --QrpFg7kclWARF2RU3saOHNGO7Q6iujf1j Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 08/07/2017 08:59 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). >=20 > - 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 v2 of these patches all look good to me. Reviewed-by: Tyler Hicks Thanks! Tyler > --- > include/linux/seccomp.h | 3 ++- > include/uapi/linux/seccomp.h | 3 ++- > kernel/seccomp.c | 22 +++++++++++++++++++++- > 3 files changed, 25 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 1f3347fc2605..297f8bfc3b72 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 > @@ -57,6 +58,7 @@ > */ > struct seccomp_filter { > refcount_t usage; > + bool kill_process; > struct seccomp_filter *prev; > struct bpf_prog *prog; > }; > @@ -450,6 +452,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. > @@ -665,7 +671,21 @@ 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); > + /* > + * The only way match can be NULL here is if something > + * went very wrong in seccomp_run_filters() (e.g. a NULL > + * filter list in struct seccomp) and the return action > + * falls back to failing closed. In this case, take the > + * strongest possible action. > + * > + * If we get here with match->kill_process set, we need > + * to kill the entire thread group. Otherwise, kill only > + * the offending thread. > + */ > + if (!match || match->kill_process) > + do_group_exit(SIGSYS); > + else > + do_exit(SIGSYS); > } > =20 > unreachable(); >=20 --QrpFg7kclWARF2RU3saOHNGO7Q6iujf1j-- --pJgtX6EKMat5X98IDChoKk1blO16xSwQ0 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJZiRxLAAoJENaSAD2qAscKDLYP/2A7ah6Quyhri8OJLeZ7syBL fxOYM+A2y11iKjOXxDxOKvKNjqtr3c1iADIxiLQ1DdIJgYyTPupy+bp+XhX5QyIV EqdduEQquKgsr8XBQvWsB6Zr39OwNDc2w78cWy3AsTqT2gLiApYB7NNU7nkvzl5y pYKCxerAL+sYgBbMmb5tKECKzOodzqiUgr6ZzfqsVXAygiuFh9GvBtBArUH460dI /Vvm/zSngKDaRzTpCxn/VZ5L7+6+e/Jp9L3+nUarvkvyt8y2Pk2bZo+b/44HTohy E3DYzlGSBZb1TuFrKwXvMnIla4NNjr2WWKQuN0+l4z3mL9agsZ7v4PpoeXgh2LkV Nmz5G9yb8luvnoO/bYlx88y7BKrDSmtQIUFMRgCHZ22B1xTmtzpFOrR9+N+0kxJB WhdjqKZ8kKAeAQ3FFgjFNNEle5DyyveSgVLuz0/jMS5+HiebCrbqLATJKqS6bpzD MaMOTvXUXrBtB1Dms58hvVPD7wGtBzKHX4dal3GpV5gHJcY5KcWKHm8EoTKrbrk+ uqjIbB2jzc2y6kyM+CiVSd466/nDXlmnEVhkyHkdQ/r/8fiD3rtv/xTuQz+EJmnK 2l4U/GtAB6iB0mIKlKM9AETXVIim+h9ui+IIkGIpxWGCU9S9uhz1IE5FizxKss4v 3IRarmE8LNoEuO2wUU0g =xve9 -----END PGP SIGNATURE----- --pJgtX6EKMat5X98IDChoKk1blO16xSwQ0--