diff mbox series

[Part2,RFC,v4,32/40] KVM: SVM: Add support to handle GHCB GPA register VMGEXIT

Message ID 20210707183616.5620-33-brijesh.singh@amd.com
State New
Headers show
Series [Part2,RFC,v4,01/40] KVM: SVM: Add support to handle AP reset MSR protocol | expand

Commit Message

Brijesh Singh July 7, 2021, 6:36 p.m. UTC
SEV-SNP guests are required to perform a GHCB GPA registration (see
section 2.5.2 in GHCB specification). Before using a GHCB GPA for a vCPU
the first time, a guest must register the vCPU GHCB GPA. If hypervisor
can work with the guest requested GPA then it must respond back with the
same GPA otherwise return -1.

On VMEXIT, Verify that GHCB GPA matches with the registered value. If a
mismatch is detected then abort the guest.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 arch/x86/include/asm/sev-common.h |  2 ++
 arch/x86/kvm/svm/sev.c            | 25 +++++++++++++++++++++++++
 arch/x86/kvm/svm/svm.h            |  7 +++++++
 3 files changed, 34 insertions(+)

Comments

Sean Christopherson July 16, 2021, 8:45 p.m. UTC | #1
On Wed, Jul 07, 2021, Brijesh Singh wrote:
> SEV-SNP guests are required to perform a GHCB GPA registration (see
> section 2.5.2 in GHCB specification). Before using a GHCB GPA for a vCPU

It's section 2.3.2 in version 2.0 of the spec.

> the first time, a guest must register the vCPU GHCB GPA. If hypervisor
> can work with the guest requested GPA then it must respond back with the
> same GPA otherwise return -1.
>
> On VMEXIT, Verify that GHCB GPA matches with the registered value. If a
> mismatch is detected then abort the guest.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  arch/x86/include/asm/sev-common.h |  2 ++
>  arch/x86/kvm/svm/sev.c            | 25 +++++++++++++++++++++++++
>  arch/x86/kvm/svm/svm.h            |  7 +++++++
>  3 files changed, 34 insertions(+)
> 
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 466baa9cd0f5..6990d5a9d73c 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -60,8 +60,10 @@
>  	GHCB_MSR_GPA_REG_REQ)
>  
>  #define GHCB_MSR_GPA_REG_RESP		0x013
> +#define GHCB_MSR_GPA_REG_ERROR		GENMASK_ULL(51, 0)
>  #define GHCB_MSR_GPA_REG_RESP_VAL(v)	((v) >> GHCB_MSR_GPA_REG_VALUE_POS)
>  
> +
>  /* SNP Page State Change */
>  #define GHCB_MSR_PSC_REQ		0x014
>  #define SNP_PAGE_STATE_PRIVATE		1
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index fd2d00ad80b7..3af5d1ad41bf 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2922,6 +2922,25 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>  				GHCB_MSR_INFO_MASK, GHCB_MSR_INFO_POS);
>  		break;
>  	}
> +	case GHCB_MSR_GPA_REG_REQ: {

Shouldn't KVM also support "Get preferred GHCB GPA", at least to the point where
it responds with "No preferred GPA".  AFAICT, this series doesn't cover that,
i.e. KVM will kill a guest that requests the VMM's preferred GPA.

> +		kvm_pfn_t pfn;
> +		u64 gfn;
> +
> +		gfn = get_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_GFN_MASK,
> +					GHCB_MSR_GPA_REG_VALUE_POS);

This is confusing, the MASK/POS reference both GPA and GFN.

> +
> +		pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
> +		if (is_error_noslot_pfn(pfn))

Checking the mapped PFN at this time isn't wrong, but it's also not complete,
e.g. nothing prevents userspace from changing the gpa->hva mapping after the
initial registration.  Not that that's likely to happen (or not break the guest),
but my point is that random checks on the backing PFN really have no meaning in
KVM unless KVM can guarantee that the PFN is stable for the duration of its use.

And conversely, the GHCB doesn't require the GHCB to be shared until the first
use.  E.g. arguably KVM should fully check the usability of the GPA, but the
GHCB spec disallows that.  And I honestly can't see why SNP is special with
respect to the GHCB.  ES guests will explode just as badly if the GPA points at
garbage.

