Re: [PATCH v13 bpf-next 1/6] bpf: Add kfunc bpf_get_file_xattr

[Song Liu <song@xxxxxxxxxx>]

On Mon, Nov 27, 2023 at 10:05 AM Song Liu <song@xxxxxxxxxx> wrote:
>
> Hi Christian,
>
> Thanks again for your comments.
>
> On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@xxxxxxxxxx> wrote:
> >
[...]
> Going back to xattr_permission itself. AFAICT, it does 3 checks:
>
> 1. MAY_WRITE check;
> 2. prefix check;
> 3. inode_permission().
>
> We don't need MAY_WRITE check as bpf_get_file_xattr is read only.
> We have the prefix check embedded in bpf_get_file_xattr():
>
>        if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
>                return -EPERM;
>
> inode_permission() is a little trickier here, which checks against idmap.
> However, I don't think the check makes sense in the context of LSM.
> In this case, we have two processes: one security daemon, which
> owns the BPF LSM program, and a process being monitored.
> idmap here, from file_mnt_idmap(file), is the idmap from the being
> monitored process. However, whether the BPF LSM program have the
> permission to read the xattr should be determined by the security
> daemon.

Maybe we should check against nop_mnt_idmap? Would something like
the following make more sense?

Thanks,
Song

diff --git i/kernel/trace/bpf_trace.c w/kernel/trace/bpf_trace.c
index 0019e990408e..62fc51bc57af 100644
--- i/kernel/trace/bpf_trace.c
+++ w/kernel/trace/bpf_trace.c
@@ -1453,6 +1453,7 @@ __bpf_kfunc int bpf_get_file_xattr(struct file
*file, const char *name__str,
        struct dentry *dentry;
        u32 value_len;
        void *value;
+       int ret;

        if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN))
                return -EPERM;
@@ -1463,6 +1464,9 @@ __bpf_kfunc int bpf_get_file_xattr(struct file
*file, const char *name__str,
                return -EINVAL;

        dentry = file_dentry(file);
+       ret = inode_permission(&nop_mnt_idmap, dentry->d_inode, MAY_READ);
+       if (ret)
+               return ret;
        return __vfs_getxattr(dentry, dentry->d_inode, name__str,
value, value_len);
 }