Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Christian Brauner <brauner@kernel.org>]

On Fri, Nov 24, 2023 at 09:07:33AM -0800, Song Liu wrote:
> On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@kernel.org> wrote:
> >
> > On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu wrote:
> > > It is common practice for security solutions to store tags/labels in
> > > xattrs. To implement similar functionalities in BPF LSM, add new kfunc
> > > bpf_get_file_xattr().
> > >
> > > The first use case of bpf_get_file_xattr() is to implement file
> > > verifications with asymmetric keys. Specificially, security applications
> > > could use fsverity for file hashes and use xattr to store file signatures.
> > > (kfunc for fsverity hash will be added in a separate commit.)
> > >
> > > Currently, only xattrs with "user." prefix can be read with kfunc
> > > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix
> > > for bpf_get_file_xattr().
> > >
> > > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks.
> > >
> > > Signed-off-by: Song Liu <song@kernel.org>
> > > ---
> >
> > Looks ok to me. But see below for a question.
> >
> > If you ever allow the retrieval of additional extended attributes
> > through bfs_get_file_xattr() or other bpf interfaces we would like to be
> > Cced, please. The xattr stuff is (/me looks for suitable words)...
> >
> > Over the last months we've moved POSIX_ACL retrieval out of these
> > low-level functions. They now have a dedicated api. The same is going to
> > happen for fscaps as well.
> >
> > But even with these out of the way we would want the bpf helpers to
> > always maintain an allowlist of retrievable attributes.
> 
> Agreed. We will be very specific which attributes are available to bpf
> helpers/kfuncs.
> 
> >
> > >  kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 63 insertions(+)
> > >
> > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> > > index f0b8b7c29126..55758a6fbe90 100644
> > > --- a/kernel/trace/bpf_trace.c
> > > +++ b/kernel/trace/bpf_trace.c
> > > @@ -24,6 +24,7 @@
> > >  #include <linux/key.h>
> > >  #include <linux/verification.h>
> > >  #include <linux/namei.h>
> > > +#include <linux/fileattr.h>
> > >
> > >  #include <net/bpf_sk_storage.h>
> > >
> > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void)
> > >  late_initcall(bpf_key_sig_kfuncs_init);
> > >  #endif /* CONFIG_KEYS */
> > >
> > > +/* filesystem kfuncs */
> > > +__bpf_kfunc_start_defs();
> > > +
> > > +/**
> > > + * bpf_get_file_xattr - get xattr of a file
> > > + * @file: file to get xattr from
> > > + * @name__str: name of the xattr
> > > + * @value_ptr: output buffer of the xattr value
> > > + *
> > > + * Get xattr *name__str* of *file* and store the output in *value_ptr*.
> > > + *
> > > + * For security reasons, only *name__str* with prefix "user." is allowed.
> > > + *
> > > + * Return: 0 on success, a negative value on error.
> > > + */
> > > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
> > > +                                struct bpf_dynptr_kern *value_ptr)
> > > +{
> > > +     struct dentry *dentry;
> > > +     u32 value_len;
> > > +     void *value;
> > > +
> > > +     if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> > > +             return -EPERM;
> > > +
> > > +     value_len = __bpf_dynptr_size(value_ptr);
> > > +     value = __bpf_dynptr_data_rw(value_ptr, value_len);
> > > +     if (!value)
> > > +             return -EINVAL;
> > > +
> > > +     dentry = file_dentry(file);
> > > +     return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len);
> >
> > By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at
> > least inode_permission() from xattr_permission(). I'm probably just
> > missing or forgot the context. But why is that ok?
> 
> AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
> check in xattr_permission().
> 
> For inode_permission(), I think it is not required because we already
> have the "struct file" of  the target file. Did I misunderstand something
> here?

I had overlooked that you don't allow writing xattrs. But there's still
some issues:

So if you look at the system call interface:

fgetxattr(fd)
-> getxattr()
   -> do_getxattr()
      -> vfs_getxattr()
         -> xattr_permission()
         -> __vfs_getxattr()

and io_uring:

do_getxattr()
-> vfs_getxattr()
   -> xattr_permission()
   -> __vfs_getxattr()

you can see that xattr_permission() is a _read/write-time check_, not an
open check. That's because the read/write permissions may depend on what
xattr is read/written. Since you don't know what xattr will be
read/written at open-time.

So there needs to be a good reason for bpf_get_file_xattr() to deviate
from the system call and io_uring interface. And I'd like to hear it,
please. :)

I think I might see the argument because you document the helper as "may
only be called from BPF LSM function" in which case you're trying to say
that bpf_get_file_xattr() is equivalent to a call to __vfs_getxattr()
from an LSM to get at it's own security xattr.

But if that's the case you really should have a way to verify that these
helpers are only callable from a specific BPF context. Because you
otherwise omit read/write-time permission checking when retrieving
xattrs which is a potentialy security issue and may be abused by a BPF
program to skip permission checks that are otherwise enforced.

Is there a way for BPF to enforce/verify that a function is only called
from a specific BPF program? It should be able to recognize that, no?
And then refuse to load that BPF program if a helper is called outside
it's intended context.