Re: [PATCH bpf-next 1/2] fs/xattr: bpf: Introduce security.bpf xattr name prefix

Jiri Olsa
2024-10-07T08:40:01Z

On Wed, Oct 02, 2024 at 02:46:36PM -0700, Song Liu wrote:
> Introduct new xattr name prefix security.bpf, and enable reading these
> xattrs from bpf kfuncs bpf_get_[file|dentry]_xattr(). Note that
> "security.bpf" could be the name of the xattr or the prefix of the
> name. For example, both "security.bpf" and "security.bpf.xxx" are
> valid xattr name; while "security.bpfxxx" is not valid.
>
> Signed-off-by: Song Liu <song@kernel.org>
> ---
> fs/bpf_fs_kfuncs.c | 19 ++++++++++++++++++-
> include/uapi/linux/xattr.h | 4 ++++
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 3fe9f59ef867..339c4fef8f6e 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
> @@ -93,6 +93,23 @@ __bpf_kfunc int bpf_path_d_path(struct path *path, char *buf, size_t buf__sz)
> return len;
> }
>
> +static bool bpf_xattr_name_allowed(const char *name__str)
> +{
> + /* Allow xattr names with user. prefix */
> + if (!strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> + return true;
> +
> + /* Allow security.bpf. prefix or just security.bpf */
> + if (!strncmp(name__str, XATTR_NAME_BPF_LSM, XATTR_NAME_BPF_LSM_LEN) &&
> + (name__str[XATTR_NAME_BPF_LSM_LEN] == '\0' ||
> + name__str[XATTR_NAME_BPF_LSM_LEN] == '.')) {
> + return true;
> + }
> +
> + /* Disallow anything else */
> + return false;
> +}
> +
> /**
> * bpf_get_dentry_xattr - get xattr of a dentry
> * @dentry: dentry to get xattr from
> @@ -117,7 +134,7 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st

nit, I guess the comment for bpf_get_dentry_xattr function needs to be updated

* For security reasons, only *name__str* with prefix "user." is allowed.

jirka

> if (WARN_ON(!inode))
> return -EINVAL;
>
> - if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> + if (!bpf_xattr_name_allowed(name__str))
> return -EPERM;
>
> value_len = __bpf_dynptr_size(value_ptr);
> diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
> index 9463db2dfa9d..166ef2f1f1b3 100644
> --- a/include/uapi/linux/xattr.h
> +++ b/include/uapi/linux/xattr.h
> @@ -76,6 +76,10 @@
> #define XATTR_CAPS_SUFFIX "capability"
> #define XATTR_NAME_CAPS XATTR_SECURITY_PREFIX XATTR_CAPS_SUFFIX
>
> +#define XATTR_BPF_LSM_SUFFIX "bpf"
> +#define XATTR_NAME_BPF_LSM (XATTR_SECURITY_PREFIX XATTR_BPF_LSM_SUFFIX)
> +#define XATTR_NAME_BPF_LSM_LEN (sizeof(XATTR_NAME_BPF_LSM) - 1)
> +
> #define XATTR_POSIX_ACL_ACCESS "posix_acl_access"
> #define XATTR_NAME_POSIX_ACL_ACCESS XATTR_SYSTEM_PREFIX XATTR_POSIX_ACL_ACCESS
> #define XATTR_POSIX_ACL_DEFAULT "posix_acl_default"
> --
> 2.43.5
>
>