[PATCH] binfmt_elf: Introduce KUnit test

[PATCH] 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").

Cc: Eric Biederman <ebiederm@xmission.com>
Cc: David Gow <davidgow@google.com>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: "Magnus Groß" <magnus.gross@rwth-aachen.de>
Cc: kunit-dev@googlegroups.com
Cc: linux-fsdevel@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
I'm exploring ways to mock copy_to_user() for more tests in here.
kprobes doesn't seem to let me easily hijack a function...
---
 fs/Kconfig.binfmt      | 17 +++++++++++
 fs/binfmt_elf.c        |  4 +++
 fs/binfmt_elf_test.c   | 64 ++++++++++++++++++++++++++++++++++++++++++
 fs/compat_binfmt_elf.c |  2 ++
 4 files changed, 87 insertions(+)
 create mode 100644 fs/binfmt_elf_test.c

diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index 4d5ae61580aa..8e14589ee9cc 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -28,6 +28,23 @@ 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.
+
+	  KUnit tests run during boot and output the results to the debug log
+	  in TAP format (https://testanything.org/). Only useful for kernel devs
+	  running KUnit test harness and are not for inclusion into a
+	  production build.
+
+	  For more information on KUnit and unit tests in general please refer
+	  to the KUnit documentation in Documentation/dev-tools/kunit/.
+
+	  If unsure, say N.
+
 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 76ff2af15ba5..9bea703ed1c2 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -2335,3 +2335,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..486ad419f763
--- /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/lkml/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.30.2

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Daniel Latypov]

On Wed, Feb 23, 2022 at 9:43 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").
>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: "Magnus Groß" <magnus.gross@rwth-aachen.de>
> Cc: kunit-dev@googlegroups.com
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> I'm exploring ways to mock copy_to_user() for more tests in here.
> kprobes doesn't seem to let me easily hijack a function...

Yeah, there doesn't seem to be a good way to do so. It seems more
feasible if one is willing to write arch-specific code, but I'm not
quite sure if that works either.

https://kunit.dev/mocking.html has some thoughts on this.
Not sure if there's anything there that would be useful to you, but
perhaps it can give you some ideas.

> ---
>  fs/Kconfig.binfmt      | 17 +++++++++++
>  fs/binfmt_elf.c        |  4 +++
>  fs/binfmt_elf_test.c   | 64 ++++++++++++++++++++++++++++++++++++++++++
>  fs/compat_binfmt_elf.c |  2 ++
>  4 files changed, 87 insertions(+)
>  create mode 100644 fs/binfmt_elf_test.c
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 4d5ae61580aa..8e14589ee9cc 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -28,6 +28,23 @@ 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.
> +
> +         KUnit tests run during boot and output the results to the debug log
> +         in TAP format (https://testanything.org/). Only useful for kernel devs

Tangent: should we update the kunit style guide to not refer to TAP
anymore as it's not accurate?
The KTAP spec is live on kernel.org at
https://www.kernel.org/doc/html/latest/dev-tools/ktap.html

We can leave this patch as-is and update later, or have it be the
guinea pig for the new proposed wording.

(I'm personally in favor of people not copy-pasting these paragraphs
in the first place, but that is what the style-guide currently
recommends)

> +         running KUnit test harness and are not for inclusion into a
> +         production build.
> +
> +         For more information on KUnit and unit tests in general please refer
> +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +         If unsure, say N.
> +
>  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 76ff2af15ba5..9bea703ed1c2 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -2335,3 +2335,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..486ad419f763
> --- /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/lkml/YfF18Dy85mCntXrx@fractal.localdomain */

Slight nit, it looks like that message wasn't sent to lkml.
lore gives a suggestion to change to
https://lore.kernel.org/linux-fsdevel/YfF18Dy85mCntXrx@fractal.localdomain/

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Kees Cook]

On Wed, Feb 23, 2022 at 10:07:04PM -0800, Daniel Latypov wrote:
> On Wed, Feb 23, 2022 at 9:43 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").
> >
> > Cc: Eric Biederman <ebiederm@xmission.com>
> > Cc: David Gow <davidgow@google.com>
> > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > Cc: "Magnus Groß" <magnus.gross@rwth-aachen.de>
> > Cc: kunit-dev@googlegroups.com
> > Cc: linux-fsdevel@vger.kernel.org
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> > I'm exploring ways to mock copy_to_user() for more tests in here.
> > kprobes doesn't seem to let me easily hijack a function...
> 
> Yeah, there doesn't seem to be a good way to do so. It seems more
> feasible if one is willing to write arch-specific code, but I'm not
> quite sure if that works either.

