lkml.org 
[lkml]   [2018]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: get_arg_page() && ptr_size accounting
On Mon, Sep 10, 2018 at 5:29 AM, Oleg Nesterov <oleg@redhat.com> 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

\
 
 \ /
  Last update: 2018-09-10 18:42    [W:0.118 / U:0.164 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site