mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
To: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Cc: kosaki.motohiro@jp.fujitsu.com,
	Roland McGrath <roland@redhat.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-kernel@vger.kernel.org, oss-security@lists.openwall.com,
	Solar Designer <solar@openwall.com>,
	Kees Cook <kees.cook@canonical.com>,
	Al Viro <viro@zeniv.linux.org.uk>,
	Oleg Nesterov <oleg@redhat.com>,
	Neil Horman <nhorman@tuxdriver.com>,
	linux-fsdevel@vger.kernel.org, pageexec@freemail.hu,
	"Brad Spengler <spender@grsecurity.net>,
	Eugene Teo"  <eugene@redhat.com>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Subject: [PATCH 2/2] execve: check the VM has enough memory at first
Date: Thu,  9 Sep 2010 14:04:42 +0900 (JST)	[thread overview]
Message-ID: <20100909140439.C945.A69D9226@jp.fujitsu.com> (raw)
In-Reply-To: <20100909134842.C93F.A69D9226@jp.fujitsu.com>

Now, Argv size of execve are limited by STACK_LIMIT/4. In other
words, If we are setting 'ulimit -s unlimited', we've lost any
guard of argv size.

More unfortunately, current argv setup logic is bypassing the VM
overcommitment check unintentionally. because current overcommitment
check don't care gradually increased stack.
therefore, wrong argument of execve() easily makes OOM instead
execve() failure. that's bad.

After this patch, execve() expand stack at first and receive to
check vm_enough_memory() properly. then, too long argument of
execve() than the machine memory return EFAULT properly.

