Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752064AbdHHBbi (ORCPT ); Mon, 7 Aug 2017 21:31:38 -0400 Received: from youngberry.canonical.com ([91.189.89.112]:35354 "EHLO youngberry.canonical.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751891AbdHHBbh (ORCPT ); Mon, 7 Aug 2017 21:31:37 -0400 Subject: Re: [PATCH 1/4] seccomp: Provide matching filter for introspection 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 References: <1501730353-46840-1-git-send-email-keescook@chromium.org> <1501730353-46840-2-git-send-email-keescook@chromium.org> Message-ID: <2fe27623-c9fe-6771-d97f-5a0f787cb280@canonical.com> Date: Mon, 7 Aug 2017 20:31:29 -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: Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="hC8qke7Wofd31mC8MclLCtJJEmdX5loML" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5694 Lines: 158 This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --hC8qke7Wofd31mC8MclLCtJJEmdX5loML Content-Type: multipart/mixed; boundary="Aiqmq6DTrMbj0u4eo52HRTPfWhdmU9nxs"; 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: <2fe27623-c9fe-6771-d97f-5a0f787cb280@canonical.com> Subject: Re: [PATCH 1/4] seccomp: Provide matching filter for introspection References: <1501730353-46840-1-git-send-email-keescook@chromium.org> <1501730353-46840-2-git-send-email-keescook@chromium.org> In-Reply-To: --Aiqmq6DTrMbj0u4eo52HRTPfWhdmU9nxs Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable On 08/07/2017 08:03 PM, Tyler Hicks wrote: > On 08/02/2017 10:19 PM, Kees Cook wrote: >> Both the upcoming logging improvements and changes to RET_KILL will ne= ed >> to know which filter a given seccomp return value originated from. In >> order to delay logic processing of result until after the seccomp loop= , >> this adds a single pointer assignment on matches. This will allow both= >> log and RET_KILL logic to work off the filter rather than doing more >> expensive tests inside the time-critical run_filters loop. >> >> Running tight cycles of getpid() with filters attached shows no measur= able >> difference in speed. >> >> Suggested-by: Tyler Hicks >> Signed-off-by: Kees Cook >> --- >> kernel/seccomp.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/kernel/seccomp.c b/kernel/seccomp.c >> index 98b59b5db90b..8bdcf01379e4 100644 >> --- a/kernel/seccomp.c >> +++ b/kernel/seccomp.c >> @@ -171,10 +171,12 @@ static int seccomp_check_filter(struct sock_filt= er *filter, unsigned int flen) >> /** >> * seccomp_run_filters - evaluates all seccomp filters against @sd >> * @sd: optional seccomp data to be passed to filters >> + * @match: stores struct seccomp_filter that resulted in the return v= alue Thinking just a bit more about this patch, can you document that @match may be NULL upon return? Tyler >> * >> * Returns valid seccomp BPF response codes. >> */ >> -static u32 seccomp_run_filters(const struct seccomp_data *sd) >> +static u32 seccomp_run_filters(const struct seccomp_data *sd, >> + struct seccomp_filter **match) >> { >> struct seccomp_data sd_local; >> u32 ret =3D SECCOMP_RET_ALLOW; >=20 > My version of this patch initialized *match to f here. The reason I did= > that is because if BPF_PROG_RUN() returns RET_ALLOW for all > filters, I didn't want *match to remain NULL when seccomp_run_filters()= > returns. FILTER_FLAG_LOG nor FILTER_FLAG_KILL_PROCESS would be affected= > by this because they don't care about RET_ALLOW actions but there could= > conceivably be a filter flag in the future that cares about RET_ALLOW > and not initializing *match to the first filter could result in a laten= t > bug for that filter flag. >=20 > I'm fine with not adding the initialization since this is a hot path an= d > it doesn't help any of the currently existing/planned filter flags but = I > wanted to at least mention it. >=20 > Reviewed-by: Tyler Hicks >=20 > Tyler >=20 >> @@ -198,8 +200,10 @@ static u32 seccomp_run_filters(const struct secco= mp_data *sd) >> for (; f; f =3D f->prev) { >> u32 cur_ret =3D BPF_PROG_RUN(f->prog, sd); >> =20 >> - if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION)) >> + if ((cur_ret & SECCOMP_RET_ACTION) < (ret & SECCOMP_RET_ACTION)) { >> ret =3D cur_ret; >> + *match =3D f; >> + } >> } >> return ret; >> } >> @@ -566,6 +570,7 @@ static int __seccomp_filter(int this_syscall, cons= t struct seccomp_data *sd, >> const bool recheck_after_trace) >> { >> u32 filter_ret, action; >> + struct seccomp_filter *match =3D NULL; >> int data; >> =20 >> /* >> @@ -574,7 +579,7 @@ static int __seccomp_filter(int this_syscall, cons= t struct seccomp_data *sd, >> */ >> rmb(); >> =20 >> - filter_ret =3D seccomp_run_filters(sd); >> + filter_ret =3D seccomp_run_filters(sd, &match); >> data =3D filter_ret & SECCOMP_RET_DATA; >> action =3D filter_ret & SECCOMP_RET_ACTION; >> =20 >> >=20 >=20 --Aiqmq6DTrMbj0u4eo52HRTPfWhdmU9nxs-- --hC8qke7Wofd31mC8MclLCtJJEmdX5loML Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCgAGBQJZiRRxAAoJENaSAD2qAscKfjYP/A5b7Ee5yBsDYKTGHRZ39K77 Oo+uft0iCFA26uS9C6TGExKG7594070JL6Dmb7MJgXwx9aCUJZs9QI8UzfR9/4Qn Gs/qpreoSKIVyIvsqmXJhIncPVPxCg1FjOIqTCTV1M3r3Pr7A0reFRGbq11Eh1os z0K0SObOkKi3S34e9Hp5fRotrfXSF+IjHKa4v9o4QLP8Pzu9ncj4QXYKLr6MyHGT AkLSHjy/ob0/JXRQXsn6RoLG/M+qMMNkzuzxFT7D/lJij9+uA9O6v8M3VVJ4LaY2 udzTjcTtdIB5iKRnN7zvirGAXBWSvfN3niw1gd8NhyTScmvVcM5LFj+J+6zmyeqT J+ORKmz4tv2O1cnVTS+NpaLlxWqtfwIcLwcq23bK4WjVyZTVhTcZ9DjJ0qAiXobt kmt0fzdHHjp6dNrSJ5nS1pBaNXYFrU7Eka4NOAR5Z2RBMK4AKUfB6EV/TlLRD8Wo wvMBHhn7orqJCRh5RCQrg7YoBY/rVkkvN4g3CvK+Jz4BxvkgZJWt5my2Rraf6d3/ AFwkJTftXsnTXBIVFL/aOgMBWS6XW5KHRqP0tFN0XGfHsGic7dHuhxSvgG0Vj2Rx DOs53qn4qFcUBD+60iL5hQFEeXRyvNZqCGwFTRZco9lLS7iI5gKLGkLDjr0IVetQ gXAwO/tz8lTGA0Fe+8mi =edjd -----END PGP SIGNATURE----- --hC8qke7Wofd31mC8MclLCtJJEmdX5loML--