lkml.org 
[lkml]   [2018]   [Sep]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: get_arg_page() && ptr_size accounting
On 09/10, Kees Cook wrote:
>
> > 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.

Yes, because otherwise oom-killer can't account the memory populated by
get_arg_page() in bprm->mm.

> I couldn't find any place where the argc/envc
> pointers were being included in the count,

But why??? To clarify,

size += ptr_size;

after acct_arg_size() is clear and correct, we are going to check rlim_stack
and thus the size should include the pointers we will add in create_elf_tables().


But acct_arg_size() should only account the pages we allocate for bprm->mm,
nothing more. create_elf_tables() does not allocate the memory when it populates
arg_start/arg_end/env_start/env_end. Plus at this time the process has already
switched to bprm->mm.

> > 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?

Please forget. I meant that _if_ we actually wanted to account this additional
memory in bprm->pages, than we would probably need something like
acct_arg_size(size/PAGE_SIZE + DIV_ROUND_UP(ptr_size, PAGE_SIZE)).

> > 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

I won't argue, but to me evrything looks obvious as long as we don't pass
ptr_size acct_arg_size().

Oleg.

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