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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZwOeXj9GGt7RdqsQ@krava>
Date: Mon, 7 Oct 2024 10:39:58 +0200
From: Jiri Olsa <olsajiri@...il.com>
To: Song Liu <song@...nel.org>
Cc: bpf@...r.kernel.org, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org, kernel-team@...a.com,
	andrii@...nel.org, eddyz87@...il.com, ast@...nel.org,
	daniel@...earbox.net, martin.lau@...ux.dev, viro@...iv.linux.org.uk,
	brauner@...nel.org, jack@...e.cz, kpsingh@...nel.org,
	mattbobrowski@...gle.com
Subject: Re: [PATCH bpf-next 1/2] fs/xattr: bpf: Introduce security.bpf xattr
 name prefix

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@...nel.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
> 
>