Yeah, I'm hoping maybe Steven has some ideas.

Steven, I want to do fancy live-patch kind or things to replace functions,
but it doesn't need to be particularly fancy because KUnit tests (usually)
run single-threaded, etc. It looks like kprobes could almost do it, but
I don't see a way to have it _avoid_ making a function call.

> https://kunit.dev/mocking.html has some thoughts on this.
> Not sure if there's anything there that would be useful to you, but
> perhaps it can give you some ideas.

Yeah, I figure a small refactoring to use a passed task_struct can avoid
the "current" uses in load_elf_binary(), etc, but the copy_to_user() is
more of a problem. I have considered inverting the Makefile logic,
though, and having binfmt_elf_test.c include binfmt_elf.c and have it
just use a #define to redirect copy_to_user, kind of how all the compat
handling is already done. But it'd be nice to have a "cleaner" mocking
solution...

> 
> > ---
> >  fs/Kconfig.binfmt      | 17 +++++++++++
> >  fs/binfmt_elf.c        |  4 +++
> >  fs/binfmt_elf_test.c   | 64 ++++++++++++++++++++++++++++++++++++++++++
> >  fs/compat_binfmt_elf.c |  2 ++
> >  4 files changed, 87 insertions(+)
> >  create mode 100644 fs/binfmt_elf_test.c
> >
> > diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> > index 4d5ae61580aa..8e14589ee9cc 100644
> > --- a/fs/Kconfig.binfmt
> > +++ b/fs/Kconfig.binfmt
> > @@ -28,6 +28,23 @@ 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.
> > +
> > +         KUnit tests run during boot and output the results to the debug log
> > +         in TAP format (https://testanything.org/). Only useful for kernel devs
> 
> Tangent: should we update the kunit style guide to not refer to TAP
> anymore as it's not accurate?
> The KTAP spec is live on kernel.org at
> https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
> 
> We can leave this patch as-is and update later, or have it be the
> guinea pig for the new proposed wording.

Oops, good point. I was actually thinking it doesn't make too much sense
to keep repeating the same long boilerplate generally.

> (I'm personally in favor of people not copy-pasting these paragraphs
> in the first place, but that is what the style-guide currently
> recommends)

Let's change the guide? :)

> 
> > +         running KUnit test harness and are not for inclusion into a
> > +         production build.
> > +
> > +         For more information on KUnit and unit tests in general please refer
> > +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > +         If unsure, say N.
> > +
> >  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 76ff2af15ba5..9bea703ed1c2 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -2335,3 +2335,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..486ad419f763
> > --- /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/lkml/YfF18Dy85mCntXrx@fractal.localdomain */
> 
> Slight nit, it looks like that message wasn't sent to lkml.
> lore gives a suggestion to change to
> https://lore.kernel.org/linux-fsdevel/YfF18Dy85mCntXrx@fractal.localdomain/

Ah, thank you. I was replacing the /r/ that used to be in that URL, and
got lost. :)

-- 
Kees Cook

Re: [PATCH] binfmt_elf: Introduce KUnit test

[David Gow]

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

On Thu, Feb 24, 2022 at 1:43 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").
>

(Just as a note to anyone else testing this, this hasn't hit
linux-next yet, so the test does fail out of the box.)

> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: David Gow <davidgow@google.com>
> Cc: Alexey Dobriyan <adobriyan@gmail.com>
> Cc: "Magnus Groß" <magnus.gross@rwth-aachen.de>
> Cc: kunit-dev@googlegroups.com
> Cc: linux-fsdevel@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---

This looks like a pretty sensible test to me. It's definitely doing a
few weird things with the whole COMPAT support bit, and I do think
that inverting the flow and having the binfmt_elf_test.c file include
binfmt_elf.c is probably the more flexible solution.

In any case, the test works well for me (including the compat_
variant), modulo a build error and needing to apply the aforementioned
fix first.

Cheers,
-- David

> I'm exploring ways to mock copy_to_user() for more tests in here.
> kprobes doesn't seem to let me easily hijack a function...

Not much to add to what Daniel said here, other than to re-tread some
old discussions around using ftrace, weak-linked symbols, and all
sorts of other horrors to intercept function calls.

