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

[Christian Brauner <brauner@kernel.org>]

On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote:
> Hi Christian,
> 
> Thanks again for your comments.
> 
> On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@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

Ok, good!

> 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.

Great.

> 
> > 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.

Yes, this is exactly what I meant.

> 
> 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?

If the helper is only callable from an LSM context then this should be
fine.