[PATCH 0/4] mm/memfd: MFD_NOEXEC for memfd_create

[PATCH 0/4] mm/memfd: MFD_NOEXEC for memfd_create

[Daniel Verkamp]

The default file permissions on a memfd include execute bits, which
means that such a memfd can be filled with a executable and passed to
the exec() family of functions. This is undesirable on systems where all
code is verified and all filesystems are intended to be mounted noexec,
since an attacker may be able to use a memfd to load unverified code and
execute it.

Additionally, execution via memfd is a common way to avoid scrutiny for
malicious code, since it allows execution of a program without a file
ever appearing on disk. This attack vector is not totally mitigated with
this new flag, since the default memfd file permissions must remain
executable to avoid breaking existing legitimate uses, but it should be
possible to use other security mechanisms to prevent memfd_create calls
without MFD_NOEXEC on systems where it is known that executable memfds
are not necessary.

This patch series adds a new MFD_NOEXEC flag for memfd_create(), which
allows creation of non-executable memfds, and as part of the
implementation of this new flag, it also adds a new F_SEAL_EXEC seal,
which will prevent modification of any of the execute bits of a sealed
memfd.

I am not sure if this is the best way to implement the desired behavior
(for example, the F_SEAL_EXEC seal is really more of an implementation
detail and feels a bit clunky to expose), so suggestions are welcome
for alternate approaches.

Daniel Verkamp (4):
  mm/memfd: add F_SEAL_EXEC
  mm/memfd: add MFD_NOEXEC flag to memfd_create
  selftests/memfd: add tests for F_SEAL_EXEC
  selftests/memfd: add tests for MFD_NOEXEC

 include/uapi/linux/fcntl.h                 |   1 +
 include/uapi/linux/memfd.h                 |   1 +
 mm/memfd.c                                 |  12 ++-
 mm/shmem.c                                 |   6 ++
 tools/testing/selftests/memfd/memfd_test.c | 114 +++++++++++++++++++++
 5 files changed, 133 insertions(+), 1 deletion(-)

-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 1/4] mm/memfd: add F_SEAL_EXEC

[Daniel Verkamp]

The new F_SEAL_EXEC flag will prevent modification of the exec bits:
written as traditional octal mask, 0111, or as named flags, S_IXUSR |
S_IXGRP | S_IXOTH. Any chmod(2) or similar call that attempts to modify
any of these bits after the seal is applied will fail with errno EPERM.

This will preserve the execute bits as they are at the time of sealing,
so the memfd will become either permanently executable or permanently
un-executable.

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 include/uapi/linux/fcntl.h | 1 +
 mm/memfd.c                 | 2 ++
 mm/shmem.c                 | 6 ++++++
 3 files changed, 9 insertions(+)

diff --git a/include/uapi/linux/fcntl.h b/include/uapi/linux/fcntl.h
index 2f86b2ad6d7e..a472ba69596c 100644
--- a/include/uapi/linux/fcntl.h
+++ b/include/uapi/linux/fcntl.h
@@ -43,6 +43,7 @@
 #define F_SEAL_GROW	0x0004	/* prevent file from growing */
 #define F_SEAL_WRITE	0x0008	/* prevent writes */
 #define F_SEAL_FUTURE_WRITE	0x0010  /* prevent future writes while mapped */
