bpf: Fix incorrect precision backtracking

Commit Message

Tao Lyu Nov. 1, 2024, 7:57 p.m. UTC

Hi,

The process_iter_arg check function misses the type check on the iter
args, which leads to any pointer types can be passed as iter args.

As the attached testcase shows, when I pass a ptr_to_map_value whose
offset is 0, process_iter_arg still regards it as a stack pointer and
use its offset to check the stack slot types.

In this case, as long as the stack slot types matched with the
ptr_to_map_value offset is correct, then checks can be bypassed.

I attached the fix, which checks if the argument type is stack pointer.

Please let me know if this fix might be incomplete.
I'm happy to revise it.

Best,
Tao

Signed-off-by: Tao Lyu <[e-mail]>
---
 kernel/bpf/verifier.c                     |  6 ++++++
 tools/testing/selftests/bpf/progs/iters.c | 23 +++++++++++++++++++++++
 2 files changed, 29 insertions(+)

Comments

Alexei Starovoitov Nov. 1, 2024, 8:22 p.m. UTC | #1

On Fri, Nov 1, 2024 at 1:04 PM Tao Lyu <[e-mail]> wrote:
>
> Hi,
>
> The process_iter_arg check function misses the type check on the iter
> args, which leads to any pointer types can be passed as iter args.
>
> As the attached testcase shows, when I pass a ptr_to_map_value whose
> offset is 0, process_iter_arg still regards it as a stack pointer and
> use its offset to check the stack slot types.
>
> In this case, as long as the stack slot types matched with the
> ptr_to_map_value offset is correct, then checks can be bypassed.
>
> I attached the fix, which checks if the argument type is stack pointer.
>
> Please let me know if this fix might be incomplete.
> I'm happy to revise it.
>
> Best,
> Tao
>
> Signed-off-by: Tao Lyu <[e-mail]>
> ---
>  kernel/bpf/verifier.c                     |  6 ++++++
>  tools/testing/selftests/bpf/progs/iters.c | 23 +++++++++++++++++++++++
>  2 files changed, 29 insertions(+)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 797cf3ed32e0..bc968d2b76d9 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -8031,6 +8031,12 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
>                 return -EINVAL;
>         }
>         t = btf_type_by_id(meta->btf, btf_id);
> +
> +       // Ensure the iter arg is a stack pointer

no c++ comments pls.

Also I believe Kumar sent a fix for this already.
It fell through the cracks.

Kumar,
please resend.

pw-bot: cr

> +       if (reg->type != PTR_TO_STACK) {
> +               verbose(env, "iter pointer should be the PTR_TO_STACK type\n");
> +               return -EINVAL;
> +       }

Kumar Kartikeya Dwivedi Nov. 3, 2024, 3:56 p.m. UTC | #2

On Fri, 1 Nov 2024 at 15:22, Alexei Starovoitov
<[e-mail]> wrote:
>
> On Fri, Nov 1, 2024 at 1:04 PM Tao Lyu <[e-mail]> wrote:
> >
> > Hi,
> >
> > The process_iter_arg check function misses the type check on the iter
> > args, which leads to any pointer types can be passed as iter args.
> >
> > As the attached testcase shows, when I pass a ptr_to_map_value whose
> > offset is 0, process_iter_arg still regards it as a stack pointer and
> > use its offset to check the stack slot types.
> >
> > In this case, as long as the stack slot types matched with the
> > ptr_to_map_value offset is correct, then checks can be bypassed.
> >
> > I attached the fix, which checks if the argument type is stack pointer.
> >
> > Please let me know if this fix might be incomplete.
> > I'm happy to revise it.
> >
> > Best,
> > Tao
> >
> > Signed-off-by: Tao Lyu <[e-mail]>
> > ---
> >  kernel/bpf/verifier.c                     |  6 ++++++
> >  tools/testing/selftests/bpf/progs/iters.c | 23 +++++++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> > index 797cf3ed32e0..bc968d2b76d9 100644
> > --- a/kernel/bpf/verifier.c
> > +++ b/kernel/bpf/verifier.c
> > @@ -8031,6 +8031,12 @@ static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
> >                 return -EINVAL;
> >         }
> >         t = btf_type_by_id(meta->btf, btf_id);
> > +
> > +       // Ensure the iter arg is a stack pointer
>
> no c++ comments pls.
>
> Also I believe Kumar sent a fix for this already.
> It fell through the cracks.
>
> Kumar,
> please resend.

For this one, I didn't. I believe the one you're referring to was a
different bug, and yeah, that appears to have fallen through.
I handed it over to someone and they unfortunately happened to switch
jobs after that.
I'll sync with Tao and one of us will resend the other fix.

Tao,
For this, I think you need to fix your subject line as well, and add a
selftest covering this particular case.
Passing in e.g. a map value should be sufficient to test this behavior.

>
> pw-bot: cr
>
> > +       if (reg->type != PTR_TO_STACK) {
> > +               verbose(env, "iter pointer should be the PTR_TO_STACK type\n");
> > +               return -EINVAL;
> > +       }

Patch

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 797cf3ed32e0..bc968d2b76d9 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -8031,6 +8031,12 @@  static int process_iter_arg(struct bpf_verifier_env *env, int regno, int insn_id
 		return -EINVAL;
 	}
 	t = btf_type_by_id(meta->btf, btf_id);
+
+	// Ensure the iter arg is a stack pointer
+	if (reg->type != PTR_TO_STACK) {
+		verbose(env, "iter pointer should be the PTR_TO_STACK type\n");
+		return -EINVAL;
+	}
 	nr_slots = t->size / BPF_REG_SIZE;
 
 	if (is_iter_new_kfunc(meta)) {
diff --git a/tools/testing/selftests/bpf/progs/iters.c b/tools/testing/selftests/bpf/progs/iters.c
index ef70b88bccb2..52078fc395fd 100644
--- a/tools/testing/selftests/bpf/progs/iters.c
+++ b/tools/testing/selftests/bpf/progs/iters.c
@@ -1486,4 +1486,27 @@  int iter_subprog_check_stacksafe(const void *ctx)
 	return 0;
 }
 
+SEC("raw_tp")
+__failure __msg("iter pointer should be the PTR_TO_STACK type")
+int iter_check_arg_type(const void *ctx)
+{
+        struct bpf_iter_num it;
+        int *v;
+
+        int *map_val = NULL;
+        int key = 0;
+
+        map_val = bpf_map_lookup_elem(&arr_map, &key);
+        if (!map_val)
+                return 0;
+
+        bpf_iter_num_new(&it, 0, 3);
+        while ((v = bpf_iter_num_next((struct bpf_iter_num*)map_val))) {
+                bpf_printk("ITER_BASIC: E1 VAL: v=%d", *v);
+        }
+        bpf_iter_num_destroy(&it);
+
+        return 0;
+}
+
 char _license[] SEC("license") = "GPL";