Note, almost all user are using OVERCOMMIT_GUESS overcommit mode
(because it's default). It provide only guess. It doesn't works
perfectly on some case. However usually execve() failure is better
than OOM-killer and swap-storm compbination.

Cc: pageexec@freemail.hu
Cc: Roland McGrath <roland@redhat.com>
Cc: Solar Designer <solar@openwall.com>
Cc: Brad Spengler <spender@grsecurity.net>
Cc: Eugene Teo <eteo@redhat.com>
Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
---
 fs/compat.c |   38 +++++++++++++++++++++++++++++++-------
 fs/exec.c   |   38 +++++++++++++++++++++++++++++++-------
 2 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index b631120..ff32573 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1394,10 +1394,39 @@ static int compat_copy_strings(int argc, compat_uptr_t __user *argv,
 	char *kaddr = NULL;
 	unsigned long kpos = 0;
 	int ret;
+	compat_uptr_t str;
+	int len;
+	int i;
+	unsigned long total_len = 0;
+
+	for (i = 0; i < argc; i++) {
+		ret = -EFAULT;
+		if (get_user(str, argv+i))
+			goto out;
+		len = strnlen_user(compat_ptr(str), MAX_ARG_STRLEN);
+		if (!len)
+			goto out;
+
+		ret = -E2BIG;
+		if (len > MAX_ARG_STRLEN)
+			goto out;
+
+		total_len += len;
+		if (total_len > bprm->p)
+			goto out;
+	}
+
+	/*
+	 * Firstly, we try to expand stack. It also invoke
+	 * security_vm_enough_memory() and get failure when we don't
+	 * have enough space. It help to avoid stack smashing by plenty argv.
+	 */
+	ret = get_user_pages(current, bprm->mm, bprm->p - total_len,
+			     1, 1, 1, NULL, NULL);
+	if (ret < 0)
+		goto out;
 
 	while (argc-- > 0) {
-		compat_uptr_t str;
-		int len;
 		unsigned long pos;
 
 		if (get_user(str, argv+argc) ||
@@ -1406,11 +1435,6 @@ static int compat_copy_strings(int argc, compat_uptr_t __user *argv,
 			goto out;
 		}
 
-		if (len > MAX_ARG_STRLEN) {
-			ret = -E2BIG;
-			goto out;
-		}
-
 		/* We're going to work our way backwords. */
 		pos = bprm->p;
 		str += len;
diff --git a/fs/exec.c b/fs/exec.c
index b41834c..ef8b9dc 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -396,10 +396,39 @@ static int copy_strings(int argc, const char __user *const __user *argv,
 	char *kaddr = NULL;
 	unsigned long kpos = 0;
 	int ret;
+	int i;
+	unsigned long total_len = 0;
+	const char __user *str;
+	int len;
+
+	for (i = 0; i < argc; i++) {
+		ret = -EFAULT;
+		if (get_user(str, argv+i))
+			goto out;
+		len = strnlen_user(str, MAX_ARG_STRLEN);
+		if (!len)
+			goto out;
+
+		ret = -E2BIG;
+		if (!valid_arg_len(bprm, len))
+			goto out;
+
+		total_len += len;
+		if (total_len > bprm->p)
+			goto out;
+	}
+
+	/*
+	 * Firstly, we try to expand stack. It also invoke
+	 * security_vm_enough_memory() and get failure when we don't
+	 * have enough space. It help to avoid stack smashing by plenty argv.
+	 */
+	ret = get_user_pages(current, bprm->mm, bprm->p - total_len,
+			     1, 1, 1, NULL, NULL);
+	if (ret < 0)
+		goto out;
 
 	while (argc-- > 0) {
-		const char __user *str;
-		int len;
 		unsigned long pos;
 
 		if (get_user(str, argv+argc) ||
@@ -408,11 +437,6 @@ static int copy_strings(int argc, const char __user *const __user *argv,
 			goto out;
 		}
 
-		if (!valid_arg_len(bprm, len)) {
-			ret = -E2BIG;
-			goto out;
-		}
-
 		/* We're going to work our way backwords. */
 		pos = bprm->p;
 		str += len;
-- 
1.6.5.2




  parent reply	other threads:[~2010-09-09  5:04 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-08-27 22:02 [PATCH] exec argument expansion can inappropriately trigger OOM-killer Kees Cook
2010-08-30  0:19 ` KOSAKI Motohiro
2010-08-30  0:56 ` Roland McGrath
2010-08-30  3:23   ` Solar Designer
2010-08-30 10:06     ` Roland McGrath
2010-08-30 19:48       ` Solar Designer
2010-08-31  0:40         ` Roland McGrath
2010-09-08  2:34       ` [PATCH 0/3] execve argument-copying fixes Roland McGrath
2010-09-08  2:35         ` [PATCH 1/3] setup_arg_pages: diagnose excessive argument size Roland McGrath
2010-09-08  8:29           ` pageexec
2010-09-10  8:59             ` Roland McGrath
2010-09-11 13:30               ` pageexec
2010-09-14 19:33                 ` Roland McGrath
2010-09-14 22:35                   ` pageexec
2010-09-08 11:57           ` Brad Spengler
2010-09-09  5:31             ` KOSAKI Motohiro
2010-09-10  9:25               ` Roland McGrath
2010-09-10  9:43                 ` KOSAKI Motohiro
2010-09-11 13:39                 ` pageexec
2010-09-14 18:51                   ` Roland McGrath
2010-09-14 20:28                     ` pageexec
2010-09-14 21:16                       ` Roland McGrath
2010-09-14 22:27                         ` pageexec
2010-09-14 23:04                           ` Roland McGrath
2010-09-15  9:27                             ` pageexec
2010-09-10  9:18             ` Roland McGrath
2010-09-08  2:36         ` [PATCH 2/3] execve: improve interactivity with large arguments Roland McGrath
2010-09-08  2:37         ` [PATCH 3/3] execve: make responsive to SIGKILL " Roland McGrath
2010-09-08  3:00         ` [PATCH 0/3] execve argument-copying fixes KOSAKI Motohiro
2010-09-09  5:01         ` [PATCH 0/2] execve memory exhaust of " KOSAKI Motohiro
2010-09-09  5:03           ` [PATCH 1/2] oom: don't ignore rss in nascent mm KOSAKI Motohiro
2010-09-09 22:05             ` Oleg Nesterov
2010-09-10  9:39               ` Roland McGrath
2010-09-10  9:57               ` [PATCH] move cred_guard_mutex from task_struct to signal_struct KOSAKI Motohiro
2010-09-10 17:24                 ` Oleg Nesterov
2010-09-16  5:51                   ` KOSAKI Motohiro
2010-09-09  5:04           ` KOSAKI Motohiro [this message]
2010-09-10 15:06             ` [PATCH 2/2] execve: check the VM has enough memory at first Linus Torvalds
2010-09-14  1:52               ` KOSAKI Motohiro
2010-09-16  5:51                 ` KOSAKI Motohiro
2010-09-16 15:01                   ` Linus Torvalds
2010-08-30 17:49     ` [PATCH] exec argument expansion can inappropriately trigger OOM-killer Solar Designer
2010-08-30 22:08       ` Brad Spengler
2010-08-31 11:53         ` Solar Designer
2010-08-31 11:56         ` [PATCH] exec argument expansion can inappropriately triggerOOM-killer Tetsuo Handa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20100909140439.C945.A69D9226@jp.fujitsu.com \
    --to=kosaki.motohiro@jp.fujitsu.com \
    --cc=akpm@linux-foundation.org \
    --cc=eugene@redhat.com \
    --cc=kamezawa.hiroyu@jp.fujitsu.com \
    --cc=kees.cook@canonical.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nhorman@tuxdriver.com \
    --cc=oleg@redhat.com \
    --cc=oss-security@lists.openwall.com \
    --cc=pageexec@freemail.hu \
    --cc=roland@redhat.com \
    --cc=solar@openwall.com \
    --cc=torvalds@linux-foundation.org \
    --cc=viro@zeniv.linux.org.uk \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®