| Message ID | 20200807094559.571260-9-jolsa@kernel.org |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | bpf: Add d_path helper | expand |
On Fri, Aug 7, 2020 at 2:47 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 | 18 ++++++++++++------ > 3 files changed, 45 insertions(+), 6 deletions(-) > [...] > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index b6ccfce3bf4c..041d151be15b 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; > + 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)) { Put this on a wishlist for now. I don't think we should expect fb->check_btf_id() to do btf_struct_ids_match() internally, so to support this, we'd have to call fb->check_btf_id() inside the loop while doing WALK_STRUCT struct. But let's not change all this in this patch set, it's involved enough already. > 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)) { Isn't this still wrong? if ids_match, but reg->var_off is non-zero, that's still bad, right? ids_match just "mitigates" reg->off check, so should be something like this: if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || reg->var_off.value) ... then bad ... > verbose(env, "R%d is a pointer to in-kernel struct with non-zero offset\n", > regno); > return -EACCES; > -- > 2.25.4 >
On Fri, Aug 07, 2020 at 01:04:26PM -0700, Andrii Nakryiko wrote: SNIP > > + } > > } > > } else if (!fn->check_btf_id(reg->btf_id, arg)) { > > Put this on a wishlist for now. I don't think we should expect > fb->check_btf_id() to do btf_struct_ids_match() internally, so to > support this, we'd have to call fb->check_btf_id() inside the loop > while doing WALK_STRUCT struct. But let's not change all this in this > patch set, it's involved enough already. > > > 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)) { > > Isn't this still wrong? if ids_match, but reg->var_off is non-zero, > that's still bad, right? > ids_match just "mitigates" reg->off check, so should be something like this: > > if ((reg->off && !ids_match) || !tnum_is_const(reg->var_off) || > reg->var_off.value) > ... then bad ... damn you're right, those are separated things, I mixed it up, I'll send new version thanks, jirka
diff --git a/include/linux/bpf.h b/include/linux/bpf.h index cef4ef0d2b4e..9fead3baa31c 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -1338,6 +1338,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 d8d64201c4e0..df966acaaeb1 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..041d151be15b 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; + 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 | 18 ++++++++++++------ 3 files changed, 45 insertions(+), 6 deletions(-)