* Re: [PATCH] exec: Check stack space more strictly
2017-07-17 22:22 [PATCH] exec: Check stack space more strictly Andy Lutomirski
@ 2017-07-17 22:51 ` Kees Cook
2017-08-16 12:17 ` Nikolay Borisov
1 sibling, 0 replies; 3+ messages in thread
From: Kees Cook @ 2017-07-17 22:51 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: security, Andrew Morton, LKML
On Mon, Jul 17, 2017 at 3:22 PM, Andy Lutomirski <luto@kernel.org> wrote:
> We can currently blow past the stack rlimit and cause odd behavior
> if there are accounting bugs, rounding issues, or races. It's not
> clear that the odd behavior is actually a problem, but it's nicer to
> fail the exec instead of getting out of sync with stack limits.
>
> Improve the code to more carefully check for space and to abort if
> our stack mm gets too large in setup_arg_pages().
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
This is a nice check to have in case binfmts do insane things to the
vma before setup_arg_pages(). However, we've still got
create_elf_tables() which is a fixed size _except_ that it adds all
the argv/envp pointers. Those are being accounted for in the
get_arg_page() calls from copy_strings(), though.
Does anything use stack between copy_strings() and create_elf_tables()?
> ---
> fs/exec.c | 44 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 62175cbcc801..0c60c0495269 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -764,23 +764,47 @@ int setup_arg_pages(struct linux_binprm *bprm,
> /* mprotect_fixup is overkill to remove the temporary stack flags */
> vma->vm_flags &= ~VM_STACK_INCOMPLETE_SETUP;
>
> - stack_expand = 131072UL; /* randomly 32*4k (or 2*64k) pages */
> - stack_size = vma->vm_end - vma->vm_start;
> /*
> * Align this down to a page boundary as expand_stack
> * will align it up.
> */
> rlim_stack = rlimit(RLIMIT_STACK) & PAGE_MASK;
> + stack_size = vma->vm_end - vma->vm_start;
> +
> + if (stack_size > rlim_stack) {
> + /*
> + * If we've already used too much space (due to accounting
> + * bugs, alignment, races, or any other cause), bail.
> + */
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + /*
> + * stack_expand is the amount of space beyond the space already used
> + * that we're going to pre-allocate in our stack. For historical
> + * reasons, it's 128kB, unless we have less space than that available
> + * in our rlimit.
> + *
> + * This particular historical wart is wrong-headed, though, since
> + * we haven't finished binfmt-specific setup, and the binfmt code
> + * is going to eat up some or all of this space.
> + */
> + stack_expand = min(rlim_stack - stack_size, 131072UL);
Yeah, this needs to account for argv/envp pointers, IMO. But it's
still a magic number not tied to the actual results of
create_elf_tables() (which is effectively fixed in stack usage except
for argv/envp pointers).
I think create_elf_tables() needs checking added, or the magic number
should be tied to the size of the AUX strings (platform,
base_platform, rand_bytes), and elf_info words.
Hmm, does anything use the stack _after_ create_elf_tables()? Could we
move create_elf_tables() right after setup_arg_pages() and then
perform the check?
> +
> #ifdef CONFIG_STACK_GROWSUP
> - if (stack_size + stack_expand > rlim_stack)
> - stack_base = vma->vm_start + rlim_stack;
> - else
> - stack_base = vma->vm_end + stack_expand;
> + if (TASK_SIZE_MAX - vma->vm_end < stack_expand) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> + stack_base = vma->vm_end + stack_expand;
> #else
> - if (stack_size + stack_expand > rlim_stack)
> - stack_base = vma->vm_end - rlim_stack;
> - else
> - stack_base = vma->vm_start - stack_expand;
> + if (vma->vm_start < mmap_min_addr ||
> + vma->vm_start - mmap_min_addr < stack_expand) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> + stack_base = vma->vm_start - stack_expand;
> #endif
> current->mm->start_stack = bprm->p;
> ret = expand_stack(vma, stack_base);
> --
> 2.9.4
>
FWIW, here are my current notes on the whole existing flow (though
note my other series which is trying to move the secureexec callback
earlier):
do_execvat_common:
check RLIMIT_NPROC
unshare_files() (restored on failure)
allocate bprm
prepare_bprm_creds() (allocate bprm->cred)
mutex_lock_interruptible(¤t->signal->cred_guard_mutex)
prepare_exec_creds()
prepare_creds()
memcpy(new, old, ...)
security_prepare_creds() (unimplemented by commoncap)
check_unsafe_exec()
do_open_execat()
sched_exec()
if closed fd, set BINPRM_FLAGS_PATH_INACCESSIBLE
bprm_mm_init() (allocate bprm->mm)
prepare_binprm()
bprm_fill_uid() (fill bprm->cred)
resets euid/egid to current euid/egid
sets euid/egid on bprm based on set*id
security_bprm_set_creds() (handles all euid etc logic)
kernel_read(bprm->file, ...)
copy_strings() (argv)
get_arg_page() (could fail with E2BIG for args size)
copy_strings() (envp)
get_arg_page() (could fail with E2BIG for args size)
would_dump() (examine inode perms for MAY_READ)
maybe set BINPRM_FLAGS_ENFORCE_NONDUMP
exec_binprm()
search_binary_handler() (may go recursive for script and misc)
security_bprm_check()
for each format
load_binary() (e.g. load_elf_binary)
load_elf_phdrs()
kernel_read(bprm->file, ..., elf_interpreter)
would_dump() (interpreter)
maybe set BINPRM_FLAGS_ENFORCE_NONDUMP
PT_GNU_STACK
arch_check_elf()
flush_old_exec()
de_thread()
exec_mmap()
current->mm = bprm->mm
bprm->mm = NULL; (failures after here will SEGV current)
flush_thread()
current->personality
setup_new_exec()
arch_pick_mmap_layout() (uses current RLIMIT_STACK)
dumpable (using current uid/euid?!)
arch_setup_new_exec()
perf_event_exec()
bprm->cred uid/euid
pdeath_signal = 0
dumpable (based on BINPRM_FLAGS_ENFORCE_NONDUMP)
current->self_exec_id++
flush_signal_handlers()
install_exec_creds()
security_bprm_committing_creds()
commit_creds()
bprm->cred = NULL
if !dumpable
perf_event_exit_task()
security_bprm_committed_creds()
mutex_unlock(¤t->signal->cred_guard_mutex)
setup_arg_pages()
uses RLIMIT_STACK
load ELF image (ET_EXEC vs ET_DYN, load_bias, etc)
set_brk()
load_elf_interp() (if elf_interpreter)
create_elf_tables() (eats fixed stack except argv/envp ptrs)
arch_randomize_brk()
start_thread() (everything is up and running now)
if (retval < 0 && !bprm->mm)
SIGSEGV (can we send signals to my setuid parent?)
audit_bprm()
trace_sched_process_exec()
ptrace_event(PTRACE_EVENT_EXEC)
proc_exec_connector()
clean up
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] exec: Check stack space more strictly
2017-07-17 22:22 [PATCH] exec: Check stack space more strictly Andy Lutomirski
2017-07-17 22:51 ` Kees Cook
@ 2017-08-16 12:17 ` Nikolay Borisov
1 sibling, 0 replies; 3+ messages in thread
From: Nikolay Borisov @ 2017-08-16 12:17 UTC (permalink / raw)
To: Andy Lutomirski, security, Andrew Morton; +Cc: linux-kernel
minor nit below
On 18.07.2017 01:22, Andy Lutomirski wrote:
> We can currently blow past the stack rlimit and cause odd behavior
> if there are accounting bugs, rounding issues, or races. It's not
> clear that the odd behavior is actually a problem, but it's nicer to
> fail the exec instead of getting out of sync with stack limits.
>
> Improve the code to more carefully check for space and to abort if
> our stack mm gets too large in setup_arg_pages().
>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> fs/exec.c | 44 ++++++++++++++++++++++++++++++++++----------
> 1 file changed, 34 insertions(+), 10 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 62175cbcc801..0c60c0495269 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -764,23 +764,47 @@ int setup_arg_pages(struct linux_binprm *bprm,
> /* mprotect_fixup is overkill to remove the temporary stack flags */
> vma->vm_flags &= ~VM_STACK_INCOMPLETE_SETUP;
>
> - stack_expand = 131072UL; /* randomly 32*4k (or 2*64k) pages */
> - stack_size = vma->vm_end - vma->vm_start;
> /*
> * Align this down to a page boundary as expand_stack
> * will align it up.
> */
> rlim_stack = rlimit(RLIMIT_STACK) & PAGE_MASK;
> + stack_size = vma->vm_end - vma->vm_start;
> +
> + if (stack_size > rlim_stack) {
> + /*
> + * If we've already used too much space (due to accounting
> + * bugs, alignment, races, or any other cause), bail.
> + */
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> +
> + /*
> + * stack_expand is the amount of space beyond the space already used
> + * that we're going to pre-allocate in our stack. For historical
> + * reasons, it's 128kB, unless we have less space than that available
> + * in our rlimit.
> + *
> + * This particular historical wart is wrong-headed, though, since
> + * we haven't finished binfmt-specific setup, and the binfmt code
> + * is going to eat up some or all of this space.
> + */
> + stack_expand = min(rlim_stack - stack_size, 131072UL);
nit: Use the SZ_128K from sizes.h.
> +
> #ifdef CONFIG_STACK_GROWSUP
> - if (stack_size + stack_expand > rlim_stack)
> - stack_base = vma->vm_start + rlim_stack;
> - else
> - stack_base = vma->vm_end + stack_expand;
> + if (TASK_SIZE_MAX - vma->vm_end < stack_expand) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> + stack_base = vma->vm_end + stack_expand;
> #else
> - if (stack_size + stack_expand > rlim_stack)
> - stack_base = vma->vm_end - rlim_stack;
> - else
> - stack_base = vma->vm_start - stack_expand;
> + if (vma->vm_start < mmap_min_addr ||
> + vma->vm_start - mmap_min_addr < stack_expand) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
> + stack_base = vma->vm_start - stack_expand;
> #endif
> current->mm->start_stack = bprm->p;
> ret = expand_stack(vma, stack_base);
>
^ permalink raw reply [flat|nested] 3+ messages in thread