| Message ID | 20221114191547.1694267-20-memxor@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Allocated objects, BPF linked lists | expand |
On Tue, Nov 15, 2022 at 12:45:40AM +0530, Kumar Kartikeya Dwivedi wrote: > if (type_may_be_null(reg->type) && reg->id == id && > !WARN_ON_ONCE(!reg->id)) { > - if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || > - !tnum_equals_const(reg->var_off, 0) || > - reg->off)) { > + if (reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0) || reg->off) { .... > + if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0))) That is too much copy-paste between two lines. Please combine the checks.
On Tue, Nov 15, 2022 at 11:53:19AM IST, Alexei Starovoitov wrote: > On Tue, Nov 15, 2022 at 12:45:40AM +0530, Kumar Kartikeya Dwivedi wrote: > > if (type_may_be_null(reg->type) && reg->id == id && > > !WARN_ON_ONCE(!reg->id)) { > > - if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || > > - !tnum_equals_const(reg->var_off, 0) || > > - reg->off)) { > > + if (reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0) || reg->off) { > .... > > + if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0))) > > That is too much copy-paste between two lines. > Please combine the checks. I have rewritten it like this: if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0))) ‣a: reg->var_off ‣b: 0 ‣: int return; if (reg->type != (PTR_TO_BTF_ID | MEM_ALLOC | PTR_MAYBE_NULL) && WARN_ON_ONCE(reg->off)) ‣: int return; I prefer to keep the WARN, as it would be pretty clearly a verifier bug that would be silently missed since the return type is void.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 7372737cbde9..e194c3feb01f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -10800,15 +10800,20 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state, { if (type_may_be_null(reg->type) && reg->id == id && !WARN_ON_ONCE(!reg->id)) { - if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || - !tnum_equals_const(reg->var_off, 0) || - reg->off)) { + if (reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0) || reg->off) { /* Old offset (both fixed and variable parts) should * have been known-zero, because we don't allow pointer * arithmetic on pointers that might be NULL. If we * see this happening, don't convert the register. + * + * But in some cases, some helpers that return local + * kptrs advance offset for the returned pointer. + * In those cases, it is fine to expect to see reg->off. */ - return; + if (WARN_ON_ONCE(reg->type != (PTR_TO_BTF_ID | MEM_ALLOC | PTR_MAYBE_NULL))) + return; + if (WARN_ON_ONCE(reg->smin_value || reg->smax_value || !tnum_equals_const(reg->var_off, 0))) + return; } if (is_null) { reg->type = SCALAR_VALUE;
Pointer increment on seeing PTR_MAYBE_NULL is already protected against, hence make an exception for PTR_TO_BTF_ID | MEM_ALLOC while still keeping the warning for other unintended cases that might creep in. bpf_list_pop_{front,_back} helpers planned to be introduced in next commit will return a MEM_ALLOC register with incremented offset pointing to bpf_list_node field. The user is supposed to then obtain the pointer to the entry using container_of after NULL checking it. The current restrictions trigger a warning when doing the NULL checking. Revisiting the reason, it is meant as an assertion which seems to actually work and catch the bad case. Hence, under no other circumstances can reg->off be non-zero for a register that has the PTR_MAYBE_NULL type flag set. Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com> --- kernel/bpf/verifier.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)