| Message ID | 20220304191657.981240-2-haoluo@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | bff61f6faedb36db6b135da898840d29aa74cbbb |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: add __percpu tagging in vmlinux BTF | expand |
On 3/4/22 11:16 AM, Hao Luo wrote: > With the introduction of MEM_USER in > > commit c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way") > > PTR_TO_BTF_ID can be combined with a MEM_USER tag. Therefore, most > likely, when we compare reg_type against PTR_TO_BTF_ID, we want to use > the reg's base_type. Previously the check in check_mem_access() wants > to say: if the reg is BTF_ID but not NULL, the execution flow falls > into the 'then' branch. But now a reg of (BTF_ID | MEM_USER), which > should go into the 'then' branch, goes into the 'else'. > > The end results before and after this patch are the same: regs tagged > with MEM_USER get rejected, but not in a way we intended. So fix the > condition, the error message now is correct. > > Before (log from commit 696c39011538): > > $ ./test_progs -v -n 22/3 > ... > libbpf: prog 'test_user1': BPF program load failed: Permission denied > libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG -- > R1 type=ctx expected=fp > 0: R1=ctx(id=0,off=0,imm=0) R10=fp0 > ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 136561 type STRUCT 'bpf_testmod_btf_type_tag_1' > 1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,off=0,imm=0) > ; g = arg->a; > 1: (61) r1 = *(u32 *)(r1 +0) > R1 invalid mem access 'user_ptr_' > > Now: > > libbpf: prog 'test_user1': BPF program load failed: Permission denied > libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG -- > R1 type=ctx expected=fp > 0: R1=ctx(id=0,off=0,imm=0) R10=fp0 > ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg) > 0: (79) r1 = *(u64 *)(r1 +0) > func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 104036 type STRUCT 'bpf_testmod_btf_type_tag_1' > 1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,ref_obj_id=0,off=0,imm=0) > ; g = arg->a; > 1: (61) r1 = *(u32 *)(r1 +0) > R1 is ptr_bpf_testmod_btf_type_tag_1 access user memory: off=0 > > Note the error message for the reason of rejection. > > Fixes: c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way") > Cc: Yonghong Song <yhs@fb.com> > Signed-off-by: Hao Luo <haoluo@google.com> Thanks for the fix! Acked-by: Yonghong Song <yhs@fb.com>
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index a57db4b2803c..d63b1f40e029 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -4556,7 +4556,8 @@ static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn err = check_tp_buffer_access(env, reg, regno, off, size); if (!err && t == BPF_READ && value_regno >= 0) mark_reg_unknown(env, regs, value_regno); - } else if (reg->type == PTR_TO_BTF_ID) { + } else if (base_type(reg->type) == PTR_TO_BTF_ID && + !type_may_be_null(reg->type)) { err = check_ptr_to_btf_access(env, regs, regno, off, size, t, value_regno); } else if (reg->type == CONST_PTR_TO_MAP) {
With the introduction of MEM_USER in commit c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way") PTR_TO_BTF_ID can be combined with a MEM_USER tag. Therefore, most likely, when we compare reg_type against PTR_TO_BTF_ID, we want to use the reg's base_type. Previously the check in check_mem_access() wants to say: if the reg is BTF_ID but not NULL, the execution flow falls into the 'then' branch. But now a reg of (BTF_ID | MEM_USER), which should go into the 'then' branch, goes into the 'else'. The end results before and after this patch are the same: regs tagged with MEM_USER get rejected, but not in a way we intended. So fix the condition, the error message now is correct. Before (log from commit 696c39011538): $ ./test_progs -v -n 22/3 ... libbpf: prog 'test_user1': BPF program load failed: Permission denied libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG -- R1 type=ctx expected=fp 0: R1=ctx(id=0,off=0,imm=0) R10=fp0 ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg) 0: (79) r1 = *(u64 *)(r1 +0) func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 136561 type STRUCT 'bpf_testmod_btf_type_tag_1' 1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,off=0,imm=0) ; g = arg->a; 1: (61) r1 = *(u32 *)(r1 +0) R1 invalid mem access 'user_ptr_' Now: libbpf: prog 'test_user1': BPF program load failed: Permission denied libbpf: prog 'test_user1': -- BEGIN PROG LOAD LOG -- R1 type=ctx expected=fp 0: R1=ctx(id=0,off=0,imm=0) R10=fp0 ; int BPF_PROG(test_user1, struct bpf_testmod_btf_type_tag_1 *arg) 0: (79) r1 = *(u64 *)(r1 +0) func 'bpf_testmod_test_btf_type_tag_user_1' arg0 has btf_id 104036 type STRUCT 'bpf_testmod_btf_type_tag_1' 1: R1_w=user_ptr_bpf_testmod_btf_type_tag_1(id=0,ref_obj_id=0,off=0,imm=0) ; g = arg->a; 1: (61) r1 = *(u32 *)(r1 +0) R1 is ptr_bpf_testmod_btf_type_tag_1 access user memory: off=0 Note the error message for the reason of rejection. Fixes: c6f1bfe89ac9 ("bpf: reject program if a __user tagged memory accessed in kernel way") Cc: Yonghong Song <yhs@fb.com> Signed-off-by: Hao Luo <haoluo@google.com> --- kernel/bpf/verifier.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)