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]

Date:   Thu, 17 Jun 2021 13:22:34 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jiri Olsa <jolsa@...hat.com>, Mark Wielaard <mark@...mp.org>
Cc:     Yonghong Song <yhs@...com>,
        Tony Ambardar <tony.ambardar@...il.com>,
        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>
Subject: Re: [PATCH bpf v1] bpf: fix libelf endian handling in resolv_btfids

On 6/17/21 11:02 AM, Jiri Olsa wrote:
> On Thu, Jun 17, 2021 at 12:28:00AM +0200, Mark Wielaard wrote:
>> On Wed, Jun 16, 2021 at 06:38:33PM +0200, Jiri Olsa wrote:
>>>>> 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
>>
>> It is correct to use ELF_T_WORD, which means a 32bit unsigned word.
>>
>> The comment is meant to explain that, but is really confusing if you
>> don't know that Elf32_Word and Elf64_Word are the same thing (a 32bit
>> unsigned word). This comes from being "too consistent" in defining all
>> data types for both 32bit and 64bit ELF, even if those types are the
>> same in both formats...
>>
>> Only Elf32_Addr/Elf64_Addr and Elf32_Off/Elf64_Off are different
>> sizes. But Elf32/Elf_64_Half (16 bit), Elf32/Elf64_Word (32 bit),
>> Elf32/Elf64_Xword (64 bit) and their Sword/Sxword (signed) variants
>> are all identical data types in both the Elf32 and Elf64 formats.
>>
>> I don't really know why. It seems the original ELF spec was 32bit only
>> and when introducing the ELF64 format "they" simply duplicated all
>> data types whether or not those data type were actually different
>> between the 32 and 64 bit format.
> 
> nice, thanks for details
> 
> Acked-by: Jiri Olsa <jolsa@...hat.com>

Tony, could you do a v2 and summarize the remainder of the discussion in
here for the commit message? Would be good to explicitly document the
assumptions made and why they work.

Thanks everyone,
Daniel