linux-kernel - [PATCH] kernel/fork: stop playing lockless games for exe

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]

Date:   Sun, 13 Aug 2023 14:33:33 +0200
From:   Mateusz Guzik <mjguzik@...il.com>
To:     linux-kernel@...r.kernel.org
Cc:     torvalds@...ux-foundation.org, brauner@...nel.org,
        ebiederm@...ssion.com, david@...hat.com, akpm@...ux-foundation.org,
        linux-mm@...ck.org, koct9i@...il.com, oleg@...hat.com,
        dave@...olabs.net, Mateusz Guzik <mjguzik@...il.com>
Subject: [PATCH] kernel/fork: stop playing lockless games for exe_file replacement

xchg originated in 6e399cd144d8 ("prctl: avoid using mmap_sem for
exe_file serialization"). While the commit message does not explain
*why* the change, clearly the intent was to use mmap_sem less in this
codepath. I found the original submission [1] which ultimately claims it
cleans things up.

However, replacement itself takes it for reading before doing any work
and other places associated with it also take it.

fe69d560b5bd ("kernel/fork: always deny write access to current MM
exe_file") added another lock trip to synchronize the state of exe_file
against fork, further defeating the point of xchg.

As such I think the atomic here only adds complexity for no benefit.

Just write-lock around the replacement.

I also note that replacement races against the mapping check loop as
nothing synchronizes actual assignment with with said checks but I am
not addressing it in this patch. (Is the loop of any use to begin with?)

Link: https://lore.kernel.org/linux-mm/1424979417.10344.14.camel@stgolabs.net/ [1]
Signed-off-by: Mateusz Guzik <mjguzik@...il.com>
---
 kernel/fork.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/kernel/fork.c b/kernel/fork.c
index d2e12b6d2b18..f576ce341e43 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1464,22 +1464,20 @@ int replace_mm_exe_file(struct mm_struct *mm, struct file *new_exe_file)
 			return ret;
 	}

-	/* set the new file, lockless */
 	ret = deny_write_access(new_exe_file);
 	if (ret)
 		return -EACCES;
 	get_file(new_exe_file);

-	old_exe_file = xchg(&mm->exe_file, new_exe_file);
+	/* set the new file */
+	mmap_write_lock(mm);
+	old_exe_file = rcu_dereference_raw(mm->exe_file);
+	rcu_assign_pointer(mm->exe_file, new_exe_file);
+	mmap_write_unlock(mm);
+
 	if (old_exe_file) {
-		/*
-		 * Don't race with dup_mmap() getting the file and disallowing
-		 * write access while someone might open the file writable.
-		 */
-		mmap_read_lock(mm);
 		allow_write_access(old_exe_file);
 		fput(old_exe_file);
-		mmap_read_unlock(mm);
 	}
 	return 0;
 }
-- 
2.39.2