Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a1ed2308-b521-14c0-a118-19c1afffd1d6@grsecurity.net>
Date:   Wed, 5 Apr 2023 14:38:13 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.9.0
Subject: Re: [PATCH] KVM: x86/mmu: Refresh CR0.WP prior to checking for
 emulated permission faults
Content-Language: en-US, de-DE
To:     Sean Christopherson <seanjc@google.com>,
        Paolo Bonzini <pbonzini@redhat.com>
Cc:     kvm@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20230405002608.418442-1-seanjc@google.com>
From:   Mathias Krause <minipli@grsecurity.net>
In-Reply-To: <20230405002608.418442-1-seanjc@google.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 05.04.23 02:26, Sean Christopherson wrote:
> If CR0.WP may be guest-owned, i.e. TDP is enabled, refresh the MMU's
> snapshot of the guest's CR0.WP prior to checking for permission faults
> when emulating a guest memory access.  If the guest toggles only CR0.WP
> and triggers emulation of a supervisor write, e.g. when KVM is emulating
> UMIP, KVM may consume a stale CR0.WP, i.e. use stale protection bits
> metadata.

This reads a little awkward for a non-native speaker. Maybe something
like this:

    As CR0.WP may be guest-owned if TDP is enabled, refresh...
    [in between as is]
    ...consume a stale CR0.WP and use stale protection bits metadata.

> 
> Reported-by: Mathias Krause <minipli@grsecurity.net>
> Link: https://lkml.kernel.org/r/677169b4-051f-fcae-756b-9a3e1bb9f8fe%40grsecurity.net
> Fixes: fb509f76acc8 ("KVM: VMX: Make CR0.WP a guest owned bit")
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/mmu.h     | 26 +++++++++++++++++++++++++-
>  arch/x86/kvm/mmu/mmu.c | 15 +++++++++++++++
>  2 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index 89f532516a45..92d5a1924fc1 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -113,6 +113,8 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
>  bool kvm_can_do_async_pf(struct kvm_vcpu *vcpu);
>  int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>  				u64 fault_address, char *insn, int insn_len);
> +void __kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
> +					struct kvm_mmu *mmu);
>  
>  int kvm_mmu_load(struct kvm_vcpu *vcpu);
>  void kvm_mmu_unload(struct kvm_vcpu *vcpu);
> @@ -153,6 +155,24 @@ static inline void kvm_mmu_load_pgd(struct kvm_vcpu *vcpu)
>  					  vcpu->arch.mmu->root_role.level);
>  }
>  
> +static inline void kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
> +						    struct kvm_mmu *mmu)
> +{
> +	/*
> +	 * When EPT is enabled, KVM may passthrough CR0.WP to the guest, i.e.
> +	 * @mmu's snapshot of CR0.WP and thus all related paging metadata may
> +	 * be stale.  Refresh CR0.WP and the metadata on-demand when checking
> +	 * for permission faults.  Exempt nested MMUs, i.e. MMUs for shadowing
> +	 * nEPT and nNPT, as CR0.WP is ignored in both cases.  Note, KVM does
> +	 * need to refresh nested_mmu, a.k.a. the walker used to translate L2
> +	 * GVAs to GPAs, as that "MMU" needs to honor L2's CR0.WP.
> +	 */
> +	if (!tdp_enabled || mmu == &vcpu->arch.guest_mmu)
> +		return;
> +
> +	__kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
> +}
> +
>  /*
>   * Check if a given access (described through the I/D, W/R and U/S bits of a
>   * page fault error code pfec) causes a permission fault with the given PTE
> @@ -184,8 +204,12 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  	u64 implicit_access = access & PFERR_IMPLICIT_ACCESS;
>  	bool not_smap = ((rflags & X86_EFLAGS_AC) | implicit_access) == X86_EFLAGS_AC;
>  	int index = (pfec + (not_smap << PFERR_RSVD_BIT)) >> 1;
> -	bool fault = (mmu->permissions[index] >> pte_access) & 1;
>  	u32 errcode = PFERR_PRESENT_MASK;
> +	bool fault;
> +
> +	kvm_mmu_refresh_passthrough_bits(vcpu, mmu);
> +
> +	fault = (mmu->permissions[index] >> pte_access) & 1;
>  
>  	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
>  	if (unlikely(mmu->pkru_mask)) {
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 4c874d4ec68f..47269d50e98d 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -5186,6 +5186,21 @@ static union kvm_cpu_role kvm_calc_cpu_role(struct kvm_vcpu *vcpu,
>  	return role;
>  }
>  
> +void __kvm_mmu_refresh_passthrough_bits(struct kvm_vcpu *vcpu,
> +					struct kvm_mmu *mmu)
> +{
> +	const bool cr0_wp = kvm_is_cr0_bit_set(vcpu, X86_CR0_WP);
> +
> +	BUILD_BUG_ON((KVM_MMU_CR0_ROLE_BITS & KVM_POSSIBLE_CR0_GUEST_BITS) != X86_CR0_WP);

> +	BUILD_BUG_ON((KVM_MMU_CR4_ROLE_BITS & KVM_POSSIBLE_CR4_GUEST_BITS));

Just curious, this should assert that we don't run into similar issues
if we make more bits of CR4 guest owned?

> +
> +	if (is_cr0_wp(mmu) == cr0_wp)
> +		return;
> +
> +	mmu->cpu_role.base.cr0_wp = cr0_wp;
> +	reset_guest_paging_metadata(vcpu, mmu);
> +}
> +
>  static inline int kvm_mmu_get_tdp_level(struct kvm_vcpu *vcpu)
>  {
>  	/* tdp_root_level is architecture forced level, use it if nonzero */
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393

Tested-by: Mathias Krause <minipli@grsecurity.net>

Thanks again, Sean!