Re: [PATCH] libbpf: check for empty BTF data section in btf_parse_elf

["Ihor Solodrai" <ihor.solodrai@linux.dev>]

On 4/9/25 4:14 PM, Andrii Nakryiko wrote:
> On Tue, Apr 8, 2025 at 11:41 AM Ihor Solodrai <ihor.solodrai@linux.dev> wrote:
>>
>> A valid ELF file may contain a SHT_NOBITS .BTF section. This case is
>> not handled correctly in btf_parse_elf, which leads to a segfault.
>>
>> Add a null check for a buffer returned by elf_getdata() before
>> proceeding with its processing.
>>
>> Bug report: https://github.com/libbpf/libbpf/issues/894
>>
>> Signed-off-by: Ihor Solodrai <ihor.solodrai@linux.dev>
>> ---
>>  tools/lib/bpf/btf.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
>> index 38bc6b14b066..90599f0311bd 100644
>> --- a/tools/lib/bpf/btf.c
>> +++ b/tools/lib/bpf/btf.c
>> @@ -1201,6 +1201,12 @@ static struct btf *btf_parse_elf(const char *path, struct btf *base_btf,
>>                 goto done;
>>         }
>>
>> +       if (!secs.btf_data->d_buf) {
>> +               pr_warn("BTF data is empty in %s\n", path);
>> +               err = -ENODATA;
>> +               goto done;
>> +       }
>> +
>
> let's handle this more generally for all BTF data sections in
> btf_find_elf_sections()?

Sure. I think it makes sense to check for the section type before
attempting to load a buffer like this:

@@ -1148,6 +1148,12 @@ static int btf_find_elf_sections(Elf *elf, const char *path, struct btf_elf_secs
                else
                        continue;
 
+               if (sh.sh_type == SHT_NOBITS) {
+                       pr_warn("failed to get section(%d, %s) data from %s\n",
+                               idx, name, path);
+                       goto err;
+               }
+

But then we might as well test for the expected section type, which is
supposed to be SHT_PROGBITS, if I understand correctly.

What I don't know is whether this is *the only* possible expected type
(for ".BTF", ".BTF.ext" and ".BTF.base"), or are there others?

Andrii, do you know if that's the case?

>
> let's also use similar style of warning messaging to others, maybe
> something like
>
> "failed to get section(%s, %d) data from %s\n" ?
>
>
> pw-bot: cr
>
>>         if (secs.btf_base_data) {
>>                 dist_base_btf = btf_new(secs.btf_base_data->d_buf, secs.btf_base_data->d_size,
>>                                         NULL);
>> --
>> 2.49.0
>>
>>