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

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

[Kees Cook]

Hi,

This pair of patches replaces the last patch in this[1] series.

Perform bprm argument overflow checking but only do argmin checks for MMU
systems. To avoid tripping over this again, argmin is explicitly defined
only for CONFIG_MMU. Thank you to Guenter Roeck for finding this issue
(again)!

-Kees

[1] https://lore.kernel.org/all/20240520021337.work.198-kees@kernel.org/

Kees Cook (2):
  execve: Keep bprm->argmin behind CONFIG_MMU
  exec: Avoid pathological argc, envc, and bprm->p values

 fs/exec.c               | 36 +++++++++++++++++++++++++++++-------
 fs/exec_test.c          | 30 +++++++++++++++++++++++++++++-
 include/linux/binfmts.h |  2 +-
 3 files changed, 59 insertions(+), 9 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] execve: Keep bprm->argmin behind CONFIG_MMU

[Kees Cook]

When argmin was added in commit 655c16a8ce9c ("exec: separate
MM_ANONPAGES and RLIMIT_STACK accounting"), it was intended only for
validating stack limits on CONFIG_MMU[1]. All checking for reaching the
limit (argmin) is wrapped in CONFIG_MMU ifdef checks, though setting
argmin was not. That argmin is only supposed to be used under CONFIG_MMU
was rediscovered recently[2], and I don't want to trip over this again.

Move argmin's declaration into the existing CONFIG_MMU area, and add
helpers functions so the MMU tests can be consolidated.

Link: https://lore.kernel.org/all/20181126122307.GA1660@redhat.com [1]
Link: https://lore.kernel.org/all/202406211253.7037F69@keescook/ [2]
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Alexander Viro <viro@zeniv.linux.org.uk>
Cc: Christian Brauner <brauner@kernel.org>
Cc: Jan Kara <jack@suse.cz>
Cc: Alexey Dobriyan <adobriyan@gmail.com>
Cc: Laurent Vivier <laurent@vivier.eu>
Cc: Lukas Bulwahn <lukas.bulwahn@gmail.com>
Cc: linux-fsdevel@vger.kernel.org
Cc: linux-mm@kvack.org
---
 fs/exec.c               | 26 ++++++++++++++++++++------
 fs/exec_test.c          |  2 ++
 include/linux/binfmts.h |  2 +-
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index c3bec126505b..b7bc63bfb907 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -486,6 +486,23 @@ static int count_strings_kernel(const char *const *argv)
 	return i;
 }
 
+static inline int bprm_set_stack_limit(struct linux_binprm *bprm,
+				       unsigned long limit)
+{
+#ifdef CONFIG_MMU
+	bprm->argmin = bprm->p - limit;
+#endif
+	return 0;
+}
+static inline bool bprm_hit_stack_limit(struct linux_binprm *bprm)
+{
+#ifdef CONFIG_MMU
+	return bprm->p < bprm->argmin;
+#else
+	return false;
+#endif
+}
+
 /*
  * Calculate bprm->argmin from:
  * - _STK_LIM
@@ -532,8 +549,7 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
 		return -E2BIG;
 	limit -= ptr_size;
 
-	bprm->argmin = bprm->p - limit;
-	return 0;
+	return bprm_set_stack_limit(bprm, limit);
 }
 
 /*
@@ -571,10 +587,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
 		pos = bprm->p;
 		str += len;
 		bprm->p -= len;
-#ifdef CONFIG_MMU
-		if (bprm->p < bprm->argmin)
+		if (bprm_hit_stack_limit(bprm))
 			goto out;
-#endif
 
 		while (len > 0) {
 			int offset, bytes_to_copy;
@@ -649,7 +663,7 @@ int copy_string_kernel(const char *arg, struct linux_binprm *bprm)
 	/* We're going to work our way backwards. */
 	arg += len;
 	bprm->p -= len;
-	if (IS_ENABLED(CONFIG_MMU) && bprm->p < bprm->argmin)
+	if (bprm_hit_stack_limit(bprm))
 		return -E2BIG;
 
 	while (len > 0) {
diff --git a/fs/exec_test.c b/fs/exec_test.c
index 32a90c6f47e7..8fea0bf0b7f5 100644
--- a/fs/exec_test.c
+++ b/fs/exec_test.c
@@ -96,7 +96,9 @@ static void exec_test_bprm_stack_limits(struct kunit *test)
 
 		rc = bprm_stack_limits(&bprm);
 		KUNIT_EXPECT_EQ_MSG(test, rc, result->expected_rc, "on loop %d", i);
+#ifdef CONFIG_MMU
 		KUNIT_EXPECT_EQ_MSG(test, bprm.argmin, result->expected_argmin, "on loop %d", i);
+#endif
 	}
 }
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 70f97f685bff..e6c00e860951 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -19,13 +19,13 @@ struct linux_binprm {
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
 	unsigned long vma_pages;
+	unsigned long argmin; /* rlimit marker for copy_strings() */
 #else
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];
 #endif
 	struct mm_struct *mm;
 	unsigned long p; /* current top of mem */
