[PATCH v2] binfmt_elf: Introduce KUnit test

[PATCH v2] binfmt_elf: Introduce KUnit test

[Kees Cook]

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

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.
-- 
2.32.0

Re: [PATCH v2] binfmt_elf: Introduce KUnit test

[David Gow]

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

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

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

Re: [PATCH v2] binfmt_elf: Introduce KUnit test

[Alexey Dobriyan]

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.

Re: [PATCH v2] binfmt_elf: Introduce KUnit test

[Kees Cook]

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.

-- 
Kees Cook