* [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
@ 2026-05-03 20:08 Matt Bobrowski
2026-05-03 20:08 ` [PATCH bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Matt Bobrowski @ 2026-05-03 20:08 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
Jiri Olsa, Alexander Viro, Christian Brauner, Jan Kara,
Kumar Kartikeya Dwivedi, Matt Bobrowski
Enforce VFS constraints and semantics regarding name and value lengths
within the xattr related BPF kfuncs. Specifically, reject names that
are empty or longer than XATTR_NAME_MAX, and values larger than
XATTR_SIZE_MAX. Also validate the supplied flags to ensure that only
XATTR_CREATE and XATTR_REPLACE can be used alongside the default flag
value 0.
Fixes: 56467292794b ("bpf: fs/xattr: Add BPF kfuncs to set and remove xattrs")
Closes: https://lore.kernel.org/bpf/20260429221005.6D1C6C19425@smtp.kernel.org/
Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
---
fs/bpf_fs_kfuncs.c | 80 +++++++++++++++++++++++++++++++++++++---------
1 file changed, 65 insertions(+), 15 deletions(-)
diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
index 9d27be058494..a48cf35dedd2 100644
--- a/fs/bpf_fs_kfuncs.c
+++ b/fs/bpf_fs_kfuncs.c
@@ -93,6 +93,21 @@ __bpf_kfunc int bpf_path_d_path(const struct path *path, char *buf, size_t buf__
return len;
}
+static int bpf_xattr_validate_name(const char *name)
+{
+ u32 name_len;
+
+ /*
+ * Impose the same restrictions on the supplied name as done so within
+ * the VFS by helpers like import_xattr_name().
+ */
+ name_len = strlen(name);
+ if (!name_len || name_len > XATTR_NAME_MAX)
+ return -ERANGE;
+
+ return 0;
+}
+
static bool match_security_bpf_prefix(const char *name__str)
{
return !strncmp(name__str, XATTR_NAME_BPF_LSM, XATTR_NAME_BPF_LSM_LEN);
@@ -117,10 +132,10 @@ static int bpf_xattr_read_permission(const char *name, struct inode *inode)
* @name__str: name of the xattr
* @value_p: output buffer of the xattr value
*
- * Get xattr *name__str* of *dentry* and store the output in *value_ptr*.
+ * Get xattr *name__str* of *dentry* and store the output in *value_p*.
*
- * For security reasons, only *name__str* with prefixes "user." or
- * "security.bpf." are allowed.
+ * For security reasons, only *name__str* values prefixed with "user." or
+ * "security.bpf." are permitted.
*
* Return: length of the xattr value on success, a negative value on error.
*/
@@ -133,6 +148,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
void *value;
int ret;
+ ret = bpf_xattr_validate_name(name__str);
+ if (ret)
+ return ret;
+
value_len = __bpf_dynptr_size(value_ptr);
value = __bpf_dynptr_data_rw(value_ptr, value_len);
if (!value)
@@ -150,10 +169,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
* @name__str: name of the xattr
* @value_p: output buffer of the xattr value
*
- * Get xattr *name__str* of *file* and store the output in *value_ptr*.
+ * Get xattr *name__str* of *file* and store the output in *value_p*.
*
- * For security reasons, only *name__str* with prefixes "user." or
- * "security.bpf." are allowed.
+ * For security reasons, only *name__str* values prefixed with "user." or
+ * "security.bpf." are permitted.
*
* Return: length of the xattr value on success, a negative value on error.
*/
@@ -187,10 +206,18 @@ static int bpf_xattr_write_permission(const char *name, struct inode *inode)
* @value_p: xattr value
* @flags: flags to pass into filesystem operations
*
- * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
+ * Set xattr *name__str* of *dentry* to the value in *value_p*.
*
- * For security reasons, only *name__str* with prefix "security.bpf."
- * is allowed.
+ * For security reasons, only *name__str* values prefixed with "security.bpf."
+ * are permitted.
+ *
+ * The length constraints imposed on both the xattr name and value abide those
+ * that are also enforced by the VFS. Additionally, the flags argument respects
+ * what's enforced by the VFS in the same way. By default, the flag value of 0
+ * is permitted and an xattr will be created if it does not exist, or the value
+ * will be replaced if the xattr already exists. More course grained control
+ * over these exact semantics is permitted by explicitly specifying either
+ * XATTR_CREATE or XATTR_REPLACE.
*
* The caller already locked dentry->d_inode.
*
@@ -206,7 +233,17 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
u32 value_len;
int ret;
+ if (flags & ~(XATTR_CREATE | XATTR_REPLACE))
+ return -EINVAL;
+
+ ret = bpf_xattr_validate_name(name__str);
+ if (ret)
+ return ret;
+
value_len = __bpf_dynptr_size(value_ptr);
+ if (value_len > XATTR_SIZE_MAX)
+ return -E2BIG;
+
value = __bpf_dynptr_data(value_ptr, value_len);
if (!value)
return -EINVAL;
@@ -237,8 +274,8 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
*
* Rmove xattr *name__str* of *dentry*.
*
- * For security reasons, only *name__str* with prefix "security.bpf."
- * is allowed.
+ * For security reasons, only *name__str* values prefixed with "security.bpf."
+ * are permitted.
*
* The caller already locked dentry->d_inode.
*
@@ -249,6 +286,10 @@ int bpf_remove_dentry_xattr_locked(struct dentry *dentry, const char *name__str)
struct inode *inode = d_inode(dentry);
int ret;
+ ret = bpf_xattr_validate_name(name__str);
+ if (ret)
+ return ret;
+
ret = bpf_xattr_write_permission(name__str, inode);
if (ret)
return ret;
@@ -274,11 +315,19 @@ __bpf_kfunc_start_defs();
* @value_p: xattr value
* @flags: flags to pass into filesystem operations
*
- * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
+ * Set xattr *name__str* of *dentry* to the value in *value_p*.
*
* For security reasons, only *name__str* with prefix "security.bpf."
* is allowed.
*
+ * The length constraints imposed on both the xattr name and value abide those
+ * that are also enforced by the VFS. Additionally, the flags argument respects
+ * what's enforced by the VFS in the same way. By default, the flag value of 0
+ * is permitted and an xattr will be created if it does not exist, or the value
+ * will be replaced if the xattr already exists. More course grained control
+ * over these exact semantics is permitted by explicitly specifying either
+ * XATTR_CREATE or XATTR_REPLACE.
+ *
* The caller has not locked dentry->d_inode.
*
* Return: 0 on success, a negative value on error.
@@ -327,14 +376,15 @@ __bpf_kfunc int bpf_remove_dentry_xattr(struct dentry *dentry, const char *name_
* @name__str: name of the xattr
* @value_p: output buffer of the xattr value
*
- * Get xattr *name__str* of *cgroup* and store the output in *value_ptr*.
+ * Get xattr *name__str* of *cgroup* and store the output in *value_p*.
*
- * For security reasons, only *name__str* with prefix "user." is allowed.
+ * For security reasons, only *name__str* values prefixed with "user." are
+ * permitted.
*
* Return: length of the xattr value on success, a negative value on error.
*/
__bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
- struct bpf_dynptr *value_p)
+ struct bpf_dynptr *value_p)
{
struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
u32 value_len;
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH bpf-next 2/2] selftests/bpf: add new negative tests for xattr related BPF kfuncs
2026-05-03 20:08 [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
@ 2026-05-03 20:08 ` Matt Bobrowski
2026-05-03 20:58 ` sashiko-bot
2026-05-03 20:43 ` [PATCH bpf-next 1/2] bpf: enforce VFS constraints " sashiko-bot
2026-05-03 20:44 ` bot+bpf-ci
2 siblings, 1 reply; 5+ messages in thread
From: Matt Bobrowski @ 2026-05-03 20:08 UTC (permalink / raw)
To: bpf
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
Jiri Olsa, Alexander Viro, Christian Brauner, Jan Kara,
Kumar Kartikeya Dwivedi, Matt Bobrowski
Add a set of negative tests to verify the newly enforced constraints
applied to xattr related BPF kfuncs.
Signed-off-by: Matt Bobrowski <mattbobrowski@google.com>
---
.../selftests/bpf/prog_tests/fs_kfuncs.c | 13 ++++++++++++
.../bpf/progs/test_set_remove_xattr.c | 21 +++++++++++++++++++
2 files changed, 34 insertions(+)
diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
index 43a26ec69a8e..168ffcdccc4a 100644
--- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
+++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
@@ -128,6 +128,11 @@ static void test_set_remove_xattr(void)
if (!ASSERT_OK_PTR(skel, "test_set_remove_xattr__open_and_load"))
return;
+ /* Prepare the long name for negative test */
+ memset(skel->bss->long_name, 'a', 256);
+ memcpy(skel->bss->long_name, "security.bpf.", 13);
+ skel->bss->long_name[256] = '\0';
+
/* Set security.bpf.foo to "hello" */
err = setxattr(testfile, skel->rodata->xattr_foo, value_foo, strlen(value_foo) + 1, 0);
if (err && errno == EOPNOTSUPP) {
@@ -188,6 +193,14 @@ static void test_set_remove_xattr(void)
ASSERT_TRUE(skel->bss->locked_remove_security_selinux_fail,
"locked_remove_security_selinux_fail");
+ ASSERT_EQ(skel->bss->ret_name_empty, -ERANGE, "ret_code_name_empty");
+ ASSERT_EQ(skel->bss->ret_name_too_long, -ERANGE,
+ "ret_code_name_too_long");
+ ASSERT_EQ(skel->bss->ret_value_too_large, -E2BIG,
+ "ret_code_value_too_large");
+ ASSERT_EQ(skel->bss->ret_invalid_flags, -EINVAL,
+ "ret_code_invalid_flags");
+
out:
close(fd);
test_set_remove_xattr__destroy(skel);
diff --git a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
index 6a612cf168d3..7c857d665eae 100644
--- a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
+++ b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
@@ -17,6 +17,14 @@ static const char xattr_selinux[] = "security.selinux";
char value_bar[] = "world";
char read_value[32];
+const char xattr_negative[] = "security.bpf.negative";
+int ret_code_name_empty;
+int ret_code_name_too_long;
+int ret_code_value_too_large;
+int ret_code_invalid_flags;
+char long_name[257];
+char large_value[65537];
+
bool set_security_bpf_bar_success;
bool remove_security_bpf_bar_success;
bool set_security_selinux_fail;
@@ -73,6 +81,19 @@ int BPF_PROG(test_inode_getxattr, struct dentry *dentry, char *name)
remove_security_selinux_fail = true;
}
+ bpf_dynptr_from_mem(read_value, sizeof(read_value), 0, &value_ptr);
+ ret_code_name_empty = bpf_get_dentry_xattr(dentry, "", &value_ptr);
+ ret_code_name_too_long =
+ bpf_get_dentry_xattr(dentry, long_name, &value_ptr);
+
+ bpf_dynptr_from_mem(large_value, sizeof(large_value), 0, &value_ptr);
+ ret_code_value_too_large =
+ bpf_set_dentry_xattr(dentry, xattr_negative, &value_ptr, 0);
+
+ bpf_dynptr_from_mem(value_bar, sizeof(value_bar), 0, &value_ptr);
+ ret_code_invalid_flags = bpf_set_dentry_xattr(dentry, xattr_negative,
+ &value_ptr, 0xFFFF);
+
return 0;
}
--
2.54.0.545.g6539524ca2-goog
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next 2/2] selftests/bpf: add new negative tests for xattr related BPF kfuncs
2026-05-03 20:08 ` [PATCH bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
@ 2026-05-03 20:58 ` sashiko-bot
0 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-05-03 20:58 UTC (permalink / raw)
To: Matt Bobrowski; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The BPF program will be unconditionally rejected by the verifier because a mutable `.bss` variable is passed to a kfunc argument requiring a constant string, causing the test to fail and leak a temporary file.
- [Low] Mismatch between BPF program variable names and the generated skeleton fields accessed in the test, which will cause a build failure.
--
commit fced0fe9408f12be9c0811a441c1071b5b9e4b39
Author: Matt Bobrowski <mattbobrowski@google.com>
selftests/bpf: add new negative tests for xattr related BPF kfuncs
Add a set of negative tests to verify the newly enforced constraints
applied to xattr related BPF kfuncs.
> diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> --- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
> @@ -128,6 +128,11 @@ static void test_set_remove_xattr(void)
> if (!ASSERT_OK_PTR(skel, "test_set_remove_xattr__open_and_load"))
> return;
>
> + /* Prepare the long name for negative test */
> + memset(skel->bss->long_name, 'a', 256);
> + memcpy(skel->bss->long_name, "security.bpf.", 13);
> + skel->bss->long_name[256] = '\0';
> +
Can this leak the temporary testfile on the filesystem? If open_and_load
fails due to verifier issues with long_name, this returns directly instead
of taking the goto out path to reach remove(testfile) called later in the
function.
[ ... ]
> @@ -188,6 +193,14 @@ static void test_set_remove_xattr(void)
> ASSERT_TRUE(skel->bss->locked_remove_security_selinux_fail,
> "locked_remove_security_selinux_fail");
>
> + ASSERT_EQ(skel->bss->ret_name_empty, -ERANGE, "ret_code_name_empty");
> + ASSERT_EQ(skel->bss->ret_name_too_long, -ERANGE,
> + "ret_code_name_too_long");
> + ASSERT_EQ(skel->bss->ret_value_too_large, -E2BIG,
> + "ret_code_value_too_large");
> + ASSERT_EQ(skel->bss->ret_invalid_flags, -EINVAL,
> + "ret_code_invalid_flags");
> +
Will this cause a build failure? The BPF program declares these variables
with a ret_code_ prefix (such as ret_code_name_empty), so the generated
skeleton structure will not contain fields like ret_name_empty.
> diff --git a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> --- a/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> +++ b/tools/testing/selftests/bpf/progs/test_set_remove_xattr.c
> @@ -17,6 +17,14 @@ static const char xattr_selinux[] = "security.selinux";
> char value_bar[] = "world";
> char read_value[32];
>
> +const char xattr_negative[] = "security.bpf.negative";
> +int ret_code_name_empty;
> +int ret_code_name_too_long;
> +int ret_code_value_too_large;
> +int ret_code_invalid_flags;
> +char long_name[257];
Will the verifier reject this program? The kfunc bpf_get_dentry_xattr
expects a constant string for its name__str argument. Because long_name
is declared without the const qualifier, it is placed in the writable
.bss map rather than the read-only .rodata map.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260503200819.1530328-1-mattbobrowski@google.com?part=2
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-03 20:08 [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
2026-05-03 20:08 ` [PATCH bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
@ 2026-05-03 20:43 ` sashiko-bot
2026-05-03 20:44 ` bot+bpf-ci
2 siblings, 0 replies; 5+ messages in thread
From: sashiko-bot @ 2026-05-03 20:43 UTC (permalink / raw)
To: Matt Bobrowski; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] bpf_set_dentry_xattr and bpf_remove_dentry_xattr will crash the kernel with a NULL pointer dereference if passed a negative dentry.
- [High] bpf_xattr_write_permission() bypasses IS_APPEND() checks, allowing BPF programs to modify xattrs on append-only files.
- [High] bpf_xattr_read_permission and bpf_xattr_write_permission use WARN_ON(!inode), allowing BPF programs to trivially spam the kernel log or trigger panics.
- [Medium] bpf_cgroup_read_xattr() misses the new VFS name length constraint check.
--
commit 6ac57f528df35ba363e277e2447059faac6477b7
Author: Matt Bobrowski <mattbobrowski@google.com>
bpf: enforce VFS constraints for xattr related BPF kfuncs
This commit enforces VFS constraints and semantics regarding name and value
lengths within the xattr related BPF kfuncs. It rejects names that are empty
or longer than XATTR_NAME_MAX, and values larger than XATTR_SIZE_MAX. It also
validates the supplied flags to ensure only XATTR_CREATE and XATTR_REPLACE can
be used alongside the default flag value 0.
Closes: https://lore.kernel.org/bpf/20260429221005.6D1C6C19425@smtp.kernel.org/
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..a48cf35dedd2 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
[ ... ]
> @@ -117,10 +132,10 @@ static int bpf_xattr_read_permission(const char *name, struct inode *inode)
This wasn't introduced by this patch, but do bpf_xattr_read_permission() and
bpf_xattr_write_permission() use WARN_ON(!inode), allowing BPF programs to
trivially spam the kernel log or trigger panics?
BPF LSM programs can attach to hooks that inherently supply negative dentries
(like vfs_mkdir), where d_inode(dentry) is legally NULL. Passing a negative
dentry would intentionally trigger the warning in these permission checks:
static int bpf_xattr_write_permission(const char *name, struct inode *inode)
{
if (WARN_ON(!inode))
return -EINVAL;
This could panic the system if panic_on_warn is configured. Should these checks
gracefully return -ENOENT or -EINVAL without firing a WARN_ON()?
> * @name__str: name of the xattr
> * @value_p: output buffer of the xattr value
> *
[ ... ]
> @@ -187,10 +206,18 @@ static int bpf_xattr_write_permission(const char *name, struct inode *inode)
This wasn't introduced by this patch, but does bpf_xattr_write_permission()
bypass IS_APPEND() checks, allowing BPF programs to modify xattrs on
append-only files?
Standard VFS xattr modifications enforce restrictions via xattr_permission(),
which explicitly checks IS_IMMUTABLE(inode) || IS_APPEND(inode) to block
metadata tampering on append-only files. However, bpf_xattr_write_permission()
bypasses xattr_permission() and calls inode_permission(..., MAY_WRITE)
directly.
Because inode_permission() checks IS_IMMUTABLE but not IS_APPEND, could BPF
programs be erroneously permitted to modify security.bpf.* xattrs on files
marked as append-only?
> * @value_p: xattr value
> * @flags: flags to pass into filesystem operations
> *
[ ... ]
> @@ -274,11 +315,19 @@ __bpf_kfunc_start_defs();
> * @value_p: xattr value
> * @flags: flags to pass into filesystem operations
> *
> - * Set xattr *name__str* of *dentry* to the value in *value_ptr*.
> + * Set xattr *name__str* of *dentry* to the value in *value_p*.
This problem wasn't introduced by this patch, but will bpf_set_dentry_xattr()
and bpf_remove_dentry_xattr() crash the kernel with a NULL pointer dereference
if passed a negative dentry?
BPF LSM programs can attach to hooks that inherently supply negative dentries
(such as inode_create or vfs_mkdir), where d_inode(dentry) is legally NULL. If
a BPF program invokes bpf_set_dentry_xattr() or bpf_remove_dentry_xattr() on
such a negative dentry, the kfunc will assign inode = NULL and subsequently
call inode_lock(inode).
Because inode_lock() unconditionally dereferences the inode pointer, would this
immediately trigger a NULL pointer dereference and oops the kernel?
> *
> * For security reasons, only *name__str* with prefix "security.bpf."
> * is allowed.
[ ... ]
> @@ -327,14 +376,15 @@ __bpf_kfunc int bpf_remove_dentry_xattr(struct dentry *dentry, const char *name_
> * @name__str: name of the xattr
> * @value_p: output buffer of the xattr value
> *
> - * Get xattr *name__str* of *cgroup* and store the output in *value_ptr*.
> + * Get xattr *name__str* of *cgroup* and store the output in *value_p*.
> *
> - * For security reasons, only *name__str* with prefix "user." is allowed.
> + * For security reasons, only *name__str* values prefixed with "user." are
> + * permitted.
> *
> * Return: length of the xattr value on success, a negative value on error.
> */
> __bpf_kfunc int bpf_cgroup_read_xattr(struct cgroup *cgroup, const char *name__str,
> - struct bpf_dynptr *value_p)
> + struct bpf_dynptr *value_p)
> {
> struct bpf_dynptr_kern *value_ptr = (struct bpf_dynptr_kern *)value_p;
> u32 value_len;
Does bpf_cgroup_read_xattr() miss the new VFS name length constraint check?
The commit message explicitly states the goal is to "Enforce VFS constraints
and semantics regarding name and value lengths within the xattr related BPF
kfuncs", specifically rejecting names that are empty or longer than
XATTR_NAME_MAX.
While bpf_xattr_validate_name() was correctly added to the dentry-related
kfuncs like bpf_get_dentry_xattr() and bpf_set_dentry_xattr(), it appears to
be completely omitted in bpf_cgroup_read_xattr(). Could this allow BPF
programs to bypass the intended length constraints when reading cgroup xattrs?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260503200819.1530328-1-mattbobrowski@google.com?part=1
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-03 20:08 [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
2026-05-03 20:08 ` [PATCH bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
2026-05-03 20:43 ` [PATCH bpf-next 1/2] bpf: enforce VFS constraints " sashiko-bot
@ 2026-05-03 20:44 ` bot+bpf-ci
2 siblings, 0 replies; 5+ messages in thread
From: bot+bpf-ci @ 2026-05-03 20:44 UTC (permalink / raw)
To: mattbobrowski, bpf
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
jolsa, viro, brauner, jack, memxor, mattbobrowski, ast, andrii,
daniel, martin.lau, eddyz87, yonghong.song, clm, ihor.solodrai
[-- Attachment #1: Type: text/plain, Size: 3039 bytes --]
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..a48cf35dedd2 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
[ ... ]
> @@ -117,10 +132,10 @@ static int bpf_xattr_read_permission(const char *name, struct inode *inode)
> * @name__str: name of the xattr
> * @value_p: output buffer of the xattr value
> *
> - * Get xattr *name__str* of *dentry* and store the output in *value_ptr*.
> + * Get xattr *name__str* of *dentry* and store the output in *value_p*.
> *
> - * For security reasons, only *name__str* with prefixes "user." or
> - * "security.bpf." are allowed.
> + * For security reasons, only *name__str* values prefixed with "user." or
> + * "security.bpf." are permitted.
> *
> * Return: length of the xattr value on success, a negative value on error.
> */
> @@ -133,6 +148,10 @@ __bpf_kfunc int bpf_get_dentry_xattr(struct dentry *dentry, const char *name__st
> void *value;
> int ret;
>
> + ret = bpf_xattr_validate_name(name__str);
> + if (ret)
> + return ret;
> +
In the previous patch discussion, Sashiko AI raised a concern about
WARN_ON(!inode) in bpf_xattr_read_permission() potentially causing a DoS
on panic_on_warn systems. You acknowledged that dropping the WARN_ON
makes sense since attempting to set xattr on a negative dentry is
invalid and should be handled gracefully. Can this still happen if a
BPF program passes a negative dentry to bpf_get_dentry_xattr() or the
_locked variants? If d_inode(dentry) returns NULL and is passed to
bpf_xattr_read_permission() or bpf_xattr_write_permission(), wouldn't
the WARN_ON(!inode) trigger?
[ ... ]
> @@ -206,7 +233,17 @@ int bpf_set_dentry_xattr_locked(struct dentry *dentry, const char *name__str,
> u32 value_len;
> int ret;
>
> + if (flags & ~(XATTR_CREATE | XATTR_REPLACE))
> + return -EINVAL;
> +
> + ret = bpf_xattr_validate_name(name__str);
> + if (ret)
> + return ret;
> +
> value_len = __bpf_dynptr_size(value_ptr);
> + if (value_len > XATTR_SIZE_MAX)
> + return -E2BIG;
> +
Sashiko AI raised a concern about a potential VFS lock bypass through
the BPF verifier's kfunc substitution mechanism. The verifier
unconditionally rewrites calls to bpf_set_dentry_xattr() to
bpf_set_dentry_xattr_locked() for hooks that hold inode locks, but
doesn't verify that the dentry argument is the specific context
argument that was locked.
Can a BPF program obtain a trusted pointer to an unrelated, unlocked
dentry and pass it to the kfunc, causing the substituted _locked
variant to execute __vfs_setxattr() without acquiring the proper VFS
locks? You mentioned this is "probably a valid concern" that "should be
reviewed in further depth separately," but should this be addressed
before the patch is merged to avoid introducing a security regression?
[ ... ]
---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/25289845959
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2026-05-03 20:58 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-03 20:08 [PATCH bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
2026-05-03 20:08 ` [PATCH bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
2026-05-03 20:58 ` sashiko-bot
2026-05-03 20:43 ` [PATCH bpf-next 1/2] bpf: enforce VFS constraints " sashiko-bot
2026-05-03 20:44 ` bot+bpf-ci
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox