Re: [RESEND][RFC][PATCH 2/3] bpf-lsm: Limit values that can be returned by security modules

[Alexei Starovoitov]

On Fri, Nov 4, 2022 at 8:29 AM Roberto Sassu
<roberto.sassu@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, 2022-11-03 at 16:09 +0100, KP Singh wrote:
> > On Fri, Oct 28, 2022 at 6:55 PM Roberto Sassu
> > <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> > > From: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > >
> > > BPF LSM defines a bpf_lsm_*() function for each LSM hook, so that
> > > security modules can define their own implementation for the desired hooks.
> > >
> > > Unfortunately, BPF LSM does not restrict which values security modules can
> > > return (for non-void LSM hooks). Security modules might follow the
> > > conventions stated in include/linux/lsm_hooks.h, or put arbitrary values.
> > >
> > > This could cause big troubles, as the kernel is not ready to handle
> > > possibly malicious return values from LSMs. Until now, it was not the
> >
> > I am not sure I would call this malicious. This would be incorrect, if
> > someone is writing a BPF LSM program they already have the powers
> > to willingly do a lot of malicious stuff.
> >
> > It's about unknowingly returning values that can break the system.
>
> Maybe it is possible to return specific values that lead to acquire
> more information/do actions that the eBPF program is not supposed to
> cause.
>
> I don't have a concrete example, so I will use the word you suggested.
>
> > > case, as each LSM is carefully reviewed and it won't be accepted if it
> > > does not meet the return value conventions.
> > >
> > > The biggest problem is when an LSM returns a positive value, instead of a
> > > negative value, as it could be converted to a pointer. Since such pointer
> > > escapes the IS_ERR() check, its use later in the code can cause
> > > unpredictable consequences (e.g. invalid memory access).
> > >
> > > Another problem is returning zero when an LSM is supposed to have done some
> > > operations. For example, the inode_init_security hook expects that their
> > > implementations return zero only if they set the name and value of the new
> > > xattr to be added to the new inode. Otherwise, other kernel subsystems
> > > might encounter unexpected conditions leading to a crash (e.g.
> > > evm_protected_xattr_common() getting NULL as argument).
> > >
> > > Finally, there are LSM hooks which are supposed to return just one as
> > > positive value, or non-negative values. Also in these cases, although it
> > > seems less critical, it is safer to return to callers of the LSM
> > > infrastructure more precisely what they expect.
> > >
> > > As eBPF allows code outside the kernel to run, it is its responsibility
> > > to ensure that only expected values are returned to LSM infrastructure
> > > callers.
> > >
> > > Create four new BTF ID sets, respectively for hooks that can return
> > > positive values, only one as positive value, that cannot return zero, and
> > > that cannot return negative values. Create also corresponding functions to
> > > check if the hook a security module is attached to belongs to one of the
> > > defined sets.
> > >
> > > Finally, check in the eBPF verifier the value returned by security modules
> > > for each attached LSM hook, and return -EINVAL (the security module cannot
> > > run) if the hook implementation does not satisfy the hook return value
> > > policy.
> > >
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: 9d3fdea789c8 ("bpf: lsm: Provide attachment points for BPF LSM programs")
> > > Signed-off-by: Roberto Sassu <roberto.sassu@xxxxxxxxxx>
> > > ---
> > >  include/linux/bpf_lsm.h | 24 ++++++++++++++++++
> > >  kernel/bpf/bpf_lsm.c    | 56 +++++++++++++++++++++++++++++++++++++++++
> > >  kernel/bpf/verifier.c   | 35 +++++++++++++++++++++++---
> > >  3 files changed, 112 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/include/linux/bpf_lsm.h b/include/linux/bpf_lsm.h
> > > index 4bcf76a9bb06..cd38aca4cfc0 100644
> > > --- a/include/linux/bpf_lsm.h
> > > +++ b/include/linux/bpf_lsm.h
> > > @@ -28,6 +28,10 @@ int bpf_lsm_verify_prog(struct bpf_verifier_log *vlog,
> > >                         const struct bpf_prog *prog);
> > >
> > >  bool bpf_lsm_is_sleepable_hook(u32 btf_id);
> > > +bool bpf_lsm_can_ret_pos_value(u32 btf_id);
> > > +bool bpf_lsm_can_ret_only_one_as_pos_value(u32 btf_id);
> > > +bool bpf_lsm_cannot_ret_zero(u32 btf_id);
> > > +bool bpf_lsm_cannot_ret_neg_value(u32 btf_id);
> > >
> >
> > This does not need to be exported to the rest of the kernel. Please
> > have this logic in bpf_lsm.c and export a single verify function.
> >
> > Also, these really don't need to be such scattered logic, Could we
> > somehow encode this into the LSM_HOOK definition?
>
> The problem is that a new LSM_HOOK definition would apply to every LSM
> hook, while we need the ability to select subsets.
>
> I was thinking, but I didn't check yet, what about using BTF_ID_FLAGS,
> introducing a flag for each interval (<0, 0, 1, >1) and setting the
> appropriate flags for each LSM hook?

Before adding infra to all hooks, let's analyze all hooks first.
I thought the number of exceptions is very small.
99% of hooks will be fine with IS_ERR.
If so, adding an extra flag to every hook will cause too much churn.