| Message ID | 20200801170322.75218-9-jolsa@kernel.org |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Sat, Aug 1, 2020 at 10:04 AM Jiri Olsa <jolsa@kernel.org> wrote: > > Adding btf_struct_ids_match function to check if given address provided > by BTF object + offset is also address of another nested BTF object. > > This allows to pass an argument to helper, which is defined via parent > BTF object + offset, like for bpf_d_path (added in following changes): > > SEC("fentry/filp_close") > int BPF_PROG(prog_close, struct file *file, void *id) > { > ... > ret = bpf_d_path(&file->f_path, ... > > The first bpf_d_path argument is hold by verifier as BTF file object > plus offset of f_path member. > > The btf_struct_ids_match function will walk the struct file object and > check if there's nested struct path object on the given offset. > > Signed-off-by: Jiri Olsa <jolsa@kernel.org> > --- > include/linux/bpf.h | 2 ++ > kernel/bpf/btf.c | 31 +++++++++++++++++++++++++++++++ > kernel/bpf/verifier.c | 20 +++++++++++++------- > 3 files changed, 46 insertions(+), 7 deletions(-) > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > index 40c5e206ecf2..8206d5e324be 100644 > --- a/include/linux/bpf.h > +++ b/include/linux/bpf.h > @@ -1337,6 +1337,8 @@ int btf_struct_access(struct bpf_verifier_log *log, > const struct btf_type *t, int off, int size, > enum bpf_access_type atype, > u32 *next_btf_id); > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > + int off, u32 id, u32 need_type_id); > int btf_resolve_helper_id(struct bpf_verifier_log *log, > const struct bpf_func_proto *fn, int); > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > index 7bacc2f56061..ba05b15ad599 100644 > --- a/kernel/bpf/btf.c > +++ b/kernel/bpf/btf.c > @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log, > return -EINVAL; > } > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > + int off, u32 id, u32 need_type_id) > +{ > + const struct btf_type *type; > + int err; > + > + /* Are we already done? */ > + if (need_type_id == id && off == 0) > + return true; > + > +again: > + type = btf_type_by_id(btf_vmlinux, id); > + if (!type) > + return false; > + err = btf_struct_walk(log, type, off, 1, &id); nit: this size=1 looks a bit artificial, seems like btf_struct_walk() will work with size==0 just as well, no? > + if (err != WALK_STRUCT) > + return false; > + > + /* We found nested struct object. If it matches > + * the requested ID, we're done. Otherwise let's > + * continue the search with offset 0 in the new > + * type. > + */ > + if (need_type_id != id) { > + off = 0; > + goto again; > + } > + > + return true; > +} > + > int btf_resolve_helper_id(struct bpf_verifier_log *log, > const struct bpf_func_proto *fn, int arg) > { > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index b6ccfce3bf4c..bb6ca19f282d 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -3960,16 +3960,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > goto err_type; > } > } else if (arg_type == ARG_PTR_TO_BTF_ID) { > + bool ids_match = false; > + > expected_type = PTR_TO_BTF_ID; > if (type != expected_type) > goto err_type; > if (!fn->check_btf_id) { > - if (reg->btf_id != meta->btf_id) { > - verbose(env, "Helper has type %s got %s in R%d\n", > - kernel_type_name(meta->btf_id), > - kernel_type_name(reg->btf_id), regno); > - > - return -EACCES; > + if (reg->btf_id != meta->btf_id || reg->off) { Will it ever succeed if reg->btf_id == meta->btf_id, but reg->off > 0? That would require recursively nested type, which is not possible, right? Or what am I missing? Is it just a simplification of the error handling path? > + ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id, > + meta->btf_id); > + if (!ids_match) { > + verbose(env, "Helper has type %s got %s in R%d\n", > + kernel_type_name(meta->btf_id), > + kernel_type_name(reg->btf_id), regno); > + return -EACCES; > + } > } > } else if (!fn->check_btf_id(reg->btf_id, arg)) { > verbose(env, "Helper does not support %s in R%d\n", > @@ -3977,7 +3982,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > return -EACCES; > } > - if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) { > + if (!ids_match && > + (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) { > verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", > regno); > return -EACCES; > -- > 2.25.4 >
On Tue, Aug 04, 2020 at 11:27:55PM -0700, Andrii Nakryiko wrote: SNIP > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > index 7bacc2f56061..ba05b15ad599 100644 > > --- a/kernel/bpf/btf.c > > +++ b/kernel/bpf/btf.c > > @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log, > > return -EINVAL; > > } > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > + int off, u32 id, u32 need_type_id) > > +{ > > + const struct btf_type *type; > > + int err; > > + > > + /* Are we already done? */ > > + if (need_type_id == id && off == 0) > > + return true; > > + > > +again: > > + type = btf_type_by_id(btf_vmlinux, id); > > + if (!type) > > + return false; > > + err = btf_struct_walk(log, type, off, 1, &id); > > nit: this size=1 looks a bit artificial, seems like btf_struct_walk() > will work with size==0 just as well, no? right, it will work the same for 0 ... not sure why I put originaly 1 byte for size.. probably got mixed up by some condition in btf_struct_walk that I thought 0 wouldn't pass, but it should work, I'll change it, it's less tricky > > > + if (err != WALK_STRUCT) > > + return false; > > + > > + /* We found nested struct object. If it matches > > + * the requested ID, we're done. Otherwise let's > > + * continue the search with offset 0 in the new > > + * type. > > + */ > > + if (need_type_id != id) { > > + off = 0; > > + goto again; > > + } > > + > > + return true; > > +} > > + > > int btf_resolve_helper_id(struct bpf_verifier_log *log, > > const struct bpf_func_proto *fn, int arg) > > { > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index b6ccfce3bf4c..bb6ca19f282d 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -3960,16 +3960,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, > > goto err_type; > > } > > } else if (arg_type == ARG_PTR_TO_BTF_ID) { > > + bool ids_match = false; > > + > > expected_type = PTR_TO_BTF_ID; > > if (type != expected_type) > > goto err_type; > > if (!fn->check_btf_id) { > > - if (reg->btf_id != meta->btf_id) { > > - verbose(env, "Helper has type %s got %s in R%d\n", > > - kernel_type_name(meta->btf_id), > > - kernel_type_name(reg->btf_id), regno); > > - > > - return -EACCES; > > + if (reg->btf_id != meta->btf_id || reg->off) { > > Will it ever succeed if reg->btf_id == meta->btf_id, but reg->off > 0? > That would require recursively nested type, which is not possible, > right? Or what am I missing? Is it just a simplification of the error > handling path? ok, I wanted to cover all possible cases, but did not realized this one is not possible ;-) will revert it to previous version thanks, jirka
On Wed, Aug 05, 2020 at 07:56:51PM +0200, Jiri Olsa wrote: > On Tue, Aug 04, 2020 at 11:27:55PM -0700, Andrii Nakryiko wrote: > > SNIP > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > index 7bacc2f56061..ba05b15ad599 100644 > > > --- a/kernel/bpf/btf.c > > > +++ b/kernel/bpf/btf.c > > > @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > return -EINVAL; > > > } > > > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > + int off, u32 id, u32 need_type_id) > > > +{ > > > + const struct btf_type *type; > > > + int err; > > > + > > > + /* Are we already done? */ > > > + if (need_type_id == id && off == 0) > > > + return true; > > > + > > > +again: > > > + type = btf_type_by_id(btf_vmlinux, id); > > > + if (!type) > > > + return false; > > > + err = btf_struct_walk(log, type, off, 1, &id); > > > > nit: this size=1 looks a bit artificial, seems like btf_struct_walk() > > will work with size==0 just as well, no? > > right, it will work the same for 0 ... not sure why I put > originaly 1 byte for size.. probably got mixed up by some > condition in btf_struct_walk that I thought 0 wouldn't pass, > but it should work, I'll change it, it's less tricky ok, I found why it's 1 ;-) it's this condition in btf_struct_walk: for_each_member(i, t, member) { /* offset of the field in bytes */ moff = btf_member_bit_offset(t, member) / 8; if (off + size <= moff) /* won't find anything, field is already too far */ break; I originaly chose to use 'size = 1' not to medle with this (and probably causing other issues) and in any case we expect that anything we find have at least byte size, so it has some logic ;-) we could make 0 size a special case and don't break the loop for it, but I wonder there's already someone calling it with zero and is expecting it to fail jirka
On Wed, Aug 5, 2020 at 2:31 PM Jiri Olsa <jolsa@redhat.com> wrote: > > On Wed, Aug 05, 2020 at 07:56:51PM +0200, Jiri Olsa wrote: > > On Tue, Aug 04, 2020 at 11:27:55PM -0700, Andrii Nakryiko wrote: > > > > SNIP > > > > > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c > > > > index 7bacc2f56061..ba05b15ad599 100644 > > > > --- a/kernel/bpf/btf.c > > > > +++ b/kernel/bpf/btf.c > > > > @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log, > > > > return -EINVAL; > > > > } > > > > > > > > +bool btf_struct_ids_match(struct bpf_verifier_log *log, > > > > + int off, u32 id, u32 need_type_id) > > > > +{ > > > > + const struct btf_type *type; > > > > + int err; > > > > + > > > > + /* Are we already done? */ > > > > + if (need_type_id == id && off == 0) > > > > + return true; > > > > + > > > > +again: > > > > + type = btf_type_by_id(btf_vmlinux, id); > > > > + if (!type) > > > > + return false; > > > > + err = btf_struct_walk(log, type, off, 1, &id); > > > > > > nit: this size=1 looks a bit artificial, seems like btf_struct_walk() > > > will work with size==0 just as well, no? > > > > right, it will work the same for 0 ... not sure why I put > > originaly 1 byte for size.. probably got mixed up by some > > condition in btf_struct_walk that I thought 0 wouldn't pass, > > but it should work, I'll change it, it's less tricky > > ok, I found why it's 1 ;-) it's this condition in btf_struct_walk: > > for_each_member(i, t, member) { > /* offset of the field in bytes */ > moff = btf_member_bit_offset(t, member) / 8; > if (off + size <= moff) > /* won't find anything, field is already too far */ > break; > > I originaly chose to use 'size = 1' not to medle with this (and probably causing > other issues) and in any case we expect that anything we find have at least byte > size, so it has some logic ;-) > > we could make 0 size a special case and don't break the loop for it, > but I wonder there's already someone calling it with zero and is > expecting it to fail > I see, ok, probably no need. Just let it be for now, I guess. > jirka >
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 40c5e206ecf2..8206d5e324be 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1337,6 +1337,8 @@ int btf_struct_access(struct bpf_verifier_log *log, const struct btf_type *t, int off, int size, enum bpf_access_type atype, u32 *next_btf_id); +bool btf_struct_ids_match(struct bpf_verifier_log *log, + int off, u32 id, u32 need_type_id); int btf_resolve_helper_id(struct bpf_verifier_log *log, const struct bpf_func_proto *fn, int); diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c index 7bacc2f56061..ba05b15ad599 100644 --- a/kernel/bpf/btf.c +++ b/kernel/bpf/btf.c @@ -4160,6 +4160,37 @@ int btf_struct_access(struct bpf_verifier_log *log, return -EINVAL; } +bool btf_struct_ids_match(struct bpf_verifier_log *log, + int off, u32 id, u32 need_type_id) +{ + const struct btf_type *type; + int err; + + /* Are we already done? */ + if (need_type_id == id && off == 0) + return true; + +again: + type = btf_type_by_id(btf_vmlinux, id); + if (!type) + return false; + err = btf_struct_walk(log, type, off, 1, &id); + if (err != WALK_STRUCT) + return false; + + /* We found nested struct object. If it matches + * the requested ID, we're done. Otherwise let's + * continue the search with offset 0 in the new + * type. + */ + if (need_type_id != id) { + off = 0; + goto again; + } + + return true; +} + int btf_resolve_helper_id(struct bpf_verifier_log *log, const struct bpf_func_proto *fn, int arg) { diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index b6ccfce3bf4c..bb6ca19f282d 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -3960,16 +3960,21 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, goto err_type; } } else if (arg_type == ARG_PTR_TO_BTF_ID) { + bool ids_match = false; + expected_type = PTR_TO_BTF_ID; if (type != expected_type) goto err_type; if (!fn->check_btf_id) { - if (reg->btf_id != meta->btf_id) { - verbose(env, "Helper has type %s got %s in R%d\n", - kernel_type_name(meta->btf_id), - kernel_type_name(reg->btf_id), regno); - - return -EACCES; + if (reg->btf_id != meta->btf_id || reg->off) { + ids_match = btf_struct_ids_match(&env->log, reg->off, reg->btf_id, + meta->btf_id); + if (!ids_match) { + verbose(env, "Helper has type %s got %s in R%d\n", + kernel_type_name(meta->btf_id), + kernel_type_name(reg->btf_id), regno); + return -EACCES; + } } } else if (!fn->check_btf_id(reg->btf_id, arg)) { verbose(env, "Helper does not support %s in R%d\n", @@ -3977,7 +3982,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, return -EACCES; } - if (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off) { + if (!ids_match && + (!tnum_is_const(reg->var_off) || reg->var_off.value || reg->off)) { verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", regno); return -EACCES;
Adding btf_struct_ids_match function to check if given address provided by BTF object + offset is also address of another nested BTF object. This allows to pass an argument to helper, which is defined via parent BTF object + offset, like for bpf_d_path (added in following changes): SEC("fentry/filp_close") int BPF_PROG(prog_close, struct file *file, void *id) { ... ret = bpf_d_path(&file->f_path, ... The first bpf_d_path argument is hold by verifier as BTF file object plus offset of f_path member. The btf_struct_ids_match function will walk the struct file object and check if there's nested struct path object on the given offset. Signed-off-by: Jiri Olsa <jolsa@kernel.org> --- include/linux/bpf.h | 2 ++ kernel/bpf/btf.c | 31 +++++++++++++++++++++++++++++++ kernel/bpf/verifier.c | 20 +++++++++++++------- 3 files changed, 46 insertions(+), 7 deletions(-)