Messages in this thread | | | Date | Mon, 10 Sep 2018 19:18:22 +0200 | From | Oleg Nesterov <> | Subject | Re: 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.
| |