+#define F_SEAL_EXEC     0x0020  /* prevent chmod modifying exec bits */
 /* (1U << 31) is reserved for signed error codes */
 
 /*
diff --git a/mm/memfd.c b/mm/memfd.c
index 08f5f8304746..4ebeab94aa74 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -147,6 +147,7 @@ static unsigned int *memfd_file_seals_ptr(struct file *file)
 }
 
 #define F_ALL_SEALS (F_SEAL_SEAL | \
+		     F_SEAL_EXEC | \
 		     F_SEAL_SHRINK | \
 		     F_SEAL_GROW | \
 		     F_SEAL_WRITE | \
@@ -175,6 +176,7 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 	 *   SEAL_SHRINK: Prevent the file from shrinking
 	 *   SEAL_GROW: Prevent the file from growing
 	 *   SEAL_WRITE: Prevent write access to the file
+	 *   SEAL_EXEC: Prevent modification of the exec bits in the file mode
 	 *
 	 * As we don't require any trust relationship between two parties, we
 	 * must prevent seals from being removed. Therefore, sealing a file
diff --git a/mm/shmem.c b/mm/shmem.c
index 529c9ad3e926..a5ca9675fc29 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1083,6 +1083,12 @@ static int shmem_setattr(struct user_namespace *mnt_userns,
 	if (error)
 		return error;
 
+	if ((info->seals & F_SEAL_EXEC) && (attr->ia_valid & ATTR_MODE)) {
+		if ((inode->i_mode ^ attr->ia_mode) & 0111) {
+			return -EPERM;
+		}
+	}
+
 	if (S_ISREG(inode->i_mode) && (attr->ia_valid & ATTR_SIZE)) {
 		loff_t oldsize = inode->i_size;
 		loff_t newsize = attr->ia_size;
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 2/4] mm/memfd: add MFD_NOEXEC flag to memfd_create

[Daniel Verkamp]

The new MFD_NOEXEC flag allows the creation of a permanently
non-executable memfd. This is accomplished by creating it with a
different set of file mode bits (0666) than the default (0777) and
applying the F_SEAL_EXEC seal at creation time, so there is no window
between memfd creation and seal application.

Unfortunately, the default for memfd must remain executable, since
changing this would be an API break, and some programs depend on being
able to exec code from a memfd directly. However, this new flag will
allow programs to create non-executable memfds, and a distribution may
choose to enforce use of this flag in memfd_create calls via other
security mechanisms.

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 include/uapi/linux/memfd.h |  1 +
 mm/memfd.c                 | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
index 7a8a26751c23..140e125c9f65 100644
--- a/include/uapi/linux/memfd.h
+++ b/include/uapi/linux/memfd.h
@@ -8,6 +8,7 @@
 #define MFD_CLOEXEC		0x0001U
 #define MFD_ALLOW_SEALING	0x0002U
 #define MFD_HUGETLB		0x0004U
+#define MFD_NOEXEC              0x0008U
 
 /*
  * Huge page size encoding when MFD_HUGETLB is specified, and a huge page
diff --git a/mm/memfd.c b/mm/memfd.c
index 4ebeab94aa74..b841514eb0fd 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -263,7 +263,7 @@ long memfd_fcntl(struct file *file, unsigned int cmd, unsigned long arg)
 #define MFD_NAME_PREFIX_LEN (sizeof(MFD_NAME_PREFIX) - 1)
 #define MFD_NAME_MAX_LEN (NAME_MAX - MFD_NAME_PREFIX_LEN)
 
-#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_HUGETLB)
+#define MFD_ALL_FLAGS (MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_HUGETLB | MFD_NOEXEC)
 
 SYSCALL_DEFINE2(memfd_create,
 		const char __user *, uname,
@@ -333,6 +333,14 @@ SYSCALL_DEFINE2(memfd_create,
 		*file_seals &= ~F_SEAL_SEAL;
 	}
 
+	if (flags & MFD_NOEXEC) {
+		struct inode *inode = file_inode(file);
+
+		inode->i_mode &= ~0111;
+		file_seals = memfd_file_seals_ptr(file);
+		*file_seals |= F_SEAL_EXEC;
+	}
+
 	fd_install(fd, file);
 	kfree(name);
 	return fd;
-- 
2.35.1.1094.g7c7d902a7c-goog

[PATCH 3/4] selftests/memfd: add tests for F_SEAL_EXEC

[Daniel Verkamp]

Basic tests to ensure that user/group/other execute bits cannot be
changed after applying F_SEAL_EXEC to a memfd.

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 tools/testing/selftests/memfd/memfd_test.c | 80 ++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 94df2692e6e4..fdb0e46e9df9 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -28,6 +28,10 @@
 #define MFD_DEF_SIZE 8192
 #define STACK_SIZE 65536
 
+#ifndef F_SEAL_EXEC
+#define F_SEAL_EXEC	0x0020
+#endif
+
 /*
  * Default is not to test hugetlbfs
  */
@@ -594,6 +598,48 @@ static void mfd_fail_grow_write(int fd)
 	}
 }
 
