On Mon, 21 Aug 2023 at 19:52, Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > > > > On 8/21/23 1:36 AM, Kumar Kartikeya Dwivedi wrote: > > On Mon, 21 Aug 2023 at 05:33, Yonghong Song <yonghong.song@xxxxxxxxx> wrote: > >> > >> When reviewing local percpu kptr support, Alexei discovered a bug > >> wherea bpf_kptr_xchg() may succeed even if the map value kptr type and > >> locally allocated obj type do not match ([1]). Missed struct btf_id > >> comparison is the reason for the bug. This patch added such struct btf_id > >> comparison and will flag verification failure if types do not match. > >> > >> [1] https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t > >> > >> Reported-by: Alexei Starovoitov <ast@xxxxxxxxxx> > >> Fixes: 738c96d5e2e3 ("bpf: Allow local kptrs to be exchanged via bpf_kptr_xchg") > >> Signed-off-by: Yonghong Song <yonghong.song@xxxxxxxxx> > >> --- > > > > Acked-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > > > > But some comments below... > > > >> kernel/bpf/verifier.c | 13 ++++++++++++- > >> 1 file changed, 12 insertions(+), 1 deletion(-) > >> > >> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > >> index 02a021c524ab..4e1ecd4b8497 100644 > >> --- a/kernel/bpf/verifier.c > >> +++ b/kernel/bpf/verifier.c > >> @@ -7745,7 +7745,18 @@ static int check_reg_type(struct bpf_verifier_env *env, u32 regno, > >> verbose(env, "verifier internal error: unimplemented handling of MEM_ALLOC\n"); > >> return -EFAULT; > >> } > >> - /* Handled by helper specific checks */ > >> + if (meta->func_id == BPF_FUNC_kptr_xchg) { > >> + struct btf_field *kptr_field = meta->kptr_field; > >> + > >> + if (!btf_struct_ids_match(&env->log, reg->btf, reg->btf_id, reg->off, > >> + kptr_field->kptr.btf, kptr_field->kptr.btf_id, > >> + true)) { > >> + verbose(env, "R%d is of type %s but %s is expected\n", > >> + regno, btf_type_name(reg->btf, reg->btf_id), > >> + btf_type_name(kptr_field->kptr.btf, kptr_field->kptr.btf_id)); > >> + return -EACCES; > >> + } > >> + } > > > > The fix on its own looks ok to me, but any reason you'd not like to > > delegate to map_kptr_match_type? > > Just to collect kptr related type matching logic in its own place. It > > doesn't matter too much though. > > From comments from Alexei in > > https://lore.kernel.org/bpf/20230819002907.io3iphmnuk43xblu@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/#t > > ===== > The map_kptr_match_type() should have been used for kptrs pointing to > kernel objects only. > But you're calling it for MEM_ALLOC object with prog's BTF... > ===== > > So looks like map_kptr_match_type() is for kptrs pointing to > kernel objects only. So that is why I didn't use it. > That function was added by me. Back then I added this check as we were discussing possibly supporting such local kptr and more thought would be needed about the design before just doing type matching. Also it was using kernel_type_name which was later renamed as btf_type_name, so as a precaution I added the btf_is_kernel check. Apart from that I remember no other reason, so I think it should be ok to drop it now and use it. But as I said, it is up to you, it will in effect do the same thing as this patch, so it is ok as-is. > > > > While looking at the code, I noticed one more problem. > > > > I don't think the current code is enforcing that 'reg->off is zero' > > assumption when releasing MEM_ALLOC types. We are only saved because > > you passed strict=true which makes passing non-zero reg->off a noop > > and returns false. > > The hunk was added to check_func_arg_reg_off in > > 6a3cd3318ff6 ("bpf: Migrate release_on_unlock logic to non-owning ref > > semantics") > > which bypasses in case type is MEM_ALLOC and offset points to some > > graph root or node. > > > > I am not sure why this check exists, IIUC rbtree release helpers are > > not tagged KF_RELEASE, and no release helper can type match on them > > either. Dave, can you confirm? I think it might be an accidental > > leftover of some refactoring. > > I am sure that Dave will look into this problem. I will take > a look as well to be sure my local percpu kptr won't have > issues with what you just discovered. Thanks! > It should be a problem for anything that has the MEM_ALLOC flag set, including percpu_kptr.
