linux-kernel - 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20180910122907.GA23963@redhat.com>
Date:   Mon, 10 Sep 2018 14:29:07 +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>,
        linux-kernel@...r.kernel.org
Subject: get_arg_page() && ptr_size accounting

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().

Not to mention that ptr_size/PAGE_SIZE doesn't look right in any case...

In short. Am I totally confused or the patch below makes sense? This way we do
not need the fat comment.

Oleg.

--- x/fs/exec.c
+++ x/fs/exec.c
@@ -222,25 +222,17 @@ static struct page *get_arg_page(struct
 		unsigned long size = bprm->vma->vm_end - bprm->vma->vm_start;
 		unsigned long ptr_size, limit;
 
+		acct_arg_size(bprm, size / PAGE_SIZE);
+
 		/*
 		 * 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;
 
-		acct_arg_size(bprm, size / PAGE_SIZE);
-
 		/*
 		 * We've historically supported up to 32 pages (ARG_MAX)
 		 * of argument strings even with small stacks