Re: [PATCH v2 bpf-next 1/2] bpf: enforce VFS constraints for xattr related BPF kfuncs

[Matt Bobrowski <mattbobrowski@google.com>]

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.