On Wed, Dec 07, 2022 at 04:39:48AM IST, Dave Marchevsky wrote: > btf->struct_meta_tab is populated by btf_parse_struct_metas in btf.c. > There, a BTF record is created for any type containing a spin_lock or > any next-gen datastructure node/head. > > Currently, for non-MAP_VALUE types, reg_btf_record will only search for > a record using struct_meta_tab if the reg->type exactly matches > (PTR_TO_BTF_ID | MEM_ALLOC). This exact match is too strict: an > "allocated obj" type - returned from bpf_obj_new - might pick up other > flags while working its way through the program. > Not following. Only PTR_TO_BTF_ID | MEM_ALLOC is the valid reg->type that can be passed to helpers. reg_btf_record is used in helpers to inspect the btf_record. Any other flag combination (the only one possible is PTR_UNTRUSTED right now) cannot be passed to helpers in the first place. The reason to set PTR_UNTRUSTED is to make then unpassable to helpers. > Loosen the check to be exact for base_type and just use MEM_ALLOC mask > for type_flag. > > This patch is marked Fixes as the original intent of reg_btf_record was > unlikely to have been to fail finding btf_record for valid alloc obj > types with additional flags, some of which (e.g. PTR_UNTRUSTED) > are valid register type states for alloc obj independent of this series. That was the actual intent, same as how check_ptr_to_btf_access uses the exact reg->type to allow the BPF_WRITE case. I think this series is the one introducing this case, passing bpf_rbtree_first's result to bpf_rbtree_remove, which I think is not possible to make safe in the first place. We decided to do bpf_list_pop_front instead of bpf_list_entry -> bpf_list_del due to this exact issue. More in [0]. [0]: https://lore.kernel.org/bpf/CAADnVQKifhUk_HE+8qQ=AOhAssH6w9LZ082Oo53rwaS+tAGtOw@xxxxxxxxxxxxxx > However, I didn't find a specific broken repro case outside of this > series' added functionality, so it's possible that nothing was > triggering this logic error before. > > Signed-off-by: Dave Marchevsky <davemarchevsky@xxxxxx> > cc: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx> > Fixes: 4e814da0d599 ("bpf: Allow locking bpf_spin_lock in allocated objects") > --- > kernel/bpf/verifier.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index 1d51bd9596da..67a13110bc22 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -451,6 +451,11 @@ static bool reg_type_not_null(enum bpf_reg_type type) > type == PTR_TO_SOCK_COMMON; > } > > +static bool type_is_ptr_alloc_obj(u32 type) > +{ > + return base_type(type) == PTR_TO_BTF_ID && type_flag(type) & MEM_ALLOC; > +} > + > static struct btf_record *reg_btf_record(const struct bpf_reg_state *reg) > { > struct btf_record *rec = NULL; > @@ -458,7 +463,7 @@ static struct btf_record *reg_btf_record(const struct bpf_reg_state *reg) > > if (reg->type == PTR_TO_MAP_VALUE) { > rec = reg->map_ptr->record; > - } else if (reg->type == (PTR_TO_BTF_ID | MEM_ALLOC)) { > + } else if (type_is_ptr_alloc_obj(reg->type)) { > meta = btf_find_struct_meta(reg->btf, reg->btf_id); > if (meta) > rec = meta->record; > -- > 2.30.2 >
