From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754717AbcJEVZj (ORCPT ); Wed, 5 Oct 2016 17:25:39 -0400 Received: from mail-wm0-f54.google.com ([74.125.82.54]:36874 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751020AbcJEVZf (ORCPT ); Wed, 5 Oct 2016 17:25:35 -0400 MIME-Version: 1.0 In-Reply-To: <57F5696E.6020502@digikod.net> References: <20160914072415.26021-1-mic@digikod.net> <20160914072415.26021-17-mic@digikod.net> <57F5696E.6020502@digikod.net> From: Kees Cook Date: Wed, 5 Oct 2016 14:25:33 -0700 X-Google-Sender-Auth: sOKEQj48K6af4j0Atk_imSd52-A 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 u95LPkuh014574 On Wed, Oct 5, 2016 at 1:58 PM, Mickaël Salaün wrote: > > > On 04/10/2016 01:43, Kees Cook wrote: >> 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. :) > > 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