| Message ID | 20220304044831.962450-1-keescook@chromium.org (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 9e1a3ce0a952450a1163cc93ab1df6d4fa8c8155 |
| Headers | show |
| Series | [v2] binfmt_elf: Introduce KUnit test | expand |
On Fri, Mar 4, 2022 at 12:48 PM Kees Cook <keescook@chromium.org> wrote: > > Adds simple KUnit test for some binfmt_elf internals: specifically a > regression test for the problem fixed by commit 8904d9cd90ee ("ELF: > fix overflow in total mapping size calculation"). > > $ ./tools/testing/kunit/kunit.py run --arch x86_64 \ > --kconfig_add CONFIG_IA32_EMULATION=y '*binfmt_elf' > ... > [19:41:08] ================== binfmt_elf (1 subtest) ================== > [19:41:08] [PASSED] total_mapping_size_test > [19:41:08] =================== [PASSED] binfmt_elf ==================== > [19:41:08] ============== compat_binfmt_elf (1 subtest) =============== > [19:41:08] [PASSED] total_mapping_size_test > [19:41:08] ================ [PASSED] compat_binfmt_elf ================ > [19:41:08] ============================================================ > [19:41:08] Testing complete. Passed: 2, Failed: 0, Crashed: 0, Skipped: 0, Errors: 0 > > Cc: Eric Biederman <ebiederm@xmission.com> > Signed-off-by: Kees Cook <keescook@chromium.org> > --- > This is now at the top of my for-next/execve tree > v1: https://lore.kernel.org/lkml/20220224054332.1852813-1-keescook@chromium.org > v2: > - improve commit log > - fix comment URL (Daniel) > - drop redundant KUnit Kconfig help info (Daniel) > - note in Kconfig help that COMPAT builds add a compat test (David) > --- This looks good to me, and works fine with those two prerequisite patches from your tree: - ELF: Properly redefine PT_GNU_* in terms of PT_LOOS - ELF: fix overflow in total mapping size calculation (I even played a few of the games which triggered the ELF bug to make sure it was fixed, too. :-) ) Reviewed-by: David Gow <davidgow@google.com> Cheers, -- David
On Thu, Mar 03, 2022 at 08:48:31PM -0800, Kees Cook wrote: > Adds simple KUnit test for some binfmt_elf internals: specifically a > regression test for the problem fixed by commit 8904d9cd90ee ("ELF: > fix overflow in total mapping size calculation"). > + /* No headers, no size. */ > + KUNIT_EXPECT_EQ(test, total_mapping_size(NULL, 0), 0); This is meaningless test. This whole function only makes sense if program headers are read and loading process advances far enough so that pointer is not NULL. Are we going to mock every single function in the kernel? Disgusting.
On Wed, Mar 09, 2022 at 12:24:56AM +0300, Alexey Dobriyan wrote: > On Thu, Mar 03, 2022 at 08:48:31PM -0800, Kees Cook wrote: > > Adds simple KUnit test for some binfmt_elf internals: specifically a > > regression test for the problem fixed by commit 8904d9cd90ee ("ELF: > > fix overflow in total mapping size calculation"). > > > + /* No headers, no size. */ > > + KUNIT_EXPECT_EQ(test, total_mapping_size(NULL, 0), 0); > > This is meaningless test. This whole function only makes sense > if program headers are read and loading process advances far enough > so that pointer is not NULL. I think it's important to start adding incremental unit testing to core kernel APIs. This is a case of adding a regression test for a specific misbehavior. This is good, but in addition, testing should check any other corner cases as well. Yes, the above EXPECT line is total nonsense, and it makes sure that nonsense actually reports back the expected failure state "0". > Are we going to mock every single function in the kernel? > Disgusting. I'm not really interested in a slippery slope debate, but honestly, if we _could_ mock everything in the kernel and create unit tests for everything in the kernel, then yes, we should. It's certainly not feasible, but at least _getting started_ on unit testing execve is worth it.
diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt index 4d5ae61580aa..f7065e825b7f 100644 --- a/fs/Kconfig.binfmt +++ b/fs/Kconfig.binfmt @@ -28,6 +28,16 @@ config BINFMT_ELF ld.so (check the file <file:Documentation/Changes> for location and latest version). +config BINFMT_ELF_KUNIT_TEST + bool "Build KUnit tests for ELF binary support" if !KUNIT_ALL_TESTS + depends on KUNIT=y && BINFMT_ELF=y + default KUNIT_ALL_TESTS + help + This builds the ELF loader KUnit tests, which try to gather + prior bug fixes into a regression test collection. This is really + only needed for debugging. Note that with CONFIG_COMPAT=y, the + compat_binfmt_elf KUnit test is also created. + config COMPAT_BINFMT_ELF def_bool y depends on COMPAT && BINFMT_ELF diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c index 4c5a2175f605..eaf39b1bdbbb 100644 --- a/fs/binfmt_elf.c +++ b/fs/binfmt_elf.c @@ -2346,3 +2346,7 @@ static void __exit exit_elf_binfmt(void) core_initcall(init_elf_binfmt); module_exit(exit_elf_binfmt); MODULE_LICENSE("GPL"); + +#ifdef CONFIG_BINFMT_ELF_KUNIT_TEST +#include "binfmt_elf_test.c" +#endif diff --git a/fs/binfmt_elf_test.c b/fs/binfmt_elf_test.c new file mode 100644 index 000000000000..11d734fec366 --- /dev/null +++ b/fs/binfmt_elf_test.c @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: GPL-2.0-only +#include <kunit/test.h> + +static void total_mapping_size_test(struct kunit *test) +{ + struct elf_phdr empty[] = { + { .p_type = PT_LOAD, .p_vaddr = 0, .p_memsz = 0, }, + { .p_type = PT_INTERP, .p_vaddr = 10, .p_memsz = 999999, }, + }; + /* + * readelf -lW /bin/mount | grep '^ .*0x0' | awk '{print "\t\t{ .p_type = PT_" \ + * $1 ", .p_vaddr = " $3 ", .p_memsz = " $6 ", },"}' + */ + struct elf_phdr mount[] = { + { .p_type = PT_PHDR, .p_vaddr = 0x00000040, .p_memsz = 0x0002d8, }, + { .p_type = PT_INTERP, .p_vaddr = 0x00000318, .p_memsz = 0x00001c, }, + { .p_type = PT_LOAD, .p_vaddr = 0x00000000, .p_memsz = 0x0033a8, }, + { .p_type = PT_LOAD, .p_vaddr = 0x00004000, .p_memsz = 0x005c91, }, + { .p_type = PT_LOAD, .p_vaddr = 0x0000a000, .p_memsz = 0x0022f8, }, + { .p_type = PT_LOAD, .p_vaddr = 0x0000d330, .p_memsz = 0x000d40, }, + { .p_type = PT_DYNAMIC, .p_vaddr = 0x0000d928, .p_memsz = 0x000200, }, + { .p_type = PT_NOTE, .p_vaddr = 0x00000338, .p_memsz = 0x000030, }, + { .p_type = PT_NOTE, .p_vaddr = 0x00000368, .p_memsz = 0x000044, }, + { .p_type = PT_GNU_PROPERTY, .p_vaddr = 0x00000338, .p_memsz = 0x000030, }, + { .p_type = PT_GNU_EH_FRAME, .p_vaddr = 0x0000b490, .p_memsz = 0x0001ec, }, + { .p_type = PT_GNU_STACK, .p_vaddr = 0x00000000, .p_memsz = 0x000000, }, + { .p_type = PT_GNU_RELRO, .p_vaddr = 0x0000d330, .p_memsz = 0x000cd0, }, + }; + size_t mount_size = 0xE070; + /* https://lore.kernel.org/linux-fsdevel/YfF18Dy85mCntXrx@fractal.localdomain */ + struct elf_phdr unordered[] = { + { .p_type = PT_LOAD, .p_vaddr = 0x00000000, .p_memsz = 0x0033a8, }, + { .p_type = PT_LOAD, .p_vaddr = 0x0000d330, .p_memsz = 0x000d40, }, + { .p_type = PT_LOAD, .p_vaddr = 0x00004000, .p_memsz = 0x005c91, }, + { .p_type = PT_LOAD, .p_vaddr = 0x0000a000, .p_memsz = 0x0022f8, }, + }; + + /* No headers, no size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(NULL, 0), 0); + KUNIT_EXPECT_EQ(test, total_mapping_size(empty, 0), 0); + /* Empty headers, no size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(empty, 1), 0); + /* No PT_LOAD headers, no size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(&empty[1], 1), 0); + /* Empty PT_LOAD and non-PT_LOAD headers, no size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(empty, 2), 0); + + /* Normal set of PT_LOADS, and expected size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(mount, ARRAY_SIZE(mount)), mount_size); + /* Unordered PT_LOADs result in same size. */ + KUNIT_EXPECT_EQ(test, total_mapping_size(unordered, ARRAY_SIZE(unordered)), mount_size); +} + +static struct kunit_case binfmt_elf_test_cases[] = { + KUNIT_CASE(total_mapping_size_test), + {}, +}; + +static struct kunit_suite binfmt_elf_test_suite = { + .name = KBUILD_MODNAME, + .test_cases = binfmt_elf_test_cases, +}; + +kunit_test_suite(binfmt_elf_test_suite); diff --git a/fs/compat_binfmt_elf.c b/fs/compat_binfmt_elf.c index 95e72d271b95..8f0af4f62631 100644 --- a/fs/compat_binfmt_elf.c +++ b/fs/compat_binfmt_elf.c @@ -135,6 +135,8 @@ #define elf_format compat_elf_format #define init_elf_binfmt init_compat_elf_binfmt #define exit_elf_binfmt exit_compat_elf_binfmt +#define binfmt_elf_test_cases compat_binfmt_elf_test_cases +#define binfmt_elf_test_suite compat_binfmt_elf_test_suite /* * We share all the actual code with the native (64-bit) version.
Adds simple KUnit test for some binfmt_elf internals: specifically a regression test for the problem fixed by commit 8904d9cd90ee ("ELF: fix overflow in total mapping size calculation"). $ ./tools/testing/kunit/kunit.py run --arch x86_64 \ --kconfig_add CONFIG_IA32_EMULATION=y '*binfmt_elf' ... [19:41:08] ================== binfmt_elf (1 subtest) ================== [19:41:08] [PASSED] total_mapping_size_test [19:41:08] =================== [PASSED] binfmt_elf ==================== [19:41:08] ============== compat_binfmt_elf (1 subtest) =============== [19:41:08] [PASSED] total_mapping_size_test [19:41:08] ================ [PASSED] compat_binfmt_elf ================ [19:41:08] ============================================================ [19:41:08] Testing complete. Passed: 2, Failed: 0, Crashed: 0, Skipped: 0, Errors: 0 Cc: Eric Biederman <ebiederm@xmission.com> Signed-off-by: Kees Cook <keescook@chromium.org> --- This is now at the top of my for-next/execve tree v1: https://lore.kernel.org/lkml/20220224054332.1852813-1-keescook@chromium.org v2: - improve commit log - fix comment URL (Daniel) - drop redundant KUnit Kconfig help info (Daniel) - note in Kconfig help that COMPAT builds add a compat test (David) --- fs/Kconfig.binfmt | 10 +++++++ fs/binfmt_elf.c | 4 +++ fs/binfmt_elf_test.c | 64 ++++++++++++++++++++++++++++++++++++++++++ fs/compat_binfmt_elf.c | 2 ++ 4 files changed, 80 insertions(+) create mode 100644 fs/binfmt_elf_test.c