On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
>
> 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?
Thanks,
Song
> > +}
> > +
[...]
