On Fri, May 08, 2020 at 12:52:34PM -0400, Michael S. Tsirkin wrote:
> On Wed, Apr 22, 2020 at 05:26:32PM -0700, Daniel Colascione wrote:
> > This sysctl can be set to either zero or one. When zero (the default)
> > the system lets all users call userfaultfd with or without
> > UFFD_USER_MODE_ONLY, modulo other access controls. When
> > unprivileged_userfaultfd_user_mode_only is set to one, users without
> > CAP_SYS_PTRACE must pass UFFD_USER_MODE_ONLY to userfaultfd or the API
> > will fail with EPERM. This facility allows administrators to reduce
> > the likelihood that an attacker with access to userfaultfd can delay
> > faulting kernel code to widen timing windows for other exploits.
> >
> > Signed-off-by: Daniel Colascione <dancol@xxxxxxxxxx>
>
> The approach taken looks like a hard-coded security policy.
> For example, it won't be possible to set the sysctl knob
> in question on any sytem running kvm. So this is
> no good for any general purpose system.
>
> What's wrong with using a security policy for this instead?
In fact I see the original thread already mentions selinux,
so it's just a question of making this controllable by
selinux.
>
>
> > ---
> > Documentation/admin-guide/sysctl/vm.rst | 13 +++++++++++++
> > fs/userfaultfd.c | 11 ++++++++++-
> > include/linux/userfaultfd_k.h | 1 +
> > kernel/sysctl.c | 9 +++++++++
> > 4 files changed, 33 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/admin-guide/sysctl/vm.rst b/Documentation/admin-guide/sysctl/vm.rst
> > index 0329a4d3fa9e..4296b508ab74 100644
> > --- a/Documentation/admin-guide/sysctl/vm.rst
> > +++ b/Documentation/admin-guide/sysctl/vm.rst
> > @@ -850,6 +850,19 @@ privileged users (with SYS_CAP_PTRACE capability).
> >
> > The default value is 1.
> >
> > +unprivileged_userfaultfd_user_mode_only
> > +========================================
> > +
> > +This flag controls whether unprivileged users can use the userfaultfd
> > +system calls to handle page faults in kernel mode. If set to zero,
> > +userfaultfd works with or without UFFD_USER_MODE_ONLY, modulo
> > +unprivileged_userfaultfd above. If set to one, users without
> > +SYS_CAP_PTRACE must pass UFFD_USER_MODE_ONLY in order for userfaultfd
> > +to succeed. Prohibiting use of userfaultfd for handling faults from
> > +kernel mode may make certain vulnerabilities more difficult
> > +to exploit.
> > +
> > +The default value is 0.
> >
> > user_reserve_kbytes
> > ===================
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index 21378abe8f7b..85cc1ab74361 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -29,6 +29,7 @@
> > #include <linux/hugetlb.h>
> >
> > int sysctl_unprivileged_userfaultfd __read_mostly = 1;
> > +int sysctl_unprivileged_userfaultfd_user_mode_only __read_mostly = 0;
> >
> > static struct kmem_cache *userfaultfd_ctx_cachep __read_mostly;
> >
> > @@ -2009,8 +2010,16 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
> > static const int uffd_flags = UFFD_USER_MODE_ONLY;
> > struct userfaultfd_ctx *ctx;
> > int fd;
> > + bool need_cap_check = false;
> >
> > - if (!sysctl_unprivileged_userfaultfd && !capable(CAP_SYS_PTRACE))
> > + if (!sysctl_unprivileged_userfaultfd)
> > + need_cap_check = true;
> > +
> > + if (sysctl_unprivileged_userfaultfd_user_mode_only &&
> > + (flags & UFFD_USER_MODE_ONLY) == 0)
> > + need_cap_check = true;
> > +
> > + if (need_cap_check && !capable(CAP_SYS_PTRACE))
> > return -EPERM;
> >
> > BUG_ON(!current->mm);
> > diff --git a/include/linux/userfaultfd_k.h b/include/linux/userfaultfd_k.h
> > index a8e5f3ea9bb2..d81e30074bf5 100644
> > --- a/include/linux/userfaultfd_k.h
> > +++ b/include/linux/userfaultfd_k.h
> > @@ -31,6 +31,7 @@
> > #define UFFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS)
> >
> > extern int sysctl_unprivileged_userfaultfd;
> > +extern int sysctl_unprivileged_userfaultfd_user_mode_only;
> >
> > extern vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason);
> >
> > diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> > index 8a176d8727a3..9cbdf4483961 100644
> > --- a/kernel/sysctl.c
> > +++ b/kernel/sysctl.c
> > @@ -1719,6 +1719,15 @@ static struct ctl_table vm_table[] = {
> > .extra1 = SYSCTL_ZERO,
> > .extra2 = SYSCTL_ONE,
> > },
> > + {
> > + .procname = "unprivileged_userfaultfd_user_mode_only",
> > + .data = &sysctl_unprivileged_userfaultfd_user_mode_only,
> > + .maxlen = sizeof(sysctl_unprivileged_userfaultfd_user_mode_only),
> > + .mode = 0644,
> > + .proc_handler = proc_dointvec_minmax,
> > + .extra1 = SYSCTL_ZERO,
> > + .extra2 = SYSCTL_ONE,
> > + },
> > #endif
> > { }
> > };
> > --
> > 2.26.2.303.gf8c07b1a785-goog
> >
