diff mbox series

[07/16] KVM: x86: Move synthetic PFERR_* sanity checks to SVM's #NPF handler

Message ID 20240228024147.41573-8-seanjc@google.com
State New
Headers
Series KVM: x86/mmu: Page fault and MMIO cleanups |

Commit Message

Sean Christopherson Feb. 28, 2024, 2:41 a.m. UTC 
  Move the sanity check that hardware never sets bits that collide with KVM-
define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
i.e. make the sanity check #NPF specific.  The legacy #PF path already
WARNs if _any_ of bits 63:32 are set, and the error code that comes from
VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
EXIT_QUALIFICATION into error code flags).

Add a compile-time assert in the legacy #PF handler to make sure that KVM-
define flags are covered by its existing sanity check on the upper bits.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/mmu/mmu.c | 12 +++---------
 arch/x86/kvm/svm/svm.c |  9 +++++++++
 2 files changed, 12 insertions(+), 9 deletions(-)

Comments

Kai Huang Feb. 29, 2024, 10:19 p.m. UTC | #1 
On 28/02/2024 3:41 pm, Sean Christopherson wrote:
> Move the sanity check that hardware never sets bits that collide with KVM-
> define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
> i.e. make the sanity check #NPF specific.  The legacy #PF path already
> WARNs if _any_ of bits 63:32 are set, and the error code that comes from
> VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
> EXIT_QUALIFICATION into error code flags).
> 
> Add a compile-time assert in the legacy #PF handler to make sure that KVM-
> define flags are covered by its existing sanity check on the upper bits.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/mmu/mmu.c | 12 +++---------
>   arch/x86/kvm/svm/svm.c |  9 +++++++++
>   2 files changed, 12 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 5d892bd59c97..bd342ebd0809 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -4561,6 +4561,9 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>   	if (WARN_ON_ONCE(error_code >> 32))
>   		error_code = lower_32_bits(error_code);
>   
> +	/* Ensure the above sanity check also covers KVM-defined flags. */
> +	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
> +

Could you explain why adding this BUILD_BUG_ON() here, but not ...