+static void mfd_assert_mode(int fd, int mode)
+{
+	struct stat st;
+
+	if (fstat(fd, &st) < 0) {
+		printf("fstat(%d) failed: %m\n", fd);
+		abort();
+	} else if ((st.st_mode & 07777) != mode) {
+		printf("wrong file mode 0%04o, but expected 0%04o\n",
+		       (int)st.st_mode & 07777, mode);
+		abort();
+	}
+}
+
+static void mfd_assert_chmod(int fd, int mode)
+{
+	if (fchmod(fd, mode) < 0) {
+		printf("fchmod(0%04o) failed: %m\n", mode);
+		abort();
+	}
+
+	mfd_assert_mode(fd, mode);
+}
+
+static void mfd_fail_chmod(int fd, int mode)
+{
+	struct stat st;
+
+	if (fstat(fd, &st) < 0) {
+		printf("fstat(%d) failed: %m\n", fd);
+		abort();
+	}
+
+	if (fchmod(fd, mode) == 0) {
+		printf("fchmod(0%04o) didn't fail as expected\n");
+		abort();
+	}
+
+	/* verify that file mode bits did not change */
+	mfd_assert_mode(fd, st.st_mode & 07777);
+}
+
 static int idle_thread_fn(void *arg)
 {
 	sigset_t set;
@@ -880,6 +926,39 @@ static void test_seal_resize(void)
 	close(fd);
 }
 
