On 4/20/19 5:38 AM, Paul Chaignon wrote:
> In case of a null check on a pointer inside a subprog, we should mark all
> registers with this pointer as either safe or unknown, in both the current
> and previous frames. Currently, only spilled registers and registers in
> the current frame are marked. This first patch also marks registers in
> previous frames.
>
> A good reproducer looks as follow:
>
> 1: ptr = bpf_map_lookup_elem(map, &key);
> 2: ret = subprog(ptr) {
> 3: return ptr != NULL;
> 4: }
> 5: if (ret)
> 6: value = *ptr;
>
> With the above, the verifier will complain on line 6 because it sees ptr
> as map_value_or_null despite the null check in subprog 1. The second
> patch implements the above as a new test case.
>
> Note that this patch fixes another resulting bug when using
> bpf_sk_release():
>
> 1: sk = bpf_sk_lookup_tcp();
> 2: subprog(sk) {
> 3: if (sk)
> 4: bpf_sk_release(sk, 0);
The specification for bpf_sk_release() in uapi/linux/bpf.h is:
int bpf_sk_release(struct bpf_sock *sock)
Do you explain what is bpf_sk_release(sk, 0)?
> 5: }
> 6: if (!sk)
> 7: return 0;
> 8: return sk;
If sk has been released, the program should not really return sk, right?
>
> In the above, mark_ptr_or_null_regs will warn on line 6 because it will
> try to free the reference state, even though it was already freed on
> line 3.
>
> Paul Chaignon (2):
> bpf: mark registers as safe or unknown in all frames
> selftests/bpf: test case for pointer null check in subprog
>
> kernel/bpf/verifier.c | 6 ++---
> tools/testing/selftests/bpf/verifier/calls.c | 25 ++++++++++++++++++++
> 2 files changed, 28 insertions(+), 3 deletions(-)
>
