[PATCH 0/2] exec: Add KUnit test for bprm_stack_limits()

[PATCH 0/2] exec: Add KUnit test for bprm_stack_limits()

[Kees Cook]

Hi,

This adds a first KUnit test to the core exec code. With the ability to
manipulate userspace memory from KUnit coming[1], I wanted to at least
get the KUnit framework in place in exec.c. Most of the coming tests
will likely be to binfmt_elf.c, but still, this serves as a reasonable
first step.

-Kees

[1] https://lore.kernel.org/linux-hardening/20240519190422.work.715-kees@kernel.org/

Kees Cook (2):
  exec: Add KUnit test for bprm_stack_limits()
  exec: Avoid pathological argc, envc, and bprm->p values

 MAINTAINERS       |   2 +
 fs/Kconfig.binfmt |   8 +++
 fs/exec.c         |  24 +++++++-
 fs/exec_test.c    | 137 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 170 insertions(+), 1 deletion(-)
 create mode 100644 fs/exec_test.c

-- 
2.34.1

[PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()

[Kees Cook]

Since bprm_stack_limits() operates with very limited side-effects, add
it as the first exec.c KUnit test. Add to Kconfig and adjust MAINTAINERS
file to include it.

Tested on 64-bit UML:
$ tools/testing/kunit/kunit.py run exec

Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 MAINTAINERS       |   2 +
 fs/Kconfig.binfmt |   8 ++++
 fs/exec.c         |  13 ++++++
 fs/exec_test.c    | 113 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 136 insertions(+)
 create mode 100644 fs/exec_test.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7c121493f43d..845165dbb756 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8039,7 +8039,9 @@ S:	Supported
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/execve
 F:	Documentation/userspace-api/ELF.rst
 F:	fs/*binfmt_*.c
+F:	fs/Kconfig.binfmt
 F:	fs/exec.c
+F:	fs/exec_test.c
 F:	include/linux/binfmts.h
 F:	include/linux/elf.h
 F:	include/uapi/linux/binfmts.h
diff --git a/fs/Kconfig.binfmt b/fs/Kconfig.binfmt
index f5693164ca9a..58657f2d9719 100644
--- a/fs/Kconfig.binfmt
+++ b/fs/Kconfig.binfmt
@@ -176,4 +176,12 @@ config COREDUMP
 	  certainly want to say Y here. Not necessary on systems that never
 	  need debugging or only ever run flawless code.
 
+config EXEC_KUNIT_TEST
+	bool "Build execve tests" if !KUNIT_ALL_TESTS
+	depends on KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This builds the exec KUnit tests, which tests boundary conditions
+	  of various aspects of the exec internals.
+
 endmenu
diff --git a/fs/exec.c b/fs/exec.c
index b3c40fbb325f..1d45e1a2d620 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -475,6 +475,15 @@ static int count_strings_kernel(const char *const *argv)
 	return i;
 }
 
+/*
+ * Calculate bprm->argmin from:
+ * - _STK_LIM
+ * - ARG_MAX
+ * - bprm->rlim_stack.rlim_cur
+ * - bprm->argc
+ * - bprm->envc
+ * - bprm->p
+ */
 static int bprm_stack_limits(struct linux_binprm *bprm)
 {
 	unsigned long limit, ptr_size;
@@ -2200,3 +2209,7 @@ static int __init init_fs_exec_sysctls(void)
 
 fs_initcall(init_fs_exec_sysctls);
 #endif /* CONFIG_SYSCTL */
+
+#ifdef CONFIG_EXEC_KUNIT_TEST
+#include "exec_test.c"
+#endif
diff --git a/fs/exec_test.c b/fs/exec_test.c
new file mode 100644
index 000000000000..32a90c6f47e7
--- /dev/null
+++ b/fs/exec_test.c
@@ -0,0 +1,113 @@
+// SPDX-License-Identifier: GPL-2.0-only
+#include <kunit/test.h>
+
+struct bprm_stack_limits_result {
+	struct linux_binprm bprm;
+	int expected_rc;
+	unsigned long expected_argmin;
+};
+
+static const struct bprm_stack_limits_result bprm_stack_limits_results[] = {
+	/* Giant values produce -E2BIG */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = INT_MAX, .envc = INT_MAX }, .expected_rc = -E2BIG },
+	/*
+	 * 0 rlim_stack will get raised to ARG_MAX. With 1 string pointer,
+	 * we should see p - ARG_MAX + sizeof(void *).
+	 */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = 1, .envc = 0 }, .expected_argmin = ULONG_MAX - ARG_MAX + sizeof(void *)},
+	/* Validate that argc is always raised to a minimum of 1. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = 0, .envc = 0 }, .expected_argmin = ULONG_MAX - ARG_MAX + sizeof(void *)},
+	/*
+	 * 0 rlim_stack will get raised to ARG_MAX. With pointers filling ARG_MAX,
+	 * we should see -E2BIG. (Note argc is always raised to at least 1.)
+	 */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = ARG_MAX / sizeof(void *), .envc = 0 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = 0, .envc = ARG_MAX / sizeof(void *) - 1 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = ARG_MAX / sizeof(void *) + 1, .envc = 0 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = 0, .envc = ARG_MAX / sizeof(void *) }, .expected_rc = -E2BIG },
+	/* And with one less, we see space for exactly 1 pointer. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = (ARG_MAX / sizeof(void *)) - 1, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 0,
+	    .argc = 0, .envc = (ARG_MAX / sizeof(void *)) - 2, },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	/* If we raise rlim_stack / 4 to exactly ARG_MAX, nothing changes. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = ARG_MAX / sizeof(void *), .envc = 0 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = 0, .envc = ARG_MAX / sizeof(void *) - 1 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = ARG_MAX / sizeof(void *) + 1, .envc = 0 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = 0, .envc = ARG_MAX / sizeof(void *) }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = (ARG_MAX / sizeof(void *)) - 1, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ARG_MAX * 4,
+	    .argc = 0, .envc = (ARG_MAX / sizeof(void *)) - 2, },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	/* But raising it another pointer * 4 will provide space for 1 more pointer. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = (ARG_MAX + sizeof(void *)) * 4,
+	    .argc = ARG_MAX / sizeof(void *), .envc = 0 },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = (ARG_MAX + sizeof(void *)) * 4,
+	    .argc = 0, .envc = ARG_MAX / sizeof(void *) - 1 },
+	  .expected_argmin = ULONG_MAX - sizeof(void *) },
+	/* Raising rlim_stack / 4 to _STK_LIM / 4 * 3 will see more space. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * (_STK_LIM / 4 * 3),
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * (_STK_LIM / 4 * 3),
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+	/* But raising it any further will see no increase. */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * (_STK_LIM / 4 * 3 + sizeof(void *)),
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * (_STK_LIM / 4 *  + sizeof(void *)),
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * _STK_LIM,
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = 4 * _STK_LIM,
+	    .argc = 0, .envc = 0 },
+	  .expected_argmin = ULONG_MAX - (_STK_LIM / 4 * 3) + sizeof(void *) },
+};
+
+static void exec_test_bprm_stack_limits(struct kunit *test)
+{
+	/* Double-check the constants. */
+	KUNIT_EXPECT_EQ(test, _STK_LIM, SZ_8M);
+	KUNIT_EXPECT_EQ(test, ARG_MAX, 32 * SZ_4K);
+
+	for (int i = 0; i < ARRAY_SIZE(bprm_stack_limits_results); i++) {
+		const struct bprm_stack_limits_result *result = &bprm_stack_limits_results[i];
+		struct linux_binprm bprm = result->bprm;
+		int rc;
+
+		rc = bprm_stack_limits(&bprm);
+		KUNIT_EXPECT_EQ_MSG(test, rc, result->expected_rc, "on loop %d", i);
+		KUNIT_EXPECT_EQ_MSG(test, bprm.argmin, result->expected_argmin, "on loop %d", i);
+	}
+}
+
+static struct kunit_case exec_test_cases[] = {
+	KUNIT_CASE(exec_test_bprm_stack_limits),
+	{},
+};
+
+static struct kunit_suite exec_test_suite = {
+	.name = "exec",
+	.test_cases = exec_test_cases,
+};
+
+kunit_test_suite(exec_test_suite);
-- 
2.34.1

