| Message ID | 20201019194212.1050855-1-kafai@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | bpf: Enforce NULL check on new _OR_NULL return types | expand |
On Mon, Oct 19, 2020 at 12:43 PM Martin KaFai Lau <kafai@fb.com> wrote: > > The commit af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper") > introduces RET_PTR_TO_BTF_ID_OR_NULL and > the commit eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()") > introduces RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL. > Note that for RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, the reg0->type > could become PTR_TO_MEM_OR_NULL which is not covered by > BPF_PROBE_MEM. > > The BPF_REG_0 will then hold a _OR_NULL pointer type. This _OR_NULL > pointer type requires the bpf program to explicitly do a NULL check first. > After NULL check, the verifier will mark all registers having > the same reg->id as safe to use. However, the reg->id > is not set for those new _OR_NULL return types. One of the ways > that may be wrong is, checking NULL for one btf_id typed pointer will > end up validating all other btf_id typed pointers because > all of them have id == 0. The later tests will exercise > this path. > > To fix it and also avoid similar issue in the future, this patch > moves the id generation logic out of each individual RET type > test in check_helper_call(). Instead, it does one > reg_type_may_be_null() test and then do the id generation > if needed. > > This patch also adds a WARN_ON_ONCE in mark_ptr_or_null_reg() > to catch future breakage. > > The _OR_NULL pointer usage in the bpf_iter_reg.ctx_arg_info is > fine because it just happens that the existing id generation after > check_ctx_access() has covered it. It is also using the > reg_type_may_be_null() to decide if id generation is needed or not. > > Fixes: af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper") > Fixes: eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()") > Cc: Yonghong Song <yhs@fb.com> > Cc: Hao Luo <haoluo@google.com> > Signed-off-by: Martin KaFai Lau <kafai@fb.com> Good catch. The fix makes sense to me. Applied.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 39d7f44e7c92..6200519582a6 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5133,24 +5133,19 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn regs[BPF_REG_0].id = ++env->id_gen; } else { regs[BPF_REG_0].type = PTR_TO_MAP_VALUE_OR_NULL; - regs[BPF_REG_0].id = ++env->id_gen; } } else if (fn->ret_type == RET_PTR_TO_SOCKET_OR_NULL) { mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_SOCKET_OR_NULL; - regs[BPF_REG_0].id = ++env->id_gen; } else if (fn->ret_type == RET_PTR_TO_SOCK_COMMON_OR_NULL) { mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_SOCK_COMMON_OR_NULL; - regs[BPF_REG_0].id = ++env->id_gen; } else if (fn->ret_type == RET_PTR_TO_TCP_SOCK_OR_NULL) { mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_TCP_SOCK_OR_NULL; - regs[BPF_REG_0].id = ++env->id_gen; } else if (fn->ret_type == RET_PTR_TO_ALLOC_MEM_OR_NULL) { mark_reg_known_zero(env, regs, BPF_REG_0); regs[BPF_REG_0].type = PTR_TO_MEM_OR_NULL; - regs[BPF_REG_0].id = ++env->id_gen; regs[BPF_REG_0].mem_size = meta.mem_size; } else if (fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL || fn->ret_type == RET_PTR_TO_MEM_OR_BTF_ID) { @@ -5199,6 +5194,9 @@ static int check_helper_call(struct bpf_verifier_env *env, int func_id, int insn return -EINVAL; } + if (reg_type_may_be_null(regs[BPF_REG_0].type)) + regs[BPF_REG_0].id = ++env->id_gen; + if (is_ptr_cast_function(func_id)) { /* For release_reference() */ regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; @@ -7212,7 +7210,8 @@ static void mark_ptr_or_null_reg(struct bpf_func_state *state, struct bpf_reg_state *reg, u32 id, bool is_null) { - if (reg_type_may_be_null(reg->type) && reg->id == id) { + if (reg_type_may_be_null(reg->type) && reg->id == id && + !WARN_ON_ONCE(!reg->id)) { /* 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.
The commit af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper") introduces RET_PTR_TO_BTF_ID_OR_NULL and the commit eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()") introduces RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL. Note that for RET_PTR_TO_MEM_OR_BTF_ID_OR_NULL, the reg0->type could become PTR_TO_MEM_OR_NULL which is not covered by BPF_PROBE_MEM. The BPF_REG_0 will then hold a _OR_NULL pointer type. This _OR_NULL pointer type requires the bpf program to explicitly do a NULL check first. After NULL check, the verifier will mark all registers having the same reg->id as safe to use. However, the reg->id is not set for those new _OR_NULL return types. One of the ways that may be wrong is, checking NULL for one btf_id typed pointer will end up validating all other btf_id typed pointers because all of them have id == 0. The later tests will exercise this path. To fix it and also avoid similar issue in the future, this patch moves the id generation logic out of each individual RET type test in check_helper_call(). Instead, it does one reg_type_may_be_null() test and then do the id generation if needed. This patch also adds a WARN_ON_ONCE in mark_ptr_or_null_reg() to catch future breakage. The _OR_NULL pointer usage in the bpf_iter_reg.ctx_arg_info is fine because it just happens that the existing id generation after check_ctx_access() has covered it. It is also using the reg_type_may_be_null() to decide if id generation is needed or not. Fixes: af7ec1383361 ("bpf: Add bpf_skc_to_tcp6_sock() helper") Fixes: eaa6bcb71ef6 ("bpf: Introduce bpf_per_cpu_ptr()") Cc: Yonghong Song <yhs@fb.com> Cc: Hao Luo <haoluo@google.com> Signed-off-by: Martin KaFai Lau <kafai@fb.com> --- kernel/bpf/verifier.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)