[PATCH bpf-next 0/2] security.bpf xattr name prefix

[PATCH bpf-next 0/2] security.bpf xattr name prefix

[Song Liu]

Follow up discussion in LPC 2024 [1], that we need security.bpf xattr
prefix. This set adds "security.bpf" xattr name prefix, and allows
bpf kfuncs bpf_get_[file|dentry]_xattr() to read these xattrs.


[1] https://lpc.events/event/18/contributions/1940/

Song Liu (2):
  fs/xattr: bpf: Introduce security.bpf xattr name prefix
  selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

 fs/bpf_fs_kfuncs.c                            | 19 ++++++++-
 include/uapi/linux/xattr.h                    |  4 ++
 .../selftests/bpf/prog_tests/fs_kfuncs.c      | 40 ++++++++++++++-----
 .../selftests/bpf/progs/test_get_xattr.c      | 30 ++++++++++++--
 4 files changed, 78 insertions(+), 15 deletions(-)

--
2.43.5

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

[Song Liu]

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

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

[Jiri Olsa]

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

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

[Song Liu]

Hi Jiri,

Thanks for the review!

> On Oct 7, 2024, at 1:39 AM, Jiri Olsa <olsajiri@gmail.com> wrote:
> 
> 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.

Good catch! We can update it as:

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

Song

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

[Christian Brauner]

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

Acked-by: Christian Brauner <brauner@kernel.org>

[PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
"security.bpfxxx" and "security.selinux" cannot be read.

Signed-off-by: Song Liu <song@kernel.org>
---
 .../selftests/bpf/prog_tests/fs_kfuncs.c      | 40 ++++++++++++++-----
 .../selftests/bpf/progs/test_get_xattr.c      | 30 ++++++++++++--
 2 files changed, 56 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
index 5a0b51157451..986dd5eabaa6 100644
--- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
+++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
@@ -12,7 +12,7 @@
 
 static const char testfile[] = "/tmp/test_progs_fs_kfuncs";
 
-static void test_xattr(void)
+static void test_get_xattr(const char *name, const char *value, bool allow_access)
 {
 	struct test_get_xattr *skel = NULL;
 	int fd = -1, err;
@@ -25,7 +25,7 @@ static void test_xattr(void)
 	close(fd);
 	fd = -1;
 
-	err = setxattr(testfile, "user.kfuncs", "hello", sizeof("hello"), 0);
+	err = setxattr(testfile, name, value, strlen(value) + 1, 0);
 	if (err && errno == EOPNOTSUPP) {
 		printf("%s:SKIP:local fs doesn't support xattr (%d)\n"
 		       "To run this test, make sure /tmp filesystem supports xattr.\n",
@@ -48,16 +48,23 @@ static void test_xattr(void)
 		goto out;
 
 	fd = open(testfile, O_RDONLY, 0644);
+
 	if (!ASSERT_GE(fd, 0, "open_file"))
 		goto out;
 
-	ASSERT_EQ(skel->bss->found_xattr_from_file, 1, "found_xattr_from_file");
-
 	/* Trigger security_inode_getxattr */
-	err = getxattr(testfile, "user.kfuncs", v, sizeof(v));
-	ASSERT_EQ(err, -1, "getxattr_return");
-	ASSERT_EQ(errno, EINVAL, "getxattr_errno");
-	ASSERT_EQ(skel->bss->found_xattr_from_dentry, 1, "found_xattr_from_dentry");
+	err = getxattr(testfile, name, v, sizeof(v));
+
+	if (allow_access) {
+		ASSERT_EQ(err, -1, "getxattr_return");
+		ASSERT_EQ(errno, EINVAL, "getxattr_errno");
+		ASSERT_EQ(skel->bss->found_xattr_from_file, 1, "found_xattr_from_file");
+		ASSERT_EQ(skel->bss->found_xattr_from_dentry, 1, "found_xattr_from_dentry");
+	} else {
+		ASSERT_EQ(err, strlen(value) + 1, "getxattr_return");
+		ASSERT_EQ(skel->bss->found_xattr_from_file, 0, "found_xattr_from_file");
+		ASSERT_EQ(skel->bss->found_xattr_from_dentry, 0, "found_xattr_from_dentry");
+	}
 
 out:
 	close(fd);
@@ -141,8 +148,21 @@ static void test_fsverity(void)
 
 void test_fs_kfuncs(void)
 {
-	if (test__start_subtest("xattr"))
-		test_xattr();
+	/* Matches xattr_names in progs/test_get_xattr.c */
+	if (test__start_subtest("user_xattr"))
+		test_get_xattr("user.kfuncs", "hello", true);
+
+	if (test__start_subtest("security_bpf_xattr_1"))
+		test_get_xattr("security.bpf", "hello", true);
+
+	if (test__start_subtest("security_bpf_xattr_2"))
+		test_get_xattr("security.bpf.xxx", "hello", true);
+
+	if (test__start_subtest("security_bpfxxx_xattr_error"))
+		test_get_xattr("security.bpfxxx", "hello", false);
+
+	if (test__start_subtest("security_selinux_xattr_error"))
+		test_get_xattr("security.selinux", "hello", false);
 
 	if (test__start_subtest("fsverity"))
 		test_fsverity();
diff --git a/tools/testing/selftests/bpf/progs/test_get_xattr.c b/tools/testing/selftests/bpf/progs/test_get_xattr.c
index 66e737720f7c..0be8120683cd 100644
--- a/tools/testing/selftests/bpf/progs/test_get_xattr.c
+++ b/tools/testing/selftests/bpf/progs/test_get_xattr.c
@@ -17,12 +17,26 @@ static const char expected_value[] = "hello";
 char value1[32];
 char value2[32];
 
+#define NUM_OF_XATTR_NAME 5
+
+/* Matches caller of test_get_xattr() in prog_tests/fs_kfuncs.c */
+static const char *xattr_names[NUM_OF_XATTR_NAME] = {
+	/* The following work. */
+	"user.kfuncs",
+	"security.bpf",
+	"security.bpf.xxx",
+
+	/* The following do not work. */
+	"security.bpfxxx",
+	"security.selinux"
+};
+
 SEC("lsm.s/file_open")
 int BPF_PROG(test_file_open, struct file *f)
 {
 	struct bpf_dynptr value_ptr;
 	__u32 pid;
-	int ret;
+	int ret, i;
 
 	pid = bpf_get_current_pid_tgid() >> 32;
 	if (pid != monitored_pid)
@@ -30,7 +44,11 @@ int BPF_PROG(test_file_open, struct file *f)
 
 	bpf_dynptr_from_mem(value1, sizeof(value1), 0, &value_ptr);
 
-	ret = bpf_get_file_xattr(f, "user.kfuncs", &value_ptr);
+	for (i = 0; i < NUM_OF_XATTR_NAME; i++) {
+		ret = bpf_get_file_xattr(f, xattr_names[i], &value_ptr);
+		if (ret == sizeof(expected_value))
+			break;
+	}
 	if (ret != sizeof(expected_value))
 		return 0;
 	if (bpf_strncmp(value1, ret, expected_value))
@@ -44,7 +62,7 @@ int BPF_PROG(test_inode_getxattr, struct dentry *dentry, char *name)
 {
 	struct bpf_dynptr value_ptr;
 	__u32 pid;
-	int ret;
+	int ret, i;
 
 	pid = bpf_get_current_pid_tgid() >> 32;
 	if (pid != monitored_pid)
@@ -52,7 +70,11 @@ int BPF_PROG(test_inode_getxattr, struct dentry *dentry, char *name)
 
 	bpf_dynptr_from_mem(value2, sizeof(value2), 0, &value_ptr);
 
-	ret = bpf_get_dentry_xattr(dentry, "user.kfuncs", &value_ptr);
+	for (i = 0; i < NUM_OF_XATTR_NAME; i++) {
+		ret = bpf_get_dentry_xattr(dentry, xattr_names[i], &value_ptr);
+		if (ret == sizeof(expected_value))
+			break;
+	}
 	if (ret != sizeof(expected_value))
 		return 0;
 	if (bpf_strncmp(value2, ret, expected_value))
-- 
2.43.5

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Wed, Oct 02, 2024 at 02:46:37PM -0700, Song Liu wrote:
> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
> "security.bpfxxx" and "security.selinux" cannot be read.

So you read code from untrusted user.* xattrs?  How can you carve out
that space and not known any pre-existing userspace cod uses kfuncs
for it's own purpose?

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

Hi Christoph,  

> On Oct 14, 2024, at 10:07 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Wed, Oct 02, 2024 at 02:46:37PM -0700, Song Liu wrote:
>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
>> "security.bpfxxx" and "security.selinux" cannot be read.
> 
> So you read code from untrusted user.* xattrs?  How can you carve out
> that space and not known any pre-existing userspace cod uses kfuncs
> for it's own purpose?

I don't quite follow the comment here. 

Do you mean user.* xattrs are untrusted (any user can set it), so we 
should not allow BPF programs to read them? Or do you mean xattr 
name "user.kfuncs" might be taken by some use space?

Thanks,
Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
> >> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
> >> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
> >> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
> >> "security.bpfxxx" and "security.selinux" cannot be read.
> > 
> > So you read code from untrusted user.* xattrs?  How can you carve out
> > that space and not known any pre-existing userspace cod uses kfuncs
> > for it's own purpose?
> 
> I don't quite follow the comment here. 
> 
> Do you mean user.* xattrs are untrusted (any user can set it), so we 
> should not allow BPF programs to read them? Or do you mean xattr 
> name "user.kfuncs" might be taken by some use space?

All of the above.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

> On Oct 14, 2024, at 10:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
>>>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
>>>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
>>>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
>>>> "security.bpfxxx" and "security.selinux" cannot be read.
>>> 
>>> So you read code from untrusted user.* xattrs?  How can you carve out
>>> that space and not known any pre-existing userspace cod uses kfuncs
>>> for it's own purpose?
>> 
>> I don't quite follow the comment here. 
>> 
>> Do you mean user.* xattrs are untrusted (any user can set it), so we 
>> should not allow BPF programs to read them? Or do you mean xattr 
>> name "user.kfuncs" might be taken by some use space?
> 
> All of the above.

This is a selftest, "user.kfunc" is picked for this test. The kfuncs
(bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 

Reading untrusted xattrs from trust BPF LSM program can be useful. 
For example, we can sign a binary with private key, and save the
signature in the xattr. Then the kernel can verify the signature
and the binary matches the public key. If the xattr is modified by
untrusted user space, the BPF program will just deny the access. 

Did these answer your questions?

Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 05:52:02AM +0000, Song Liu wrote:
> >> Do you mean user.* xattrs are untrusted (any user can set it), so we 
> >> should not allow BPF programs to read them? Or do you mean xattr 
> >> name "user.kfuncs" might be taken by some use space?
> > 
> > All of the above.
> 
> This is a selftest, "user.kfunc" is picked for this test. The kfuncs
> (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
> 
> Reading untrusted xattrs from trust BPF LSM program can be useful. 
> For example, we can sign a binary with private key, and save the
> signature in the xattr. Then the kernel can verify the signature
> and the binary matches the public key.

I would expect that to be done through an actual privileged interface.
Taking an arbitrary name that was available for use by user space
programs for 20 years and now giving it a new meaning is not a good
idea.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

Hi Christoph,

> On Oct 14, 2024, at 11:42 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Tue, Oct 15, 2024 at 05:52:02AM +0000, Song Liu wrote:
>>>> Do you mean user.* xattrs are untrusted (any user can set it), so we 
>>>> should not allow BPF programs to read them? Or do you mean xattr 
>>>> name "user.kfuncs" might be taken by some use space?
>>> 
>>> All of the above.
>> 
>> This is a selftest, "user.kfunc" is picked for this test. The kfuncs
>> (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
>> 
>> Reading untrusted xattrs from trust BPF LSM program can be useful. 
>> For example, we can sign a binary with private key, and save the
>> signature in the xattr. Then the kernel can verify the signature
>> and the binary matches the public key.
> 
> I would expect that to be done through an actual privileged interface.
> Taking an arbitrary name that was available for use by user space
> programs for 20 years and now giving it a new meaning is not a good
> idea.

Agreed that using security.bpf xattrs are better for this use case. 
In fact, this patchset adds the support for security.bpf xattrs. 
Support for user.* xattrs were added last year. 

Thanks,
Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Tue, Oct 15, 2024 at 01:54:08PM +0000, Song Liu wrote:
> Agreed that using security.bpf xattrs are better for this use case. 
> In fact, this patchset adds the support for security.bpf xattrs. 
> Support for user.* xattrs were added last year. 

I think we'll need to revert it ASAP before it hits distros.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Jan Kara]

On Tue 15-10-24 05:52:02, Song Liu wrote:
> > On Oct 14, 2024, at 10:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
> >>>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
> >>>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
> >>>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
> >>>> "security.bpfxxx" and "security.selinux" cannot be read.
> >>> 
> >>> So you read code from untrusted user.* xattrs?  How can you carve out
> >>> that space and not known any pre-existing userspace cod uses kfuncs
> >>> for it's own purpose?
> >> 
> >> I don't quite follow the comment here. 
> >> 
> >> Do you mean user.* xattrs are untrusted (any user can set it), so we 
> >> should not allow BPF programs to read them? Or do you mean xattr 
> >> name "user.kfuncs" might be taken by some use space?
> > 
> > All of the above.
> 
> This is a selftest, "user.kfunc" is picked for this test. The kfuncs
> (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
> 
> Reading untrusted xattrs from trust BPF LSM program can be useful. 
> For example, we can sign a binary with private key, and save the
> signature in the xattr. Then the kernel can verify the signature
> and the binary matches the public key. If the xattr is modified by
> untrusted user space, the BPF program will just deny the access. 

So I tend to agree with Christoph that e.g. for the above LSM usecase you
mention, using user. xattr space is a poor design choice because you have
to very carefully validate any xattr contents (anybody can provide
malicious content) and more importantly as different similar usecases
proliferate the chances of name collisions and resulting funcionality
issues increase. It is similar as if you decided to store some information
in a specially named file in each directory. If you choose special enough
name, it will likely work but long-term someone is going to break you :)

I think that getting user.* xattrs from bpf hooks can still be useful for
introspection and other tasks so I'm not convinced we should revert that
functionality but maybe it is too easy to misuse? I'm not really decided.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christian Brauner]

On Wed, Oct 16, 2024 at 03:51:55PM +0200, Jan Kara wrote:
> On Tue 15-10-24 05:52:02, Song Liu wrote:
> > > On Oct 14, 2024, at 10:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
> > > On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
> > >>>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
> > >>>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
> > >>>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
> > >>>> "security.bpfxxx" and "security.selinux" cannot be read.
> > >>> 
> > >>> So you read code from untrusted user.* xattrs?  How can you carve out
> > >>> that space and not known any pre-existing userspace cod uses kfuncs
> > >>> for it's own purpose?
> > >> 
> > >> I don't quite follow the comment here. 
> > >> 
> > >> Do you mean user.* xattrs are untrusted (any user can set it), so we 
> > >> should not allow BPF programs to read them? Or do you mean xattr 
> > >> name "user.kfuncs" might be taken by some use space?
> > > 
> > > All of the above.
> > 
> > This is a selftest, "user.kfunc" is picked for this test. The kfuncs
> > (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
> > 
> > Reading untrusted xattrs from trust BPF LSM program can be useful. 
> > For example, we can sign a binary with private key, and save the
> > signature in the xattr. Then the kernel can verify the signature
> > and the binary matches the public key. If the xattr is modified by
> > untrusted user space, the BPF program will just deny the access. 
> 
> So I tend to agree with Christoph that e.g. for the above LSM usecase you
> mention, using user. xattr space is a poor design choice because you have
> to very carefully validate any xattr contents (anybody can provide
> malicious content) and more importantly as different similar usecases
> proliferate the chances of name collisions and resulting funcionality
> issues increase. It is similar as if you decided to store some information
> in a specially named file in each directory. If you choose special enough
> name, it will likely work but long-term someone is going to break you :)
> 
> I think that getting user.* xattrs from bpf hooks can still be useful for
> introspection and other tasks so I'm not convinced we should revert that
> functionality but maybe it is too easy to misuse? I'm not really decided.

Reading user.* xattr is fine. If an LSM decides to built a security
model around it then imho that's their business and since that happens
in out-of-tree LSM programs: shrug.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

> On Oct 16, 2024, at 7:51 AM, Christian Brauner <brauner@kernel.org> wrote:
> 
> On Wed, Oct 16, 2024 at 03:51:55PM +0200, Jan Kara wrote:
>> On Tue 15-10-24 05:52:02, Song Liu wrote:
>>>> On Oct 14, 2024, at 10:25 PM, Christoph Hellwig <hch@infradead.org> wrote:
>>>> On Tue, Oct 15, 2024 at 05:21:48AM +0000, Song Liu wrote:
>>>>>>> Extend test_progs fs_kfuncs to cover different xattr names. Specifically:
>>>>>>> xattr name "user.kfuncs", "security.bpf", and "security.bpf.xxx" can be
>>>>>>> read from BPF program with kfuncs bpf_get_[file|dentry]_xattr(); while
>>>>>>> "security.bpfxxx" and "security.selinux" cannot be read.
>>>>>> 
>>>>>> So you read code from untrusted user.* xattrs?  How can you carve out
>>>>>> that space and not known any pre-existing userspace cod uses kfuncs
>>>>>> for it's own purpose?
>>>>> 
>>>>> I don't quite follow the comment here. 
>>>>> 
>>>>> Do you mean user.* xattrs are untrusted (any user can set it), so we 
>>>>> should not allow BPF programs to read them? Or do you mean xattr 
>>>>> name "user.kfuncs" might be taken by some use space?
>>>> 
>>>> All of the above.
>>> 
>>> This is a selftest, "user.kfunc" is picked for this test. The kfuncs
>>> (bpf_get_[file|dentry]_xattr) can read any user.* xattrs. 
>>> 
>>> Reading untrusted xattrs from trust BPF LSM program can be useful. 
>>> For example, we can sign a binary with private key, and save the
>>> signature in the xattr. Then the kernel can verify the signature
>>> and the binary matches the public key. If the xattr is modified by
>>> untrusted user space, the BPF program will just deny the access.
>> 
>> So I tend to agree with Christoph that e.g. for the above LSM usecase you
>> mention, using user. xattr space is a poor design choice because you have
>> to very carefully validate any xattr contents (anybody can provide
>> malicious content) and more importantly as different similar usecases
>> proliferate the chances of name collisions and resulting funcionality
>> issues increase. It is similar as if you decided to store some information
>> in a specially named file in each directory. If you choose special enough
>> name, it will likely work but long-term someone is going to break you :)

Yes, with user.* xattr, name conflicts is always an issue. That's why we 
are adding the security.bpf prefix in this set. 

However, besides name conflicts, I don't think there are many more issues
with using user. xattrs. VFS code does not block any access to the
security.* xattrs. It is up to the LSMs to block read/write of certain
xattrs. IOW, if the LSM writer decide to use user.xxx for some use cases, 
it is up to LSM writer to protect this xattr from unauthorized access 
(via security_inode_setxattr hook). 

>> 
>> I think that getting user.* xattrs from bpf hooks can still be useful for
>> introspection and other tasks so I'm not convinced we should revert that
>> functionality but maybe it is too easy to misuse? I'm not really decided.
> 
> Reading user.* xattr is fine. If an LSM decides to built a security
> model around it then imho that's their business and since that happens
> in out-of-tree LSM programs: shrug.

Thanks,
Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Wed, Oct 16, 2024 at 04:51:37PM +0200, Christian Brauner wrote:
> > 
> > I think that getting user.* xattrs from bpf hooks can still be useful for
> > introspection and other tasks so I'm not convinced we should revert that
> > functionality but maybe it is too easy to misuse? I'm not really decided.
> 
> Reading user.* xattr is fine. If an LSM decides to built a security
> model around it then imho that's their business and since that happens
> in out-of-tree LSM programs: shrug.

By that argument user.kfuncs is even more useless as just being able
to read all xattrs should be just as fine.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christian Brauner]

On Thu, Oct 17, 2024 at 08:03:51AM -0700, Christoph Hellwig wrote:
> On Wed, Oct 16, 2024 at 04:51:37PM +0200, Christian Brauner wrote:
> > > 
> > > I think that getting user.* xattrs from bpf hooks can still be useful for
> > > introspection and other tasks so I'm not convinced we should revert that
> > > functionality but maybe it is too easy to misuse? I'm not really decided.
> > 
> > Reading user.* xattr is fine. If an LSM decides to built a security
> > model around it then imho that's their business and since that happens
> > in out-of-tree LSM programs: shrug.
> 
> By that argument user.kfuncs is even more useless as just being able
> to read all xattrs should be just as fine.

bpf shouldn't read security.* of another LSM or a host of other examples...

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Mon, Oct 21, 2024 at 03:24:30PM +0200, Christian Brauner wrote:
> On Thu, Oct 17, 2024 at 08:03:51AM -0700, Christoph Hellwig wrote:
> > On Wed, Oct 16, 2024 at 04:51:37PM +0200, Christian Brauner wrote:
> > > > 
> > > > I think that getting user.* xattrs from bpf hooks can still be useful for
> > > > introspection and other tasks so I'm not convinced we should revert that
> > > > functionality but maybe it is too easy to misuse? I'm not really decided.
> > > 
> > > Reading user.* xattr is fine. If an LSM decides to built a security
> > > model around it then imho that's their business and since that happens
> > > in out-of-tree LSM programs: shrug.
> > 
> > By that argument user.kfuncs is even more useless as just being able
> > to read all xattrs should be just as fine.
> 
> bpf shouldn't read security.* of another LSM or a host of other examples...

Sorry if I was unclear, but this was all about user.*.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

Hi Christoph, 

> On Oct 22, 2024, at 11:45 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Mon, Oct 21, 2024 at 03:24:30PM +0200, Christian Brauner wrote:
>> On Thu, Oct 17, 2024 at 08:03:51AM -0700, Christoph Hellwig wrote:
>>> On Wed, Oct 16, 2024 at 04:51:37PM +0200, Christian Brauner wrote:
>>>>> 
>>>>> I think that getting user.* xattrs from bpf hooks can still be useful for
>>>>> introspection and other tasks so I'm not convinced we should revert that
>>>>> functionality but maybe it is too easy to misuse? I'm not really decided.
>>>> 
>>>> Reading user.* xattr is fine. If an LSM decides to built a security
>>>> model around it then imho that's their business and since that happens
>>>> in out-of-tree LSM programs: shrug.
>>> 
>>> By that argument user.kfuncs is even more useless as just being able
>>> to read all xattrs should be just as fine.
>> 
>> bpf shouldn't read security.* of another LSM or a host of other examples...
> 
> Sorry if I was unclear, but this was all about user.*.

Given bpf kfuncs can read user.* xattrs for almost a year now, I think we 
cannot simply revert it. We already have some users using it. 

Instead, we can work on a plan to deprecated it. How about we add a 
WARN_ON_ONCE as part of this patchset, and then remove user.* support 
after some time?

Thanks,
Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Christoph Hellwig]

On Wed, Oct 30, 2024 at 08:44:26PM +0000, Song Liu wrote:
> Given bpf kfuncs can read user.* xattrs for almost a year now, I think we 
> cannot simply revert it. We already have some users using it. 
> 
> Instead, we can work on a plan to deprecated it. How about we add a 
> WARN_ON_ONCE as part of this patchset, and then remove user.* support 
> after some time?

As Christian mentioned having bpf access to user xattrs is probably
not a big issue.  OTOH anything that makes security decisions based
on it is probably pretty broken.  Not sure how you want to best
handle that.

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Song Liu]

> On Oct 30, 2024, at 11:56 PM, Christoph Hellwig <hch@infradead.org> wrote:
> 
> On Wed, Oct 30, 2024 at 08:44:26PM +0000, Song Liu wrote:
>> Given bpf kfuncs can read user.* xattrs for almost a year now, I think we 
>> cannot simply revert it. We already have some users using it. 
>> 
>> Instead, we can work on a plan to deprecated it. How about we add a 
>> WARN_ON_ONCE as part of this patchset, and then remove user.* support 
>> after some time?
> 
> As Christian mentioned having bpf access to user xattrs is probably
> not a big issue.  OTOH anything that makes security decisions based
> on it is probably pretty broken.  Not sure how you want to best
> handle that.

Agreed that we really need security.bpf prefix for security use cases. 
Reading user.* xattrs could be useful for some tracing use cases. We
may also introduce other prefixes for future use cases.

Thanks,
Song

Re: [PATCH bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names

[Alexei Starovoitov]

On Thu, Oct 31, 2024 at 9:02 AM Song Liu <songliubraving@meta.com> wrote:
>
> >  Not sure how you want to best handle that.
>
>  We may also introduce other prefixes for future use cases.

bpf infra makes zero effort to prevent insecure/nonsensical bpf programs.
It's futile. Humans will always find ways to shoot themselves in the foot.
Before bpf-lsm existed people were selling "security" products
where _tracing_ bpf programs monitored syscall activity with kprobes
suffering all TOCTOU issues and signaling root user same daemon
via bpf maps/ring buffers to kill "bad" processes.
Such startups still exist. There is no technical solution
to human "ingenuity".

Re: [PATCH bpf-next 0/2] security.bpf xattr name prefix

[Song Liu]

Hi Christian, Al, and Jan,

Could you please review and share your comments on this set?

Thanks,
Song

On Wed, Oct 2, 2024 at 2:47 PM Song Liu <song@kernel.org> wrote:
>
> Follow up discussion in LPC 2024 [1], that we need security.bpf xattr
> prefix. This set adds "security.bpf" xattr name prefix, and allows
> bpf kfuncs bpf_get_[file|dentry]_xattr() to read these xattrs.
>
>
> [1] https://lpc.events/event/18/contributions/1940/
>
> Song Liu (2):
>   fs/xattr: bpf: Introduce security.bpf xattr name prefix
>   selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names
>
>  fs/bpf_fs_kfuncs.c                            | 19 ++++++++-
>  include/uapi/linux/xattr.h                    |  4 ++
>  .../selftests/bpf/prog_tests/fs_kfuncs.c      | 40 ++++++++++++++-----
>  .../selftests/bpf/progs/test_get_xattr.c      | 30 ++++++++++++--
>  4 files changed, 78 insertions(+), 15 deletions(-)
>
> --
> 2.43.5

Re: [PATCH bpf-next 0/2] security.bpf xattr name prefix

[Christian Brauner]

On Fri, Oct 11, 2024 at 10:40:02AM -0700, Song Liu wrote:
> Hi Christian, Al, and Jan,
> 
> Could you please review and share your comments on this set?

Yes, as I said in-person, I think adding security.bpf is fine.