mm/secretmem: one fix and one refactoring

folio_is_secretmem() currently relies on secretmem folios being LRU folios, to save some cycles.

However, folios might reside in a folio batch without the LRU flag set, or temporarily have their LRU flag cleared. Consequently, the LRU flag is unreliable for this purpose.

In particular, this is the case when secretmem_fault() allocates a fresh page and calls filemap_add_folio()->folio_add_lru(). The folio might be added to the per-cpu folio batch and won't get the LRU flag set until the batch was drained using e.g., lru_add_drain().

Consequently, folio_is_secretmem() might not detect secretmem folios and GUP-fast can succeed in grabbing a secretmem folio, crashing the kernel when we would later try reading/writing to the folio, because the folio has been unmapped from the directmap.

Fix it by removing that unreliable check.

Reported-by xingwei lee <[email protected]>

Reported-by yue sun <[email protected]>

Closes https://lore.kernel.org/lkml/CABOYnLyevJeravW=QrH0JUPYEcDN160aZFb7kwndm-J2rmz0HQ@mail.gmail.com/ open here

Debugged-by Miklos Szeredi <[email protected]>

Tested-by Miklos Szeredi <[email protected]>

Fixes 1507f51255c9 ("mm: introduce memfd_secret system call to create "secret" memory areas")

Cc <[email protected]>

Signed-off-by David Hildenbrand <[email protected]>

---
 include/linux/secretmem.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/linux/secretmem.h +2 -2

index 35f3a4a8ceb1..acf7e1a3f3de 100644
@@ -13,10 +13,10 @@ static inline bool folio_is_secretmem(struct folio *folio)
 	/*
 	 * Using folio_mapping() is quite slow because of the actual call
 	 * instruction.
-	 * We know that secretmem pages are not compound and LRU so we can
+	 * We know that secretmem pages are not compound, so we can
 	 * save a couple of cycles here.
 	 */
-	if (folio_test_large(folio) || !folio_test_lru(folio))
+	if (folio_test_large(folio))
 		return false;
 
 	mapping = (struct address_space *)

Signature · 1 line

2.43.2

Let's add a simple reproducer for a scenario where GUP-fast could succeed on secretmem folios, making vmsplice() succeed instead of failing. The reproducer is based on a reproducer [1] by Miklos Szeredi.

We want to perform two tests: vmsplice() when a fresh page was just faulted in, and vmsplice() on an existing page after munmap() that would drain certain LRU caches/batches in the kernel.

In an ideal world, we could use fallocate(FALLOC_FL_PUNCH_HOLE) / MADV_REMOVE to remove any existing page. As that is currently not possible, run the test before any other tests that would allocate memory in the secretmem fd.

Perform the ftruncate() only once, and check the return value.

[1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com

Signed-off-by David Hildenbrand <[email protected]>

---
 tools/testing/selftests/mm/memfd_secret.c | 51 ++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 2 deletions(-)

tools/testing/selftests/mm/memfd_secret.c +49 -2

index 9b298f6a04b3..9a0597310a76 100644
@@ -20,6 +20,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <stdio.h>
+#include <fcntl.h>
 
 #include "../kselftest.h"
 

@@ -83,6 +84,45 @@ static void test_mlock_limit(int fd)
 	pass("mlock limit is respected\n");
 }
 
+static void test_vmsplice(int fd, const char *desc)
+{
+	ssize_t transferred;
+	struct iovec iov;
+	int pipefd[2];
+	char *mem;
+
+	if (pipe(pipefd)) {
+		fail("pipe failed: %s\n", strerror(errno));
+		return;
+	}
+
+	mem = mmap(NULL, page_size, prot, mode, fd, 0);
+	if (mem == MAP_FAILED) {
+		fail("Unable to mmap secret memory\n");
+		goto close_pipe;
+	}
+
+	/*
+	 * vmsplice() may use GUP-fast, which must also fail. Prefault the
+	 * page table, so GUP-fast could find it.
+	 */
+	memset(mem, PATTERN, page_size);
+
+	iov.iov_base = mem;
+	iov.iov_len = page_size;
+	transferred = vmsplice(pipefd[1], &iov, 1, 0);
+
+	if (transferred < 0 && errno == EFAULT)
+		pass("vmsplice is blocked as expected with %s\n", desc);
+	else
+		fail("vmsplice: unexpected memory access with %s\n", desc);
+
+	munmap(mem, page_size);
+close_pipe:
+	close(pipefd[0]);
+	close(pipefd[1]);
+}
+
 static void try_process_vm_read(int fd, int pipefd[2])
 {
 	struct iovec liov, riov;

@@ -187,7 +227,6 @@ static void test_remote_access(int fd, const char *name,
 		return;
 	}
 
-	ftruncate(fd, page_size);
 	memset(mem, PATTERN, page_size);
 
 	if (write(pipefd[1], &mem, sizeof(mem)) < 0) {

@@ -258,7 +297,7 @@ static void prepare(void)
 				   strerror(errno));
 }
 
