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

[Song Liu]

Hi Christian,

Thanks again for your comments.

On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner at kernel.org> wrote:
>
[...]
> >
> > AFAICT, the XATTR_USER_PREFIX above is equivalent to the prefix
> > check in xattr_permission().
> >
> > For inode_permission(), I think it is not required because we already
> > have the "struct file" of  the target file. Did I misunderstand something
> > here?
>
> I had overlooked that you don't allow writing xattrs. But there's still
> some issues:
>
> So if you look at the system call interface:
>
> fgetxattr(fd)
> -> getxattr()
>    -> do_getxattr()
>       -> vfs_getxattr()
>          -> xattr_permission()
>          -> __vfs_getxattr()
>
> and io_uring:
>
> do_getxattr()
> -> vfs_getxattr()
>    -> xattr_permission()
>    -> __vfs_getxattr()
>
> you can see that xattr_permission() is a _read/write-time check_, not an
> open check. That's because the read/write permissions may depend on what
> xattr is read/written. Since you don't know what xattr will be
> read/written at open-time.
>
> So there needs to be a good reason for bpf_get_file_xattr() to deviate
> from the system call and io_uring interface. And I'd like to hear it,
> please. :)
>
> I think I might see the argument because you document the helper as "may
> only be called from BPF LSM function" in which case you're trying to say
> that bpf_get_file_xattr() is equivalent to a call to __vfs_getxattr()
> from an LSM to get at it's own security xattr.
>
> But if that's the case you really should have a way to verify that these
> helpers are only callable from a specific BPF context. Because you
> otherwise omit read/write-time permission checking when retrieving
> xattrs which is a potentialy security issue and may be abused by a BPF
> program to skip permission checks that are otherwise enforced.

What do you mean by "a specific BPF context"? Current implementation
makes sure the helper only works on LSM hooks with "struct file *" in the
argument list. Specifically, we can only use them from the following hooks:

    security_binder_transfer_file
    security_bprm_creds_from_file
    security_file_permission
    security_file_alloc_security
    security_file_free_security
    security_file_ioctl
    security_mmap_file
    security_file_lock
    security_file_fcntl
    security_file_set_fowner
    security_file_receive
    security_file_open
    security_file_truncate
    security_kernel_read_file
    security_kernel_post_read_file

Note that, we disallow pointer-walking with the kfunc, so the kfunc is not
allowed from hooks with indirect access to "struct file". For example, we
cannot use it with security_bprm_creds_for_exec(struct linux_binprm *bprm)
as this hook only has bprm, and calling bpf_get_file_xattr(bprm->file) is
not allowed.

> Is there a way for BPF to enforce/verify that a function is only called
> from a specific BPF program? It should be able to recognize that, no?
> And then refuse to load that BPF program if a helper is called outside
> it's intended context.

Similarly, I am not quite sure what you mean by "a specific BPF program".
My answer to this is probably the same as above.

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.

Overall, we can technically add xattr_permission() check here. But I
don't think that's the right check for the LSM use case.

Does this make sense? Did I miss or misunderstand something?

Thanks,
Song