| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 17 Jun 2024 13:51:58 -0700
From: Andrii Nakryiko <andrii.nakryiko@...il.com>
To: Alan Maguire <alan.maguire@...cle.com>
Cc: Donglin Peng <dolinux.peng@...il.com>, ast@...nel.org, daniel@...earbox.net,
song@...nel.org, andrii@...nel.org, eddyz87@...il.com, haoluo@...gle.com,
yonghong.song@...ux.dev, bpf@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] libbpf: checking the btf_type kind when fixing variable offsets
On Mon, Jun 17, 2024 at 4:30 AM Alan Maguire <alan.maguire@...cle.com> wrote:
>
> On 16/06/2024 01:29, Donglin Peng wrote:
> > I encountered an issue when building the test_progs using the repository[1]:
> >
> > $ clang --version
> > Ubuntu clang version 17.0.6 (++20231208085846+6009708b4367-1~exp1~20231208085949.74)
> > Target: x86_64-pc-linux-gnu
> > Thread model: posix
> > InstalledDir: /usr/bin
> >
> > $ pwd
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/
> >
> > $ make test_progs V=1
> > ...
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/tools/sbin/bpftool
> > gen object
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked2.o
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o
> > libbpf: failed to find symbol for variable 'bpf_dynptr_slice' in section
> > '.ksyms'
> > Error: failed to link
> > '/work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.bpf.linked1.o':
> > No such file or directory (2)
> > make: *** [Makefile:656:
> > /work/Qemu/x86_64/linux-6.10-rc2/tools/testing/selftests/bpf/ip_check_defrag.skel.h]
> > Error 254
> >
> > After investigation, I found that the btf_types in the '.ksyms' section have a kind of
> > BTF_KIND_FUNC instead of BTF_KIND_VAR:
> >
> > $ bpftool btf dump file ./ip_check_defrag.bpf.linked1.o
> > ...
> > [2] DATASEC '.ksyms' size=0 vlen=2
> > type_id=16 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
> > type_id=17 offset=0 size=0 (FUNC 'bpf_dynptr_slice')
> > ...
> > [16] FUNC 'bpf_dynptr_from_skb' type_id=82 linkage=extern
> > [17] FUNC 'bpf_dynptr_slice' type_id=85 linkage=extern
> > ...
> >
> > To fix this, we can a add check for the kind.
> >
> > [1] https://github.com/eddyz87/bpf/tree/binsort-btf-dedup
> > Link: https://lore.kernel.org/all/4f551dc5fc792936ca364ce8324c0adea38162f1.camel@gmail.com/
> >
>
> The fix makes sense; what I was trying to figure out is why we're only
> seeing this now with the above repo.
>
> So as I understand it, the reason the kfuncs end up in the .ksyms
> section is due to the "__weak __ksym" tagging recently added to
> vmlinux.h construction from BTF via
>
> 770abbb5a25a ("bpftool: Support dumping kfunc prototypes from BTF")
>
> We see as noted
>
> [112] DATASEC '.ksyms' size=0 vlen=2
> type_id=84 offset=0 size=0 (FUNC 'bpf_dynptr_from_skb')
> type_id=90 offset=0 size=0 (FUNC 'bpf_dynptr_slice')
>
> So that makes sense, but prior to the above series, we also tagged
> kfuncs in this way before via bpf_kfuncs.h. So there should be no
> difference there.
>
> And with an upstream kernel I don't run into this problem.
>
> The only thing I could come up with is we were usually lucky; when we
> misinterpreted the func as a var and looked its type up, we got
>
> int var_linkage = btf_var(vt)->linkage;
>
> ...and were lucky it never equalled 1 (BTF_VAR_GLOBAL_ALLOCATED):
>
> /* no need to patch up static or extern vars */
> if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
> continue;
>
> In the case of a function, the above btf_var(vt) would really be
> pointing at the struct btf_type immediately after the relevant
> function's struct btf_type (since unlike variables, functions don't have
> metadata following them). So the only way we'd trip this bug would be if
> the struct btf_type following the func was had a name_off value that
> happened to equal 1 (BTF_VAR_GLOBAL_ALLOCATED).
>
> So maybe the sorting changes to BTF order resulted in us tripping on
> this bug, but regardless the fix seems right to me.
>
> > Fixes: 8fd27bf69b86 ("libbpf: Add BPF static linker BTF and BTF.ext support")
> > Signed-off-by: Donglin Peng <dolinux.peng@...il.com>
>
> A few small things below, but
>
> Reviewed-by: Alan Maguire <alan.maguire@...cle.com>
>
> > ---
> > tools/lib/bpf/linker.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/lib/bpf/linker.c b/tools/lib/bpf/linker.c
> > index 0d4be829551b..7f5fc9ac4ad6 100644
> > --- a/tools/lib/bpf/linker.c
> > +++ b/tools/lib/bpf/linker.c
> > @@ -2213,10 +2213,17 @@ static int linker_fixup_btf(struct src_obj *obj)
> > vi = btf_var_secinfos(t);
> > for (j = 0, m = btf_vlen(t); j < m; j++, vi++) {
> > const struct btf_type *vt = btf__type_by_id(obj->btf, vi->type);
> > - const char *var_name = btf__str_by_offset(obj->btf, vt->name_off);
> > - int var_linkage = btf_var(vt)->linkage;
> > + const char *var_name;
> > + int var_linkage;
> > Elf64_Sym *sym;
> >
> > + /* should be a variable */
> > + if (btf_kind(vt) != BTF_KIND_VAR)
>
> nit: could use if (!btf_is_var(vt)) here instead. It might also be worth
> reworking the comment to acknowledge that we can legitimately have a
> function in this section; i.e. something like
>
> /* could be a variable or function */
>
> We handle the func case elsewhere in libbpf (see add_dummy_ksym_var()).
Let's go a half-step further and validate that we see either VAR or
FUNC and for anything unexpected error out loudly, so this is brought
to someone's attention sooner?
Something like:
if (btf_is_func(vt))
continue; /* FUNC doesn't need adjustment */
if (!btf_is_var(vt)) {
pr_warn(<helpful and descriptive enough message about unexpected type>);
return -EINVAL;
}
(use btf_kind_str() for error string, btw)
pw-bot: cr
>
>
> > + continue;
> > +
> > + var_name = btf__str_by_offset(obj->btf, vt->name_off);
> > + var_linkage = btf_var(vt)->linkage;
> > +
> > /* no need to patch up static or extern vars */
> > if (var_linkage != BTF_VAR_GLOBAL_ALLOCATED)
> > continue;
Powered by blists - more mailing lists