Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200323164415.12943-1-kpsingh@chromium.org> <20200323164415.12943-6-kpsingh@chromium.org>
 <202003231237.F654B379@keescook> <20200323194759.GB18787@chromium.org>
In-Reply-To: <20200323194759.GB18787@chromium.org>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Mon, 23 Mar 2020 13:21:02 -0700
Message-ID: <CAEf4BzZTsssnKgRFEzZBOnBMjCxk2wzkq7j_bOHmPpt5RmhqKA@mail.gmail.com>
Subject: Re: [PATCH bpf-next v5 5/7] bpf: lsm: Initialize the BPF LSM hooks
To:     KP Singh <kpsingh@chromium.org>
Cc:     Kees Cook <keescook@chromium.org>,
        open list <linux-kernel@vger.kernel.org>,
        bpf <bpf@vger.kernel.org>, linux-security-module@vger.kernel.org,
        Brendan Jackman <jackmanb@google.com>,
        Florent Revest <revest@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        James Morris <jmorris@namei.org>, Paul Turner <pjt@google.com>,
        Jann Horn <jannh@google.com>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, Mar 23, 2020 at 12:48 PM KP Singh <kpsingh@chromium.org> wrote:
>
> On 23-M=C3=A4r 12:44, Kees Cook wrote:
> > On Mon, Mar 23, 2020 at 05:44:13PM +0100, KP Singh wrote:
> > > From: KP Singh <kpsingh@google.com>
> > >
> > > The bpf_lsm_ nops are initialized into the LSM framework like any oth=
er
> > > LSM.  Some LSM hooks do not have 0 as their default return value. The
> > > __weak symbol for these hooks is overridden by a corresponding
> > > definition in security/bpf/hooks.c
> > >
> > > The LSM can be enabled / disabled with CONFIG_LSM.
> > >
> > > Signed-off-by: KP Singh <kpsingh@google.com>
> >
> > Nice! This is super clean on the LSM side of things. :)
> >
> > One note below...
> >
> > > Reviewed-by: Brendan Jackman <jackmanb@google.com>
>
> [...]
>
> > > +
> > > +/*
> > > + * Copyright (C) 2020 Google LLC.
> > > + */
> > > +#include <linux/lsm_hooks.h>
> > > +#include <linux/bpf_lsm.h>
> > > +
> > > +/* Some LSM hooks do not have 0 as their default return values. Over=
ride the
> > > + * __weak definitons generated by default for these hooks
> >
> > If you wanted to avoid this, couldn't you make the default return value
> > part of lsm_hooks.h?
> >
> > e.g.:
> >
> > LSM_HOOK(int, -EOPNOTSUPP, inode_getsecurity, struct inode *inode,
> >        const char *name, void **buffer, bool alloc)
> >
> > ...
> >
> > #define LSM_HOOK(RET, DEFAULT, NAME, ...)     \
> >       LSM_HOOK_##RET(NAME, DEFAULT, __VA_ARGS__)
> > ...
> > #define LSM_HOOK_int(NAME, DEFAULT, ...)      \
> > noinline int bpf_lsm_##NAME(__VA_ARGS__)      \
> > {                                             \
> >       return (DEFAULT);                       \
> > }
> >
> > Then all the __weak stuff is gone, and the following 4 functions don't
> > need to be written out, and the information is available to the macros
> > if anyone else might ever want it.
>
> Thanks, I like it!
>
> If no-one objects, I will update it in the next revision.
>

I was about to propose the same, explicit default value seems like a
much cleaner and more straightforward way to do this.

> - KP
>
> >
> > -Kees
> >
> > > + */
> > > +noinline int bpf_lsm_inode_getsecurity(struct inode *inode, const ch=
ar *name,
> > > +                                  void **buffer, bool alloc)
> > > +};
>
> [...]
>
> > > --
> > > 2.20.1
> > >
> >
> > --
> > Kees Cook