On Wed, Feb 16, 2022 at 9:33 AM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
>
> On Wed, Feb 16, 2022 at 09:15:47PM IST, Alexei Starovoitov wrote:
> > On Wed, Feb 16, 2022 at 1:13 AM Kumar Kartikeya Dwivedi
> > <memxor@xxxxxxxxx> wrote:
> > >
> > > When commit e6ac2450d6de ("bpf: Support bpf program calling kernel
> > > function") added kfunc support, it defined reg2btf_ids as a cheap way to
> > > translate the verifier reg type to the appropriate btf_vmlinux BTF ID,
> > > however commit c25b2ae13603 ("bpf: Replace PTR_TO_XXX_OR_NULL with
> > > PTR_TO_XXX | PTR_MAYBE_NULL") moved the __BPF_REG_TYPE_MAX from the last
> > > member of bpf_reg_type enum to after the base register types, and
> > > defined other variants using type flag composition. However, now, the
> > > direct usage of reg->type to index into reg2btf_ids may no longer fall
> > > into __BPF_REG_TYPE_MAX range, and hence lead to out of bounds access
> > > and kernel crash on dereference of bad pointer.
> > ...
> > > [ 20.488393] RDX: 0000000000000000 RSI: 0000000000000004 RDI: 0000000000524156
> > > [ 20.489045] RBP: ffffffffa398c6d4 R08: ffffffffa14dd991 R09: 0000000000000000
> > > [ 20.489696] R10: fffffbfff484f31c R11: 0000000000000001 R12: ffff888007bf8600
> > > [ 20.490377] R13: ffff88800c2f6078 R14: 0000000000000000 R15: 0000000000000001
> > > [ 20.491065] FS: 00007fe06ae70740(0000) GS:ffff88808cc00000(0000) knlGS:0000000000000000
> > > [ 20.491782] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [ 20.492272] CR2: 0000000000524156 CR3: 000000000902a004 CR4: 0000000000770ef0
> > > [ 20.492925] PKRU: 55555554
> >
> > Please do not include a full kernel dump in the commit log.
> > It provides no value.
> > The first paragraph was enough.
> >
>
> Ok, won't include next time.
>
> > > Cc: Martin KaFai Lau <kafai@xxxxxx>
> > > Cc: Hao Luo <haoluo@xxxxxxxxxx>
> > > Fixes: c25b2ae13603 ("bpf: Replace PTR_TO_XXX_OR_NULL with PTR_TO_XXX | PTR_MAYBE_NULL")
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@xxxxxxxxx>
> > > ---
> > > kernel/bpf/btf.c | 22 +++++++++++++++-------
> > > 1 file changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
> > > index e16dafeb2450..416345798e0a 100644
> > > --- a/kernel/bpf/btf.c
> > > +++ b/kernel/bpf/btf.c
> > > @@ -5568,13 +5568,21 @@ int btf_check_type_match(struct bpf_verifier_log *log, const struct bpf_prog *pr
> > > return btf_check_func_type_match(log, btf1, t1, btf2, t2);
> > > }
> > >
> > > -static u32 *reg2btf_ids[__BPF_REG_TYPE_MAX] = {
> > > +static u32 *reg2btf_ids(enum bpf_reg_type type)
> > > +{
> > > + switch (type) {
> > > #ifdef CONFIG_NET
> > > - [PTR_TO_SOCKET] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK],
> > > - [PTR_TO_SOCK_COMMON] = &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON],
> > > - [PTR_TO_TCP_SOCK] = &btf_sock_ids[BTF_SOCK_TYPE_TCP],
> > > + case PTR_TO_SOCKET:
> > > + return &btf_sock_ids[BTF_SOCK_TYPE_SOCK];
> > > + case PTR_TO_SOCK_COMMON:
> > > + return &btf_sock_ids[BTF_SOCK_TYPE_SOCK_COMMON];
> > > + case PTR_TO_TCP_SOCK:
> > > + return &btf_sock_ids[BTF_SOCK_TYPE_TCP];
> > > #endif
> > > -};
> > > + default:
> > > + return NULL;
> > > + }
> > > +}
> > >
> > > /* Returns true if struct is composed of scalars, 4 levels of nesting allowed */
> > > static bool __btf_type_is_scalar_struct(struct bpf_verifier_log *log,
> > > @@ -5688,7 +5696,7 @@ static int btf_check_func_arg_match(struct bpf_verifier_env *env,
> > > }
> > > if (check_ptr_off_reg(env, reg, regno))
> > > return -EINVAL;
> > > - } else if (is_kfunc && (reg->type == PTR_TO_BTF_ID || reg2btf_ids[reg->type])) {
> > > + } else if (is_kfunc && (reg->type == PTR_TO_BTF_ID || reg2btf_ids(reg->type))) {
> >
> > Just use reg2btf_ids[base_type(reg->type)] instead?
>
> That would be incorrect I think, then we'd allow e.g. PTR_TO_TCP_SOCK_OR_NULL
> and treat it as PTR_TO_TCP_SOCK, while current code only wants to permit
> non-NULL variants for these three.
add && !type_flag(reg->type) ?
But, first, please describe how you found it.
Tried to pass PTR_TO_BTF_ID_OR_NULL into kfunc?
Do you have some other changes in the verifier?