+/*
+ * Test SEAL_EXEC
+ * Test that chmod() cannot change x bits after sealing
+ */
+static void test_seal_exec(void)
+{
+	int fd;
+
+	printf("%s SEAL-EXEC\n", memfd_str);
+
+	fd = mfd_assert_new("kern_memfd_seal_exec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+	mfd_assert_mode(fd, 0777);
+
+	mfd_assert_chmod(fd, 0644);
+
+	mfd_assert_has_seals(fd, 0);
+	mfd_assert_add_seals(fd, F_SEAL_EXEC);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+
+	mfd_assert_chmod(fd, 0600);
+	mfd_fail_chmod(fd, 0777);
+	mfd_fail_chmod(fd, 0670);
+	mfd_fail_chmod(fd, 0605);
+	mfd_fail_chmod(fd, 0700);
+	mfd_fail_chmod(fd, 0100);
+	mfd_assert_chmod(fd, 0666);
+
+	close(fd);
+}
+
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -1059,6 +1138,7 @@ int main(int argc, char **argv)
 	test_seal_shrink();
 	test_seal_grow();
 	test_seal_resize();
+	test_seal_exec();
 
 	test_share_dup("SHARE-DUP", "");
 	test_share_mmap("SHARE-MMAP", "");
-- 
2.35.1.1094.g7c7d902a7c-goog

Re: [PATCH 3/4] selftests/memfd: add tests for F_SEAL_EXEC

[Shuah Khan]

On 4/1/22 4:08 PM, Daniel Verkamp wrote:
> Basic tests to ensure that user/group/other execute bits cannot be
> changed after applying F_SEAL_EXEC to a memfd.
> 
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> ---
>   tools/testing/selftests/memfd/memfd_test.c | 80 ++++++++++++++++++++++
>   1 file changed, 80 insertions(+)
> 
> diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
> index 94df2692e6e4..fdb0e46e9df9 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -28,6 +28,10 @@
>   #define MFD_DEF_SIZE 8192
>   #define STACK_SIZE 65536
>   
> +#ifndef F_SEAL_EXEC
> +#define F_SEAL_EXEC	0x0020
> +#endif
> +
>   /*
>    * Default is not to test hugetlbfs
>    */
> @@ -594,6 +598,48 @@ static void mfd_fail_grow_write(int fd)
>   	}
>   }
>   
> +static void mfd_assert_mode(int fd, int mode)
> +{
> +	struct stat st;
> +
> +	if (fstat(fd, &st) < 0) {
> +		printf("fstat(%d) failed: %m\n", fd);

Let's print the filename here - just printing fd isn't useful.

> +		abort();
> +	} else if ((st.st_mode & 07777) != mode) {
> +		printf("wrong file mode 0%04o, but expected 0%04o\n",
> +		       (int)st.st_mode & 07777, mode);

This one doesn't even print fd - same comment here about filename.

> +		abort();
> +	}
> +}
> +
> +static void mfd_assert_chmod(int fd, int mode)
> +{
> +	if (fchmod(fd, mode) < 0) {
> +		printf("fchmod(0%04o) failed: %m\n", mode);

Same here.

> +		abort();
> +	}
> +
> +	mfd_assert_mode(fd, mode);
> +}
> +
> +static void mfd_fail_chmod(int fd, int mode)
> +{
> +	struct stat st;
> +
> +	if (fstat(fd, &st) < 0) {
> +		printf("fstat(%d) failed: %m\n", fd);

Same comment about filename

> +		abort();
> +	}
> +
> +	if (fchmod(fd, mode) == 0) {
> +		printf("fchmod(0%04o) didn't fail as expected\n");

Same comment about filename

> +		abort();
> +	}
> +
> +	/* verify that file mode bits did not change */
> +	mfd_assert_mode(fd, st.st_mode & 07777);
> +}
> +
>   static int idle_thread_fn(void *arg)
>   {
>   	sigset_t set;
> @@ -880,6 +926,39 @@ static void test_seal_resize(void)
>   	close(fd);
>   }
>   
> +/*
> + * Test SEAL_EXEC
> + * Test that chmod() cannot change x bits after sealing
> + */
> +static void test_seal_exec(void)
> +{
> +	int fd;
> +
> +	printf("%s SEAL-EXEC\n", memfd_str);
> +
> +	fd = mfd_assert_new("kern_memfd_seal_exec",
> +			    mfd_def_size,
> +			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
> +
> +	mfd_assert_mode(fd, 0777);
> +
> +	mfd_assert_chmod(fd, 0644);
> +
> +	mfd_assert_has_seals(fd, 0);
> +	mfd_assert_add_seals(fd, F_SEAL_EXEC);
> +	mfd_assert_has_seals(fd, F_SEAL_EXEC);
> +
> +	mfd_assert_chmod(fd, 0600);
> +	mfd_fail_chmod(fd, 0777);
> +	mfd_fail_chmod(fd, 0670);
> +	mfd_fail_chmod(fd, 0605);
> +	mfd_fail_chmod(fd, 0700);
> +	mfd_fail_chmod(fd, 0100);
> +	mfd_assert_chmod(fd, 0666);
> +
> +	close(fd);
> +}
> +
>   /*
>    * Test sharing via dup()
>    * Test that seals are shared between dupped FDs and they're all equal.
> @@ -1059,6 +1138,7 @@ int main(int argc, char **argv)
>   	test_seal_shrink();
>   	test_seal_grow();
>   	test_seal_resize();
> +	test_seal_exec();
>   
>   	test_share_dup("SHARE-DUP", "");
>   	test_share_mmap("SHARE-MMAP", "");
> 

The rest looks good.

thanks,
-- Shuah

[PATCH 3/4] selftests/memfd: add tests for F_SEAL_EXEC

[Jeff Xu]

From: Daniel Verkamp <dverkamp@chromium.org>

Basic tests to ensure that user/group/other execute bits cannot be
changed after applying F_SEAL_EXEC to a memfd.

Co-developed-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 tools/testing/selftests/memfd/memfd_test.c | 129 ++++++++++++++++++++-
 1 file changed, 128 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 94df2692e6e4..1d7e7b36bbdd 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -28,12 +28,44 @@
 #define MFD_DEF_SIZE 8192
 #define STACK_SIZE 65536
 
+#ifndef F_SEAL_EXEC
+#define F_SEAL_EXEC	0x0020
+#endif
+
+#ifndef MAX_PATH
+#define MAX_PATH 256
+#endif
+
 /*
  * Default is not to test hugetlbfs
  */
 static size_t mfd_def_size = MFD_DEF_SIZE;
 static const char *memfd_str = MEMFD_STR;
 
+static ssize_t fd2name(int fd, char *buf, size_t bufsize)
+{
+	char buf1[MAX_PATH];
+	int size;
+	ssize_t nbytes;
+
+	size = snprintf(buf1, MAX_PATH, "/proc/self/fd/%d", fd);
+	if (size < 0) {
+		printf("snprintf(%d) failed on %m\n", fd);
+		abort();
+	}
+
+	/*
+	 * reserver one byte for string termination.
+	 */
+	nbytes = readlink(buf1, buf, bufsize-1);
+	if (nbytes == -1) {
+		printf("readlink(%s) failed %m\n", buf1);
+		abort();
+	}
+	buf[nbytes] = '\0';
+	return nbytes;
+}
+
 static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags)
 {
 	int r, fd;
@@ -98,11 +130,14 @@ static unsigned int mfd_assert_get_seals(int fd)
 
 static void mfd_assert_has_seals(int fd, unsigned int seals)
 {
+	char buf[MAX_PATH];
+	int nbytes;
 	unsigned int s;
+	fd2name(fd, buf, MAX_PATH);
 
 	s = mfd_assert_get_seals(fd);
 	if (s != seals) {
-		printf("%u != %u = GET_SEALS(%d)\n", seals, s, fd);
+		printf("%u != %u = GET_SEALS(%s)\n", seals, s, buf);
 		abort();
 	}
 }
@@ -594,6 +629,64 @@ static void mfd_fail_grow_write(int fd)
 	}
 }
 
