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

[Matt Bobrowski <mattbobrowski@google.com>]

On Thu, Jan 30, 2025 at 04:20:04PM +0100, Christian Brauner wrote:
> On Thu, Jan 30, 2025 at 10:57:35AM +0000, Matt Bobrowski wrote:
> > On Wed, Jan 29, 2025 at 12:59:51PM -0800, Song Liu wrote:
> > > Introduct new xattr name prefix security.bpf., and enable reading these
> > > xattrs from bpf kfuncs bpf_get_[file|dentry]_xattr().
> > > 
> > > 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
> > > success.
> > 
> > Reviewed-by: Matt Bobrowski <mattbobrowski@google.com>
> > 
> > > Signed-off-by: Song Liu <song@kernel.org>
> > > Acked-by: Christian Brauner <brauner@kernel.org>
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/bpf_fs_kfuncs.c         | 19 ++++++++++++++-----
> > >  include/uapi/linux/xattr.h |  4 ++++
> > >  2 files changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/fs/bpf_fs_kfuncs.c b/fs/bpf_fs_kfuncs.c
> > > index 3fe9f59ef867..8a65184c8c2c 100644
> > > --- a/fs/bpf_fs_kfuncs.c
> > > +++ b/fs/bpf_fs_kfuncs.c
> > > @@ -93,6 +93,11 @@ __bpf_kfunc int bpf_path_d_path(struct path *path, char *buf, size_t buf__sz)
> > >  	return len;
> > >  }
> > >  
> > > +static bool match_security_bpf_prefix(const char *name__str)
> > > +{
> > > +	return !strncmp(name__str, XATTR_NAME_BPF_LSM, XATTR_NAME_BPF_LSM_LEN);
> > > +}
> > 
> > I think this can also just be match_xattr_prefix(const char
> > *name__str, const char *prefix, size_t len) such that we can do the
> > same checks for aribitrary xattr prefixes i.e. XATTR_USER_PREFIX,
> > XATTR_NAME_BPF_LSM.
> > 
> > >  /**
> > >   * bpf_get_dentry_xattr - get xattr of a dentry
> > >   * @dentry: dentry to get xattr from
> > > @@ -101,9 +106,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
> >       	  	   	    	 	     	  ^ prefixes
> > 						  
> > > + * "security.bpf." is allowed.
> >                       ^ are
> > 
> > Out of curiosity, what is the security reasoning here? This isn't
> > obvious to me, and I'd like to understand this better. Is it simply
> > frowned upon to read arbitrary xattr values from the context of a BPF
> > LSM program, or has it got something to do with the backing xattr
> > handler that ends up being called once we step into __vfs_getxattr()
> > and such?  Also, just so that it's clear, I don't have anything
> > against this allow listing approach either, I just genuinely don't
> > understand the security implications.
> 
> I've explained this at lenghts in multiple threads. The gist is various
> xattrs require you to have access to properties that are carried by
> objects you don't have access to (e.g., the mount) or can't guarantee
> that you're in the correct context and interpreting those xattrs without
> this information is either meaningless or actively wrong.

Oh, right, I see. Thank you Christian!