| Message ID | 20231013182644.2346458-3-song@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | bpf: file verification with LSM and fsverity | expand |
On Fri, Oct 13, 2023 at 11:26:41AM -0700, Song Liu wrote: > The kfunc can be used to read fsverity_digest, so that we can verify > signature in BPF LSM. > > This kfunc is added to fs/verity/measure.c because some data structure used > in the function is private to fsverity (fs/verity/fsverity_private.h). > > Signed-off-by: Song Liu <song@kernel.org> > --- > fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/fs/verity/measure.c b/fs/verity/measure.c > index eec5956141da..2d4b2e6f5a5d 100644 > --- a/fs/verity/measure.c > +++ b/fs/verity/measure.c > @@ -8,6 +8,8 @@ > #include "fsverity_private.h" > > #include <linux/uaccess.h> > +#include <linux/bpf.h> > +#include <linux/btf.h> > > /** > * fsverity_ioctl_measure() - get a verity file's digest > @@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode, > return hash_alg->digest_size; > } > EXPORT_SYMBOL_GPL(fsverity_get_digest); > + > +/* bpf kfuncs */ > +__diag_push(); > +__diag_ignore_all("-Wmissing-prototypes", > + "kfuncs which will be used in BPF programs"); > + > +/** > + * bpf_get_fsverity_digest: read fsverity digest of file > + * @file: file to get digest from > + * @digest_ptr: (out) dynptr for struct fsverity_digest > + * > + * Read fsverity_digest of *file* into *digest_ptr*. > + * > + * Return: 0 on success, a negative value on error. > + */ > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > +{ > + const struct inode *inode = file_inode(file); > + struct fsverity_digest *arg = digest_ptr->data; What alignment is guaranteed here? > + const struct fsverity_info *vi; > + const struct fsverity_hash_alg *hash_alg; > + int out_digest_sz; > + > + if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest)) > + return -EINVAL; > + > + vi = fsverity_get_info(inode); > + if (!vi) > + return -ENODATA; /* not a verity file */ > + > + hash_alg = vi->tree_params.hash_alg; > + > + arg->digest_algorithm = hash_alg - fsverity_hash_algs; > + arg->digest_size = hash_alg->digest_size; > + > + out_digest_sz = __bpf_dynptr_size(digest_ptr) - sizeof(struct fsverity_digest); > + > + /* copy digest */ > + memcpy(arg->digest, vi->file_digest, min_t(int, hash_alg->digest_size, out_digest_sz)); > + > + /* fill the extra buffer with zeros */ > + memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size); Can't 'out_digest_sz - hash_alg->digest_size' underflow? > + > + return 0; > +} > + > +__diag_pop(); > + > +BTF_SET8_START(fsverity_set) > +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE) Should it be sleepable? Nothing in it sleeps, as far as I can tell. > +BTF_SET8_END(fsverity_set) > + > +const struct btf_kfunc_id_set bpf_fsverity_set = { > + .owner = THIS_MODULE, > + .set = &fsverity_set, > +}; static const? > + > +static int __init bpf_fsverity_init(void) > +{ > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, > + &bpf_fsverity_set); > +} > + > +late_initcall(bpf_fsverity_init); Maybe this should be called by the existing fsverity_init() initcall instead of having a brand new initcall just for this. Also, doesn't this all need to be guarded by a kconfig such as CONFIG_BPF? Also, it looks like I'm being signed up to maintain this. This isn't a stable UAPI, right? No need to document this in Documentation/? - Eric
On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > [...] > > + */ > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > > +{ > > + const struct inode *inode = file_inode(file); > > + struct fsverity_digest *arg = digest_ptr->data; > > What alignment is guaranteed here? drnptr doesn't not provide alignment guarantee for digest_ptr->data. If we need alignment guarantee, we need to add it here. > > > + const struct fsverity_info *vi; > > + const struct fsverity_hash_alg *hash_alg; > > + int out_digest_sz; > > + > > + if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest)) > > + return -EINVAL; > > + > > + vi = fsverity_get_info(inode); > > + if (!vi) > > + return -ENODATA; /* not a verity file */ > > + > > + hash_alg = vi->tree_params.hash_alg; > > + > > + arg->digest_algorithm = hash_alg - fsverity_hash_algs; > > + arg->digest_size = hash_alg->digest_size; > > + > > + out_digest_sz = __bpf_dynptr_size(digest_ptr) - sizeof(struct fsverity_digest); > > + > > + /* copy digest */ > > + memcpy(arg->digest, vi->file_digest, min_t(int, hash_alg->digest_size, out_digest_sz)); > > + > > + /* fill the extra buffer with zeros */ > > + memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size); > > Can't 'out_digest_sz - hash_alg->digest_size' underflow? Will fix this in the next version. > > > + > > + return 0; > > +} > > + > > +__diag_pop(); > > + > > +BTF_SET8_START(fsverity_set) > > +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE) > > Should it be sleepable? Nothing in it sleeps, as far as I can tell. Indeed, we can remove sleepable requirement here. > > > +BTF_SET8_END(fsverity_set) > > + > > +const struct btf_kfunc_id_set bpf_fsverity_set = { > > + .owner = THIS_MODULE, > > + .set = &fsverity_set, > > +}; > > static const? Will fix in v2. > > > + > > +static int __init bpf_fsverity_init(void) > > +{ > > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, > > + &bpf_fsverity_set); > > +} > > + > > +late_initcall(bpf_fsverity_init); > > Maybe this should be called by the existing fsverity_init() initcall instead of > having a brand new initcall just for this. Yeah, that would also work. > > Also, doesn't this all need to be guarded by a kconfig such as CONFIG_BPF? Will add this in v2. > > Also, it looks like I'm being signed up to maintain this. This isn't a stable > UAPI, right? No need to document this in Documentation/? BPF kfuncs are not UAPI. They are as stable as exported symbols. We do have some documents for BPF kfuncs, for example in Documentation/bpf/cpumasks.rst. Do you have a recommendation or preference on where we should document this? AFAICT, we can either add it to fsverity.rst or somewhere in Documentation/bpf/. Thanks, Song
On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote: > On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > [...] > > > + */ > > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > > > +{ > > > + const struct inode *inode = file_inode(file); > > > + struct fsverity_digest *arg = digest_ptr->data; > > > > What alignment is guaranteed here? > > drnptr doesn't not provide alignment guarantee for digest_ptr->data. > If we need alignment guarantee, we need to add it here. So technically it's wrong to cast it to struct fsverity_digest, then. > > > > Also, it looks like I'm being signed up to maintain this. This isn't a stable > > UAPI, right? No need to document this in Documentation/? > > BPF kfuncs are not UAPI. They are as stable as exported symbols. > We do have some documents for BPF kfuncs, for example in > Documentation/bpf/cpumasks.rst. > > Do you have a recommendation or preference on where we should > document this? AFAICT, we can either add it to fsverity.rst or somewhere > in Documentation/bpf/. The BPF documentation seems like the right place. - Eric
On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote: > > On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > [...] > > > > + */ > > > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > > > > +{ > > > > + const struct inode *inode = file_inode(file); > > > > + struct fsverity_digest *arg = digest_ptr->data; > > > > > > What alignment is guaranteed here? > > > > drnptr doesn't not provide alignment guarantee for digest_ptr->data. > > If we need alignment guarantee, we need to add it here. > > So technically it's wrong to cast it to struct fsverity_digest, then. We can enforce alignment here. Would __aligned(2) be sufficient? Thanks, Song
On Mon, Oct 16, 2023 at 10:35:16PM -0700, Song Liu wrote: > On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote: > > > On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > [...] > > > > > + */ > > > > > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > > > > > +{ > > > > > + const struct inode *inode = file_inode(file); > > > > > + struct fsverity_digest *arg = digest_ptr->data; > > > > > > > > What alignment is guaranteed here? > > > > > > drnptr doesn't not provide alignment guarantee for digest_ptr->data. > > > If we need alignment guarantee, we need to add it here. > > > > So technically it's wrong to cast it to struct fsverity_digest, then. > > We can enforce alignment here. Would __aligned(2) be sufficient? > Do you mean something like the following: if (!IS_ALIGNED((uintptr_t)digest_ptr->data, __alignof__(*arg))) return -EINVAL;
> On Oct 16, 2023, at 10:46 PM, Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Oct 16, 2023 at 10:35:16PM -0700, Song Liu wrote: >> On Mon, Oct 16, 2023 at 8:12 PM Eric Biggers <ebiggers@kernel.org> wrote: >>> >>> On Mon, Oct 16, 2023 at 01:10:40PM -0700, Song Liu wrote: >>>> On Sun, Oct 15, 2023 at 12:07 AM Eric Biggers <ebiggers@kernel.org> wrote: >>>>> >>>> [...] >>>>>> + */ >>>>>> +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) >>>>>> +{ >>>>>> + const struct inode *inode = file_inode(file); >>>>>> + struct fsverity_digest *arg = digest_ptr->data; >>>>> >>>>> What alignment is guaranteed here? >>>> >>>> drnptr doesn't not provide alignment guarantee for digest_ptr->data. >>>> If we need alignment guarantee, we need to add it here. >>> >>> So technically it's wrong to cast it to struct fsverity_digest, then. >> >> We can enforce alignment here. Would __aligned(2) be sufficient? >> > > Do you mean something like the following: > > if (!IS_ALIGNED((uintptr_t)digest_ptr->data, __alignof__(*arg))) > return -EINVAL; I was thinking about hard-coding the alignment requirement. __alignof__ is much better. Thanks for the suggestion! Song
On Fri, Oct 13, 2023 at 11:29 AM Song Liu <song@kernel.org> wrote: > > The kfunc can be used to read fsverity_digest, so that we can verify > signature in BPF LSM. > > This kfunc is added to fs/verity/measure.c because some data structure used > in the function is private to fsverity (fs/verity/fsverity_private.h). > > Signed-off-by: Song Liu <song@kernel.org> > --- > fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/fs/verity/measure.c b/fs/verity/measure.c > index eec5956141da..2d4b2e6f5a5d 100644 > --- a/fs/verity/measure.c > +++ b/fs/verity/measure.c > @@ -8,6 +8,8 @@ > #include "fsverity_private.h" > > #include <linux/uaccess.h> > +#include <linux/bpf.h> > +#include <linux/btf.h> > > /** > * fsverity_ioctl_measure() - get a verity file's digest > @@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode, > return hash_alg->digest_size; > } > EXPORT_SYMBOL_GPL(fsverity_get_digest); > + > +/* bpf kfuncs */ > +__diag_push(); > +__diag_ignore_all("-Wmissing-prototypes", > + "kfuncs which will be used in BPF programs"); > + > +/** > + * bpf_get_fsverity_digest: read fsverity digest of file > + * @file: file to get digest from > + * @digest_ptr: (out) dynptr for struct fsverity_digest > + * > + * Read fsverity_digest of *file* into *digest_ptr*. > + * > + * Return: 0 on success, a negative value on error. > + */ > +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) > +{ > + const struct inode *inode = file_inode(file); > + struct fsverity_digest *arg = digest_ptr->data; this can be null I think we need some internal helpers that are similar to bpf_dynptr_slice() that would handle invalid dynptr cases, as well as abstract away potentially non-contiguous memory dynptr points to. WDYT? > + const struct fsverity_info *vi; > + const struct fsverity_hash_alg *hash_alg; > + int out_digest_sz; > + > + if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest)) > + return -EINVAL; > + > + vi = fsverity_get_info(inode); > + if (!vi) > + return -ENODATA; /* not a verity file */ > + > + hash_alg = vi->tree_params.hash_alg; > + > + arg->digest_algorithm = hash_alg - fsverity_hash_algs; > + arg->digest_size = hash_alg->digest_size; > + > + out_digest_sz = __bpf_dynptr_size(digest_ptr) - sizeof(struct fsverity_digest); > + > + /* copy digest */ > + memcpy(arg->digest, vi->file_digest, min_t(int, hash_alg->digest_size, out_digest_sz)); > + > + /* fill the extra buffer with zeros */ > + memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size); > + > + return 0; > +} > + > +__diag_pop(); > + > +BTF_SET8_START(fsverity_set) > +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE) > +BTF_SET8_END(fsverity_set) > + > +const struct btf_kfunc_id_set bpf_fsverity_set = { > + .owner = THIS_MODULE, > + .set = &fsverity_set, > +}; > + > +static int __init bpf_fsverity_init(void) > +{ > + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, > + &bpf_fsverity_set); > +} > + > +late_initcall(bpf_fsverity_init); > -- > 2.34.1 >
diff --git a/fs/verity/measure.c b/fs/verity/measure.c index eec5956141da..2d4b2e6f5a5d 100644 --- a/fs/verity/measure.c +++ b/fs/verity/measure.c @@ -8,6 +8,8 @@ #include "fsverity_private.h" #include <linux/uaccess.h> +#include <linux/bpf.h> +#include <linux/btf.h> /** * fsverity_ioctl_measure() - get a verity file's digest @@ -100,3 +102,67 @@ int fsverity_get_digest(struct inode *inode, return hash_alg->digest_size; } EXPORT_SYMBOL_GPL(fsverity_get_digest); + +/* bpf kfuncs */ +__diag_push(); +__diag_ignore_all("-Wmissing-prototypes", + "kfuncs which will be used in BPF programs"); + +/** + * bpf_get_fsverity_digest: read fsverity digest of file + * @file: file to get digest from + * @digest_ptr: (out) dynptr for struct fsverity_digest + * + * Read fsverity_digest of *file* into *digest_ptr*. + * + * Return: 0 on success, a negative value on error. + */ +__bpf_kfunc int bpf_get_fsverity_digest(struct file *file, struct bpf_dynptr_kern *digest_ptr) +{ + const struct inode *inode = file_inode(file); + struct fsverity_digest *arg = digest_ptr->data; + const struct fsverity_info *vi; + const struct fsverity_hash_alg *hash_alg; + int out_digest_sz; + + if (__bpf_dynptr_size(digest_ptr) < sizeof(struct fsverity_digest)) + return -EINVAL; + + vi = fsverity_get_info(inode); + if (!vi) + return -ENODATA; /* not a verity file */ + + hash_alg = vi->tree_params.hash_alg; + + arg->digest_algorithm = hash_alg - fsverity_hash_algs; + arg->digest_size = hash_alg->digest_size; + + out_digest_sz = __bpf_dynptr_size(digest_ptr) - sizeof(struct fsverity_digest); + + /* copy digest */ + memcpy(arg->digest, vi->file_digest, min_t(int, hash_alg->digest_size, out_digest_sz)); + + /* fill the extra buffer with zeros */ + memset(arg->digest + arg->digest_size, 0, out_digest_sz - hash_alg->digest_size); + + return 0; +} + +__diag_pop(); + +BTF_SET8_START(fsverity_set) +BTF_ID_FLAGS(func, bpf_get_fsverity_digest, KF_SLEEPABLE) +BTF_SET8_END(fsverity_set) + +const struct btf_kfunc_id_set bpf_fsverity_set = { + .owner = THIS_MODULE, + .set = &fsverity_set, +}; + +static int __init bpf_fsverity_init(void) +{ + return register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, + &bpf_fsverity_set); +} + +late_initcall(bpf_fsverity_init);
The kfunc can be used to read fsverity_digest, so that we can verify signature in BPF LSM. This kfunc is added to fs/verity/measure.c because some data structure used in the function is private to fsverity (fs/verity/fsverity_private.h). Signed-off-by: Song Liu <song@kernel.org> --- fs/verity/measure.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 66 insertions(+)