On Wed, Oct 5, 2016 at 1:58 PM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: > > > On 04/10/2016 01:43, Kees Cook wrote: >> On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün <mic@xxxxxxxxxxx> wrote: >>> This allows to add new eBPF programs to Landlock hooks dedicated to a >>> cgroup thanks to the BPF_PROG_ATTACH command. Like for socket eBPF >>> programs, the Landlock hooks attached to a cgroup are propagated to the >>> nested cgroups. However, when a new Landlock program is attached to one >>> of this nested cgroup, this cgroup hierarchy fork the Landlock hooks. >>> This design is simple and match the current CONFIG_BPF_CGROUP >>> inheritance. The difference lie in the fact that Landlock programs can >>> only be stacked but not removed. This match the append-only seccomp >>> behavior. Userland is free to handle Landlock hooks attached to a cgroup >>> in more complicated ways (e.g. continuous inheritance), but care should >>> be taken to properly handle error cases (e.g. memory allocation errors). >>> >>> Changes since v2: >>> * new design based on BPF_PROG_ATTACH (suggested by Alexei Starovoitov) >>> >>> Signed-off-by: Mickaël Salaün <mic@xxxxxxxxxxx> >>> Cc: Alexei Starovoitov <ast@xxxxxxxxxx> >>> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx> >>> Cc: Daniel Borkmann <daniel@xxxxxxxxxxxxx> >>> Cc: Daniel Mack <daniel@xxxxxxxxxx> >>> Cc: David S. Miller <davem@xxxxxxxxxxxxx> >>> Cc: Kees Cook <keescook@xxxxxxxxxxxx> >>> Cc: Tejun Heo <tj@xxxxxxxxxx> >>> Link: https://lkml.kernel.org/r/20160826021432.GA8291@xxxxxxxxxxxxxxxxxxxxxxx >>> Link: https://lkml.kernel.org/r/20160827204307.GA43714@xxxxxxxxxxxxxxxxxxxxxxx >>> --- >>> include/linux/bpf-cgroup.h | 7 +++++++ >>> include/linux/cgroup-defs.h | 2 ++ >>> include/linux/landlock.h | 9 +++++++++ >>> include/uapi/linux/bpf.h | 1 + >>> kernel/bpf/cgroup.c | 33 ++++++++++++++++++++++++++++++--- >>> kernel/bpf/syscall.c | 11 +++++++++++ >>> security/landlock/lsm.c | 40 +++++++++++++++++++++++++++++++++++++++- >>> security/landlock/manager.c | 32 ++++++++++++++++++++++++++++++++ >>> 8 files changed, 131 insertions(+), 4 deletions(-) >>> >>> [...] >>> diff --git a/kernel/bpf/cgroup.c b/kernel/bpf/cgroup.c >>> index 7b75fa692617..1c18fe46958a 100644 >>> --- a/kernel/bpf/cgroup.c >>> +++ b/kernel/bpf/cgroup.c >>> @@ -15,6 +15,7 @@ >>> #include <linux/bpf.h> >>> #include <linux/bpf-cgroup.h> >>> #include <net/sock.h> >>> +#include <linux/landlock.h> >>> >>> DEFINE_STATIC_KEY_FALSE(cgroup_bpf_enabled_key); >>> EXPORT_SYMBOL(cgroup_bpf_enabled_key); >>> @@ -31,7 +32,15 @@ void cgroup_bpf_put(struct cgroup *cgrp) >>> union bpf_object pinned = cgrp->bpf.pinned[type]; >>> >>> if (pinned.prog) { >>> - bpf_prog_put(pinned.prog); >>> + switch (type) { >>> + case BPF_CGROUP_LANDLOCK: >>> +#ifdef CONFIG_SECURITY_LANDLOCK >>> + put_landlock_hooks(pinned.hooks); >>> + break; >>> +#endif /* CONFIG_SECURITY_LANDLOCK */ >>> + default: >>> + bpf_prog_put(pinned.prog); >>> + } >>> static_branch_dec(&cgroup_bpf_enabled_key); >>> } >>> } >> >> I get creeped out by type-controlled unions of pointers. :P I don't >> have a suggestion to improve this, but I don't like seeing a pointer >> type managed separately from the pointer itself as it tends to bypass >> a lot of both static and dynamic checking. A union is better than a >> cast of void *, but it still worries me. :) > > This is not fully satisfactory for me neither but the other approach is > to use two distinct struct fields instead of a union. > Do you prefer if there is a "type" field in the "pinned" struct to > select the union? Since memory usage isn't a huge deal for this, I'd actually prefer there just be no union at all. Have a type field, and a distinct pointer field for each type you're expecting to use. That way there can never be confusion between types and you could even validate that only a single field type has been populated, etc. -Kees -- Kees Cook Nexus Security -- To unsubscribe from this list: send the line "unsubscribe linux-api" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
