| Message ID | 20221222024414.29539-1-sunhao.th@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 8374bfd5a3c90a5b250f7c087c4d2b8ac467b12e |
| Delegated to: | BPF |
| Headers | show |
| Series | [bpf-next,v3,1/2] bpf: fix nullness propagation for reg to reg comparisons | expand |
Hello: This series was applied to bpf/bpf.git (master) by Martin KaFai Lau <martin.lau@kernel.org>: On Thu, 22 Dec 2022 10:44:13 +0800 you wrote: > After befae75856ab, the verifier would propagate null information after > JEQ/JNE, e.g., if two pointers, one is maybe_null and the other is not, > the former would be marked as non-null in eq path. However, as comment > "PTR_TO_BTF_ID points to a kernel struct that does not need to be null > checked by the BPF program ... The verifier must keep this in mind and > can make no assumptions about null or non-null when doing branch ...". > If one pointer is maybe_null and the other is PTR_TO_BTF, the former is > incorrectly marked non-null. The following BPF prog can trigger a > null-ptr-deref, also see this report for more details[1]: > > [...] Here is the summary with links: - [bpf-next,v3,1/2] bpf: fix nullness propagation for reg to reg comparisons https://git.kernel.org/bpf/bpf/c/8374bfd5a3c9 - [bpf-next,v3,2/2] selftests/bpf: check null propagation only neither reg is PTR_TO_BTF_ID https://git.kernel.org/bpf/bpf/c/cedebd74cf38 You are awesome, thank you!
On 12/21/22 6:44 PM, Hao Sun wrote: > After befae75856ab, the verifier would propagate null information after > JEQ/JNE, e.g., if two pointers, one is maybe_null and the other is not, > the former would be marked as non-null in eq path. However, as comment > "PTR_TO_BTF_ID points to a kernel struct that does not need to be null > checked by the BPF program ... The verifier must keep this in mind and > can make no assumptions about null or non-null when doing branch ...". > If one pointer is maybe_null and the other is PTR_TO_BTF, the former is > incorrectly marked non-null. The following BPF prog can trigger a > null-ptr-deref, also see this report for more details[1]: > > 0: (18) r1 = map_fd ; R1_w=map_ptr(ks=4, vs=4) > 2: (79) r6 = *(u64 *)(r1 +8) ; R6_w=bpf_map->inner_map_data > ; R6 is PTR_TO_BTF_ID > ; equals to null at runtime > 3: (bf) r2 = r10 > 4: (07) r2 += -4 > 5: (62) *(u32 *)(r2 +0) = 0 > 6: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null > 7: (1d) if r6 == r0 goto pc+1 > 8: (95) exit > ; from 7 to 9: R0=map_value R6=ptr_bpf_map > 9: (61) r0 = *(u32 *)(r0 +0) ; null-ptr-deref > 10: (95) exit > > So, make the verifier propagate nullness information for reg to reg > comparisons only if neither reg is PTR_TO_BTF_ID. > > [1] https://lore.kernel.org/bpf/CACkBjsaFJwjC5oiw-1KXvcazywodwXo4zGYsRHwbr2gSG9WcSw@mail.gmail.com/T/#u > > Fixes: befae75856ab4 ("bpf: propagate nullness information for reg to reg comparisons") The "Fixes" tag has one more hex digit. I have corrected it and applied to the bpf tree. Thanks. Please run checkpatch.pl in the future: WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title line>")' - ie: 'Fixes: befae75856ab ("bpf: propagate nullness information for reg to reg comparisons")' #35: Fixes: befae75856ab4 ("bpf: propagate nullness information for reg to reg comparisons")
Martin KaFai Lau <martin.lau@linux.dev> 于2022年12月23日周五 09:31写道： > > On 12/21/22 6:44 PM, Hao Sun wrote: > > After befae75856ab, the verifier would propagate null information after > > JEQ/JNE, e.g., if two pointers, one is maybe_null and the other is not, > > the former would be marked as non-null in eq path. However, as comment > > "PTR_TO_BTF_ID points to a kernel struct that does not need to be null > > checked by the BPF program ... The verifier must keep this in mind and > > can make no assumptions about null or non-null when doing branch ...". > > If one pointer is maybe_null and the other is PTR_TO_BTF, the former is > > incorrectly marked non-null. The following BPF prog can trigger a > > null-ptr-deref, also see this report for more details[1]: > > > > 0: (18) r1 = map_fd ; R1_w=map_ptr(ks=4, vs=4) > > 2: (79) r6 = *(u64 *)(r1 +8) ; R6_w=bpf_map->inner_map_data > > ; R6 is PTR_TO_BTF_ID > > ; equals to null at runtime > > 3: (bf) r2 = r10 > > 4: (07) r2 += -4 > > 5: (62) *(u32 *)(r2 +0) = 0 > > 6: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null > > 7: (1d) if r6 == r0 goto pc+1 > > 8: (95) exit > > ; from 7 to 9: R0=map_value R6=ptr_bpf_map > > 9: (61) r0 = *(u32 *)(r0 +0) ; null-ptr-deref > > 10: (95) exit > > > > So, make the verifier propagate nullness information for reg to reg > > comparisons only if neither reg is PTR_TO_BTF_ID. > > > > [1] https://lore.kernel.org/bpf/CACkBjsaFJwjC5oiw-1KXvcazywodwXo4zGYsRHwbr2gSG9WcSw@mail.gmail.com/T/#u > > > > Fixes: befae75856ab4 ("bpf: propagate nullness information for reg to reg comparisons") > The "Fixes" tag has one more hex digit. I have corrected it and applied to the > bpf tree. Thanks. > > Please run checkpatch.pl in the future: > > WARNING: Please use correct Fixes: style 'Fixes: <12 chars of sha1> ("<title > line>")' - ie: 'Fixes: befae75856ab ("bpf: propagate nullness information for > reg to reg comparisons")' > #35: > Fixes: befae75856ab4 ("bpf: propagate nullness information for reg to reg > comparisons") > Noted, thanks!
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index faa358b3d5d7..966d98bfdb60 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -11823,10 +11823,17 @@ static int check_cond_jmp_op(struct bpf_verifier_env *env, * register B - not null * for JNE A, B, ... - A is not null in the false branch; * for JEQ A, B, ... - A is not null in the true branch. + * + * Since PTR_TO_BTF_ID points to a kernel struct that does + * not need to be null checked by the BPF program, i.e., + * could be null even without PTR_MAYBE_NULL marking, so + * only propagate nullness when neither reg is that type. */ if (!is_jmp32 && BPF_SRC(insn->code) == BPF_X && __is_pointer_value(false, src_reg) && __is_pointer_value(false, dst_reg) && - type_may_be_null(src_reg->type) != type_may_be_null(dst_reg->type)) { + type_may_be_null(src_reg->type) != type_may_be_null(dst_reg->type) && + base_type(src_reg->type) != PTR_TO_BTF_ID && + base_type(dst_reg->type) != PTR_TO_BTF_ID) { eq_branch_regs = NULL; switch (opcode) { case BPF_JEQ:
After befae75856ab, the verifier would propagate null information after JEQ/JNE, e.g., if two pointers, one is maybe_null and the other is not, the former would be marked as non-null in eq path. However, as comment "PTR_TO_BTF_ID points to a kernel struct that does not need to be null checked by the BPF program ... The verifier must keep this in mind and can make no assumptions about null or non-null when doing branch ...". If one pointer is maybe_null and the other is PTR_TO_BTF, the former is incorrectly marked non-null. The following BPF prog can trigger a null-ptr-deref, also see this report for more details[1]: 0: (18) r1 = map_fd ; R1_w=map_ptr(ks=4, vs=4) 2: (79) r6 = *(u64 *)(r1 +8) ; R6_w=bpf_map->inner_map_data ; R6 is PTR_TO_BTF_ID ; equals to null at runtime 3: (bf) r2 = r10 4: (07) r2 += -4 5: (62) *(u32 *)(r2 +0) = 0 6: (85) call bpf_map_lookup_elem#1 ; R0_w=map_value_or_null 7: (1d) if r6 == r0 goto pc+1 8: (95) exit ; from 7 to 9: R0=map_value R6=ptr_bpf_map 9: (61) r0 = *(u32 *)(r0 +0) ; null-ptr-deref 10: (95) exit So, make the verifier propagate nullness information for reg to reg comparisons only if neither reg is PTR_TO_BTF_ID. [1] https://lore.kernel.org/bpf/CACkBjsaFJwjC5oiw-1KXvcazywodwXo4zGYsRHwbr2gSG9WcSw@mail.gmail.com/T/#u Fixes: befae75856ab4 ("bpf: propagate nullness information for reg to reg comparisons") Signed-off-by: Hao Sun <sunhao.th@gmail.com> Acked-by: Yonghong Song <yhs@fb.com> --- v1 -> v2 add explanation comments above changes v2 -> v3 rewrite selftests that run under test_progs to use CO-RE --- kernel/bpf/verifier.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) base-commit: 7b43df6c6ec38c9097420902a1c8165c4b25bf70