Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <700dffccdfeeb3d19c5385550e4c84f08c705e19.camel@huaweicloud.com>
 <20221116154712.4115929-1-roberto.sassu@huaweicloud.com> <CAADnVQLQswvu3oGyeevLrKMT200yD4hzCbkBUAs=1bKSDVaOQg@mail.gmail.com>
 <05bf553f795ac93ea3032cfc1b56ca35fd6a920a.camel@huaweicloud.com> <CAADnVQJu7isDCi4+f8s4LfiwcYJbN4kXkvgJ8+ZnsS+QGDVnMw@mail.gmail.com>
In-Reply-To: <CAADnVQJu7isDCi4+f8s4LfiwcYJbN4kXkvgJ8+ZnsS+QGDVnMw@mail.gmail.com>
From:   KP Singh <kpsingh@kernel.org>
Date:   Wed, 16 Nov 2022 20:04:42 +0100
Message-ID: <CACYkzJ6-zLNZhRmpaax+_ZywWLa5R2HwcxRke8hCHu_FgYceeA@mail.gmail.com>
Subject: Re: [PoC][PATCH] bpf: Call return value check function in the JITed code
To:     Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc:     Roberto Sassu <roberto.sassu@huaweicloud.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <martin.lau@linux.dev>,
        Song Liu <song@kernel.org>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        Stanislav Fomichev <sdf@google.com>,
        Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org>,
        Florent Revest <revest@chromium.org>,
        Brendan Jackman <jackmanb@chromium.org>,
        Paul Moore <paul@paul-moore.com>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>, bpf <bpf@vger.kernel.org>,
        LSM List <linux-security-module@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Roberto Sassu <roberto.sassu@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Nov 16, 2022 at 6:55 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Wed, Nov 16, 2022 at 8:41 AM Roberto Sassu
> <roberto.sassu@huaweicloud.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@huaweicloud.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

>
> Also denylist those that Casey mentioned like security_secid_to_secctx ?

Just replied to Casey's comment and I agree, these hooks should be denylisted.