linux-kernel - Re: get_arg_page() && ptr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180910171822.GA27005@redhat.com>
Date:   Mon, 10 Sep 2018 19:18:22 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Rik van Riel <riel@...hat.com>, Michal Hocko <mhocko@...e.com>,
        Stanislav Kozina <skozina@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
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.