| Message ID | 20220411211015.3091615-10-bgardon@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: x86: Add a cap to disable NX hugepages on a VM | expand |
On Mon, Apr 11, 2022, Ben Gardon wrote: > Ensure that the userspace actor attempting to disable NX hugepages has > permission to reboot the system. Since disabling NX hugepages would > allow a guest to crash the system, it is similar to reboot permissions. > > This approach is the simplest permission gating, but passing a file > descriptor opened for write for the module parameter would also work > well and be more precise. > The latter approach was suggested by Sean Christopherson. State _why_ the latter approach wasn't chosen, vague hand waving about this being simpler doesn't help the reader understand how unbelievably painful it would be to actually get at the module param (I looked briefy, it'd be beyond ugly). We can still hand wave a bit, but there should at least be a hint as to why option A was chosen instead of option B. It'd also be helpful to call out what is lost by requiring CAP_SYS_BOOT, because again "precision" is rather vague. The important aspect of loss of precision is that a userspace process can't be given access to _just_ the NX module param to opt out of the workaround, it needs full reboot permissions. E.g. Ideally, KVM would require userspace to prove it has access to KVM's nx_huge_pages module param, e.g. so that userspace can opt out without needing full reboot permissions. But getting access to the module param file info is a mess since it's buried in layers of sysfs and module glue, and requiring CAP_SYS_BOOT is sufficient for all known use cases. > Suggested-by: Jim Mattson <jmattson@google.com> > Signed-off-by: Ben Gardon <bgardon@google.com> > --- > Documentation/virt/kvm/api.rst | 2 ++ > arch/x86/kvm/x86.c | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst > index 31fb002632bb..021452a9fa91 100644 > --- a/Documentation/virt/kvm/api.rst > +++ b/Documentation/virt/kvm/api.rst > @@ -7861,6 +7861,8 @@ should adjust CPUID leaf 0xA to reflect that the PMU is disabled. > :Capability KVM_CAP_PMU_CAPABILITY > :Architectures: x86 > :Type: vm > +:Returns 0 on success, -EPERM if the userspace process does not > + have CAP_SYS_BOOT > > This capability disables the NX huge pages mitigation for iTLB MULTIHIT. > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index de1d211f8aa3..8d3d6c48c5ec 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -6081,6 +6081,15 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, > mutex_unlock(&kvm->lock); > break; > case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES: > + /* > + * Since the risk of disabling NX hugepages is a guest crashing > + * the system, ensure the userspace process has permission to > + * reboot the system. > + */ > + if (!capable(CAP_SYS_BOOT)) { > + r = -EPERM; > + break; > + } This needs to go with the introduction of the cap. There is zero reason to create a window where the kernel is vulnerable.
diff --git a/Documentation/virt/kvm/api.rst b/Documentation/virt/kvm/api.rst index 31fb002632bb..021452a9fa91 100644 --- a/Documentation/virt/kvm/api.rst +++ b/Documentation/virt/kvm/api.rst @@ -7861,6 +7861,8 @@ should adjust CPUID leaf 0xA to reflect that the PMU is disabled. :Capability KVM_CAP_PMU_CAPABILITY :Architectures: x86 :Type: vm +:Returns 0 on success, -EPERM if the userspace process does not + have CAP_SYS_BOOT This capability disables the NX huge pages mitigation for iTLB MULTIHIT. diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index de1d211f8aa3..8d3d6c48c5ec 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -6081,6 +6081,15 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm, mutex_unlock(&kvm->lock); break; case KVM_CAP_VM_DISABLE_NX_HUGE_PAGES: + /* + * Since the risk of disabling NX hugepages is a guest crashing + * the system, ensure the userspace process has permission to + * reboot the system. + */ + if (!capable(CAP_SYS_BOOT)) { + r = -EPERM; + break; + } kvm->arch.disable_nx_huge_pages = true; kvm_update_nx_huge_pages(kvm); r = 0;
Ensure that the userspace actor attempting to disable NX hugepages has permission to reboot the system. Since disabling NX hugepages would allow a guest to crash the system, it is similar to reboot permissions. This approach is the simplest permission gating, but passing a file descriptor opened for write for the module parameter would also work well and be more precise. The latter approach was suggested by Sean Christopherson. Suggested-by: Jim Mattson <jmattson@google.com> Signed-off-by: Ben Gardon <bgardon@google.com> --- Documentation/virt/kvm/api.rst | 2 ++ arch/x86/kvm/x86.c | 9 +++++++++ 2 files changed, 11 insertions(+)