Re: get_arg_page() && ptr_size accounting
From: Oleg Nesterov
Date: Mon Sep 10 2018 - 13:18:29 EST
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