The least horrifying solution we've thought of thus far is to
literally change the function to add a test to see if it's running
under a KUnit test which wants to mock it out (using the pointer in
task_struct and KUnit named resources to check). Even then, that's
definitely the sort of thing that you want behind an #ifdef
CONFIG_KUNIT check, and even then is likely to be met with some
disapproval for something as performance-sensitive as copy_to_user().
Of course, if just including the binfmt_elf.c file from within the
test file and using #defines works -- i.e., we don't need to care
about very indirect calls from within different files -- then that's
the simplest solution.

More binfmt_elf tests would be great, though, particularly if we can
get some of those nasty parsing functions tested.

> ---
>  fs/Kconfig.binfmt      | 17 +++++++++++
>  fs/binfmt_elf.c        |  4 +++
>  fs/binfmt_elf_test.c   | 64 ++++++++++++++++++++++++++++++++++++++++++
>  fs/compat_binfmt_elf.c |  2 ++
>  4 files changed, 87 insertions(+)
>  create mode 100644 fs/binfmt_elf_test.c
>
> diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> index 4d5ae61580aa..8e14589ee9cc 100644
> --- a/fs/Kconfig.binfmt
> +++ b/fs/Kconfig.binfmt
> @@ -28,6 +28,23 @@ 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.
> +
> +         KUnit tests run during boot and output the results to the debug log
> +         in TAP format (https://testanything.org/). Only useful for kernel devs
> +         running KUnit test harness and are not for inclusion into a
> +         production build.

It might be worth documenting here that this particular config option
actually builds two test suites if COMPAT is enabled.

> +
> +         For more information on KUnit and unit tests in general please refer
> +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> +         If unsure, say N.
> +
>  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 76ff2af15ba5..9bea703ed1c2 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -2335,3 +2335,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..486ad419f763
> --- /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, },

This doesn't build for me, as PT_GNU_RELRO isn't defined. Adding it
makes the test build:

---
Signed-off-by: David Gow <davidgow@google.com>
---
diff --git a/include/uapi/linux/elf.h b/include/uapi/linux/elf.h
index 61bf4774b8f2..c33cdb4d9464 100644
--- a/include/uapi/linux/elf.h
+++ b/include/uapi/linux/elf.h
@@ -36,6 +36,7 @@ typedef __s64 Elf64_Sxword;
#define PT_LOPROC  0x70000000
#define PT_HIPROC  0x7fffffff
#define PT_GNU_EH_FRAME                0x6474e550
+#define PT_GNU_RELRO           0x6474e552
#define PT_GNU_PROPERTY                0x6474e553

#define PT_GNU_STACK   (PT_LOOS + 0x474e551)
---