-#define NUM_TESTS 4
+#define NUM_TESTS 6
 
 int main(int argc, char *argv[])
 {

@@ -277,9 +316,17 @@ int main(int argc, char *argv[])
 			ksft_exit_fail_msg("memfd_secret failed: %s\n",
 					   strerror(errno));
 	}
+	if (ftruncate(fd, page_size))
+		ksft_exit_fail_msg("ftruncate failed: %s\n", strerror(errno));
 
 	test_mlock_limit(fd);
 	test_file_apis(fd);
+	/*
+	 * We have to run the first vmsplice test before any secretmem page was
+	 * allocated for this fd.
+	 */
+	test_vmsplice(fd, "fresh page");
+	test_vmsplice(fd, "existing page");
 	test_process_vm_read(fd);
 	test_ptrace(fd);
 

Signature · 1 line

2.43.2

Let's add a simple reproducer for a scenario where GUP-fast could succeed on secretmem folios, making vmsplice() succeed instead of failing. The reproducer is based on a reproducer [1] by Miklos Szeredi.

We want to perform two tests: vmsplice() when a fresh page was just faulted in, and vmsplice() on an existing page after munmap() that would drain certain LRU caches/batches in the kernel.

In an ideal world, we could use fallocate(FALLOC_FL_PUNCH_HOLE) / MADV_REMOVE to remove any existing page. As that is currently not possible, run the test before any other tests that would allocate memory in the secretmem fd.

Perform the ftruncate() only once, and check the return value.

[1] https://lkml.kernel.org/r/CAJfpegt3UCsMmxd0taOY11Uaw5U=eS1fE5dn0wZX3HF0oy8-oQ@mail.gmail.com

Signed-off-by David Hildenbrand <[email protected]>

---
 tools/testing/selftests/mm/memfd_secret.c | 51 ++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 2 deletions(-)

tools/testing/selftests/mm/memfd_secret.c +14 -0

index 9b298f6a04b3..9a0597310a76 100644
@@ -20,6 +20,7 @@
 #include <unistd.h>
 #include <errno.h>
 #include <stdio.h>
+#include <fcntl.h>
 
 #include "../kselftest.h"
 

@@ -83,6 +84,45 @@ static void test_mlock_limit(int fd)
        pass("mlock limit is respected\n");
 }
 
+static void test_vmsplice(int fd, const char *desc)
+{
+       ssize_t transferred;
+       struct iovec iov;
+       int pipefd[2];
+       char *mem;
+
+       if (pipe(pipefd)) {
+               fail("pipe failed: %s\n", strerror(errno));
+               return;
+       }
+
+       mem = mmap(NULL, page_size, prot, mode, fd, 0);

Quoted 17 lines

tools/testing/selftests/mm/memfd_secret.c +17 -0

+static void test_vmsplice(int fd, const char *desc)
+{
+       ssize_t transferred;
+       struct iovec iov;
+       int pipefd[2];
+       char *mem;
+
+       if (pipe(pipefd)) {
+               fail("pipe failed: %s\n", strerror(errno));
+               return;
+       }
+
+       mem = mmap(NULL, page_size, prot, mode, fd, 0);
+       if (mem == MAP_FAILED) {
+               fail("Unable to mmap secret memory\n");
+               goto close_pipe;
+       }

tools/testing/selftests/mm/memfd_secret.c static void test_mlock_limit(int fd) · line 109

Quoted 6 lines

tools/testing/selftests/mm/memfd_secret.c +6 -0

+
+       /*
+        * vmsplice() may use GUP-fast, which must also fail. Prefault the
+        * page table, so GUP-fast could find it.
+        */
+       memset(mem, PATTERN, page_size);

Shouldn't the non-prefault case be tested as well?