linux-kernel - Re: [PATCH bpf-next v3] bpftool: Fix gen object segfault

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: <tencent_2AC9A80622AF8180D05996E4E58A4AE6EB06@qq.com>
Date: Fri, 6 Dec 2024 10:22:35 +0800
From: Rong Tao <rtoax@...mail.com>
To: Quentin Monnet <qmo@...nel.org>,
 Andrii Nakryiko <andrii.nakryiko@...il.com>, alexei.starovoitov@...il.com
Cc: ast@...nel.org, daniel@...earbox.net, rongtao@...tc.cn,
 Andrii Nakryiko <andrii@...nel.org>, Martin KaFai Lau
 <martin.lau@...ux.dev>, Eduard Zingerman <eddyz87@...il.com>,
 Song Liu <song@...nel.org>, Yonghong Song <yonghong.song@...ux.dev>,
 John Fastabend <john.fastabend@...il.com>, KP Singh <kpsingh@...nel.org>,
 Stanislav Fomichev <sdf@...ichev.me>, Hao Luo <haoluo@...gle.com>,
 Jiri Olsa <jolsa@...nel.org>, bpf@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v3] bpftool: Fix gen object segfault


On 12/6/24 09:56, Quentin Monnet wrote:
> 2024-12-06 09:11 UTC+0800 ~ Rong Tao <rtoax@...mail.com>
>> On 12/6/24 05:34, Andrii Nakryiko wrote:
>>> On Thu, Dec 5, 2024 at 4:22 AM Quentin Monnet <qmo@...nel.org> wrote:
>>>> On 05/12/2024 12:09, Rong Tao wrote:
>>>>> From: Rong Tao <rongtao@...tc.cn>
>>>>>
>>>>> If the input file and output file are the same, the input file is
>>>>> cleared
>>>>> due to opening, resulting in a NULL pointer access by libbpf.
>>>>>
>>>>>       $ bpftool gen object prog.o prog.o
>>>>>       libbpf: failed to get ELF header for prog.o: invalid `Elf' handle
>>>>>       Segmentation fault
>>>>>
>>>>>       (gdb) bt
>>>>>       #0  0x0000000000450285 in linker_append_elf_syms
>>>>> (linker=0x4feda0, obj=0x7fffffffe100) at linker.c:1296
>>>>>       #1  bpf_linker__add_file (linker=0x4feda0, filename=<optimized
>>>>> out>, opts=<optimized out>) at linker.c:453
>>>>>       #2  0x000000000040c235 in do_object ()
>>>>>       #3  0x00000000004021d7 in main ()
>>>>>       (gdb) frame 0
>>>>>       #0  0x0000000000450285 in linker_append_elf_syms
>>>>> (linker=0x4feda0, obj=0x7fffffffe100) at linker.c:1296
>>>>>       1296              Elf64_Sym *sym = symtab->data->d_buf;
>>>>>
>>>>> Signed-off-by: Rong Tao <rongtao@...tc.cn>
>>>> Tested-by: Quentin Monnet <qmo@...nel.org>
>>>> Reviewed-by: Quentin Monnet <qmo@...nel.org>
>>> Isn't this papering over a deeper underlying issue? Why do we get
>>> SIGSEGV inside the linker at all instead of just erroring out?
>>> Comparison based on file path isn't a reliable way to check if input
>>> and output are both the same file, so this fixes the most obvious
>>> case, but not the actual issue.
>> Thanks for your replay! The current scenario is similar to the following
>> code.
>> After a.txt is opened in read mode, it is opened in write mode again, which
>> causes the contents of a.txt file to be cleared, resulting in no data
>> being read,
>>
>>
>>      fpr = fopen("a.txt", "r");
>>      fpw = fopen("a.txt", "w");
>>
>>      /* fgets() will get nothing, It's not glibc's fault. */
>>      while (fgets(buff, sizeof(buff), fpr))
>>          printf("%s", buff);
>>
>>      fprintf(fpw, "....");
>>
>>      fclose(fpr);
>>      fclose(fpw);
>>
>> corresponding to the SEGV of bpftool. Perhaps we can add the following
>> warning
>>
>>      if (x == NULL) {
>>          fprintf(stderr, "Maybe the file was opened for writing after
>> opened for read\n");
>>          return -EINVAL;
>>      }
>>
>> Whether this warning can be added may depend on libelf's processing. I will
>> try to fix this SEGV in libbpf, hopefully it can be fixed.
> Thank you Rong, I'm not sure I followed your explanation (the above is
> not bpftool code, is it?), but I think we just addressed the issue in
> libbpf with:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git/commit/?id=e10500b69c3f3378f3dcfc8c2fe4cdb74fc844f5
Thanks for this, i just try this patch, there is still one problem as 
follows:

    $ cd tools/bpf/bpftool
    $ make -j8
    $ ls -l prog.o
    -rw-r--r--. 1 rongtao rongtao 78408 Dec  6 10:18 prog.o
    $ ./bpftool gen object prog.o prog.o
    libbpf: failed to get ELF header for prog.o: invalid `Elf' handle
    Error: failed to link 'prog.o': Invalid argument (22)
    $ ls -l prog.o
    -rw-r--r--. 1 rongtao rongtao 0 Dec  6 10:18 prog.o

The input file is cleared (size=0), which is not what the user expected.

Thanks,
Rong Tao
>
> We can drop the patch with the check on the names (sorry!). As Andrii
> mentioned, it's not very reliable to compare filenames. It's true that
> users can truncate files if they pass the same input and output file,
> but then that's the case with many command-line tools if you don't use
> them properly.
>
> So, no action required. Feel free to test with the patch above, the
> segfault should not longer occur.
>
> Thanks,
> Quentin