Re: [PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()

[kernel test robot]

Hi Kees,

kernel test robot noticed the following build errors:

[auto build test ERROR on kees/for-next/execve]
[also build test ERROR on kees/for-next/pstore kees/for-next/kspp brauner-vfs/vfs.all linus/master v6.9 next-20240520]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/exec-Avoid-pathological-argc-envc-and-bprm-p-values/20240520-101851
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/execve
patch link:    https://lore.kernel.org/r/20240520021615.741800-1-keescook%40chromium.org
patch subject: [PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()
config: i386-randconfig-004-20240520 (https://download.01.org/0day-ci/archive/20240520/202405202157.xE9dP8fI-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240520/202405202157.xE9dP8fI-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405202157.xE9dP8fI-lkp@intel.com/

All errors (new ones prefixed by >>):

   ld: fs/exec.o: in function `exec_test_bprm_stack_limits':
>> fs/exec_test.c:98:(.text+0xdfc): undefined reference to `kunit_binary_assert_format'
>> ld: fs/exec_test.c:98:(.text+0xe0c): undefined reference to `__kunit_do_failed_assertion'
>> ld: fs/exec_test.c:99:(.text+0xe56): undefined reference to `kunit_binary_assert_format'
   ld: fs/exec_test.c:99:(.text+0xe66): undefined reference to `__kunit_do_failed_assertion'

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for FB_IOMEM_HELPERS
   Depends on [n]: HAS_IOMEM [=y] && FB_CORE [=n]
   Selected by [m]:
   - DRM_XE_DISPLAY [=y] && HAS_IOMEM [=y] && DRM_XE [=m] && DRM_XE [=m]=m


vim +98 fs/exec_test.c

    85	
    86	static void exec_test_bprm_stack_limits(struct kunit *test)
    87	{
    88		/* Double-check the constants. */
    89		KUNIT_EXPECT_EQ(test, _STK_LIM, SZ_8M);
    90		KUNIT_EXPECT_EQ(test, ARG_MAX, 32 * SZ_4K);
    91	
    92		for (int i = 0; i < ARRAY_SIZE(bprm_stack_limits_results); i++) {
    93			const struct bprm_stack_limits_result *result = &bprm_stack_limits_results[i];
    94			struct linux_binprm bprm = result->bprm;
    95			int rc;
    96	
    97			rc = bprm_stack_limits(&bprm);
  > 98			KUNIT_EXPECT_EQ_MSG(test, rc, result->expected_rc, "on loop %d", i);
  > 99			KUNIT_EXPECT_EQ_MSG(test, bprm.argmin, result->expected_argmin, "on loop %d", i);
   100		}
   101	}
   102	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()

[kernel test robot]

Hi Kees,

kernel test robot noticed the following build errors:

[auto build test ERROR on kees/for-next/execve]
[also build test ERROR on kees/for-next/pstore kees/for-next/kspp brauner-vfs/vfs.all linus/master vfs-idmapping/for-next v6.9 next-20240520]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kees-Cook/exec-Avoid-pathological-argc-envc-and-bprm-p-values/20240520-101851
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git for-next/execve
patch link:    https://lore.kernel.org/r/20240520021615.741800-1-keescook%40chromium.org
patch subject: [PATCH 1/2] exec: Add KUnit test for bprm_stack_limits()
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20240520/202405202231.3Q9gWCar-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240520/202405202231.3Q9gWCar-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405202231.3Q9gWCar-lkp@intel.com/

All errors (new ones prefixed by >>):

   nios2-linux-ld: fs/exec.o: in function `exec_test_bprm_stack_limits':
   exec.c:(.text+0x1904): undefined reference to `kunit_binary_assert_format'
>> nios2-linux-ld: exec.c:(.text+0x192c): undefined reference to `kunit_binary_assert_format'
>> nios2-linux-ld: exec.c:(.text+0x19e8): undefined reference to `__kunit_do_failed_assertion'
>> exec.c:(.text+0x19e8): relocation truncated to fit: R_NIOS2_CALL26 against `__kunit_do_failed_assertion'
   nios2-linux-ld: exec.c:(.text+0x1a28): undefined reference to `__kunit_do_failed_assertion'
   exec.c:(.text+0x1a28): relocation truncated to fit: R_NIOS2_CALL26 against `__kunit_do_failed_assertion'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values

[Kees Cook]

Make sure nothing goes wrong with the string counters or the bprm's
belief about the stack pointer. Add checks and matching self-tests.

For 32-bit validation, this was run under 32-bit UML:
$ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec

Signed-off-by: Kees Cook <keescook@chromium.org>
---
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Justin Stitt <justinstitt@google.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 fs/exec.c      | 11 ++++++++++-
 fs/exec_test.c | 26 +++++++++++++++++++++++++-
 2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 1d45e1a2d620..5dcdd115739e 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -503,6 +503,9 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
 	 * of argument strings even with small stacks
 	 */
 	limit = max_t(unsigned long, limit, ARG_MAX);
+	/* Reject totally pathological counts. */
+	if (bprm->argc < 0 || bprm->envc < 0)
+		return -E2BIG;
 	/*
 	 * We must account for the size of all the argv and envp pointers to
 	 * the argv and envp strings, since they will also take up space in
@@ -516,11 +519,17 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
 	 * argc can never be 0, to keep them from walking envp by accident.
 	 * See do_execveat_common().
 	 */
-	ptr_size = (max(bprm->argc, 1) + bprm->envc) * sizeof(void *);
+	if (check_add_overflow(max(bprm->argc, 1), bprm->envc, &ptr_size) ||
+	    check_mul_overflow(ptr_size, sizeof(void *), &ptr_size))
+		return -E2BIG;
 	if (limit <= ptr_size)
 		return -E2BIG;
 	limit -= ptr_size;
 
+	/* Avoid a pathological bprm->p. */
+	if (bprm->p < limit)
+		return -E2BIG;
+
 	bprm->argmin = bprm->p - limit;
 	return 0;
 }
diff --git a/fs/exec_test.c b/fs/exec_test.c
index 32a90c6f47e7..f2d4a80c861d 100644
--- a/fs/exec_test.c
+++ b/fs/exec_test.c
@@ -8,9 +8,32 @@ struct bprm_stack_limits_result {
 };
 
 static const struct bprm_stack_limits_result bprm_stack_limits_results[] = {
-	/* Giant values produce -E2BIG */
+	/* Negative argc/envc counts produce -E2BIG */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = INT_MIN, .envc = INT_MIN }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = 5, .envc = -1 }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = -1, .envc = 10 }, .expected_rc = -E2BIG },
+	/* The max value of argc or envc is MAX_ARG_STRINGS. */
 	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
 	    .argc = INT_MAX, .envc = INT_MAX }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = MAX_ARG_STRINGS, .envc = MAX_ARG_STRINGS }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = 0, .envc = MAX_ARG_STRINGS }, .expected_rc = -E2BIG },
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = MAX_ARG_STRINGS, .envc = 0 }, .expected_rc = -E2BIG },
+	/*
+	 * On 32-bit system these argc and envc counts, while likely impossible
+	 * to represent within the associated TASK_SIZE, could overflow the
+	 * limit calculation, and bypass the ptr_size <= limit check.
+	 */
+	{ { .p = ULONG_MAX, .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = 0x20000001, .envc = 0x20000001 }, .expected_rc = -E2BIG },
+	/* Make sure a pathological bprm->p doesn't cause an overflow. */
+	{ { .p = sizeof(void *), .rlim_stack.rlim_cur = ULONG_MAX,
+	    .argc = 10, .envc = 10 }, .expected_rc = -E2BIG },
 	/*
 	 * 0 rlim_stack will get raised to ARG_MAX. With 1 string pointer,
 	 * we should see p - ARG_MAX + sizeof(void *).
@@ -88,6 +111,7 @@ static void exec_test_bprm_stack_limits(struct kunit *test)
 	/* Double-check the constants. */
 	KUNIT_EXPECT_EQ(test, _STK_LIM, SZ_8M);
 	KUNIT_EXPECT_EQ(test, ARG_MAX, 32 * SZ_4K);
