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.
--
Kartikeya
