From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753513AbcJCXnh (ORCPT ); Mon, 3 Oct 2016 19:43:37 -0400 Received: from mail-wm0-f45.google.com ([74.125.82.45]:35751 "EHLO mail-wm0-f45.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752422AbcJCXn3 (ORCPT ); Mon, 3 Oct 2016 19:43:29 -0400 MIME-Version: 1.0 In-Reply-To: <20160914072415.26021-17-mic@digikod.net> References: <20160914072415.26021-1-mic@digikod.net> <20160914072415.26021-17-mic@digikod.net> From: Kees Cook Date: Mon, 3 Oct 2016 16:43:26 -0700 X-Google-Sender-Auth: BBw9DdIviPce_S2C5ZYytef1dNs Message-ID: Subject: Re: [RFC v3 16/22] bpf/cgroup,landlock: Handle Landlock hooks per cgroup To: =?UTF-8?B?TWlja2HDq2wgU2FsYcO8bg==?= Cc: LKML , Alexei Starovoitov , Andy Lutomirski , Arnd Bergmann , Casey Schaufler , Daniel Borkmann , Daniel Mack , David Drysdale , "David S . Miller" , Elena Reshetova , "Eric W . Biederman" , James Morris , Paul Moore , Sargun Dhillon , "Serge E . Hallyn" , Tejun Heo , Will Drewry , "kernel-hardening@lists.openwall.com" , Linux API , linux-security-module , Network Development , Cgroups Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id u93NhfLh004554 On Wed, Sep 14, 2016 at 12:24 AM, Mickaël Salaün 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 > Cc: Alexei Starovoitov > Cc: Andy Lutomirski > Cc: Daniel Borkmann > Cc: Daniel Mack > Cc: David S. Miller > Cc: Kees Cook > Cc: Tejun Heo > Link: https://lkml.kernel.org/r/20160826021432.GA8291@ast-mbp.thefacebook.com > Link: https://lkml.kernel.org/r/20160827204307.GA43714@ast-mbp.thefacebook.com > --- > 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 > #include > #include > +#include > > 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. :) -Kees -- Kees Cook Nexus Security