-	unsigned long argmin; /* rlimit marker for copy_strings() */
 	unsigned int
 		/* Should an execfd be passed to userspace? */
 		have_execfd:1,
-- 
2.34.1

[PATCH v2 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.

Take special care for !CONFIG_MMU, since argmin is not exposed there.

For 32-bit validation, 32-bit UML was used:
$ tools/testing/kunit/kunit.py run \
	--make_options CROSS_COMPILE=i686-linux-gnu- \
	--make_options SUBARCH=i386 \
	exec

For !MMU validation, m68k was used:
$ tools/testing/kunit/kunit.py run \
	--arch m68k --make_option CROSS_COMPILE=m68k-linux-gnu- \
	exec

Link: https://lore.kernel.org/r/20240520021615.741800-2-keescook@chromium.org
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Guenter Roeck <linux@roeck-us.net>
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      | 10 +++++++++-
 fs/exec_test.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index b7bc63bfb907..5b580ff8d955 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -490,6 +490,9 @@ static inline int bprm_set_stack_limit(struct linux_binprm *bprm,
 				       unsigned long limit)
 {
 #ifdef CONFIG_MMU
+	/* Avoid a pathological bprm->p. */
+	if (bprm->p < limit)
+		return -E2BIG;
 	bprm->argmin = bprm->p - limit;
 #endif
 	return 0;
@@ -531,6 +534,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
@@ -544,7 +550,9 @@ 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;
diff --git a/fs/exec_test.c b/fs/exec_test.c
index 8fea0bf0b7f5..7c77d039680b 100644
--- a/fs/exec_test.c
+++ b/fs/exec_test.c
@@ -8,9 +8,34 @@ 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 },
+#ifdef CONFIG_MMU
+	/* 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 },
+#endif
 	/*
 	 * 0 rlim_stack will get raised to ARG_MAX. With 1 string pointer,
 	 * we should see p - ARG_MAX + sizeof(void *).
@@ -88,6 +113,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 v2 0/2] exec: Avoid pathological argc, envc, and bprm->p values

[Guenter Roeck]

On 6/21/24 13:50, Kees Cook wrote:
> Hi,
> 
> This pair of patches replaces the last patch in this[1] series.
> 
> Perform bprm argument overflow checking but only do argmin checks for MMU
> systems. To avoid tripping over this again, argmin is explicitly defined
> only for CONFIG_MMU. Thank you to Guenter Roeck for finding this issue
> (again)!
> 

That does make me wonder: Is anyone but me testing, much less running,
the nommu code in the kernel ?

mps2-an385 trips over the same problem, and xtensa:nommu_kc705_defconfig
doesn't even build in linux-next right now (spoiler alert: I suspect that
the problem is caused by "kunit: test: Add vm_mmap() allocation resource
manager", but I did not have time to bisect it).

I am kind of tired keeping those tests alive, and I would not exactly
shed tears if nommu support would just be dropped entirely.

Guenter

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

[Kees Cook]

On Fri, Jun 21, 2024 at 02:44:05PM -0700, Guenter Roeck wrote:
> On 6/21/24 13:50, Kees Cook wrote:
> > Hi,
> > 
> > This pair of patches replaces the last patch in this[1] series.
> > 
> > Perform bprm argument overflow checking but only do argmin checks for MMU
> > systems. To avoid tripping over this again, argmin is explicitly defined
> > only for CONFIG_MMU. Thank you to Guenter Roeck for finding this issue
> > (again)!
> > 
> 
> That does make me wonder: Is anyone but me testing, much less running,
> the nommu code in the kernel ?
> 
> mps2-an385 trips over the same problem, and xtensa:nommu_kc705_defconfig
> doesn't even build in linux-next right now (spoiler alert: I suspect that
> the problem is caused by "kunit: test: Add vm_mmap() allocation resource
> manager", but I did not have time to bisect it).

This has a fixed pending:
https://lore.kernel.org/lkml/202406271005.4E767DAE@keescook/

> I am kind of tired keeping those tests alive, and I would not exactly
> shed tears if nommu support would just be dropped entirely.

I haven't ever used the nommu builds, so I don't have a useful opinion
here. :)

-Kees

-- 
Kees Cook