On Tue, Jun 30, 2020 at 12:09 PM John Fastabend
<john.fastabend@xxxxxxxxx> wrote:
>
> Yonghong Song wrote:
> > Wenbo reported an issue in [1] where a checking of null
> > pointer is evaluated as always false. In this particular
> > case, the program type is tp_btf and the pointer to
> > compare is a PTR_TO_BTF_ID.
> >
> > The current verifier considers PTR_TO_BTF_ID always
> > reprents a non-null pointer, hence all PTR_TO_BTF_ID compares
> > to 0 will be evaluated as always not-equal, which resulted
> > in the branch elimination.
> >
> > For example,
> > struct bpf_fentry_test_t {
> > struct bpf_fentry_test_t *a;
> > };
> > int BPF_PROG(test7, struct bpf_fentry_test_t *arg)
> > {
> > if (arg == 0)
> > test7_result = 1;
> > return 0;
> > }
> > int BPF_PROG(test8, struct bpf_fentry_test_t *arg)
> > {
> > if (arg->a == 0)
> > test8_result = 1;
> > return 0;
> > }
> >
> > In above bpf programs, both branch arg == 0 and arg->a == 0
> > are removed. This may not be what developer expected.
> >
> > The bug is introduced by Commit cac616db39c2 ("bpf: Verifier
> > track null pointer branch_taken with JNE and JEQ"),
> > where PTR_TO_BTF_ID is considered to be non-null when evaluting
> > pointer vs. scalar comparison. This may be added
> > considering we have PTR_TO_BTF_ID_OR_NULL in the verifier
> > as well.
> >
> > PTR_TO_BTF_ID_OR_NULL is added to explicitly requires
> > a non-NULL testing in selective cases. The current generic
> > pointer tracing framework in verifier always
> > assigns PTR_TO_BTF_ID so users does not need to
> > check NULL pointer at every pointer level like a->b->c->d.
>
> Thanks for fixing this.
>
> But, don't we really need to check for null? I'm trying to
> understand how we can avoid the check. If b is NULL above
> we will have a problem no?
BPF JIT installs an exception handler for each direct memory access,
so that if it turns out to be NULL, it will be caught and 0 will be
assigned to the target register and BPF program will continue
execution. In that sense, it simulates bpf_probe_read() behavior and
significantly improves usability.
>
> Also, we probably shouldn't name the type PTR_TO_BTF_ID if
> it can be NULL. How about renaming it in bpf-next then although
> it will be code churn... Or just fix the comments? Probably
> bpf-next content though. wdyt? In my opinion the comments and
> type names are really misleading as it stands.
>
[...]