+	KUNIT_EXPECT_EQ(test, MAX_ARG_STRINGS, 0x7FFFFFFF);
 
 	for (int i = 0; i < ARRAY_SIZE(bprm_stack_limits_results); i++) {
 		const struct bprm_stack_limits_result *result = &bprm_stack_limits_results[i];
-- 
2.34.1

Re: [PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values

[Guenter Roeck]

Hi,

On Sun, May 19, 2024 at 07:16:12PM -0700, Kees Cook wrote:
> Make sure nothing goes wrong with the string counters or the bprm's
> belief about the stack pointer. Add checks and matching self-tests.
> 
> For 32-bit validation, this was run under 32-bit UML:
> $ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>

With this patch in linux-next, the qemu m68k:mcf5208evb emulation
fails to boot. The error is:

Run /init as init process
Failed to execute /init (error -7)
Run /sbin/init as init process
Starting init: /sbin/init exists but couldn't execute it (error -7)
Run /etc/init as init process
Run /bin/init as init process
Run /bin/sh as init process
Starting init: /bin/sh exists but couldn't execute it (error -7)
Kernel panic - not syncing: No working init found.  Try passing init= option to kernel. See Linux Documentation/admin-guide/init.rst for guidance.
CPU: 0 UID: 0 PID: 1 Comm: swapper Not tainted 6.10.0-rc4-next-20240620 #1
Stack from 4081ff74:
        4081ff74 40387a22 40387a22 00000000 0000000a 4039db60 4031b2fe 40387a22
        40314742 00000000 00000000 4039db60 00000000 40314186 4031b494 00000000
        00000000 4031b57e 4037f784 403a3440 40020474 00000000 00000000 00000000
        00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
        00000000 00002000 00000000
Call Trace: [<4031b2fe>] dump_stack+0xc/0x10
 [<40314742>] panic+0xce/0x262
 [<40314186>] try_to_run_init_process+0x0/0x38
 [<4031b494>] kernel_init+0x0/0xf0
 [<4031b57e>] kernel_init+0xea/0xf0
 [<40020474>] ret_from_kernel_thread+0xc/0x14

bisect essentially points to the merge of the for-next/execve branch;
see below. Subsequent failures are false positives. Branch analysis
then pointed to this patch. The image boots after reverting this patch
(or after reverting the entire merge).

Guenter

---
# bad: [b992b79ca8bc336fa8e2c80990b5af80ed8f36fd] Add linux-next specific files for 20240620
# good: [6ba59ff4227927d3a8530fc2973b80e94b54d58f] Linux 6.10-rc4
git bisect start 'HEAD' 'v6.10-rc4'
# good: [c02e717c5a89654b244fec58bb5cda32770966b5] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect good c02e717c5a89654b244fec58bb5cda32770966b5
# good: [29e7d78253b7ebf4b76fcf6d95e227d0b0c57dc0] Merge branch 'msm-next' of https://gitlab.freedesktop.org/drm/msm.git
git bisect good 29e7d78253b7ebf4b76fcf6d95e227d0b0c57dc0
# good: [bf8fd0d956bfcbf4fd6ff063366374c4bf87d806] Merge branch 'non-rcu/next' of git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
git bisect good bf8fd0d956bfcbf4fd6ff063366374c4bf87d806
# good: [1110f16317b1e0742521eaef5613eb1eb17f55ca] Merge branch 'icc-next' of git://git.kernel.org/pub/scm/linux/kernel/git/djakov/icc.git
git bisect good 1110f16317b1e0742521eaef5613eb1eb17f55ca
# good: [63f3716198e5644713748d83e6a6df3b4a6a3b10] Merge branch 'gpio/for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git
git bisect good 63f3716198e5644713748d83e6a6df3b4a6a3b10
# good: [91b48d9adafddb242264ba19c0bae6e23f71b18a] Merge branch 'kunit' of git://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git
git bisect good 91b48d9adafddb242264ba19c0bae6e23f71b18a
# good: [c54c059b3c3c980c66e2a34b08724d9e529f590d] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/srini/nvmem.git
git bisect good c54c059b3c3c980c66e2a34b08724d9e529f590d
# good: [de95d30c03c42225c4fad714bf657c9ebb345fe9] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/mic/linux.git
git bisect good de95d30c03c42225c4fad714bf657c9ebb345fe9
# bad: [cb328321926903f7f54866029590abb8faf48ef6] Merge branch 'for-next/execve' of git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git
git bisect bad cb328321926903f7f54866029590abb8faf48ef6
# bad: [aef9d25e7f5631543a0276d0532151f2c61174d6] sysctl: Remove superfluous empty allocations from sysctl internals
git bisect bad aef9d25e7f5631543a0276d0532151f2c61174d6
# bad: [c819e252c2874479b27f6a356b44f8aa73cf5a81] sysctl: Add module description to sysctl-testing
git bisect bad c819e252c2874479b27f6a356b44f8aa73cf5a81
# bad: [b5ffbd1396885f76bf87e67d590a3ef063e6d831] sysctl: move the extra1/2 boundary check of u8 to sysctl_check_table_array
git bisect bad b5ffbd1396885f76bf87e67d590a3ef063e6d831
# bad: [98ca62ba9e2be5863c7d069f84f7166b45a5b2f4] sysctl: always initialize i_uid/i_gid
git bisect bad 98ca62ba9e2be5863c7d069f84f7166b45a5b2f4
# first bad commit: [98ca62ba9e2be5863c7d069f84f7166b45a5b2f4] sysctl: always initialize i_uid/i_gid

Re: [PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values

[Kees Cook]

On Thu, Jun 20, 2024 at 05:19:55PM -0700, Guenter Roeck wrote:
> Hi,
> 
> On Sun, May 19, 2024 at 07:16:12PM -0700, Kees Cook wrote:
> > Make sure nothing goes wrong with the string counters or the bprm's
> > belief about the stack pointer. Add checks and matching self-tests.
> > 
> > For 32-bit validation, this was run under 32-bit UML:
> > $ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> 
> With this patch in linux-next, the qemu m68k:mcf5208evb emulation
> fails to boot. The error is:

Eeek. Thanks for the report! I've dropped this patch from my for-next
tree.

> Run /init as init process
> Failed to execute /init (error -7)

-7 is E2BIG, so it's certainly one of the 3 new added checks. I must
have made a mistake in my reasoning about how bprm->p is initialized;
the other two checks seems extremely unlikely to be tripped.

I will try to get qemu set up and take a close look at what's happening.
While I'm doing that, if it's easy for you, can you try it with just
this removed (i.e. the other 2 new -E2BIG cases still in place):

	/* Avoid a pathological bprm->p. */
	if (bprm->p < limit)
		return -E2BIG;


-- 
Kees Cook

Re: [PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values

[Guenter Roeck]

On 6/21/24 00:00, Kees Cook wrote:
> On Thu, Jun 20, 2024 at 05:19:55PM -0700, Guenter Roeck wrote:
>> Hi,
>>
>> On Sun, May 19, 2024 at 07:16:12PM -0700, Kees Cook wrote:
>>> Make sure nothing goes wrong with the string counters or the bprm's
>>> belief about the stack pointer. Add checks and matching self-tests.
>>>
>>> For 32-bit validation, this was run under 32-bit UML:
>>> $ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>
>> With this patch in linux-next, the qemu m68k:mcf5208evb emulation
>> fails to boot. The error is:
> 
> Eeek. Thanks for the report! I've dropped this patch from my for-next
> tree.
> 
>> Run /init as init process
>> Failed to execute /init (error -7)
> 
> -7 is E2BIG, so it's certainly one of the 3 new added checks. I must
> have made a mistake in my reasoning about how bprm->p is initialized;
> the other two checks seems extremely unlikely to be tripped.
> 
> I will try to get qemu set up and take a close look at what's happening.
> While I'm doing that, if it's easy for you, can you try it with just
> this removed (i.e. the other 2 new -E2BIG cases still in place):
> 
> 	/* Avoid a pathological bprm->p. */
> 	if (bprm->p < limit)
> 		return -E2BIG;

I added a printk:

argc: 1 envc: 2 p: 262140 limit: 2097152
                 ^^^^^^^^^^^^^^^^^^^^^^^^
Removing the check above does indeed fix the problem.

Guenter

Re: [PATCH 2/2] exec: Avoid pathological argc, envc, and bprm->p values

[Kees Cook]

On Fri, Jun 21, 2024 at 06:21:15AM -0700, Guenter Roeck wrote:
> On 6/21/24 00:00, Kees Cook wrote:
> > On Thu, Jun 20, 2024 at 05:19:55PM -0700, Guenter Roeck wrote:
> > > Hi,
> > > 
> > > On Sun, May 19, 2024 at 07:16:12PM -0700, Kees Cook wrote:
> > > > Make sure nothing goes wrong with the string counters or the bprm's
> > > > belief about the stack pointer. Add checks and matching self-tests.
> > > > 
> > > > For 32-bit validation, this was run under 32-bit UML:
> > > > $ tools/testing/kunit/kunit.py run --make_options SUBARCH=i386 exec
> > > > 
> > > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > 
> > > With this patch in linux-next, the qemu m68k:mcf5208evb emulation
> > > fails to boot. The error is:
> > 
> > Eeek. Thanks for the report! I've dropped this patch from my for-next
> > tree.
> > 
> > > Run /init as init process
> > > Failed to execute /init (error -7)
> > 
> > -7 is E2BIG, so it's certainly one of the 3 new added checks. I must
> > have made a mistake in my reasoning about how bprm->p is initialized;
> > the other two checks seems extremely unlikely to be tripped.
> > 
> > I will try to get qemu set up and take a close look at what's happening.
> > While I'm doing that, if it's easy for you, can you try it with just
> > this removed (i.e. the other 2 new -E2BIG cases still in place):
> > 
> > 	/* Avoid a pathological bprm->p. */
> > 	if (bprm->p < limit)
> > 		return -E2BIG;
> 
> I added a printk:
> 
> argc: 1 envc: 2 p: 262140 limit: 2097152
>                 ^^^^^^^^^^^^^^^^^^^^^^^^
> Removing the check above does indeed fix the problem.

Thanks for checking this!

And I've found my mistake. "argmin" is only valid for CONFIG_MMU. And
you noticed this back in 2018. ;)

http://lkml.kernel.org/r/20181126122307.GA1660@redhat.com

I will try to fix this better so we don't trip over it again.

-- 
Kees Cook