I guess I'm not against the check, but it feels extremely arbitrary.

> +			gfn = GHCB_MSR_GPA_REG_ERROR;
> +		else
> +			svm->ghcb_registered_gpa = gfn_to_gpa(gfn);
> +
> +		set_ghcb_msr_bits(svm, gfn, GHCB_MSR_GPA_REG_GFN_MASK,
> +				  GHCB_MSR_GPA_REG_VALUE_POS);
> +		set_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_RESP, GHCB_MSR_INFO_MASK,
> +				  GHCB_MSR_INFO_POS);
> +		break;
> +	}
>  	case GHCB_MSR_TERM_REQ: {
>  		u64 reason_set, reason_code;
>  
> @@ -2970,6 +2989,12 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>  		return -EINVAL;
>  	}
>  
> +	/* SEV-SNP guest requires that the GHCB GPA must be registered */
> +	if (sev_snp_guest(svm->vcpu.kvm) && !ghcb_gpa_is_registered(svm, ghcb_gpa)) {
> +		vcpu_unimpl(&svm->vcpu, "vmgexit: GHCB GPA [%#llx] is not registered.\n", ghcb_gpa);

I saw this a few other place.  vcpu_unimpl() is not the right API.  KVM supports
the guest request, the problem is that the GHCB spec _requires_ KVM to terminate
the guest in this case.

> +		return -EINVAL;
> +	}
> +
>  	svm->ghcb = svm->ghcb_map.hva;
>  	ghcb = svm->ghcb_map.hva;
>  
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 32abcbd774d0..af4cce39b30f 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -185,6 +185,8 @@ struct vcpu_svm {
>  	bool ghcb_sa_free;
>  
>  	bool guest_state_loaded;
> +
> +	u64 ghcb_registered_gpa;
>  };
>  
>  struct svm_cpu_data {
> @@ -245,6 +247,11 @@ static inline bool sev_snp_guest(struct kvm *kvm)
>  #endif
>  }
>  
> +static inline bool ghcb_gpa_is_registered(struct vcpu_svm *svm, u64 val)
> +{
> +	return svm->ghcb_registered_gpa == val;
> +}
> +
>  static inline void vmcb_mark_all_dirty(struct vmcb *vmcb)
>  {
>  	vmcb->control.clean = 0;
> -- 
> 2.17.1
>
Brijesh Singh July 17, 2021, 12:44 a.m. UTC | #2
On 7/16/21 3:45 PM, Sean Christopherson wrote:
> On Wed, Jul 07, 2021, Brijesh Singh wrote:
>> SEV-SNP guests are required to perform a GHCB GPA registration (see
>> section 2.5.2 in GHCB specification). Before using a GHCB GPA for a vCPU
> It's section 2.3.2 in version 2.0 of the spec.
Ah, I will fix it.
>
>> the first time, a guest must register the vCPU GHCB GPA. If hypervisor
>> can work with the guest requested GPA then it must respond back with the
>> same GPA otherwise return -1.
>>
>> On VMEXIT, Verify that GHCB GPA matches with the registered value. If a
>> mismatch is detected then abort the guest.
>>
>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>> ---
>>  arch/x86/include/asm/sev-common.h |  2 ++
>>  arch/x86/kvm/svm/sev.c            | 25 +++++++++++++++++++++++++
>>  arch/x86/kvm/svm/svm.h            |  7 +++++++
>>  3 files changed, 34 insertions(+)
>>
>> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
>> index 466baa9cd0f5..6990d5a9d73c 100644
>> --- a/arch/x86/include/asm/sev-common.h
>> +++ b/arch/x86/include/asm/sev-common.h
>> @@ -60,8 +60,10 @@
>>  	GHCB_MSR_GPA_REG_REQ)
>>  
>>  #define GHCB_MSR_GPA_REG_RESP		0x013
>> +#define GHCB_MSR_GPA_REG_ERROR		GENMASK_ULL(51, 0)
>>  #define GHCB_MSR_GPA_REG_RESP_VAL(v)	((v) >> GHCB_MSR_GPA_REG_VALUE_POS)
>>  
>> +
>>  /* SNP Page State Change */
>>  #define GHCB_MSR_PSC_REQ		0x014
>>  #define SNP_PAGE_STATE_PRIVATE		1
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index fd2d00ad80b7..3af5d1ad41bf 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2922,6 +2922,25 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>>  				GHCB_MSR_INFO_MASK, GHCB_MSR_INFO_POS);
>>  		break;
>>  	}
>> +	case GHCB_MSR_GPA_REG_REQ: {
> Shouldn't KVM also support "Get preferred GHCB GPA", at least to the point where
> it responds with "No preferred GPA".  AFAICT, this series doesn't cover that,
> i.e. KVM will kill a guest that requests the VMM's preferred GPA.

Good point, for the completeness we should add the preferred GPA MSR and
return appropriate response code to cover the cases where non Linux
guest may use this vmgexit to determine the GHCB GPA.


>
>> +		kvm_pfn_t pfn;
>> +		u64 gfn;
>> +
>> +		gfn = get_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_GFN_MASK,
>> +					GHCB_MSR_GPA_REG_VALUE_POS);
> This is confusing, the MASK/POS reference both GPA and GFN.

