On Mon, Sep 10, 2018 at 5:29 AM, Oleg Nesterov <[email protected]> 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
