diff mbox series

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

Message ID 20240520021615.741800-2-keescook@chromium.org (mailing list archive)
State New
Headers show
Series exec: Add KUnit test for bprm_stack_limits() | expand

Commit Message

Kees Cook May 20, 2024, 2:16 a.m. UTC 
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(-)

Comments

Guenter Roeck June 21, 2024, 12:19 a.m. UTC | #1 
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
Kees Cook June 21, 2024, 7 a.m. UTC | #2 
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;
Guenter Roeck June 21, 2024, 1:21 p.m. UTC | #3 
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
Kees Cook June 21, 2024, 7:54 p.m. UTC | #4 
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.
diff mbox series

Patch

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];