Let me see if I can improve it to avoid the naming confusion. Most of
the naming recommending came during the part1 review, I will check with
Boris and other to see if they are okay with new names.


>
>> +
>> +		pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
>> +		if (is_error_noslot_pfn(pfn))
> Checking the mapped PFN at this time isn't wrong, but it's also not complete,
> e.g. nothing prevents userspace from changing the gpa->hva mapping after the
> initial registration.  Not that that's likely to happen (or not break the guest),
> but my point is that random checks on the backing PFN really have no meaning in
> KVM unless KVM can guarantee that the PFN is stable for the duration of its use.
>
> And conversely, the GHCB doesn't require the GHCB to be shared until the first
> use.  E.g. arguably KVM should fully check the usability of the GPA, but the
> GHCB spec disallows that.  And I honestly can't see why SNP is special with
> respect to the GHCB.  ES guests will explode just as badly if the GPA points at
> garbage.
>
> I guess I'm not against the check, but it feels extremely arbitrary.
>
>> +			gfn = GHCB_MSR_GPA_REG_ERROR;
>> +		else
>> +			svm->ghcb_registered_gpa = gfn_to_gpa(gfn);
>> +
>> +		set_ghcb_msr_bits(svm, gfn, GHCB_MSR_GPA_REG_GFN_MASK,
>> +				  GHCB_MSR_GPA_REG_VALUE_POS);
>> +		set_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_RESP, GHCB_MSR_INFO_MASK,
>> +				  GHCB_MSR_INFO_POS);
>> +		break;
>> +	}
>>  	case GHCB_MSR_TERM_REQ: {
>>  		u64 reason_set, reason_code;
>>  
>> @@ -2970,6 +2989,12 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>>  		return -EINVAL;
>>  	}
>>  
>> +	/* SEV-SNP guest requires that the GHCB GPA must be registered */
>> +	if (sev_snp_guest(svm->vcpu.kvm) && !ghcb_gpa_is_registered(svm, ghcb_gpa)) {
>> +		vcpu_unimpl(&svm->vcpu, "vmgexit: GHCB GPA [%#llx] is not registered.\n", ghcb_gpa);
> I saw this a few other place.  vcpu_unimpl() is not the right API.  KVM supports
> the guest request, the problem is that the GHCB spec _requires_ KVM to terminate
> the guest in this case.

What is the preferred method to log it so that someone debugging know
what went wrong.

thanks
Sean Christopherson July 19, 2021, 8:04 p.m. UTC | #3
On Fri, Jul 16, 2021, Brijesh Singh wrote:
> 

> On 7/16/21 3:45 PM, Sean Christopherson wrote:

> > On Wed, Jul 07, 2021, Brijesh Singh wrote:

> >> +	/* SEV-SNP guest requires that the GHCB GPA must be registered */

> >> +	if (sev_snp_guest(svm->vcpu.kvm) && !ghcb_gpa_is_registered(svm, ghcb_gpa)) {

> >> +		vcpu_unimpl(&svm->vcpu, "vmgexit: GHCB GPA [%#llx] is not registered.\n", ghcb_gpa);

> > I saw this a few other place.  vcpu_unimpl() is not the right API.  KVM supports

> > the guest request, the problem is that the GHCB spec _requires_ KVM to terminate

> > the guest in this case.

> 

> What is the preferred method to log it so that someone debugging know

> what went wrong.


Using the kernel log is probably a bad choice in general for this error.  Because
this and the other GHCB GPA sanity checks can be triggered from the guest, any
kernel logging needs to be ratelimited.  Ratelimiting is problematic because it
means some errors may not be logged; that's quite unlikely in this case, but it's
less than ideal.

The other issue is that KVM can't dump the RIP because guest state is encrypted,
e.g. KVM can provide the task PID, but that's it.

The best solution I can think of at the moment would be some form of
KVM_EXIT_INTERNAL_ERROR, i.e. kick out to userspace with a meaningful error code
and the bad GPA so that userspace can take action.

I believe Jim also has some thoughts on how to improve "logging" of guest errors,
but he's on vacation for a few weeks.
diff mbox series

Patch

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 466baa9cd0f5..6990d5a9d73c 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -60,8 +60,10 @@ 
 	GHCB_MSR_GPA_REG_REQ)
 
 #define GHCB_MSR_GPA_REG_RESP		0x013
+#define GHCB_MSR_GPA_REG_ERROR		GENMASK_ULL(51, 0)
 #define GHCB_MSR_GPA_REG_RESP_VAL(v)	((v) >> GHCB_MSR_GPA_REG_VALUE_POS)
 
+
 /* SNP Page State Change */
 #define GHCB_MSR_PSC_REQ		0x014
 #define SNP_PAGE_STATE_PRIVATE		1
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index fd2d00ad80b7..3af5d1ad41bf 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2922,6 +2922,25 @@  static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 				GHCB_MSR_INFO_MASK, GHCB_MSR_INFO_POS);
 		break;
 	}
