• John Fastabend's avatar
    bpf: Verifier track null pointer branch_taken with JNE and JEQ · cac616db
    John Fastabend authored
    Currently, when considering the branches that may be taken for a jump
    instruction if the register being compared is a pointer the verifier
    assumes both branches may be taken. But, if the jump instruction
    is comparing if a pointer is NULL we have this information in the
    verifier encoded in the reg->type so we can do better in these cases.
    Specifically, these two common cases can be handled.
    
     * If the instruction is BPF_JEQ and we are comparing against a
       zero value. This test is 'if ptr == 0 goto +X' then using the
       type information in reg->type we can decide if the ptr is not
       null. This allows us to avoid pushing both branches onto the
       stack and instead only use the != 0 case. For example
       PTR_TO_SOCK and PTR_TO_SOCK_OR_NULL encode the null pointer.
       Note if the type is PTR_TO_SOCK_OR_NULL we can not learn anything.
       And also if the value is non-zero we learn nothing because it
       could be any arbitrary value a different pointer for example
    
     * If the instruction is BPF_JNE and ware comparing against a zero
       value then a similar analysis as above can be done. The test in
       asm looks like 'if ptr != 0 goto +X'. Again using the type
       information if the non null type is set (from above PTR_TO_SOCK)
       we know the jump is taken.
    
    In this patch we extend is_branch_taken() to consider this extra
    information and to return only the branch that will be taken. This
    resolves a verifier issue reported with C code like the following.
    See progs/test_sk_lookup_kern.c in selftests.
    
     sk = bpf_sk_lookup_tcp(skb, tuple, tuple_len, BPF_F_CURRENT_NETNS, 0);
     bpf_printk("sk=%d\n", sk ? 1 : 0);
     if (sk)
       bpf_sk_release(sk);
     return sk ? TC_ACT_OK : TC_ACT_UNSPEC;
    
    In the above the bpf_printk() will resolve the pointer from
    PTR_TO_SOCK_OR_NULL to PTR_TO_SOCK. Then the second test guarding
    the release will cause the verifier to walk both paths resulting
    in the an unreleased sock reference. See verifier/ref_tracking.c
    in selftests for an assembly version of the above.
    
    After the above additional logic is added the C code above passes
    as expected.
    Reported-by: default avatarAndrey Ignatov <rdna@fb.com>
    Suggested-by: default avatarAlexei Starovoitov <ast@kernel.org>
    Signed-off-by: default avatarJohn Fastabend <john.fastabend@gmail.com>
    Signed-off-by: default avatarAlexei Starovoitov <ast@kernel.org>
    Link: https://lore.kernel.org/bpf/159009164651.6313.380418298578070501.stgit@john-Precision-5820-Tower
    cac616db
verifier.c 315 KB