On Thu, Dec 8, 2022 at 8:27 AM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Wed, Dec 07, 2022 at 03:49:36PM +0000, jeffxu@xxxxxxxxxxxx wrote:
> > From: Jeff Xu <jeffxu@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > Co-developed-by: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> > Signed-off-by: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> > Reported-by: kernel test robot <lkp@xxxxxxxxx>
>
> Please rearrange these tags, and add a link to the lkp report:
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Link: ...url.to.lkp.lore.email...
> Co-developed-by: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> Signed-off-by: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> Signed-off-by: Jeff Xu <jeffxu@xxxxxxxxxx>
>
> > ---
> > 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@xxxxxxxxx/
> > + 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
