| Message ID | 20240520021615.741800-2-keescook@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] exec: Add KUnit test for bprm_stack_limits() | expand |
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;
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
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 --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];
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(-)