Re: [PATCH bpf-next 01/13] bpf: Loosen alloc obj test in verifier's reg_btf_record

From: Kumar Kartikeya Dwivedi <memxor@gmail.com>
To: Dave Marchevsky <davemarchevsky@fb.com>
Cc: bpf@vger.kernel.org, Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Andrii Nakryiko <andrii@kernel.org>,
	Kernel Team <kernel-team@fb.com>, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH bpf-next 01/13] bpf: Loosen alloc obj test in verifier's reg_btf_record
Date: Wed, 7 Dec 2022 22:11:21 +0530	[thread overview]
Message-ID: <20221207164121.h6wm5crfhhvekqvd@apollo> (raw)
In-Reply-To: <20221206231000.3180914-2-davemarchevsky@fb.com>

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@mail.gmail.com

> 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@fb.com>
> cc: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> 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
>