* [PATCH v2 bpf-next 2/2] selftests/bpf: add new negative tests for xattr related BPF kfuncs
2026-05-04 8:54 [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
@ 2026-05-04 8:54 ` Matt Bobrowski
2026-05-04 9:31 ` [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints " bot+bpf-ci
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Matt Bobrowski @ 2026-05-04 8:54 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>
---
Changes in v2:
- Fixed leaking of test file test_progs_fs_kfuncs when the
__open_and_load() helper fails within test_set_remove_xattr()
(Sashiko AI).
- Fixed invalid global variable name references within fs_kfuncs (Sashiko
AI).
- Modified global variable long_name such that it is initialized with
a long hardcoded string to satisfy the verifier (Sashiko AI).
.../selftests/bpf/prog_tests/fs_kfuncs.c | 16 ++++++++++----
.../bpf/progs/test_set_remove_xattr.c | 21 +++++++++++++++++++
2 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
index 43a26ec69a8e..37544c6fa9a6 100644
--- a/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
+++ b/tools/testing/selftests/bpf/prog_tests/fs_kfuncs.c
@@ -115,18 +115,18 @@ static void validate_bar_removed(struct test_set_remove_xattr *skel)
static void test_set_remove_xattr(void)
{
struct test_set_remove_xattr *skel = NULL;
- int fd = -1, err;
+ int fd, err;
fd = open(testfile, O_CREAT | O_RDONLY, 0644);
if (!ASSERT_GE(fd, 0, "create_file"))
return;
close(fd);
- fd = -1;
skel = test_set_remove_xattr__open_and_load();
if (!ASSERT_OK_PTR(skel, "test_set_remove_xattr__open_and_load"))
- return;
+ goto out;
+
/* Set security.bpf.foo to "hello" */
err = setxattr(testfile, skel->rodata->xattr_foo, value_foo, strlen(value_foo) + 1, 0);
@@ -188,8 +188,16 @@ 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_code_name_empty, -ERANGE,
+ "ret_code_name_empty");
+ ASSERT_EQ(skel->bss->ret_code_name_too_long, -ERANGE,
+ "ret_code_name_too_long");
+ ASSERT_EQ(skel->bss->ret_code_value_too_large, -E2BIG,
+ "ret_code_value_too_large");
+ ASSERT_EQ(skel->bss->ret_code_invalid_flags, -EINVAL,
+ "ret_code_invalid_flags");
+
out:
- close(fd);
test_set_remove_xattr__destroy(skel);
remove(testfile);
}
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..e69a5c51c60a 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;
+const char long_name[] = "security.bpf.aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa";
+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] 6+ messages in thread* Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-04 8:54 [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
2026-05-04 8:54 ` [PATCH v2 bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
@ 2026-05-04 9:31 ` bot+bpf-ci
2026-05-04 9:37 ` sashiko-bot
2026-05-04 9:42 ` Christian Brauner
3 siblings, 0 replies; 6+ messages in thread
From: bot+bpf-ci @ 2026-05-04 9:31 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: 6719 bytes --]
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..eb9aef1e135e 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;
> +
> value_len = __bpf_dynptr_size(value_ptr);
> value = __bpf_dynptr_data_rw(value_ptr, value_len);
> if (!value)
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. Looking at bpf_xattr_read_permission():
static int bpf_xattr_read_permission(const char *name, struct inode *inode)
{
if (WARN_ON(!inode))
return -ENOENT;
...
If a BPF program passes a negative dentry, this would intentionally trigger
the warning.
Could this panic the system if panic_on_warn is configured? Should this
gracefully return -ENOENT or -EINVAL without firing WARN_ON()?
Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/
[ ... ]
> @@ -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.
> *
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:
static int bpf_xattr_write_permission(const char *name, struct inode *inode)
{
...
return inode_permission(&nop_mnt_idmap, inode, MAY_WRITE);
}
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?
Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/
> @@ -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;
[ ... ]
> @@ -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.
BPF LSM programs can attach to hooks that supply negative dentries (e.g.,
inode_create, vfs_mkdir) where d_inode(dentry) is legally NULL.
Looking at bpf_set_dentry_xattr():
__bpf_kfunc int bpf_set_dentry_xattr(struct dentry *dentry, const char *name__str,
struct bpf_dynptr *value_p, int flags)
{
struct inode *inode = d_inode(dentry);
int ret;
inode_lock(inode);
...
If a BPF program invokes bpf_set_dentry_xattr() on a negative dentry,
inode will be NULL and inode_lock(inode) will unconditionally dereference
the NULL pointer.
Will this trigger a NULL pointer dereference and oops the kernel?
The same issue affects bpf_remove_dentry_xattr().
Reference: https://lore.kernel.org/bpf/20260503204353.2F800C2BCB4@smtp.kernel.org/
[ ... ]
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?
Reference: https://lore.kernel.org/bpf/5c447a881915a06db87c17ef1384f42674b0c171858c82e33666cb667101b338@mail.kernel.org/
---
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/25310655349
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-04 8:54 [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
2026-05-04 8:54 ` [PATCH v2 bpf-next 2/2] selftests/bpf: add new negative tests " Matt Bobrowski
2026-05-04 9:31 ` [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints " bot+bpf-ci
@ 2026-05-04 9:37 ` sashiko-bot
2026-05-04 9:42 ` Christian Brauner
3 siblings, 0 replies; 6+ messages in thread
From: sashiko-bot @ 2026-05-04 9:37 UTC (permalink / raw)
To: Matt Bobrowski; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The unlocked BPF kfuncs `bpf_set_dentry_xattr` and `bpf_remove_dentry_xattr` unconditionally call `inode_lock(inode)` without checking if the inode is NULL, leading to a kernel panic.
--
commit 1eddd22ded9c4ecb43cf15dc45c01aaba4a3f609
Author: Matt Bobrowski <mattbobrowski@google.com>
bpf: enforce VFS constraints for xattr related BPF kfuncs
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/
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..eb9aef1e135e 100644
> --- a/fs/bpf_fs_kfuncs.c
> +++ b/fs/bpf_fs_kfuncs.c
[ ... ]
> @@ -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.
This isn't a regression introduced by this patch, but there is a pre-existing
flaw when bpf_set_dentry_xattr and bpf_remove_dentry_xattr call inode_lock
unconditionally.
Can sleepable BPF LSM hooks such as bpf_lsm_inode_create, bpf_lsm_inode_mkdir,
and bpf_lsm_inode_mknod receive negative dentries where d_inode is NULL?
If so, since these hooks are not in the d_inode_locked_hooks list, the BPF
verifier would allow programs attached to them to call the unlocked kfuncs
bpf_set_dentry_xattr and bpf_remove_dentry_xattr. When called with a negative
dentry, the kfuncs extract a NULL inode via d_inode(dentry) and immediately
invoke inode_lock(inode).
This results in down_write(&NULL->i_rwsem), triggering a fatal NULL pointer
dereference. The WARN_ON(!inode) safety check located later in
bpf_xattr_write_permission is unreachable because the system crashes earlier
during lock acquisition.
Could we add a NULL check for the inode before attempting to lock it?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260504085428.2865671-1-mattbobrowski@google.com?part=1
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-04 8:54 [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs Matt Bobrowski
` (2 preceding siblings ...)
2026-05-04 9:37 ` sashiko-bot
@ 2026-05-04 9:42 ` Christian Brauner
2026-05-04 10:39 ` Matt Bobrowski
3 siblings, 1 reply; 6+ messages in thread
From: Christian Brauner @ 2026-05-04 9:42 UTC (permalink / raw)
To: Matt Bobrowski
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
Jiri Olsa, Alexander Viro, Jan Kara, Kumar Kartikeya Dwivedi
On Mon, May 04, 2026 at 08:54:27AM +0000, Matt Bobrowski wrote:
> 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>
> ---
> Changes in v2:
> - New helper bpf_xattr_validate_name() incorporated into pre-existing
> BPF kfunc bpf_cgroup_read_xattr() (Sashiko AI).
>
> fs/bpf_fs_kfuncs.c | 85 ++++++++++++++++++++++++++++++++++++++--------
> 1 file changed, 70 insertions(+), 15 deletions(-)
>
> diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> index 9d27be058494..eb9aef1e135e 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;
> +}
Can you please unify the check between import_xattr_name() and bpf,
please. This just asks for someone starting to return different errnos
from the two locations if we need any additional checks at some point...
> +
> 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;
> +
Hm, this is also open-coded in fs/xattr.c and here now. I wonder whether
we can easily build this around the same infra as fs/xattr.c is. So use
struct kernel_xattr_ctx and struct kname because then all codepaths:
syscall/internal, io_uring and the bpf interface are the same thing with
shared helpers and behavior.
> 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,18 +376,24 @@ __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;
> void *value;
> + int ret;
> +
> + ret = bpf_xattr_validate_name(name__str);
> + if (ret)
> + return ret;
>
> /* Only allow reading "user.*" xattrs */
> if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
> --
> 2.54.0.545.g6539524ca2-goog
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs
2026-05-04 9:42 ` Christian Brauner
@ 2026-05-04 10:39 ` Matt Bobrowski
0 siblings, 0 replies; 6+ messages in thread
From: Matt Bobrowski @ 2026-05-04 10:39 UTC (permalink / raw)
To: Christian Brauner
Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
Jiri Olsa, Alexander Viro, Jan Kara, Kumar Kartikeya Dwivedi
On Mon, May 04, 2026 at 11:42:56AM +0200, Christian Brauner wrote:
> On Mon, May 04, 2026 at 08:54:27AM +0000, Matt Bobrowski wrote:
> > 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>
> > ---
> > Changes in v2:
> > - New helper bpf_xattr_validate_name() incorporated into pre-existing
> > BPF kfunc bpf_cgroup_read_xattr() (Sashiko AI).
> >
> > fs/bpf_fs_kfuncs.c | 85 ++++++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 70 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> > index 9d27be058494..eb9aef1e135e 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;
> > +}
>
> Can you please unify the check between import_xattr_name() and bpf,
> please. This just asks for someone starting to return different errnos
> from the two locations if we need any additional checks at some point...
I'm glad you suggested this because I was thinking the exact same
thing, although wasn't sure whether you'd be open to it.
> > +
> > 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;
> > +
>
> Hm, this is also open-coded in fs/xattr.c and here now. I wonder whether
> we can easily build this around the same infra as fs/xattr.c is. So use
> struct kernel_xattr_ctx and struct kname because then all codepaths:
>
> syscall/internal, io_uring and the bpf interface are the same thing with
> shared helpers and behavior.
At this point, I can't really see why this couldn't be done. I'll try
and introduce a set of shareable helpers which are tasked with
enforcing these constraints uniformly across all of these various
subsystems.
^ permalink raw reply [flat|nested] 6+ messages in thread