[PATCH v6 0/6] mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC

[PATCH v6 0/6] mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

Since Linux introduced the memfd feature, memfd have always had their
execute bit set, and the memfd_create() syscall doesn't allow setting
it differently.

However, in a secure by default system, such as ChromeOS, (where all
executables should come from the rootfs, which is protected by Verified
boot), this executable nature of memfd opens a door for NoExec bypass
and enables “confused deputy attack”.  E.g, in VRP bug [1]: cros_vm
process created a memfd to share the content with an external process,
however the memfd is overwritten and used for executing arbitrary code
and root escalation. [2] lists more VRP in this kind.

On the other hand, executable memfd has its legit use, runc uses memfd’s
seal and executable feature to copy the contents of the binary then
execute them, for such system, we need a solution to differentiate runc's
use of  executable memfds and an attacker's [3].

To address those above, this set of patches add following:
1> Let memfd_create() set X bit at creation time.
2> Let memfd to be sealed for modifying X bit.
3> A new pid namespace sysctl: vm.memfd_noexec to control the behavior of
   X bit.For example, if a container has vm.memfd_noexec=2, then
   memfd_create() without MFD_NOEXEC_SEAL will be rejected.
4> A new security hook in memfd_create(). This make it possible to a new
LSM, which rejects or allows executable memfd based on its security policy.

This is V6 version of patch: see [4] [5] [6] [7] for previous versions.

[1] https://crbug.com/1305411
[2] https://bugs.chromium.org/p/chromium/issues/list?q=type%3Dbug-security%20memfd%20escalation&can=1
[3] https://lwn.net/Articles/781013/
[4] https://lwn.net/Articles/890096/
[5] https://lore.kernel.org/lkml/20220805222126.142525-1-jeffxu@google.com/
[6] https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
[7] https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/

Daniel Verkamp (2):
  mm/memfd: add F_SEAL_EXEC
  selftests/memfd: add tests for F_SEAL_EXEC

Jeff Xu (4):
  mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC
  mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd
  selftests/memfd: add tests for MFD_NOEXEC_SEAL MFD_EXEC
  mm/memfd: security hook for memfd_create

 include/linux/lsm_hook_defs.h              |   1 +
 include/linux/lsm_hooks.h                  |   4 +
 include/linux/pid_namespace.h              |  19 ++
 include/linux/security.h                   |   6 +
 include/uapi/linux/fcntl.h                 |   1 +
 include/uapi/linux/memfd.h                 |   4 +
 kernel/pid_namespace.c                     |   5 +
 kernel/pid_sysctl.h                        |  59 ++++
 mm/memfd.c                                 |  61 +++-
 mm/shmem.c                                 |   6 +
 security/security.c                        |  13 +
 tools/testing/selftests/memfd/fuse_test.c  |   1 +
 tools/testing/selftests/memfd/memfd_test.c | 348 ++++++++++++++++++++-
 13 files changed, 525 insertions(+), 3 deletions(-)
 create mode 100644 kernel/pid_sysctl.h


base-commit: eb7081409f94a9a8608593d0fb63a1aa3d6f95d8
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 1/6] mm/memfd: add F_SEAL_EXEC

[jeffxu]

From: Daniel Verkamp <dverkamp@chromium.org>

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>
Co-developed-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Jeff Xu <jeffxu@google.com>
---
 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..e8c07da58c9f 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 c1d8b8a1aa3b..e18a9cf9d937 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1085,6 +1085,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.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 2/6] selftests/memfd: add tests for F_SEAL_EXEC

[jeffxu]

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.

Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Co-developed-by: Jeff Xu <jeffxu@google.com>
Signed-off-by: Jeff Xu <jeffxu@google.com>
---
 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.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

The new MFD_NOEXEC_SEAL and MFD_EXEC flags allows application to
set executable bit at creation time (memfd_create).

When MFD_NOEXEC_SEAL is set, memfd is created without executable bit
(mode:0666), and sealed with F_SEAL_EXEC, so it can't be chmod to
be executable (mode: 0777) after creation.

when MFD_EXEC flag is set, memfd is created with executable bit
(mode:0777), this is the same as the old behavior of memfd_create.

The new pid namespaced sysctl vm.memfd_noexec has 3 values:
0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
	MFD_EXEC was set.
1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
	MFD_NOEXEC_SEAL was set.
2: memfd_create() without MFD_NOEXEC_SEAL will be rejected.

The sysctl allows finer control of memfd_create for old-software
that doesn't set the executable bit, for example, a container with
vm.memfd_noexec=1 means the old-software will create non-executable
memfd by default. Also, the value of memfd_noexec is passed to child
namespace at creation time. For example, if the init namespace has
vm.memfd_noexec=2, all its children namespaces will be created with 2.

Signed-off-by: Jeff Xu <jeffxu@google.com>
Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
Reported-by: kernel test robot <lkp@intel.com>
---
 include/linux/pid_namespace.h | 19 +++++++++++
 include/uapi/linux/memfd.h    |  4 +++
 kernel/pid_namespace.c        |  5 +++
 kernel/pid_sysctl.h           | 59 +++++++++++++++++++++++++++++++++++
 mm/memfd.c                    | 48 ++++++++++++++++++++++++++--
 5 files changed, 133 insertions(+), 2 deletions(-)
 create mode 100644 kernel/pid_sysctl.h

diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 07481bb87d4e..a4789a7b34a9 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -16,6 +16,21 @@
 
 struct fs_pin;
 
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
+/*
+ * sysctl for vm.memfd_noexec
+ * 0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
+ *	acts like MFD_EXEC was set.
+ * 1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
+ *	acts like MFD_NOEXEC_SEAL was set.
+ * 2: memfd_create() without MFD_NOEXEC_SEAL will be
+ *	rejected.
+ */
+#define MEMFD_NOEXEC_SCOPE_EXEC		0
+#define MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL		1
+#define MEMFD_NOEXEC_SCOPE_NOEXEC_ENFORCED	2
+#endif
+
 struct pid_namespace {
 	struct idr idr;
 	struct rcu_head rcu;
@@ -31,6 +46,10 @@ struct pid_namespace {
 	struct ucounts *ucounts;
 	int reboot;	/* group exit code if this pidns was rebooted */
 	struct ns_common ns;
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
+	/* sysctl for vm.memfd_noexec */
+	int memfd_noexec_scope;
+#endif
 } __randomize_layout;
 
 extern struct pid_namespace init_pid_ns;
diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
index 7a8a26751c23..273a4e15dfcf 100644
--- a/include/uapi/linux/memfd.h
+++ b/include/uapi/linux/memfd.h
@@ -8,6 +8,10 @@
 #define MFD_CLOEXEC		0x0001U
 #define MFD_ALLOW_SEALING	0x0002U
 #define MFD_HUGETLB		0x0004U
+/* not executable and sealed to prevent changing to executable. */
+#define MFD_NOEXEC_SEAL		0x0008U
+/* executable */
+#define MFD_EXEC		0x0010U
 
 /*
  * Huge page size encoding when MFD_HUGETLB is specified, and a huge page
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index f4f8cb0435b4..8a98b1af9376 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -23,6 +23,7 @@
 #include <linux/sched/task.h>
 #include <linux/sched/signal.h>
 #include <linux/idr.h>
+#include "pid_sysctl.h"
 
 static DEFINE_MUTEX(pid_caches_mutex);
 static struct kmem_cache *pid_ns_cachep;
@@ -110,6 +111,8 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
 	ns->ucounts = ucounts;
 	ns->pid_allocated = PIDNS_ADDING;
 
+	initialize_memfd_noexec_scope(ns);
+
 	return ns;
 
 out_free_idr:
@@ -455,6 +458,8 @@ static __init int pid_namespaces_init(void)
 #ifdef CONFIG_CHECKPOINT_RESTORE
 	register_sysctl_paths(kern_path, pid_ns_ctl_table);
 #endif
+
+	register_pid_ns_sysctl_table_vm();
 	return 0;
 }
 
diff --git a/kernel/pid_sysctl.h b/kernel/pid_sysctl.h
new file mode 100644
index 000000000000..5986d6493b5b
--- /dev/null
+++ b/kernel/pid_sysctl.h
@@ -0,0 +1,59 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef LINUX_PID_SYSCTL_H
+#define LINUX_PID_SYSCTL_H
+
+#include <linux/pid_namespace.h>
+
+#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
+static inline void initialize_memfd_noexec_scope(struct pid_namespace *ns)
+{
+	ns->memfd_noexec_scope =
+		task_active_pid_ns(current)->memfd_noexec_scope;
+}
+
+static int pid_mfd_noexec_dointvec_minmax(struct ctl_table *table,
+	int write, void *buf, size_t *lenp, loff_t *ppos)
+{
+	struct pid_namespace *ns = task_active_pid_ns(current);
+	struct ctl_table table_copy;
+
+	if (write && !capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	table_copy = *table;
+	if (ns != &init_pid_ns)
+		table_copy.data = &ns->memfd_noexec_scope;
+
+	/*
+	 * set minimum to current value, the effect is only bigger
+	 * value is accepted.
+	 */
+	if (*(int *)table_copy.data > *(int *)table_copy.extra1)
+		table_copy.extra1 = table_copy.data;
+
+	return proc_dointvec_minmax(&table_copy, write, buf, lenp, ppos);
+}
+
+static struct ctl_table pid_ns_ctl_table_vm[] = {
+	{
+		.procname	= "memfd_noexec",
+		.data		= &init_pid_ns.memfd_noexec_scope,
+		.maxlen		= sizeof(init_pid_ns.memfd_noexec_scope),
+		.mode		= 0644,
+		.proc_handler	= pid_mfd_noexec_dointvec_minmax,
+		.extra1		= SYSCTL_ZERO,
+		.extra2		= SYSCTL_TWO,
+	},
+	{ }
+};
+static struct ctl_path vm_path[] = { { .procname = "vm", }, { } };
+static inline void register_pid_ns_sysctl_table_vm(void)
+{
+	register_sysctl_paths(vm_path, pid_ns_ctl_table_vm);
+}
+#else
+static inline void set_memfd_noexec_scope(struct pid_namespace *ns) {}
+static inline void register_pid_ns_ctl_table_vm(void) {}
+#endif
+
+#endif /* LINUX_PID_SYSCTL_H */
diff --git a/mm/memfd.c b/mm/memfd.c
index 4ebeab94aa74..ec70675a7069 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -18,6 +18,7 @@
 #include <linux/hugetlb.h>
 #include <linux/shmem_fs.h>
 #include <linux/memfd.h>
