Message-ID: <87v87laxrh.fsf@email.froward.int.ebiederm.org>
Date: Mon, 22 Jan 2024 15:01:06 -0600
From: "Eric W. Biederman" <ebiederm@...ssion.com>
To: Kees Cook <keescook@...omium.org>
Cc: Jan Bujak <j@...a.io>,  linux-mm@...ck.org,
  linux-kernel@...r.kernel.org,  viro@...iv.linux.org.uk,
  brauner@...nel.org,  linux-fsdevel@...r.kernel.org
Subject: Re: Recent-ish changes in binfmt_elf made my program segfault

Kees Cook <keescook@...omium.org> writes:

> On Mon, Jan 22, 2024 at 10:43:59AM -0600, Eric W. Biederman wrote:
>> Jan Bujak <j@...a.io> 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@...ssion.com>
>> > 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.

Such as printing a warning and skipping the action entirely for
a pure bss segment.  That would essentially replicate the previous
behavior.

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.

Eric

Powered by blists - more mailing lists