+	case GHCB_MSR_GPA_REG_REQ: {
+		kvm_pfn_t pfn;
+		u64 gfn;
+
+		gfn = get_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_GFN_MASK,
+					GHCB_MSR_GPA_REG_VALUE_POS);
+
+		pfn = kvm_vcpu_gfn_to_pfn(vcpu, gfn);
+		if (is_error_noslot_pfn(pfn))
+			gfn = GHCB_MSR_GPA_REG_ERROR;
+		else
+			svm->ghcb_registered_gpa = gfn_to_gpa(gfn);
+
+		set_ghcb_msr_bits(svm, gfn, GHCB_MSR_GPA_REG_GFN_MASK,
+				  GHCB_MSR_GPA_REG_VALUE_POS);
+		set_ghcb_msr_bits(svm, GHCB_MSR_GPA_REG_RESP, GHCB_MSR_INFO_MASK,
+				  GHCB_MSR_INFO_POS);
+		break;
+	}
 	case GHCB_MSR_TERM_REQ: {
 		u64 reason_set, reason_code;
 
@@ -2970,6 +2989,12 @@  int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		return -EINVAL;
 	}
 
+	/* SEV-SNP guest requires that the GHCB GPA must be registered */
+	if (sev_snp_guest(svm->vcpu.kvm) && !ghcb_gpa_is_registered(svm, ghcb_gpa)) {
+		vcpu_unimpl(&svm->vcpu, "vmgexit: GHCB GPA [%#llx] is not registered.\n", ghcb_gpa);
+		return -EINVAL;
+	}
+
 	svm->ghcb = svm->ghcb_map.hva;
 	ghcb = svm->ghcb_map.hva;
 
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 32abcbd774d0..af4cce39b30f 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -185,6 +185,8 @@  struct vcpu_svm {
 	bool ghcb_sa_free;
 
 	bool guest_state_loaded;
+
+	u64 ghcb_registered_gpa;
 };
 
 struct svm_cpu_data {
@@ -245,6 +247,11 @@  static inline bool sev_snp_guest(struct kvm *kvm)
 #endif
 }
 
+static inline bool ghcb_gpa_is_registered(struct vcpu_svm *svm, u64 val)
+{
+	return svm->ghcb_registered_gpa == val;
+}
+
 static inline void vmcb_mark_all_dirty(struct vmcb *vmcb)
 {
 	vmcb->control.clean = 0;