Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221106021657.1145519-1-pedro.falcato@gmail.com> <202211061948.46D3F78@keescook>
In-Reply-To: <202211061948.46D3F78@keescook>
From:   Pedro Falcato <pedro.falcato@gmail.com>
Date:   Tue, 8 Nov 2022 06:13:44 +0000
Message-ID: <CAKbZUD3=4qZJJSDf7GfDRnunoJ_RETEi+iuu3oZewENhC+3i_A@mail.gmail.com>
Subject: Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling
To:     Kees Cook <keescook@chromium.org>
Cc:     David Gow <davidgow@google.com>, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, sam@gentoo.org,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        Eric Biederman <ebiederm@xmission.com>,
        linux-fsdevel@vger.kernel.org, Rich Felker <dalias@libc.org>,
        linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Nov 7, 2022 at 3:59 AM Kees Cook <keescook@chromium.org> wrote:
> Thanks for the patch! I need to triple-check this logic, as there have
> been some overlapping (or out-of-order) LOAD bugs in the past too, and I
> want to make sure we don't accidentally zero things that already got
> loaded, etc.
Hi Kees,

Thanks for looking at my patch. I've submitted an (unprompted) v2 with
an additional fix for
a loading bug that could load segments after a .bss on top of .bss
itself, which would
overwrite any bss zeroing efforts. Note that this bug was already
present in load_elf_binary.

See a repro at https://github.com/heatd/elf-bug-questionmark, and the
comments on the patch/repro for more info.
Most ELF loading operating systems out there seem to fail on this one.

As for overlapping/out-of-order LOAD segments, what kind of handling
do we want here?
The ELF spec (http://www.sco.com/developers/gabi/latest/ch5.pheader.html)
says that
"Loadable segment entries in the program header table appear in
ascending order, sorted on the p_vaddr member.",
so do we really want to support that? My -v2 was substantially
simplified based on assuming ELF-compliant executables.

> David, has there been any work on adding a way to instantiate
> userspace VMAs in a KUnit test? I tried to write this myself, but I
> couldn't figure out how to make the userspace memory mappings appear.
> Here's my fumbling attempt:
> https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git/log/?h=devel/kunit/usercopy
>
> I really wish KUnit had userspace mapping support -- I have a bunch of
> unit tests that need to get built up around checking for regressions
> here, etc.
+1 on getting this unit-tested, this is a bit of a minefield

Thanks,
Pedro