Received-SPF: pass (google.com: domain of linux-kernel+bounces-34182-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: "Eric W. Biederman" <ebiederm@xmission.com>
To: Kees Cook <keescook@chromium.org>
Cc: Jan Bujak <j@exia.io>,  linux-mm@kvack.org,
  linux-kernel@vger.kernel.org,  viro@zeniv.linux.org.uk,
  brauner@kernel.org,  linux-fsdevel@vger.kernel.org
References: <c7209e19-89c4-446a-b364-83100e30cc00@exia.io>
	<874jf5co8g.fsf@email.froward.int.ebiederm.org>
	<202401221226.DAFA58B78@keescook>
Date: Mon, 22 Jan 2024 15:01:06 -0600
In-Reply-To: <202401221226.DAFA58B78@keescook> (Kees Cook's message of "Mon,
	22 Jan 2024 12:48:06 -0800")
Message-ID: <87v87laxrh.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Subject: Re: Recent-ish changes in binfmt_elf made my program segfault

Kees Cook <keescook@chromium.org> writes:

> On Mon, Jan 22, 2024 at 10:43:59AM -0600, Eric W. Biederman wrote:
>> Jan Bujak <j@exia.io> writes:
>>=20
>> > Hi.
>> >
>> > I recently updated my kernel and one of my programs started segfaultin=
g.
>> >
>> > The issue seems to be related to how the kernel interprets PT_LOAD hea=
ders;
>> > consider the following program headers (from 'readelf' of my reproduct=
ion):
>> >
>> > Program Headers:
>> > =C2=A0 Type=C2=A0 Offset=C2=A0=C2=A0 VirtAddr=C2=A0 PhysAddr=C2=A0 Fil=
eSiz=C2=A0 MemSiz=C2=A0=C2=A0 Flg Align
>> > =C2=A0 LOAD=C2=A0 0x001000 0x10000=C2=A0=C2=A0 0x10000=C2=A0=C2=A0 0x0=
00010 0x000010 R=C2=A0=C2=A0 0x1000
>> > =C2=A0 LOAD=C2=A0 0x002000 0x11000=C2=A0=C2=A0 0x11000=C2=A0=C2=A0 0x0=
00010 0x000010 RW=C2=A0 0x1000
>> > =C2=A0 LOAD=C2=A0 0x002010 0x11010=C2=A0=C2=A0 0x11010=C2=A0=C2=A0 0x0=
00000 0x000004 RW=C2=A0 0x1000
>> > =C2=A0 LOAD=C2=A0 0x003000 0x12000=C2=A0=C2=A0 0x12000=C2=A0=C2=A0 0x0=
000d2 0x0000d2 R E 0x1000
>> > =C2=A0 LOAD=C2=A0 0x004000 0x20000=C2=A0=C2=A0 0x20000=C2=A0=C2=A0 0x0=
00004 0x000004 RW=C2=A0 0x1000
>> >
>> > Old kernels load this ELF file in the following way ('/proc/self/maps'=
):
>> >
>> > 00010000-00011000 r--p 00001000 00:02 131=C2=A0 ./bug-reproduction
>> > 00011000-00012000 rw-p 00002000 00:02 131=C2=A0 ./bug-reproduction
>> > 00012000-00013000 r-xp 00003000 00:02 131=C2=A0 ./bug-reproduction
>> > 00020000-00021000 rw-p 00004000 00:02 131=C2=A0 ./bug-reproduction
>> >
>> > And new kernels do it like this:
>> >
>> > 00010000-00011000 r--p 00001000 00:02 131=C2=A0 ./bug-reproduction
>> > 00011000-00012000 rw-p 00000000 00:00 0
>> > 00012000-00013000 r-xp 00003000 00:02 131=C2=A0 ./bug-reproduction
>> > 00020000-00021000 rw-p 00004000 00:02 131=C2=A0 ./bug-reproduction
>> >
>> > That map between 0x11000 and 0x12000 is the program's '.data' and '.bs=
s'
>> > sections to which it tries to write to, and since the kernel doesn't m=
ap
>> > them anymore it crashes.
>> >
>> > I bisected the issue to the following commit:
>> >
>> > commit 585a018627b4d7ed37387211f667916840b5c5ea
>> > Author: Eric W. Biederman <ebiederm@xmission.com>
>> > Date:=C2=A0=C2=A0 Thu Sep 28 20:24:29 2023 -0700
>> >
>> > =C2=A0=C2=A0=C2=A0 binfmt_elf: Support segments with 0 filesz and misa=
ligned 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 ther=
e's
>> > a prebuilt binary in 'bin/bug-reproduction` which you can run. (It's t=
iny,
>> > 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.
>>=20
>> 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.
>>=20
>> 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.
>>=20
>> The code was fixed to honor your .bss segment and now your .data segment
>> is being stomped, because you defined them to overlap.
>>=20
>> 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.
>>=20
>>=20
>> That said regressions suck and it would be good if we could update the
>> code to do something reasonable in this case.
>>=20
>> 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