* [PATCH v2 bpf-next 1/2] fs/xattr: bpf: Introduce security.bpf xattr name prefix
2024-10-16 7:09 [PATCH v2 bpf-next 0/2] security.bpf xattr name prefix Song Liu
@ 2024-10-16 7:09 ` Song Liu
2024-10-16 7:09 ` [PATCH v2 bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names Song Liu
1 sibling, 0 replies; 3+ messages in thread
From: Song Liu @ 2024-10-16 7:09 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-kernel
Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
brauner, jack, kpsingh, mattbobrowski, 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.
As we are on it, correct the comments for return value of
bpf_get_[file|dentry]_xattr(), i.e. return length the xattr value on
successl.
Signed-off-by: Song Liu <song@kernel.org>
Acked-by: Christian Brauner <brauner@kernel.org>
---
fs/bpf_fs_kfuncs.c | 29 ++++++++++++++++++++++++-----
include/uapi/linux/xattr.h | 4 ++++
2 files changed, 28 insertions(+), 5 deletions(-)
diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 3fe9f59ef867..be78a13f1d82 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
@@ -101,9 +118,10 @@ __bpf_kfunc int bpf_path_d_path(struct path *path, char *buf, size_t buf__sz)
*
* Get xattr *name__str* of *dentry* and store the output in *value_ptr*.
*
- * For security reasons, only *name__str* with prefix "user." is allowed.
+ * For security reasons, only *name__str* with prefix "user." or
+ * "security.bpf" is allowed.
*
- * Return: 0 on success, a negative value on error.
+ * Return: length of the xattr value on success, a negative value on error.
*/
__bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__str,
struct bpf_dynptr *value_p)
@@ -117,7 +135,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);
@@ -139,9 +157,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
*
* Get xattr *name__str* of *file* and store the output in *value_ptr*.
*
- * For security reasons, only *name__str* with prefix "user." is allowed.
+ * For security reasons, only *name__str* with prefix "user." or
+ * "security.bpf" is allowed.
*
- * Return: 0 on success, a negative value on error.
+ * Return: length of the xattr value on success, a negative value on error.
*/
__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str,
struct bpf_dynptr *value_p)
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* [PATCH v2 bpf-next 2/2] selftests/bpf: Extend test fs_kfuncs to cover security.bpf xattr names
2024-10-16 7:09 [PATCH v2 bpf-next 0/2] security.bpf xattr name prefix Song Liu
2024-10-16 7:09 ` [PATCH v2 bpf-next 1/2] fs/xattr: bpf: Introduce " Song Liu
@ 2024-10-16 7:09 ` Song Liu
1 sibling, 0 replies; 3+ messages in thread
From: Song Liu @ 2024-10-16 7:09 UTC (permalink / raw)
To: bpf, linux-fsdevel, linux-kernel
Cc: kernel-team, andrii, eddyz87, ast, daniel, martin.lau, viro,
brauner, jack, kpsingh, mattbobrowski, 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
^ permalink raw reply related [flat|nested] 3+ messages in thread