+#include <linux/pid_namespace.h>
 #include <uapi/linux/memfd.h>
 
 /*
@@ -263,12 +264,14 @@ 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_SEAL | MFD_EXEC)
 
 SYSCALL_DEFINE2(memfd_create,
 		const char __user *, uname,
 		unsigned int, flags)
 {
+	char comm[TASK_COMM_LEN];
+	struct pid_namespace *ns;
 	unsigned int *file_seals;
 	struct file *file;
 	int fd, error;
@@ -285,6 +288,39 @@ SYSCALL_DEFINE2(memfd_create,
 			return -EINVAL;
 	}
 
+	/* Invalid if both EXEC and NOEXEC_SEAL are set.*/
+	if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
+		return -EINVAL;
+
+	if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
+#ifdef CONFIG_SYSCTL
+		int sysctl = MEMFD_NOEXEC_SCOPE_EXEC;
+
+		ns = task_active_pid_ns(current);
+		if (ns)
+			sysctl = ns->memfd_noexec_scope;
+
+		switch (sysctl) {
+		case MEMFD_NOEXEC_SCOPE_EXEC:
+			flags |= MFD_EXEC;
+			break;
+		case MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL:
+			flags |= MFD_NOEXEC_SEAL;
+			break;
+		default:
+			pr_warn_ratelimited(
+				"memfd_create(): MFD_NOEXEC_SEAL is enforced, pid=%d '%s'\n",
+				task_pid_nr(current), get_task_comm(comm, current));
+			return -EINVAL;
+		}
+#else
+		flags |= MFD_EXEC;
+#endif
+		pr_warn_ratelimited(
+			"memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
+			task_pid_nr(current), get_task_comm(comm, current));
+	}
+
 	/* length includes terminating zero */
 	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
 	if (len <= 0)
@@ -328,7 +364,15 @@ SYSCALL_DEFINE2(memfd_create,
 	file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
 	file->f_flags |= O_LARGEFILE;
 
-	if (flags & MFD_ALLOW_SEALING) {
+	if (flags & MFD_NOEXEC_SEAL) {
+		struct inode *inode = file_inode(file);
+
+		inode->i_mode &= ~0111;
+		file_seals = memfd_file_seals_ptr(file);
+		*file_seals &= ~F_SEAL_SEAL;
+		*file_seals |= F_SEAL_EXEC;
+	} else if (flags & MFD_ALLOW_SEALING) {
+		/* MFD_EXEC and MFD_ALLOW_SEALING are set */
 		file_seals = memfd_file_seals_ptr(file);
 		*file_seals &= ~F_SEAL_SEAL;
 	}
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 4/6] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

In order to avoid WX mappings, add F_SEAL_WRITE when apply
F_SEAL_EXEC to an executable memfd, so W^X from start.

This implys application need to fill the content of the memfd first,
after F_SEAL_EXEC is applied, application can no longer modify the
content of the memfd.

Typically, application seals the memfd right after writing to it.
For example:
1. memfd_create(MFD_EXEC).
2. write() code to the memfd.
3. fcntl(F_ADD_SEALS, F_SEAL_EXEC) to convert the memfd to W^X.
4. call exec() on the memfd.

Signed-off-by: Jeff Xu <jeffxu@google.com>
---
 mm/memfd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/memfd.c b/mm/memfd.c
index ec70675a7069..92f0a5765f7c 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -222,6 +222,12 @@ static int memfd_add_seals(struct file *file, unsigned int seals)
 		}
 	}
 
+	/*
+	 * SEAL_EXEC implys SEAL_WRITE, making W^X from the start.
+	 */
+	if (seals & F_SEAL_EXEC && inode->i_mode & 0111)
+		seals |= F_SEAL_SHRINK|F_SEAL_GROW|F_SEAL_WRITE|F_SEAL_FUTURE_WRITE;
+
 	*file_seals |= seals;
 	error = 0;
 
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 5/6] selftests/memfd: add tests for MFD_NOEXEC_SEAL MFD_EXEC

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

Tests to verify MFD_NOEXEC, MFD_EXEC and vm.memfd_noexec sysctl.

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

diff --git a/tools/testing/selftests/memfd/fuse_test.c b/tools/testing/selftests/memfd/fuse_test.c
index be675002f918..93798c8c5d54 100644
--- a/tools/testing/selftests/memfd/fuse_test.c
+++ b/tools/testing/selftests/memfd/fuse_test.c
@@ -22,6 +22,7 @@
 #include <linux/falloc.h>
 #include <fcntl.h>
 #include <linux/memfd.h>
+#include <linux/types.h>
 #include <sched.h>
 #include <stdio.h>
 #include <stdlib.h>
diff --git a/tools/testing/selftests/memfd/memfd_test.c b/tools/testing/selftests/memfd/memfd_test.c
index 1d7e7b36bbdd..cbf1768fb1b9 100644
--- a/tools/testing/selftests/memfd/memfd_test.c
+++ b/tools/testing/selftests/memfd/memfd_test.c
@@ -32,10 +32,19 @@
 #define F_SEAL_EXEC	0x0020
 #endif
 
+#define F_WX_SEALS (F_SEAL_SHRINK | \
+		    F_SEAL_GROW | \
+		    F_SEAL_WRITE | \
+		    F_SEAL_FUTURE_WRITE | \
+		    F_SEAL_EXEC)
 #ifndef MAX_PATH
 #define MAX_PATH 256
 #endif
 
+#ifndef MFD_NOEXEC_SEAL
+#define MFD_NOEXEC_SEAL	0x0008U
+#endif
+
 /*
  * Default is not to test hugetlbfs
  */
@@ -86,6 +95,37 @@ static int mfd_assert_new(const char *name, loff_t sz, unsigned int flags)
 	return fd;
 }
 
