Received-SPF: pass (google.com: domain of linux-kernel+bounces-34149-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Mon, 22 Jan 2024 12:48:06 -0800
From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
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
Subject: Re: Recent-ish changes in binfmt_elf made my program segfault
Message-ID: <202401221226.DAFA58B78@keescook>
References: <c7209e19-89c4-446a-b364-83100e30cc00@exia.io>
 <874jf5co8g.fsf@email.froward.int.ebiederm.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <874jf5co8g.fsf@email.froward.int.ebiederm.org>

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

-- 
Kees Cook