netdev - Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YMopCb5CqOYsl6HR@krava>
Date:   Wed, 16 Jun 2021 18:38:33 +0200
From:   Jiri Olsa <jolsa@...hat.com>
To:     Yonghong Song <yhs@...com>
Cc:     Tony Ambardar <tony.ambardar@...il.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>, bpf@...r.kernel.org,
        netdev@...r.kernel.org, stable@...r.kernel.org,
        Jiri Olsa <jolsa@...nel.org>, Frank Eigler <fche@...hat.com>,
        Mark Wielaard <mjw@...hat.com>
Subject: Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids

On Wed, Jun 16, 2021 at 08:56:42AM -0700, Yonghong Song wrote:
> 
> 
> On 6/16/21 2:25 AM, Tony Ambardar wrote:
> > While patching the .BTF_ids section in vmlinux, resolve_btfids writes type
> > ids using host-native endianness, and relies on libelf for any required
> > translation when finally updating vmlinux. However, the default type of the
> > .BTF_ids section content is ELF_T_BYTE (i.e. unsigned char), and undergoes
> > no translation. This results in incorrect patched values if cross-compiling
> > to non-native endianness, and can manifest as kernel Oops and test failures
> > which are difficult to debug.

nice catch, great libelf can do that ;-)

> > 
> > Explicitly set the type of patched data to ELF_T_WORD, allowing libelf to
> > transparently handle the endian conversions.
> > 
> > Fixes: fbbb68de80a4 ("bpf: Add resolve_btfids tool to resolve BTF IDs in ELF object")
> > Cc: stable@...r.kernel.org # v5.10+
> > Cc: Jiri Olsa <jolsa@...nel.org>
> > Cc: Yonghong Song <yhs@...com>
> > Link: https://lore.kernel.org/bpf/CAPGftE_eY-Zdi3wBcgDfkz_iOr1KF10n=9mJHm1_a_PykcsoeA@mail.gmail.com/
> > Signed-off-by: Tony Ambardar <Tony.Ambardar@...il.com>
> > ---
> >   tools/bpf/resolve_btfids/main.c | 3 +++
> >   1 file changed, 3 insertions(+)
> > 
> > diff --git a/tools/bpf/resolve_btfids/main.c b/tools/bpf/resolve_btfids/main.c
> > index d636643ddd35..f32c059fbfb4 100644
> > --- a/tools/bpf/resolve_btfids/main.c
> > +++ b/tools/bpf/resolve_btfids/main.c
> > @@ -649,6 +649,9 @@ static int symbols_patch(struct object *obj)
> >   	if (sets_patch(obj))
> >   		return -1;
> > +	/* Set type to ensure endian translation occurs. */
> > +	obj->efile.idlist->d_type = ELF_T_WORD;
> 
> The change makes sense to me as .BTF_ids contains just a list of
> u32's.
> 
> Jiri, could you double check on this?

the comment in ELF_T_WORD declaration suggests the size depends on
elf's class?

  ELF_T_WORD,                   /* Elf32_Word, Elf64_Word, ... */

data in .BTF_ids section are allways u32

I have no idea how is this handled in libelf (perhaps it's ok),
but just that comment above suggests it could be also 64 bits,
cc-ing Frank and Mark for more insight

thanks,
jirka

> 
> > +
> >   	elf_flagdata(obj->efile.idlist, ELF_C_SET, ELF_F_DIRTY);
> >   	err = elf_update(obj->efile.elf, ELF_C_WRITE);
> > 
>