Re: Recent-ish changes in binfmt_elf made my program segfault

[Linux regression tracking (Thorsten Leemhuis)]

Hi, Thorsten here, the Linux kernel's regression tracker. Top-posting
for once, to make this easily accessible to everyone.

Eric, what's the status wrt. to this regression? Things from here look
stalled, but I might be missing something.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
If I did something stupid, please tell me, as explained on that page.

#regzbot poke

On 22.01.24 23:12, Kees Cook wrote:
> On Mon, Jan 22, 2024 at 03:01:06PM -0600, Eric W. Biederman wrote:
>> Kees Cook <keescook@xxxxxxxxxxxx> writes:
>>
>>> On Mon, Jan 22, 2024 at 10:43:59AM -0600, Eric W. Biederman wrote:
>>>> Jan Bujak <j@xxxxxxx> writes:
>>>>
>>>>> Hi.
>>>>>
>>>>> I recently updated my kernel and one of my programs started segfaulting.
>>>>>
>>>>> The issue seems to be related to how the kernel interprets PT_LOAD headers;
>>>>> consider the following program headers (from 'readelf' of my reproduction):
>>>>>
>>>>> Program Headers:
>>>>>   Type  Offset   VirtAddr  PhysAddr  FileSiz  MemSiz   Flg Align
>>>>>   LOAD  0x001000 0x10000   0x10000   0x000010 0x000010 R   0x1000
>>>>>   LOAD  0x002000 0x11000   0x11000   0x000010 0x000010 RW  0x1000
>>>>>   LOAD  0x002010 0x11010   0x11010   0x000000 0x000004 RW  0x1000
>>>>>   LOAD  0x003000 0x12000   0x12000   0x0000d2 0x0000d2 R E 0x1000
>>>>>   LOAD  0x004000 0x20000   0x20000   0x000004 0x000004 RW  0x1000
>>>>>
>>>>> Old kernels load this ELF file in the following way ('/proc/self/maps'):
>>>>>
>>>>> 00010000-00011000 r--p 00001000 00:02 131  ./bug-reproduction
>>>>> 00011000-00012000 rw-p 00002000 00:02 131  ./bug-reproduction
>>>>> 00012000-00013000 r-xp 00003000 00:02 131  ./bug-reproduction
>>>>> 00020000-00021000 rw-p 00004000 00:02 131  ./bug-reproduction
>>>>>
>>>>> And new kernels do it like this:
>>>>>
>>>>> 00010000-00011000 r--p 00001000 00:02 131  ./bug-reproduction
>>>>> 00011000-00012000 rw-p 00000000 00:00 0
>>>>> 00012000-00013000 r-xp 00003000 00:02 131  ./bug-reproduction
>>>>> 00020000-00021000 rw-p 00004000 00:02 131  ./bug-reproduction
>>>>>
>>>>> That map between 0x11000 and 0x12000 is the program's '.data' and '.bss'
>>>>> sections to which it tries to write to, and since the kernel doesn't map
>>>>> them anymore it crashes.
>>>>>
>>>>> I bisected the issue to the following commit:
>>>>>
>>>>> commit 585a018627b4d7ed37387211f667916840b5c5ea
>>>>> Author: Eric W. Biederman <ebiederm@xxxxxxxxxxxx>
>>>>> Date:   Thu Sep 28 20:24:29 2023 -0700
>>>>>
>>>>>     binfmt_elf: Support segments with 0 filesz and misaligned starts
>>>>>
>>>>> I can confirm that with this commit the issue reproduces, and with it
>>>>> reverted it doesn't.
>>>>>
>>>>> I have prepared a minimal reproduction of the problem available here,
>>>>> along with all of the scripts I used for bisecting:
>>>>>
>>>>> https://github.com/koute/linux-elf-loading-bug
>>>>>
>>>>> You can either compile it from source (requires Rust and LLD), or there's
>>>>> a prebuilt binary in 'bin/bug-reproduction` which you can run. (It's tiny,
>>>>> so you can easily check with 'objdump -d' that it isn't malicious).
>>>>>
>>>>> On old kernels this will run fine, and on new kernels it will
>>>>> segfault.
>>>>
>>>> Frankly your ELF binary is buggy, and probably the best fix would be to
>>>> fix the linker script that is used to generate your binary.
>>>>
>>>> The problem is the SYSV ABI defines everything in terms of pages and so
>>>> placing two ELF segments on the same page results in undefined behavior.
>>>>
>>>> The code was fixed to honor your .bss segment and now your .data segment
>>>> is being stomped, because you defined them to overlap.
>>>>
>>>> Ideally your linker script would place both your .data and .bss in
>>>> the same segment.  That would both fix the issue and give you a more
>>>> compact elf binary, while not changing the generated code at all.
>>>>
>>>>
>>>> That said regressions suck and it would be good if we could update the
>>>> code to do something reasonable in this case.
>>>>
>>>> We can perhaps we can update the .bss segment to just memset an existing
>>>> page if one has already been mapped.  Which would cleanly handle a case
>>>> like yours.  I need to think about that for a moment to see what the
>>>> code would look like to do that.
>>>
>>> It's the "if one has already been mapped" part which might
>>> become expensive...
>>
>> I am wondering if perhaps we can add MAP_FIXED_NOREPLACE and take
>> some appropriate action if there is already a mapping there.
> 
> Yeah, in the general case we had to back out MAP_FIXED_NOREPLACE usage
> for individual LOADs because there were so many cases of overlapping
> LOADs. :( Currently it's only used during the initial mapping (when
> "total_size" is set), to avoid colliding with the stack.
> 
> But, as you suggest, if we only use it for filesz==0, it could work.
> 
>> Such as printing a warning and skipping the action entirely for
>> a pure bss segment.  That would essentially replicate the previous
>> behavior.
> 
> Instead of failing, perhaps we just fallback to not using
> MAP_FIXED_NOREPLACE and do the memset? (And maybe pr_warn_once?)
> 
>> At a minimum adding MAP_FIXED_NOREPLACE should allow us to
>> deterministically detect and warn about problems, making it easier
>> for people to understand why their binary won't run.
> 
> Yeah, it seems like it's the vm_brk_flags() that is clobber the mapping,
> so we have to skip that for the MAP_FIXED_NOREPLACE fails on a filesz==0
> case?
>