+static void mfd_assert_mode(int fd, int mode)
+{
+	struct stat st;
+	char buf[MAX_PATH];
+	int nbytes;
+
+	fd2name(fd, buf, MAX_PATH);
+
+	if (fstat(fd, &st) < 0) {
+		printf("fstat(%s) failed: %m\n", buf);
+		abort();
+	}
+
+	if ((st.st_mode & 07777) != mode) {
+		printf("fstat(%s) wrong file mode 0%04o, but expected 0%04o\n",
+		       buf, (int)st.st_mode & 07777, mode);
+		abort();
+	}
+}
+
+static void mfd_assert_chmod(int fd, int mode)
+{
+	char buf[MAX_PATH];
+	int nbytes;
+
+	fd2name(fd, buf, MAX_PATH);
+
+	if (fchmod(fd, mode) < 0) {
+		printf("fchmod(%s, 0%04o) failed: %m\n", buf, mode);
+		abort();
+	}
+
+	mfd_assert_mode(fd, mode);
+}
+
+static void mfd_fail_chmod(int fd, int mode)
+{
+	struct stat st;
+	char buf[MAX_PATH];
+	int nbytes;
+
+	fd2name(fd, buf, MAX_PATH);
+
+	if (fstat(fd, &st) < 0) {
+		printf("fstat(%s) failed: %m\n", buf);
+		abort();
+	}
+
+	if (fchmod(fd, mode) == 0) {
+		printf("fchmod(%s, 0%04o) didn't fail as expected\n",
+		       buf, mode);
+		abort();
+	}
+
+	/* verify that file mode bits did not change */
+	mfd_assert_mode(fd, st.st_mode & 07777);
+}
+
 static int idle_thread_fn(void *arg)
 {
 	sigset_t set;
@@ -880,6 +973,39 @@ static void test_seal_resize(void)
 	close(fd);
 }
 
+/*
+ * Test SEAL_EXEC
+ * Test that chmod() cannot change x bits after sealing
+ */
+static void test_seal_exec(void)
+{
+	int fd;
+
+	printf("%s SEAL-EXEC\n", memfd_str);
+
+	fd = mfd_assert_new("kern_memfd_seal_exec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+	mfd_assert_mode(fd, 0777);
+
+	mfd_assert_chmod(fd, 0644);
+
+	mfd_assert_has_seals(fd, 0);
+	mfd_assert_add_seals(fd, F_SEAL_EXEC);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+
+	mfd_assert_chmod(fd, 0600);
+	mfd_fail_chmod(fd, 0777);
+	mfd_fail_chmod(fd, 0670);
+	mfd_fail_chmod(fd, 0605);
+	mfd_fail_chmod(fd, 0700);
+	mfd_fail_chmod(fd, 0100);
+	mfd_assert_chmod(fd, 0666);
+
+	close(fd);
+}
+
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -1059,6 +1185,7 @@ int main(int argc, char **argv)
 	test_seal_shrink();
 	test_seal_grow();
 	test_seal_resize();
+	test_seal_exec();
 
 	test_share_dup("SHARE-DUP", "");
 	test_share_mmap("SHARE-MMAP", "");
-- 
2.37.1.455.g008518b4e5-goog

[PATCH 4/4] selftests/memfd: add tests for MFD_NOEXEC

[Jeff Xu]

From: Daniel Verkamp <dverkamp@chromium.org>

Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and
cannot be chmod-ed into being executable.

Co-developed-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 1d7e7b36bbdd..4906f778564e 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -36,6 +36,10 @@
 #define MAX_PATH 256
 #endif
 
+#ifndef MFD_NOEXEC
+#define MFD_NOEXEC	0x0008U
+#endif
+
 /*
  * Default is not to test hugetlbfs
  */
@@ -1006,6 +1010,35 @@ static void test_seal_exec(void)
 	close(fd);
 }
 
