Re: get_arg_page() && ptr_size accounting

[Kees Cook]

On Mon, Sep 10, 2018 at 5:29 AM, Oleg Nesterov <oleg@xxxxxxxxxx> wrote:
> Hi Kees,
>
> I was thinking about backporting the commit 98da7d08850fb8bde
> ("fs/exec.c: account for argv/envp pointers"), but I am not sure
> I understand it...
>
> So get_arg_page() does
>
>                 /*
>                  * 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;
>
> OK, but
>                 acct_arg_size(bprm, size / PAGE_SIZE);
>
> after that doesn't look exactly right. This additional space will be used later
> when the process already uses bprm->mm, right? so it shouldn't be accounted by
> acct_arg_size().

My understanding (based on the comment about acct_arg_size()) is that
before exec_mmap() happens, the memory used to build the new arguments
copy memory area gets accounted to the MM_ANONPAGES resource limit of
the execing process. I couldn't find any place where the argc/envc
pointers were being included in the count, so I believe this needs to
stay as-is otherwise it's a resource limit bypass.

> Not to mention that ptr_size/PAGE_SIZE doesn't look right in any case...

Hm? acct_arg_size() takes pages, not bytes. I think this is correct?
What doesn't look right to you?

> In short. Am I totally confused or the patch below makes sense? This way we do
> not need the fat comment.

Even if I'm wrong about acct_arg_size(), we need to keep the comment
because it still applies to "size" and the following ARG_MAX test and
the _STK_LIM test. I think it's very non-obvious that we need to
always keep the full argc/envc count each time we go through these
calculations.

-Kees

-- 
Kees Cook
Pixel Security