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@xxxxxxxxxx>
>
> > Signed-off-by: Song Liu <song@xxxxxxxxxx>
> > Acked-by: Christian Brauner <brauner@xxxxxxxxxx>
> > Reviewed-by: Jan Kara <jack@xxxxxxx>
> > ---
> > 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.