+/*
+ * Test memfd_create with MFD_NOEXEC flag
+ * Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits
+ */
+static void test_noexec(void)
+{
+	int fd;
+
+	printf("%s NOEXEC\n", memfd_str);
+
+	/* Create with NOEXEC and ALLOW_SEALING */
+	fd = mfd_assert_new("kern_memfd_noexec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+
+	/* Create with NOEXEC but without ALLOW_SEALING */
+	fd = mfd_assert_new("kern_memfd_noexec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_NOEXEC);
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+}
+
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -1179,6 +1212,7 @@ int main(int argc, char **argv)
 
 	test_create();
 	test_basic();
+	test_noexec();
 
 	test_seal_write();
 	test_seal_future_write();
-- 
2.37.1.455.g008518b4e5-goog

Re: [PATCH 4/4] selftests/memfd: add tests for MFD_NOEXEC

[Jeff Xu]

> > + /* Create with NOEXEC and ALLOW_SEALING */
> > + fd = mfd_assert_new("kern_memfd_noexec",
> > +    mfd_def_size,
> > +    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);


> Don't we need to check fd here?


mfd_assert_new will abort() if fd is not valid, so we don't check fd here.

Jeff


On Thu, Jul 28, 2022 at 11:15 PM Jeff Xu <jeffxu@google.com> wrote:
>
> From: Daniel Verkamp <dverkamp@chromium.org>
>
> Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and
> cannot be chmod-ed into being executable.
>
> Co-developed-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> ---
>  tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
> index 1d7e7b36bbdd..4906f778564e 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -36,6 +36,10 @@
>  #define MAX_PATH 256
>  #endif
>
> +#ifndef MFD_NOEXEC
> +#define MFD_NOEXEC     0x0008U
> +#endif
> +
>  /*
>   * Default is not to test hugetlbfs
>   */
> @@ -1006,6 +1010,35 @@ static void test_seal_exec(void)
>         close(fd);
>  }
>
> +/*
> + * Test memfd_create with MFD_NOEXEC flag
> + * Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits
> + */
> +static void test_noexec(void)
> +{
> +       int fd;
> +
> +       printf("%s NOEXEC\n", memfd_str);
> +
> +       /* Create with NOEXEC and ALLOW_SEALING */
> +       fd = mfd_assert_new("kern_memfd_noexec",
> +                           mfd_def_size,
> +                           MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);
> +       mfd_assert_mode(fd, 0666);
> +       mfd_assert_has_seals(fd, F_SEAL_EXEC);
> +       mfd_fail_chmod(fd, 0777);
> +       close(fd);
> +
> +       /* Create with NOEXEC but without ALLOW_SEALING */
> +       fd = mfd_assert_new("kern_memfd_noexec",
> +                           mfd_def_size,
> +                           MFD_CLOEXEC | MFD_NOEXEC);
> +       mfd_assert_mode(fd, 0666);
> +       mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL);
> +       mfd_fail_chmod(fd, 0777);
> +       close(fd);
> +}
> +
>  /*
>   * Test sharing via dup()
>   * Test that seals are shared between dupped FDs and they're all equal.
> @@ -1179,6 +1212,7 @@ int main(int argc, char **argv)
>
>         test_create();
>         test_basic();
> +       test_noexec();
>
>         test_seal_write();
>         test_seal_future_write();
> --
> 2.37.1.455.g008518b4e5-goog
>

Re: [PATCH 3/4] selftests/memfd: add tests for F_SEAL_EXEC

[Jeff Xu]

Hi Shuah Khan

I will continue Daniel Verkamp's work on this patch.

Could you please take a look at the new patch set I sent to see if all
your comments are addressed ?

Much appreciated.

Best Regards,
Jeff.


On Thu, Jul 28, 2022 at 11:15 PM Jeff Xu <jeffxu@google.com> wrote:
>
> From: Daniel Verkamp <dverkamp@chromium.org>
>
> Basic tests to ensure that user/group/other execute bits cannot be
> changed after applying F_SEAL_EXEC to a memfd.
>
> Co-developed-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> ---
>  tools/testing/selftests/memfd/memfd_test.c | 129 ++++++++++++++++++++-
>  1 file changed, 128 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
> index 94df2692e6e4..1d7e7b36bbdd 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -28,12 +28,44 @@
>  #define MFD_DEF_SIZE 8192
>  #define STACK_SIZE 65536
>
> +#ifndef F_SEAL_EXEC
> +#define F_SEAL_EXEC    0x0020
> +#endif
> +
> +#ifndef MAX_PATH
> +#define MAX_PATH 256
> +#endif
> +
>  /*
>   * Default is not to test hugetlbfs
>   */
>  static size_t mfd_def_size = MFD_DEF_SIZE;
>  static const char *memfd_str = MEMFD_STR;
>
> +static ssize_t fd2name(int fd, char *buf, size_t bufsize)
> +{
> +       char buf1[MAX_PATH];
> +       int size;
> +       ssize_t nbytes;
> +
> +       size = snprintf(buf1, MAX_PATH, "/proc/self/fd/%d", fd);
> +       if (size < 0) {
> +               printf("snprintf(%d) failed on %m\n", fd);
> +               abort();
> +       }
> +
> +       /*
> +        * reserver one byte for string termination.
> +        */
> +       nbytes = readlink(buf1, buf, bufsize-1);
> +       if (nbytes == -1) {
> +               printf("readlink(%s) failed %m\n", buf1);
> +               abort();
> +       }
> +       buf[nbytes] = '\0';
> +       return nbytes;
> +}
> +
>  static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags)
>  {
>         int r, fd;
> @@ -98,11 +130,14 @@ static unsigned int mfd_assert_get_seals(int fd)
>
>  static void mfd_assert_has_seals(int fd, unsigned int seals)
>  {
> +       char buf[MAX_PATH];
> +       int nbytes;
>         unsigned int s;
> +       fd2name(fd, buf, MAX_PATH);
>
>         s = mfd_assert_get_seals(fd);
>         if (s != seals) {
> -               printf("%u != %u = GET_SEALS(%d)\n", seals, s, fd);
> +               printf("%u != %u = GET_SEALS(%s)\n", seals, s, buf);
>                 abort();
>         }
>  }
> @@ -594,6 +629,64 @@ static void mfd_fail_grow_write(int fd)
>         }
>  }
>
> +static void mfd_assert_mode(int fd, int mode)
> +{
> +       struct stat st;
> +       char buf[MAX_PATH];
> +       int nbytes;
> +
> +       fd2name(fd, buf, MAX_PATH);
> +
> +       if (fstat(fd, &st) < 0) {
> +               printf("fstat(%s) failed: %m\n", buf);
> +               abort();
> +       }
> +
> +       if ((st.st_mode & 07777) != mode) {
> +               printf("fstat(%s) wrong file mode 0%04o, but expected 0%04o\n",
> +                      buf, (int)st.st_mode & 07777, mode);
> +               abort();
> +       }
> +}
> +
> +static void mfd_assert_chmod(int fd, int mode)
> +{
> +       char buf[MAX_PATH];
> +       int nbytes;
> +
> +       fd2name(fd, buf, MAX_PATH);
> +
> +       if (fchmod(fd, mode) < 0) {
> +               printf("fchmod(%s, 0%04o) failed: %m\n", buf, mode);
> +               abort();
> +       }
> +
> +       mfd_assert_mode(fd, mode);
> +}
> +
> +static void mfd_fail_chmod(int fd, int mode)
> +{
> +       struct stat st;
> +       char buf[MAX_PATH];
> +       int nbytes;
> +
> +       fd2name(fd, buf, MAX_PATH);
> +
> +       if (fstat(fd, &st) < 0) {
> +               printf("fstat(%s) failed: %m\n", buf);
> +               abort();
> +       }
> +
> +       if (fchmod(fd, mode) == 0) {
> +               printf("fchmod(%s, 0%04o) didn't fail as expected\n",
> +                      buf, mode);
> +               abort();
> +       }
> +
> +       /* verify that file mode bits did not change */
> +       mfd_assert_mode(fd, st.st_mode & 07777);
> +}
> +
>  static int idle_thread_fn(void *arg)
>  {
>         sigset_t set;
> @@ -880,6 +973,39 @@ static void test_seal_resize(void)
>         close(fd);
>  }
>
> +/*
> + * Test SEAL_EXEC
> + * Test that chmod() cannot change x bits after sealing
> + */
> +static void test_seal_exec(void)
> +{
> +       int fd;
> +
> +       printf("%s SEAL-EXEC\n", memfd_str);
> +
> +       fd = mfd_assert_new("kern_memfd_seal_exec",
> +                           mfd_def_size,
> +                           MFD_CLOEXEC | MFD_ALLOW_SEALING);
> +
> +       mfd_assert_mode(fd, 0777);
> +
> +       mfd_assert_chmod(fd, 0644);
> +
> +       mfd_assert_has_seals(fd, 0);
> +       mfd_assert_add_seals(fd, F_SEAL_EXEC);
> +       mfd_assert_has_seals(fd, F_SEAL_EXEC);
> +
> +       mfd_assert_chmod(fd, 0600);
> +       mfd_fail_chmod(fd, 0777);
> +       mfd_fail_chmod(fd, 0670);
> +       mfd_fail_chmod(fd, 0605);
> +       mfd_fail_chmod(fd, 0700);
> +       mfd_fail_chmod(fd, 0100);
> +       mfd_assert_chmod(fd, 0666);
> +
> +       close(fd);
> +}
> +
>  /*
>   * Test sharing via dup()
>   * Test that seals are shared between dupped FDs and they're all equal.
> @@ -1059,6 +1185,7 @@ int main(int argc, char **argv)
>         test_seal_shrink();
>         test_seal_grow();
>         test_seal_resize();
> +       test_seal_exec();
>
>         test_share_dup("SHARE-DUP", "");
>         test_share_mmap("SHARE-MMAP", "");
> --
> 2.37.1.455.g008518b4e5-goog
>

[PATCH 4/4] selftests/memfd: add tests for MFD_NOEXEC

[Daniel Verkamp]

Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and
cannot be chmod-ed into being executable.

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
---
 tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index fdb0e46e9df9..a79567161cdf 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -32,6 +32,10 @@
 #define F_SEAL_EXEC	0x0020
 #endif
 
+#ifndef MFD_NOEXEC
+#define MFD_NOEXEC	0x0008U
+#endif
+
 /*
  * Default is not to test hugetlbfs
  */