>   	vcpu->arch.l1tf_flush_l1d = true;
>   	if (!flags) {
>   		trace_kvm_page_fault(vcpu, fault_address, error_code);
> @@ -5845,15 +5848,6 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>   	int r, emulation_type = EMULTYPE_PF;
>   	bool direct = vcpu->arch.mmu->root_role.direct;
>   
> -	/*
> -	 * WARN if hardware generates a fault with an error code that collides
> -	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
> -	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
> -	 * flag that KVM doesn't know about.
> -	 */
> -	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
> -		error_code &= ~PFERR_SYNTHETIC_MASK;
> -
>   	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
>   		return RET_PF_RETRY;
>   
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index e90b429c84f1..199c4dd8d214 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2055,6 +2055,15 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>   	u64 fault_address = svm->vmcb->control.exit_info_2;
>   	u64 error_code = svm->vmcb->control.exit_info_1;
>   
> +	/*
> +	 * WARN if hardware generates a fault with an error code that collides
> +	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
> +	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
> +	 * flag that KVM doesn't know about.
> +	 */
> +	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
> +		error_code &= ~PFERR_SYNTHETIC_MASK;
> +
>   	trace_kvm_page_fault(vcpu, fault_address, error_code);
>   	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
>   			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?

..  in npf_interception() or some common place like in 
kvm_mmu_page_fault()?

Otherwise,

Reviewed-by: Kai Huang <kai.huang@intel.com>
Sean Christopherson Feb. 29, 2024, 10:52 p.m. UTC | #2 
On Fri, Mar 01, 2024, Kai Huang wrote:
> 
> 
> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
> > Move the sanity check that hardware never sets bits that collide with KVM-
> > define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
> > i.e. make the sanity check #NPF specific.  The legacy #PF path already
> > WARNs if _any_ of bits 63:32 are set, and the error code that comes from
> > VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
> > EXIT_QUALIFICATION into error code flags).
> > 
> > Add a compile-time assert in the legacy #PF handler to make sure that KVM-
> > define flags are covered by its existing sanity check on the upper bits.
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/kvm/mmu/mmu.c | 12 +++---------
> >   arch/x86/kvm/svm/svm.c |  9 +++++++++
> >   2 files changed, 12 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > index 5d892bd59c97..bd342ebd0809 100644
> > --- a/arch/x86/kvm/mmu/mmu.c
> > +++ b/arch/x86/kvm/mmu/mmu.c
> > @@ -4561,6 +4561,9 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
> >   	if (WARN_ON_ONCE(error_code >> 32))
> >   		error_code = lower_32_bits(error_code);
> > +	/* Ensure the above sanity check also covers KVM-defined flags. */
> > +	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
> > +
> 
> Could you explain why adding this BUILD_BUG_ON() here, but not ...
> 
> >   	vcpu->arch.l1tf_flush_l1d = true;
> >   	if (!flags) {
> >   		trace_kvm_page_fault(vcpu, fault_address, error_code);
> > @@ -5845,15 +5848,6 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
> >   	int r, emulation_type = EMULTYPE_PF;
> >   	bool direct = vcpu->arch.mmu->root_role.direct;
> > -	/*
> > -	 * WARN if hardware generates a fault with an error code that collides
> > -	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
> > -	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
> > -	 * flag that KVM doesn't know about.
> > -	 */
> > -	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
> > -		error_code &= ~PFERR_SYNTHETIC_MASK;
> > -
> >   	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
> >   		return RET_PF_RETRY;
> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index e90b429c84f1..199c4dd8d214 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -2055,6 +2055,15 @@ static int npf_interception(struct kvm_vcpu *vcpu)
> >   	u64 fault_address = svm->vmcb->control.exit_info_2;
> >   	u64 error_code = svm->vmcb->control.exit_info_1;
> > +	/*
> > +	 * WARN if hardware generates a fault with an error code that collides
> > +	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
> > +	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
> > +	 * flag that KVM doesn't know about.
> > +	 */
> > +	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
> > +		error_code &= ~PFERR_SYNTHETIC_MASK;
> > +
> >   	trace_kvm_page_fault(vcpu, fault_address, error_code);
> >   	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
> >   			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
> 
> ...  in npf_interception() or

The intent of the BUILD_BUG_ON() is to ensure that kvm_handle_page_fault()'s
sanity check that bits 63:32 also serves as a sanity check that hardware doesn't
generate an error code that collides with any of KVM's synthetic flags.

E.g. if we were to add a KVM-defined flag in the lower 32 bits, then the #NPF
path would Just Work, because it already sanity checks all synthetic bits.  But
the #PF path would need new code, thus the BUILD_BUG_ON() to scream that new code
is needed.

> some common place like in kvm_mmu_page_fault()?

Because again, the logic being enforced is very specific to intercepted #PFs.
Kai Huang Feb. 29, 2024, 11:14 p.m. UTC | #3 
On 1/03/2024 11:52 am, Sean Christopherson wrote:
> On Fri, Mar 01, 2024, Kai Huang wrote:
>>
>>
>> On 28/02/2024 3:41 pm, Sean Christopherson wrote:
>>> Move the sanity check that hardware never sets bits that collide with KVM-
>>> define synthetic bits from kvm_mmu_page_fault() to npf_interception(),
>>> i.e. make the sanity check #NPF specific.  The legacy #PF path already
>>> WARNs if _any_ of bits 63:32 are set, and the error code that comes from
>>> VMX's EPT Violatation and Misconfig is 100% synthesized (KVM morphs VMX's
>>> EXIT_QUALIFICATION into error code flags).
>>>
>>> Add a compile-time assert in the legacy #PF handler to make sure that KVM-
>>> define flags are covered by its existing sanity check on the upper bits.
>>>
>>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>>> ---
>>>    arch/x86/kvm/mmu/mmu.c | 12 +++---------
>>>    arch/x86/kvm/svm/svm.c |  9 +++++++++
>>>    2 files changed, 12 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
>>> index 5d892bd59c97..bd342ebd0809 100644
>>> --- a/arch/x86/kvm/mmu/mmu.c
>>> +++ b/arch/x86/kvm/mmu/mmu.c
>>> @@ -4561,6 +4561,9 @@ int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
>>>    	if (WARN_ON_ONCE(error_code >> 32))
>>>    		error_code = lower_32_bits(error_code);
>>> +	/* Ensure the above sanity check also covers KVM-defined flags. */
>>> +	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
>>> +
>>
>> Could you explain why adding this BUILD_BUG_ON() here, but not ...
>>
>>>    	vcpu->arch.l1tf_flush_l1d = true;
>>>    	if (!flags) {
>>>    		trace_kvm_page_fault(vcpu, fault_address, error_code);
>>> @@ -5845,15 +5848,6 @@ int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
>>>    	int r, emulation_type = EMULTYPE_PF;
>>>    	bool direct = vcpu->arch.mmu->root_role.direct;
>>> -	/*
>>> -	 * WARN if hardware generates a fault with an error code that collides
>>> -	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
>>> -	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
>>> -	 * flag that KVM doesn't know about.
>>> -	 */
>>> -	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
>>> -		error_code &= ~PFERR_SYNTHETIC_MASK;
>>> -
>>>    	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
>>>    		return RET_PF_RETRY;
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index e90b429c84f1..199c4dd8d214 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -2055,6 +2055,15 @@ static int npf_interception(struct kvm_vcpu *vcpu)
>>>    	u64 fault_address = svm->vmcb->control.exit_info_2;
>>>    	u64 error_code = svm->vmcb->control.exit_info_1;
>>> +	/*
>>> +	 * WARN if hardware generates a fault with an error code that collides
>>> +	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
>>> +	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
>>> +	 * flag that KVM doesn't know about.
>>> +	 */
>>> +	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
>>> +		error_code &= ~PFERR_SYNTHETIC_MASK;
>>> +
>>>    	trace_kvm_page_fault(vcpu, fault_address, error_code);
>>>    	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
>>>    			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?
>>
>> ...  in npf_interception() or
> 
> The intent of the BUILD_BUG_ON() is to ensure that kvm_handle_page_fault()'s
> sanity check that bits 63:32 also serves as a sanity check that hardware doesn't
> generate an error code that collides with any of KVM's synthetic flags.
> 
> E.g. if we were to add a KVM-defined flag in the lower 32 bits, then the #NPF
> path would Just Work, because it already sanity checks all synthetic bits.  But
> the #PF path would need new code, thus the BUILD_BUG_ON() to scream that new code
> is needed.

Ah, right.  Thanks for explaining :-)

