Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Tue, 12 Apr 2022 18:08:57 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Ben Gardon <bgardon@google.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        Paolo Bonzini <pbonzini@redhat.com>,
        Peter Xu <peterx@redhat.com>, Peter Shier <pshier@google.com>,
        David Dunn <daviddunn@google.com>,
        Junaid Shahid <junaids@google.com>,
        Jim Mattson <jmattson@google.com>,
        David Matlack <dmatlack@google.com>,
        Mingwei Zhang <mizhang@google.com>,
        Jing Zhang <jingzhangos@google.com>
Subject: Re: [PATCH v4 09/10] KVM: x86/MMU: Require reboot permission to
 disable NX hugepages
Message-ID: <YlXAOQOMu4Unryt6@google.com>
References: <20220411211015.3091615-1-bgardon@google.com>
 <20220411211015.3091615-10-bgardon@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220411211015.3091615-10-bgardon@google.com>
Precedence: bulk

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.