@@ -959,6 +963,35 @@ static void test_seal_exec(void)
 	close(fd);
 }
 
+/*
+ * Test memfd_create with MFD_NOEXEC flag
+ * Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits
+ */
+static void test_noexec(void)
+{
+	int fd;
+
+	printf("%s NOEXEC\n", memfd_str);
+
+	/* Create with NOEXEC and ALLOW_SEALING */
+	fd = mfd_assert_new("kern_memfd_noexec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+
+	/* Create with NOEXEC but without ALLOW_SEALING */
+	fd = mfd_assert_new("kern_memfd_noexec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_NOEXEC);
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+}
+
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -1132,6 +1165,7 @@ int main(int argc, char **argv)
 
 	test_create();
 	test_basic();
+	test_noexec();
 
 	test_seal_write();
 	test_seal_future_write();
-- 
2.35.1.1094.g7c7d902a7c-goog

Re: [PATCH 4/4] selftests/memfd: add tests for MFD_NOEXEC

[Shuah Khan]

On 4/1/22 4:08 PM, Daniel Verkamp wrote:
> Tests that ensure MFD_NOEXEC memfds have the appropriate mode bits and
> cannot be chmod-ed into being executable.
> 
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> ---
>   tools/testing/selftests/memfd/memfd_test.c | 34 ++++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
> index fdb0e46e9df9..a79567161cdf 100644
> --- a/tools/testing/selftests/memfd/memfd_test.c
> +++ b/tools/testing/selftests/memfd/memfd_test.c
> @@ -32,6 +32,10 @@
>   #define F_SEAL_EXEC	0x0020
>   #endif
>   
> +#ifndef MFD_NOEXEC
> +#define MFD_NOEXEC	0x0008U
> +#endif
> +
>   /*
>    * Default is not to test hugetlbfs
>    */
> @@ -959,6 +963,35 @@ static void test_seal_exec(void)
>   	close(fd);
>   }
>   
> +/*
> + * Test memfd_create with MFD_NOEXEC flag
> + * Test that MFD_NOEXEC applies F_SEAL_EXEC and prevents change of exec bits
> + */
> +static void test_noexec(void)
> +{
> +	int fd;
> +
> +	printf("%s NOEXEC\n", memfd_str);
> +
> +	/* Create with NOEXEC and ALLOW_SEALING */
> +	fd = mfd_assert_new("kern_memfd_noexec",
> +			    mfd_def_size,
> +			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_NOEXEC);

Don't we need to check fd here?

> +	mfd_assert_mode(fd, 0666);
> +	mfd_assert_has_seals(fd, F_SEAL_EXEC);
> +	mfd_fail_chmod(fd, 0777);
> +	close(fd);
> +
> +	/* Create with NOEXEC but without ALLOW_SEALING */
> +	fd = mfd_assert_new("kern_memfd_noexec",
> +			    mfd_def_size,
> +			    MFD_CLOEXEC | MFD_NOEXEC);

What happens when mfd_assert_new() fails - don't we need to check fd?

> +	mfd_assert_mode(fd, 0666);
> +	mfd_assert_has_seals(fd, F_SEAL_EXEC | F_SEAL_SEAL);
> +	mfd_fail_chmod(fd, 0777);
> +	close(fd);
> +}
> +
>   /*
>    * Test sharing via dup()
>    * Test that seals are shared between dupped FDs and they're all equal.
> @@ -1132,6 +1165,7 @@ int main(int argc, char **argv)
>   
>   	test_create();
>   	test_basic();
> +	test_noexec();
>   
>   	test_seal_write();
>   	test_seal_future_write();
> 

fd isn't checked in the other test F_SEAL_EXEC in the 3/4 patch.

thanks,
-- Shuah