On Tue, 22 Aug 2023 at 10:45, David Marchevsky <david.marchevsky@xxxxxxxxx> wrote: > > On 8/21/23 4: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. > > > > 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 think you're correct, that's probably leftover from when > helpers were tagged KF_RELEASE in earlier rbtree impl revisions. > > I also think it's reasonable to delete the hunk as you've done > in attached patches. > Ok, then I'll submit it as a patch after Yonghong's patch is accepted (since both will conflict trying to add local_kptr_stash_fail test cases) and you can give your Acked-by on it.