> +       };
> +       size_t mount_size = 0xE070;
> +       /* https://lore.kernel.org/lkml/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.30.2
>

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

Re: [PATCH] binfmt_elf: Introduce KUnit test

[David Gow]

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

On Thu, Feb 24, 2022 at 2:13 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Feb 23, 2022 at 10:07:04PM -0800, Daniel Latypov wrote:
> > On Wed, Feb 23, 2022 at 9:43 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").
> > >
> > > Cc: Eric Biederman <ebiederm@xmission.com>
> > > Cc: David Gow <davidgow@google.com>
> > > Cc: Alexey Dobriyan <adobriyan@gmail.com>
> > > Cc: "Magnus Groß" <magnus.gross@rwth-aachen.de>
> > > Cc: kunit-dev@googlegroups.com
> > > Cc: linux-fsdevel@vger.kernel.org
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > > I'm exploring ways to mock copy_to_user() for more tests in here.
> > > kprobes doesn't seem to let me easily hijack a function...
> >
> > Yeah, there doesn't seem to be a good way to do so. It seems more
> > feasible if one is willing to write arch-specific code, but I'm not
> > quite sure if that works either.
>
> Yeah, I'm hoping maybe Steven has some ideas.
>
> Steven, I want to do fancy live-patch kind or things to replace functions,
> but it doesn't need to be particularly fancy because KUnit tests (usually)
> run single-threaded, etc. It looks like kprobes could almost do it, but
> I don't see a way to have it _avoid_ making a function call.
>
> > https://kunit.dev/mocking.html has some thoughts on this.
> > Not sure if there's anything there that would be useful to you, but
> > perhaps it can give you some ideas.
>
> Yeah, I figure a small refactoring to use a passed task_struct can avoid
> the "current" uses in load_elf_binary(), etc, but the copy_to_user() is
> more of a problem. I have considered inverting the Makefile logic,
> though, and having binfmt_elf_test.c include binfmt_elf.c and have it
> just use a #define to redirect copy_to_user, kind of how all the compat
> handling is already done. But it'd be nice to have a "cleaner" mocking
> solution...
>

I think inverting the Makefile makes some sense here, even if it leads
to some code-duplication #define ugliness. Unfortunately, there just
doesn't seem to be a "clean" way of mocking out functions which is
also safe (particularly for something like copy_to_user(), which might
be running in a different thread concurrently with a test) and
performant. If there is a way to refactor the code to avoid the need
for mocking, that's always nice, but can lead to a lot of extraneous
exported functions / interfaces and other code churn.

> >
> > > ---
> > >  fs/Kconfig.binfmt      | 17 +++++++++++
> > >  fs/binfmt_elf.c        |  4 +++
> > >  fs/binfmt_elf_test.c   | 64 ++++++++++++++++++++++++++++++++++++++++++
> > >  fs/compat_binfmt_elf.c |  2 ++
> > >  4 files changed, 87 insertions(+)
> > >  create mode 100644 fs/binfmt_elf_test.c
> > >
> > > diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
> > > index 4d5ae61580aa..8e14589ee9cc 100644
> > > --- a/fs/Kconfig.binfmt
> > > +++ b/fs/Kconfig.binfmt
> > > @@ -28,6 +28,23 @@ 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.
> > > +
> > > +         KUnit tests run during boot and output the results to the debug log
> > > +         in TAP format (https://testanything.org/). Only useful for kernel devs
> >
> > Tangent: should we update the kunit style guide to not refer to TAP
> > anymore as it's not accurate?
> > The KTAP spec is live on kernel.org at
> > https://www.kernel.org/doc/html/latest/dev-tools/ktap.html
> >
> > We can leave this patch as-is and update later, or have it be the
> > guinea pig for the new proposed wording.
>
> Oops, good point. I was actually thinking it doesn't make too much sense
> to keep repeating the same long boilerplate generally.
>

The KUnit style guide actually never referred to TAP in its example
Kconfig entry, though a number of existing tests did:
https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#test-kconfig-entries

> > (I'm personally in favor of people not copy-pasting these paragraphs
> > in the first place, but that is what the style-guide currently
> > recommends)
>
> Let's change the guide? :)
>

I don't think the actual text recommended in the guide is a problem:
it basically just points to the KUnit documentation, but I can send a
patch to soften the wording from "you MUST describe KUnit in the help
text" (or remove it entirely) if people really don't like it.

> >
> > > +         running KUnit test harness and are not for inclusion into a
> > > +         production build.
> > > +
> > > +         For more information on KUnit and unit tests in general please refer
> > > +         to the KUnit documentation in Documentation/dev-tools/kunit/.
> > > +
> > > +         If unsure, say N.
> > > +
> > >  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 76ff2af15ba5..9bea703ed1c2 100644
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > > @@ -2335,3 +2335,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..486ad419f763
> > > --- /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/lkml/YfF18Dy85mCntXrx@fractal.localdomain */
> >
> > Slight nit, it looks like that message wasn't sent to lkml.
> > lore gives a suggestion to change to
> > https://lore.kernel.org/linux-fsdevel/YfF18Dy85mCntXrx@fractal.localdomain/
>
> Ah, thank you. I was replacing the /r/ that used to be in that URL, and
> got lost. :)
>
> --
> Kees Cook

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

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Alexey Dobriyan]

On Wed, Feb 23, 2022 at 09:43:32PM -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").

This can be tested by small 2 segment program run from userspace.
Make 2 segments, swap them, and see executable rejected because mmap
gets too big an area.

	PT_ALEXEY

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Steven Rostedt]

On Wed, 23 Feb 2022 22:13:25 -0800
Kees Cook <keescook@chromium.org> wrote:

> Steven, I want to do fancy live-patch kind or things to replace functions,
> but it doesn't need to be particularly fancy because KUnit tests (usually)
> run single-threaded, etc. It looks like kprobes could almost do it, but
> I don't see a way to have it _avoid_ making a function call.


// This is called just before the hijacked function is called
static void notrace my_tramp(unsigned long ip, unsigned long parent_ip,
			     struct ftrace_ops *ops,
			     struct ftrace_regs *fregs)
{
	int bit;

        bit = ftrace_test_recursion_trylock(ip, parent_ip);
        if (WARN_ON_ONCE(bit < 0))
                return;

