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