Re: [RFC][PATCH] bpf: Check xattr name/value pair from bpf_lsm_inode_init_security()

[KP Singh]

On Wed, Oct 26, 2022 at 7:14 PM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
>
> On Wed, Oct 26, 2022 at 1:42 AM Roberto Sassu
> <roberto.sassu@xxxxxxxxxxxxxxx> wrote:
> >
> > On 10/26/2022 8:37 AM, Alexei Starovoitov wrote:
> > > On Tue, Oct 25, 2022 at 7:58 AM Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> > >>
> > >> On 10/25/2022 12:43 AM, Roberto Sassu wrote:
> > >>> On Mon, 2022-10-24 at 19:13 -0700, Alexei Starovoitov wrote:
> > >>>> I'm looking at security_inode_init_security() and it is indeed messy.
> > >>>> Per file system initxattrs callback that processes kmalloc-ed
> > >>>> strings.
> > >>>> Yikes.
> > >>>>
> > >>>> In the short term we should denylist inode_init_security hook to
> > >>>> disallow attaching bpf-lsm there. set/getxattr should be done
> > >>>> through kfuncs instead of such kmalloc-a-string hack.
> > >>> Inode_init_security is an example. It could be that the other hooks are
> > >>> affected too. What happens if they get arbitrary positive values too?
> > >>
> > >> TL;DR - Things will go cattywampus.
> > >>
> > >> The LSM infrastructure is an interface that has "grown organically",
> > >> and isn't necessarily consistent in what it requires of the security
> > >> module implementations. There are cases where it assumes that the
> > >> security module hooks are well behaved, as you've discovered. I have
> > >> no small amount of fear that someone is going to provide an eBPF
> > >> program for security_secid_to_secctx(). There has been an assumption,
> > >> oft stated, that all security modules are going to be reviewed as
> > >> part of the upstream process. The review process ought to catch hooks
> > >> that return unacceptable values. Alas, we've lost that with BPF.
> > >>
> > >> It would take a(nother) major overhaul of the LSM infrastructure to
> > >> make it safe against hooks that are not well behaved. From what I have
> > >> seen so far it wouldn't be easy/convenient/performant to do it in the
> > >> BPF security module either. I personally think that BPF needs to
> > >> ensure that the eBPF implementations don't return inappropriate values,
> > >> but I understand why that is problematic.
> > >
> > > That's an accurate statement. Thank you.
> > >
> > > Going back to the original question...
> > > We fix bugs when we discover them.
> > > Regardless of the subsystem they belong to.
> > > No finger pointing.
> >
> > I'm concerned about the following situation:
> >
> > struct <something> *function()
> > {
> >
> >         ret = security_*();
> >         if (ret)
> >                 return ERR_PTR(ret);
> >
> > }
> >
> > int caller()
> > {
> >         ptr = function()
> >         if (IS_ERR(ptr)
> >                 goto out;
> >
> >         <use of invalid pointer>
> > }
> >
> > I quickly found an occurrence of this:
> >
> > static int lookup_one_common()
> > {
> >
> > [...]
> >
> >         return inode_permission();
> > }
> >
> > struct dentry *try_lookup_one_len()
> > {
> >
> > [...]
> >
> >          err = lookup_one_common(&init_user_ns, name, base, len, &this);
> >          if (err)
> >                  return ERR_PTR(err);
> >
> >
> > Unfortunately, attaching to inode_permission causes the kernel
> > to crash immediately (it does not happen with negative return
> > values).
> >
> > So, I think the fix should be broader, and not limited to the
> > inode_init_security hook. Will try to see how it can be fixed.
>
> I see. Let's restrict bpf-lsm return values to IS_ERR_VALUE.
> Trivial verifier change.

Thanks, yes this indeed is an issue. We need to do a few things:

1. Restrict some hooks that we know the BPF LSM will never need.
2. A verifier function that checks return values of LSM
hooks.
For most LSK hooks IS_ERR_VALUE is fine, however, there are some hooks
like *xattr hooks that use a return value of 1 to indicate a
capability check is required which might need special handling.