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

[Christian Brauner <brauner@kernel.org>]

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.

>  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?

> +}
> +
> +__bpf_kfunc_end_defs();
> +
> +BTF_SET8_START(fs_kfunc_set_ids)
> +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS)
> +BTF_SET8_END(fs_kfunc_set_ids)
> +
> +static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id)
> +{
> +	if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id))
> +		return 0;
> +
> +	/* Only allow to attach from LSM hooks, to avoid recursion */
> +	return prog->type != BPF_PROG_TYPE_LSM ? -EACCES : 0;
> +}
> +
> +const struct btf_kfunc_id_set bpf_fs_kfunc_set = {
> +	.owner = THIS_MODULE,
> +	.set = &fs_kfunc_set_ids,
> +	.filter = bpf_get_file_xattr_filter,
> +};
> +
> +static int __init bpf_fs_kfuncs_init(void)
> +{
> +	return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set);
> +}
> +
> +late_initcall(bpf_fs_kfuncs_init);
> +
>  static const struct bpf_func_proto *
>  bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
>  {
> -- 
> 2.34.1
>