| Message ID | 20231123233936.3079687-2-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: File verification with LSM and fsverity | expand |
On Thu, Nov 23, 2023 at 3:40 PM Song Liu <song@kernel.org> wrote: > > It is common practice for security solutions to store tags/labels in > xattrs. To implement similar functionalities in BPF LSM, add new kfunc > bpf_get_file_xattr(). > > The first use case of bpf_get_file_xattr() is to implement file > verifications with asymmetric keys. Specificially, security applications > could use fsverity for file hashes and use xattr to store file signatures. > (kfunc for fsverity hash will be added in a separate commit.) > > Currently, only xattrs with "user." prefix can be read with kfunc > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix > for bpf_get_file_xattr(). > > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks. > > Signed-off-by: Song Liu <song@kernel.org> > --- > kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index f0b8b7c29126..55758a6fbe90 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -24,6 +24,7 @@ > #include <linux/key.h> > #include <linux/verification.h> > #include <linux/namei.h> > +#include <linux/fileattr.h> > > #include <net/bpf_sk_storage.h> > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void) > late_initcall(bpf_key_sig_kfuncs_init); > #endif /* CONFIG_KEYS */ > > +/* filesystem kfuncs */ > +__bpf_kfunc_start_defs(); > + > +/** > + * bpf_get_file_xattr - get xattr of a file > + * @file: file to get xattr from > + * @name__str: name of the xattr > + * @value_ptr: output buffer of the xattr value > + * > + * Get xattr *name__str* of *file* and store the output in *value_ptr*. > + * > + * For security reasons, only *name__str* with prefix "user." is allowed. > + * > + * Return: 0 on success, a negative value on error. > + */ > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str, > + struct bpf_dynptr_kern *value_ptr) > +{ > + struct dentry *dentry; > + u32 value_len; > + void *value; > + > + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) > + return -EPERM; > + > + value_len = __bpf_dynptr_size(value_ptr); > + value = __bpf_dynptr_data_rw(value_ptr, value_len); > + if (!value) > + return -EINVAL; > + > + dentry = file_dentry(file); > + return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len); > +} > + > +__bpf_kfunc_end_defs(); > + > +BTF_SET8_START(fs_kfunc_set_ids) > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS) > +BTF_SET8_END(fs_kfunc_set_ids) > + > +static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id) > +{ > + if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id)) > + return 0; > + > + /* Only allow to attach from LSM hooks, to avoid recursion */ > + return prog->type != BPF_PROG_TYPE_LSM ? -EACCES : 0; > +} > + > +const struct btf_kfunc_id_set bpf_fs_kfunc_set = { Missed static here. If there will be v14, I will fix it here. Thanks, Song > + .owner = THIS_MODULE, > + .set = &fs_kfunc_set_ids, > + .filter = bpf_get_file_xattr_filter, > +}; > + > +static int __init bpf_fs_kfuncs_init(void) > +{ > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); > +} > + > +late_initcall(bpf_fs_kfuncs_init); > + > static const struct bpf_func_proto * > bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > { > -- > 2.34.1 >
On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu wrote: > It is common practice for security solutions to store tags/labels in > xattrs. To implement similar functionalities in BPF LSM, add new kfunc > bpf_get_file_xattr(). > > The first use case of bpf_get_file_xattr() is to implement file > verifications with asymmetric keys. Specificially, security applications > could use fsverity for file hashes and use xattr to store file signatures. > (kfunc for fsverity hash will be added in a separate commit.) > > Currently, only xattrs with "user." prefix can be read with kfunc > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix > for bpf_get_file_xattr(). > > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks. > > Signed-off-by: Song Liu <song@kernel.org> > --- Looks ok to me. But see below for a question. If you ever allow the retrieval of additional extended attributes through bfs_get_file_xattr() or other bpf interfaces we would like to be Cced, please. The xattr stuff is (/me looks for suitable words)... Over the last months we've moved POSIX_ACL retrieval out of these low-level functions. They now have a dedicated api. The same is going to happen for fscaps as well. But even with these out of the way we would want the bpf helpers to always maintain an allowlist of retrievable attributes. > kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > index f0b8b7c29126..55758a6fbe90 100644 > --- a/kernel/trace/bpf_trace.c > +++ b/kernel/trace/bpf_trace.c > @@ -24,6 +24,7 @@ > #include <linux/key.h> > #include <linux/verification.h> > #include <linux/namei.h> > +#include <linux/fileattr.h> > > #include <net/bpf_sk_storage.h> > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void) > late_initcall(bpf_key_sig_kfuncs_init); > #endif /* CONFIG_KEYS */ > > +/* filesystem kfuncs */ > +__bpf_kfunc_start_defs(); > + > +/** > + * bpf_get_file_xattr - get xattr of a file > + * @file: file to get xattr from > + * @name__str: name of the xattr > + * @value_ptr: output buffer of the xattr value > + * > + * Get xattr *name__str* of *file* and store the output in *value_ptr*. > + * > + * For security reasons, only *name__str* with prefix "user." is allowed. > + * > + * Return: 0 on success, a negative value on error. > + */ > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str, > + struct bpf_dynptr_kern *value_ptr) > +{ > + struct dentry *dentry; > + u32 value_len; > + void *value; > + > + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) > + return -EPERM; > + > + value_len = __bpf_dynptr_size(value_ptr); > + value = __bpf_dynptr_data_rw(value_ptr, value_len); > + if (!value) > + return -EINVAL; > + > + dentry = file_dentry(file); > + return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len); By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at least inode_permission() from xattr_permission(). I'm probably just missing or forgot the context. But why is that ok? > +} > + > +__bpf_kfunc_end_defs(); > + > +BTF_SET8_START(fs_kfunc_set_ids) > +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS) > +BTF_SET8_END(fs_kfunc_set_ids) > + > +static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id) > +{ > + if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id)) > + return 0; > + > + /* Only allow to attach from LSM hooks, to avoid recursion */ > + return prog->type != BPF_PROG_TYPE_LSM ? -EACCES : 0; > +} > + > +const struct btf_kfunc_id_set bpf_fs_kfunc_set = { > + .owner = THIS_MODULE, > + .set = &fs_kfunc_set_ids, > + .filter = bpf_get_file_xattr_filter, > +}; > + > +static int __init bpf_fs_kfuncs_init(void) > +{ > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); > +} > + > +late_initcall(bpf_fs_kfuncs_init); > + > static const struct bpf_func_proto * > bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) > { > -- > 2.34.1 >
On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@kernel.org> wrote: > > On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu wrote: > > It is common practice for security solutions to store tags/labels in > > xattrs. To implement similar functionalities in BPF LSM, add new kfunc > > bpf_get_file_xattr(). > > > > The first use case of bpf_get_file_xattr() is to implement file > > verifications with asymmetric keys. Specificially, security applications > > could use fsverity for file hashes and use xattr to store file signatures. > > (kfunc for fsverity hash will be added in a separate commit.) > > > > Currently, only xattrs with "user." prefix can be read with kfunc > > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix > > for bpf_get_file_xattr(). > > > > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks. > > > > Signed-off-by: Song Liu <song@kernel.org> > > --- > > Looks ok to me. But see below for a question. > > If you ever allow the retrieval of additional extended attributes > through bfs_get_file_xattr() or other bpf interfaces we would like to be > Cced, please. The xattr stuff is (/me looks for suitable words)... > > Over the last months we've moved POSIX_ACL retrieval out of these > low-level functions. They now have a dedicated api. The same is going to > happen for fscaps as well. > > But even with these out of the way we would want the bpf helpers to > always maintain an allowlist of retrievable attributes. Agreed. We will be very specific which attributes are available to bpf helpers/kfuncs. > > > kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 63 insertions(+) > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > index f0b8b7c29126..55758a6fbe90 100644 > > --- a/kernel/trace/bpf_trace.c > > +++ b/kernel/trace/bpf_trace.c > > @@ -24,6 +24,7 @@ > > #include <linux/key.h> > > #include <linux/verification.h> > > #include <linux/namei.h> > > +#include <linux/fileattr.h> > > > > #include <net/bpf_sk_storage.h> > > > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void) > > late_initcall(bpf_key_sig_kfuncs_init); > > #endif /* CONFIG_KEYS */ > > > > +/* filesystem kfuncs */ > > +__bpf_kfunc_start_defs(); > > + > > +/** > > + * bpf_get_file_xattr - get xattr of a file > > + * @file: file to get xattr from > > + * @name__str: name of the xattr > > + * @value_ptr: output buffer of the xattr value > > + * > > + * Get xattr *name__str* of *file* and store the output in *value_ptr*. > > + * > > + * For security reasons, only *name__str* with prefix "user." is allowed. > > + * > > + * Return: 0 on success, a negative value on error. > > + */ > > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str, > > + struct bpf_dynptr_kern *value_ptr) > > +{ > > + struct dentry *dentry; > > + u32 value_len; > > + void *value; > > + > > + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) > > + return -EPERM; > > + > > + value_len = __bpf_dynptr_size(value_ptr); > > + value = __bpf_dynptr_data_rw(value_ptr, value_len); > > + if (!value) > > + return -EINVAL; > > + > > + dentry = file_dentry(file); > > + return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len); > > By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at > least inode_permission() from xattr_permission(). I'm probably just > missing or forgot the context. But why is that ok? 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? Thanks, Song > > +} > > + [...]
On Fri, Nov 24, 2023 at 09:07:33AM -0800, Song Liu wrote: > On Fri, Nov 24, 2023 at 12:44 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Thu, Nov 23, 2023 at 03:39:31PM -0800, Song Liu wrote: > > > It is common practice for security solutions to store tags/labels in > > > xattrs. To implement similar functionalities in BPF LSM, add new kfunc > > > bpf_get_file_xattr(). > > > > > > The first use case of bpf_get_file_xattr() is to implement file > > > verifications with asymmetric keys. Specificially, security applications > > > could use fsverity for file hashes and use xattr to store file signatures. > > > (kfunc for fsverity hash will be added in a separate commit.) > > > > > > Currently, only xattrs with "user." prefix can be read with kfunc > > > bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix > > > for bpf_get_file_xattr(). > > > > > > To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks. > > > > > > Signed-off-by: Song Liu <song@kernel.org> > > > --- > > > > Looks ok to me. But see below for a question. > > > > If you ever allow the retrieval of additional extended attributes > > through bfs_get_file_xattr() or other bpf interfaces we would like to be > > Cced, please. The xattr stuff is (/me looks for suitable words)... > > > > Over the last months we've moved POSIX_ACL retrieval out of these > > low-level functions. They now have a dedicated api. The same is going to > > happen for fscaps as well. > > > > But even with these out of the way we would want the bpf helpers to > > always maintain an allowlist of retrievable attributes. > > Agreed. We will be very specific which attributes are available to bpf > helpers/kfuncs. > > > > > > kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 63 insertions(+) > > > > > > diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c > > > index f0b8b7c29126..55758a6fbe90 100644 > > > --- a/kernel/trace/bpf_trace.c > > > +++ b/kernel/trace/bpf_trace.c > > > @@ -24,6 +24,7 @@ > > > #include <linux/key.h> > > > #include <linux/verification.h> > > > #include <linux/namei.h> > > > +#include <linux/fileattr.h> > > > > > > #include <net/bpf_sk_storage.h> > > > > > > @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void) > > > late_initcall(bpf_key_sig_kfuncs_init); > > > #endif /* CONFIG_KEYS */ > > > > > > +/* filesystem kfuncs */ > > > +__bpf_kfunc_start_defs(); > > > + > > > +/** > > > + * bpf_get_file_xattr - get xattr of a file > > > + * @file: file to get xattr from > > > + * @name__str: name of the xattr > > > + * @value_ptr: output buffer of the xattr value > > > + * > > > + * Get xattr *name__str* of *file* and store the output in *value_ptr*. > > > + * > > > + * For security reasons, only *name__str* with prefix "user." is allowed. > > > + * > > > + * Return: 0 on success, a negative value on error. > > > + */ > > > +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str, > > > + struct bpf_dynptr_kern *value_ptr) > > > +{ > > > + struct dentry *dentry; > > > + u32 value_len; > > > + void *value; > > > + > > > + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) > > > + return -EPERM; > > > + > > > + value_len = __bpf_dynptr_size(value_ptr); > > > + value = __bpf_dynptr_data_rw(value_ptr, value_len); > > > + if (!value) > > > + return -EINVAL; > > > + > > > + dentry = file_dentry(file); > > > + return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len); > > > > By calling __vfs_getxattr() from bpf_get_file_xattr() you're skipping at > > least inode_permission() from xattr_permission(). I'm probably just > > missing or forgot the context. But why is that ok? > > 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. 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.
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 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
On Mon, Nov 27, 2023 at 10:05 AM Song Liu <song@kernel.org> wrote: > > Hi Christian, > > Thanks again for your comments. > > On Mon, Nov 27, 2023 at 2:50 AM Christian Brauner <brauner@kernel.org> 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); }
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.
Hi Christian, On Tue, Nov 28, 2023 at 1:13 AM Christian Brauner <brauner@kernel.org> wrote: > > On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote: [...] > > > > 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. If everything looks good, would you please give an official Acked-by or Reviewed-by? Thanks, Song
On Tue, Nov 28, 2023 at 06:19:35AM -0800, Song Liu wrote: > Hi Christian, > > On Tue, Nov 28, 2023 at 1:13 AM Christian Brauner <brauner@kernel.org> wrote: > > > > On Mon, Nov 27, 2023 at 10:05:23AM -0800, Song Liu wrote: > [...] > > > > > > 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. > > If everything looks good, would you please give an official Acked-by or > Reviewed-by? Yeah looks ok to me, Acked-by: Christian Brauner <brauner@kernel.org>
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c index f0b8b7c29126..55758a6fbe90 100644 --- a/kernel/trace/bpf_trace.c +++ b/kernel/trace/bpf_trace.c @@ -24,6 +24,7 @@ #include <linux/key.h> #include <linux/verification.h> #include <linux/namei.h> +#include <linux/fileattr.h> #include <net/bpf_sk_storage.h> @@ -1431,6 +1432,68 @@ static int __init bpf_key_sig_kfuncs_init(void) late_initcall(bpf_key_sig_kfuncs_init); #endif /* CONFIG_KEYS */ +/* filesystem kfuncs */ +__bpf_kfunc_start_defs(); + +/** + * bpf_get_file_xattr - get xattr of a file + * @file: file to get xattr from + * @name__str: name of the xattr + * @value_ptr: output buffer of the xattr value + * + * Get xattr *name__str* of *file* and store the output in *value_ptr*. + * + * For security reasons, only *name__str* with prefix "user." is allowed. + * + * Return: 0 on success, a negative value on error. + */ +__bpf_kfunc int bpf_get_file_xattr(struct file *file, const char *name__str, + struct bpf_dynptr_kern *value_ptr) +{ + struct dentry *dentry; + u32 value_len; + void *value; + + if (strncmp(name__str, XATTR_USER_PREFIX, XATTR_USER_PREFIX_LEN)) + return -EPERM; + + value_len = __bpf_dynptr_size(value_ptr); + value = __bpf_dynptr_data_rw(value_ptr, value_len); + if (!value) + return -EINVAL; + + dentry = file_dentry(file); + return __vfs_getxattr(dentry, dentry->d_inode, name__str, value, value_len); +} + +__bpf_kfunc_end_defs(); + +BTF_SET8_START(fs_kfunc_set_ids) +BTF_ID_FLAGS(func, bpf_get_file_xattr, KF_SLEEPABLE | KF_TRUSTED_ARGS) +BTF_SET8_END(fs_kfunc_set_ids) + +static int bpf_get_file_xattr_filter(const struct bpf_prog *prog, u32 kfunc_id) +{ + if (!btf_id_set8_contains(&fs_kfunc_set_ids, kfunc_id)) + return 0; + + /* Only allow to attach from LSM hooks, to avoid recursion */ + return prog->type != BPF_PROG_TYPE_LSM ? -EACCES : 0; +} + +const struct btf_kfunc_id_set bpf_fs_kfunc_set = { + .owner = THIS_MODULE, + .set = &fs_kfunc_set_ids, + .filter = bpf_get_file_xattr_filter, +}; + +static int __init bpf_fs_kfuncs_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_fs_kfunc_set); +} + +late_initcall(bpf_fs_kfuncs_init); + static const struct bpf_func_proto * bpf_tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog) {
It is common practice for security solutions to store tags/labels in xattrs. To implement similar functionalities in BPF LSM, add new kfunc bpf_get_file_xattr(). The first use case of bpf_get_file_xattr() is to implement file verifications with asymmetric keys. Specificially, security applications could use fsverity for file hashes and use xattr to store file signatures. (kfunc for fsverity hash will be added in a separate commit.) Currently, only xattrs with "user." prefix can be read with kfunc bpf_get_file_xattr(). As use cases evolve, we may add a dedicated prefix for bpf_get_file_xattr(). To avoid recursion, bpf_get_file_xattr can be only called from LSM hooks. Signed-off-by: Song Liu <song@kernel.org> --- kernel/trace/bpf_trace.c | 63 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 63 insertions(+)