From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.3 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 01606C2BA83 for ; Wed, 12 Feb 2020 13:27:27 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id C952321569 for ; Wed, 12 Feb 2020 13:27:27 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727662AbgBLN11 (ORCPT ); Wed, 12 Feb 2020 08:27:27 -0500 Received: from www62.your-server.de ([213.133.104.62]:57368 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725887AbgBLN11 (ORCPT ); Wed, 12 Feb 2020 08:27:27 -0500 Received: from sslproxy03.your-server.de ([88.198.220.132]) by www62.your-server.de with esmtpsa (TLSv1.2:DHE-RSA-AES256-GCM-SHA384:256) (Exim 4.89_1) (envelope-from ) id 1j1s2v-0004et-Je; Wed, 12 Feb 2020 14:27:18 +0100 Received: from [2001:1620:665:0:5795:5b0a:e5d5:5944] (helo=linux-3.fritz.box) by sslproxy03.your-server.de with esmtpsa (TLSv1.3:TLS_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1j1s2u-000X31-Sa; Wed, 12 Feb 2020 14:27:16 +0100 Subject: Re: BPF LSM and fexit [was: [PATCH bpf-next v3 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM] To: Alexei Starovoitov Cc: Jann Horn , KP Singh , kernel list , bpf , linux-security-module , Brendan Jackman , Florent Revest , Thomas Garnier , Alexei Starovoitov , James Morris , Kees Cook , Thomas Garnier , Michael Halcrow , Paul Turner , Brendan Gregg , Matthew Garrett , Christian Brauner , =?UTF-8?Q?Micka=c3=abl_Sala=c3=bcn?= , Florent Revest , Brendan Jackman , "Serge E. Hallyn" , Mauro Carvalho Chehab , "David S. Miller" , Greg Kroah-Hartman , Kernel Team References: <20200211124334.GA96694@google.com> <20200211175825.szxaqaepqfbd2wmg@ast-mbp> <20200211190943.sysdbz2zuz5666nq@ast-mbp> <20200211201039.om6xqoscfle7bguz@ast-mbp> <20200211213819.j4ltrjjkuywihpnv@ast-mbp> <1cd10710-a81b-8f9b-696d-aa40b0a67225@iogearbox.net> <20200212024542.gdsafhvqykucdp4h@ast-mbp> From: Daniel Borkmann Message-ID: Date: Wed, 12 Feb 2020 14:27:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 MIME-Version: 1.0 In-Reply-To: <20200212024542.gdsafhvqykucdp4h@ast-mbp> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Authenticated-Sender: daniel@iogearbox.net X-Virus-Scanned: Clear (ClamAV 0.102.1/25721/Wed Feb 12 06:24:38 2020) Sender: bpf-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org On 2/12/20 3:45 AM, Alexei Starovoitov wrote: > On Wed, Feb 12, 2020 at 01:09:07AM +0100, Daniel Borkmann wrote: >> >> Another approach could be to have a special nop inside call_int_hook() >> macro which would then get patched to avoid these situations. Somewhat >> similar like static keys where it could be defined anywhere in text but >> with updating of call_int_hook()'s RC for the verdict. > > Sounds nice in theory. I couldn't quite picture how that would look > in the code, so I hacked: > diff --git a/security/security.c b/security/security.c > index 565bc9b67276..ce4bc1e5e26c 100644 > --- a/security/security.c > +++ b/security/security.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > #define MAX_LSM_EVM_XATTR 2 > > @@ -678,12 +679,26 @@ static void __init lsm_early_task(struct task_struct *task) > * This is a hook that returns a value. > */ > > +#define LSM_HOOK_NAME(FUNC) \ > + DEFINE_STATIC_KEY_FALSE(bpf_lsm_key_##FUNC); > +#include > +#undef LSM_HOOK_NAME > +__diag_push(); > +__diag_ignore(GCC, 8, "-Wstrict-prototypes", ""); > +#define LSM_HOOK_NAME(FUNC) \ > + int bpf_lsm_call_##FUNC() {return 0;} > +#include > +#undef LSM_HOOK_NAME > +__diag_pop(); > + > #define call_void_hook(FUNC, ...) \ > do { \ > struct security_hook_list *P; \ > \ > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \ > P->hook.FUNC(__VA_ARGS__); \ > + if (static_branch_unlikely(&bpf_lsm_key_##FUNC)) \ > + (void)bpf_lsm_call_##FUNC(__VA_ARGS__); \ > } while (0) > > #define call_int_hook(FUNC, IRC, ...) ({ \ > @@ -696,6 +711,8 @@ static void __init lsm_early_task(struct task_struct *task) > if (RC != 0) \ > break; \ > } \ > + if (RC == IRC && static_branch_unlikely(&bpf_lsm_key_##FUNC)) \ > + RC = bpf_lsm_call_##FUNC(__VA_ARGS__); \ Nit: the `RC == IRC` test could be moved behind the static_branch_unlikely() so that it would be bypassed when not enabled. > } while (0); \ > RC; \ > }) > > The assembly looks good from correctness and performance points. > union security_list_options can be split into lsm_hook_names.h too > to avoid __diag_ignore. Is that what you have in mind? > I don't see how one can improve call_int_hook() macro without > full refactoring of linux/lsm_hooks.h > imo static_key doesn't have to be there in the first set. We can add this > optimization later. Yes, like the above diff looks good, and then we'd dynamically attach the program at bpf_lsm_call_##FUNC()'s fexit hook for a direct jump, so all the security_blah() internals could stay as-is which then might also address Jann's concerns wrt concrete annotation as well as potential locking changes inside security_blah(). Agree that patching out via static key could be optional but since you were talking about avoiding indirect jumps.. Thanks, Daniel