linux-hardening - [PATCH v2 1/2] execve: Keep bprm->argmin behind CONFIG

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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20240621205046.4001362-1-kees@kernel.org>
Date: Fri, 21 Jun 2024 13:50:43 -0700
From: Kees Cook <kees@...nel.org>
To: Guenter Roeck <linux@...ck-us.net>
Cc: Kees Cook <kees@...nel.org>,
	Eric Biederman <ebiederm@...ssion.com>,
	Alexander Viro <viro@...iv.linux.org.uk>,
	Christian Brauner <brauner@...nel.org>,
	Jan Kara <jack@...e.cz>,
	Alexey Dobriyan <adobriyan@...il.com>,
	Laurent Vivier <laurent@...ier.eu>,
	Lukas Bulwahn <lukas.bulwahn@...il.com>,
	linux-fsdevel@...r.kernel.org,
	linux-mm@...ck.org,
	Justin Stitt <justinstitt@...gle.com>,
	linux-kernel@...r.kernel.org,
	linux-hardening@...r.kernel.org
Subject: [PATCH v2 1/2] execve: Keep bprm->argmin behind CONFIG_MMU

When argmin was added in commit 655c16a8ce9c ("exec: separate
MM_ANONPAGES and RLIMIT_STACK accounting"), it was intended only for
validating stack limits on CONFIG_MMU[1]. All checking for reaching the
limit (argmin) is wrapped in CONFIG_MMU ifdef checks, though setting
argmin was not. That argmin is only supposed to be used under CONFIG_MMU
was rediscovered recently[2], and I don't want to trip over this again.

Move argmin's declaration into the existing CONFIG_MMU area, and add
helpers functions so the MMU tests can be consolidated.

Link: https://lore.kernel.org/all/20181126122307.GA1660@redhat.com [1]
Link: https://lore.kernel.org/all/202406211253.7037F69@keescook/ [2]
Signed-off-by: Kees Cook <kees@...nel.org>
---
Cc: Guenter Roeck <linux@...ck-us.net>
Cc: Eric Biederman <ebiederm@...ssion.com>
Cc: Alexander Viro <viro@...iv.linux.org.uk>
Cc: Christian Brauner <brauner@...nel.org>
Cc: Jan Kara <jack@...e.cz>
Cc: Alexey Dobriyan <adobriyan@...il.com>
Cc: Laurent Vivier <laurent@...ier.eu>
Cc: Lukas Bulwahn <lukas.bulwahn@...il.com>
Cc: linux-fsdevel@...r.kernel.org
Cc: linux-mm@...ck.org
---
 fs/exec.c               | 26 ++++++++++++++++++++------
 fs/exec_test.c          |  2 ++
 include/linux/binfmts.h |  2 +-
 3 files changed, 23 insertions(+), 7 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index c3bec126505b..b7bc63bfb907 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -486,6 +486,23 @@ static int count_strings_kernel(const char *const *argv)
 	return i;
 }
 
+static inline int bprm_set_stack_limit(struct linux_binprm *bprm,
+				       unsigned long limit)
+{
+#ifdef CONFIG_MMU
+	bprm->argmin = bprm->p - limit;
+#endif
+	return 0;
+}
+static inline bool bprm_hit_stack_limit(struct linux_binprm *bprm)
+{
+#ifdef CONFIG_MMU
+	return bprm->p < bprm->argmin;
+#else
+	return false;
+#endif
+}
+
 /*
  * Calculate bprm->argmin from:
  * - _STK_LIM
@@ -532,8 +549,7 @@ static int bprm_stack_limits(struct linux_binprm *bprm)
 		return -E2BIG;
 	limit -= ptr_size;
 
-	bprm->argmin = bprm->p - limit;
-	return 0;
+	return bprm_set_stack_limit(bprm, limit);
 }
 
 /*
@@ -571,10 +587,8 @@ static int copy_strings(int argc, struct user_arg_ptr argv,
 		pos = bprm->p;
 		str += len;
 		bprm->p -= len;
-#ifdef CONFIG_MMU
-		if (bprm->p < bprm->argmin)
+		if (bprm_hit_stack_limit(bprm))
 			goto out;
-#endif
 
 		while (len > 0) {
 			int offset, bytes_to_copy;
@@ -649,7 +663,7 @@ int copy_string_kernel(const char *arg, struct linux_binprm *bprm)
 	/* We're going to work our way backwards. */
 	arg += len;
 	bprm->p -= len;
-	if (IS_ENABLED(CONFIG_MMU) && bprm->p < bprm->argmin)
+	if (bprm_hit_stack_limit(bprm))
 		return -E2BIG;
 
 	while (len > 0) {
diff --git a/fs/exec_test.c b/fs/exec_test.c
index 32a90c6f47e7..8fea0bf0b7f5 100644
--- a/fs/exec_test.c
+++ b/fs/exec_test.c
@@ -96,7 +96,9 @@ static void exec_test_bprm_stack_limits(struct kunit *test)
 
 		rc = bprm_stack_limits(&bprm);
 		KUNIT_EXPECT_EQ_MSG(test, rc, result->expected_rc, "on loop %d", i);
+#ifdef CONFIG_MMU
 		KUNIT_EXPECT_EQ_MSG(test, bprm.argmin, result->expected_argmin, "on loop %d", i);
+#endif
 	}
 }
 
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 70f97f685bff..e6c00e860951 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -19,13 +19,13 @@ struct linux_binprm {
 #ifdef CONFIG_MMU
 	struct vm_area_struct *vma;
 	unsigned long vma_pages;
+	unsigned long argmin; /* rlimit marker for copy_strings() */
 #else
 # define MAX_ARG_PAGES	32
 	struct page *page[MAX_ARG_PAGES];
 #endif
 	struct mm_struct *mm;
 	unsigned long p; /* current top of mem */
-	unsigned long argmin; /* rlimit marker for copy_strings() */
 	unsigned int
 		/* Should an execfd be passed to userspace? */
 		have_execfd:1,
-- 
2.34.1