* Re: [PATCH bpf-next v2 04/10] bpf: lsm: Add mutable hooks list for the BPF LSM
From: KP Singh @ 2020-01-16 10:19 UTC (permalink / raw)
To: Casey Schaufler; +Cc: KP Singh, bpf, Linux Security Module list
In-Reply-To: <5793e9a8-e9cf-dd2d-261d-61f533cca20c@schaufler-ca.com>
On 15-Jan 22:33, Casey Schaufler wrote:
> On 1/15/2020 9:13 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > - The list of hooks registered by an LSM is currently immutable as they
> > are declared with __lsm_ro_after_init and they are attached to a
> > security_hook_heads struct.
> > - For the BPF LSM we need to de/register the hooks at runtime. Making
> > the existing security_hook_heads mutable broadens an
> > attack vector, so a separate security_hook_heads is added for only
> > those that ~must~ be mutable.
> > - These mutable hooks are run only after all the static hooks have
> > successfully executed.
> >
> > This is based on the ideas discussed in:
> >
> > https://lore.kernel.org/lkml/20180408065916.GA2832@ircssh-2.c.rugged-nimbus-611.internal
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > MAINTAINERS | 1 +
> > include/linux/bpf_lsm.h | 71 +++++++++++++++++++++++++++++++++++++++++
> > security/bpf/Kconfig | 1 +
> > security/bpf/Makefile | 2 +-
> > security/bpf/hooks.c | 20 ++++++++++++
> > security/bpf/lsm.c | 9 +++++-
> > security/security.c | 24 +++++++-------
> > 7 files changed, 115 insertions(+), 13 deletions(-)
> > create mode 100644 include/linux/bpf_lsm.h
> > create mode 100644 security/bpf/hooks.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 0941f478cfa5..02d7e05e9b75 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3209,6 +3209,7 @@ L: linux-security-module@vger.kernel.org
> > L: bpf@vger.kernel.org
> > S: Maintained
> > F: security/bpf/
> > +F: include/linux/bpf_lsm.h
> >
> > BROADCOM B44 10/100 ETHERNET DRIVER
> > M: Michael Chan <michael.chan@broadcom.com>
> > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > new file mode 100644
> > index 000000000000..9883cf25241c
> > --- /dev/null
> > +++ b/include/linux/bpf_lsm.h
> > @@ -0,0 +1,71 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#ifndef _LINUX_BPF_LSM_H
> > +#define _LINUX_BPF_LSM_H
> > +
> > +#include <linux/bpf.h>
> > +#include <linux/lsm_hooks.h>
> > +
> > +#ifdef CONFIG_SECURITY_BPF
> > +
> > +/* Mutable hooks defined at runtime and executed after all the statically
> > + * define LSM hooks.
> > + */
> > +extern struct security_hook_heads bpf_lsm_hook_heads;
> > +
> > +int bpf_lsm_srcu_read_lock(void);
> > +void bpf_lsm_srcu_read_unlock(int idx);
> > +
> > +#define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...) \
> > + do { \
> > + struct security_hook_list *P; \
> > + int _idx; \
> > + \
> > + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> > + break; \
> > + \
> > + _idx = bpf_lsm_srcu_read_lock(); \
> > + hlist_for_each_entry(P, &bpf_lsm_hook_heads.FUNC, list) \
> > + P->hook.FUNC(__VA_ARGS__); \
> > + bpf_lsm_srcu_read_unlock(_idx); \
> > + } while (0)
> > +
> > +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) ({ \
> > + do { \
> > + struct security_hook_list *P; \
> > + int _idx; \
> > + \
> > + if (hlist_empty(&bpf_lsm_hook_heads.FUNC)) \
> > + break; \
> > + \
> > + _idx = bpf_lsm_srcu_read_lock(); \
> > + \
> > + hlist_for_each_entry(P, \
> > + &bpf_lsm_hook_heads.FUNC, list) { \
> > + RC = P->hook.FUNC(__VA_ARGS__); \
> > + if (RC && IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE)) \
> > + break; \
> > + } \
> > + bpf_lsm_srcu_read_unlock(_idx); \
> > + } while (0); \
> > + IS_ENABLED(CONFIG_SECURITY_BPF_ENFORCE) ? RC : 0; \
> > +})
> > +
> > +#else /* !CONFIG_SECURITY_BPF */
> > +
> > +#define CALL_BPF_LSM_INT_HOOKS(RC, FUNC, ...) (RC)
> > +#define CALL_BPF_LSM_VOID_HOOKS(...)
> > +
> > +static inline int bpf_lsm_srcu_read_lock(void)
> > +{
> > + return 0;
> > +}
> > +static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> > +
> > +#endif /* CONFIG_SECURITY_BPF */
> > +
> > +#endif /* _LINUX_BPF_LSM_H */
> > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> > index a5f6c67ae526..595e4ad597ae 100644
> > --- a/security/bpf/Kconfig
> > +++ b/security/bpf/Kconfig
> > @@ -6,6 +6,7 @@ config SECURITY_BPF
> > bool "BPF-based MAC and audit policy"
> > depends on SECURITY
> > depends on BPF_SYSCALL
> > + depends on SRCU
> > help
> > This enables instrumentation of the security hooks with
> > eBPF programs.
> > diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> > index c78a8a056e7e..c526927c337d 100644
> > --- a/security/bpf/Makefile
> > +++ b/security/bpf/Makefile
> > @@ -2,4 +2,4 @@
> > #
> > # Copyright 2019 Google LLC.
> >
> > -obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o
> > +obj-$(CONFIG_SECURITY_BPF) := lsm.o ops.o hooks.o
> > diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> > new file mode 100644
> > index 000000000000..b123d9cb4cd4
> > --- /dev/null
> > +++ b/security/bpf/hooks.c
> > @@ -0,0 +1,20 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#include <linux/bpf_lsm.h>
> > +#include <linux/srcu.h>
> > +
> > +DEFINE_STATIC_SRCU(security_hook_srcu);
> > +
> > +int bpf_lsm_srcu_read_lock(void)
> > +{
> > + return srcu_read_lock(&security_hook_srcu);
> > +}
> > +
> > +void bpf_lsm_srcu_read_unlock(int idx)
> > +{
> > + return srcu_read_unlock(&security_hook_srcu, idx);
> > +}
> > diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> > index 5c5c14f990ce..d4ea6aa9ddb8 100644
> > --- a/security/bpf/lsm.c
> > +++ b/security/bpf/lsm.c
> > @@ -4,14 +4,21 @@
> > * Copyright 2019 Google LLC.
> > */
> >
> > +#include <linux/bpf_lsm.h>
> > #include <linux/lsm_hooks.h>
> >
> > /* This is only for internal hooks, always statically shipped as part of the
> > - * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> > + * BPF LSM. Statically defined hooks are appended to the security_hook_heads
> > * which is common for LSMs and R/O after init.
> > */
> > static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
> >
> > +/* Security hooks registered dynamically by the BPF LSM and must be accessed
> > + * by holding bpf_lsm_srcu_read_lock and bpf_lsm_srcu_read_unlock. The mutable
> > + * hooks dynamically allocated by the BPF LSM are appeneded here.
> > + */
> > +struct security_hook_heads bpf_lsm_hook_heads;
> > +
> > static int __init lsm_init(void)
> > {
> > security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> > diff --git a/security/security.c b/security/security.c
> > index cd2d18d2d279..4a2eb4c089b2 100644
> > --- a/security/security.c
> > +++ b/security/security.c
> > @@ -27,6 +27,7 @@
> > #include <linux/backing-dev.h>
> > #include <linux/string.h>
> > #include <linux/msg.h>
> > +#include <linux/bpf_lsm.h>
> > #include <net/flow.h>
> >
> > #define MAX_LSM_EVM_XATTR 2
> > @@ -652,20 +653,21 @@ static void __init lsm_early_task(struct task_struct *task)
> > \
> > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) \
> > P->hook.FUNC(__VA_ARGS__); \
> > + CALL_BPF_LSM_VOID_HOOKS(FUNC, __VA_ARGS__); \
> > } while (0)
> >
> > -#define call_int_hook(FUNC, IRC, ...) ({ \
> > - int RC = IRC; \
> > - do { \
> > - struct security_hook_list *P; \
> > - \
> > +#define call_int_hook(FUNC, IRC, ...) ({ \
> > + int RC = IRC; \
> > + do { \
> > + struct security_hook_list *P; \
> > hlist_for_each_entry(P, &security_hook_heads.FUNC, list) { \
> > - RC = P->hook.FUNC(__VA_ARGS__); \
> > - if (RC != 0) \
> > - break; \
> > - } \
> > - } while (0); \
> > - RC; \
> > + RC = P->hook.FUNC(__VA_ARGS__); \
> > + if (RC != 0) \
> > + break; \
> > + } \
> > + RC = CALL_BPF_LSM_INT_HOOKS(RC, FUNC, __VA_ARGS__); \
>
> Do not do this. Add LSM_ORDER_LAST for the lsm_order field of lsm_info
> and use that to identify your module as one to be put on the list last.
> Update the LSM registration code to do this. It will be much like the code
> that uses LSM_ORDER_FIRST to put the capabilities at the head of the lists.
>
> What you have here to way to much like the way Yama was invoked before
> stacking.
Thanks for taking a look!
The BPF LSM has two kinds of hooks. The static hooks which are
appended to the security_hook_heads in security.c and mutable hooks
which are allocated at runtime and attached to a separate
security_hook_heads (bpf_lsm_hook_heads) defined in security/bpf/lsm.c
This macro runs these mutable hooks (dynamically allocated at runtime)
under SRCU protection.
Having a separate security_hook_heads allows:
- The security_hook_heads in security.c to be __lsm_ro_after_init
and thus limits the attack surface does not change the existing
behaviour
- The SRCU critical section be limited to the execution of dynamically
allocated hooks
I agree that the static hooks should indeed be LSM_ORDER_LAST which
makes logical sense and represents correctly how the LSM will behave
in reality. (i.e. its hooks are executed last irrespective of the
position it is mentioned in the list of LSMs.)
I will update this for v3.
- KP
>
> > + } while (0); \
> > + RC; \
> > })
> >
> > /* Security operations */
>
^ permalink raw reply
* Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-16 12:49 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: KP Singh, open list, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Jann Horn, Matthew Garrett, Christian Brauner,
Mickaël Salaün, Florent Revest, Brendan Jackman,
Martin KaFai Lau, Song Liu, Yonghong Song, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <CAEf4BzZodr3LKJuM7QwD38BiEH02Cc1UbtnGpVkCJ00Mf+V_Qg@mail.gmail.com>
Thanks for the review Andrii!
I will incorporate the fixes in the next revision.
On 15-Jan 13:19, Andrii Nakryiko wrote:
> On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > From: KP Singh <kpsingh@google.com>
> >
> > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > attr->lsm_hook_index
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > tools/lib/bpf/bpf.c | 6 +-
> > tools/lib/bpf/bpf.h | 1 +
> > tools/lib/bpf/libbpf.c | 143 ++++++++++++++++++++++++++++++++++-----
> > tools/lib/bpf/libbpf.h | 4 ++
> > tools/lib/bpf/libbpf.map | 3 +
> > 5 files changed, 138 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > index 500afe478e94..b138d98ff862 100644
> > --- a/tools/lib/bpf/bpf.c
> > +++ b/tools/lib/bpf/bpf.c
> > @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > memset(&attr, 0, sizeof(attr));
> > attr.prog_type = load_attr->prog_type;
> > attr.expected_attach_type = load_attr->expected_attach_type;
> > - if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > + if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> > + attr.lsm_hook_index = load_attr->lsm_hook_index;
> > + } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > attr.attach_btf_id = load_attr->attach_btf_id;
> > } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> > attr.attach_btf_id = load_attr->attach_btf_id;
> > @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > attr.prog_ifindex = load_attr->prog_ifindex;
> > attr.kern_version = load_attr->kern_version;
> > }
> > +
> > attr.insn_cnt = (__u32)load_attr->insns_cnt;
> > attr.insns = ptr_to_u64(load_attr->insns);
> > attr.license = ptr_to_u64(load_attr->license);
> > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > index 56341d117e5b..54458a102939 100644
> > --- a/tools/lib/bpf/bpf.h
> > +++ b/tools/lib/bpf/bpf.h
> > @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
> > __u32 prog_ifindex;
> > __u32 attach_btf_id;
> > };
> > + __u32 lsm_hook_index;
>
>
> this is changing memory layout of struct bpf_load_program_attr, which
> is part of public API, so breaking backward compatibility. But I think
> you intended to put it inside union along the attach_btf_id?
>
Correct, I moved it to the union.
> also, we use idx for index pretty consistently (apart from ifindex),
> so maybe lsm_hook_idx?
Renamed all usages of index -> idx.
>
> > __u32 prog_btf_fd;
> > __u32 func_info_rec_size;
> > const void *func_info;
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 0c229f00a67e..60737559a9a6 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -229,6 +229,7 @@ struct bpf_program {
> > enum bpf_attach_type expected_attach_type;
> > __u32 attach_btf_id;
> > __u32 attach_prog_fd;
> > + __u32 lsm_hook_index
> > void *func_info;
> > __u32 func_info_rec_size;
> > __u32 func_info_cnt;
> > @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > load_attr.insns = insns;
> > load_attr.insns_cnt = insns_cnt;
> > load_attr.license = license;
> > - if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > +
> > + if (prog->type == BPF_PROG_TYPE_LSM) {
> > + load_attr.lsm_hook_index = prog->lsm_hook_index;
> > + } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > load_attr.attach_btf_id = prog->attach_btf_id;
> > } else if (prog->type == BPF_PROG_TYPE_TRACING) {
> > load_attr.attach_prog_fd = prog->attach_prog_fd;
> > @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > load_attr.kern_version = kern_version;
> > load_attr.prog_ifindex = prog->prog_ifindex;
> > }
> > +
> > /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
> > if (prog->obj->btf_ext)
> > btf_fd = bpf_object__btf_fd(prog->obj);
> > @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
> > enum bpf_attach_type attach_type,
> > __u32 attach_prog_fd);
> >
> > +static __s32 btf__find_lsm_hook_index(const char *name);
> > +
> > int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > {
> > - int err = 0, fd, i, btf_id;
> > + int err = 0, fd, i, btf_id, index;
> >
> > if (prog->type == BPF_PROG_TYPE_TRACING) {
> > btf_id = libbpf_find_attach_btf_id(prog->section_name,
> > @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > prog->attach_btf_id = btf_id;
> > }
> >
> > + if (prog->type == BPF_PROG_TYPE_LSM) {
> > + index = btf__find_lsm_hook_index(prog->section_name);
> > + if (index < 0)
> > + return index;
> > + prog->lsm_hook_index = index;
> > + }
> > +
> > if (prog->instances.nr < 0 || !prog->instances.fds) {
> > if (prog->preprocessor) {
> > pr_warn("Internal error: can't load program '%s'\n",
> > @@ -6207,6 +6221,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog) \
> > } \
> >
> > BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
> > +BPF_PROG_TYPE_FNS(lsm, BPF_PROG_TYPE_LSM);
> > BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
> > BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
> > BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
> > @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> > struct bpf_program *prog);
> > static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > struct bpf_program *prog);
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > + struct bpf_program *prog);
> >
> > struct bpf_sec_def {
> > const char *sec;
> > @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
> > .expected_attach_type = BPF_TRACE_FEXIT,
> > .is_attach_btf = true,
> > .attach_fn = attach_trace),
> > + SEC_DEF("lsm/", LSM,
> > + .expected_attach_type = BPF_LSM_MAC,
> > + .attach_fn = attach_lsm),
> > BPF_PROG_SEC("xdp", BPF_PROG_TYPE_XDP),
> > BPF_PROG_SEC("perf_event", BPF_PROG_TYPE_PERF_EVENT),
> > BPF_PROG_SEC("lwt_in", BPF_PROG_TYPE_LWT_IN),
> > BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> > BPF_PROG_SEC("lwt_xmit", BPF_PROG_TYPE_LWT_XMIT),
> > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
> > + BPF_PROG_BTF("lsm/", BPF_PROG_TYPE_LSM,
> > + BPF_LSM_MAC),
>
> This is just a duplicate of SEC_DEF above, remove?
Oops. Removed.
>
> > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB,
> > BPF_CGROUP_INET_INGRESS),
> > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> > @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> > return -EINVAL;
> > }
> >
> > -#define BTF_PREFIX "btf_trace_"
> > +#define BTF_TRACE_PREFIX "btf_trace_"
> > +
> > +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> > + const char *prefix, __u32 kind)
>
> this is internal helper, not really BTF API, let's call it
> find_btf_by_prefix_kind? Also const char *prefix more logically should
> go before name argument?
Done.
>
> > +{
> > + char btf_type_name[128];
> > +
> > + snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
>
> check overflow?
Done.
>
> > + return btf__find_by_name_kind(btf, btf_type_name, kind);
> > +}
> > +
> > +static __s32 btf__find_lsm_hook_index(const char *name)
>
> this name is violating libbpf naming guidelines. Just
> `find_lsm_hook_idx` for now?
Cool. I think I finally hang of the naming convention now :). I mixed
up btf.c and libbpf.c here.
>
> > +{
> > + struct btf *btf = bpf_find_kernel_btf();
>
> ok, it's probably time to do this right. Let's ensure we load kernel
> BTF just once, keep it inside bpf_object while we need it and then
> release it after successful load. We are at the point where all the
> new types of program is loading/releasing kernel BTF for every section
> and it starts to feel very wasteful.
Sure, will give it a shot in v3.
>
> > + const struct bpf_sec_def *sec_def;
> > + const struct btf_type *hl_type;
> > + struct btf_member *m;
> > + __u16 vlen;
> > + __s32 hl_id;
> > + int j;
>
> j without having i used anywhere?...
Fixed.
>
> > +
> > + sec_def = find_sec_def(name);
> > + if (!sec_def)
> > + return -ESRCH;
> > +
> > + name += sec_def->len;
> > +
> > + hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> > + BTF_KIND_STRUCT);
> > + if (hl_id < 0) {
> > + pr_debug("security_hook_heads cannot be found in BTF\n");
>
> "in vmlinux BTF" ?
>
> and it should be pr_warn(), we don't really expect this, right?
Fixed both.
>
> and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
> and then return hl_id ? : -ESRCH, which further proves we need to
> change btf__find_by_name_kind() as I suggested below.
>
> > + return hl_id;
> > + }
> > +
> > + hl_type = btf__type_by_id(btf, hl_id);
> > + if (!hl_type) {
> > + pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> > + return -EINVAL;
>
> -ESRCH?
Done.
>
> > + }
> > +
> > + m = btf_members(hl_type);
> > + vlen = btf_vlen(hl_type);
> > +
> > + for (j = 0; j < vlen; j++) {
>
> can add succinct `, m++` here instead
Done.
>
> > + if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > + return j + 1;
>
> I looked briefly through kernel-side patch introducing lsm_hook_index,
> but it didn't seem to explain why this index needs to be (unnaturally)
> 1-based. So asking here first as I'm looking through libbpf changes?
The lsm_hook_idx is one-based as it makes it easy to validate the
input. If we make it zero-based it's hard to check if the user
intended to attach to the LSM hook at index 0 or did not set it.
We are then up to the verifier to reject the loaded program which
may or may not match the signature of the hook at lsm_hook_idx = 0.
I will clarify this in the commit log as well.
>
> > + m++;
> > + }
> > +
> > + pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
>
> it's not offset, rather member index?
Correct, fixed.
>
> > + return -ENOENT;
>
> not entirely clear about distinction between ENOENT and ESRCH? So far
> we typically used ESRCH, does ENOENT have more specific semantics?
Updated it to ESRCH, to be consistent with libbpf's convention.
>
> > +}
> > +
> > int libbpf_find_vmlinux_btf_id(const char *name,
> > enum bpf_attach_type attach_type)
> > {
> > struct btf *btf = bpf_find_kernel_btf();
> > - char raw_tp_btf[128] = BTF_PREFIX;
> > - char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> > - const char *btf_name;
> > int err = -EINVAL;
> > - __u32 kind;
> >
> > if (IS_ERR(btf)) {
> > pr_warn("vmlinux BTF is not found\n");
> > return -EINVAL;
> > }
> >
> > - if (attach_type == BPF_TRACE_RAW_TP) {
> > - /* prepend "btf_trace_" prefix per kernel convention */
> > - strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> > - btf_name = raw_tp_btf;
> > - kind = BTF_KIND_TYPEDEF;
> > - } else {
> > - btf_name = name;
> > - kind = BTF_KIND_FUNC;
> > - }
> > - err = btf__find_by_name_kind(btf, btf_name, kind);
> > + if (attach_type == BPF_TRACE_RAW_TP)
> > + err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> > + BTF_KIND_TYPEDEF);
> > + else
> > + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > +
> > + /* err = 0 means void / UNKNOWN which is treated as an error */
> > + if (err == 0)
> > + err = -EINVAL;
>
> I think it's actually less error-prone to make btf__find_by_name_kind
> and btf__find_by_prefix_kind to return -ESRCH when type is not found,
> instead of a valid type_id 0. I just checked, and struct_ops code
> already is mishandling it, only checking for <0. Could you make this
> change and just do a natural <0 check everywhere?
>
>
> > +
> > btf__free(btf);
> > return err;
> > }
> > @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > }
> > err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > btf__free(btf);
> > - if (err <= 0) {
> > + if (err < 0) {
> > pr_warn("%s is not found in prog's BTF\n", name);
> > goto out;
> > }
> > @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > return bpf_program__attach_trace(prog);
> > }
> >
> > +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> > +{
> > + char errmsg[STRERR_BUFSIZE];
> > + struct bpf_link_fd *link;
> > + int prog_fd, pfd;
> > +
> > + prog_fd = bpf_program__fd(prog);
> > + if (prog_fd < 0) {
> > + pr_warn("program '%s': can't attach before loaded\n",
> > + bpf_program__title(prog, false));
> > + return ERR_PTR(-EINVAL);
> > + }
> > +
> > + link = calloc(1, sizeof(*link));
> > + if (!link)
> > + return ERR_PTR(-ENOMEM);
> > + link->link.detach = &bpf_link__detach_fd;
> > +
> > + pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> > + BPF_F_ALLOW_OVERRIDE);
>
> do we want to always specify ALLOW_OVERRIDE? Or should it be an option?
I think this is a relic from the duplicate attachment code. With the
anonymous-fd + link which can be destroyed. The way the OVERRIDE
should work is:
- Destroy the link (detach)
- And attach.
We don't use the BPF_F_ALLOW_OVERRIDE in the attach logic in the LSM.
So I will get rid of that.
>
> > + if (pfd < 0) {
> > + pfd = -errno;
> > + pr_warn("program '%s': failed to attach: %s\n",
> > + bpf_program__title(prog, false),
> > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > + return ERR_PTR(pfd);
>
> leaking link here
Fixed.
>
> > + }
> > + link->fd = pfd;
> > + return (struct bpf_link *)link;
> > +}
> > +
> > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > + struct bpf_program *prog)
> > +{
> > + return bpf_program__attach_lsm(prog);
> > +}
> > +
> > struct bpf_link *bpf_program__attach(struct bpf_program *prog)
> > {
> > const struct bpf_sec_def *sec_def;
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 01639f9a1062..a97e709a29e6 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
> > bpf_program__attach_trace(struct bpf_program *prog);
> > struct bpf_map;
> > LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> > +LIBBPF_API struct bpf_link *
> > +bpf_program__attach_lsm(struct bpf_program *prog);
>
> nit: put it after attach_trace, so that program attaches and map
> attaches are grouped together, not intermixed
Done.
>
> > struct bpf_insn;
> >
> > /*
> > @@ -318,6 +320,7 @@ LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
> > LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
> > LIBBPF_API int bpf_program__set_tracing(struct bpf_program *prog);
> > LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> > +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
> >
> > LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> > LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> > @@ -339,6 +342,7 @@ LIBBPF_API bool bpf_program__is_xdp(const struct bpf_program *prog);
> > LIBBPF_API bool bpf_program__is_perf_event(const struct bpf_program *prog);
> > LIBBPF_API bool bpf_program__is_tracing(const struct bpf_program *prog);
> > LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> > +LIBBPF_API bool bpf_program__is_lsm(const struct bpf_program *prog);
> >
> > /*
> > * No need for __attribute__((packed)), all members of 'bpf_map_def'
> > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > index a19f04e6e3d9..3da0452ce679 100644
> > --- a/tools/lib/bpf/libbpf.map
> > +++ b/tools/lib/bpf/libbpf.map
> > @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
> > bpf_program__is_struct_ops;
> > bpf_program__set_struct_ops;
> > btf__align_of;
> > + bpf_program__is_lsm;
> > + bpf_program__set_lsm;
> > + bpf_program__attach_lsm;
>
> preserve alphabetical order, please
Sure.
>
> > } LIBBPF_0.0.6;
>
> > --
> > 2.20.1
> >
^ permalink raw reply
* Re: [PATCH bpf-next v2 02/10] bpf: lsm: Add a skeleton and config options
From: KP Singh @ 2020-01-16 12:52 UTC (permalink / raw)
To: Casey Schaufler; +Cc: KP Singh, Linux Security Module list, bpf
In-Reply-To: <7b11f92b-259f-f2e1-924c-5c0518f49b3f@schaufler-ca.com>
On 15-Jan 23:04, Casey Schaufler wrote:
> On 1/15/2020 9:13 AM, KP Singh wrote:
> > From: KP Singh <kpsingh@google.com>
> >
> > The LSM can be enabled by CONFIG_SECURITY_BPF.
> > Without CONFIG_SECURITY_BPF_ENFORCE, the LSM will run the
> > attached eBPF programs but not enforce MAC policy based
> > on the return value of the attached programs.
> >
> > Signed-off-by: KP Singh <kpsingh@google.com>
> > ---
> > MAINTAINERS | 7 +++++++
> > security/Kconfig | 11 ++++++-----
> > security/Makefile | 2 ++
> > security/bpf/Kconfig | 22 ++++++++++++++++++++++
> > security/bpf/Makefile | 5 +++++
> > security/bpf/lsm.c | 25 +++++++++++++++++++++++++
> > 6 files changed, 67 insertions(+), 5 deletions(-)
> > create mode 100644 security/bpf/Kconfig
> > create mode 100644 security/bpf/Makefile
> > create mode 100644 security/bpf/lsm.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 66a2e5e07117..0941f478cfa5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -3203,6 +3203,13 @@ S: Supported
> > F: arch/x86/net/
> > X: arch/x86/net/bpf_jit_comp32.c
> >
> > +BPF SECURITY MODULE
> > +M: KP Singh <kpsingh@chromium.org>
> > +L: linux-security-module@vger.kernel.org
> > +L: bpf@vger.kernel.org
> > +S: Maintained
> > +F: security/bpf/
> > +
> > BROADCOM B44 10/100 ETHERNET DRIVER
> > M: Michael Chan <michael.chan@broadcom.com>
> > L: netdev@vger.kernel.org
> > diff --git a/security/Kconfig b/security/Kconfig
> > index 2a1a2d396228..6f1aab195e7d 100644
> > --- a/security/Kconfig
> > +++ b/security/Kconfig
> > @@ -236,6 +236,7 @@ source "security/tomoyo/Kconfig"
> > source "security/apparmor/Kconfig"
> > source "security/loadpin/Kconfig"
> > source "security/yama/Kconfig"
> > +source "security/bpf/Kconfig"
> > source "security/safesetid/Kconfig"
> > source "security/lockdown/Kconfig"
> >
> > @@ -277,11 +278,11 @@ endchoice
> >
> > config LSM
> > string "Ordered list of enabled LSMs"
> > - default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor" if DEFAULT_SECURITY_SMACK
> > - default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo" if DEFAULT_SECURITY_APPARMOR
> > - default "lockdown,yama,loadpin,safesetid,integrity,tomoyo" if DEFAULT_SECURITY_TOMOYO
> > - default "lockdown,yama,loadpin,safesetid,integrity" if DEFAULT_SECURITY_DAC
> > - default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor"
> > + default "lockdown,yama,loadpin,safesetid,integrity,smack,selinux,tomoyo,apparmor,bpf" if DEFAULT_SECURITY_SMACK
> > + default "lockdown,yama,loadpin,safesetid,integrity,apparmor,selinux,smack,tomoyo,bpf" if DEFAULT_SECURITY_APPARMOR
> > + default "lockdown,yama,loadpin,safesetid,integrity,tomoyo,bpf" if DEFAULT_SECURITY_TOMOYO
> > + default "lockdown,yama,loadpin,safesetid,integrity,bpf" if DEFAULT_SECURITY_DAC
> > + default "lockdown,yama,loadpin,safesetid,integrity,selinux,smack,tomoyo,apparmor,bpf"
> > help
> > A comma-separated list of LSMs, in initialization order.
> > Any LSMs left off this list will be ignored. This can be
> > diff --git a/security/Makefile b/security/Makefile
> > index be1dd9d2cb2f..50e6821dd7b7 100644
> > --- a/security/Makefile
> > +++ b/security/Makefile
> > @@ -12,6 +12,7 @@ subdir-$(CONFIG_SECURITY_YAMA) += yama
> > subdir-$(CONFIG_SECURITY_LOADPIN) += loadpin
> > subdir-$(CONFIG_SECURITY_SAFESETID) += safesetid
> > subdir-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown
> > +subdir-$(CONFIG_SECURITY_BPF) += bpf
> >
> > # always enable default capabilities
> > obj-y += commoncap.o
> > @@ -29,6 +30,7 @@ obj-$(CONFIG_SECURITY_YAMA) += yama/
> > obj-$(CONFIG_SECURITY_LOADPIN) += loadpin/
> > obj-$(CONFIG_SECURITY_SAFESETID) += safesetid/
> > obj-$(CONFIG_SECURITY_LOCKDOWN_LSM) += lockdown/
> > +obj-$(CONFIG_SECURITY_BPF) += bpf/
> > obj-$(CONFIG_CGROUP_DEVICE) += device_cgroup.o
> >
> > # Object integrity file lists
> > diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> > new file mode 100644
> > index 000000000000..a5f6c67ae526
> > --- /dev/null
> > +++ b/security/bpf/Kconfig
> > @@ -0,0 +1,22 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright 2019 Google LLC.
> > +
> > +config SECURITY_BPF
> > + bool "BPF-based MAC and audit policy"
> > + depends on SECURITY
> > + depends on BPF_SYSCALL
> > + help
> > + This enables instrumentation of the security hooks with
> > + eBPF programs.
> > +
> > + If you are unsure how to answer this question, answer N.
> > +
> > +config SECURITY_BPF_ENFORCE
> > + bool "Deny operations based on the evaluation of the attached programs"
> > + depends on SECURITY_BPF
> > + help
> > + eBPF programs attached to hooks can be used for both auditing and
> > + enforcement. Enabling enforcement implies that the evaluation result
> > + from the attached eBPF programs will allow or deny the operation
> > + guarded by the security hook.
> > diff --git a/security/bpf/Makefile b/security/bpf/Makefile
> > new file mode 100644
> > index 000000000000..26a0ab6f99b7
> > --- /dev/null
> > +++ b/security/bpf/Makefile
> > @@ -0,0 +1,5 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# Copyright 2019 Google LLC.
> > +
> > +obj-$(CONFIG_SECURITY_BPF) := lsm.o
> > diff --git a/security/bpf/lsm.c b/security/bpf/lsm.c
> > new file mode 100644
> > index 000000000000..5c5c14f990ce
> > --- /dev/null
> > +++ b/security/bpf/lsm.c
> > @@ -0,0 +1,25 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +/*
> > + * Copyright 2019 Google LLC.
> > + */
> > +
> > +#include <linux/lsm_hooks.h>
> > +
> > +/* This is only for internal hooks, always statically shipped as part of the
> > + * BPF LSM. Statically defined hooks are appeneded to the security_hook_heads
> > + * which is common for LSMs and R/O after init.
> > + */
> > +static struct security_hook_list lsm_hooks[] __lsm_ro_after_init = {};
>
> s/lsm_hooks/bpf_hooks/
>
> The lsm prefix is for the infrastructure. The way you have it is massively confusing.
Good point, I changed this to bpf_lsm_hooks as we prefix most types
with bpf_lsm_
>
> > +
> > +static int __init lsm_init(void)
>
> s/lsm_init/bpf_init/
>
> Same reason. When I'm looking at several security modules at once I
> need to be able to tell them apart.
Changed to bpf_lsm_init.
>
> > +{
> > + security_add_hooks(lsm_hooks, ARRAY_SIZE(lsm_hooks), "bpf");
> > + pr_info("eBPF and LSM are friends now.\n");
>
> Cute message, but not very informative if you haven't read the code.
> "LSM support for eBPF active\n" is more likely to be comprehensible.
Agreed, Updated :)
- KP
>
> > + return 0;
> > +}
> > +
> > +DEFINE_LSM(bpf) = {
> > + .name = "bpf",
> > + .init = lsm_init,
> > +};
>
^ permalink raw reply
* [PATCH AUTOSEL 4.19 007/671] apparmor: Fix network performance issue in aa_label_sk_perm
From: Sasha Levin @ 2020-01-16 16:43 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Tony Jones, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20200116165502.8838-1-sashal@kernel.org>
From: Tony Jones <tonyj@suse.de>
[ Upstream commit 5f997580e8b12b9f585e34cc16304925d26ce49e ]
The netperf benchmark shows a 5.73% reduction in throughput for
small (64 byte) transfers by unconfined tasks.
DEFINE_AUDIT_SK() in aa_label_sk_perm() should not be performed
unconditionally, rather only when the label is confined.
netperf-tcp
56974a6fc^ 56974a6fc
Min 64 563.48 ( 0.00%) 531.17 ( -5.73%)
Min 128 1056.92 ( 0.00%) 999.44 ( -5.44%)
Min 256 1945.95 ( 0.00%) 1867.97 ( -4.01%)
Min 1024 6761.40 ( 0.00%) 6364.23 ( -5.87%)
Min 2048 11110.53 ( 0.00%) 10606.20 ( -4.54%)
Min 3312 13692.67 ( 0.00%) 13158.41 ( -3.90%)
Min 4096 14926.29 ( 0.00%) 14457.46 ( -3.14%)
Min 8192 18399.34 ( 0.00%) 18091.65 ( -1.67%)
Min 16384 21384.13 ( 0.00%) 21158.05 ( -1.06%)
Hmean 64 564.96 ( 0.00%) 534.38 ( -5.41%)
Hmean 128 1064.42 ( 0.00%) 1010.12 ( -5.10%)
Hmean 256 1965.85 ( 0.00%) 1879.16 ( -4.41%)
Hmean 1024 6839.77 ( 0.00%) 6478.70 ( -5.28%)
Hmean 2048 11154.80 ( 0.00%) 10671.13 ( -4.34%)
Hmean 3312 13838.12 ( 0.00%) 13249.01 ( -4.26%)
Hmean 4096 15009.99 ( 0.00%) 14561.36 ( -2.99%)
Hmean 8192 18975.57 ( 0.00%) 18326.54 ( -3.42%)
Hmean 16384 21440.44 ( 0.00%) 21324.59 ( -0.54%)
Stddev 64 1.24 ( 0.00%) 2.85 (-130.64%)
Stddev 128 4.51 ( 0.00%) 6.53 ( -44.84%)
Stddev 256 11.67 ( 0.00%) 8.50 ( 27.16%)
Stddev 1024 48.33 ( 0.00%) 75.07 ( -55.34%)
Stddev 2048 54.82 ( 0.00%) 65.16 ( -18.86%)
Stddev 3312 153.57 ( 0.00%) 56.29 ( 63.35%)
Stddev 4096 100.25 ( 0.00%) 88.50 ( 11.72%)
Stddev 8192 358.13 ( 0.00%) 169.99 ( 52.54%)
Stddev 16384 43.99 ( 0.00%) 141.82 (-222.39%)
Signed-off-by: Tony Jones <tonyj@suse.de>
Fixes: 56974a6fcfef ("apparmor: add base infastructure for socket
mediation")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/net.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/security/apparmor/net.c b/security/apparmor/net.c
index bb24cfa0a164..d5d72dd1ca1f 100644
--- a/security/apparmor/net.c
+++ b/security/apparmor/net.c
@@ -146,17 +146,20 @@ int aa_af_perm(struct aa_label *label, const char *op, u32 request, u16 family,
static int aa_label_sk_perm(struct aa_label *label, const char *op, u32 request,
struct sock *sk)
{
- struct aa_profile *profile;
- DEFINE_AUDIT_SK(sa, op, sk);
+ int error = 0;
AA_BUG(!label);
AA_BUG(!sk);
- if (unconfined(label))
- return 0;
+ if (!unconfined(label)) {
+ struct aa_profile *profile;
+ DEFINE_AUDIT_SK(sa, op, sk);
- return fn_for_each_confined(label, profile,
- aa_profile_af_sk_perm(profile, &sa, request, sk));
+ error = fn_for_each_confined(label, profile,
+ aa_profile_af_sk_perm(profile, &sa, request, sk));
+ }
+
+ return error;
}
int aa_sk_perm(const char *op, u32 request, struct sock *sk)
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 004/371] apparmor: don't try to replace stale label in ptrace access check
From: Sasha Levin @ 2020-01-16 17:11 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Jann Horn, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20200116171719.16965-1-sashal@kernel.org>
From: Jann Horn <jannh@google.com>
[ Upstream commit 1f8266ff58840d698a1e96d2274189de1bdf7969 ]
As a comment above begin_current_label_crit_section() explains,
begin_current_label_crit_section() must run in sleepable context because
when label_is_stale() is true, aa_replace_current_label() runs, which uses
prepare_creds(), which can sleep.
Until now, the ptrace access check (which runs with a task lock held)
violated this rule.
Also add a might_sleep() assertion to begin_current_label_crit_section(),
because asserts are less likely to be ignored than comments.
Fixes: b2d09ae449ced ("apparmor: move ptrace checks to using labels")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/include/context.h | 2 ++
security/apparmor/lsm.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/include/context.h b/security/apparmor/include/context.h
index 6ae07e9aaa17..812cdec9dd3b 100644
--- a/security/apparmor/include/context.h
+++ b/security/apparmor/include/context.h
@@ -191,6 +191,8 @@ static inline struct aa_label *begin_current_label_crit_section(void)
{
struct aa_label *label = aa_current_raw_label();
+ might_sleep();
+
if (label_is_stale(label)) {
label = aa_get_newest_label(label);
if (aa_replace_current_label(label) == 0)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 1346ee5be04f..4f08023101f3 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -108,12 +108,12 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
struct aa_label *tracer, *tracee;
int error;
- tracer = begin_current_label_crit_section();
+ tracer = __begin_current_label_crit_section();
tracee = aa_get_task_label(child);
error = aa_may_ptrace(tracer, tracee,
mode == PTRACE_MODE_READ ? AA_PTRACE_READ : AA_PTRACE_TRACE);
aa_put_label(tracee);
- end_current_label_crit_section(tracer);
+ __end_current_label_crit_section(tracer);
return error;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-16 17:26 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200116124955.GD240584@google.com>
On 16-Jan 13:49, KP Singh wrote:
> Thanks for the review Andrii!
>
> I will incorporate the fixes in the next revision.
>
> On 15-Jan 13:19, Andrii Nakryiko wrote:
> > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > > attr->lsm_hook_index
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > > tools/lib/bpf/bpf.c | 6 +-
> > > tools/lib/bpf/bpf.h | 1 +
> > > tools/lib/bpf/libbpf.c | 143 ++++++++++++++++++++++++++++++++++-----
> > > tools/lib/bpf/libbpf.h | 4 ++
> > > tools/lib/bpf/libbpf.map | 3 +
> > > 5 files changed, 138 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/tools/lib/bpf/bpf.c b/tools/lib/bpf/bpf.c
> > > index 500afe478e94..b138d98ff862 100644
> > > --- a/tools/lib/bpf/bpf.c
> > > +++ b/tools/lib/bpf/bpf.c
> > > @@ -235,7 +235,10 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > > memset(&attr, 0, sizeof(attr));
> > > attr.prog_type = load_attr->prog_type;
> > > attr.expected_attach_type = load_attr->expected_attach_type;
> > > - if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > +
> > > + if (attr.prog_type == BPF_PROG_TYPE_LSM) {
> > > + attr.lsm_hook_index = load_attr->lsm_hook_index;
> > > + } else if (attr.prog_type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > attr.attach_btf_id = load_attr->attach_btf_id;
> > > } else if (attr.prog_type == BPF_PROG_TYPE_TRACING) {
> > > attr.attach_btf_id = load_attr->attach_btf_id;
> > > @@ -244,6 +247,7 @@ int bpf_load_program_xattr(const struct bpf_load_program_attr *load_attr,
> > > attr.prog_ifindex = load_attr->prog_ifindex;
> > > attr.kern_version = load_attr->kern_version;
> > > }
> > > +
> > > attr.insn_cnt = (__u32)load_attr->insns_cnt;
> > > attr.insns = ptr_to_u64(load_attr->insns);
> > > attr.license = ptr_to_u64(load_attr->license);
> > > diff --git a/tools/lib/bpf/bpf.h b/tools/lib/bpf/bpf.h
> > > index 56341d117e5b..54458a102939 100644
> > > --- a/tools/lib/bpf/bpf.h
> > > +++ b/tools/lib/bpf/bpf.h
> > > @@ -86,6 +86,7 @@ struct bpf_load_program_attr {
> > > __u32 prog_ifindex;
> > > __u32 attach_btf_id;
> > > };
> > > + __u32 lsm_hook_index;
> >
> >
> > this is changing memory layout of struct bpf_load_program_attr, which
> > is part of public API, so breaking backward compatibility. But I think
> > you intended to put it inside union along the attach_btf_id?
> >
>
> Correct, I moved it to the union.
>
> > also, we use idx for index pretty consistently (apart from ifindex),
> > so maybe lsm_hook_idx?
>
> Renamed all usages of index -> idx.
>
> >
> > > __u32 prog_btf_fd;
> > > __u32 func_info_rec_size;
> > > const void *func_info;
> > > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > > index 0c229f00a67e..60737559a9a6 100644
> > > --- a/tools/lib/bpf/libbpf.c
> > > +++ b/tools/lib/bpf/libbpf.c
> > > @@ -229,6 +229,7 @@ struct bpf_program {
> > > enum bpf_attach_type expected_attach_type;
> > > __u32 attach_btf_id;
> > > __u32 attach_prog_fd;
> > > + __u32 lsm_hook_index
> > > void *func_info;
> > > __u32 func_info_rec_size;
> > > __u32 func_info_cnt;
> > > @@ -4886,7 +4887,10 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > > load_attr.insns = insns;
> > > load_attr.insns_cnt = insns_cnt;
> > > load_attr.license = license;
> > > - if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > +
> > > + if (prog->type == BPF_PROG_TYPE_LSM) {
> > > + load_attr.lsm_hook_index = prog->lsm_hook_index;
> > > + } else if (prog->type == BPF_PROG_TYPE_STRUCT_OPS) {
> > > load_attr.attach_btf_id = prog->attach_btf_id;
> > > } else if (prog->type == BPF_PROG_TYPE_TRACING) {
> > > load_attr.attach_prog_fd = prog->attach_prog_fd;
> > > @@ -4895,6 +4899,7 @@ load_program(struct bpf_program *prog, struct bpf_insn *insns, int insns_cnt,
> > > load_attr.kern_version = kern_version;
> > > load_attr.prog_ifindex = prog->prog_ifindex;
> > > }
> > > +
> > > /* if .BTF.ext was loaded, kernel supports associated BTF for prog */
> > > if (prog->obj->btf_ext)
> > > btf_fd = bpf_object__btf_fd(prog->obj);
> > > @@ -4967,9 +4972,11 @@ static int libbpf_find_attach_btf_id(const char *name,
> > > enum bpf_attach_type attach_type,
> > > __u32 attach_prog_fd);
> > >
> > > +static __s32 btf__find_lsm_hook_index(const char *name);
> > > +
> > > int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > > {
> > > - int err = 0, fd, i, btf_id;
> > > + int err = 0, fd, i, btf_id, index;
> > >
> > > if (prog->type == BPF_PROG_TYPE_TRACING) {
> > > btf_id = libbpf_find_attach_btf_id(prog->section_name,
> > > @@ -4980,6 +4987,13 @@ int bpf_program__load(struct bpf_program *prog, char *license, __u32 kern_ver)
> > > prog->attach_btf_id = btf_id;
> > > }
> > >
> > > + if (prog->type == BPF_PROG_TYPE_LSM) {
> > > + index = btf__find_lsm_hook_index(prog->section_name);
> > > + if (index < 0)
> > > + return index;
> > > + prog->lsm_hook_index = index;
> > > + }
> > > +
> > > if (prog->instances.nr < 0 || !prog->instances.fds) {
> > > if (prog->preprocessor) {
> > > pr_warn("Internal error: can't load program '%s'\n",
> > > @@ -6207,6 +6221,7 @@ bool bpf_program__is_##NAME(const struct bpf_program *prog) \
> > > } \
> > >
> > > BPF_PROG_TYPE_FNS(socket_filter, BPF_PROG_TYPE_SOCKET_FILTER);
> > > +BPF_PROG_TYPE_FNS(lsm, BPF_PROG_TYPE_LSM);
> > > BPF_PROG_TYPE_FNS(kprobe, BPF_PROG_TYPE_KPROBE);
> > > BPF_PROG_TYPE_FNS(sched_cls, BPF_PROG_TYPE_SCHED_CLS);
> > > BPF_PROG_TYPE_FNS(sched_act, BPF_PROG_TYPE_SCHED_ACT);
> > > @@ -6272,6 +6287,8 @@ static struct bpf_link *attach_raw_tp(const struct bpf_sec_def *sec,
> > > struct bpf_program *prog);
> > > static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > > struct bpf_program *prog);
> > > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > > + struct bpf_program *prog);
> > >
> > > struct bpf_sec_def {
> > > const char *sec;
> > > @@ -6315,12 +6332,17 @@ static const struct bpf_sec_def section_defs[] = {
> > > .expected_attach_type = BPF_TRACE_FEXIT,
> > > .is_attach_btf = true,
> > > .attach_fn = attach_trace),
> > > + SEC_DEF("lsm/", LSM,
> > > + .expected_attach_type = BPF_LSM_MAC,
> > > + .attach_fn = attach_lsm),
> > > BPF_PROG_SEC("xdp", BPF_PROG_TYPE_XDP),
> > > BPF_PROG_SEC("perf_event", BPF_PROG_TYPE_PERF_EVENT),
> > > BPF_PROG_SEC("lwt_in", BPF_PROG_TYPE_LWT_IN),
> > > BPF_PROG_SEC("lwt_out", BPF_PROG_TYPE_LWT_OUT),
> > > BPF_PROG_SEC("lwt_xmit", BPF_PROG_TYPE_LWT_XMIT),
> > > BPF_PROG_SEC("lwt_seg6local", BPF_PROG_TYPE_LWT_SEG6LOCAL),
> > > + BPF_PROG_BTF("lsm/", BPF_PROG_TYPE_LSM,
> > > + BPF_LSM_MAC),
> >
> > This is just a duplicate of SEC_DEF above, remove?
>
> Oops. Removed.
>
> >
> > > BPF_APROG_SEC("cgroup_skb/ingress", BPF_PROG_TYPE_CGROUP_SKB,
> > > BPF_CGROUP_INET_INGRESS),
> > > BPF_APROG_SEC("cgroup_skb/egress", BPF_PROG_TYPE_CGROUP_SKB,
> > > @@ -6576,32 +6598,80 @@ static int bpf_object__collect_struct_ops_map_reloc(struct bpf_object *obj,
> > > return -EINVAL;
> > > }
> > >
> > > -#define BTF_PREFIX "btf_trace_"
> > > +#define BTF_TRACE_PREFIX "btf_trace_"
> > > +
> > > +static inline int btf__find_by_prefix_kind(struct btf *btf, const char *name,
> > > + const char *prefix, __u32 kind)
> >
> > this is internal helper, not really BTF API, let's call it
> > find_btf_by_prefix_kind? Also const char *prefix more logically should
> > go before name argument?
>
> Done.
>
> >
> > > +{
> > > + char btf_type_name[128];
> > > +
> > > + snprintf(btf_type_name, sizeof(btf_type_name), "%s%s", prefix, name);
> >
> > check overflow?
>
> Done.
>
> >
> > > + return btf__find_by_name_kind(btf, btf_type_name, kind);
> > > +}
> > > +
> > > +static __s32 btf__find_lsm_hook_index(const char *name)
> >
> > this name is violating libbpf naming guidelines. Just
> > `find_lsm_hook_idx` for now?
>
> Cool. I think I finally hang of the naming convention now :). I mixed
> up btf.c and libbpf.c here.
>
> >
> > > +{
> > > + struct btf *btf = bpf_find_kernel_btf();
> >
> > ok, it's probably time to do this right. Let's ensure we load kernel
> > BTF just once, keep it inside bpf_object while we need it and then
> > release it after successful load. We are at the point where all the
> > new types of program is loading/releasing kernel BTF for every section
> > and it starts to feel very wasteful.
>
> Sure, will give it a shot in v3.
Since this will be useful for other programs as well, I will send it
as a separate patch sooner than the v3 of the LSM patch-set.
- KP
>
> >
> > > + const struct bpf_sec_def *sec_def;
> > > + const struct btf_type *hl_type;
> > > + struct btf_member *m;
> > > + __u16 vlen;
> > > + __s32 hl_id;
> > > + int j;
> >
> > j without having i used anywhere?...
>
> Fixed.
>
> >
> > > +
> > > + sec_def = find_sec_def(name);
> > > + if (!sec_def)
> > > + return -ESRCH;
> > > +
> > > + name += sec_def->len;
> > > +
> > > + hl_id = btf__find_by_name_kind(btf, "security_hook_heads",
> > > + BTF_KIND_STRUCT);
> > > + if (hl_id < 0) {
> > > + pr_debug("security_hook_heads cannot be found in BTF\n");
> >
> > "in vmlinux BTF" ?
> >
> > and it should be pr_warn(), we don't really expect this, right?
>
> Fixed both.
>
> >
> > and it should be (hl_id <= 0) with current btf__find_by_name_kind(),
> > and then return hl_id ? : -ESRCH, which further proves we need to
> > change btf__find_by_name_kind() as I suggested below.
> >
> > > + return hl_id;
> > > + }
> > > +
> > > + hl_type = btf__type_by_id(btf, hl_id);
> > > + if (!hl_type) {
> > > + pr_warn("Can't find type for security_hook_heads: %u\n", hl_id);
> > > + return -EINVAL;
> >
> > -ESRCH?
>
> Done.
>
> >
> > > + }
> > > +
> > > + m = btf_members(hl_type);
> > > + vlen = btf_vlen(hl_type);
> > > +
> > > + for (j = 0; j < vlen; j++) {
> >
> > can add succinct `, m++` here instead
>
> Done.
>
> >
> > > + if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > + return j + 1;
> >
> > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > but it didn't seem to explain why this index needs to be (unnaturally)
> > 1-based. So asking here first as I'm looking through libbpf changes?
>
> The lsm_hook_idx is one-based as it makes it easy to validate the
> input. If we make it zero-based it's hard to check if the user
> intended to attach to the LSM hook at index 0 or did not set it.
>
> We are then up to the verifier to reject the loaded program which
> may or may not match the signature of the hook at lsm_hook_idx = 0.
>
> I will clarify this in the commit log as well.
>
> >
> > > + m++;
> > > + }
> > > +
> > > + pr_warn("Cannot find offset for %s in security_hook_heads\n", name);
> >
> > it's not offset, rather member index?
>
> Correct, fixed.
>
> >
> > > + return -ENOENT;
> >
> > not entirely clear about distinction between ENOENT and ESRCH? So far
> > we typically used ESRCH, does ENOENT have more specific semantics?
>
> Updated it to ESRCH, to be consistent with libbpf's convention.
>
> >
> > > +}
> > > +
> > > int libbpf_find_vmlinux_btf_id(const char *name,
> > > enum bpf_attach_type attach_type)
> > > {
> > > struct btf *btf = bpf_find_kernel_btf();
> > > - char raw_tp_btf[128] = BTF_PREFIX;
> > > - char *dst = raw_tp_btf + sizeof(BTF_PREFIX) - 1;
> > > - const char *btf_name;
> > > int err = -EINVAL;
> > > - __u32 kind;
> > >
> > > if (IS_ERR(btf)) {
> > > pr_warn("vmlinux BTF is not found\n");
> > > return -EINVAL;
> > > }
> > >
> > > - if (attach_type == BPF_TRACE_RAW_TP) {
> > > - /* prepend "btf_trace_" prefix per kernel convention */
> > > - strncat(dst, name, sizeof(raw_tp_btf) - sizeof(BTF_PREFIX));
> > > - btf_name = raw_tp_btf;
> > > - kind = BTF_KIND_TYPEDEF;
> > > - } else {
> > > - btf_name = name;
> > > - kind = BTF_KIND_FUNC;
> > > - }
> > > - err = btf__find_by_name_kind(btf, btf_name, kind);
> > > + if (attach_type == BPF_TRACE_RAW_TP)
> > > + err = btf__find_by_prefix_kind(btf, name, BTF_TRACE_PREFIX,
> > > + BTF_KIND_TYPEDEF);
> > > + else
> > > + err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > > +
> > > + /* err = 0 means void / UNKNOWN which is treated as an error */
> > > + if (err == 0)
> > > + err = -EINVAL;
> >
> > I think it's actually less error-prone to make btf__find_by_name_kind
> > and btf__find_by_prefix_kind to return -ESRCH when type is not found,
> > instead of a valid type_id 0. I just checked, and struct_ops code
> > already is mishandling it, only checking for <0. Could you make this
> > change and just do a natural <0 check everywhere?
> >
> >
> > > +
> > > btf__free(btf);
> > > return err;
> > > }
> > > @@ -6630,7 +6700,7 @@ static int libbpf_find_prog_btf_id(const char *name, __u32 attach_prog_fd)
> > > }
> > > err = btf__find_by_name_kind(btf, name, BTF_KIND_FUNC);
> > > btf__free(btf);
> > > - if (err <= 0) {
> > > + if (err < 0) {
> > > pr_warn("%s is not found in prog's BTF\n", name);
> > > goto out;
> > > }
> > > @@ -7395,6 +7465,43 @@ static struct bpf_link *attach_trace(const struct bpf_sec_def *sec,
> > > return bpf_program__attach_trace(prog);
> > > }
> > >
> > > +struct bpf_link *bpf_program__attach_lsm(struct bpf_program *prog)
> > > +{
> > > + char errmsg[STRERR_BUFSIZE];
> > > + struct bpf_link_fd *link;
> > > + int prog_fd, pfd;
> > > +
> > > + prog_fd = bpf_program__fd(prog);
> > > + if (prog_fd < 0) {
> > > + pr_warn("program '%s': can't attach before loaded\n",
> > > + bpf_program__title(prog, false));
> > > + return ERR_PTR(-EINVAL);
> > > + }
> > > +
> > > + link = calloc(1, sizeof(*link));
> > > + if (!link)
> > > + return ERR_PTR(-ENOMEM);
> > > + link->link.detach = &bpf_link__detach_fd;
> > > +
> > > + pfd = bpf_prog_attach(prog_fd, 0, BPF_LSM_MAC,
> > > + BPF_F_ALLOW_OVERRIDE);
> >
> > do we want to always specify ALLOW_OVERRIDE? Or should it be an option?
>
> I think this is a relic from the duplicate attachment code. With the
> anonymous-fd + link which can be destroyed. The way the OVERRIDE
> should work is:
>
> - Destroy the link (detach)
> - And attach.
>
> We don't use the BPF_F_ALLOW_OVERRIDE in the attach logic in the LSM.
> So I will get rid of that.
>
> >
> > > + if (pfd < 0) {
> > > + pfd = -errno;
> > > + pr_warn("program '%s': failed to attach: %s\n",
> > > + bpf_program__title(prog, false),
> > > + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> > > + return ERR_PTR(pfd);
> >
> > leaking link here
>
> Fixed.
>
> >
> > > + }
> > > + link->fd = pfd;
> > > + return (struct bpf_link *)link;
> > > +}
> > > +
> > > +static struct bpf_link *attach_lsm(const struct bpf_sec_def *sec,
> > > + struct bpf_program *prog)
> > > +{
> > > + return bpf_program__attach_lsm(prog);
> > > +}
> > > +
> > > struct bpf_link *bpf_program__attach(struct bpf_program *prog)
> > > {
> > > const struct bpf_sec_def *sec_def;
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 01639f9a1062..a97e709a29e6 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -241,6 +241,8 @@ LIBBPF_API struct bpf_link *
> > > bpf_program__attach_trace(struct bpf_program *prog);
> > > struct bpf_map;
> > > LIBBPF_API struct bpf_link *bpf_map__attach_struct_ops(struct bpf_map *map);
> > > +LIBBPF_API struct bpf_link *
> > > +bpf_program__attach_lsm(struct bpf_program *prog);
> >
> > nit: put it after attach_trace, so that program attaches and map
> > attaches are grouped together, not intermixed
>
> Done.
>
> >
> > > struct bpf_insn;
> > >
> > > /*
> > > @@ -318,6 +320,7 @@ LIBBPF_API int bpf_program__set_xdp(struct bpf_program *prog);
> > > LIBBPF_API int bpf_program__set_perf_event(struct bpf_program *prog);
> > > LIBBPF_API int bpf_program__set_tracing(struct bpf_program *prog);
> > > LIBBPF_API int bpf_program__set_struct_ops(struct bpf_program *prog);
> > > +LIBBPF_API int bpf_program__set_lsm(struct bpf_program *prog);
> > >
> > > LIBBPF_API enum bpf_prog_type bpf_program__get_type(struct bpf_program *prog);
> > > LIBBPF_API void bpf_program__set_type(struct bpf_program *prog,
> > > @@ -339,6 +342,7 @@ LIBBPF_API bool bpf_program__is_xdp(const struct bpf_program *prog);
> > > LIBBPF_API bool bpf_program__is_perf_event(const struct bpf_program *prog);
> > > LIBBPF_API bool bpf_program__is_tracing(const struct bpf_program *prog);
> > > LIBBPF_API bool bpf_program__is_struct_ops(const struct bpf_program *prog);
> > > +LIBBPF_API bool bpf_program__is_lsm(const struct bpf_program *prog);
> > >
> > > /*
> > > * No need for __attribute__((packed)), all members of 'bpf_map_def'
> > > diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> > > index a19f04e6e3d9..3da0452ce679 100644
> > > --- a/tools/lib/bpf/libbpf.map
> > > +++ b/tools/lib/bpf/libbpf.map
> > > @@ -227,4 +227,7 @@ LIBBPF_0.0.7 {
> > > bpf_program__is_struct_ops;
> > > bpf_program__set_struct_ops;
> > > btf__align_of;
> > > + bpf_program__is_lsm;
> > > + bpf_program__set_lsm;
> > > + bpf_program__attach_lsm;
> >
> > preserve alphabetical order, please
>
> Sure.
>
> >
> > > } LIBBPF_0.0.6;
> >
> > > --
> > > 2.20.1
> > >
^ permalink raw reply
* [PATCH AUTOSEL 4.9 072/251] keys: Timestamp new keys
From: Sasha Levin @ 2020-01-16 17:33 UTC (permalink / raw)
To: linux-kernel, stable
Cc: David Howells, James Morris, Sasha Levin, keyrings,
linux-security-module
In-Reply-To: <20200116173641.22137-1-sashal@kernel.org>
From: David Howells <dhowells@redhat.com>
[ Upstream commit 7c1857bdbdf1e4c541e45eab477ee23ed4333ea4 ]
Set the timestamp on new keys rather than leaving it unset.
Fixes: 31d5a79d7f3d ("KEYS: Do LRU discard in full keyrings")
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.morris@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/keys/key.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/keys/key.c b/security/keys/key.c
index 7276d1a009d4..280b4feccdc0 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -296,6 +296,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
key->gid = gid;
key->perm = perm;
key->restrict_link = restrict_link;
+ key->last_used_at = ktime_get_real_seconds();
if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
key->flags |= 1 << KEY_FLAG_IN_QUOTA;
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.14 100/371] keys: Timestamp new keys
From: Sasha Levin @ 2020-01-16 17:19 UTC (permalink / raw)
To: linux-kernel, stable
Cc: David Howells, James Morris, Sasha Levin, keyrings,
linux-security-module
In-Reply-To: <20200116172403.18149-1-sashal@kernel.org>
From: David Howells <dhowells@redhat.com>
[ Upstream commit 7c1857bdbdf1e4c541e45eab477ee23ed4333ea4 ]
Set the timestamp on new keys rather than leaving it unset.
Fixes: 31d5a79d7f3d ("KEYS: Do LRU discard in full keyrings")
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.morris@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/keys/key.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/keys/key.c b/security/keys/key.c
index 87172f99f73e..17244f5f54c6 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -297,6 +297,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
key->gid = gid;
key->perm = perm;
key->restrict_link = restrict_link;
+ key->last_used_at = ktime_get_real_seconds();
if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
key->flags |= 1 << KEY_FLAG_IN_QUOTA;
--
2.20.1
^ permalink raw reply related
* [PATCH RESEND] MAINTAINERS: fix style in SAFESETID SECURITY MODULE
From: Lukas Bulwahn @ 2020-01-16 18:58 UTC (permalink / raw)
To: Micah Morton
Cc: Joe Perches, linux-security-module, kernel-janitors, linux-kernel,
Lukas Bulwahn
Commit fc5b34a35458 ("Add entry in MAINTAINERS file for SafeSetID LSM")
slips in some formatting with spaces instead of tabs, which
./scripts/checkpatch.pl -f MAINTAINERS complains about:
WARNING: MAINTAINERS entries use one tab after TYPE:
#14394: FILE: MAINTAINERS:14394:
+M: Micah Morton <mortonm@chromium.org>
WARNING: MAINTAINERS entries use one tab after TYPE:
#14395: FILE: MAINTAINERS:14395:
+S: Supported
WARNING: MAINTAINERS entries use one tab after TYPE:
#14396: FILE: MAINTAINERS:14396:
+F: security/safesetid/
WARNING: MAINTAINERS entries use one tab after TYPE:
#14397: FILE: MAINTAINERS:14397:
+F: Documentation/admin-guide/LSM/SafeSetID.rst
Fixes: fc5b34a35458 ("Add entry in MAINTAINERS file for SafeSetID LSM")
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
applies cleanly on v5.5-rc6 and next-20200116
Micah, please pick this patch.
MAINTAINERS | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/MAINTAINERS b/MAINTAINERS
index a66cb722c675..d2aa9db61ab6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14415,10 +14415,10 @@ F: drivers/media/pci/saa7146/
F: include/media/drv-intf/saa7146*
SAFESETID SECURITY MODULE
-M: Micah Morton <mortonm@chromium.org>
-S: Supported
-F: security/safesetid/
-F: Documentation/admin-guide/LSM/SafeSetID.rst
+M: Micah Morton <mortonm@chromium.org>
+S: Supported
+F: security/safesetid/
+F: Documentation/admin-guide/LSM/SafeSetID.rst
SAMSUNG AUDIO (ASoC) DRIVERS
M: Krzysztof Kozlowski <krzk@kernel.org>
--
2.17.1
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 183/671] keys: Timestamp new keys
From: Sasha Levin @ 2020-01-16 16:51 UTC (permalink / raw)
To: linux-kernel, stable
Cc: David Howells, James Morris, Sasha Levin, keyrings,
linux-security-module
In-Reply-To: <20200116165940.10720-1-sashal@kernel.org>
From: David Howells <dhowells@redhat.com>
[ Upstream commit 7c1857bdbdf1e4c541e45eab477ee23ed4333ea4 ]
Set the timestamp on new keys rather than leaving it unset.
Fixes: 31d5a79d7f3d ("KEYS: Do LRU discard in full keyrings")
Signed-off-by: David Howells <dhowells@redhat.com>
Signed-off-by: James Morris <james.morris@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/keys/key.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/security/keys/key.c b/security/keys/key.c
index 249a6da4d277..749a5cf27a19 100644
--- a/security/keys/key.c
+++ b/security/keys/key.c
@@ -297,6 +297,7 @@ struct key *key_alloc(struct key_type *type, const char *desc,
key->gid = gid;
key->perm = perm;
key->restrict_link = restrict_link;
+ key->last_used_at = ktime_get_real_seconds();
if (!(flags & KEY_ALLOC_NOT_IN_QUOTA))
key->flags |= 1 << KEY_FLAG_IN_QUOTA;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: Andrii Nakryiko @ 2020-01-16 19:10 UTC (permalink / raw)
To: KP Singh
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200116124955.GD240584@google.com>
On Thu, Jan 16, 2020 at 4:49 AM KP Singh <kpsingh@chromium.org> wrote:
>
> Thanks for the review Andrii!
>
> I will incorporate the fixes in the next revision.
>
> On 15-Jan 13:19, Andrii Nakryiko wrote:
> > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > >
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > > attr->lsm_hook_index
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > ---
> > > tools/lib/bpf/bpf.c | 6 +-
> > > tools/lib/bpf/bpf.h | 1 +
> > > tools/lib/bpf/libbpf.c | 143 ++++++++++++++++++++++++++++++++++-----
> > > tools/lib/bpf/libbpf.h | 4 ++
> > > tools/lib/bpf/libbpf.map | 3 +
> > > 5 files changed, 138 insertions(+), 19 deletions(-)
> > >
[...]
> >
> > > +{
> > > + struct btf *btf = bpf_find_kernel_btf();
> >
> > ok, it's probably time to do this right. Let's ensure we load kernel
> > BTF just once, keep it inside bpf_object while we need it and then
> > release it after successful load. We are at the point where all the
> > new types of program is loading/releasing kernel BTF for every section
> > and it starts to feel very wasteful.
>
> Sure, will give it a shot in v3.
thanks!
[...]
> >
> > > + if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > + return j + 1;
> >
> > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > but it didn't seem to explain why this index needs to be (unnaturally)
> > 1-based. So asking here first as I'm looking through libbpf changes?
>
> The lsm_hook_idx is one-based as it makes it easy to validate the
> input. If we make it zero-based it's hard to check if the user
> intended to attach to the LSM hook at index 0 or did not set it.
Think about providing FDs. 0 is a valid, though rarely
intended/correct value. Yet we don't make all FD arguments
artificially 1-based, right? This extra +1/-1 translation just makes
for more confusing interface, IMO. If user "accidentally" guessed type
signature of very first hook, well, so be it... If not, BPF verifier
will politely refuse. Seems like enough protection.
>
> We are then up to the verifier to reject the loaded program which
> may or may not match the signature of the hook at lsm_hook_idx = 0.
>
> I will clarify this in the commit log as well.
>
[...]
^ permalink raw reply
* [PATCH AUTOSEL 4.19 009/671] apparmor: don't try to replace stale label in ptrace access check
From: Sasha Levin @ 2020-01-16 16:44 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Jann Horn, John Johansen, Sasha Levin, linux-security-module
In-Reply-To: <20200116165502.8838-1-sashal@kernel.org>
From: Jann Horn <jannh@google.com>
[ Upstream commit 1f8266ff58840d698a1e96d2274189de1bdf7969 ]
As a comment above begin_current_label_crit_section() explains,
begin_current_label_crit_section() must run in sleepable context because
when label_is_stale() is true, aa_replace_current_label() runs, which uses
prepare_creds(), which can sleep.
Until now, the ptrace access check (which runs with a task lock held)
violated this rule.
Also add a might_sleep() assertion to begin_current_label_crit_section(),
because asserts are less likely to be ignored than comments.
Fixes: b2d09ae449ced ("apparmor: move ptrace checks to using labels")
Signed-off-by: Jann Horn <jannh@google.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
security/apparmor/include/cred.h | 2 ++
security/apparmor/lsm.c | 4 ++--
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/include/cred.h b/security/apparmor/include/cred.h
index e287b7d0d4be..265ae6641a06 100644
--- a/security/apparmor/include/cred.h
+++ b/security/apparmor/include/cred.h
@@ -151,6 +151,8 @@ static inline struct aa_label *begin_current_label_crit_section(void)
{
struct aa_label *label = aa_current_raw_label();
+ might_sleep();
+
if (label_is_stale(label)) {
label = aa_get_newest_label(label);
if (aa_replace_current_label(label) == 0)
diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
index 8b8b70620bbe..e3f40c20b9b4 100644
--- a/security/apparmor/lsm.c
+++ b/security/apparmor/lsm.c
@@ -114,13 +114,13 @@ static int apparmor_ptrace_access_check(struct task_struct *child,
struct aa_label *tracer, *tracee;
int error;
- tracer = begin_current_label_crit_section();
+ tracer = __begin_current_label_crit_section();
tracee = aa_get_task_label(child);
error = aa_may_ptrace(tracer, tracee,
(mode & PTRACE_MODE_READ) ? AA_PTRACE_READ
: AA_PTRACE_TRACE);
aa_put_label(tracee);
- end_current_label_crit_section(tracer);
+ __end_current_label_crit_section(tracer);
return error;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2] selinux: remove redundant selinux_nlmsg_perm
From: Paul Moore @ 2020-01-16 19:37 UTC (permalink / raw)
To: Huaisheng Ye
Cc: Stephen Smalley, Eric Paris, James Morris, Serge Hallyn, tyu1,
linux-security-module, selinux, linux-kernel, Huaisheng Ye
In-Reply-To: <20200113150331.34108-1-yehs2007@zoho.com>
On Mon, Jan 13, 2020 at 10:05 AM Huaisheng Ye <yehs2007@zoho.com> wrote:
> From: Huaisheng Ye <yehs1@lenovo.com>
>
> selinux_nlmsg_perm is used for only by selinux_netlink_send. Remove
> the redundant function to simplify the code.
>
> Fix a typo by suggestion from Stephen.
>
> Signed-off-by: Huaisheng Ye <yehs1@lenovo.com>
> Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
> ---
> security/selinux/hooks.c | 73 ++++++++++++++++++++++--------------------------
> 1 file changed, 34 insertions(+), 39 deletions(-)
Merged into selinux/next, thanks!
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH bpf-next v2 05/10] bpf: lsm: BTF API for LSM hooks
From: Andrii Nakryiko @ 2020-01-17 0:28 UTC (permalink / raw)
To: KP Singh
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <20200115171333.28811-6-kpsingh@chromium.org>
On Wed, Jan 15, 2020 at 9:14 AM KP Singh <kpsingh@chromium.org> wrote:
>
> From: KP Singh <kpsingh@google.com>
>
> The BTF API provides information required by the BPF verifier to
> attach eBPF programs to the LSM hooks by using the BTF information of
> two types:
>
> - struct security_hook_heads: This type provides the offset which
> a new dynamically allocated security hook must be attached to.
> - union security_list_options: This provides the information about the
> function prototype required by the hook.
>
> When the program is loaded:
>
> - The verifier receives the index of a member in struct
> security_hook_heads to which a program must be attached as
> prog->aux->lsm_hook_index. The index is one-based for better
> verification.
> - bpf_lsm_type_by_index is used to determine the func_proto of
> the LSM hook and updates prog->aux->attach_func_proto
> - bpf_lsm_head_by_index is used to determine the hlist_head to which
> the BPF program must be attached.
>
> Signed-off-by: KP Singh <kpsingh@google.com>
> ---
> include/linux/bpf_lsm.h | 12 +++++
> security/bpf/Kconfig | 1 +
> security/bpf/hooks.c | 104 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 117 insertions(+)
>
> diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> index 9883cf25241c..a9b4f7b41c65 100644
> --- a/include/linux/bpf_lsm.h
> +++ b/include/linux/bpf_lsm.h
> @@ -19,6 +19,8 @@ extern struct security_hook_heads bpf_lsm_hook_heads;
>
> int bpf_lsm_srcu_read_lock(void);
> void bpf_lsm_srcu_read_unlock(int idx);
> +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 offset);
> +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 id);
>
> #define CALL_BPF_LSM_VOID_HOOKS(FUNC, ...) \
> do { \
> @@ -65,6 +67,16 @@ static inline int bpf_lsm_srcu_read_lock(void)
> return 0;
> }
> static inline void bpf_lsm_srcu_read_unlock(int idx) {}
> +static inline const struct btf_type *bpf_lsm_type_by_index(
> + struct btf *btf, u32 index)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
> +static inline const struct btf_member *bpf_lsm_head_by_index(
> + struct btf *btf, u32 id)
> +{
> + return ERR_PTR(-EOPNOTSUPP);
> +}
>
> #endif /* CONFIG_SECURITY_BPF */
>
> diff --git a/security/bpf/Kconfig b/security/bpf/Kconfig
> index 595e4ad597ae..9438d899b618 100644
> --- a/security/bpf/Kconfig
> +++ b/security/bpf/Kconfig
> @@ -7,6 +7,7 @@ config SECURITY_BPF
> depends on SECURITY
> depends on BPF_SYSCALL
> depends on SRCU
> + depends on DEBUG_INFO_BTF
> help
> This enables instrumentation of the security hooks with
> eBPF programs.
> diff --git a/security/bpf/hooks.c b/security/bpf/hooks.c
> index b123d9cb4cd4..82725611693d 100644
> --- a/security/bpf/hooks.c
> +++ b/security/bpf/hooks.c
> @@ -5,6 +5,8 @@
> */
>
> #include <linux/bpf_lsm.h>
> +#include <linux/bpf.h>
> +#include <linux/btf.h>
> #include <linux/srcu.h>
>
> DEFINE_STATIC_SRCU(security_hook_srcu);
> @@ -18,3 +20,105 @@ void bpf_lsm_srcu_read_unlock(int idx)
> {
> return srcu_read_unlock(&security_hook_srcu, idx);
> }
> +
> +static inline int validate_hlist_head(struct btf *btf, u32 type_id)
> +{
> + s32 hlist_id;
> +
> + hlist_id = btf_find_by_name_kind(btf, "hlist_head", BTF_KIND_STRUCT);
> + if (hlist_id < 0 || hlist_id != type_id)
> + return -EINVAL;
This feels backwards and expensive. You already have type_id you want
to check. Do a quick look up, check type and other attributes, if you
want. There is no need to do linear search for struct named
"hlist_head".
But in reality, you should trust kernel BTF, you already know that you
found correct "security_hook_heads" struct, so its member has to be
hlist_head, no?
> +
> + return 0;
> +}
> +
> +/* Find the BTF representation of the security_hook_heads member for a member
> + * with a given index in struct security_hook_heads.
> + */
> +const struct btf_member *bpf_lsm_head_by_index(struct btf *btf, u32 index)
> +{
> + const struct btf_member *member;
> + const struct btf_type *t;
> + u32 off, i;
> + int ret;
> +
> + t = btf_type_by_name_kind(btf, "security_hook_heads", BTF_KIND_STRUCT);
> + if (WARN_ON_ONCE(IS_ERR(t)))
> + return ERR_CAST(t);
> +
> + for_each_member(i, t, member) {
> + /* We've found the id requested and need to check the
> + * the following:
> + *
> + * - Is it at a valid alignment for struct hlist_head?
> + *
> + * - Is it a valid hlist_head struct?
> + */
> + if (index == i) {
Also not efficient. Check index to be < vlen(t), then member =
btf_type_member(t) + index;
> + off = btf_member_bit_offset(t, member);
> + if (off % 8)
> + /* valid c code cannot generate such btf */
> + return ERR_PTR(-EINVAL);
> + off /= 8;
> +
> + if (off % __alignof__(struct hlist_head))
> + return ERR_PTR(-EINVAL);
> +
> + ret = validate_hlist_head(btf, member->type);
> + if (ret < 0)
> + return ERR_PTR(ret);
> +
> + return member;
This feels a bit over-cautious to double-check this. If
security_hook_heads definition is controlled by kernel sources, then
we could just trust vmlinux BTF?
> + }
> + }
> +
> + return ERR_PTR(-ENOENT);
> +}
> +
> +/* Given an index of a member in security_hook_heads return the
> + * corresponding type for the LSM hook. The members of the union
> + * security_list_options have the same name as the security_hook_heads which
> + * is ensured by the LSM_HOOK_INIT macro defined in include/linux/lsm_hooks.h
> + */
> +const struct btf_type *bpf_lsm_type_by_index(struct btf *btf, u32 index)
> +{
> + const struct btf_member *member, *hook_head = NULL;
> + const struct btf_type *t, *hook_type = NULL;
> + u32 i;
> +
> + hook_head = bpf_lsm_head_by_index(btf, index);
> + if (IS_ERR(hook_head))
> + return ERR_PTR(PTR_ERR(hook_head));
> +
> + t = btf_type_by_name_kind(btf, "security_list_options", BTF_KIND_UNION);
> + if (WARN_ON_ONCE(IS_ERR(t)))
> + return ERR_CAST(t);
btf_type_by_name_kind() is a linear search (at least right now), so it
might be a good idea to cache found type_id's of security_list_options
and security_hook_heads?
> +
> + for_each_member(i, t, member) {
> + if (hook_head->name_off == member->name_off) {
> + /* There should be only one member with the same name
> + * as the LSM hook. This should never really happen
> + * and either indicates malformed BTF or someone trying
> + * trick the LSM.
> + */
> + if (WARN_ON(hook_type))
> + return ERR_PTR(-EINVAL);
> +
> + hook_type = btf_type_by_id(btf, member->type);
> + if (unlikely(!hook_type))
> + return ERR_PTR(-EINVAL);
> +
> + if (!btf_type_is_ptr(hook_type))
> + return ERR_PTR(-EINVAL);
> + }
> + }
> +
> + if (!hook_type)
> + return ERR_PTR(-ENOENT);
> +
> + t = btf_type_by_id(btf, hook_type->type);
> + if (unlikely(!t))
> + return ERR_PTR(-EINVAL);
why not do this inside the loop when you find correct member and not
continue processing all the fields?
> +
> + return t;
> +}
> --
> 2.20.1
>
^ permalink raw reply
* Re: [RFC PATCH 2/2] dm-crypt: Use any key type which is registered
From: Maik Otto @ 2020-01-17 11:52 UTC (permalink / raw)
To: Franck LENORMAND, linux-security-module, keyrings
Cc: horia.geanta, silvano.dininno, agk, snitzer, dm-devel, dhowells,
jmorris, serge
In-Reply-To: <1551456599-10603-3-git-send-email-franck.lenormand@nxp.com>
Hi
little bug fix, because this version chrashed with access violation
on an i.MX6Quad with a Mainline Kernel 4.19.88
> There was only 2 key_type supported by dm-crypt which limits other
> implementations.
>
> This patch allows to use any key_type which is registered obtaining
> the key_type from key framework.
>
> This also remove the compilation dependency between dm-crypt and
> key implementations.
>
> Signed-off-by: Franck LENORMAND <franck.lenormand@nxp.com>
> ---
> drivers/md/dm-crypt.c | 11 ++++++-----
> include/linux/key-type.h | 2 ++
> security/keys/key.c | 42 ++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index dd538e6..e25efc2 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -35,6 +35,7 @@
> #include <crypto/authenc.h>
> #include <linux/rtnetlink.h> /* for struct rtattr and RTA macros only */
> #include <keys/user-type.h>
> +#include <linux/key-type.h>
>
> #include <linux/device-mapper.h>
>
> @@ -2010,6 +2011,7 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
> int ret;
> struct key *key;
> const struct user_key_payload *ukp;
> + struct key_type *type;
>
> /*
> * Reject key_string with whitespace. dm core currently lacks code for
> @@ -2025,16 +2027,15 @@ static int crypt_set_keyring_key(struct crypt_config *cc, const char *key_string
> if (!key_desc || key_desc == key_string || !strlen(key_desc + 1))
> return -EINVAL;
>
> - if (strncmp(key_string, "logon:", key_desc - key_string + 1) &&
> - strncmp(key_string, "user:", key_desc - key_string + 1))
> - return -EINVAL;
> + type = get_key_type(key_string, key_desc - key_string);
> + if (!type)
> + return -ENOENT;
>
> new_key_string = kstrdup(key_string, GFP_KERNEL);
> if (!new_key_string)
> return -ENOMEM;
>
> - key = request_key(key_string[0] == 'l' ? &key_type_logon : &key_type_user,
> - key_desc + 1, NULL);
> + key = request_key(type, key_desc + 1, NULL);
> if (IS_ERR(key)) {
> kzfree(new_key_string);
> return PTR_ERR(key);
> diff --git a/include/linux/key-type.h b/include/linux/key-type.h
> index bc9af55..2b2167b 100644
> --- a/include/linux/key-type.h
> +++ b/include/linux/key-type.h
> @@ -176,6 +176,8 @@ extern struct key_type key_type_keyring;
> extern int register_key_type(struct key_type *ktype);
> extern void unregister_key_type(struct key_type *ktype);
>
> +extern struct key_type *get_key_type(const char *type_name, size_t string_size);
> +
> extern int key_payload_reserve(struct key *key, size_t datalen);
> extern int key_instantiate_and_link(struct key *key,
> const void *data,
> diff --git a/security/keys/key.c b/security/keys/key.c
> index 44a80d6..ef76114 100644
> --- a/security/keys/key.c
> +++ b/security/keys/key.c
> @@ -1156,6 +1156,48 @@ void unregister_key_type(struct key_type *ktype)
> }
> EXPORT_SYMBOL(unregister_key_type);
>
> +/**
> + * get_key_type - Get the type of key using its name
> + * @type_name: Name of the key type to get
> + * @string_size: Size of the string to match
> + *
> + * The functions support null ended string (string_size == 0) as well as
> + * pointer on a string matching a number of characters (string_size > 0)
> + *
> + * Returns a pointer on the key type if successful, -ENOENT if the key type
> + * is not registered.
> + */
> +struct key_type *get_key_type(const char *type_name, size_t string_size)
> +{
> + struct key_type *p;
> + struct key_type *ktype = ERR_PTR(-ENOENT);
> +
> + if (!type_name)
> + return ktype;
> +
> + down_write(&key_types_sem);
down_write(&key_types_sem); changed to down_read(&key_types_sem);
> +
> + /* Search the key type in the list */
> + list_for_each_entry(p, &key_types_list, link) {
> + if (string_size) {
> + if (strncmp(p->name, type_name, string_size) == 0) {
> + ktype = p;
> + break;
> + }
> + } else {
> + if (strcmp(p->name, type_name) == 0) {
> + ktype = p;
> + break;
> + }
> + }
> + }
> +
> + up_read(&key_types_sem);
> +
> + return ktype;
> +}
> +EXPORT_SYMBOL(get_key_type);
> +
> /*
> * Initialise the key management state.
> */
>
^ permalink raw reply
* Re: [PATCH v2 1/2] security/keys/secure_key: Adds the secure key support based on CAAM.
From: Maik Otto @ 2020-01-17 11:52 UTC (permalink / raw)
To: Udit Agarwal, dhowells, zohar, jmorris, serge, linux-integrity,
keyrings, linux-security-module, jarkko.sakkinen
Cc: sahil.malhotra, ruchika.gupta, horia.geanta, aymen.sghaier
In-Reply-To: <20180723111432.26830-1-udit.agarwal@nxp.com>
Hi
I tested both patches in combination with
[RFC,2/2] dm-crypt: Use any key type which is registered from
https://patchwork.kernel.org/patch/10835633/ with bug fix
and an i.MX6Quad (logged device) with Mainline Kernel 4.19.88
The following tests were successful:
- key generation with CAAM
keyctl add secure kmk-master "new 64" @s
- export and import key blob with same controller
keyctl pipe 332995568 > secure_key.blob
reboot device
keyctl add secure kmk-master "load `cat secure_key.blob`" @s
- import keyblob with an other cpu and same keys for secure boot
caam_jr 2102000.jr1: caam op done err: 20000c1a
[ 185.788931] secure_key: key_blob decap fail (-22)
add_key: Invalid argument
=> failing import was expected: pass
- use key from keyring in dmcrypt with an sd-card
dmsetup create test --table "0 106496 crypt aes-xts-plain64
:64:secure:kmk-master 0 /dev/mmcblk0p3 0"
write,read reboot and read again
Tested-by: Maik Otto<m.otto@phytec.de>
Am 23.07.2018 um 13:14 schrieb Udit Agarwal:
> Secure keys are derieved using CAAM crypto block.
>
> Secure keys derieved are the random number symmetric keys from CAAM.
> Blobs corresponding to the key are formed using CAAM. User space
> will only be able to view the blob of the key.
>
> Signed-off-by: Udit Agarwal <udit.agarwal@nxp.com>
> Reviewed-by: Sahil Malhotra <sahil.malhotra@nxp.com>
> ---
> Changes in v2:
> Changes security/keys/Makefile to compile securekey module
> when CONFIG_SECURE_KEY is enabled as module.
>
> Documentation/security/keys/secure-key.rst | 67 +++
> MAINTAINERS | 11 +
> include/keys/secure-type.h | 33 ++
> security/keys/Kconfig | 11 +
> security/keys/Makefile | 5 +
> security/keys/secure_key.c | 339 ++++++++++++
> security/keys/securekey_desc.c | 608 +++++++++++++++++++++
> security/keys/securekey_desc.h | 141 +++++
> 8 files changed, 1215 insertions(+)
> create mode 100644 Documentation/security/keys/secure-key.rst
> create mode 100644 include/keys/secure-type.h
> create mode 100644 security/keys/secure_key.c
> create mode 100644 security/keys/securekey_desc.c
> create mode 100644 security/keys/securekey_desc.h
>
> diff --git a/Documentation/security/keys/secure-key.rst b/Documentation/security/keys/secure-key.rst
> new file mode 100644
> index 000000000000..a33ffd09d7bd
> --- /dev/null
> +++ b/Documentation/security/keys/secure-key.rst
> @@ -0,0 +1,67 @@
> +==========
> +Secure Key
> +==========
> +
> +Secure key is the new type added to kernel key ring service.
> +Secure key is a symmetric type key of minimum length 32 bytes
> +and with maximum possible length to be 128 bytes. It is produced
> +in kernel using the CAAM crypto engine. Userspace can only see
> +the blob for the corresponding key. All the blobs are displayed
> +or loaded in hex ascii.
> +
> +Secure key can be created on platforms which supports CAAM
> +hardware block. Secure key can also be used as a master key to
> +create the encrypted keys along with the existing key types in
> +kernel.
> +
> +Secure key uses CAAM hardware to generate the key and blobify its
> +content for userspace. Generated blobs are tied up with the hardware
> +secret key stored in CAAM, hence the same blob will not be able to
> +de-blobify with the different secret key on another machine.
> +
> +Usage::
> +
> + keyctl add secure <name> "new <keylen>" <ring>
> + keyctl load secure <name> "load <hex_blob>" <ring>
> + keyctl print <key_id>
> +
> +"keyctl add secure" option will create the random data of the
> +specified key len using CAAM and store it as a key in kernel.
> +Key contents will be displayed as blobs to the user in hex ascii.
> +User can input key len from 32 bytes to 128 bytes.
> +
> +"keyctl load secure" option will load the blob contents. In kernel,
> +key will be deirved using input blob and CAAM, along with the secret
> +key stored in CAAM.
> +
> +"keyctl print" will return the hex string of the blob corresponding to
> +key_id. Returned blob will be of key_len + 48 bytes. Extra 48 bytes are
> +the header bytes added by the CAAM.
> +
> +Example of secure key usage::
> +
> +1. Create the secure key with name kmk-master of length 32 bytes::
> +
> + $ keyctl add secure kmk-master "new 32" @u
> + 46001928
> +
> + $keyctl show
> + Session Keyring
> + 1030783626 --alswrv 0 65534 keyring: _uid_ses.0
> + 695927745 --alswrv 0 65534 \_ keyring: _uid.0
> + 46001928 --als-rv 0 0 \_ secure: kmk-master
> +
> +2. Print the blob contents for the kmk-master key::
> +
> + $ keyctl print 46001928
> + d9743445b640f3d59c1670dddc0bc9c2
> + 34fc9aab7dd05c965e6120025012f029b
> + 07faa4776c4f6ed02899e35a135531e9a
> + 6e5c2b51132f9d5aef28f68738e658296
> + 3fe583177cfe50d2542b659a13039
> +
> + $ keyctl pipe 46001928 > secure_key.blob
> +
> +3. Load the blob in the user key ring::
> +
> + $ keyctl load secure kmk-master "load 'cat secure_key.blob'" @u
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9fd5e8808208..654be2ee4b0a 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7939,6 +7939,17 @@ F: include/keys/trusted-type.h
> F: security/keys/trusted.c
> F: security/keys/trusted.h
>
> +KEYS-SECURE
> +M: Udit Agarwal <udit.agarwal@nxp.com>
> +R: Sahil Malhotra <sahil.malhotra@nxp.com>
> +L: linux-security-module@vger.kernel.org
> +L: keyrings@vger.kernel.org
> +S: Supported
> +F: include/keys/secure-type.h
> +F: security/keys/secure_key.c
> +F: security/keys/securekey_desc.c
> +F: security/keys/securekey_desc.h
> +
> KEYS/KEYRINGS:
> M: David Howells <dhowells@redhat.com>
> L: keyrings@vger.kernel.org
> diff --git a/include/keys/secure-type.h b/include/keys/secure-type.h
> new file mode 100644
> index 000000000000..5b7a5f144e41
> --- /dev/null
> +++ b/include/keys/secure-type.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2018 NXP.
> + *
> + */
> +
> +#ifndef _KEYS_SECURE_TYPE_H
> +#define _KEYS_SECURE_TYPE_H
> +
> +#include <linux/key.h>
> +#include <linux/rcupdate.h>
> +
> +/* Minimum key size to be used is 32 bytes and maximum key size fixed
> + * is 128 bytes.
> + * Blob size to be kept is Maximum key size + blob header added by CAAM.
> + */
> +
> +#define MIN_KEY_SIZE 32
> +#define MAX_KEY_SIZE 128
> +#define BLOB_HEADER_SIZE 48
> +
> +#define MAX_BLOB_SIZE (MAX_KEY_SIZE + BLOB_HEADER_SIZE)
> +
> +struct secure_key_payload {
> + struct rcu_head rcu;
> + unsigned int key_len;
> + unsigned int blob_len;
> + unsigned char key[MAX_KEY_SIZE + 1];
> + unsigned char blob[MAX_BLOB_SIZE];
> +};
> +
> +extern struct key_type key_type_secure;
> +#endif
> diff --git a/security/keys/Kconfig b/security/keys/Kconfig
> index 6462e6654ccf..7eb138b5a54f 100644
> --- a/security/keys/Kconfig
> +++ b/security/keys/Kconfig
> @@ -71,6 +71,17 @@ config TRUSTED_KEYS
>
> If you are unsure as to whether this is required, answer N.
>
> +config SECURE_KEYS
> + tristate "SECURE_KEYS"
> + depends on KEYS && CRYPTO_DEV_FSL_CAAM && CRYPTO_DEV_FSL_CAAM_JR
> + help
> + This option provide support for creating secure-type key and blobs
> + in kernel. Secure keys are random number symmetric keys generated
> + from CAAM. The CAAM creates the blobs for the random key.
> + Userspace will only be able to see the blob.
> +
> + If you are unsure as to whether this is required, answer N.
> +
> config ENCRYPTED_KEYS
> tristate "ENCRYPTED KEYS"
> depends on KEYS
> diff --git a/security/keys/Makefile b/security/keys/Makefile
> index ef1581b337a3..eec1c350e922 100644
> --- a/security/keys/Makefile
> +++ b/security/keys/Makefile
> @@ -28,4 +28,9 @@ obj-$(CONFIG_KEY_DH_OPERATIONS) += dh.o
> #
> obj-$(CONFIG_BIG_KEYS) += big_key.o
> obj-$(CONFIG_TRUSTED_KEYS) += trusted.o
> +CFLAGS_secure_key.o += -I$(obj)/../../drivers/crypto/caam/
> +CFLAGS_securekey_desc.o += -I$(obj)/../../drivers/crypto/caam/
> +obj-$(CONFIG_SECURE_KEYS) += securekey.o
> +securekey-y := securekey_desc.o \
> + secure_key.o
> obj-$(CONFIG_ENCRYPTED_KEYS) += encrypted-keys/
> diff --git a/security/keys/secure_key.c b/security/keys/secure_key.c
> new file mode 100644
> index 000000000000..ec8ad4394549
> --- /dev/null
> +++ b/security/keys/secure_key.c
> @@ -0,0 +1,339 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (C) 2018 NXP
> + * Secure key is generated using NXP CAAM hardware block. CAAM generates the
> + * random number (used as a key) and creates its blob for the user.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/parser.h>
> +#include <linux/string.h>
> +#include <linux/key-type.h>
> +#include <linux/rcupdate.h>
> +#include <keys/secure-type.h>
> +#include <linux/completion.h>
> +
> +#include "securekey_desc.h"
> +
> +static const char hmac_alg[] = "hmac(sha1)";
> +static const char hash_alg[] = "sha1";
> +
> +static struct crypto_shash *hashalg;
> +static struct crypto_shash *hmacalg;
> +
> +enum {
> + error = -1,
> + new_key,
> + load_blob,
> +};
> +
> +static const match_table_t key_tokens = {
> + {new_key, "new"},
> + {load_blob, "load"},
> + {error, NULL}
> +};
> +
> +static struct secure_key_payload *secure_payload_alloc(struct key *key)
> +{
> + struct secure_key_payload *sec_key = NULL;
> + int ret = 0;
> +
> + ret = key_payload_reserve(key, sizeof(*sec_key));
> + if (ret < 0)
> + goto out;
> +
> + sec_key = kzalloc(sizeof(*sec_key), GFP_KERNEL);
> + if (!sec_key)
> + goto out;
> +
> +out:
> + return sec_key;
> +}
> +
> +/*
> + * parse_inputdata - parse the keyctl input data and fill in the
> + * payload structure for key or its blob.
> + * param[in]: data pointer to the data to be parsed for creating key.
> + * param[in]: p pointer to secure key payload structure to fill parsed data
> + * On success returns 0, otherwise -EINVAL.
> + */
> +static int parse_inputdata(char *data, struct secure_key_payload *p)
> +{
> + substring_t args[MAX_OPT_ARGS];
> + long keylen = 0;
> + int ret = -EINVAL;
> + int key_cmd = -EINVAL;
> + char *c = NULL;
> +
> + c = strsep(&data, " \t");
> + if (!c) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* Get the keyctl command i.e. new_key or load_blob etc */
> + key_cmd = match_token(c, key_tokens, args);
> +
> + switch (key_cmd) {
> + case new_key:
> + /* first argument is key size */
> + c = strsep(&data, " \t");
> + if (!c) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = kstrtol(c, 10, &keylen);
> + if (ret < 0 || keylen < MIN_KEY_SIZE ||
> + keylen > MAX_KEY_SIZE) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + p->key_len = keylen;
> + ret = new_key;
> +
> + break;
> + case load_blob:
> + /* first argument is blob data for CAAM*/
> + c = strsep(&data, " \t");
> + if (!c) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + /* Blob_len = No of characters in blob/2 */
> + p->blob_len = strlen(c) / 2;
> + if (p->blob_len > MAX_BLOB_SIZE) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + ret = hex2bin(p->blob, c, p->blob_len);
> + if (ret < 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + ret = load_blob;
> +
> + break;
> + case error:
> + ret = -EINVAL;
> + break;
> + }
> +
> +out:
> + return ret;
> +}
> +
> +/*
> + * secure_instantiate - create a new secure type key.
> + * Supports the operation to generate a new key. A random number
> + * is generated from CAAM as key data and the corresponding red blob
> + * is formed and stored as key_blob.
> + * Also supports the operation to load the blob and key is derived using
> + * that blob from CAAM.
> + * On success, return 0. Otherwise return errno.
> + */
> +static int secure_instantiate(struct key *key,
> + struct key_preparsed_payload *prep)
> +{
> + struct secure_key_payload *payload = NULL;
> + size_t datalen = prep->datalen;
> + char *data = NULL;
> + int key_cmd = 0;
> + int ret = 0;
> + enum sk_req_type sk_op_type;
> + struct device *dev = NULL;
> +
> + if (datalen <= 0 || datalen > 32767 || !prep->data) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + data = kmalloc(datalen + 1, GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + memcpy(data, prep->data, datalen);
> + data[datalen] = '\0';
> +
> + payload = secure_payload_alloc(key);
> + if (!payload) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Allocate caam job ring for operation to be performed from CAAM */
> + dev = caam_jr_alloc();
> + if (!dev) {
> + pr_info("caam_jr_alloc failed\n");
> + ret = -ENODEV;
> + goto out;
> + }
> +
> + key_cmd = parse_inputdata(data, payload);
> + if (key_cmd < 0) {
> + ret = key_cmd;
> + goto out;
> + }
> +
> + switch (key_cmd) {
> + case load_blob:
> + /*
> + * Red blob decryption to be done for load operation
> + * to derive the key.
> + */
> + sk_op_type = sk_red_blob_dec;
> + ret = key_deblob(payload, sk_op_type, dev);
> + if (ret != 0) {
> + pr_info("secure_key: key_blob decap fail (%d)\n", ret);
> + goto out;
> + }
> + break;
> + case new_key:
> + /* Get Random number from caam of the specified length */
> + sk_op_type = sk_get_random;
> + ret = caam_get_random(payload, sk_op_type, dev);
> + if (ret != 0) {
> + pr_info("secure_key: get_random fail (%d)\n", ret);
> + goto out;
> + }
> +
> + /* Generate red blob of key random bytes with CAAM */
> + sk_op_type = sk_red_blob_enc;
> + ret = key_blob(payload, sk_op_type, dev);
> + if (ret != 0) {
> + pr_info("secure_key: key_blob encap fail (%d)\n", ret);
> + goto out;
> + }
> + break;
> + default:
> + ret = -EINVAL;
> + goto out;
> + }
> +out:
> + if (data)
> + kzfree(data);
> + if (dev)
> + caam_jr_free(dev);
> +
> + if (!ret)
> + rcu_assign_keypointer(key, payload);
> + else
> + kzfree(payload);
> +
> + return ret;
> +}
> +
> +/*
> + * secure_read - copy the blob data to userspace in hex.
> + * param[in]: key pointer to key struct
> + * param[in]: buffer pointer to user data for creating key
> + * param[in]: buflen is the length of the buffer
> + * On success, return to userspace the secure key data size.
> + */
> +static long secure_read(const struct key *key, char __user *buffer,
> + size_t buflen)
> +{
> + const struct secure_key_payload *p = NULL;
> + char *ascii_buf;
> + char *bufp;
> + int i;
> +
> + p = dereference_key_locked(key);
> + if (!p)
> + return -EINVAL;
> +
> + if (buffer && buflen >= 2 * p->blob_len) {
> + ascii_buf = kmalloc(2 * p->blob_len, GFP_KERNEL);
> + if (!ascii_buf)
> + return -ENOMEM;
> +
> + bufp = ascii_buf;
> + for (i = 0; i < p->blob_len; i++)
> + bufp = hex_byte_pack(bufp, p->blob[i]);
> + if (copy_to_user(buffer, ascii_buf, 2 * p->blob_len) != 0) {
> + kzfree(ascii_buf);
> + return -EFAULT;
> + }
> + kzfree(ascii_buf);
> + }
> + return 2 * p->blob_len;
> +}
> +
> +/*
> + * secure_destroy - clear and free the key's payload
> + */
> +static void secure_destroy(struct key *key)
> +{
> + kzfree(key->payload.data[0]);
> +}
> +
> +struct key_type key_type_secure = {
> + .name = "secure",
> + .instantiate = secure_instantiate,
> + .destroy = secure_destroy,
> + .read = secure_read,
> +};
> +EXPORT_SYMBOL_GPL(key_type_secure);
> +
> +static void secure_shash_release(void)
> +{
> + if (hashalg)
> + crypto_free_shash(hashalg);
> + if (hmacalg)
> + crypto_free_shash(hmacalg);
> +}
> +
> +static int __init secure_shash_alloc(void)
> +{
> + int ret;
> +
> + hmacalg = crypto_alloc_shash(hmac_alg, 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(hmacalg)) {
> + pr_info("secure_key: could not allocate crypto %s\n",
> + hmac_alg);
> + return PTR_ERR(hmacalg);
> + }
> +
> + hashalg = crypto_alloc_shash(hash_alg, 0, CRYPTO_ALG_ASYNC);
> + if (IS_ERR(hashalg)) {
> + pr_info("secure_key: could not allocate crypto %s\n",
> + hash_alg);
> + ret = PTR_ERR(hashalg);
> + goto hashalg_fail;
> + }
> +
> + return 0;
> +
> +hashalg_fail:
> + crypto_free_shash(hmacalg);
> + return ret;
> +}
> +
> +static int __init init_secure_key(void)
> +{
> + int ret;
> +
> + ret = secure_shash_alloc();
> + if (ret < 0)
> + return ret;
> +
> + ret = register_key_type(&key_type_secure);
> + if (ret < 0)
> + secure_shash_release();
> + return ret;
> +}
> +
> +static void __exit cleanup_secure_key(void)
> +{
> + secure_shash_release();
> + unregister_key_type(&key_type_secure);
> +}
> +
> +late_initcall(init_secure_key);
> +module_exit(cleanup_secure_key);
> +
> +MODULE_LICENSE("GPL");
> diff --git a/security/keys/securekey_desc.c b/security/keys/securekey_desc.c
> new file mode 100644
> index 000000000000..03a57cb93a74
> --- /dev/null
> +++ b/security/keys/securekey_desc.c
> @@ -0,0 +1,608 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018 NXP
> + *
> + */
> +
> +#include <keys/secure-type.h>
> +#include "securekey_desc.h"
> +
> +/* key modifier for blob encapsulation & decapsulation descriptor */
> +u8 key_modifier[] = "SECURE_KEY";
> +u32 key_modifier_len = 10;
> +
> +void caam_sk_rng_desc(struct sk_req *skreq, struct sk_desc *skdesc)
> +{
> + struct sk_fetch_rnd_data *fetch_rnd_data = NULL;
> + struct random_desc *rnd_desc = NULL;
> + size_t len = 0;
> + u32 *desc = skreq->hwdesc;
> +
> + init_job_desc(desc, 0);
> +
> + fetch_rnd_data = &skreq->req_u.sk_fetch_rnd_data;
> + rnd_desc = &skdesc->dma_u.random_descp;
> + len = fetch_rnd_data->key_len;
> +
> + /* command 0x82500000 */
> + append_cmd(desc, CMD_OPERATION | OP_TYPE_CLASS1_ALG |
> + OP_ALG_ALGSEL_RNG);
> + /* command 0x60340000 | len */
> + append_cmd(desc, CMD_FIFO_STORE | FIFOST_TYPE_RNGSTORE | len);
> + append_ptr(desc, rnd_desc->rnd_data);
> +}
> +
> +void caam_sk_redblob_encap_desc(struct sk_req *skreq, struct sk_desc *skdesc)
> +{
> + struct redblob_encap_desc *red_blob_desc =
> + &skdesc->dma_u.redblob_encapdesc;
> + struct sk_red_blob_encap *red_blob_req =
> + &skreq->req_u.sk_red_blob_encap;
> + u32 *desc = skreq->hwdesc;
> +
> + init_job_desc(desc, 0);
> +
> + /* Load class 2 key with key modifier. */
> + append_key_as_imm(desc, key_modifier, key_modifier_len,
> + key_modifier_len, CLASS_2 | KEY_DEST_CLASS_REG);
> +
> + /* SEQ IN PTR Command. */
> + append_seq_in_ptr(desc, red_blob_desc->in_data, red_blob_req->data_sz,
> + 0);
> +
> + /* SEQ OUT PTR Command. */
> + append_seq_out_ptr(desc, red_blob_desc->redblob,
> + red_blob_req->redblob_sz, 0);
> +
> + /* RedBlob encapsulation PROTOCOL Command. */
> + append_operation(desc, OP_TYPE_ENCAP_PROTOCOL | OP_PCLID_BLOB);
> +}
> +
> +/* void caam_sk_redblob_decap_desc(struct sk_req *skreq, struct sk_desc *skdesc)
> + * brief CAAM Descriptor creator from redblob to plaindata.
> + * param[in] skreq Pointer to secure key request structure
> + * param[in] skdesc Pointer to secure key descriptor structure
> + */
> +void caam_sk_redblob_decap_desc(struct sk_req *skreq, struct sk_desc *skdesc)
> +{
> + struct redblob_decap_desc *red_blob_desc =
> + &skdesc->dma_u.redblob_decapdesc;
> + struct sk_red_blob_decap *red_blob_req =
> + &skreq->req_u.sk_red_blob_decap;
> + u32 *desc = skreq->hwdesc;
> +
> + init_job_desc(desc, 0);
> +
> + /* Load class 2 key with key modifier. */
> + append_key_as_imm(desc, key_modifier, key_modifier_len,
> + key_modifier_len, CLASS_2 | KEY_DEST_CLASS_REG);
> +
> + /* SEQ IN PTR Command. */
> + append_seq_in_ptr(desc, red_blob_desc->redblob,
> + red_blob_req->redblob_sz, 0);
> +
> + /* SEQ OUT PTR Command. */
> + append_seq_out_ptr(desc, red_blob_desc->out_data,
> + red_blob_req->data_sz, 0);
> +
> + /* RedBlob decapsulation PROTOCOL Command. */
> + append_operation(desc, OP_TYPE_DECAP_PROTOCOL | OP_PCLID_BLOB);
> +}
> +
> +/* int caam_sk_get_random_map(struct device *dev, struct sk_req *req,
> + * struct sk_desc *skdesc)
> + * brief DMA map the buffer virtual pointers to physical address.
> + * param[in] dev Pointer to job ring device structure
> + * param[in] req Pointer to secure key request structure
> + * param[in] skdesc Pointer to secure key descriptor structure
> + * return 0 on success, error value otherwise.
> + */
> +int caam_sk_get_random_map(struct device *dev, struct sk_req *req,
> + struct sk_desc *skdesc)
> +{
> + struct sk_fetch_rnd_data *fetch_rnd_data;
> + struct random_desc *rnd_desc;
> +
> + fetch_rnd_data = &req->req_u.sk_fetch_rnd_data;
> + rnd_desc = &skdesc->dma_u.random_descp;
> +
> + rnd_desc->rnd_data = dma_map_single(dev, fetch_rnd_data->data,
> + fetch_rnd_data->key_len, DMA_FROM_DEVICE);
> +
> + if (dma_mapping_error(dev, rnd_desc->rnd_data)) {
> + dev_err(dev, "Unable to map memory\n");
> + goto sk_random_map_fail;
> + }
> + return 0;
> +
> +sk_random_map_fail:
> + return -ENOMEM;
> +}
> +
> +/* int caam_sk_redblob_encap_map(struct device *dev, struct sk_req *req,
> + * struct sk_desc *skdesc)
> + * brief DMA map the buffer virtual pointers to physical address.
> + * param[in] dev Pointer to job ring device structure
> + * param[in] req Pointer to secure key request structure
> + * param[in] skdesc Pointer to secure key descriptor structure
> + * return 0 on success, error value otherwise.
> + */
> +int caam_sk_redblob_encap_map(struct device *dev, struct sk_req *req,
> + struct sk_desc *skdesc)
> +{
> + struct sk_red_blob_encap *red_blob_encap;
> + struct redblob_encap_desc *red_blob_desc;
> +
> + red_blob_encap = &req->req_u.sk_red_blob_encap;
> + red_blob_desc = &skdesc->dma_u.redblob_encapdesc;
> +
> + red_blob_desc->in_data = dma_map_single(dev, red_blob_encap->data,
> + red_blob_encap->data_sz, DMA_TO_DEVICE);
> + if (dma_mapping_error(dev, red_blob_desc->in_data)) {
> + dev_err(dev, "Unable to map memory\n");
> + goto sk_data_fail;
> + }
> +
> + red_blob_desc->redblob = dma_map_single(dev, red_blob_encap->redblob,
> + red_blob_encap->redblob_sz, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, red_blob_desc->redblob)) {
> + dev_err(dev, "Unable to map memory\n");
> + goto sk_redblob_fail;
> + }
> +
> + return 0;
> +
> +sk_redblob_fail:
> + dma_unmap_single(dev, red_blob_desc->in_data, red_blob_encap->data_sz,
> + DMA_TO_DEVICE);
> +sk_data_fail:
> + return -ENOMEM;
> +}
> +
> +/* static int caam_sk_redblob_decap_map(struct device *dev,
> + * struct sk_req *req,
> + * struct sk_desc *skdesc)
> + * brief DMA map the buffer virtual pointers to physical address.
> + * param[in] dev Pointer to job ring device structure
> + * param[in] req Pointer to secure key request structure
> + * param[in] skdesc Pointer to secure key descriptor structure
> + * return 0 on success, error value otherwise.
> + */
> +int caam_sk_redblob_decap_map(struct device *dev, struct sk_req *req,
> + struct sk_desc *skdesc)
> +{
> + struct sk_red_blob_decap *red_blob_decap;
> + struct redblob_decap_desc *red_blob_desc;
> +
> + red_blob_decap = &req->req_u.sk_red_blob_decap;
> + red_blob_desc = &skdesc->dma_u.redblob_decapdesc;
> +
> + red_blob_desc->redblob = dma_map_single(dev, red_blob_decap->redblob,
> + red_blob_decap->redblob_sz, DMA_TO_DEVICE);
> + if (dma_mapping_error(dev, red_blob_desc->redblob)) {
> + dev_err(dev, "Unable to map memory\n");
> + goto sk_redblob_fail;
> + }
> +
> + red_blob_desc->out_data = dma_map_single(dev, red_blob_decap->data,
> + red_blob_decap->data_sz, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, red_blob_desc->out_data)) {
> + dev_err(dev, "Unable to map memory\n");
> + goto sk_data_fail;
> + }
> +
> + return 0;
> +
> +sk_data_fail:
> + dma_unmap_single(dev, red_blob_desc->redblob,
> + red_blob_decap->redblob_sz, DMA_TO_DEVICE);
> +sk_redblob_fail:
> + return -ENOMEM;
> +}
> +
> +/* @fn void securekey_unmap(struct device *dev,
> + * struct sk_desc *skdesc, struct sk_req *req)
> + * @brief DMA unmap the buffer pointers.
> + * @param[in] dev Pointer to job ring device structure
> + * @param[in] skdesc Pointer to secure key descriptor structure
> + * @param[in] req Pointer to secure key request structure
> + */
> +void securekey_unmap(struct device *dev,
> + struct sk_desc *skdesc, struct sk_req *req)
> +{
> +
> + switch (req->type) {
> + case sk_get_random:
> + {
> + struct sk_fetch_rnd_data *fetch_rnd_data;
> + struct random_desc *rnd_desc;
> +
> + fetch_rnd_data = &req->req_u.sk_fetch_rnd_data;
> + rnd_desc = &skdesc->dma_u.random_descp;
> +
> + /* Unmap Descriptor buffer pointers. */
> + dma_unmap_single(dev, rnd_desc->rnd_data,
> + fetch_rnd_data->key_len,
> + DMA_FROM_DEVICE);
> + break;
> + }
> + case sk_red_blob_enc:
> + {
> + struct sk_red_blob_encap *red_blob_encap;
> + struct redblob_encap_desc *red_blob_desc;
> +
> + red_blob_encap = &req->req_u.sk_red_blob_encap;
> + red_blob_desc = &skdesc->dma_u.redblob_encapdesc;
> +
> + /* Unmap Descriptor buffer pointers. */
> + dma_unmap_single(dev, red_blob_desc->in_data,
> + red_blob_encap->data_sz,
> + DMA_TO_DEVICE);
> +
> + dma_unmap_single(dev, red_blob_desc->redblob,
> + red_blob_encap->redblob_sz,
> + DMA_FROM_DEVICE);
> +
> + break;
> + }
> + case sk_red_blob_dec:
> + {
> + struct sk_red_blob_decap *red_blob_decap;
> + struct redblob_decap_desc *red_blob_desc;
> +
> + red_blob_decap = &req->req_u.sk_red_blob_decap;
> + red_blob_desc = &skdesc->dma_u.redblob_decapdesc;
> +
> + /* Unmap Descriptor buffer pointers. */
> + dma_unmap_single(dev, red_blob_desc->redblob,
> + red_blob_decap->redblob_sz,
> + DMA_TO_DEVICE);
> +
> + dma_unmap_single(dev, red_blob_desc->out_data,
> + red_blob_decap->data_sz,
> + DMA_FROM_DEVICE);
> +
> + break;
> + }
> + default:
> + dev_err(dev, "Unable to find request type\n");
> + break;
> + }
> + kfree(skdesc);
> +}
> +
> +/* int caam_securekey_desc_init(struct device *dev, struct sk_req *req)
> + * brief CAAM Descriptor creator for secure key operations.
> + * param[in] dev Pointer to job ring device structure
> + * param[in] req Pointer to secure key request structure
> + * return 0 on success, error value otherwise.
> + */
> +int caam_securekey_desc_init(struct device *dev, struct sk_req *req)
> +{
> + struct sk_desc *skdesc = NULL;
> + int ret = 0;
> +
> + switch (req->type) {
> + case sk_get_random:
> + {
> + skdesc = kmalloc(sizeof(*skdesc), GFP_DMA);
> + if (!skdesc) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + skdesc->req_type = req->type;
> +
> + if (caam_sk_get_random_map(dev, req, skdesc)) {
> + dev_err(dev, "caam get_random map fail\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> + caam_sk_rng_desc(req, skdesc);
> + break;
> + }
> + case sk_red_blob_enc:
> + {
> + skdesc = kmalloc(sizeof(*skdesc), GFP_DMA);
> + if (!skdesc) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + skdesc->req_type = req->type;
> +
> + if (caam_sk_redblob_encap_map(dev, req, skdesc)) {
> + dev_err(dev, "caam redblob_encap map fail\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Descriptor function to create redblob from data. */
> + caam_sk_redblob_encap_desc(req, skdesc);
> + break;
> + }
> +
> + case sk_red_blob_dec:
> + {
> + skdesc = kmalloc(sizeof(*skdesc), GFP_DMA);
> + if (!skdesc) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + skdesc->req_type = req->type;
> +
> + if (caam_sk_redblob_decap_map(dev, req, skdesc)) {
> + dev_err(dev, "caam redblob_decap map fail\n");
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + /* Descriptor function to decap data from redblob. */
> + caam_sk_redblob_decap_desc(req, skdesc);
> + break;
> + }
> + default:
> + pr_debug("Unknown request type\n");
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + req->desc_pointer = (void *)skdesc;
> +
> +out:
> + return ret;
> +}
> +
> +/* static void caam_op_done (struct device *dev, u32 *desc, u32 ret,
> + * void *context)
> + * brief callback function to be called when descriptor executed.
> + * param[in] dev Pointer to device structure
> + * param[in] desc descriptor pointer
> + * param[in] ret return status of Job submitted
> + * param[in] context void pointer
> + */
> +static void caam_op_done(struct device *dev, u32 *desc, u32 ret,
> + void *context)
> +{
> + struct sk_req *req = context;
> +
> + if (ret) {
> + dev_err(dev, "caam op done err: %x\n", ret);
> + /* print the error source name. */
> + caam_jr_strstatus(dev, ret);
> + }
> + /* Call securekey_unmap function for unmapping the buffer pointers. */
> + securekey_unmap(dev, req->desc_pointer, req);
> +
> + req->ret = ret;
> + complete(&req->comp);
> +}
> +
> +
> +/* static int sk_job_submit(struct device *jrdev, struct sk_req *req)
> + * brief Enqueue a Job descriptor to Job ring and wait until SEC returns.
> + * param[in] jrdev Pointer to job ring device structure
> + * param[in] req Pointer to secure key request structure
> + * return 0 on success, error value otherwise.
> + */
> +static int sk_job_submit(struct device *jrdev, struct sk_req *req)
> +{
> + int ret;
> +
> + init_completion(&req->comp);
> +
> + /* caam_jr_enqueue function for Enqueue a job descriptor */
> + ret = caam_jr_enqueue(jrdev, req->hwdesc, caam_op_done, req);
> + if (!ret)
> + wait_for_completion_interruptible(&req->comp);
> +
> + ret = req->ret;
> + return ret;
> +}
> +
> +/* caam_get_random(struct secure_key_payload *p, enum sk_req_type fetch_rnd,
> + * struct device *dev)
> + * Create the random number of the specified length using CAAM block
> + * param[in]: out pointer to place the random bytes
> + * param[in]: length for the random data bytes.
> + * param[in]: dev Pointer to job ring device structure
> + * If operation is successful return 0, otherwise error.
> + */
> +int caam_get_random(struct secure_key_payload *p, enum sk_req_type fetch_rnd,
> + struct device *dev)
> +{
> + struct sk_fetch_rnd_data *fetch_rnd_data = NULL;
> + struct sk_req *req = NULL;
> + int ret = 0;
> + void *temp = NULL;
> +
> + req = kmalloc(sizeof(struct sk_req), GFP_DMA);
> + if (!req) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + req->type = fetch_rnd;
> + fetch_rnd_data = &(req->req_u.sk_fetch_rnd_data);
> +
> + /* initialise with key length */
> + fetch_rnd_data->key_len = p->key_len;
> +
> + temp = kmalloc(fetch_rnd_data->key_len, GFP_DMA);
> + if (!temp) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + fetch_rnd_data->data = temp;
> +
> + ret = caam_securekey_desc_init(dev, req);
> +
> + if (ret) {
> + pr_info("caam_securekey_desc_init failed\n");
> + goto out;
> + }
> +
> + ret = sk_job_submit(dev, req);
> + if (!ret) {
> + /*Copy output to key buffer. */
> + memcpy(p->key, fetch_rnd_data->data, p->key_len);
> + } else {
> + ret = -EINVAL;
> + }
> +
> +out:
> + if (req)
> + kfree(req);
> +
> + if (temp)
> + kfree(temp);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(caam_get_random);
> +
> +/* key_deblob(struct secure_key_payload *p, enum sk_req_type decap_type
> + * struct device *dev)
> + * Deblobify the blob to get the key data and fill in secure key payload struct
> + * param[in] p pointer to the secure key payload
> + * param[in] decap_type operation to be done.
> + * param[in] dev dev Pointer to job ring device structure
> + * If operation is successful return 0, otherwise error.
> + */
> +int key_deblob(struct secure_key_payload *p, enum sk_req_type decap_type,
> + struct device *dev)
> +{
> + unsigned int blob_len;
> + struct sk_red_blob_decap *d_blob;
> + struct sk_req *req = NULL;
> + int total_sz = 0, *temp = NULL, ret = 0;
> +
> + req = kmalloc(sizeof(struct sk_req), GFP_DMA);
> + if (!req) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + d_blob = &(req->req_u.sk_red_blob_decap);
> + blob_len = p->blob_len;
> + req->type = decap_type;
> +
> + /*
> + * Red blob size is the blob_len filled in payload struct
> + * Data_sz i.e. key is the blob_len - blob header size
> + */
> +
> + d_blob->redblob_sz = blob_len;
> + d_blob->data_sz = blob_len - (SK_BLOB_KEY_SZ + SK_BLOB_MAC_SZ);
> + total_sz = d_blob->data_sz + d_blob->redblob_sz;
> +
> + temp = kmalloc(total_sz, GFP_DMA);
> + if (!temp) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + req->mem_pointer = temp;
> + d_blob->redblob = temp;
> + d_blob->data = d_blob->redblob + d_blob->redblob_sz;
> + memcpy(d_blob->redblob, p->blob, blob_len);
> +
> + ret = caam_securekey_desc_init(dev, req);
> +
> + if (ret) {
> + pr_info("caam_securekey_desc_init: Failed\n");
> + goto out;
> + }
> +
> + ret = sk_job_submit(dev, req);
> + if (!ret) {
> + /*Copy output to key buffer. */
> + p->key_len = d_blob->data_sz;
> + memcpy(p->key, d_blob->data, p->key_len);
> + } else {
> + ret = -EINVAL;
> + }
> +
> +out:
> + if (temp)
> + kfree(temp);
> + if (req)
> + kfree(req);
> + return ret;
> +}
> +EXPORT_SYMBOL(key_deblob);
> +
> +/* key_blob(struct secure_key_payload *p, enum sk_req_type encap_type,
> + * struct device *dev)
> + * To blobify the key data to get the blob. This blob can only be seen by
> + * userspace.
> + * param[in] p pointer to the secure key payload
> + * param[in] decap_type operation to be done.
> + * param[in] dev dev Pointer to job ring device structure
> + * If operation is successful return 0, otherwise error.
> + */
> +int key_blob(struct secure_key_payload *p, enum sk_req_type encap_type,
> + struct device *dev)
> +{
> + unsigned int key_len;
> + struct sk_red_blob_encap *k_blob;
> + struct sk_req *req = NULL;
> + int total_sz = 0, *temp = NULL, ret = 0;
> +
> + req = kmalloc(sizeof(struct sk_req), GFP_DMA);
> + if (!req) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + key_len = p->key_len;
> +
> + req->type = encap_type;
> + k_blob = &(req->req_u.sk_red_blob_encap);
> +
> + /*
> + * Data_sz i.e. key len and the corresponding blob_len is
> + * key_len + BLOB header size.
> + */
> +
> + k_blob->data_sz = key_len;
> + k_blob->redblob_sz = key_len + SK_BLOB_KEY_SZ + SK_BLOB_MAC_SZ;
> + total_sz = k_blob->data_sz + k_blob->redblob_sz;
> +
> + temp = kmalloc(total_sz, GFP_DMA);
> + if (!temp) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + req->mem_pointer = temp;
> + k_blob->data = temp;
> +
> + k_blob->redblob = k_blob->data + k_blob->data_sz;
> + memcpy(k_blob->data, p->key, key_len);
> +
> + ret = caam_securekey_desc_init(dev, req);
> +
> + if (ret) {
> + pr_info("caam_securekey_desc_init failed\n");
> + goto out;
> + }
> +
> + ret = sk_job_submit(dev, req);
> + if (!ret) {
> + /*Copy output to key buffer. */
> + p->blob_len = k_blob->redblob_sz;
> + memcpy(p->blob, k_blob->redblob, p->blob_len);
> + } else {
> + ret = -EINVAL;
> + }
> +
> +out:
> + if (temp)
> + kfree(req->mem_pointer);
> + if (req)
> + kfree(req);
> + return ret;
> +
> +}
> +EXPORT_SYMBOL(key_blob);
> diff --git a/security/keys/securekey_desc.h b/security/keys/securekey_desc.h
> new file mode 100644
> index 000000000000..0ee26e95b205
> --- /dev/null
> +++ b/security/keys/securekey_desc.h
> @@ -0,0 +1,141 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2018 NXP
> + *
> + */
> +#ifndef _SECUREKEY_DESC_H_
> +#define _SECUREKEY_DESC_H_
> +
> +#include "compat.h"
> +#include "regs.h"
> +#include "intern.h"
> +#include "desc.h"
> +#include "desc_constr.h"
> +#include "jr.h"
> +#include "error.h"
> +#include "pdb.h"
> +
> +#define SK_BLOB_KEY_SZ 32 /* Blob key size. */
> +#define SK_BLOB_MAC_SZ 16 /* Blob MAC size. */
> +
> +/*
> + * brief defines different kinds of operations supported by this module.
> + */
> +enum sk_req_type {
> + sk_get_random,
> + sk_red_blob_enc,
> + sk_red_blob_dec,
> +};
> +
> +
> +/*
> + * struct random_des
> + * param[out] rnd_data output buffer for random data.
> + */
> +struct random_desc {
> + dma_addr_t rnd_data;
> +};
> +
> +/* struct redblob_encap_desc
> + * details Structure containing dma address for redblob encapsulation.
> + * param[in] in_data input data to redblob encap descriptor.
> + * param[out] redblob output buffer for redblob.
> + */
> +struct redblob_encap_desc {
> + dma_addr_t in_data;
> + dma_addr_t redblob;
> +};
> +
> +/* struct redblob_decap_desc
> + * details Structure containing dma address for redblob decapsulation.
> + * param[in] redblob input buffer to redblob decap descriptor.
> + * param[out] out_data output data from redblob decap descriptor.
> + */
> +struct redblob_decap_desc {
> + dma_addr_t redblob;
> + dma_addr_t out_data;
> +};
> +
> +/* struct sk_desc
> + * details Structure for securekey descriptor creation.
> + * param[in] req_type operation supported.
> + * param[in] dma_u union of struct for supported operation.
> + */
> +struct sk_desc {
> + u32 req_type;
> + union {
> + struct redblob_encap_desc redblob_encapdesc;
> + struct redblob_decap_desc redblob_decapdesc;
> + struct random_desc random_descp;
> + } dma_u;
> +};
> +
> +/* struct sk_fetch_rnd_data
> + * decriptor structure containing key length.
> + */
> +struct sk_fetch_rnd_data {
> + void *data;
> + size_t key_len;
> +};
> +
> +/* struct sk_red_blob_encap
> + * details Structure containing buffer pointers for redblob encapsulation.
> + * param[in] data Input data.
> + * param[in] data_sz size of Input data.
> + * param[out] redblob output buffer for redblob.
> + * param[in] redblob_sz size of redblob.
> + */
> +struct sk_red_blob_encap {
> + void *data;
> + uint32_t data_sz;
> + void *redblob;
> + uint32_t redblob_sz;
> +};
> +
> +/* struct sk_red_blob_decap
> + * details Structure containing buffer pointers for redblob decapsulation.
> + * param[in] redblob Input redblob.
> + * param[in] redblob_sz size of redblob.
> + * param[out] data output buffer for data.
> + * param[in] data_sz size of output data.
> + */
> +struct sk_red_blob_decap {
> + void *redblob;
> + uint32_t redblob_sz;
> + void *data;
> + uint32_t data_sz;
> +};
> +
> +/* struct sk_req
> + * details Structure for securekey request creation.
> + * param[in] type operation supported.
> + * param[in] req_u union of struct for supported operation.
> + * param[out] ret return status of CAAM operation.
> + * param[in] mem_pointer memory pointer for allocated kernel memory.
> + * param[in] desc_pointer Pointer to securekey descriptor creation structure.
> + * param[in] comp struct completion object.
> + * param[in] hwdesc contains descriptor instructions.
> + */
> +struct sk_req {
> + enum sk_req_type type;
> + void *arg;
> + union {
> + struct sk_red_blob_encap sk_red_blob_encap;
> + struct sk_red_blob_decap sk_red_blob_decap;
> + struct sk_fetch_rnd_data sk_fetch_rnd_data;
> + } req_u;
> + int ret;
> + void *mem_pointer;
> + void *desc_pointer;
> + struct completion comp;
> + u32 hwdesc[MAX_CAAM_DESCSIZE];
> +};
> +
> +int caam_get_random(struct secure_key_payload *p, enum sk_req_type fetch_rnd,
> + struct device *dev);
> +int key_blob(struct secure_key_payload *p, enum sk_req_type encap_type,
> + struct device *dev);
> +int key_deblob(struct secure_key_payload *p, enum sk_req_type decap_type,
> + struct device *dev);
> +
> +#endif /*_SECUREKEY_DESC_H_*/
^ permalink raw reply
* Re: [PATCH bpf-next v2 08/10] tools/libbpf: Add support for BPF_PROG_TYPE_LSM
From: KP Singh @ 2020-01-17 22:16 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: open list, bpf, linux-security-module, Alexei Starovoitov,
Daniel Borkmann, James Morris, Kees Cook, Thomas Garnier,
Michael Halcrow, Paul Turner, Brendan Gregg, Jann Horn,
Matthew Garrett, Christian Brauner, Mickaël Salaün,
Florent Revest, Brendan Jackman, Martin KaFai Lau, Song Liu,
Yonghong Song, Serge E. Hallyn, Mauro Carvalho Chehab,
David S. Miller, Greg Kroah-Hartman, Nicolas Ferre,
Stanislav Fomichev, Quentin Monnet, Andrey Ignatov, Joe Stringer
In-Reply-To: <CAEf4BzYa+m0OCZXqnNsab+q3+HLboL45eRdBrGdZD+6Z4CRSiQ@mail.gmail.com>
On 16-Jan 11:10, Andrii Nakryiko wrote:
> On Thu, Jan 16, 2020 at 4:49 AM KP Singh <kpsingh@chromium.org> wrote:
> >
> > Thanks for the review Andrii!
> >
> > I will incorporate the fixes in the next revision.
> >
> > On 15-Jan 13:19, Andrii Nakryiko wrote:
> > > On Wed, Jan 15, 2020 at 9:13 AM KP Singh <kpsingh@chromium.org> wrote:
> > > >
> > > > From: KP Singh <kpsingh@google.com>
> > > >
> > > > * Add functionality in libbpf to attach eBPF program to LSM hooks
> > > > * Lookup the index of the LSM hook in security_hook_heads and pass it in
> > > > attr->lsm_hook_index
> > > >
> > > > Signed-off-by: KP Singh <kpsingh@google.com>
> > > > ---
> > > > tools/lib/bpf/bpf.c | 6 +-
> > > > tools/lib/bpf/bpf.h | 1 +
> > > > tools/lib/bpf/libbpf.c | 143 ++++++++++++++++++++++++++++++++++-----
> > > > tools/lib/bpf/libbpf.h | 4 ++
> > > > tools/lib/bpf/libbpf.map | 3 +
> > > > 5 files changed, 138 insertions(+), 19 deletions(-)
> > > >
>
> [...]
>
> > >
> > > > +{
> > > > + struct btf *btf = bpf_find_kernel_btf();
> > >
> > > ok, it's probably time to do this right. Let's ensure we load kernel
> > > BTF just once, keep it inside bpf_object while we need it and then
> > > release it after successful load. We are at the point where all the
> > > new types of program is loading/releasing kernel BTF for every section
> > > and it starts to feel very wasteful.
> >
> > Sure, will give it a shot in v3.
>
> thanks!
>
> [...]
>
> > >
> > > > + if (!strcmp(btf__name_by_offset(btf, m->name_off), name))
> > > > + return j + 1;
> > >
> > > I looked briefly through kernel-side patch introducing lsm_hook_index,
> > > but it didn't seem to explain why this index needs to be (unnaturally)
> > > 1-based. So asking here first as I'm looking through libbpf changes?
> >
> > The lsm_hook_idx is one-based as it makes it easy to validate the
> > input. If we make it zero-based it's hard to check if the user
> > intended to attach to the LSM hook at index 0 or did not set it.
>
> Think about providing FDs. 0 is a valid, though rarely
> intended/correct value. Yet we don't make all FD arguments
> artificially 1-based, right? This extra +1/-1 translation just makes
> for more confusing interface, IMO. If user "accidentally" guessed type
> signature of very first hook, well, so be it... If not, BPF verifier
> will politely refuse. Seems like enough protection.
Thanks! I see your point and will update to using the
more-conventional 0-based indexing for the next revision.
- KP
>
> >
> > We are then up to the verifier to reject the loaded program which
> > may or may not match the signature of the hook at lsm_hook_idx = 0.
> >
> > I will clarify this in the commit log as well.
> >
>
> [...]
^ permalink raw reply
* Re: inconsistent lock state in ima_process_queued_keys
From: syzbot @ 2020-01-18 3:14 UTC (permalink / raw)
To: dmitry.kasatkin, dvyukov, jmorris, linux-integrity, linux-kernel,
linux-security-module, nramas, serge, syzkaller-bugs, zohar
In-Reply-To: <000000000000486474059c19f4d7@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 2747d5fd Add linux-next specific files for 20200116
git tree: linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=14ee1cc9e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=22f506e7a3a37fe2
dashboard link: https://syzkaller.appspot.com/bug?extid=a4a503d7f37292ae1664
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=128b4495e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=12b673b9e00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+a4a503d7f37292ae1664@syzkaller.appspotmail.com
================================
WARNING: inconsistent lock state
5.5.0-rc6-next-20200116-syzkaller #0 Not tainted
--------------------------------
inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
kworker/u4:3/125 [HC0[0]:SC1[1]:HE1:SE0] takes:
ffffffff8a03ce58 (ima_keys_lock){+.?.}, at: spin_lock include/linux/spinlock.h:338 [inline]
ffffffff8a03ce58 (ima_keys_lock){+.?.}, at: ima_process_queued_keys+0x4f/0x320 security/integrity/ima/ima_asymmetric_keys.c:144
{SOFTIRQ-ON-W} state was registered at:
lock_acquire+0x190/0x410 kernel/locking/lockdep.c:4484
__raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock+0x2f/0x40 kernel/locking/spinlock.c:151
spin_lock include/linux/spinlock.h:338 [inline]
ima_queue_key security/integrity/ima/ima_asymmetric_keys.c:111 [inline]
ima_post_key_create_or_update+0x234/0x470 security/integrity/ima/ima_asymmetric_keys.c:194
key_create_or_update+0x6b8/0xcb0 security/keys/key.c:944
load_system_certificate_list+0x1ba/0x25e certs/system_keyring.c:161
do_one_initcall+0x120/0x820 init/main.c:1109
do_initcall_level init/main.c:1182 [inline]
do_initcalls init/main.c:1198 [inline]
do_basic_setup init/main.c:1218 [inline]
kernel_init_freeable+0x522/0x5d0 init/main.c:1402
kernel_init+0x12/0x1bf init/main.c:1309
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
irq event stamp: 15703140
hardirqs last enabled at (15703140): [<ffffffff87ee0813>] __raw_spin_unlock_irq include/linux/spinlock_api_smp.h:168 [inline]
hardirqs last enabled at (15703140): [<ffffffff87ee0813>] _raw_spin_unlock_irq+0x23/0x80 kernel/locking/spinlock.c:199
hardirqs last disabled at (15703139): [<ffffffff87ee09fa>] __raw_spin_lock_irq include/linux/spinlock_api_smp.h:126 [inline]
hardirqs last disabled at (15703139): [<ffffffff87ee09fa>] _raw_spin_lock_irq+0x3a/0x80 kernel/locking/spinlock.c:167
softirqs last enabled at (15702640): [<ffffffff87cf9855>] spin_unlock_bh include/linux/spinlock.h:383 [inline]
softirqs last enabled at (15702640): [<ffffffff87cf9855>] batadv_nc_purge_paths+0x265/0x370 net/batman-adv/network-coding.c:470
softirqs last disabled at (15702651): [<ffffffff8147a05b>] invoke_softirq kernel/softirq.c:373 [inline]
softirqs last disabled at (15702651): [<ffffffff8147a05b>] irq_exit+0x19b/0x1e0 kernel/softirq.c:413
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
----
lock(ima_keys_lock);
<Interrupt>
lock(ima_keys_lock);
*** DEADLOCK ***
4 locks held by kworker/u4:3/125:
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: __write_once_size include/linux/compiler.h:250 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: arch_atomic64_set arch/x86/include/asm/atomic64_64.h:34 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: atomic64_set include/asm-generic/atomic-instrumented.h:869 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: atomic_long_set include/asm-generic/atomic-long.h:41 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: set_work_data kernel/workqueue.c:615 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: set_work_pool_and_clear_pending kernel/workqueue.c:642 [inline]
#0: ffff88821512a928 ((wq_completion)bat_events){+.+.}, at: process_one_work+0x8dd/0x17a0 kernel/workqueue.c:2235
#1: ffffc90001397dc0 ((work_completion)(&(&bat_priv->nc.work)->work)){+.+.}, at: process_one_work+0x917/0x17a0 kernel/workqueue.c:2239
#2: ffffffff89bb0400 (rcu_read_lock){....}, at: batadv_nc_purge_orig_hash net/batman-adv/network-coding.c:405 [inline]
#2: ffffffff89bb0400 (rcu_read_lock){....}, at: batadv_nc_worker+0xe3/0x760 net/batman-adv/network-coding.c:718
#3: ffffc90000d98d50 ((&ima_key_queue_timer)){+.-.}, at: lockdep_copy_map include/linux/lockdep.h:172 [inline]
#3: ffffc90000d98d50 ((&ima_key_queue_timer)){+.-.}, at: call_timer_fn+0xe0/0x780 kernel/time/timer.c:1394
stack backtrace:
CPU: 1 PID: 125 Comm: kworker/u4:3 Not tainted 5.5.0-rc6-next-20200116-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: bat_events batadv_nc_worker
Call Trace:
<IRQ>
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x197/0x210 lib/dump_stack.c:118
print_usage_bug.cold+0x327/0x378 kernel/locking/lockdep.c:3100
valid_state kernel/locking/lockdep.c:3111 [inline]
mark_lock_irq kernel/locking/lockdep.c:3308 [inline]
mark_lock+0xbb4/0x1220 kernel/locking/lockdep.c:3665
mark_usage kernel/locking/lockdep.c:3565 [inline]
__lock_acquire+0x1e8e/0x4a00 kernel/locking/lockdep.c:3908
lock_acquire+0x190/0x410 kernel/locking/lockdep.c:4484
__raw_spin_lock include/linux/spinlock_api_smp.h:142 [inline]
_raw_spin_lock+0x2f/0x40 kernel/locking/spinlock.c:151
spin_lock include/linux/spinlock.h:338 [inline]
ima_process_queued_keys+0x4f/0x320 security/integrity/ima/ima_asymmetric_keys.c:144
ima_timer_handler+0x15/0x20 security/integrity/ima/ima_asymmetric_keys.c:46
call_timer_fn+0x1ac/0x780 kernel/time/timer.c:1404
expire_timers kernel/time/timer.c:1449 [inline]
__run_timers kernel/time/timer.c:1773 [inline]
__run_timers kernel/time/timer.c:1740 [inline]
run_timer_softirq+0x6c3/0x1790 kernel/time/timer.c:1786
__do_softirq+0x262/0x98c kernel/softirq.c:292
invoke_softirq kernel/softirq.c:373 [inline]
irq_exit+0x19b/0x1e0 kernel/softirq.c:413
exiting_irq arch/x86/include/asm/apic.h:536 [inline]
smp_apic_timer_interrupt+0x1a3/0x610 arch/x86/kernel/apic/apic.c:1137
apic_timer_interrupt+0xf/0x20 arch/x86/entry/entry_64.S:829
</IRQ>
RIP: 0010:arch_local_irq_restore arch/x86/include/asm/paravirt.h:752 [inline]
RIP: 0010:lock_acquire+0x20b/0x410 kernel/locking/lockdep.c:4487
Code: 9c 08 00 00 00 00 00 00 48 c1 e8 03 80 3c 10 00 0f 85 d3 01 00 00 48 83 3d 49 7d 58 08 00 0f 84 53 01 00 00 48 8b 7d c8 57 9d <0f> 1f 44 00 00 48 8d 65 d8 5b 41 5c 41 5d 41 5e 41 5f 5d c3 65 8b
RSP: 0018:ffffc90001397c70 EFLAGS: 00000286 ORIG_RAX: ffffffffffffff13
RAX: 1ffffffff136774f RBX: ffff8880a922c600 RCX: ffffffff815ae240
RDX: dffffc0000000000 RSI: 0000000000000008 RDI: 0000000000000286
RBP: ffffc90001397cb8 R08: 1ffffffff16a51a4 R09: fffffbfff16a51a5
R10: ffff8880a922cef0 R11: ffff8880a922c600 R12: ffffffff89bb0400
R13: 0000000000000000 R14: 0000000000000000 R15: 0000000000000002
rcu_lock_acquire include/linux/rcupdate.h:208 [inline]
rcu_read_lock include/linux/rcupdate.h:601 [inline]
batadv_nc_purge_orig_hash net/batman-adv/network-coding.c:407 [inline]
batadv_nc_worker+0x117/0x760 net/batman-adv/network-coding.c:718
process_one_work+0xa05/0x17a0 kernel/workqueue.c:2264
worker_thread+0x98/0xe40 kernel/workqueue.c:2410
kthread+0x361/0x430 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
^ permalink raw reply
* Re: inconsistent lock state in ima_process_queued_keys
From: Lakshmi Ramasubramanian @ 2020-01-18 5:22 UTC (permalink / raw)
To: syzbot, dmitry.kasatkin, dvyukov, jmorris, linux-integrity,
linux-kernel, linux-security-module, serge, syzkaller-bugs, zohar
In-Reply-To: <000000000000a1d91b059c6173c6@google.com>
On 1/17/2020 7:14 PM, syzbot wrote:
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+a4a503d7f37292ae1664@syzkaller.appspotmail.com
>
> ================================
> WARNING: inconsistent lock state
> 5.5.0-rc6-next-20200116-syzkaller #0 Not tainted
> --------------------------------
> inconsistent {SOFTIRQ-ON-W} -> {IN-SOFTIRQ-W} usage.
> kworker/u4:3/125 [HC0[0]:SC1[1]:HE1:SE0] takes:
> ffffffff8a03ce58 (ima_keys_lock){+.?.}, at: spin_lock include/linux/spinlock.h:338 [inline]
> ffffffff8a03ce58 (ima_keys_lock){+.?.}, at: ima_process_queued_keys+0x4f/0x320 security/integrity/ima/ima_asymmetric_keys.c:144
> {SOFTIRQ-ON-W} state was registered at:
The fix for this issue is in next-integrity branch. Should be merged to
linux-next shortly.
-lakshmi
^ permalink raw reply
* [RFC PATCH linux-next] apparmor: build_aa_ext_struct() can be static
From: kbuild test robot @ 2020-01-18 10:52 UTC (permalink / raw)
To: Mike Salvatore
Cc: kbuild-all, Shuah Khan, Brendan Higgins, Kees Cook, John Johansen,
James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
In-Reply-To: <202001181831.bFmypzrR%lkp@intel.com>
Fixes: 4d944bcd4e73 ("apparmor: add AppArmor KUnit tests for policy unpack")
Signed-off-by: kbuild test robot <lkp@intel.com>
---
policy_unpack_test.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/security/apparmor/policy_unpack_test.c b/security/apparmor/policy_unpack_test.c
index 533137f45361c..dbcfeb12a019f 100644
--- a/security/apparmor/policy_unpack_test.c
+++ b/security/apparmor/policy_unpack_test.c
@@ -48,8 +48,8 @@ struct policy_unpack_fixture {
size_t e_size;
};
-struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
- struct kunit *test, size_t buf_size)
+static struct aa_ext *build_aa_ext_struct(struct policy_unpack_fixture *puf,
+ struct kunit *test, size_t buf_size)
{
char *buf;
struct aa_ext *e;
^ permalink raw reply related
* [linux-next:master 7161/9861] security/apparmor/policy_unpack_test.c:51:15: sparse: sparse: symbol 'build_aa_ext_struct' was not declared. Should it be static?
From: kbuild test robot @ 2020-01-18 10:52 UTC (permalink / raw)
To: Mike Salvatore
Cc: kbuild-all, Shuah Khan, Brendan Higgins, Kees Cook, John Johansen,
James Morris, Serge E. Hallyn, linux-security-module,
linux-kernel
tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master
head: de970dffa7d19eae1d703c3534825308ef8d5dec
commit: 4d944bcd4e731ab7bfe8d01a7041ea0ebdc090f1 [7161/9861] apparmor: add AppArmor KUnit tests for policy unpack
reproduce:
# apt-get install sparse
# sparse version: v0.6.1-131-g22978b6b-dirty
git checkout 4d944bcd4e731ab7bfe8d01a7041ea0ebdc090f1
make ARCH=x86_64 allmodconfig
make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> security/apparmor/policy_unpack_test.c:51:15: sparse: sparse: symbol 'build_aa_ext_struct' was not declared. Should it be static?
Please review and possibly fold the followup patch.
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
^ permalink raw reply
* Re: [PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF
From: kbuild test robot @ 2020-01-18 12:44 UTC (permalink / raw)
To: KP Singh
Cc: kbuild-all, linux-kernel, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Jann Horn, Matthew Garrett, Christian Brauner,
Mickaël Salaün, Florent Revest, Brendan Jackman,
Martin KaFai Lau, Song Liu, Yonghong Song, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <20200115171333.28811-2-kpsingh@chromium.org>
[-- Attachment #1: Type: text/plain, Size: 2638 bytes --]
Hi KP,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on next-20200116]
[cannot apply to bpf-next/master bpf/master linus/master security/next-testing v5.5-rc6 v5.5-rc5 v5.5-rc4 v5.5-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/KP-Singh/MAC-and-Audit-policy-using-eBPF-KRSI/20200117-070342
base: 2747d5fdab78f43210256cd52fb2718e0b3cce74
config: nds32-defconfig (attached as .config)
compiler: nds32le-linux-gcc (GCC) 9.2.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=9.2.0 make.cross ARCH=nds32
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All errors (new ones prefixed by >>):
In file included from kernel/bpf/core.c:27:
>> include/linux/btf.h:148:38: error: static declaration of 'btf_type_by_name_kind' follows non-static declaration
148 | static inline const struct btf_type *btf_type_by_name_kind(
| ^~~~~~~~~~~~~~~~~~~~~
include/linux/btf.h:70:24: note: previous declaration of 'btf_type_by_name_kind' was here
70 | const struct btf_type *btf_type_by_name_kind(
| ^~~~~~~~~~~~~~~~~~~~~
vim +/btf_type_by_name_kind +148 include/linux/btf.h
136
137 #ifdef CONFIG_BPF_SYSCALL
138 const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
139 const char *btf_name_by_offset(const struct btf *btf, u32 offset);
140 struct btf *btf_parse_vmlinux(void);
141 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
142 #else
143 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
144 u32 type_id)
145 {
146 return NULL;
147 }
> 148 static inline const struct btf_type *btf_type_by_name_kind(
149 struct btf *btf, const char *name, u8 kind)
150 {
151 return ERR_PTR(-EOPNOTSUPP);
152 }
153 static inline const char *btf_name_by_offset(const struct btf *btf,
154 u32 offset)
155 {
156 return NULL;
157 }
158 #endif
159
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10867 bytes --]
^ permalink raw reply
* Re: [RFC PATCH 0/2] Create CAAM HW key in linux keyring and use in dmcrypt
From: James Bottomley @ 2020-01-18 17:51 UTC (permalink / raw)
To: Franck Lenormand, David Howells
Cc: linux-kernel@vger.kernel.org,
linux-security-module@vger.kernel.org, keyrings@vger.kernel.org,
Horia Geanta, Silvano Di Ninno, agk@redhat.com,
snitzer@redhat.com, dm-devel@redhat.com, jmorris@namei.org,
serge@hallyn.com
In-Reply-To: <AM6PR04MB54473C2D30DDD7CDC8522DF9924C0@AM6PR04MB5447.eurprd04.prod.outlook.com>
On Thu, 2019-03-07 at 13:17 +0000, Franck Lenormand wrote:
> > -----Original Message-----
> > From: David Howells <dhowells@redhat.com>
> > Sent: Wednesday, March 6, 2019 6:30 PM
> > To: Franck Lenormand <franck.lenormand@nxp.com>
> > Cc: dhowells@redhat.com; linux-kernel@vger.kernel.org; linux-
> > security-
> > module@vger.kernel.org; keyrings@vger.kernel.org; Horia Geanta
> > <horia.geanta@nxp.com>; Silvano Di Ninno <silvano.dininno@nxp.com>;
> > agk@redhat.com; snitzer@redhat.com; dm-devel@redhat.com;
> > jmorris@namei.org; serge@hallyn.com
> > Subject: Re: [RFC PATCH 0/2] Create CAAM HW key in linux keyring
> > and use in
> > dmcrypt
> >
> > Franck LENORMAND <franck.lenormand@nxp.com> wrote:
> >
> > > The capacity to generate or load keys already available in the
> > > Linux
> > > key retention service does not allows to exploit CAAM
> > > capabilities
> > > hence we need to create a new key_type. The new key type
> > > "caam_tk"
> >
> > allows to:
> > > - Create a black key from random
> > > - Create a black key from a red key
> > > - Load a black blob to retrieve the black key
> >
> > Is it possible that this could be done through an existing key
> > type, such as the
> > asymmetric, trusted or encrypted key typed?
> >
> > David
>
> Hello David,
>
> I didn't know about asymmetric key type so I looked it up, from my
> observation, it would not be possible to use it for the caam_tk as
> we must perform operations on the data provided.
> The name " asymmetric " is also misleading for the use we would have.
>
> The trusted and encrypted does not provides the necessary
> callbacks to do what we would need or require huge modifications.
>
> I would like, for this series to focus on the change related to
> dm-crypt. In effect, it is currently not possible to pass a key
> from the asymmetric key type to it.
What you're performing are all bog standard key wrapping operations
which is why we're asking the question: do we have to expose any of
this to the user? Can this whole thing not be encapsulated in such a
way that the kernel automatically selects the best key
escrow/accelerator technology on boot and uses it (if there are > 1
there would have to be an interface for the user to choose). We make
all the accelerator device key formats distinguishable so the kernel
can figure out on load what is supposed to be handling them. That way
the user never need worry about naming the actual key handler at all,
it would all be taken care of under the covers.
The one key type per escrow/accelerator has us all going ... "aren't
there hundreds of these on the market?" which would seem to be a huge
usability explosion because a user will likely only have one, but they
have to figure out the type instructions for that one. I really think
a better way is to have a more generic key type that's capable of
interfacing to the wrap/unwrap and crypto functions in such a way that
the end user doesn't have to know which they're using: most platforms
only have one thing installed, so you use that thing.
James
^ permalink raw reply
* Re: [RFC PATCH 2/2] dm-crypt: Use any key type which is registered
From: James Bottomley @ 2020-01-18 17:55 UTC (permalink / raw)
To: Franck LENORMAND, linux-kernel, linux-security-module, keyrings
Cc: horia.geanta, silvano.dininno, agk, snitzer, dm-devel, dhowells,
jmorris, serge
In-Reply-To: <1551456599-10603-3-git-send-email-franck.lenormand@nxp.com>
On Fri, 2019-03-01 at 17:09 +0100, Franck LENORMAND wrote:
> @@ -2025,16 +2027,15 @@ static int crypt_set_keyring_key(struct
> crypt_config *cc, const char *key_string
> if (!key_desc || key_desc == key_string || !strlen(key_desc
> + 1))
> return -EINVAL;
>
> - if (strncmp(key_string, "logon:", key_desc - key_string + 1)
> &&
> - strncmp(key_string, "user:", key_desc - key_string + 1))
> - return -EINVAL;
> + type = get_key_type(key_string, key_desc - key_string);
> + if (!type)
> + return -ENOENT;
You can't do this. This check ensures that the key responds correctly
to user_key_payload_locked() lower down. To do that, the payload has
to be in a specific form. You ensured that yours are, but dm-crypt
will now accept any key type, load the user payload blindly and create
all sorts of mayhem in the kernel because of the structural differences
in payload types.
James
^ permalink raw reply
* Re: [PATCH bpf-next v2 01/10] bpf: btf: Make some of the API visible outside BTF
From: KP Singh @ 2020-01-20 11:00 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, linux-kernel, bpf, linux-security-module,
Alexei Starovoitov, Daniel Borkmann, James Morris, Kees Cook,
Thomas Garnier, Michael Halcrow, Paul Turner, Brendan Gregg,
Jann Horn, Matthew Garrett, Christian Brauner,
Mickaël Salaün, Florent Revest, Brendan Jackman,
Martin KaFai Lau, Song Liu, Yonghong Song, Serge E. Hallyn,
Mauro Carvalho Chehab, David S. Miller, Greg Kroah-Hartman,
Nicolas Ferre, Stanislav Fomichev, Quentin Monnet, Andrey Ignatov,
Joe Stringer
In-Reply-To: <202001182045.QaQ0kGP8%lkp@intel.com>
Thanks! I have fixed this in the v3 of the series. btf_find_by_name_kind
is independant of CONFIG_BPF_SYSCALL and btf_type_by_name_kind needs
to be as well.
The mistake was adding a static inline definition of the function
in the !CONFIG_BPF_SYSCALL section which is not needed in this case.
- KP
On 18-Jan 20:44, kbuild test robot wrote:
> Hi KP,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on next-20200116]
> [cannot apply to bpf-next/master bpf/master linus/master security/next-testing v5.5-rc6 v5.5-rc5 v5.5-rc4 v5.5-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help
> improve the system. BTW, we also suggest to use '--base' option to specify the
> base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
>
> url: https://github.com/0day-ci/linux/commits/KP-Singh/MAC-and-Audit-policy-using-eBPF-KRSI/20200117-070342
> base: 2747d5fdab78f43210256cd52fb2718e0b3cce74
> config: nds32-defconfig (attached as .config)
> compiler: nds32le-linux-gcc (GCC) 9.2.0
> reproduce:
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> GCC_VERSION=9.2.0 make.cross ARCH=nds32
>
> If you fix the issue, kindly add following tag
> Reported-by: kbuild test robot <lkp@intel.com>
>
> All errors (new ones prefixed by >>):
>
> In file included from kernel/bpf/core.c:27:
> >> include/linux/btf.h:148:38: error: static declaration of 'btf_type_by_name_kind' follows non-static declaration
> 148 | static inline const struct btf_type *btf_type_by_name_kind(
> | ^~~~~~~~~~~~~~~~~~~~~
> include/linux/btf.h:70:24: note: previous declaration of 'btf_type_by_name_kind' was here
> 70 | const struct btf_type *btf_type_by_name_kind(
> | ^~~~~~~~~~~~~~~~~~~~~
>
> vim +/btf_type_by_name_kind +148 include/linux/btf.h
>
> 136
> 137 #ifdef CONFIG_BPF_SYSCALL
> 138 const struct btf_type *btf_type_by_id(const struct btf *btf, u32 type_id);
> 139 const char *btf_name_by_offset(const struct btf *btf, u32 offset);
> 140 struct btf *btf_parse_vmlinux(void);
> 141 struct btf *bpf_prog_get_target_btf(const struct bpf_prog *prog);
> 142 #else
> 143 static inline const struct btf_type *btf_type_by_id(const struct btf *btf,
> 144 u32 type_id)
> 145 {
> 146 return NULL;
> 147 }
> > 148 static inline const struct btf_type *btf_type_by_name_kind(
> 149 struct btf *btf, const char *name, u8 kind)
> 150 {
> 151 return ERR_PTR(-EOPNOTSUPP);
> 152 }
> 153 static inline const char *btf_name_by_offset(const struct btf *btf,
> 154 u32 offset)
> 155 {
> 156 return NULL;
> 157 }
> 158 #endif
> 159
>
> ---
> 0-DAY kernel test infrastructure Open Source Technology Center
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org Intel Corporation
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox