| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAGXu5jLYWUMFNFaWHTmn9qhArtw7rn376rLRjfg6cdrvYNKUTg@mail.gmail.com>
Date: Tue, 11 Sep 2018 12:06:05 -0700
From: Kees Cook <keescook@...omium.org>
To: Oleg Nesterov <oleg@...hat.com>
Cc: Rik van Riel <riel@...hat.com>, Michal Hocko <mhocko@...e.com>,
Stanislav Kozina <skozina@...hat.com>,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: get_arg_page() && ptr_size accounting
Oh, I like this patch! This is much cleaner. Notes below...
On Tue, Sep 11, 2018 at 7:13 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> diff --git a/fs/exec.c b/fs/exec.c
> index 1ebf6e5..7804a5c 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -218,55 +218,10 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
> if (ret <= 0)
> return NULL;
>
> - if (write) {
> - unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
> - unsigned long ptr_size, limit;
> -
> - /*
> - * Since the stack will hold pointers to the strings, we
> - * must account for them as well.
> - *
> - * The size calculation is the entire vma while each arg page is
> - * built, so each time we get here it's calculating how far it
> - * is currently (rather than each call being just the newly
> - * added size from the arg page). As a result, we need to
> - * always add the entire size of the pointers, so that on the
> - * last call to get_arg_page() we'll actually have the entire
> - * correct size.
> - */
> - ptr_size = (bprm->argc + bprm->envc) * sizeof(void *);
> - if (ptr_size > ULONG_MAX - size)
> - goto fail;
> - size += ptr_size;
> -
> - acct_arg_size(bprm, size / PAGE_SIZE);
> -
> - /*
> - * We've historically supported up to 32 pages (ARG_MAX)
> - * of argument strings even with small stacks
> - */
> - if (size <= ARG_MAX)
> - return page;
> -
> - /*
> - * Limit to 1/4 of the max stack size or 3/4 of _STK_LIM
> - * (whichever is smaller) for the argv+env strings.
> - * This ensures that:
> - * - the remaining binfmt code will not run out of stack space,
> - * - the program will have a reasonable amount of stack left
> - * to work from.
> - */
> - limit = _STK_LIM / 4 * 3;
> - limit = min(limit, bprm->rlim_stack.rlim_cur / 4);
> - if (size > limit)
> - goto fail;
> - }
> + if (write)
> + acct_arg_size(bprm, vma_pages(bprm->vma));
>
> return page;
> -
> -fail:
> - put_page(page);
> - return NULL;
> }
>
> static void put_arg_page(struct page *page)
> @@ -410,11 +365,6 @@ static int bprm_mm_init(struct linux_binprm *bprm)
> if (!mm)
> goto err;
>
> - /* Save current stack limit for all calculations made during exec. */
> - task_lock(current->group_leader);
> - bprm->rlim_stack = current->signal->rlim[RLIMIT_STACK];
> - task_unlock(current->group_leader);
> -
I would prefer this hunk stay here since it will be more robust
against weird arch-specific things happening against rlim_stack. I had
to clean up some of these tests in arch code, so I'm nervous about
moving this further away. Here is before we call arch_bprm_mm_init(),
and I think it's better to do this as early as possible.
> err = __bprm_mm_init(bprm);
> if (err)
> goto err;
> @@ -492,6 +442,27 @@ static int count(struct user_arg_ptr argv, int max)
> return i;
> }
>
> +static int prepare_rlim_stack(struct linux_binprm *bprm)
How about collapsing this with:
bprm->argc = count(argv, MAX_ARG_STRINGS);
if ((retval = bprm->argc) < 0)
goto out;
bprm->envc = count(envp, MAX_ARG_STRINGS);
if ((retval = bprm->envc) < 0)
goto out;
and call it prepare_arg_count(struct linux_binprm *bprm,
struct user_arg_ptr argv,
struct user_arg_ptr envp)
> +{
> + unsigned long limit, ptr_size;
> +
> + task_lock(current->group_leader);
> + bprm->rlim_stack = current->signal->rlim[RLIMIT_STACK];
> + task_unlock(current->group_leader);
> +
Let's try to retain the comments here:
/*
* Limit to 1/4 of the max stack size or 3/4 of _STK_LIM
* (whichever is smaller) for the argv+env strings.
* This ensures that:
* - the remaining binfmt code will not run out of stack space,
* - the program will have a reasonable amount of stack left
* to work from.
*/
> + limit = _STK_LIM / 4 * 3;
> + limit = min(limit, bprm->rlim_stack.rlim_cur / 4);
and here:
/*
* We've historically supported up to 32 pages (ARG_MAX)
* of argument strings even with small stacks
*/
> + limit = max(limit, (unsigned long)ARG_MAX);
> + /* COMMENT */
This comment should likely be something like:
/*
* 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
* the stack. They aren't stored until much later when we can't
* signal to the parent that the child has run out of stack space.
* Instead, calculate it here so it's possible to fail gracefully.
*/
> + ptr_size = (bprm->argc + bprm->envc) * sizeof(void *);
BTW, in re-reading create_elf_tables() and its calculation of "items",
I realize the above should actually include the trailing NULL pointers
and argc, so it should be:
ptr_size = (1 + bprm->argc + 1 + bprm->envc + 1) * sizeof(void *);
> + if (limit <= ptr_size)
> + return -E2BIG;
> + limit -= ptr_size;
> +
> + bprm->p_min = bprm->p - limit;
> + return 0;
> +}
> +
> /*
> * 'copy_strings()' copies argument/environment strings from the old
> * processes's memory to the new process's stack. The call to get_user_pages()
> @@ -527,6 +498,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
> pos = bprm->p;
> str += len;
> bprm->p -= len;
> + if (bprm->p <= bprm->p_min)
> + goto out;
>
> while (len > 0) {
> int offset, bytes_to_copy;
> @@ -1801,6 +1774,10 @@ static int __do_execve_file(int fd, struct filename *filename,
> if (retval < 0)
> goto out;
>
> + retval = prepare_rlim_stack(bprm);
> + if (retval < 0)
> + goto out;
> +
> retval = copy_strings_kernel(1, &bprm->filename, bprm);
> if (retval < 0)
> goto out;
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index c05f24f..423e8c1 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -24,7 +24,7 @@ struct linux_binprm {
> struct page *page[MAX_ARG_PAGES];
> #endif
> struct mm_struct *mm;
> - unsigned long p; /* current top of mem */
> + unsigned long p, p_min; /* current top of mem */
Can you split this out to a separate line (with a new comment) to
avoid comment-confusion? Something like:
unsigned long p; /* current top of mem */
unsigned long p_min; /* the minimum allowed mem position */
> unsigned int
> /*
> * True after the bprm_set_creds hook has been called once
>
I've also spent some more time convincing myself again that there
aren't races between count(), copy_strings(), and create_elf_tables().
A malicious parent would only be able to zero-fill the stack of the
child, but not escape the counts that I can see.
-Kees
--
Kees Cook
Pixel Security
Powered by blists - more mailing lists