On Fri, Aug 7, 2020 at 2:47 AM Jiri Olsa <[email protected]> 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 <[email protected]>
> ---
> 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
>
