linux-kernel - Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 16 Nov 2022 17:40:22 -0500
From:   Paul Moore <paul@...l-moore.com>
To:     KP Singh <kpsingh@...nel.org>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Roberto Sassu <roberto.sassu@...weicloud.com>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        Song Liu <song@...nel.org>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        Stanislav Fomichev <sdf@...gle.com>,
        Hao Luo <haoluo@...gle.com>, Jiri Olsa <jolsa@...nel.org>,
        Florent Revest <revest@...omium.org>,
        Brendan Jackman <jackmanb@...omium.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>, bpf <bpf@...r.kernel.org>,
        LSM List <linux-security-module@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Roberto Sassu <roberto.sassu@...wei.com>
Subject: Re: [PoC][PATCH] bpf: Call return value check function in the JITed code

On Wed, Nov 16, 2022 at 2:04 PM KP Singh <kpsingh@...nel.org> wrote:
> On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov
> <alexei.starovoitov@...il.com> wrote:
> >
> > On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> > <roberto.sassu@...weicloud.com> wrote:
> > >
> > > On Wed, 2022-11-16 at 08:16 -0800, Alexei Starovoitov wrote:
> > > > On Wed, Nov 16, 2022 at 7:48 AM Roberto Sassu
> > > > <roberto.sassu@...weicloud.com> wrote:
> > > > > +static bool is_ret_value_allowed(int ret, u32 ret_flags)
> > > > > +{
> > > > > +       if ((ret < 0 && !(ret_flags & LSM_RET_NEG)) ||
> > > > > +           (ret == 0 && !(ret_flags & LSM_RET_ZERO)) ||
> > > > > +           (ret == 1 && !(ret_flags & LSM_RET_ONE)) ||
> > > > > +           (ret > 1 && !(ret_flags & LSM_RET_GT_ONE)))
> > > > > +               return false;
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > >  /* For every LSM hook that allows attachment of BPF programs, declare a nop
> > > > >   * function where a BPF program can be attached.
> > > > >   */
> > > > > @@ -30,6 +41,15 @@ noinline RET bpf_lsm_##NAME(__VA_ARGS__)     \
> > > > >  #include <linux/lsm_hook_defs.h>
> > > > >  #undef LSM_HOOK
> > > > >
> > > > > +#define LSM_HOOK(RET, DEFAULT, RET_FLAGS, NAME, ...)   \
> > > > > +noinline RET bpf_lsm_##NAME##_ret(int ret)     \
> > > > > +{                                              \
> > > > > +       return is_ret_value_allowed(ret, RET_FLAGS) ? ret : DEFAULT; \
> > > > > +}
> > > > > +
> > > > > +#include <linux/lsm_hook_defs.h>
> > > > > +#undef LSM_HOOK
> > > > > +
> > > >
> > > > because lsm hooks is mess of undocumented return values your
> > > > "solution" is to add hundreds of noninline functions
> > > > and hack the call into them in JITs ?!
> > >
> > > I revisited the documentation and checked each LSM hook one by one.
> > > Hopefully, I completed it correctly, but I would review again (others
> > > are also welcome to do it).
> > >
> > > Not sure if there is a more efficient way. Do you have any idea?
> > > Maybe we find a way to use only one check function (by reusing the
> > > address of the attachment point?).
> > >
> > > Regarding the JIT approach, I didn't find a reliable solution for using
> > > just the verifier. As I wrote to you, there could be the case where the
> > > range can include positive values, despite the possible return values
> > > are zero and -EACCES.
> >
> > Didn't you find that there are only 12 or so odd return cases.
> > Maybe refactor some of them to something that the verifier can enforce
> > and denylist the rest ?
>
> +1

I'm not sure we want to refactor the LSM hooks right now, we've got
too much stuff in-progress which I consider higher value/priority.
While I'm generally in favor of improving the sanity of interfaces,
I'd much rather we resolve the IMA/EVM special cases and land the
stacking changes before we start playing with refactoring the hooks.
I know this is a bummer for the BPF folks, but the IMA/EVM and
stacking patches affect everybody.

-- 
paul-moore.com