Historically, it has been shown that intercepting kernel faults with
userfaultfd (thereby forcing the kernel to wait for an arbitrary amount
of time) can be exploited, or at least can make some kinds of exploits
easier. So, in 37cd0575b8 "userfaultfd: add UFFD_USER_MODE_ONLY" we
changed things so, in order for kernel faults to be handled by
userfaultfd, either the process needs CAP_SYS_PTRACE, or this sysctl
must be configured so that any unprivileged user can do it.
In a typical implementation of a hypervisor with live migration (take
QEMU/KVM as one such example), we do indeed need to be able to handle
kernel faults. But, both options above are less than ideal:
- Toggling the sysctl increases attack surface by allowing any
unprivileged user to do it.
- Granting the live migration process CAP_SYS_PTRACE gives it this
ability, but *also* the ability to "observe and control the
execution of another process [...], and examine and change [its]
memory and registers" (from ptrace(2)). This isn't something we need
or want to be able to do, so granting this permission violates the
"principle of least privilege".
This is all a long winded way to say: we want a more fine-grained way to
grant access to userfaultfd, without granting other additional
permissions at the same time.
So, add CAP_USERFAULTFD, for this specific case.
Setup a helper which accepts either CAP_USERFAULTFD, or for backward
compatibility reasons (existing userspaces may depend on the old way of
doing things), CAP_SYS_PTRACE.
One special case is UFFD_FEATURE_EVENT_FORK: this is left requiring only
CAP_SYS_PTRACE, since it is specifically about manipulating the memory
of another (child) process, it sems like a better fit the way it is. To
my knowledge, this isn't a feature required by typical live migration
implementations, so this doesn't obviate the above.
Signed-off-by: Axel Rasmussen <[email protected]>
---
fs/userfaultfd.c | 6 +++---
include/linux/capability.h | 5 +++++
include/uapi/linux/capability.h | 7 ++++++-
security/selinux/include/classmap.h | 4 ++--
4 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index e26b10132d47..1ec0d9b49a70 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -411,7 +411,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
ctx->flags & UFFD_USER_MODE_ONLY) {
printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
"sysctl knob to 1 if kernel faults must be handled "
- "without obtaining CAP_SYS_PTRACE capability\n");
+ "without obtaining CAP_USERFAULTFD capability\n");
goto out;
}
@@ -2068,10 +2068,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
if (!sysctl_unprivileged_userfaultfd &&
(flags & UFFD_USER_MODE_ONLY) == 0 &&
- !capable(CAP_SYS_PTRACE)) {
+ !userfaultfd_capable()) {
printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
"sysctl knob to 1 if kernel faults must be handled "
- "without obtaining CAP_SYS_PTRACE capability\n");
+ "without obtaining CAP_USERFAULTFD capability\n");
return -EPERM;
}
diff --git a/include/linux/capability.h b/include/linux/capability.h
index 65efb74c3585..f1e7b3506432 100644
--- a/include/linux/capability.h
+++ b/include/linux/capability.h
@@ -270,6 +270,11 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
ns_capable(ns, CAP_SYS_ADMIN);
}
+static inline bool userfaultfd_capable(void)
+{
+ return capable(CAP_USERFAULTFD) || capable(CAP_SYS_PTRACE);
+}
+
/* audit system wants to get cap info from files as well */
int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
const struct dentry *dentry,
diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
index 463d1ba2232a..83a5d8601508 100644
--- a/include/uapi/linux/capability.h
+++ b/include/uapi/linux/capability.h
@@ -231,6 +231,7 @@ struct vfs_ns_cap_data {
#define CAP_SYS_CHROOT 18
/* Allow ptrace() of any process */
+/* Allow everything under CAP_USERFAULTFD for backward compatibility */
#define CAP_SYS_PTRACE 19
@@ -417,7 +418,11 @@ struct vfs_ns_cap_data {
#define CAP_CHECKPOINT_RESTORE 40
-#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
+/* Allow intercepting kernel faults with userfaultfd */
+
+#define CAP_USERFAULTFD 41
+
+#define CAP_LAST_CAP CAP_USERFAULTFD
#define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
index 35aac62a662e..98e37b220159 100644
--- a/security/selinux/include/classmap.h
+++ b/security/selinux/include/classmap.h
@@ -28,9 +28,9 @@
#define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
"wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
- "checkpoint_restore"
+ "checkpoint_restore", "userfaultfd"
-#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
+#if CAP_LAST_CAP > CAP_USERFAULTFD
#error New capability defined, please update COMMON_CAP2_PERMS.
#endif
--
2.35.1.574.g5d30c73bfb-goog
On 2/24/2022 10:19 AM, Axel Rasmussen wrote:
> Historically, it has been shown that intercepting kernel faults with
> userfaultfd (thereby forcing the kernel to wait for an arbitrary amount
> of time) can be exploited, or at least can make some kinds of exploits
> easier. So, in 37cd0575b8 "userfaultfd: add UFFD_USER_MODE_ONLY" we
> changed things so, in order for kernel faults to be handled by
> userfaultfd, either the process needs CAP_SYS_PTRACE, or this sysctl
> must be configured so that any unprivileged user can do it.
>
> In a typical implementation of a hypervisor with live migration (take
> QEMU/KVM as one such example), we do indeed need to be able to handle
> kernel faults. But, both options above are less than ideal:
>
> - Toggling the sysctl increases attack surface by allowing any
> unprivileged user to do it.
>
> - Granting the live migration process CAP_SYS_PTRACE gives it this
> ability, but *also* the ability to "observe and control the
> execution of another process [...], and examine and change [its]
> memory and registers" (from ptrace(2)). This isn't something we need
> or want to be able to do, so granting this permission violates the
> "principle of least privilege".
>
> This is all a long winded way to say: we want a more fine-grained way to
> grant access to userfaultfd, without granting other additional
> permissions at the same time.
>
> So, add CAP_USERFAULTFD, for this specific case.
TL;DR - No. We don't add new capabilities for a single use.
You have a program that is already using a reasonably restrictive
capability (compared to CAP_SYS_ADMIN, for example) and which I
assume you have implemented appropriately for the level of privilege
used. If you can demonstrate that this CAP_USERFAULTD has applicability
beyond your specific implementation (and the name would imply otherwise)
it could be worth considering, but as it is, no.
>
> Setup a helper which accepts either CAP_USERFAULTFD, or for backward
> compatibility reasons (existing userspaces may depend on the old way of
> doing things), CAP_SYS_PTRACE.
>
> One special case is UFFD_FEATURE_EVENT_FORK: this is left requiring only
> CAP_SYS_PTRACE, since it is specifically about manipulating the memory
> of another (child) process, it sems like a better fit the way it is. To
> my knowledge, this isn't a feature required by typical live migration
> implementations, so this doesn't obviate the above.
>
> Signed-off-by: Axel Rasmussen <[email protected]>
> ---
> fs/userfaultfd.c | 6 +++---
> include/linux/capability.h | 5 +++++
> include/uapi/linux/capability.h | 7 ++++++-
> security/selinux/include/classmap.h | 4 ++--
> 4 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index e26b10132d47..1ec0d9b49a70 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -411,7 +411,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> ctx->flags & UFFD_USER_MODE_ONLY) {
> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> "sysctl knob to 1 if kernel faults must be handled "
> - "without obtaining CAP_SYS_PTRACE capability\n");
> + "without obtaining CAP_USERFAULTFD capability\n");
> goto out;
> }
>
> @@ -2068,10 +2068,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
>
> if (!sysctl_unprivileged_userfaultfd &&
> (flags & UFFD_USER_MODE_ONLY) == 0 &&
> - !capable(CAP_SYS_PTRACE)) {
> + !userfaultfd_capable()) {
> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> "sysctl knob to 1 if kernel faults must be handled "
> - "without obtaining CAP_SYS_PTRACE capability\n");
> + "without obtaining CAP_USERFAULTFD capability\n");
> return -EPERM;
> }
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 65efb74c3585..f1e7b3506432 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -270,6 +270,11 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
> ns_capable(ns, CAP_SYS_ADMIN);
> }
>
> +static inline bool userfaultfd_capable(void)
> +{
> + return capable(CAP_USERFAULTFD) || capable(CAP_SYS_PTRACE);
> +}
> +
> /* audit system wants to get cap info from files as well */
> int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> const struct dentry *dentry,
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 463d1ba2232a..83a5d8601508 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -231,6 +231,7 @@ struct vfs_ns_cap_data {
> #define CAP_SYS_CHROOT 18
>
> /* Allow ptrace() of any process */
> +/* Allow everything under CAP_USERFAULTFD for backward compatibility */
>
> #define CAP_SYS_PTRACE 19
>
> @@ -417,7 +418,11 @@ struct vfs_ns_cap_data {
>
> #define CAP_CHECKPOINT_RESTORE 40
>
> -#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
> +/* Allow intercepting kernel faults with userfaultfd */
> +
> +#define CAP_USERFAULTFD 41
> +
> +#define CAP_LAST_CAP CAP_USERFAULTFD
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 35aac62a662e..98e37b220159 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -28,9 +28,9 @@
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
> - "checkpoint_restore"
> + "checkpoint_restore", "userfaultfd"
>
> -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> +#if CAP_LAST_CAP > CAP_USERFAULTFD
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>
On 2/24/2022 10:19 AM, Axel Rasmussen wrote:
> Historically, it has been shown that intercepting kernel faults with
> userfaultfd (thereby forcing the kernel to wait for an arbitrary amount
> of time) can be exploited, or at least can make some kinds of exploits
> easier. So, in 37cd0575b8 "userfaultfd: add UFFD_USER_MODE_ONLY" we
> changed things so, in order for kernel faults to be handled by
> userfaultfd, either the process needs CAP_SYS_PTRACE, or this sysctl
> must be configured so that any unprivileged user can do it.
>
> In a typical implementation of a hypervisor with live migration (take
> QEMU/KVM as one such example), we do indeed need to be able to handle
> kernel faults. But, both options above are less than ideal:
>
> - Toggling the sysctl increases attack surface by allowing any
> unprivileged user to do it.
>
> - Granting the live migration process CAP_SYS_PTRACE gives it this
> ability, but *also* the ability to "observe and control the
> execution of another process [...], and examine and change [its]
> memory and registers" (from ptrace(2)). This isn't something we need
> or want to be able to do, so granting this permission violates the
> "principle of least privilege".
>
> This is all a long winded way to say: we want a more fine-grained way to
> grant access to userfaultfd, without granting other additional
> permissions at the same time.
>
> So, add CAP_USERFAULTFD, for this specific case.
TL;DR - No. We don't add new capabilities for a single use.
You have a program that is already using a reasonably restrictive
capability (compared to CAP_SYS_ADMIN, for example) and which I
assume you have implemented appropriately for the level of privilege
used. If you can demonstrate that this CAP_USERFAULTD has applicability
beyond your specific implementation (and the name would imply otherwise)
it could be worth considering, but as it is, no.
>
> Setup a helper which accepts either CAP_USERFAULTFD, or for backward
> compatibility reasons (existing userspaces may depend on the old way of
> doing things), CAP_SYS_PTRACE.
>
> One special case is UFFD_FEATURE_EVENT_FORK: this is left requiring only
> CAP_SYS_PTRACE, since it is specifically about manipulating the memory
> of another (child) process, it sems like a better fit the way it is. To
> my knowledge, this isn't a feature required by typical live migration
> implementations, so this doesn't obviate the above.
>
> Signed-off-by: Axel Rasmussen <[email protected]>
> ---
> fs/userfaultfd.c | 6 +++---
> include/linux/capability.h | 5 +++++
> include/uapi/linux/capability.h | 7 ++++++-
> security/selinux/include/classmap.h | 4 ++--
> 4 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> index e26b10132d47..1ec0d9b49a70 100644
> --- a/fs/userfaultfd.c
> +++ b/fs/userfaultfd.c
> @@ -411,7 +411,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> ctx->flags & UFFD_USER_MODE_ONLY) {
> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> "sysctl knob to 1 if kernel faults must be handled "
> - "without obtaining CAP_SYS_PTRACE capability\n");
> + "without obtaining CAP_USERFAULTFD capability\n");
> goto out;
> }
>
> @@ -2068,10 +2068,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
>
> if (!sysctl_unprivileged_userfaultfd &&
> (flags & UFFD_USER_MODE_ONLY) == 0 &&
> - !capable(CAP_SYS_PTRACE)) {
> + !userfaultfd_capable()) {
> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> "sysctl knob to 1 if kernel faults must be handled "
> - "without obtaining CAP_SYS_PTRACE capability\n");
> + "without obtaining CAP_USERFAULTFD capability\n");
> return -EPERM;
> }
>
> diff --git a/include/linux/capability.h b/include/linux/capability.h
> index 65efb74c3585..f1e7b3506432 100644
> --- a/include/linux/capability.h
> +++ b/include/linux/capability.h
> @@ -270,6 +270,11 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
> ns_capable(ns, CAP_SYS_ADMIN);
> }
>
> +static inline bool userfaultfd_capable(void)
> +{
> + return capable(CAP_USERFAULTFD) || capable(CAP_SYS_PTRACE);
> +}
> +
> /* audit system wants to get cap info from files as well */
> int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> const struct dentry *dentry,
> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> index 463d1ba2232a..83a5d8601508 100644
> --- a/include/uapi/linux/capability.h
> +++ b/include/uapi/linux/capability.h
> @@ -231,6 +231,7 @@ struct vfs_ns_cap_data {
> #define CAP_SYS_CHROOT 18
>
> /* Allow ptrace() of any process */
> +/* Allow everything under CAP_USERFAULTFD for backward compatibility */
>
> #define CAP_SYS_PTRACE 19
>
> @@ -417,7 +418,11 @@ struct vfs_ns_cap_data {
>
> #define CAP_CHECKPOINT_RESTORE 40
>
> -#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
> +/* Allow intercepting kernel faults with userfaultfd */
> +
> +#define CAP_USERFAULTFD 41
> +
> +#define CAP_LAST_CAP CAP_USERFAULTFD
>
> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>
> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> index 35aac62a662e..98e37b220159 100644
> --- a/security/selinux/include/classmap.h
> +++ b/security/selinux/include/classmap.h
> @@ -28,9 +28,9 @@
>
> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
> - "checkpoint_restore"
> + "checkpoint_restore", "userfaultfd"
>
> -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> +#if CAP_LAST_CAP > CAP_USERFAULTFD
> #error New capability defined, please update COMMON_CAP2_PERMS.
> #endif
>
On Thu, Feb 24, 2022 at 11:13 AM Casey Schaufler <[email protected]> wrote:
>
> On 2/24/2022 10:19 AM, Axel Rasmussen wrote:
> > Historically, it has been shown that intercepting kernel faults with
> > userfaultfd (thereby forcing the kernel to wait for an arbitrary amount
> > of time) can be exploited, or at least can make some kinds of exploits
> > easier. So, in 37cd0575b8 "userfaultfd: add UFFD_USER_MODE_ONLY" we
> > changed things so, in order for kernel faults to be handled by
> > userfaultfd, either the process needs CAP_SYS_PTRACE, or this sysctl
> > must be configured so that any unprivileged user can do it.
> >
> > In a typical implementation of a hypervisor with live migration (take
> > QEMU/KVM as one such example), we do indeed need to be able to handle
> > kernel faults. But, both options above are less than ideal:
> >
> > - Toggling the sysctl increases attack surface by allowing any
> > unprivileged user to do it.
> >
> > - Granting the live migration process CAP_SYS_PTRACE gives it this
> > ability, but *also* the ability to "observe and control the
> > execution of another process [...], and examine and change [its]
> > memory and registers" (from ptrace(2)). This isn't something we need
> > or want to be able to do, so granting this permission violates the
> > "principle of least privilege".
> >
> > This is all a long winded way to say: we want a more fine-grained way to
> > grant access to userfaultfd, without granting other additional
> > permissions at the same time.
> >
> > So, add CAP_USERFAULTFD, for this specific case.
>
> TL;DR - No. We don't add new capabilities for a single use.
>
> You have a program that is already using a reasonably restrictive
> capability (compared to CAP_SYS_ADMIN, for example) and which I
> assume you have implemented appropriately for the level of privilege
> used. If you can demonstrate that this CAP_USERFAULTD has applicability
> beyond your specific implementation (and the name would imply otherwise)
> it could be worth considering, but as it is, no.
Thanks for taking the time to look at this Casey!
I'm not exactly clear, would you want more evidence of userspace use
cases besides just mine? Besides Google's VM implementation: Peter and
Andrea expressed interest in this to me a while back for use with
QEMU/KVM-based VMs [*], and I suspect Android folks would also use
this if it were merged (+Suren and Lokesh to CC).
Or, do you just mean that userfaultfd is too narrow a feature to
warrant a capability? When writing this I was encouraged by CAP_BPF
and CAP_CHECKPOINT_RESTORE, they seemed to me to be somewhat similar
in terms of scope (specific to a single kernel feature).
[*] Although, we talked about fine grained permissions in general, not
necessarily a capability based approach. An alternative we talked
about was to add a userfaultfd device node like /dev/userfaultfd. The
idea being, access to it could be controlled using normal filesystem
permissions (chmod/chown), and userfaultfds created that way (as
opposed to the userfaultfd() syscall) would be able to intercept
kernel faults, regardless of CAP_SYS_PTRACE. My gut feeling was that
this was significantly more complicated than the patch I'm proposing
here.
>
> >
> > Setup a helper which accepts either CAP_USERFAULTFD, or for backward
> > compatibility reasons (existing userspaces may depend on the old way of
> > doing things), CAP_SYS_PTRACE.
> >
> > One special case is UFFD_FEATURE_EVENT_FORK: this is left requiring only
> > CAP_SYS_PTRACE, since it is specifically about manipulating the memory
> > of another (child) process, it sems like a better fit the way it is. To
> > my knowledge, this isn't a feature required by typical live migration
> > implementations, so this doesn't obviate the above.
> >
> > Signed-off-by: Axel Rasmussen <[email protected]>
> > ---
> > fs/userfaultfd.c | 6 +++---
> > include/linux/capability.h | 5 +++++
> > include/uapi/linux/capability.h | 7 ++++++-
> > security/selinux/include/classmap.h | 4 ++--
> > 4 files changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
> > index e26b10132d47..1ec0d9b49a70 100644
> > --- a/fs/userfaultfd.c
> > +++ b/fs/userfaultfd.c
> > @@ -411,7 +411,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
> > ctx->flags & UFFD_USER_MODE_ONLY) {
> > printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> > "sysctl knob to 1 if kernel faults must be handled "
> > - "without obtaining CAP_SYS_PTRACE capability\n");
> > + "without obtaining CAP_USERFAULTFD capability\n");
> > goto out;
> > }
> >
> > @@ -2068,10 +2068,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
> >
> > if (!sysctl_unprivileged_userfaultfd &&
> > (flags & UFFD_USER_MODE_ONLY) == 0 &&
> > - !capable(CAP_SYS_PTRACE)) {
> > + !userfaultfd_capable()) {
> > printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
> > "sysctl knob to 1 if kernel faults must be handled "
> > - "without obtaining CAP_SYS_PTRACE capability\n");
> > + "without obtaining CAP_USERFAULTFD capability\n");
> > return -EPERM;
> > }
> >
> > diff --git a/include/linux/capability.h b/include/linux/capability.h
> > index 65efb74c3585..f1e7b3506432 100644
> > --- a/include/linux/capability.h
> > +++ b/include/linux/capability.h
> > @@ -270,6 +270,11 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
> > ns_capable(ns, CAP_SYS_ADMIN);
> > }
> >
> > +static inline bool userfaultfd_capable(void)
> > +{
> > + return capable(CAP_USERFAULTFD) || capable(CAP_SYS_PTRACE);
> > +}
> > +
> > /* audit system wants to get cap info from files as well */
> > int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
> > const struct dentry *dentry,
> > diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
> > index 463d1ba2232a..83a5d8601508 100644
> > --- a/include/uapi/linux/capability.h
> > +++ b/include/uapi/linux/capability.h
> > @@ -231,6 +231,7 @@ struct vfs_ns_cap_data {
> > #define CAP_SYS_CHROOT 18
> >
> > /* Allow ptrace() of any process */
> > +/* Allow everything under CAP_USERFAULTFD for backward compatibility */
> >
> > #define CAP_SYS_PTRACE 19
> >
> > @@ -417,7 +418,11 @@ struct vfs_ns_cap_data {
> >
> > #define CAP_CHECKPOINT_RESTORE 40
> >
> > -#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
> > +/* Allow intercepting kernel faults with userfaultfd */
> > +
> > +#define CAP_USERFAULTFD 41
> > +
> > +#define CAP_LAST_CAP CAP_USERFAULTFD
> >
> > #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
> >
> > diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
> > index 35aac62a662e..98e37b220159 100644
> > --- a/security/selinux/include/classmap.h
> > +++ b/security/selinux/include/classmap.h
> > @@ -28,9 +28,9 @@
> >
> > #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
> > "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
> > - "checkpoint_restore"
> > + "checkpoint_restore", "userfaultfd"
> >
> > -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
> > +#if CAP_LAST_CAP > CAP_USERFAULTFD
> > #error New capability defined, please update COMMON_CAP2_PERMS.
> > #endif
> >
On Thu, Feb 24, 2022 at 04:39:44PM -0800, Casey Schaufler wrote:
> What I'd want to see is multiple users where the use of CAP_USERFAULTD
> is independent of the use of CAP_SYS_PTRACE. That is, the programs would
> never require CAP_SYS_PTRACE. There should be demonstrated real value.
> Not just that a compromised program with CAP_SYS_PTRACE can do bad things,
> but that the programs with CAP_USERFAULTDD are somehow susceptible to
> being exploited to doing those bad things. Hypothetical users are just
> that, and often don't materialize.
I kind of have the same question indeed..
The use case we're talking about is VM migration, and the in-question
subject is literally the migration process or thread. Isn't that a trusted
piece of software already?
Then the question is why the extra capability (in CAP_PTRACE but not in
CAP_UFFD) could bring much risk to the system. Axel, did I miss something
important?
Thanks,
--
Peter Xu
On 2/24/2022 2:07 PM, Axel Rasmussen wrote:
> On Thu, Feb 24, 2022 at 11:13 AM Casey Schaufler <[email protected]> wrote:
>> On 2/24/2022 10:19 AM, Axel Rasmussen wrote:
>>> Historically, it has been shown that intercepting kernel faults with
>>> userfaultfd (thereby forcing the kernel to wait for an arbitrary amount
>>> of time) can be exploited, or at least can make some kinds of exploits
>>> easier. So, in 37cd0575b8 "userfaultfd: add UFFD_USER_MODE_ONLY" we
>>> changed things so, in order for kernel faults to be handled by
>>> userfaultfd, either the process needs CAP_SYS_PTRACE, or this sysctl
>>> must be configured so that any unprivileged user can do it.
>>>
>>> In a typical implementation of a hypervisor with live migration (take
>>> QEMU/KVM as one such example), we do indeed need to be able to handle
>>> kernel faults. But, both options above are less than ideal:
>>>
>>> - Toggling the sysctl increases attack surface by allowing any
>>> unprivileged user to do it.
>>>
>>> - Granting the live migration process CAP_SYS_PTRACE gives it this
>>> ability, but *also* the ability to "observe and control the
>>> execution of another process [...], and examine and change [its]
>>> memory and registers" (from ptrace(2)). This isn't something we need
>>> or want to be able to do, so granting this permission violates the
>>> "principle of least privilege".
>>>
>>> This is all a long winded way to say: we want a more fine-grained way to
>>> grant access to userfaultfd, without granting other additional
>>> permissions at the same time.
>>>
>>> So, add CAP_USERFAULTFD, for this specific case.
>> TL;DR - No. We don't add new capabilities for a single use.
>>
>> You have a program that is already using a reasonably restrictive
>> capability (compared to CAP_SYS_ADMIN, for example) and which I
>> assume you have implemented appropriately for the level of privilege
>> used. If you can demonstrate that this CAP_USERFAULTD has applicability
>> beyond your specific implementation (and the name would imply otherwise)
>> it could be worth considering, but as it is, no.
> Thanks for taking the time to look at this Casey!
>
> I'm not exactly clear, would you want more evidence of userspace use
> cases besides just mine? Besides Google's VM implementation: Peter and
> Andrea expressed interest in this to me a while back for use with
> QEMU/KVM-based VMs [*], and I suspect Android folks would also use
> this if it were merged (+Suren and Lokesh to CC).
What I'd want to see is multiple users where the use of CAP_USERFAULTD
is independent of the use of CAP_SYS_PTRACE. That is, the programs would
never require CAP_SYS_PTRACE. There should be demonstrated real value.
Not just that a compromised program with CAP_SYS_PTRACE can do bad things,
but that the programs with CAP_USERFAULTDD are somehow susceptible to
being exploited to doing those bad things. Hypothetical users are just
that, and often don't materialize.
> Or, do you just mean that userfaultfd is too narrow a feature to
> warrant a capability?
Consider that if we implemented every capability at this level of
granularity we'd have 400 to 900 of them, and they'd have to change
regularly as underlying implementations are revised.
> When writing this I was encouraged by CAP_BPF
> and CAP_CHECKPOINT_RESTORE, they seemed to me to be somewhat similar
> in terms of scope (specific to a single kernel feature).
CAP_BPF controls use of an entire access control sub-system.
I'm not going to argue about CAP_CHECKPOINT_RESTORE.
> [*] Although, we talked about fine grained permissions in general, not
> necessarily a capability based approach.
Capabilities don't lend themselves well to really fine granularity.
The real intention of capabilities is to separate the privilege mechanism
from the discretionary access control (i.e UID) mechanism.
Most people are still using root as the basis of privilege because they
find the existing capability granularity too hard to work with.
Additionally, as you're with Google I expect you're going to be
using SELinux with this, and that's going to give you all the
granularity you could possibly wish for.
> An alternative we talked
> about was to add a userfaultfd device node like /dev/userfaultfd. The
> idea being, access to it could be controlled using normal filesystem
> permissions (chmod/chown), and userfaultfds created that way (as
> opposed to the userfaultfd() syscall) would be able to intercept
> kernel faults, regardless of CAP_SYS_PTRACE. My gut feeling was that
> this was significantly more complicated than the patch I'm proposing
> here.
When I implemented Smack I was faced with the same kind of choice.
I found that filesystem based interfaces (e.g. /sys/fs/smackfs/load2)
were much easier for scripts and existing programs to use than were
syscalls. Yes, there's more kernel code involved, but you also have
more policy flexibility.
>
>>> Setup a helper which accepts either CAP_USERFAULTFD, or for backward
>>> compatibility reasons (existing userspaces may depend on the old way of
>>> doing things), CAP_SYS_PTRACE.
>>>
>>> One special case is UFFD_FEATURE_EVENT_FORK: this is left requiring only
>>> CAP_SYS_PTRACE, since it is specifically about manipulating the memory
>>> of another (child) process, it sems like a better fit the way it is. To
>>> my knowledge, this isn't a feature required by typical live migration
>>> implementations, so this doesn't obviate the above.
>>>
>>> Signed-off-by: Axel Rasmussen <[email protected]>
>>> ---
>>> fs/userfaultfd.c | 6 +++---
>>> include/linux/capability.h | 5 +++++
>>> include/uapi/linux/capability.h | 7 ++++++-
>>> security/selinux/include/classmap.h | 4 ++--
>>> 4 files changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
>>> index e26b10132d47..1ec0d9b49a70 100644
>>> --- a/fs/userfaultfd.c
>>> +++ b/fs/userfaultfd.c
>>> @@ -411,7 +411,7 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
>>> ctx->flags & UFFD_USER_MODE_ONLY) {
>>> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
>>> "sysctl knob to 1 if kernel faults must be handled "
>>> - "without obtaining CAP_SYS_PTRACE capability\n");
>>> + "without obtaining CAP_USERFAULTFD capability\n");
>>> goto out;
>>> }
>>>
>>> @@ -2068,10 +2068,10 @@ SYSCALL_DEFINE1(userfaultfd, int, flags)
>>>
>>> if (!sysctl_unprivileged_userfaultfd &&
>>> (flags & UFFD_USER_MODE_ONLY) == 0 &&
>>> - !capable(CAP_SYS_PTRACE)) {
>>> + !userfaultfd_capable()) {
>>> printk_once(KERN_WARNING "uffd: Set unprivileged_userfaultfd "
>>> "sysctl knob to 1 if kernel faults must be handled "
>>> - "without obtaining CAP_SYS_PTRACE capability\n");
>>> + "without obtaining CAP_USERFAULTFD capability\n");
>>> return -EPERM;
>>> }
>>>
>>> diff --git a/include/linux/capability.h b/include/linux/capability.h
>>> index 65efb74c3585..f1e7b3506432 100644
>>> --- a/include/linux/capability.h
>>> +++ b/include/linux/capability.h
>>> @@ -270,6 +270,11 @@ static inline bool checkpoint_restore_ns_capable(struct user_namespace *ns)
>>> ns_capable(ns, CAP_SYS_ADMIN);
>>> }
>>>
>>> +static inline bool userfaultfd_capable(void)
>>> +{
>>> + return capable(CAP_USERFAULTFD) || capable(CAP_SYS_PTRACE);
>>> +}
>>> +
>>> /* audit system wants to get cap info from files as well */
>>> int get_vfs_caps_from_disk(struct user_namespace *mnt_userns,
>>> const struct dentry *dentry,
>>> diff --git a/include/uapi/linux/capability.h b/include/uapi/linux/capability.h
>>> index 463d1ba2232a..83a5d8601508 100644
>>> --- a/include/uapi/linux/capability.h
>>> +++ b/include/uapi/linux/capability.h
>>> @@ -231,6 +231,7 @@ struct vfs_ns_cap_data {
>>> #define CAP_SYS_CHROOT 18
>>>
>>> /* Allow ptrace() of any process */
>>> +/* Allow everything under CAP_USERFAULTFD for backward compatibility */
>>>
>>> #define CAP_SYS_PTRACE 19
>>>
>>> @@ -417,7 +418,11 @@ struct vfs_ns_cap_data {
>>>
>>> #define CAP_CHECKPOINT_RESTORE 40
>>>
>>> -#define CAP_LAST_CAP CAP_CHECKPOINT_RESTORE
>>> +/* Allow intercepting kernel faults with userfaultfd */
>>> +
>>> +#define CAP_USERFAULTFD 41
>>> +
>>> +#define CAP_LAST_CAP CAP_USERFAULTFD
>>>
>>> #define cap_valid(x) ((x) >= 0 && (x) <= CAP_LAST_CAP)
>>>
>>> diff --git a/security/selinux/include/classmap.h b/security/selinux/include/classmap.h
>>> index 35aac62a662e..98e37b220159 100644
>>> --- a/security/selinux/include/classmap.h
>>> +++ b/security/selinux/include/classmap.h
>>> @@ -28,9 +28,9 @@
>>>
>>> #define COMMON_CAP2_PERMS "mac_override", "mac_admin", "syslog", \
>>> "wake_alarm", "block_suspend", "audit_read", "perfmon", "bpf", \
>>> - "checkpoint_restore"
>>> + "checkpoint_restore", "userfaultfd"
>>>
>>> -#if CAP_LAST_CAP > CAP_CHECKPOINT_RESTORE
>>> +#if CAP_LAST_CAP > CAP_USERFAULTFD
>>> #error New capability defined, please update COMMON_CAP2_PERMS.
>>> #endif
>>>
Thanks for the detailed explanation Casey!
On Thu, Feb 24, 2022 at 6:58 PM Peter Xu <[email protected]> wrote:
>
> On Thu, Feb 24, 2022 at 04:39:44PM -0800, Casey Schaufler wrote:
> > What I'd want to see is multiple users where the use of CAP_USERFAULTD
> > is independent of the use of CAP_SYS_PTRACE. That is, the programs would
> > never require CAP_SYS_PTRACE. There should be demonstrated real value.
> > Not just that a compromised program with CAP_SYS_PTRACE can do bad things,
> > but that the programs with CAP_USERFAULTDD are somehow susceptible to
> > being exploited to doing those bad things. Hypothetical users are just
> > that, and often don't materialize.
>
> I kind of have the same question indeed..
>
> The use case we're talking about is VM migration, and the in-question
> subject is literally the migration process or thread. Isn't that a trusted
> piece of software already?
>
> Then the question is why the extra capability (in CAP_PTRACE but not in
> CAP_UFFD) could bring much risk to the system. Axel, did I miss something
> important?
For me it's just a matter of giving the live migration process as
little power as I can while still letting it do its job.
Live migration is somewhat trusted, and certainly if it can mess with
the memory contents of its own VM, that's no concern. But there are
other processes or threads running alongside it to manage other parts
of the VM, like attached virtual disks. Also it's probably running on
a server which also hosts other VMs, and I think it's a common design
to have them all run as the same user (although, they may be running
in other containers).
So, it seems unfortunate to me that the live migration process can
just ptrace() any of these other things running alongside it.
Casey is right that we can restrict what it can do with e.g. SELinux
or seccomp-ebpf or whatever else. But it seems to me a more fragile
design to give the permissions and then restrict them, vs. just never
giving those permissions in the first place.
In any case though, it sounds like folks are more amenable to the
device node approach. Honestly, I got that impression from Andrea as
well when we first talked about this some months ago. So, I can pursue
that approach instead.
>
> Thanks,
>
> --
> Peter Xu
>
On 2/25/2022 10:17 AM, Axel Rasmussen wrote:
> Thanks for the detailed explanation Casey!
>
> On Thu, Feb 24, 2022 at 6:58 PM Peter Xu <[email protected]> wrote:
>> On Thu, Feb 24, 2022 at 04:39:44PM -0800, Casey Schaufler wrote:
>>> What I'd want to see is multiple users where the use of CAP_USERFAULTD
>>> is independent of the use of CAP_SYS_PTRACE. That is, the programs would
>>> never require CAP_SYS_PTRACE. There should be demonstrated real value.
>>> Not just that a compromised program with CAP_SYS_PTRACE can do bad things,
>>> but that the programs with CAP_USERFAULTDD are somehow susceptible to
>>> being exploited to doing those bad things. Hypothetical users are just
>>> that, and often don't materialize.
>> I kind of have the same question indeed..
>>
>> The use case we're talking about is VM migration, and the in-question
>> subject is literally the migration process or thread. Isn't that a trusted
>> piece of software already?
>>
>> Then the question is why the extra capability (in CAP_PTRACE but not in
>> CAP_UFFD) could bring much risk to the system. Axel, did I miss something
>> important?
> For me it's just a matter of giving the live migration process as
> little power as I can while still letting it do its job.
That's understood. But live migration is a bit of a special case,
and as mentioned above, is trusted to do an oodle of important stuff
correctly.
> Live migration is somewhat trusted, and certainly if it can mess with
> the memory contents of its own VM, that's no concern. But there are
> other processes or threads running alongside it to manage other parts
> of the VM, like attached virtual disks. Also it's probably running on
> a server which also hosts other VMs, and I think it's a common design
> to have them all run as the same user (although, they may be running
> in other containers).
That seems unwise. I am often surprised how we're eager to add
new security features to make up for the unwillingness of people
to use the existing ones.
> So, it seems unfortunate to me that the live migration process can
> just ptrace() any of these other things running alongside it.
I get that. On the other hand, most of the systems you'll run
live migration on are going to have full-up root processes,
possibly even userfaultd (in spite of instructions not to do so).
> Casey is right that we can restrict what it can do with e.g. SELinux
> or seccomp-ebpf or whatever else. But it seems to me a more fragile
> design to give the permissions and then restrict them, vs. just never
> giving those permissions in the first place.
If we lived in a universe with a root-less reality I'd agree.
> In any case though, it sounds like folks are more amenable to the
> device node approach. Honestly, I got that impression from Andrea as
> well when we first talked about this some months ago. So, I can pursue
> that approach instead.
I think that's more realistic.
>
>> Thanks,
>> --
>> Peter Xu
>>