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: <CAGXu5jKYVAfWkd3WTM4EkVHWGoUY9BtSGKMLb1RAKhUfgCVxfA@mail.gmail.com>
Date:   Mon, 10 Sep 2018 09:41:08 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Oleg Nesterov <oleg@...hat.com>
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 Mon, Sep 10, 2018 at 5:29 AM, Oleg Nesterov <oleg@...hat.com> 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