Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling

Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling

[Kees Cook]

On Sun, Nov 06, 2022 at 02:16:57AM +0000, Pedro Falcato wrote:
> The old code for ELF interpreter loading could only handle
> 1 memsz > filesz segment. This is incorrect, as evidenced
> by the elf program loading code, which could handle multiple
> such segments.
> 
> This patch fixes memsz > filesz handling for elf interpreters
> and refactors interpreter/program BSS clearing into a common
> codepath.
> 
> This bug was uncovered on builds of ppc64le musl libc with
> llvm lld 15.0.0, since ppc64 does not allocate file space
> for its .plt.
> 
> Cc: Rich Felker <dalias@libc.org>
> Signed-off-by: Pedro Falcato <pedro.falcato@gmail.com>

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.

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.

Anyway, I'll test this patch and get it applied and likely backported
to earlier kernels in the next few days.

-Kees

-- 
Kees Cook

Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling

[Pedro Falcato]

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

Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling

[David Gow]

On Mon, Nov 7, 2022 at 11:59 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Sun, Nov 06, 2022 at 02:16:57AM +0000, Pedro Falcato wrote:
> 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.

Hi Kees,

Sorry the the delayed response!

Alas, my attempts to get this to work haven't been much more
successful than yours. It's definitely something we'd like to support,
but I confess to not knowing enough about the mm code to know exactly
what would be involved.

The workaround is to load tests as modules, and use something like
Vitor's original patch here:
https://lore.kernel.org/all/20200721174036.71072-1-vitor@massaru.org/

Basically, using the existing mm of the module loader. Adapting those
changes to your branch (and fixing a couple of back-to-front KUnit
assertions) does work for me when built as a module, in an x86_64 vm:

root@slicestar:~# modprobe usercopy_kunit
[   52.986290]     # Subtest: usercopy
[   52.986701]     1..1
[   53.246058]     ok 1 - usercopy_test
[   53.246628] ok 1 - usercopy

But getting it to work with built-in tests hasn't been successful so
far. I wondered if we could just piggy-back on init_mm or similar, but
that doesn't seem to work either.

So, in the short-term, this is only possible for modules. If that's
useful enough, we can get Vitor's support patch (or something similar)
in, and just mark any tests module-only (or have them skip if there's
no mm). Because kunit.py only runs built-in tests, though, it's
definitely less convenient.

Cheers,
-- David

Attaching userspace VM to kernel thread (was Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling)

[Kees Cook]

Hi,

This has diverged from the original topic a bit, so I've changed the
Subject to hopefully gain visibility. :)

For KUnit, it would be REALLY nice to have a way to attach a userspace
VM to a kernel thread so we can do userspace memory mapping
manipulation, etc. Neither David nor I have been able to figure out the
right set of steps to make this happen. What are we missing?

Details below...

On Wed, Nov 16, 2022 at 12:34:40PM +0800, David Gow wrote:
> On Mon, Nov 7, 2022 at 11:59 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Sun, Nov 06, 2022 at 02:16:57AM +0000, Pedro Falcato wrote:
> > 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.
> 
> Hi Kees,
> 
> Sorry the the delayed response!
> 
> Alas, my attempts to get this to work haven't been much more
> successful than yours. It's definitely something we'd like to support,
> but I confess to not knowing enough about the mm code to know exactly
> what would be involved.
> 
> The workaround is to load tests as modules, and use something like
> Vitor's original patch here:
> https://lore.kernel.org/all/20200721174036.71072-1-vitor@massaru.org/
> 
> Basically, using the existing mm of the module loader. Adapting those
> changes to your branch (and fixing a couple of back-to-front KUnit
> assertions) does work for me when built as a module, in an x86_64 vm:
> 
> root@slicestar:~# modprobe usercopy_kunit
> [   52.986290]     # Subtest: usercopy
> [   52.986701]     1..1
> [   53.246058]     ok 1 - usercopy_test
> [   53.246628] ok 1 - usercopy
> 
> But getting it to work with built-in tests hasn't been successful so
> far. I wondered if we could just piggy-back on init_mm or similar, but
> that doesn't seem to work either.
> 
> So, in the short-term, this is only possible for modules. If that's
> useful enough, we can get Vitor's support patch (or something similar)
> in, and just mark any tests module-only (or have them skip if there's
> no mm). Because kunit.py only runs built-in tests, though, it's
> definitely less convenient.

Thanks for any pointers! :)

-Kees

-- 
Kees Cook

Re: Attaching userspace VM to kernel thread (was Re: [PATCH] fs/binfmt_elf: Fix memsz > filesz handling)

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3100 bytes --]

On Fri, Nov 18, 2022 at 6:06 AM Kees Cook <keescook@chromium.org> wrote:
>
> Hi,
>
> This has diverged from the original topic a bit, so I've changed the
> Subject to hopefully gain visibility. :)
>
> For KUnit, it would be REALLY nice to have a way to attach a userspace
> VM to a kernel thread so we can do userspace memory mapping
> manipulation, etc. Neither David nor I have been able to figure out the
> right set of steps to make this happen. What are we missing?
>
> Details below...
>
> On Wed, Nov 16, 2022 at 12:34:40PM +0800, David Gow wrote:
> > On Mon, Nov 7, 2022 at 11:59 AM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Sun, Nov 06, 2022 at 02:16:57AM +0000, Pedro Falcato wrote:
> > > 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.
> >
> > Hi Kees,
> >
> > Sorry the the delayed response!
> >
> > Alas, my attempts to get this to work haven't been much more
> > successful than yours. It's definitely something we'd like to support,
> > but I confess to not knowing enough about the mm code to know exactly
> > what would be involved.
> >
> > The workaround is to load tests as modules, and use something like
> > Vitor's original patch here:
> > https://lore.kernel.org/all/20200721174036.71072-1-vitor@massaru.org/
> >
> > Basically, using the existing mm of the module loader. Adapting those
> > changes to your branch (and fixing a couple of back-to-front KUnit
> > assertions) does work for me when built as a module, in an x86_64 vm:
> >
> > root@slicestar:~# modprobe usercopy_kunit
> > [   52.986290]     # Subtest: usercopy
> > [   52.986701]     1..1
> > [   53.246058]     ok 1 - usercopy_test
> > [   53.246628] ok 1 - usercopy
> >
> > But getting it to work with built-in tests hasn't been successful so
> > far. I wondered if we could just piggy-back on init_mm or similar, but
> > that doesn't seem to work either.
> >
> > So, in the short-term, this is only possible for modules. If that's
> > useful enough, we can get Vitor's support patch (or something similar)
> > in, and just mark any tests module-only (or have them skip if there's
> > no mm). Because kunit.py only runs built-in tests, though, it's
> > definitely less convenient.
>
> Thanks for any pointers! :)
>
> -Kees
>
> --
> Kees Cook

FWIW, I had another quick look at this yesterday, and I suspect that
(at least one of) the problem(s) is that function pointers like
mm->get_unmapped_area are only setup as part of exec(), so a
newly-created mm isn't actually useful. Looking at, e.g.,
arch_pick_mm_layout(), there's a whole bunch of architecture-dependent
stuff here to handle, e.g., 32-bit compat.

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]