	/*
	 * This uses the live kernel patching arch code to now return
	 * to new_function() instead of the one that was called.
	 * If you want to do a lookup, you can look at the "ip"
	 * which will give you the function you are about to replace.
	 * Note, it may not be equal to the function address,
	 * but for that, you can have this:
	 *   ip = ftrace_location(function_ip);
	 * which will give the ip that is passed here.
	 */
	klp_arch_set_pc(fregs, new_function);

	ftrace_test_recursion_unlock(bit);	
}

static struct ftrace_ops my_ops = {
	.func		= my_tramp;
	.flags		= FTRACE_OPS_FL_IPMODIFY |
#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
			  FTRACE_OPS_FL_SAVE_REGS |
#endif
			  FTRACE_OPS_FL_DYNAMIC;
};

// Assuming you know the function ip you want to hijack
register_my_kutest_ip(unsigned long func_ip)
{
	unsigned long ip;
	int ret;

	ip = ftrace_location(func_ip);
	if (!ip) // not a ftrace function?
		return;

	ret = ftrace_set_filter_ip(&my_ops, ip, 0, 0);
	if (ret < 0)
		return;

	// you can register more than one ip if the last parameter
	// is zero (1 means to reset the list)

	ret = register_ftrace_function(&my_ops);
	if (ret < 0)
		return;
}

That's pretty much it. Note, I did not compile nor test the above, so there
may be some mistakes.

For more information about how to use ftrace, see

  Documentation/trace/ftrace-uses.rst

Which should probably get a section on how to do kernel patching.

-- Steve

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Daniel Latypov]

On Thu, Feb 24, 2022 at 6:15 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Wed, 23 Feb 2022 22:13:25 -0800
> Kees Cook <keescook@chromium.org> wrote:
>
> > Steven, I want to do fancy live-patch kind or things to replace functions,
> > but it doesn't need to be particularly fancy because KUnit tests (usually)
> > run single-threaded, etc. It looks like kprobes could almost do it, but
> > I don't see a way to have it _avoid_ making a function call.
>
>
> // This is called just before the hijacked function is called
> static void notrace my_tramp(unsigned long ip, unsigned long parent_ip,
>                              struct ftrace_ops *ops,
>                              struct ftrace_regs *fregs)
> {
>         int bit;
>
>         bit = ftrace_test_recursion_trylock(ip, parent_ip);
>         if (WARN_ON_ONCE(bit < 0))
>                 return;
>
>         /*
>          * This uses the live kernel patching arch code to now return
>          * to new_function() instead of the one that was called.
>          * If you want to do a lookup, you can look at the "ip"
>          * which will give you the function you are about to replace.
>          * Note, it may not be equal to the function address,
>          * but for that, you can have this:
>          *   ip = ftrace_location(function_ip);
>          * which will give the ip that is passed here.
>          */
>         klp_arch_set_pc(fregs, new_function);

Ahah!
This was the missing bit.

David and I both got so excited by this we prototyped experimental
APIs around this over the weekend.
He also prototyped a more intrusive alternative to using ftrace and
kernel livepatch since they don't work on all arches, like UML.

We're splitting up responsibility and will each submit RFCs to the
list in the coming days.
I'll send the ftrace one based on this.
He'll send his alternative one as well.
I think we'll end up having both approaches as they both have their usecases.

It'll take some iteration to bikeshed stuff like names and make them
more consistent with each other.
I've posted my working copy on Gerrit for now, if people want to take
a look: https://kunit-review.googlesource.com/c/linux/+/5109

It should be visible publicly, but it will prompt you to sign in if
you try to post comments ;(
If anyone has comments before we send out the RFCs, feel free to email
me directly and CC kunit-dev@.

Thanks,
Daniel

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Kees Cook]

On Mon, Feb 28, 2022 at 05:48:27PM -0800, Daniel Latypov wrote:
> On Thu, Feb 24, 2022 at 6:15 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed, 23 Feb 2022 22:13:25 -0800
> > Kees Cook <keescook@chromium.org> wrote:
> >
> > > Steven, I want to do fancy live-patch kind or things to replace functions,
> > > but it doesn't need to be particularly fancy because KUnit tests (usually)
> > > run single-threaded, etc. It looks like kprobes could almost do it, but
> > > I don't see a way to have it _avoid_ making a function call.
> >
> >
> > // This is called just before the hijacked function is called
> > static void notrace my_tramp(unsigned long ip, unsigned long parent_ip,
> >                              struct ftrace_ops *ops,
> >                              struct ftrace_regs *fregs)
> > {
> >         int bit;
> >
> >         bit = ftrace_test_recursion_trylock(ip, parent_ip);
> >         if (WARN_ON_ONCE(bit < 0))
> >                 return;
> >
> >         /*
> >          * This uses the live kernel patching arch code to now return
> >          * to new_function() instead of the one that was called.
> >          * If you want to do a lookup, you can look at the "ip"
> >          * which will give you the function you are about to replace.
> >          * Note, it may not be equal to the function address,
> >          * but for that, you can have this:
> >          *   ip = ftrace_location(function_ip);
> >          * which will give the ip that is passed here.
> >          */
> >         klp_arch_set_pc(fregs, new_function);
> 
> Ahah!
> This was the missing bit.
> 
> David and I both got so excited by this we prototyped experimental
> APIs around this over the weekend.
> He also prototyped a more intrusive alternative to using ftrace and
> kernel livepatch since they don't work on all arches, like UML.

Yay! That's excellent. I didn't have time to try this myself, so I'm
delighted to see y'all got it working. Nice!

> We're splitting up responsibility and will each submit RFCs to the
> list in the coming days.
> I'll send the ftrace one based on this.
> He'll send his alternative one as well.
> I think we'll end up having both approaches as they both have their usecases.
> 
> It'll take some iteration to bikeshed stuff like names and make them
> more consistent with each other.
> I've posted my working copy on Gerrit for now, if people want to take
> a look: https://kunit-review.googlesource.com/c/linux/+/5109

Great! I'll go comment on it there.

-- 
Kees Cook

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Steven Rostedt]

On Mon, 28 Feb 2022 17:48:27 -0800
Daniel Latypov <dlatypov@google.com> wrote:

> He also prototyped a more intrusive alternative to using ftrace and
> kernel livepatch since they don't work on all arches, like UML.

Perhaps instead of working on a intrusive alternative on archs that do
not support live kernel patching, implement live kernel patching on
those archs! ;-)