+static void sysctl_assert_write(const char *val)
+{
+	int fd = open("/proc/sys/vm/memfd_noexec", O_WRONLY | O_CLOEXEC);
+
+	if (fd < 0) {
+		printf("open sysctl failed\n");
+		abort();
+	}
+
+	if (write(fd, val, strlen(val)) < 0) {
+		printf("write sysctl failed\n");
+		abort();
+	}
+}
+
+static void sysctl_fail_write(const char *val)
+{
+	int fd = open("/proc/sys/vm/memfd_noexec", O_WRONLY | O_CLOEXEC);
+
+	if (fd < 0) {
+		printf("open sysctl failed\n");
+		abort();
+	}
+
+	if (write(fd, val, strlen(val)) >= 0) {
+		printf("write sysctl %s succeeded, but failure expected\n",
+				val);
+		abort();
+	}
+}
+
 static int mfd_assert_reopen_fd(int fd_in)
 {
 	int fd;
@@ -764,6 +804,9 @@ static void test_create(void)
 	mfd_fail_new("", ~0);
 	mfd_fail_new("", 0x80000000U);
 
+	/* verify EXEC and NOEXEC_SEAL can't both be set */
+	mfd_fail_new("", MFD_EXEC | MFD_NOEXEC_SEAL);
+
 	/* verify MFD_CLOEXEC is allowed */
 	fd = mfd_assert_new("", 0, MFD_CLOEXEC);
 	close(fd);
@@ -975,20 +1018,21 @@ static void test_seal_resize(void)
 
 /*
  * Test SEAL_EXEC
- * Test that chmod() cannot change x bits after sealing
+ * Test fd is created with exec and allow sealing.
+ * chmod() cannot change x bits after sealing.
  */
-static void test_seal_exec(void)
+static void test_exec_seal(void)
 {
 	int fd;
 
 	printf("%s SEAL-EXEC\n", memfd_str);
 
+	printf("%s	Apply SEAL_EXEC\n", memfd_str);
 	fd = mfd_assert_new("kern_memfd_seal_exec",
 			    mfd_def_size,
-			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_EXEC);
 
 	mfd_assert_mode(fd, 0777);
-
 	mfd_assert_chmod(fd, 0644);
 
 	mfd_assert_has_seals(fd, 0);
@@ -1002,10 +1046,181 @@ static void test_seal_exec(void)
 	mfd_fail_chmod(fd, 0700);
 	mfd_fail_chmod(fd, 0100);
 	mfd_assert_chmod(fd, 0666);
+	mfd_assert_write(fd);
+	close(fd);
+
+	printf("%s	Apply ALL_SEALS\n", memfd_str);
+	fd = mfd_assert_new("kern_memfd_seal_exec",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING | MFD_EXEC);
+
+	mfd_assert_mode(fd, 0777);
+	mfd_assert_chmod(fd, 0700);
+
+	mfd_assert_has_seals(fd, 0);
+	mfd_assert_add_seals(fd, F_SEAL_EXEC);
+	mfd_assert_has_seals(fd, F_WX_SEALS);
 
+	mfd_fail_chmod(fd, 0711);
+	mfd_fail_chmod(fd, 0600);
+	mfd_fail_write(fd);
+	close(fd);
+}
+
+/*
+ * Test EXEC_NO_SEAL
+ * Test fd is created with exec and not allow sealing.
+ */
+static void test_exec_no_seal(void)
+{
+	int fd;
+
+	printf("%s EXEC_NO_SEAL\n", memfd_str);
+
+	/* Create with EXEC but without ALLOW_SEALING */
+	fd = mfd_assert_new("kern_memfd_exec_no_sealing",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_EXEC);
+	mfd_assert_mode(fd, 0777);
+	mfd_assert_has_seals(fd, F_SEAL_SEAL);
+	mfd_assert_chmod(fd, 0666);
 	close(fd);
 }
 
+/*
+ * Test memfd_create with MFD_NOEXEC flag
+ */
+static void test_noexec_seal(void)
+{
+	int fd;
+
+	printf("%s NOEXEC_SEAL\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_SEAL);
+	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_SEAL);
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+}
+
+static void test_sysctl_child(void)
+{
+	int fd;
+
+	printf("%s sysctl 0\n", memfd_str);
+	sysctl_assert_write("0");
+	fd = mfd_assert_new("kern_memfd_sysctl_0",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+	mfd_assert_mode(fd, 0777);
+	mfd_assert_has_seals(fd, 0);
+	mfd_assert_chmod(fd, 0644);
+	close(fd);
+
+	printf("%s sysctl 1\n", memfd_str);
+	sysctl_assert_write("1");
+	fd = mfd_assert_new("kern_memfd_sysctl_1",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+	mfd_fail_chmod(fd, 0777);
+	sysctl_fail_write("0");
+	close(fd);
+
+	printf("%s sysctl 2\n", memfd_str);
+	sysctl_assert_write("2");
+	mfd_fail_new("kern_memfd_sysctl_2",
+		MFD_CLOEXEC | MFD_ALLOW_SEALING);
+	sysctl_fail_write("0");
+	sysctl_fail_write("1");
+}
+
+static int newpid_thread_fn(void *arg)
+{
+	test_sysctl_child();
+	return 0;
+}
+
+static void test_sysctl_child2(void)
+{
+	int fd;
+
+	sysctl_fail_write("0");
+	fd = mfd_assert_new("kern_memfd_sysctl_1",
+			    mfd_def_size,
+			    MFD_CLOEXEC | MFD_ALLOW_SEALING);
+
+	mfd_assert_mode(fd, 0666);
+	mfd_assert_has_seals(fd, F_SEAL_EXEC);
+	mfd_fail_chmod(fd, 0777);
+	close(fd);
+}
+
+static int newpid_thread_fn2(void *arg)
+{
+	test_sysctl_child2();
+	return 0;
+}
+static pid_t spawn_newpid_thread(unsigned int flags, int (*fn)(void *))
+{
+	uint8_t *stack;
+	pid_t pid;
+
+	stack = malloc(STACK_SIZE);
+	if (!stack) {
+		printf("malloc(STACK_SIZE) failed: %m\n");
+		abort();
+	}
+
+	pid = clone(fn,
+		    stack + STACK_SIZE,
+		    SIGCHLD | flags,
+		    NULL);
+	if (pid < 0) {
+		printf("clone() failed: %m\n");
+		abort();
+	}
+
+	return pid;
+}
+
+static void join_newpid_thread(pid_t pid)
+{
+	waitpid(pid, NULL, 0);
+}
+
+/*
+ * Test sysctl
+ * A very basic sealing test to see whether setting/retrieving seals works.
+ */
+static void test_sysctl(void)
+{
+	int pid = spawn_newpid_thread(CLONE_NEWPID, newpid_thread_fn);
+
+	join_newpid_thread(pid);
+
+	printf("%s child ns\n", memfd_str);
+	sysctl_assert_write("1");
+
+	pid = spawn_newpid_thread(CLONE_NEWPID, newpid_thread_fn2);
+	join_newpid_thread(pid);
+}
+
 /*
  * Test sharing via dup()
  * Test that seals are shared between dupped FDs and they're all equal.
@@ -1179,13 +1394,15 @@ int main(int argc, char **argv)
 
 	test_create();
 	test_basic();
+	test_exec_seal();
+	test_exec_no_seal();
+	test_noexec_seal();
 
 	test_seal_write();
 	test_seal_future_write();
 	test_seal_shrink();
 	test_seal_grow();
 	test_seal_resize();
-	test_seal_exec();
 
 	test_share_dup("SHARE-DUP", "");
 	test_share_mmap("SHARE-MMAP", "");
@@ -1201,6 +1418,8 @@ int main(int argc, char **argv)
 	test_share_fork("SHARE-FORK", SHARED_FT_STR);
 	join_idle_thread(pid);
 
+	test_sysctl();
+
 	printf("memfd: DONE\n");
 
 	return 0;
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

[PATCH v6 6/6] mm/memfd: security hook for memfd_create

[jeffxu]

From: Jeff Xu <jeffxu@google.com>

The new security_memfd_create allows lsm to check flags of
memfd_create.

The security by default system (such as chromeos) can use this
to implement system wide lsm to allow only non-executable memfd
being created.

Signed-off-by: Jeff Xu <jeffxu@google.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 include/linux/lsm_hook_defs.h |  1 +
 include/linux/lsm_hooks.h     |  4 ++++
 include/linux/security.h      |  6 ++++++
 mm/memfd.c                    |  5 +++++
 security/security.c           | 13 +++++++++++++
 5 files changed, 29 insertions(+)

diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
index ec119da1d89b..fd40840927c8 100644
--- a/include/linux/lsm_hook_defs.h
+++ b/include/linux/lsm_hook_defs.h
@@ -164,6 +164,7 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
 LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
 LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
 	 unsigned long arg)
+LSM_HOOK(int, 0, memfd_create, char *name, unsigned int flags)
 LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
 LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
 	 unsigned long prot, unsigned long flags)
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 4ec80b96c22e..5a18a6552278 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -543,6 +543,10 @@
  *	simple integer value.  When @arg represents a user space pointer, it
  *	should never be used by the security module.
  *	Return 0 if permission is granted.
+ * @memfd_create:
+ *	@name is the name of memfd file.
+ *	@flags is the flags used in memfd_create.
+ *	Return 0 if permission is granted.
  * @mmap_addr :
  *	Check permissions for a mmap operation at @addr.
  *	@addr contains virtual address that will be used for the operation.
diff --git a/include/linux/security.h b/include/linux/security.h
index ca1b7109c0db..5b87a780822a 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -384,6 +384,7 @@ int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);
 int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
+int security_memfd_create(char *name, unsigned int flags);
 int security_mmap_file(struct file *file, unsigned long prot,
 			unsigned long flags);
 int security_mmap_addr(unsigned long addr);
@@ -963,6 +964,11 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
 	return 0;
 }
 
+static inline int security_memfd_create(char *name, unsigned int flags)
+{
+	return 0;
+}
+
 static inline int security_mmap_file(struct file *file, unsigned long prot,
 				     unsigned long flags)
 {
diff --git a/mm/memfd.c b/mm/memfd.c
index 92f0a5765f7c..f04ed5f0474f 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -356,6 +356,11 @@ SYSCALL_DEFINE2(memfd_create,
 		goto err_name;
 	}
 
+	/* security hook for memfd_create */
+	error = security_memfd_create(name, flags);
+	if (error)
+		return error;
+
 	if (flags & MFD_HUGETLB) {
 		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
 					HUGETLB_ANONHUGE_INODE,
diff --git a/security/security.c b/security/security.c
index 79d82cb6e469..5c018e080923 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1010,6 +1010,19 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
 }
 EXPORT_SYMBOL(security_sb_clone_mnt_opts);
 
+int security_add_mnt_opt(const char *option, const char *val, int len,
+			 void **mnt_opts)
+{
+	return call_int_hook(sb_add_mnt_opt, -EINVAL,
+					option, val, len, mnt_opts);
+}
+EXPORT_SYMBOL(security_add_mnt_opt);
+
+int security_memfd_create(char *name, unsigned int flags)
+{
+	return call_int_hook(memfd_create, 0, name, flags);
+}
+
 int security_move_mount(const struct path *from_path, const struct path *to_path)
 {
 	return call_int_hook(move_mount, 0, from_path, to_path);
-- 
2.39.0.rc0.267.gcb52ba06e7-goog

Re: [PATCH v6 0/6] mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:33PM +0000, jeffxu@chromium.org wrote:
> This is V6 version of patch: see [4] [5] [6] [7] for previous versions.

When sending a new version, can you include an overview of what changed
between this version and the prior version? This helps reviewers who are
following along, so it's easier to focus our attention on the
differences. Also, it's helpful to version the links:

> [4] https://lwn.net/Articles/890096/
> [5] https://lore.kernel.org/lkml/20220805222126.142525-1-jeffxu@google.com/
> [6] https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
> [7] https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/

e.g.:

v6:
- moved foo to bar
- improve comments for baz
v5: https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/
v3: https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
v2: ...etc

-Kees

-- 
Kees Cook

Re: [PATCH v6 1/6] mm/memfd: add F_SEAL_EXEC

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:34PM +0000, jeffxu@chromium.org wrote:
> From: Daniel Verkamp <dverkamp@chromium.org>
> 
> 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>
> Co-developed-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Jeff Xu <jeffxu@google.com>

It looks like my Reviewed-by: tag was dropped?

https://lore.kernel.org/lkml/202212021443.0F684E33@keescook/

This patch is unchanged, so please carry forward any review/ack/tested
tags, etc.

But for the bots:

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v6 2/6] selftests/memfd: add tests for F_SEAL_EXEC

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:35PM +0000, jeffxu@chromium.org 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.
> 
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> Co-developed-by: Jeff Xu <jeffxu@google.com>
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> ---
>  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

I'd expect this to be named PATH_MAX, and it shouldn't need to have an
#ifdef? That's a regular POSIX define.

Otherwise, looks good. Though it'd be nice if this test use
kselftest_harness.h, but that's not your problem. :)

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:36PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
> 
> The new MFD_NOEXEC_SEAL and MFD_EXEC flags allows application to
> set executable bit at creation time (memfd_create).
> 
> When MFD_NOEXEC_SEAL is set, memfd is created without executable bit
> (mode:0666), and sealed with F_SEAL_EXEC, so it can't be chmod to
> be executable (mode: 0777) after creation.
> 
> when MFD_EXEC flag is set, memfd is created with executable bit
> (mode:0777), this is the same as the old behavior of memfd_create.
> 
> The new pid namespaced sysctl vm.memfd_noexec has 3 values:
> 0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
> 	MFD_EXEC was set.
> 1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
> 	MFD_NOEXEC_SEAL was set.
> 2: memfd_create() without MFD_NOEXEC_SEAL will be rejected.
> 
> The sysctl allows finer control of memfd_create for old-software
> that doesn't set the executable bit, for example, a container with
> vm.memfd_noexec=1 means the old-software will create non-executable
> memfd by default. Also, the value of memfd_noexec is passed to child
> namespace at creation time. For example, if the init namespace has
> vm.memfd_noexec=2, all its children namespaces will be created with 2.
> 
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> Reported-by: kernel test robot <lkp@intel.com>

Please rearrange these tags, and add a link to the lkp report:

  Reported-by: kernel test robot <lkp@intel.com>
  Link: ...url.to.lkp.lore.email...
  Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
  Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
  Signed-off-by: Jeff Xu <jeffxu@google.com>

> ---
>  include/linux/pid_namespace.h | 19 +++++++++++
>  include/uapi/linux/memfd.h    |  4 +++
>  kernel/pid_namespace.c        |  5 +++
>  kernel/pid_sysctl.h           | 59 +++++++++++++++++++++++++++++++++++
>  mm/memfd.c                    | 48 ++++++++++++++++++++++++++--
>  5 files changed, 133 insertions(+), 2 deletions(-)
>  create mode 100644 kernel/pid_sysctl.h
> 
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 07481bb87d4e..a4789a7b34a9 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -16,6 +16,21 @@
>  
>  struct fs_pin;
>  
> +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> +/*
> + * sysctl for vm.memfd_noexec
> + * 0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
> + *	acts like MFD_EXEC was set.
> + * 1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
> + *	acts like MFD_NOEXEC_SEAL was set.
> + * 2: memfd_create() without MFD_NOEXEC_SEAL will be
> + *	rejected.
> + */
> +#define MEMFD_NOEXEC_SCOPE_EXEC		0
> +#define MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL		1
> +#define MEMFD_NOEXEC_SCOPE_NOEXEC_ENFORCED	2

These don't align? I think a tab is missing on MEMFD_NOEXEC_SCOPE_EXEC.

> +#endif
> +
>  struct pid_namespace {
>  	struct idr idr;
>  	struct rcu_head rcu;
> @@ -31,6 +46,10 @@ struct pid_namespace {
>  	struct ucounts *ucounts;
>  	int reboot;	/* group exit code if this pidns was rebooted */
>  	struct ns_common ns;
> +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> +	/* sysctl for vm.memfd_noexec */
> +	int memfd_noexec_scope;
> +#endif
>  } __randomize_layout;
>  
>  extern struct pid_namespace init_pid_ns;
> diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
> index 7a8a26751c23..273a4e15dfcf 100644
> --- a/include/uapi/linux/memfd.h
> +++ b/include/uapi/linux/memfd.h
> @@ -8,6 +8,10 @@
>  #define MFD_CLOEXEC		0x0001U
>  #define MFD_ALLOW_SEALING	0x0002U
>  #define MFD_HUGETLB		0x0004U
> +/* not executable and sealed to prevent changing to executable. */
> +#define MFD_NOEXEC_SEAL		0x0008U
> +/* executable */
> +#define MFD_EXEC		0x0010U
>  
>  /*
>   * Huge page size encoding when MFD_HUGETLB is specified, and a huge page
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index f4f8cb0435b4..8a98b1af9376 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -23,6 +23,7 @@
>  #include <linux/sched/task.h>
>  #include <linux/sched/signal.h>
>  #include <linux/idr.h>
> +#include "pid_sysctl.h"
>  
>  static DEFINE_MUTEX(pid_caches_mutex);
>  static struct kmem_cache *pid_ns_cachep;
> @@ -110,6 +111,8 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
>  	ns->ucounts = ucounts;
>  	ns->pid_allocated = PIDNS_ADDING;
>  
> +	initialize_memfd_noexec_scope(ns);
> +
>  	return ns;
>  
>  out_free_idr:
> @@ -455,6 +458,8 @@ static __init int pid_namespaces_init(void)
>  #ifdef CONFIG_CHECKPOINT_RESTORE
>  	register_sysctl_paths(kern_path, pid_ns_ctl_table);
>  #endif
> +
> +	register_pid_ns_sysctl_table_vm();
>  	return 0;
>  }
>  
> diff --git a/kernel/pid_sysctl.h b/kernel/pid_sysctl.h
> new file mode 100644
> index 000000000000..5986d6493b5b
> --- /dev/null
> +++ b/kernel/pid_sysctl.h
> @@ -0,0 +1,59 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef LINUX_PID_SYSCTL_H
> +#define LINUX_PID_SYSCTL_H
> +
> +#include <linux/pid_namespace.h>
> +
> +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> +static inline void initialize_memfd_noexec_scope(struct pid_namespace *ns)
> +{
> +	ns->memfd_noexec_scope =
> +		task_active_pid_ns(current)->memfd_noexec_scope;
> +}
> +
> +static int pid_mfd_noexec_dointvec_minmax(struct ctl_table *table,
> +	int write, void *buf, size_t *lenp, loff_t *ppos)
> +{
> +	struct pid_namespace *ns = task_active_pid_ns(current);
> +	struct ctl_table table_copy;
> +
> +	if (write && !capable(CAP_SYS_ADMIN))
> +		return -EPERM;

Should this be CAP_SYS_ADMIN within the userns, rather than the global
init_task CAP_SYS_ADMIN?

> +
> +	table_copy = *table;
> +	if (ns != &init_pid_ns)
> +		table_copy.data = &ns->memfd_noexec_scope;
> +
> +	/*
> +	 * set minimum to current value, the effect is only bigger
> +	 * value is accepted.
> +	 */
> +	if (*(int *)table_copy.data > *(int *)table_copy.extra1)
> +		table_copy.extra1 = table_copy.data;
> +
> +	return proc_dointvec_minmax(&table_copy, write, buf, lenp, ppos);
> +}
> +
> +static struct ctl_table pid_ns_ctl_table_vm[] = {
> +	{
> +		.procname	= "memfd_noexec",
> +		.data		= &init_pid_ns.memfd_noexec_scope,
> +		.maxlen		= sizeof(init_pid_ns.memfd_noexec_scope),
> +		.mode		= 0644,
> +		.proc_handler	= pid_mfd_noexec_dointvec_minmax,
> +		.extra1		= SYSCTL_ZERO,
> +		.extra2		= SYSCTL_TWO,
> +	},
> +	{ }
> +};
> +static struct ctl_path vm_path[] = { { .procname = "vm", }, { } };
> +static inline void register_pid_ns_sysctl_table_vm(void)
> +{
> +	register_sysctl_paths(vm_path, pid_ns_ctl_table_vm);
> +}
> +#else
> +static inline void set_memfd_noexec_scope(struct pid_namespace *ns) {}
> +static inline void register_pid_ns_ctl_table_vm(void) {}
> +#endif
> +
> +#endif /* LINUX_PID_SYSCTL_H */
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 4ebeab94aa74..ec70675a7069 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -18,6 +18,7 @@
>  #include <linux/hugetlb.h>
>  #include <linux/shmem_fs.h>
>  #include <linux/memfd.h>
> +#include <linux/pid_namespace.h>
>  #include <uapi/linux/memfd.h>
>  
>  /*
> @@ -263,12 +264,14 @@ 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_SEAL | MFD_EXEC)
>  
>  SYSCALL_DEFINE2(memfd_create,
>  		const char __user *, uname,
>  		unsigned int, flags)
>  {
> +	char comm[TASK_COMM_LEN];

I'm fine with using "comm", but technically, it's not needed: task->comm
will always be %NUL terminated.

> +	struct pid_namespace *ns;
>  	unsigned int *file_seals;
>  	struct file *file;
>  	int fd, error;
> @@ -285,6 +288,39 @@ SYSCALL_DEFINE2(memfd_create,
>  			return -EINVAL;
>  	}
>  
> +	/* Invalid if both EXEC and NOEXEC_SEAL are set.*/
> +	if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
> +		return -EINVAL;
> +
> +	if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
> +#ifdef CONFIG_SYSCTL
> +		int sysctl = MEMFD_NOEXEC_SCOPE_EXEC;
> +
> +		ns = task_active_pid_ns(current);
> +		if (ns)
> +			sysctl = ns->memfd_noexec_scope;
> +
> +		switch (sysctl) {
> +		case MEMFD_NOEXEC_SCOPE_EXEC:
> +			flags |= MFD_EXEC;
> +			break;
> +		case MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL:
> +			flags |= MFD_NOEXEC_SEAL;
> +			break;
> +		default:
> +			pr_warn_ratelimited(
> +				"memfd_create(): MFD_NOEXEC_SEAL is enforced, pid=%d '%s'\n",
> +				task_pid_nr(current), get_task_comm(comm, current));
> +			return -EINVAL;
> +		}
> +#else
> +		flags |= MFD_EXEC;
> +#endif
> +		pr_warn_ratelimited(
> +			"memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> +			task_pid_nr(current), get_task_comm(comm, current));
> +	}
> +
>  	/* length includes terminating zero */
>  	len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
>  	if (len <= 0)
> @@ -328,7 +364,15 @@ SYSCALL_DEFINE2(memfd_create,
>  	file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
>  	file->f_flags |= O_LARGEFILE;
>  
> -	if (flags & MFD_ALLOW_SEALING) {
> +	if (flags & MFD_NOEXEC_SEAL) {
> +		struct inode *inode = file_inode(file);
> +
> +		inode->i_mode &= ~0111;
> +		file_seals = memfd_file_seals_ptr(file);
> +		*file_seals &= ~F_SEAL_SEAL;
> +		*file_seals |= F_SEAL_EXEC;
> +	} else if (flags & MFD_ALLOW_SEALING) {
> +		/* MFD_EXEC and MFD_ALLOW_SEALING are set */
>  		file_seals = memfd_file_seals_ptr(file);
>  		*file_seals &= ~F_SEAL_SEAL;
>  	}
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

Otherwise looks good!

-- 
Kees Cook

Re: [PATCH v6 4/6] mm/memfd: Add write seals when apply SEAL_EXEC to executable memfd

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:37PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
> 
> In order to avoid WX mappings, add F_SEAL_WRITE when apply
> F_SEAL_EXEC to an executable memfd, so W^X from start.
> 
> This implys application need to fill the content of the memfd first,
> after F_SEAL_EXEC is applied, application can no longer modify the
> content of the memfd.
> 
> Typically, application seals the memfd right after writing to it.
> For example:
> 1. memfd_create(MFD_EXEC).
> 2. write() code to the memfd.
> 3. fcntl(F_ADD_SEALS, F_SEAL_EXEC) to convert the memfd to W^X.
> 4. call exec() on the memfd.
> 
> Signed-off-by: Jeff Xu <jeffxu@google.com>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v6 5/6] selftests/memfd: add tests for MFD_NOEXEC_SEAL MFD_EXEC

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:38PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
> 
> Tests to verify MFD_NOEXEC, MFD_EXEC and vm.memfd_noexec sysctl.
> 
> Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
> Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>

Reviewed-by: Kees Cook <keescook@chromium.org>

-- 
Kees Cook

Re: [PATCH v6 6/6] mm/memfd: security hook for memfd_create

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:39PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
> 
> The new security_memfd_create allows lsm to check flags of
> memfd_create.
> 
> The security by default system (such as chromeos) can use this
> to implement system wide lsm to allow only non-executable memfd
> being created.
> 
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Reported-by: kernel test robot <lkp@intel.com>
> ---
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/lsm_hooks.h     |  4 ++++
>  include/linux/security.h      |  6 ++++++
>  mm/memfd.c                    |  5 +++++
>  security/security.c           | 13 +++++++++++++
>  5 files changed, 29 insertions(+)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b..fd40840927c8 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -164,6 +164,7 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
>  LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>  LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>  	 unsigned long arg)
> +LSM_HOOK(int, 0, memfd_create, char *name, unsigned int flags)
>  LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
>  LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
>  	 unsigned long prot, unsigned long flags)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..5a18a6552278 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -543,6 +543,10 @@
>   *	simple integer value.  When @arg represents a user space pointer, it
>   *	should never be used by the security module.
>   *	Return 0 if permission is granted.
> + * @memfd_create:
> + *	@name is the name of memfd file.
> + *	@flags is the flags used in memfd_create.
> + *	Return 0 if permission is granted.
>   * @mmap_addr :
>   *	Check permissions for a mmap operation at @addr.
>   *	@addr contains virtual address that will be used for the operation.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0db..5b87a780822a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -384,6 +384,7 @@ int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> +int security_memfd_create(char *name, unsigned int flags);
>  int security_mmap_file(struct file *file, unsigned long prot,
>  			unsigned long flags);
>  int security_mmap_addr(unsigned long addr);
> @@ -963,6 +964,11 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
>  	return 0;
>  }
>  
> +static inline int security_memfd_create(char *name, unsigned int flags)
> +{
> +	return 0;
> +}
> +
>  static inline int security_mmap_file(struct file *file, unsigned long prot,
>  				     unsigned long flags)
>  {
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 92f0a5765f7c..f04ed5f0474f 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -356,6 +356,11 @@ SYSCALL_DEFINE2(memfd_create,
>  		goto err_name;
>  	}
>  
> +	/* security hook for memfd_create */
> +	error = security_memfd_create(name, flags);
> +	if (error)
> +		return error;
> +
>  	if (flags & MFD_HUGETLB) {
>  		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
>  					HUGETLB_ANONHUGE_INODE,
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e469..5c018e080923 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1010,6 +1010,19 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>  }
>  EXPORT_SYMBOL(security_sb_clone_mnt_opts);
>  
> +int security_add_mnt_opt(const char *option, const char *val, int len,
> +			 void **mnt_opts)
> +{
> +	return call_int_hook(sb_add_mnt_opt, -EINVAL,
> +					option, val, len, mnt_opts);
> +}
> +EXPORT_SYMBOL(security_add_mnt_opt);

I think security_add_mnt_opt() isn't supposed to be in here. :)

> +
> +int security_memfd_create(char *name, unsigned int flags)
> +{
> +	return call_int_hook(memfd_create, 0, name, flags);
> +}
> +
>  int security_move_mount(const struct path *from_path, const struct path *to_path)
>  {
>  	return call_int_hook(move_mount, 0, from_path, to_path);
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

Otherwise looks good.

Thanks!

-Kees

-- 
Kees Cook

Re: [PATCH v6 6/6] mm/memfd: security hook for memfd_create

[Kees Cook]

On Wed, Dec 07, 2022 at 03:49:39PM +0000, jeffxu@chromium.org wrote:
> From: Jeff Xu <jeffxu@google.com>
> 
> The new security_memfd_create allows lsm to check flags of
> memfd_create.
> 
> The security by default system (such as chromeos) can use this
> to implement system wide lsm to allow only non-executable memfd
> being created.
> 
> Signed-off-by: Jeff Xu <jeffxu@google.com>
> Reported-by: kernel test robot <lkp@intel.com>

Oh, btw, please CC linux-security-module@vger.kernel.org when adding new
hooks. (I've added the CC here.)

-Kees

> ---
>  include/linux/lsm_hook_defs.h |  1 +
>  include/linux/lsm_hooks.h     |  4 ++++
>  include/linux/security.h      |  6 ++++++
>  mm/memfd.c                    |  5 +++++
>  security/security.c           | 13 +++++++++++++
>  5 files changed, 29 insertions(+)
> 
> diff --git a/include/linux/lsm_hook_defs.h b/include/linux/lsm_hook_defs.h
> index ec119da1d89b..fd40840927c8 100644
> --- a/include/linux/lsm_hook_defs.h
> +++ b/include/linux/lsm_hook_defs.h
> @@ -164,6 +164,7 @@ LSM_HOOK(int, 0, file_alloc_security, struct file *file)
>  LSM_HOOK(void, LSM_RET_VOID, file_free_security, struct file *file)
>  LSM_HOOK(int, 0, file_ioctl, struct file *file, unsigned int cmd,
>  	 unsigned long arg)
> +LSM_HOOK(int, 0, memfd_create, char *name, unsigned int flags)
>  LSM_HOOK(int, 0, mmap_addr, unsigned long addr)
>  LSM_HOOK(int, 0, mmap_file, struct file *file, unsigned long reqprot,
>  	 unsigned long prot, unsigned long flags)
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 4ec80b96c22e..5a18a6552278 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -543,6 +543,10 @@
>   *	simple integer value.  When @arg represents a user space pointer, it
>   *	should never be used by the security module.
>   *	Return 0 if permission is granted.
> + * @memfd_create:
> + *	@name is the name of memfd file.
> + *	@flags is the flags used in memfd_create.
> + *	Return 0 if permission is granted.
>   * @mmap_addr :
>   *	Check permissions for a mmap operation at @addr.
>   *	@addr contains virtual address that will be used for the operation.
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ca1b7109c0db..5b87a780822a 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -384,6 +384,7 @@ int security_file_permission(struct file *file, int mask);
>  int security_file_alloc(struct file *file);
>  void security_file_free(struct file *file);
>  int security_file_ioctl(struct file *file, unsigned int cmd, unsigned long arg);
> +int security_memfd_create(char *name, unsigned int flags);
>  int security_mmap_file(struct file *file, unsigned long prot,
>  			unsigned long flags);
>  int security_mmap_addr(unsigned long addr);
> @@ -963,6 +964,11 @@ static inline int security_file_ioctl(struct file *file, unsigned int cmd,
>  	return 0;
>  }
>  
> +static inline int security_memfd_create(char *name, unsigned int flags)
> +{
> +	return 0;
> +}
> +
>  static inline int security_mmap_file(struct file *file, unsigned long prot,
>  				     unsigned long flags)
>  {
> diff --git a/mm/memfd.c b/mm/memfd.c
> index 92f0a5765f7c..f04ed5f0474f 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -356,6 +356,11 @@ SYSCALL_DEFINE2(memfd_create,
>  		goto err_name;
>  	}
>  
> +	/* security hook for memfd_create */
> +	error = security_memfd_create(name, flags);
> +	if (error)
> +		return error;
> +
>  	if (flags & MFD_HUGETLB) {
>  		file = hugetlb_file_setup(name, 0, VM_NORESERVE,
>  					HUGETLB_ANONHUGE_INODE,
> diff --git a/security/security.c b/security/security.c
> index 79d82cb6e469..5c018e080923 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1010,6 +1010,19 @@ int security_sb_clone_mnt_opts(const struct super_block *oldsb,
>  }
>  EXPORT_SYMBOL(security_sb_clone_mnt_opts);
>  
> +int security_add_mnt_opt(const char *option, const char *val, int len,
> +			 void **mnt_opts)
> +{
> +	return call_int_hook(sb_add_mnt_opt, -EINVAL,
> +					option, val, len, mnt_opts);
> +}
> +EXPORT_SYMBOL(security_add_mnt_opt);
> +
> +int security_memfd_create(char *name, unsigned int flags)
> +{
> +	return call_int_hook(memfd_create, 0, name, flags);
> +}
> +
>  int security_move_mount(const struct path *from_path, const struct path *to_path)
>  {
>  	return call_int_hook(move_mount, 0, from_path, to_path);
> -- 
> 2.39.0.rc0.267.gcb52ba06e7-goog
> 

-- 
Kees Cook

Re: [PATCH v6 0/6] mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Thu, Dec 8, 2022 at 8:13 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Dec 07, 2022 at 03:49:33PM +0000, jeffxu@chromium.org wrote:
> > This is V6 version of patch: see [4] [5] [6] [7] for previous versions.
>
> When sending a new version, can you include an overview of what changed
> between this version and the prior version? This helps reviewers who are
> following along, so it's easier to focus our attention on the
> differences. Also, it's helpful to version the links:
>
> > [4] https://lwn.net/Articles/890096/
> > [5] https://lore.kernel.org/lkml/20220805222126.142525-1-jeffxu@google.com/
> > [6] https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
> > [7] https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/
>
> e.g.:
>
> v6:
> - moved foo to bar
> - improve comments for baz
> v5: https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/
> v3: https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
> v2: ...etc
>
Will do!
Much appreciated for helping me  through the process of my first patch
in the kernel.

Jeff

> -Kees
>
> --
> Kees Cook

Re: [PATCH v6 0/6] mm/memfd: introduce MFD_NOEXEC_SEAL and MFD_EXEC

[Kees Cook]

On Thu, Dec 08, 2022 at 10:33:19AM -0800, Jeff Xu wrote:
> On Thu, Dec 8, 2022 at 8:13 AM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Wed, Dec 07, 2022 at 03:49:33PM +0000, jeffxu@chromium.org wrote:
> > > This is V6 version of patch: see [4] [5] [6] [7] for previous versions.
> >
> > When sending a new version, can you include an overview of what changed
> > between this version and the prior version? This helps reviewers who are
> > following along, so it's easier to focus our attention on the
> > differences. Also, it's helpful to version the links:
> >
> > > [4] https://lwn.net/Articles/890096/
> > > [5] https://lore.kernel.org/lkml/20220805222126.142525-1-jeffxu@google.com/
> > > [6] https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
> > > [7] https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/
> >
> > e.g.:
> >
> > v6:
> > - moved foo to bar
> > - improve comments for baz
> > v5: https://lore.kernel.org/lkml/20221206152358.1966099-1-jeffxu@google.com/
> > v3: https://lore.kernel.org/lkml/20221202013404.163143-1-jeffxu@google.com/
> > v2: ...etc
> >
> Will do!
> Much appreciated for helping me  through the process of my first patch
> in the kernel.

Happy to help! I'm excited to see this gap in memfd security closed. :)

-- 
Kees Cook

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Thu, Dec 8, 2022 at 8:27 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Dec 07, 2022 at 03:49:36PM +0000, jeffxu@chromium.org wrote:
> > From: Jeff Xu <jeffxu@google.com>
> >
> > The new MFD_NOEXEC_SEAL and MFD_EXEC flags allows application to
> > set executable bit at creation time (memfd_create).
> >
> > When MFD_NOEXEC_SEAL is set, memfd is created without executable bit
> > (mode:0666), and sealed with F_SEAL_EXEC, so it can't be chmod to
> > be executable (mode: 0777) after creation.
> >
> > when MFD_EXEC flag is set, memfd is created with executable bit
> > (mode:0777), this is the same as the old behavior of memfd_create.
> >
> > The new pid namespaced sysctl vm.memfd_noexec has 3 values:
> > 0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
> >       MFD_EXEC was set.
> > 1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL acts like
> >       MFD_NOEXEC_SEAL was set.
> > 2: memfd_create() without MFD_NOEXEC_SEAL will be rejected.
> >
> > The sysctl allows finer control of memfd_create for old-software
> > that doesn't set the executable bit, for example, a container with
> > vm.memfd_noexec=1 means the old-software will create non-executable
> > memfd by default. Also, the value of memfd_noexec is passed to child
> > namespace at creation time. For example, if the init namespace has
> > vm.memfd_noexec=2, all its children namespaces will be created with 2.
> >
> > Signed-off-by: Jeff Xu <jeffxu@google.com>
> > Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
> > Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
> > Reported-by: kernel test robot <lkp@intel.com>
>
> Please rearrange these tags, and add a link to the lkp report:
>
>   Reported-by: kernel test robot <lkp@intel.com>
>   Link: ...url.to.lkp.lore.email...
>   Co-developed-by: Daniel Verkamp <dverkamp@chromium.org>
>   Signed-off-by: Daniel Verkamp <dverkamp@chromium.org>
>   Signed-off-by: Jeff Xu <jeffxu@google.com>
>
> > ---
> >  include/linux/pid_namespace.h | 19 +++++++++++
> >  include/uapi/linux/memfd.h    |  4 +++
> >  kernel/pid_namespace.c        |  5 +++
> >  kernel/pid_sysctl.h           | 59 +++++++++++++++++++++++++++++++++++
> >  mm/memfd.c                    | 48 ++++++++++++++++++++++++++--
> >  5 files changed, 133 insertions(+), 2 deletions(-)
> >  create mode 100644 kernel/pid_sysctl.h
> >
> > diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> > index 07481bb87d4e..a4789a7b34a9 100644
> > --- a/include/linux/pid_namespace.h
> > +++ b/include/linux/pid_namespace.h
> > @@ -16,6 +16,21 @@
> >
> >  struct fs_pin;
> >
> > +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> > +/*
> > + * sysctl for vm.memfd_noexec
> > + * 0: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
> > + *   acts like MFD_EXEC was set.
> > + * 1: memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL
> > + *   acts like MFD_NOEXEC_SEAL was set.
> > + * 2: memfd_create() without MFD_NOEXEC_SEAL will be
> > + *   rejected.
> > + */
> > +#define MEMFD_NOEXEC_SCOPE_EXEC              0
> > +#define MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL               1
> > +#define MEMFD_NOEXEC_SCOPE_NOEXEC_ENFORCED   2
>
> These don't align? I think a tab is missing on MEMFD_NOEXEC_SCOPE_EXEC.
>
Done

> > +#endif
> > +
> >  struct pid_namespace {
> >       struct idr idr;
> >       struct rcu_head rcu;
> > @@ -31,6 +46,10 @@ struct pid_namespace {
> >       struct ucounts *ucounts;
> >       int reboot;     /* group exit code if this pidns was rebooted */
> >       struct ns_common ns;
> > +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> > +     /* sysctl for vm.memfd_noexec */
> > +     int memfd_noexec_scope;
> > +#endif
> >  } __randomize_layout;
> >
> >  extern struct pid_namespace init_pid_ns;
> > diff --git a/include/uapi/linux/memfd.h b/include/uapi/linux/memfd.h
> > index 7a8a26751c23..273a4e15dfcf 100644
> > --- a/include/uapi/linux/memfd.h
> > +++ b/include/uapi/linux/memfd.h
> > @@ -8,6 +8,10 @@
> >  #define MFD_CLOEXEC          0x0001U
> >  #define MFD_ALLOW_SEALING    0x0002U
> >  #define MFD_HUGETLB          0x0004U
> > +/* not executable and sealed to prevent changing to executable. */
> > +#define MFD_NOEXEC_SEAL              0x0008U
> > +/* executable */
> > +#define MFD_EXEC             0x0010U
> >
> >  /*
> >   * Huge page size encoding when MFD_HUGETLB is specified, and a huge page
> > diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> > index f4f8cb0435b4..8a98b1af9376 100644
> > --- a/kernel/pid_namespace.c
> > +++ b/kernel/pid_namespace.c
> > @@ -23,6 +23,7 @@
> >  #include <linux/sched/task.h>
> >  #include <linux/sched/signal.h>
> >  #include <linux/idr.h>
> > +#include "pid_sysctl.h"
> >
> >  static DEFINE_MUTEX(pid_caches_mutex);
> >  static struct kmem_cache *pid_ns_cachep;
> > @@ -110,6 +111,8 @@ static struct pid_namespace *create_pid_namespace(struct user_namespace *user_ns
> >       ns->ucounts = ucounts;
> >       ns->pid_allocated = PIDNS_ADDING;
> >
> > +     initialize_memfd_noexec_scope(ns);
> > +
> >       return ns;
> >
> >  out_free_idr:
> > @@ -455,6 +458,8 @@ static __init int pid_namespaces_init(void)
> >  #ifdef CONFIG_CHECKPOINT_RESTORE
> >       register_sysctl_paths(kern_path, pid_ns_ctl_table);
> >  #endif
> > +
> > +     register_pid_ns_sysctl_table_vm();
> >       return 0;
> >  }
> >
> > diff --git a/kernel/pid_sysctl.h b/kernel/pid_sysctl.h
> > new file mode 100644
> > index 000000000000..5986d6493b5b
> > --- /dev/null
> > +++ b/kernel/pid_sysctl.h
> > @@ -0,0 +1,59 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef LINUX_PID_SYSCTL_H
> > +#define LINUX_PID_SYSCTL_H
> > +
> > +#include <linux/pid_namespace.h>
> > +
> > +#if defined(CONFIG_SYSCTL) && defined(CONFIG_MEMFD_CREATE)
> > +static inline void initialize_memfd_noexec_scope(struct pid_namespace *ns)
> > +{
> > +     ns->memfd_noexec_scope =
> > +             task_active_pid_ns(current)->memfd_noexec_scope;
> > +}
> > +
> > +static int pid_mfd_noexec_dointvec_minmax(struct ctl_table *table,
> > +     int write, void *buf, size_t *lenp, loff_t *ppos)
> > +{
> > +     struct pid_namespace *ns = task_active_pid_ns(current);
> > +     struct ctl_table table_copy;
> > +
> > +     if (write && !capable(CAP_SYS_ADMIN))
> > +             return -EPERM;
>
> Should this be CAP_SYS_ADMIN within the userns, rather than the global
> init_task CAP_SYS_ADMIN?
>
Done.

> > +
> > +     table_copy = *table;
> > +     if (ns != &init_pid_ns)
> > +             table_copy.data = &ns->memfd_noexec_scope;
> > +
> > +     /*
> > +      * set minimum to current value, the effect is only bigger
> > +      * value is accepted.
> > +      */
> > +     if (*(int *)table_copy.data > *(int *)table_copy.extra1)
> > +             table_copy.extra1 = table_copy.data;
> > +
> > +     return proc_dointvec_minmax(&table_copy, write, buf, lenp, ppos);
> > +}
> > +
> > +static struct ctl_table pid_ns_ctl_table_vm[] = {
> > +     {
> > +             .procname       = "memfd_noexec",
> > +             .data           = &init_pid_ns.memfd_noexec_scope,
> > +             .maxlen         = sizeof(init_pid_ns.memfd_noexec_scope),
> > +             .mode           = 0644,
> > +             .proc_handler   = pid_mfd_noexec_dointvec_minmax,
> > +             .extra1         = SYSCTL_ZERO,
> > +             .extra2         = SYSCTL_TWO,
> > +     },
> > +     { }
> > +};
> > +static struct ctl_path vm_path[] = { { .procname = "vm", }, { } };
> > +static inline void register_pid_ns_sysctl_table_vm(void)
> > +{
> > +     register_sysctl_paths(vm_path, pid_ns_ctl_table_vm);
> > +}
> > +#else
> > +static inline void set_memfd_noexec_scope(struct pid_namespace *ns) {}
> > +static inline void register_pid_ns_ctl_table_vm(void) {}
> > +#endif
> > +
> > +#endif /* LINUX_PID_SYSCTL_H */
> > diff --git a/mm/memfd.c b/mm/memfd.c
> > index 4ebeab94aa74..ec70675a7069 100644
> > --- a/mm/memfd.c
> > +++ b/mm/memfd.c
> > @@ -18,6 +18,7 @@
> >  #include <linux/hugetlb.h>
> >  #include <linux/shmem_fs.h>
> >  #include <linux/memfd.h>
> > +#include <linux/pid_namespace.h>
> >  #include <uapi/linux/memfd.h>
> >
> >  /*
> > @@ -263,12 +264,14 @@ 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_SEAL | MFD_EXEC)
> >
> >  SYSCALL_DEFINE2(memfd_create,
> >               const char __user *, uname,
> >               unsigned int, flags)
> >  {
> > +     char comm[TASK_COMM_LEN];
>
> I'm fine with using "comm", but technically, it's not needed: task->comm
> will always be %NUL terminated.
>
get_task_comm takes a lock.
Do we need to consider the case of task->comm mutation in a
multithreaded environment ?
There seems to be work related with replacing task->comm with
get_task_comm, such as:
https://lore.kernel.org/netdev/20211108083840.4627-4-laoar.shao@gmail.com/

> > +     struct pid_namespace *ns;
> >       unsigned int *file_seals;
> >       struct file *file;
> >       int fd, error;
> > @@ -285,6 +288,39 @@ SYSCALL_DEFINE2(memfd_create,
> >                       return -EINVAL;
> >       }
> >
> > +     /* Invalid if both EXEC and NOEXEC_SEAL are set.*/
> > +     if ((flags & MFD_EXEC) && (flags & MFD_NOEXEC_SEAL))
> > +             return -EINVAL;
> > +
> > +     if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
> > +#ifdef CONFIG_SYSCTL
> > +             int sysctl = MEMFD_NOEXEC_SCOPE_EXEC;
> > +
> > +             ns = task_active_pid_ns(current);
> > +             if (ns)
> > +                     sysctl = ns->memfd_noexec_scope;
> > +
> > +             switch (sysctl) {
> > +             case MEMFD_NOEXEC_SCOPE_EXEC:
> > +                     flags |= MFD_EXEC;
> > +                     break;
> > +             case MEMFD_NOEXEC_SCOPE_NOEXEC_SEAL:
> > +                     flags |= MFD_NOEXEC_SEAL;
> > +                     break;
> > +             default:
> > +                     pr_warn_ratelimited(
> > +                             "memfd_create(): MFD_NOEXEC_SEAL is enforced, pid=%d '%s'\n",
> > +                             task_pid_nr(current), get_task_comm(comm, current));
> > +                     return -EINVAL;
> > +             }
> > +#else
> > +             flags |= MFD_EXEC;
> > +#endif
> > +             pr_warn_ratelimited(
> > +                     "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> > +                     task_pid_nr(current), get_task_comm(comm, current));
> > +     }
> > +
> >       /* length includes terminating zero */
> >       len = strnlen_user(uname, MFD_NAME_MAX_LEN + 1);
> >       if (len <= 0)
> > @@ -328,7 +364,15 @@ SYSCALL_DEFINE2(memfd_create,
> >       file->f_mode |= FMODE_LSEEK | FMODE_PREAD | FMODE_PWRITE;
> >       file->f_flags |= O_LARGEFILE;
> >
> > -     if (flags & MFD_ALLOW_SEALING) {
> > +     if (flags & MFD_NOEXEC_SEAL) {
> > +             struct inode *inode = file_inode(file);
> > +
> > +             inode->i_mode &= ~0111;
> > +             file_seals = memfd_file_seals_ptr(file);
> > +             *file_seals &= ~F_SEAL_SEAL;
> > +             *file_seals |= F_SEAL_EXEC;
> > +     } else if (flags & MFD_ALLOW_SEALING) {
> > +             /* MFD_EXEC and MFD_ALLOW_SEALING are set */
> >               file_seals = memfd_file_seals_ptr(file);
> >               *file_seals &= ~F_SEAL_SEAL;
> >       }
> > --
> > 2.39.0.rc0.267.gcb52ba06e7-goog
> >
>
> Otherwise looks good!
>
> --
> Kees Cook

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Peter Xu]

Hi, Jeff,

On Thu, Dec 08, 2022 at 02:55:45PM -0800, Jeff Xu wrote:
> > > +     if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {

[...]

> > > +             pr_warn_ratelimited(
> > > +                     "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> > > +                     task_pid_nr(current), get_task_comm(comm, current));

This will be frequently dumped right now with mm-unstable.  Is that what it
wanted to achieve?

[   10.822575] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=491 'systemd'
[   10.824743] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=495 '(sd-executor)'
...

If there's already a sane default value (and also knobs for the user to
change the default) not sure whether it's saner to just keep it silent as
before?

-- 
Peter Xu

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Fri, Dec 16, 2022 at 7:47 AM Peter Xu <peterx@redhat.com> wrote:
>
> Hi, Jeff,
>
> On Thu, Dec 08, 2022 at 02:55:45PM -0800, Jeff Xu wrote:
> > > > +     if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
>
> [...]
>
> > > > +             pr_warn_ratelimited(
> > > > +                     "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> > > > +                     task_pid_nr(current), get_task_comm(comm, current));
>
> This will be frequently dumped right now with mm-unstable.  Is that what it
> wanted to achieve?
>
> [   10.822575] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=491 'systemd'
> [   10.824743] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=495 '(sd-executor)'
> ...
>
> If there's already a sane default value (and also knobs for the user to
> change the default) not sure whether it's saner to just keep it silent as
> before?
>
Thanks for your comments.

The intention is it is a reminder to adjust API calls to explicitly
setting this bit.
The sysctl vm.memfd_noexec = 0 1 is for transaction to the final
state, and 2 depends on API call setting this bit.

The log is ratelimited, and there is a rate limit setting:
/proc/sys/kernel/printk_ratelimit
/proc/sys/kernel/printk_ratelimit_burst

Best regards,
Jeff

> --
> Peter Xu
>

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Andrew Morton]

On Fri, 16 Dec 2022 09:15:40 -0800 Jeff Xu <jeffxu@google.com> wrote:

> On Fri, Dec 16, 2022 at 7:47 AM Peter Xu <peterx@redhat.com> wrote:
> >
> > Hi, Jeff,
> >
> > On Thu, Dec 08, 2022 at 02:55:45PM -0800, Jeff Xu wrote:
> > > > > +     if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
> >
> > [...]
> >
> > > > > +             pr_warn_ratelimited(
> > > > > +                     "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> > > > > +                     task_pid_nr(current), get_task_comm(comm, current));
> >
> > This will be frequently dumped right now with mm-unstable.  Is that what it
> > wanted to achieve?
> >
> > [   10.822575] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=491 'systemd'
> > [   10.824743] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=495 '(sd-executor)'
> > ...
> >
> > If there's already a sane default value (and also knobs for the user to
> > change the default) not sure whether it's saner to just keep it silent as
> > before?
> >
> Thanks for your comments.
> 
> The intention is it is a reminder to adjust API calls to explicitly
> setting this bit.

Do we need to warn more than once per boot?  If not, use pr_warn_once()?

> The sysctl vm.memfd_noexec = 0 1 is for transaction to the final
> state, and 2 depends on API call setting this bit.
> 
> The log is ratelimited, and there is a rate limit setting:
> /proc/sys/kernel/printk_ratelimit
> /proc/sys/kernel/printk_ratelimit_burst
> 

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Fri, Dec 16, 2022 at 9:43 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 16 Dec 2022 09:15:40 -0800 Jeff Xu <jeffxu@google.com> wrote:
>
> > On Fri, Dec 16, 2022 at 7:47 AM Peter Xu <peterx@redhat.com> wrote:
> > >
> > > Hi, Jeff,
> > >
> > > On Thu, Dec 08, 2022 at 02:55:45PM -0800, Jeff Xu wrote:
> > > > > > +     if (!(flags & (MFD_EXEC | MFD_NOEXEC_SEAL))) {
> > >
> > > [...]
> > >
> > > > > > +             pr_warn_ratelimited(
> > > > > > +                     "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
> > > > > > +                     task_pid_nr(current), get_task_comm(comm, current));
> > >
> > > This will be frequently dumped right now with mm-unstable.  Is that what it
> > > wanted to achieve?
> > >
> > > [   10.822575] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=491 'systemd'
> > > [   10.824743] memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=495 '(sd-executor)'
> > > ...
> > >
> > > If there's already a sane default value (and also knobs for the user to
> > > change the default) not sure whether it's saner to just keep it silent as
> > > before?
> > >
> > Thanks for your comments.
> >
> > The intention is it is a reminder to adjust API calls to explicitly
> > setting this bit.
>
> Do we need to warn more than once per boot?  If not, use pr_warn_once()?
>
Once per boot seems too little, it would be nice if we can list all processes.
I agree ratelimited might be too much.
There is a feature gap here for logging.

Kees, what do you think ?


> > The sysctl vm.memfd_noexec = 0 1 is for transaction to the final
> > state, and 2 depends on API call setting this bit.
> >
> > The log is ratelimited, and there is a rate limit setting:
> > /proc/sys/kernel/printk_ratelimit
> > /proc/sys/kernel/printk_ratelimit_burst
> >
>

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Kees Cook]

On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
> Once per boot seems too little, it would be nice if we can list all processes.
> I agree ratelimited might be too much.
> There is a feature gap here for logging.
> 
> Kees, what do you think ?

I agree once per boot is kind of frustrating "I fixed the one warning,
oh, now it's coming from a different process". But ratelimit is, in
retrospect, still too often.

Let's go with per boot -- this should be noisy "enough" to get the
changes in API into the callers without being too much of a hassle.

-- 
Kees Cook

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Fri, Dec 16, 2022 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
> > Once per boot seems too little, it would be nice if we can list all processes.
> > I agree ratelimited might be too much.
> > There is a feature gap here for logging.
> >
> > Kees, what do you think ?
>
> I agree once per boot is kind of frustrating "I fixed the one warning,
> oh, now it's coming from a different process". But ratelimit is, in
> retrospect, still too often.
>
> Let's go with per boot -- this should be noisy "enough" to get the
> changes in API into the callers without being too much of a hassle.
>
Agreed.  Let's go with per boot.

Hi Andrew, what is your preference ? I can send a patch  or you
directly fix it in mm-unstable ?

Thanks
-Jeff

> --
> Kees Cook

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Andrew Morton]

On Fri, 16 Dec 2022 13:46:58 -0800 Jeff Xu <jeffxu@google.com> wrote:

> On Fri, Dec 16, 2022 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
> >
> > On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
> > > Once per boot seems too little, it would be nice if we can list all processes.
> > > I agree ratelimited might be too much.
> > > There is a feature gap here for logging.
> > >
> > > Kees, what do you think ?
> >
> > I agree once per boot is kind of frustrating "I fixed the one warning,
> > oh, now it's coming from a different process". But ratelimit is, in
> > retrospect, still too often.
> >
> > Let's go with per boot -- this should be noisy "enough" to get the
> > changes in API into the callers without being too much of a hassle.
> >
> Agreed.  Let's go with per boot.
> 
> Hi Andrew, what is your preference ? I can send a patch  or you
> directly fix it in mm-unstable ?

Like this?

--- a/mm/memfd.c~mm-memfd-add-mfd_noexec_seal-and-mfd_exec-fix-3
+++ a/mm/memfd.c
@@ -308,7 +308,7 @@ SYSCALL_DEFINE2(memfd_create,
 			flags |= MFD_NOEXEC_SEAL;
 			break;
 		default:
-			pr_warn_ratelimited(
+			pr_warn_once(
 				"memfd_create(): MFD_NOEXEC_SEAL is enforced, pid=%d '%s'\n",
 				task_pid_nr(current), get_task_comm(comm, current));
 			return -EINVAL;
@@ -316,7 +316,7 @@ SYSCALL_DEFINE2(memfd_create,
 #else
 		flags |= MFD_EXEC;
 #endif
-		pr_warn_ratelimited(
+		pr_warn_once(
 			"memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
 			task_pid_nr(current), get_task_comm(comm, current));
 	}
_

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Fri, Dec 16, 2022 at 2:06 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Fri, 16 Dec 2022 13:46:58 -0800 Jeff Xu <jeffxu@google.com> wrote:
>
> > On Fri, Dec 16, 2022 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
> > > > Once per boot seems too little, it would be nice if we can list all processes.
> > > > I agree ratelimited might be too much.
> > > > There is a feature gap here for logging.
> > > >
> > > > Kees, what do you think ?
> > >
> > > I agree once per boot is kind of frustrating "I fixed the one warning,
> > > oh, now it's coming from a different process". But ratelimit is, in
> > > retrospect, still too often.
> > >
> > > Let's go with per boot -- this should be noisy "enough" to get the
> > > changes in API into the callers without being too much of a hassle.
> > >
> > Agreed.  Let's go with per boot.
> >
> > Hi Andrew, what is your preference ? I can send a patch  or you
> > directly fix it in mm-unstable ?
>
> Like this?
>
Yes. Thanks!

> --- a/mm/memfd.c~mm-memfd-add-mfd_noexec_seal-and-mfd_exec-fix-3
> +++ a/mm/memfd.c
> @@ -308,7 +308,7 @@ SYSCALL_DEFINE2(memfd_create,
>                         flags |= MFD_NOEXEC_SEAL;
>                         break;
>                 default:
> -                       pr_warn_ratelimited(
> +                       pr_warn_once(
>                                 "memfd_create(): MFD_NOEXEC_SEAL is enforced, pid=%d '%s'\n",
>                                 task_pid_nr(current), get_task_comm(comm, current));
>                         return -EINVAL;
> @@ -316,7 +316,7 @@ SYSCALL_DEFINE2(memfd_create,
>  #else
>                 flags |= MFD_EXEC;
>  #endif
> -               pr_warn_ratelimited(
> +               pr_warn_once(
>                         "memfd_create() without MFD_EXEC nor MFD_NOEXEC_SEAL, pid=%d '%s'\n",
>                         task_pid_nr(current), get_task_comm(comm, current));
>         }
> _
>

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Shuah Khan]

On 12/16/22 16:40, Jeff Xu wrote:
> On Fri, Dec 16, 2022 at 2:06 PM Andrew Morton <akpm@linux-foundation.org> wrote:
>>
>> On Fri, 16 Dec 2022 13:46:58 -0800 Jeff Xu <jeffxu@google.com> wrote:
>>
>>> On Fri, Dec 16, 2022 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
>>>>
>>>> On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
>>>>> Once per boot seems too little, it would be nice if we can list all processes.
>>>>> I agree ratelimited might be too much.
>>>>> There is a feature gap here for logging.
>>>>>
>>>>> Kees, what do you think ?
>>>>
>>>> I agree once per boot is kind of frustrating "I fixed the one warning,
>>>> oh, now it's coming from a different process". But ratelimit is, in
>>>> retrospect, still too often.
>>>>
>>>> Let's go with per boot -- this should be noisy "enough" to get the
>>>> changes in API into the callers without being too much of a hassle.
>>>>
>>> Agreed.  Let's go with per boot.
>>>
>>> Hi Andrew, what is your preference ? I can send a patch  or you
>>> directly fix it in mm-unstable ?
>>
>> Like this?
>>
> Yes. Thanks!
> 

Sorry jumping into this discussion a bit late. Is it possible to provide
a way to enable full logging as a debug option to tag more processes?

thanks,
-- Shuah

Re: [PATCH v6 3/6] mm/memfd: add MFD_NOEXEC_SEAL and MFD_EXEC

[Jeff Xu]

On Tue, Dec 20, 2022 at 8:55 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> On 12/16/22 16:40, Jeff Xu wrote:
> > On Fri, Dec 16, 2022 at 2:06 PM Andrew Morton <akpm@linux-foundation.org> wrote:
> >>
> >> On Fri, 16 Dec 2022 13:46:58 -0800 Jeff Xu <jeffxu@google.com> wrote:
> >>
> >>> On Fri, Dec 16, 2022 at 12:35 PM Kees Cook <keescook@chromium.org> wrote:
> >>>>
> >>>> On Fri, Dec 16, 2022 at 10:11:44AM -0800, Jeff Xu wrote:
> >>>>> Once per boot seems too little, it would be nice if we can list all processes.
> >>>>> I agree ratelimited might be too much.
> >>>>> There is a feature gap here for logging.
> >>>>>
> >>>>> Kees, what do you think ?
> >>>>
> >>>> I agree once per boot is kind of frustrating "I fixed the one warning,
> >>>> oh, now it's coming from a different process". But ratelimit is, in
> >>>> retrospect, still too often.
> >>>>
> >>>> Let's go with per boot -- this should be noisy "enough" to get the
> >>>> changes in API into the callers without being too much of a hassle.
> >>>>
> >>> Agreed.  Let's go with per boot.
> >>>
> >>> Hi Andrew, what is your preference ? I can send a patch  or you
> >>> directly fix it in mm-unstable ?
> >>
> >> Like this?
> >>
> > Yes. Thanks!
> >
>
> Sorry jumping into this discussion a bit late. Is it possible to provide
> a way to enable full logging as a debug option to tag more processes?
>
Codewise it is possible, maybe by adding a sysctl or CONFIG_, but I am not sure
the best practice to do this with the kernel?

Kees/Andrew, do you have suggestions ?

Thanks
Jeff


> thanks,
> -- Shuah
>