Reviewed-by: Kai Huang <kai.huang@intel.com>
diff mbox series

Patch

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 5d892bd59c97..bd342ebd0809 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -4561,6 +4561,9 @@  int kvm_handle_page_fault(struct kvm_vcpu *vcpu, u64 error_code,
 	if (WARN_ON_ONCE(error_code >> 32))
 		error_code = lower_32_bits(error_code);
 
+	/* Ensure the above sanity check also covers KVM-defined flags. */
+	BUILD_BUG_ON(lower_32_bits(PFERR_SYNTHETIC_MASK));
+
 	vcpu->arch.l1tf_flush_l1d = true;
 	if (!flags) {
 		trace_kvm_page_fault(vcpu, fault_address, error_code);
@@ -5845,15 +5848,6 @@  int noinline kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 err
 	int r, emulation_type = EMULTYPE_PF;
 	bool direct = vcpu->arch.mmu->root_role.direct;
 
-	/*
-	 * WARN if hardware generates a fault with an error code that collides
-	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
-	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
-	 * flag that KVM doesn't know about.
-	 */
-	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
-		error_code &= ~PFERR_SYNTHETIC_MASK;
-
 	if (WARN_ON_ONCE(!VALID_PAGE(vcpu->arch.mmu->root.hpa)))
 		return RET_PF_RETRY;
 
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index e90b429c84f1..199c4dd8d214 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2055,6 +2055,15 @@  static int npf_interception(struct kvm_vcpu *vcpu)
 	u64 fault_address = svm->vmcb->control.exit_info_2;
 	u64 error_code = svm->vmcb->control.exit_info_1;
 
+	/*
+	 * WARN if hardware generates a fault with an error code that collides
+	 * with KVM-defined sythentic flags.  Clear the flags and continue on,
+	 * i.e. don't terminate the VM, as KVM can't possibly be relying on a
+	 * flag that KVM doesn't know about.
+	 */
+	if (WARN_ON_ONCE(error_code & PFERR_SYNTHETIC_MASK))
+		error_code &= ~PFERR_SYNTHETIC_MASK;
+
 	trace_kvm_page_fault(vcpu, fault_address, error_code);
 	return kvm_mmu_page_fault(vcpu, fault_address, error_code,
 			static_cpu_has(X86_FEATURE_DECODEASSISTS) ?