It's probably the same amount of work. Well, really, you only need to
implement the klp_arch_set_pc(fregs, new_function); part.

-- Steve

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Daniel Latypov]

On Mon, Feb 28, 2022 at 8:21 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> On Mon, 28 Feb 2022 17:48:27 -0800
> Daniel Latypov <dlatypov@google.com> wrote:
>
> > He also prototyped a more intrusive alternative to using ftrace and
> > kernel livepatch since they don't work on all arches, like UML.
>
> Perhaps instead of working on a intrusive alternative on archs that do
> not support live kernel patching, implement live kernel patching on
> those archs! ;-)
>
> It's probably the same amount of work. Well, really, you only need to
> implement the klp_arch_set_pc(fregs, new_function); part.

Yeah, that's the only bit we'd need to get working.
I called this out in "Open questions:" bit on
https://kunit-review.googlesource.com/c/linux/+/5109

As for the amount of work, I know how to do KUnit-y things, I have no
idea how to do livepatch things :)
Also, we're not aiming for something as "magic" as the ftrace one.

David's patch is here: https://kunit-review.googlesource.com/c/linux/+/5129
Here's a snippet from the example in that one:

static int add_one(int i)
{
/* This will trigger the stub if active. */
KUNIT_TRIGGER_STATIC_STUB(add_one, i);

return i + 1;
}

i.e. users just add this one macro in with <func> and <args>.
It internally expands to roughly

  if (<check if current test has registered a replacement>)
      <invoke replacement with <args>

So it's all quite simple.

But it'd definitely be interesting to try and get klp_arch_set_pc()
working on UML if that's a possibility!
Speaking from ignorance, I can see this either being somewhat simple
or very painful.

>
> -- Steve

Re: [PATCH] binfmt_elf: Introduce KUnit test

[Steven Rostedt]

On Mon, 28 Feb 2022 22:42:51 -0800
Daniel Latypov <dlatypov@google.com> wrote:

> But it'd definitely be interesting to try and get klp_arch_set_pc()
> working on UML if that's a possibility!
> Speaking from ignorance, I can see this either being somewhat simple

Looking at UML, it doesn't even look like it has ftrace support. So that
would be required to do it for that arch.

For UML, I'm not sure its worth it. I don't use UML but maybe others would
want ftrace for it? As UML runs in userspace, the motivation for things like
ftrace is not as high as you have gdb to walk through everything.

That said, I'm sure it would be a fun exercise for anyone to port ftrace to
UML :-)

-- Steve