On Tue, Jun 28, 2022 at 7:33 PM Christian Brauner [off-list ref] wrote:

On Tue, Jun 28, 2022 at 04:19:48PM +0000, KP Singh wrote:

quoted

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@kernel.org>
---
 .../testing/selftests/bpf/prog_tests/xattr.c  | 54 +++++++++++++++++++

[...]

quoted

+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;
}