From: Yonghong Song <yhs@fb.com> To: KP Singh <kpsingh@chromium.org> Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "bpf@vger.kernel.org" <bpf@vger.kernel.org>, "linux-security-module@vger.kernel.org" <linux-security-module@vger.kernel.org>, "Alexei Starovoitov" <ast@kernel.org>, "Daniel Borkmann" <daniel@iogearbox.net>, "James Morris" <jmorris@namei.org>, "Kees Cook" <keescook@chromium.org>, "Thomas Garnier" <thgarnie@chromium.org>, "Michael Halcrow" <mhalcrow@google.com>, "Paul Turner" <pjt@google.com>, "Brendan Gregg" <brendan.d.gregg@gmail.com>, "Jann Horn" <jannh@google.com>, "Matthew Garrett" <mjg59@google.com>, "Christian Brauner" <christian@brauner.io>, "Mickaël Salaün" <mic@digikod.net>, "Florent Revest" <revest@chromium.org>, "Martin Lau" <kafai@fb.com>, "Song Liu" <songliubraving@fb.com>, "Serge E. Hallyn" <serge@hallyn.com>, "Mauro Carvalho Chehab" <mchehab+samsung@kernel.org>, "David S. Miller" <davem@davemloft.net>, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>, "Nicolas Ferre" <nicolas.ferre@microchip.com>, "Stanislav Fomichev" <sdf@google.com>, "Quentin Monnet" <quentin.monnet@netronome.com>, "Andrey Ignatov" <rdna@fb.com>, "Joe Stringer" <joe@wand.net.nz> Subject: Re: [RFC v1 14/14] krsi: Pin arg pages only when needed Date: Sun, 15 Sep 2019 19:45:19 +0000 [thread overview] Message-ID: <cde5ca45-7bff-7eb9-2a70-98603cc420d0@fb.com> (raw) In-Reply-To: <20190915014008.GA19558@chromium.org> On 9/15/19 2:40 AM, KP Singh wrote: > On 15-Sep 00:33, Yonghong Song wrote: >> >> >> On 9/10/19 12:55 PM, KP Singh wrote: >>> From: KP Singh <kpsingh@google.com> >>> >>> Adds a callback which is called when a new program is attached >>> to a hook. The callback registered by the process_exection hook >>> checks if a program that has calls to a helper that requires pages to >>> be pinned (eg. krsi_get_env_var). >>> >>> Signed-off-by: KP Singh <kpsingh@google.com> >>> --- >>> include/linux/krsi.h | 1 + >>> security/krsi/include/hooks.h | 5 ++- >>> security/krsi/include/krsi_init.h | 7 ++++ >>> security/krsi/krsi.c | 62 ++++++++++++++++++++++++++++--- >>> security/krsi/ops.c | 10 ++++- >>> 5 files changed, 77 insertions(+), 8 deletions(-) >>> >>> diff --git a/include/linux/krsi.h b/include/linux/krsi.h >>> index c7d1790d0c1f..e443d0309764 100644 >>> --- a/include/linux/krsi.h >>> +++ b/include/linux/krsi.h >>> @@ -7,6 +7,7 @@ >>> >>> #ifdef CONFIG_SECURITY_KRSI >>> int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog); >>> +extern const struct bpf_func_proto krsi_get_env_var_proto; >>> #else >>> static inline int krsi_prog_attach(const union bpf_attr *attr, >>> struct bpf_prog *prog) >>> diff --git a/security/krsi/include/hooks.h b/security/krsi/include/hooks.h >>> index e070c452b5de..38293125ff99 100644 >>> --- a/security/krsi/include/hooks.h >>> +++ b/security/krsi/include/hooks.h >>> @@ -8,7 +8,7 @@ >>> * >>> * Format: >>> * >>> - * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN) >>> + * KRSI_HOOK_INIT(TYPE, NAME, LSM_HOOK, KRSI_HOOK_FN, CALLBACK) >>> * >>> * KRSI adds one layer of indirection between the name of the hook and the name >>> * it exposes to the userspace in Security FS to prevent the userspace from >>> @@ -18,4 +18,5 @@ >>> KRSI_HOOK_INIT(PROCESS_EXECUTION, >>> process_execution, >>> bprm_check_security, >>> - krsi_process_execution) >>> + krsi_process_execution, >>> + krsi_process_execution_cb) >>> diff --git a/security/krsi/include/krsi_init.h b/security/krsi/include/krsi_init.h >>> index 6152847c3b08..99801d5b273a 100644 >>> --- a/security/krsi/include/krsi_init.h >>> +++ b/security/krsi/include/krsi_init.h >>> @@ -31,6 +31,8 @@ struct krsi_ctx { >>> }; >>> }; >>> >>> +typedef int (*krsi_prog_attach_t) (struct bpf_prog_array *); >>> + >>> /* >>> * The LSM creates one file per hook. >>> * >>> @@ -61,6 +63,11 @@ struct krsi_hook { >>> * The eBPF programs that are attached to this hook. >>> */ >>> struct bpf_prog_array __rcu *progs; >>> + /* >>> + * The attach callback is called before a new program is attached >>> + * to the hook and is passed the updated bpf_prog_array as an argument. >>> + */ >>> + krsi_prog_attach_t attach_callback; >>> }; >>> >>> extern struct krsi_hook krsi_hooks_list[]; >>> diff --git a/security/krsi/krsi.c b/security/krsi/krsi.c >>> index 00a7150c1b22..a4443d7aa150 100644 >>> --- a/security/krsi/krsi.c >>> +++ b/security/krsi/krsi.c >>> @@ -5,15 +5,65 @@ >>> #include <linux/bpf.h> >>> #include <linux/binfmts.h> >>> #include <linux/highmem.h> >>> +#include <linux/krsi.h> >>> #include <linux/mm.h> >>> >>> #include "krsi_init.h" >>> >>> +/* >>> + * need_arg_pages is only updated in bprm_check_security_cb >>> + * when a mutex on krsi_hook for bprm_check_security is already >>> + * held. need_arg_pages avoids pinning pages when no program >>> + * that needs them is attached to the hook. >>> + */ >>> +static bool need_arg_pages; >>> + >>> +/* >>> + * Checks if the instruction is a BPF_CALL to an eBPF helper located >>> + * at the given address. >>> + */ >>> +static inline bool bpf_is_call_to_func(struct bpf_insn *insn, >>> + void *func_addr) >>> +{ >>> + u8 opcode = BPF_OP(insn->code); >>> + >>> + if (opcode != BPF_CALL) >>> + return false; >>> + >>> + if (insn->src_reg == BPF_PSEUDO_CALL) >>> + return false; >>> + >>> + /* >>> + * The BPF verifier updates the value of insn->imm from the >>> + * enum bpf_func_id to the offset of the address of helper >>> + * from the __bpf_call_base. >>> + */ >>> + return __bpf_call_base + insn->imm == func_addr; >> >> In how many cases, krsi program does not have krsi_get_env_var() helper? > > It depends, if the user does not choose to use log environment > variables or use the the value as a part of their MAC policy, the > pinning of the pages is not needed. Thanks. I just want to know whether we want to optimize such cases. I am not a security expert, so I am okay with whatever decision you made. > > Also, the pinning is needed since eBPF helpers cannot run a non-atomic > context. It would not be needed if "sleepable eBPF" becomes a thing. Indeed. 'sleepable BPF' might be also a good thing for realtime linux. Some works need to address bpf area spin lock, per cpu variables, etc. before that can happen. > > - KP > >> >>> +} >>> + >>> +static int krsi_process_execution_cb(struct bpf_prog_array *array) >>> +{ >>> + struct bpf_prog_array_item *item = array->items; >>> + struct bpf_prog *p; >>> + const struct bpf_func_proto *proto = &krsi_get_env_var_proto; >>> + int i; >>> + >>> + while ((p = READ_ONCE(item->prog))) { >>> + for (i = 0; i < p->len; i++) { >>> + if (bpf_is_call_to_func(&p->insnsi[i], proto->func)) >>> + need_arg_pages = true; >>> + } >>> + item++; >>> + } >>> + return 0; >>> +} >>> + >>> struct krsi_hook krsi_hooks_list[] = { >>> - #define KRSI_HOOK_INIT(TYPE, NAME, H, I) \ >>> + #define KRSI_HOOK_INIT(TYPE, NAME, H, I, CB) \ >>> [TYPE] = { \ >>> .h_type = TYPE, \ >>> .name = #NAME, \ >>> + .attach_callback = CB, \ >>> }, >>> #include "hooks.h" >>> #undef KRSI_HOOK_INIT >>> @@ -75,9 +125,11 @@ static int krsi_process_execution(struct linux_binprm *bprm) >>> .bprm = bprm, >>> }; >>> >>> - ret = pin_arg_pages(&ctx.bprm_ctx); >>> - if (ret < 0) >>> - goto out_arg_pages; >>> + if (READ_ONCE(need_arg_pages)) { >>> + ret = pin_arg_pages(&ctx.bprm_ctx); >>> + if (ret < 0) >>> + goto out_arg_pages; >>> + } >>> >>> ret = krsi_run_progs(PROCESS_EXECUTION, &ctx); >>> kfree(ctx.bprm_ctx.arg_pages); >>> @@ -87,7 +139,7 @@ static int krsi_process_execution(struct linux_binprm *bprm) >>> } >>> >>> static struct security_hook_list krsi_hooks[] __lsm_ro_after_init = { >>> - #define KRSI_HOOK_INIT(T, N, HOOK, IMPL) LSM_HOOK_INIT(HOOK, IMPL), >>> + #define KRSI_HOOK_INIT(T, N, HOOK, IMPL, CB) LSM_HOOK_INIT(HOOK, IMPL), >>> #include "hooks.h" >>> #undef KRSI_HOOK_INIT >>> }; >>> diff --git a/security/krsi/ops.c b/security/krsi/ops.c >>> index 1db94dfaac15..2de682371eff 100644 >>> --- a/security/krsi/ops.c >>> +++ b/security/krsi/ops.c >>> @@ -139,6 +139,14 @@ int krsi_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog) >>> goto unlock; >>> } >>> >>> + if (h->attach_callback) { >>> + ret = h->attach_callback(new_array); >>> + if (ret < 0) { >>> + bpf_prog_array_free(new_array); >>> + goto unlock; >>> + } >>> + } >>> + >>> rcu_assign_pointer(h->progs, new_array); >>> bpf_prog_array_free(old_array); >>> >>> @@ -278,7 +286,7 @@ BPF_CALL_5(krsi_get_env_var, struct krsi_ctx *, ctx, char *, name, u32, n_size, >>> return get_env_var(ctx, name, dest, n_size, size); >>> } >>> >>> -static const struct bpf_func_proto krsi_get_env_var_proto = { >>> +const struct bpf_func_proto krsi_get_env_var_proto = { >>> .func = krsi_get_env_var, >>> .gpl_only = true, >>> .ret_type = RET_INTEGER, >>>
prev parent reply other threads:[~2019-09-15 19:46 UTC|newest] Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-09-10 11:55 [RFC v1 00/14] Kernel Runtime Security Instrumentation KP Singh 2019-09-10 11:55 ` [RFC v1 01/14] krsi: Add a skeleton and config options for the KRSI LSM KP Singh 2019-09-10 11:55 ` [RFC v1 02/14] krsi: Introduce types for KRSI eBPF KP Singh 2019-09-10 11:55 ` [RFC v1 03/14] bpf: krsi: sync BPF UAPI header with tools KP Singh 2019-09-10 11:55 ` [RFC v1 04/14] krsi: Add support in libbpf for BPF_PROG_TYPE_KRSI KP Singh 2019-09-14 16:09 ` Yonghong Song 2019-09-10 11:55 ` [RFC v1 05/14] krsi: Initialize KRSI hooks and create files in securityfs KP Singh 2019-09-14 16:26 ` Yonghong Song 2019-09-10 11:55 ` [RFC v1 06/14] krsi: Implement eBPF operations, attachment and execution KP Singh 2019-09-14 16:56 ` Yonghong Song 2019-09-15 0:37 ` Yonghong Song 2019-09-10 11:55 ` [RFC v1 07/14] krsi: Check for premissions on eBPF attachment KP Singh 2019-09-10 11:55 ` [RFC v1 08/14] krsi: Show attached program names in hook read handler KP Singh 2019-09-10 11:55 ` [RFC v1 09/14] krsi: Add a helper function for bpf_perf_event_output KP Singh 2019-09-14 18:23 ` Yonghong Song 2019-09-10 11:55 ` [RFC v1 10/14] krsi: Handle attachment of the same program KP Singh 2019-09-10 11:55 ` [RFC v1 11/14] krsi: Pin argument pages in bprm_check_security hook KP Singh 2019-09-10 11:55 ` [RFC v1 12/14] krsi: Add an eBPF helper function to get the value of an env variable KP Singh 2019-09-15 0:16 ` Yonghong Song 2019-09-16 13:00 ` KP Singh 2019-09-17 16:58 ` Yonghong Song 2019-09-17 19:36 ` KP Singh 2019-09-10 11:55 ` [RFC v1 13/14] krsi: Provide an example to read and log environment variables KP Singh 2019-09-15 0:24 ` Yonghong Song 2019-09-10 11:55 ` [RFC v1 14/14] krsi: Pin arg pages only when needed KP Singh 2019-09-15 0:33 ` Yonghong Song 2019-09-15 1:40 ` KP Singh 2019-09-15 19:45 ` Yonghong Song [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=cde5ca45-7bff-7eb9-2a70-98603cc420d0@fb.com \ --to=yhs@fb.com \ --cc=ast@kernel.org \ --cc=bpf@vger.kernel.org \ --cc=brendan.d.gregg@gmail.com \ --cc=christian@brauner.io \ --cc=daniel@iogearbox.net \ --cc=davem@davemloft.net \ --cc=gregkh@linuxfoundation.org \ --cc=jannh@google.com \ --cc=jmorris@namei.org \ --cc=joe@wand.net.nz \ --cc=kafai@fb.com \ --cc=keescook@chromium.org \ --cc=kpsingh@chromium.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-security-module@vger.kernel.org \ --cc=mchehab+samsung@kernel.org \ --cc=mhalcrow@google.com \ --cc=mic@digikod.net \ --cc=mjg59@google.com \ --cc=nicolas.ferre@microchip.com \ --cc=pjt@google.com \ --cc=quentin.monnet@netronome.com \ --cc=rdna@fb.com \ --cc=revest@chromium.org \ --cc=sdf@google.com \ --cc=serge@hallyn.com \ --cc=songliubraving@fb.com \ --cc=thgarnie@chromium.org \ --subject='Re: [RFC v1 14/14] krsi: Pin arg pages only when needed' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).