Re: [PATCH v5 bpf-next 5/5] bpf/selftests: Add a selftest for bpf_getxattr

[KP Singh <kpsingh@xxxxxxxxxx>]

On Tue, Jun 28, 2022 at 7:33 PM Christian Brauner <brauner@xxxxxxxxxx> wrote:
>
> On Tue, Jun 28, 2022 at 04:19:48PM +0000, KP Singh wrote:
> > A simple test that adds an xattr on a copied /bin/ls and reads it back
> > when the copied ls is executed.
> >
> > Signed-off-by: KP Singh <kpsingh@xxxxxxxxxx>
> > ---
> >  .../testing/selftests/bpf/prog_tests/xattr.c  | 54 +++++++++++++++++++

[...]

> > +SEC("lsm.s/bprm_committed_creds")
> > +void BPF_PROG(bprm_cc, struct linux_binprm *bprm)
> > +{
> > +     struct task_struct *current = bpf_get_current_task_btf();
> > +     char dir_xattr_value[64] = {0};
> > +     int xattr_sz = 0;
> > +
> > +     xattr_sz = bpf_getxattr(bprm->file->f_path.dentry,
> > +                             bprm->file->f_path.dentry->d_inode, XATTR_NAME,
> > +                             dir_xattr_value, 64);
>
> Yeah, this isn't right. You're not accounting for the caller's userns
> nor for the idmapped mount. If this is supposed to work you will need a
> variant of vfs_getxattr() that takes the mount's idmapping into account
> afaict. See what needs to happen after do_getxattr().

Thanks for taking a look.

So, If I understand correctly, we don't need xattr_permission (and
other checks in
vfs_getxattr) here as the BPF programs run as CAP_SYS_ADMIN.

but...

So, Is this bit what's missing then?

error = vfs_getxattr(mnt_userns, d, kname, ctx->kvalue, ctx->size);
if (error > 0) {
    if ((strcmp(kname, XATTR_NAME_POSIX_ACL_ACCESS) == 0) ||
(strcmp(kname, XATTR_NAME_POSIX_ACL_DEFAULT) == 0))
        posix_acl_fix_xattr_to_user(mnt_userns, d_inode(d),
            ctx->kvalue, error);
    if (ctx->size && copy_to_user(ctx->value, ctx->kvalue, error))
        error = -EFAULT;
}
else if (error == -ERANGE && ctx->size >= XATTR_SIZE_MAX) {
    /* The file system tried to returned a value bigger
than XATTR_SIZE_MAX bytes. Not possible. */
    error = -E2BIG;
}