[PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
exit code and parameters fail. Since the VMGEXIT instruction can be issued
from userspace, even though userspace (likely) can't update the GHCB,
don't allow userspace to be able to kill the guest.

Return a #GP request through the GHCB when validation fails, rather than
terminating the guest.

Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/kvm/svm/sev.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 5bc887e9a986..bc77f26f0880 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2075,7 +2075,7 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
 }
 
-static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
+static bool sev_es_validate_vmgexit(struct vcpu_svm *svm)
 {
 	struct kvm_vcpu *vcpu;
 	struct ghcb *ghcb;
@@ -2174,7 +2174,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 		goto vmgexit_err;
 	}
 
-	return 0;
+	return true;
 
 vmgexit_err:
 	vcpu = &svm->vcpu;
@@ -2188,13 +2188,16 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 		dump_ghcb(svm);
 	}
 
-	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
-	vcpu->run->internal.ndata = 2;
-	vcpu->run->internal.data[0] = exit_code;
-	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+	/* Clear the valid entries fields */
+	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
 
-	return -EINVAL;
+	ghcb_set_sw_exit_info_1(ghcb, 1);
+	ghcb_set_sw_exit_info_2(ghcb,
+				X86_TRAP_GP |
+				SVM_EVTINJ_TYPE_EXEPT |
+				SVM_EVTINJ_VALID);
+
+	return false;
 }
 
 void sev_es_unmap_ghcb(struct vcpu_svm *svm)
@@ -2459,9 +2462,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 
 	exit_code = ghcb_get_sw_exit_code(ghcb);
 
-	ret = sev_es_validate_vmgexit(svm);
-	if (ret)
-		return ret;
+	if (!sev_es_validate_vmgexit(svm))
+		return 1;
 
 	sev_es_sync_from_ghcb(svm);
 	ghcb_set_sw_exit_info_1(ghcb, 0);
-- 
2.31.0

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Peter Gonda]

On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> from userspace, even though userspace (likely) can't update the GHCB,
> don't allow userspace to be able to kill the guest.
>
> Return a #GP request through the GHCB when validation fails, rather than
> terminating the guest.

Is this a gap in the spec? I don't see anything that details what
should happen if the correct fields for NAE are not set in the first
couple paragraphs of section 4 'GHCB Protocol'.

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 5/14/21 6:06 PM, Peter Gonda wrote:
> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>
>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
>> from userspace, even though userspace (likely) can't update the GHCB,
>> don't allow userspace to be able to kill the guest.
>>
>> Return a #GP request through the GHCB when validation fails, rather than
>> terminating the guest.
> 
> Is this a gap in the spec? I don't see anything that details what
> should happen if the correct fields for NAE are not set in the first
> couple paragraphs of section 4 'GHCB Protocol'.

No, I don't think the spec needs to spell out everything like this. The
hypervisor is free to determine its course of action in this case.

I suppose the spec could suggest a course of action, but I don't think the
spec should require a specific course of action.

Thanks,
Tom

> 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Mon, May 17, 2021, Tom Lendacky wrote:
> On 5/14/21 6:06 PM, Peter Gonda wrote:
> > On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>
> >> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> >> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> >> from userspace, even though userspace (likely) can't update the GHCB,
> >> don't allow userspace to be able to kill the guest.
> >>
> >> Return a #GP request through the GHCB when validation fails, rather than
> >> terminating the guest.
> > 
> > Is this a gap in the spec? I don't see anything that details what
> > should happen if the correct fields for NAE are not set in the first
> > couple paragraphs of section 4 'GHCB Protocol'.
> 
> No, I don't think the spec needs to spell out everything like this. The
> hypervisor is free to determine its course of action in this case.

The hypervisor can decide whether to inject/return an error or kill the guest,
but what errors can be returned and how they're returned absolutely needs to be
ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
is the logical place to define said ABI.

For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
which means if something does go awry KVM has just made debugging the guest that
much harder, e.g. imagine the confusion that will ensue if the end result is a
SIGBUS to userspace on CPUID.

There needs to be an explicit error code for "you gave me bad data", otherwise
we're signing ourselves up for future pain.

> I suppose the spec could suggest a course of action, but I don't think the
> spec should require a specific course of action.
> 
> Thanks,
> Tom
> 
> > 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Thu, May 20, 2021, Sean Christopherson wrote:
> On Mon, May 17, 2021, Tom Lendacky wrote:
> > On 5/14/21 6:06 PM, Peter Gonda wrote:
> > > On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > >>
> > >> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> > >> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> > >> from userspace, even though userspace (likely) can't update the GHCB,
> > >> don't allow userspace to be able to kill the guest.
> > >>
> > >> Return a #GP request through the GHCB when validation fails, rather than
> > >> terminating the guest.
> > > 
> > > Is this a gap in the spec? I don't see anything that details what
> > > should happen if the correct fields for NAE are not set in the first
> > > couple paragraphs of section 4 'GHCB Protocol'.
> > 
> > No, I don't think the spec needs to spell out everything like this. The
> > hypervisor is free to determine its course of action in this case.
> 
> The hypervisor can decide whether to inject/return an error or kill the guest,
> but what errors can be returned and how they're returned absolutely needs to be
> ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
> is the logical place to define said ABI.
> 
> For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
> completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
> which means if something does go awry KVM has just made debugging the guest that
> much harder, e.g. imagine the confusion that will ensue if the end result is a
> SIGBUS to userspace on CPUID.
> 
> There needs to be an explicit error code for "you gave me bad data", otherwise
> we're signing ourselves up for future pain.

More concretely, I think the best course of action is to define a new return code
in SW_EXITINFO1[31:0], e.g. '2', with additional information in SW_EXITINFO2.

In theory, an old-but-sane guest will interpret the unexpected return code as
fatal to whatever triggered the #VMGEXIT, e.g. SIGBUS to userspace.  Unfortunately
Linux isn't sane because sev_es_ghcb_hv_call() assumes any non-'1' result means
success, but that's trivial to fix and IMO should be fixed irrespective of where
this goes.

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Thu, May 20, 2021, Sean Christopherson wrote:
> On Thu, May 20, 2021, Sean Christopherson wrote:
> > On Mon, May 17, 2021, Tom Lendacky wrote:
> > > On 5/14/21 6:06 PM, Peter Gonda wrote:
> > > > On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> > > >>
> > > >> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> > > >> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> > > >> from userspace, even though userspace (likely) can't update the GHCB,
> > > >> don't allow userspace to be able to kill the guest.
> > > >>
> > > >> Return a #GP request through the GHCB when validation fails, rather than
> > > >> terminating the guest.
> > > > 
> > > > Is this a gap in the spec? I don't see anything that details what
> > > > should happen if the correct fields for NAE are not set in the first
> > > > couple paragraphs of section 4 'GHCB Protocol'.
> > > 
> > > No, I don't think the spec needs to spell out everything like this. The
> > > hypervisor is free to determine its course of action in this case.
> > 
> > The hypervisor can decide whether to inject/return an error or kill the guest,
> > but what errors can be returned and how they're returned absolutely needs to be
> > ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
> > is the logical place to define said ABI.
> > 
> > For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
> > completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
> > which means if something does go awry KVM has just made debugging the guest that
> > much harder, e.g. imagine the confusion that will ensue if the end result is a
> > SIGBUS to userspace on CPUID.
> > 
> > There needs to be an explicit error code for "you gave me bad data", otherwise
> > we're signing ourselves up for future pain.
> 
> More concretely, I think the best course of action is to define a new return code
> in SW_EXITINFO1[31:0], e.g. '2', with additional information in SW_EXITINFO2.
> 
> In theory, an old-but-sane guest will interpret the unexpected return code as
> fatal to whatever triggered the #VMGEXIT, e.g. SIGBUS to userspace.  Unfortunately
> Linux isn't sane because sev_es_ghcb_hv_call() assumes any non-'1' result means
> success, but that's trivial to fix and IMO should be fixed irrespective of where
> this goes.

One last thing (hopefully): Erdem pointed out that if the GCHB GPA (or any
derferenced pointers within the GHCB) is invalid or is set to a private GPA
(mostly in the context of SNP) then the VMM will likely have no choice but to
kill the guest in response to #VMGEXIT.

It's probably a good idea to add a blurb in one of the specs explicitly calling
out that #VMGEXIT can be executed from userspace, and that before returning to
uesrspace the guest kernel must always ensure that the GCHB points at a legal
GPA _and_ all primary fields are marked invalid. 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 5/20/21 2:16 PM, Sean Christopherson wrote:
> On Mon, May 17, 2021, Tom Lendacky wrote:
>> On 5/14/21 6:06 PM, Peter Gonda wrote:
>>> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>
>>>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
>>>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
>>>> from userspace, even though userspace (likely) can't update the GHCB,
>>>> don't allow userspace to be able to kill the guest.
>>>>
>>>> Return a #GP request through the GHCB when validation fails, rather than
>>>> terminating the guest.
>>>
>>> Is this a gap in the spec? I don't see anything that details what
>>> should happen if the correct fields for NAE are not set in the first
>>> couple paragraphs of section 4 'GHCB Protocol'.
>>
>> No, I don't think the spec needs to spell out everything like this. The
>> hypervisor is free to determine its course of action in this case.
> 
> The hypervisor can decide whether to inject/return an error or kill the guest,
> but what errors can be returned and how they're returned absolutely needs to be
> ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
> is the logical place to define said ABI.

For now, that is all we have for versions 1 and 2 of the spec. We can
certainly extend it in future versions if that is desired.

I would suggest starting a thread on what we would like to see in the next
version of the GHCB spec on the amd-sev-snp mailing list:

	amd-sev-snp@lists.suse.com

> 
> For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
> completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
> which means if something does go awry KVM has just made debugging the guest that
> much harder, e.g. imagine the confusion that will ensue if the end result is a
> SIGBUS to userspace on CPUID.

I see the point you're making, but I would also say that we probably
wouldn't even boot successfully if the kernel can't handle, e.g., a CPUID
#VC properly. A lot of what could go wrong with required inputs, not the
values, but the required state being communicated, should have already
been ironed out during development of whichever OS is providing the SEV-ES
support.

> 
> There needs to be an explicit error code for "you gave me bad data", otherwise
> we're signing ourselves up for future pain.

I'll make note of that for the next update to the spec and we can work on
it further during the spec review.

Thanks,
Tom

> 
>> I suppose the spec could suggest a course of action, but I don't think the
>> spec should require a specific course of action.
>>
>> Thanks,
>> Tom
>>
>>>

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 5/20/21 3:22 PM, Sean Christopherson wrote:
> On Thu, May 20, 2021, Sean Christopherson wrote:
>> On Thu, May 20, 2021, Sean Christopherson wrote:
>>> On Mon, May 17, 2021, Tom Lendacky wrote:
>>>> On 5/14/21 6:06 PM, Peter Gonda wrote:
>>>>> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>>>>>>
>>>>>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
>>>>>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
>>>>>> from userspace, even though userspace (likely) can't update the GHCB,
>>>>>> don't allow userspace to be able to kill the guest.
>>>>>>
>>>>>> Return a #GP request through the GHCB when validation fails, rather than
>>>>>> terminating the guest.
>>>>>
>>>>> Is this a gap in the spec? I don't see anything that details what
>>>>> should happen if the correct fields for NAE are not set in the first
>>>>> couple paragraphs of section 4 'GHCB Protocol'.
>>>>
>>>> No, I don't think the spec needs to spell out everything like this. The
>>>> hypervisor is free to determine its course of action in this case.
>>>
>>> The hypervisor can decide whether to inject/return an error or kill the guest,
>>> but what errors can be returned and how they're returned absolutely needs to be
>>> ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
>>> is the logical place to define said ABI.
>>>
>>> For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
>>> completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
>>> which means if something does go awry KVM has just made debugging the guest that
>>> much harder, e.g. imagine the confusion that will ensue if the end result is a
>>> SIGBUS to userspace on CPUID.
>>>
>>> There needs to be an explicit error code for "you gave me bad data", otherwise
>>> we're signing ourselves up for future pain.
>>
>> More concretely, I think the best course of action is to define a new return code
>> in SW_EXITINFO1[31:0], e.g. '2', with additional information in SW_EXITINFO2.
>>
>> In theory, an old-but-sane guest will interpret the unexpected return code as
>> fatal to whatever triggered the #VMGEXIT, e.g. SIGBUS to userspace.  Unfortunately
>> Linux isn't sane because sev_es_ghcb_hv_call() assumes any non-'1' result means
>> success, but that's trivial to fix and IMO should be fixed irrespective of where
>> this goes.
> 
> One last thing (hopefully): Erdem pointed out that if the GCHB GPA (or any
> derferenced pointers within the GHCB) is invalid or is set to a private GPA
> (mostly in the context of SNP) then the VMM will likely have no choice but to
> kill the guest in response to #VMGEXIT.
> 
> It's probably a good idea to add a blurb in one of the specs explicitly calling
> out that #VMGEXIT can be executed from userspace, and that before returning to
> uesrspace the guest kernel must always ensure that the GCHB points at a legal
> GPA _and_ all primary fields are marked invalid. 

Yes, the spec can be updated to include a "best practices" section for
OSes and Hypervisors to follow without actually having to update the
version of the GHCB spec, so that should be doable.

Thanks,
Tom

> 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Thu, May 20, 2021, Tom Lendacky wrote:
> On 5/20/21 2:16 PM, Sean Christopherson wrote:
> > On Mon, May 17, 2021, Tom Lendacky wrote:
> >> On 5/14/21 6:06 PM, Peter Gonda wrote:
> >>> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >>>>
> >>>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> >>>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> >>>> from userspace, even though userspace (likely) can't update the GHCB,
> >>>> don't allow userspace to be able to kill the guest.
> >>>>
> >>>> Return a #GP request through the GHCB when validation fails, rather than
> >>>> terminating the guest.
> >>>
> >>> Is this a gap in the spec? I don't see anything that details what
> >>> should happen if the correct fields for NAE are not set in the first
> >>> couple paragraphs of section 4 'GHCB Protocol'.
> >>
> >> No, I don't think the spec needs to spell out everything like this. The
> >> hypervisor is free to determine its course of action in this case.
> > 
> > The hypervisor can decide whether to inject/return an error or kill the guest,
> > but what errors can be returned and how they're returned absolutely needs to be
> > ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
> > is the logical place to define said ABI.
> 
> For now, that is all we have for versions 1 and 2 of the spec. We can
> certainly extend it in future versions if that is desired.
> 
> I would suggest starting a thread on what we would like to see in the next
> version of the GHCB spec on the amd-sev-snp mailing list:
> 
> 	amd-sev-snp@lists.suse.com

Will do, but in the meantime, I don't think we should merge a fix of any kind
until there is consensus on what the VMM behavior will be.  IMO, fixing this in
upstream is not urgent; I highly doubt anyone is deploying SEV-ES in production
using a bleeding edge KVM.

> > For example, "injecting" #GP if the guest botched the GHCB on #VMGEXIT(CPUID) is
> > completely nonsensical.  As is, a Linux guest appears to blindly forward the #GP,
> > which means if something does go awry KVM has just made debugging the guest that
> > much harder, e.g. imagine the confusion that will ensue if the end result is a
> > SIGBUS to userspace on CPUID.
> 
> I see the point you're making, but I would also say that we probably
> wouldn't even boot successfully if the kernel can't handle, e.g., a CPUID
> #VC properly. 

I agree that GHCB bugs in the guest will be fatal, but that doesn't give the VMM
carte blanche to do whatever it wants given bad input.

> A lot of what could go wrong with required inputs, not the values, but the
> required state being communicated, should have already been ironed out during
> development of whichever OS is providing the SEV-ES support.

Yes, but better  on the kernel never having a regression is a losing proposition.
And it doesn't even necessarily require a regression, e.g. an existing memory
corruption bug elsewhere in the guest kernel (that escaped qualification) could
corrupt the GHCB.  If the GHCB is corrupted at runtime, the guest needs
well-defined semantics from the VMM so that the guest at least has a chance of
sanely handling the error.  Handling in this case would mean an oops/panic, but
that's far, far better than a random pseudo-#GP that might not even be immediately
logged as a failure.

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Vitaly Kuznetsov]

Sean Christopherson <seanjc@google.com> writes:

> On Thu, May 20, 2021, Tom Lendacky wrote:
>> On 5/20/21 2:16 PM, Sean Christopherson wrote:
>> > On Mon, May 17, 2021, Tom Lendacky wrote:
>> >> On 5/14/21 6:06 PM, Peter Gonda wrote:
>> >>> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>> >>>>
>> >>>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
>> >>>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
>> >>>> from userspace, even though userspace (likely) can't update the GHCB,
>> >>>> don't allow userspace to be able to kill the guest.
>> >>>>
>> >>>> Return a #GP request through the GHCB when validation fails, rather than
>> >>>> terminating the guest.
>> >>>
>> >>> Is this a gap in the spec? I don't see anything that details what
>> >>> should happen if the correct fields for NAE are not set in the first
>> >>> couple paragraphs of section 4 'GHCB Protocol'.
>> >>
>> >> No, I don't think the spec needs to spell out everything like this. The
>> >> hypervisor is free to determine its course of action in this case.
>> > 
>> > The hypervisor can decide whether to inject/return an error or kill the guest,
>> > but what errors can be returned and how they're returned absolutely needs to be
>> > ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
>> > is the logical place to define said ABI.
>> 
>> For now, that is all we have for versions 1 and 2 of the spec. We can
>> certainly extend it in future versions if that is desired.
>> 
>> I would suggest starting a thread on what we would like to see in the next
>> version of the GHCB spec on the amd-sev-snp mailing list:
>> 
>> 	amd-sev-snp@lists.suse.com
>
> Will do, but in the meantime, I don't think we should merge a fix of any kind
> until there is consensus on what the VMM behavior will be.  IMO, fixing this in
> upstream is not urgent; I highly doubt anyone is deploying SEV-ES in production
> using a bleeding edge KVM.

Sorry for resurrecting this old thread but were there any deveopments
here? I may have missed something but last time I've checked a single
"rep; vmmcall" from userspace was still crashing the guest. The issue,
however, doesn't seem to reproduce with Vmware ESXi which probably means
they're just skipping the instruction and not even injecting #GP (AFAIR,
I don't have an environment to re-test handy).

-- 
Vitaly

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Wed, Jul 21, 2021, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
> 
> > On Thu, May 20, 2021, Tom Lendacky wrote:
> >> On 5/20/21 2:16 PM, Sean Christopherson wrote:
> >> > On Mon, May 17, 2021, Tom Lendacky wrote:
> >> >> On 5/14/21 6:06 PM, Peter Gonda wrote:
> >> >>> On Fri, May 14, 2021 at 1:22 PM Tom Lendacky <thomas.lendacky@amd.com> wrote:
> >> >>>>
> >> >>>> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> >> >>>> exit code and parameters fail. Since the VMGEXIT instruction can be issued
> >> >>>> from userspace, even though userspace (likely) can't update the GHCB,
> >> >>>> don't allow userspace to be able to kill the guest.
> >> >>>>
> >> >>>> Return a #GP request through the GHCB when validation fails, rather than
> >> >>>> terminating the guest.
> >> >>>
> >> >>> Is this a gap in the spec? I don't see anything that details what
> >> >>> should happen if the correct fields for NAE are not set in the first
> >> >>> couple paragraphs of section 4 'GHCB Protocol'.
> >> >>
> >> >> No, I don't think the spec needs to spell out everything like this. The
> >> >> hypervisor is free to determine its course of action in this case.
> >> > 
> >> > The hypervisor can decide whether to inject/return an error or kill the guest,
> >> > but what errors can be returned and how they're returned absolutely needs to be
> >> > ABI between guest and host, and to make the ABI vendor agnostic the GHCB spec
> >> > is the logical place to define said ABI.
> >> 
> >> For now, that is all we have for versions 1 and 2 of the spec. We can
> >> certainly extend it in future versions if that is desired.
> >> 
> >> I would suggest starting a thread on what we would like to see in the next
> >> version of the GHCB spec on the amd-sev-snp mailing list:
> >> 
> >> 	amd-sev-snp@lists.suse.com
> >
> > Will do, but in the meantime, I don't think we should merge a fix of any kind
> > until there is consensus on what the VMM behavior will be.  IMO, fixing this in
> > upstream is not urgent; I highly doubt anyone is deploying SEV-ES in production
> > using a bleeding edge KVM.
> 
> Sorry for resurrecting this old thread but were there any deveopments
> here? I may have missed something but last time I've checked a single
> "rep; vmmcall" from userspace was still crashing the guest.

I don't think it went anywhere, I completely forgot about this.  I'll bump this
back to the top of my todo list, unless someone else wants the honors :-)

> The issue, however, doesn't seem to reproduce with Vmware ESXi which probably
> means they're just skipping the instruction and not even injecting #GP
> (AFAIR, I don't have an environment to re-test handy).

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Marc Orr]

On Fri, Dec 3, 2021 at 11:00 AM Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 12/3/21 10:39 AM, Sean Christopherson wrote:
> > On Thu, Dec 02, 2021, Tom Lendacky wrote:
>
> >>
> >> -    return -EINVAL;
> >> +    return false;
> >
> > I'd really prefer that this helper continue to return 0/-EINVAL, there's no hint
> > in the function name that this return true/false.  And given the usage, there's
> > no advantage to returning true/false.  On the contrary, if there's a future
> > condition where this needs to exit to userspace, we'll end up switching this all
> > back to int.
>
> I don't have any objection to that.

I think Sean's review makes a pretty compelling case that we should
keep the int return value for `setup_vmgexit_scratch()`. In
particular, failing to allocate a host kernel buffer definitely seems
like a host error that should return to userspace. Though, failing to
read the guest GPA seems less clear cut on who is at fault (host vs.
guest), as Tom mentioned. My understanding from the commit description
is that the entire point of the patch is to protect the guest from
mis-behaving guest userspace code. So I would think that if we have a
case like mapping the guest GPA that could fail due to the guest or
the host, we should probably go ahead and use the new GHCB error codes
to return back to the guest in this case. But either way, having an
int return code seems like the way to go for
`setup_vmgexit_scratch()`. Because if we are wrong in either
direction, it's a trivial fix.

It's not as obvious to me that converting `sev_es_validate_vmgexit()`
to return a bool is not cleaner than returning an int. This function
seems to pretty much just process the GHCB buffer as a self-contained
set of bits. So it's hard to imagine how this could fail in a way
where exiting to userspace is the right thing to do. That being said,
I do not have a strong objection to returning an int. Sean is right
that an int is definitely more future proof. And I'm sure Sean (and
Tom) have much better insight into how validating the bits written
into the GHCB could potentially require code that could justify
exiting out to userspace.

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 12/3/21 12:59 PM, Tom Lendacky wrote:
> On 12/3/21 10:39 AM, Sean Christopherson wrote:
>> On Thu, Dec 02, 2021, Tom Lendacky wrote:
> 

>>
>> IMO, this should be the patch (compile tested only).
> 
> I can test this, but probably won't be able to get to it until Monday.

I did manage to get some time to test this today. Works as expected with 
an invalid scratch GPA terminating the guest.

Up to Paolo now with what he would like to see/do.

Thanks,
Tom

> 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Fri, Dec 03, 2021, Tom Lendacky wrote:
> On 12/3/21 10:39 AM, Sean Christopherson wrote:
> > On Thu, Dec 02, 2021, Tom Lendacky wrote:
> > > +			goto e_scratch;
> > >   		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
> > >   			/* Unable to copy scratch area from guest */
> > >   			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
> > >   			kvfree(scratch_va);
> > > -			return -EFAULT;
> > > +			goto e_scratch;
> > 
> > Same here, failure to read guest memory is a userspace issue and needs to be
> > reported to userspace.
> 
> But it could be a guest issue as well...  whichever is preferred is ok by me.

Arguably, any guest issue is a violation of the guest's contract with userspace,
and thus userspace needs to decide how to proceed.  E.g. userspace defines what
is RAM vs. MMIO and communicates that directly to the guest, KVM is not involved
in deciding what is/isn't RAM nor in communicating that information to the guest.
If the scratch GPA doesn't resolve to a memslot, then the guest is not honoring
the memory configuration as defined by userspace.

And if userspace unmaps an hva for whatever reason, then exiting to userspace
with -EFAULT is absolutely the right thing to do.  KVM's ABI currently sucks and
doesn't provide enough information to act on the -EFAULT, but I really want to
change that as there are multiple use cases, e.g. uffd and virtiofs truncation,
that shouldn't require any work in KVM beyond returning -EFAULT with a small
amount of metadata.

KVM could define its ABI such that failure to access the scratch area is reflected
into the guest, i.e. establish a contract with userspace, but IMO that's undesirable
as it limits KVM's options in the future, e.g. IIRC, in the potential uffd case any
failure on a uaccess needs to kick out to userspace.  KVM does have several cases
where it reflects these errors into the guest, e.g. kvm_pv_clock_pairing() and
Hyper-V emulation, but I would prefer we change those instead of adding more code
that assumes any memory failure is the guest's fault.

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 12/3/21 10:39 AM, Sean Christopherson wrote:
> On Thu, Dec 02, 2021, Tom Lendacky wrote:

>>   
>> -	return -EINVAL;
>> +	return false;
> 
> I'd really prefer that this helper continue to return 0/-EINVAL, there's no hint
> in the function name that this return true/false.  And given the usage, there's
> no advantage to returning true/false.  On the contrary, if there's a future
> condition where this needs to exit to userspace, we'll end up switching this all
> back to int.

I don't have any objection to that.

> 

>>   		}
>>   		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
>>   		if (!scratch_va)
>> -			return -ENOMEM;
> 
> ...because this is wrong.  Failure to allocate memory should exit to userspace,
> not report an error to the guest.
> 
>> +			goto e_scratch;
>>   
>>   		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
>>   			/* Unable to copy scratch area from guest */
>>   			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
>>   
>>   			kvfree(scratch_va);
>> -			return -EFAULT;
>> +			goto e_scratch;
> 
> Same here, failure to read guest memory is a userspace issue and needs to be
> reported to userspace.

But it could be a guest issue as well...  whichever is preferred is ok by me.

> 
>>   		}
>>   
>>   		/*
> 
> IMO, this should be the patch (compile tested only).

I can test this, but probably won't be able to get to it until Monday.

Thanks,
Tom

> 
> ---
>   arch/x86/include/asm/sev-common.h | 11 +++++
>   arch/x86/kvm/svm/sev.c            | 75 +++++++++++++++++++------------
>   2 files changed, 58 insertions(+), 28 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 2cef6c5a52c2..6acaf5af0a3d 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -73,4 +73,15 @@
> 
>   #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
> 
> +/*
> + * Error codes related to GHCB input that can be communicated back to the guest
> + * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
> + */
> +#define GHCB_ERR_NOT_REGISTERED		1
> +#define GHCB_ERR_INVALID_USAGE		2
> +#define GHCB_ERR_INVALID_SCRATCH_AREA	3
> +#define GHCB_ERR_MISSING_INPUT		4
> +#define GHCB_ERR_INVALID_INPUT		5
> +#define GHCB_ERR_INVALID_EVENT		6
> +
>   #endif
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 713e3daa9574..60c6d7b216eb 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2357,20 +2357,25 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>   {
>   	struct kvm_vcpu *vcpu;
>   	struct ghcb *ghcb;
> -	u64 exit_code = 0;
> +	u64 exit_code;
> +	u64 reason;
> 
>   	ghcb = svm->sev_es.ghcb;
> 
> -	/* Only GHCB Usage code 0 is supported */
> -	if (ghcb->ghcb_usage)
> -		goto vmgexit_err;
> -
>   	/*
> -	 * Retrieve the exit code now even though is may not be marked valid
> +	 * Retrieve the exit code now even though it may not be marked valid
>   	 * as it could help with debugging.
>   	 */
>   	exit_code = ghcb_get_sw_exit_code(ghcb);
> 
> +	/* Only GHCB Usage code 0 is supported */
> +	if (ghcb->ghcb_usage) {
> +		reason = GHCB_ERR_INVALID_USAGE;
> +		goto vmgexit_err;
> +	}
> +
> +	reason = GHCB_ERR_MISSING_INPUT;
> +
>   	if (!ghcb_sw_exit_code_is_valid(ghcb) ||
>   	    !ghcb_sw_exit_info_1_is_valid(ghcb) ||
>   	    !ghcb_sw_exit_info_2_is_valid(ghcb))
> @@ -2449,6 +2454,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>   	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>   		break;
>   	default:
> +		reason = GHCB_ERR_INVALID_EVENT;
>   		goto vmgexit_err;
>   	}
> 
> @@ -2457,22 +2463,25 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>   vmgexit_err:
>   	vcpu = &svm->vcpu;
> 
> -	if (ghcb->ghcb_usage) {
> +	if (reason == GHCB_ERR_INVALID_USAGE) {
>   		vcpu_unimpl(vcpu, "vmgexit: ghcb usage %#x is not valid\n",
>   			    ghcb->ghcb_usage);
> +	} else if (reason == GHCB_ERR_INVALID_EVENT) {
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
> +			    exit_code);
>   	} else {
> -		vcpu_unimpl(vcpu, "vmgexit: exit reason %#llx is not valid\n",
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
>   			    exit_code);
>   		dump_ghcb(svm);
>   	}
> 
> -	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> -	vcpu->run->internal.ndata = 2;
> -	vcpu->run->internal.data[0] = exit_code;
> -	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
> +	/* Clear the valid entries fields */
> +	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> 
> -	return -EINVAL;
> +	ghcb_set_sw_exit_info_1(ghcb, 2);
> +	ghcb_set_sw_exit_info_2(ghcb, reason);
> +
> +	return 1;
>   }
> 
>   void sev_es_unmap_ghcb(struct vcpu_svm *svm)
> @@ -2542,14 +2551,14 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
>   	if (!scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch gpa not provided\n");
> -		return -EINVAL;
> +		goto e_scratch;
>   	}
> 
>   	scratch_gpa_end = scratch_gpa_beg + len;
>   	if (scratch_gpa_end < scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
>   		       len, scratch_gpa_beg);
> -		return -EINVAL;
> +		goto e_scratch;
>   	}
> 
>   	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
> @@ -2567,7 +2576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		    scratch_gpa_end > ghcb_scratch_end) {
>   			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
>   			       scratch_gpa_beg, scratch_gpa_end);
> -			return -EINVAL;
> +			goto e_scratch;
>   		}
> 
>   		scratch_va = (void *)svm->sev_es.ghcb;
> @@ -2580,7 +2589,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		if (len > GHCB_SCRATCH_AREA_LIMIT) {
>   			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
>   			       len, GHCB_SCRATCH_AREA_LIMIT);
> -			return -EINVAL;
> +			goto e_scratch;
>   		}
>   		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
>   		if (!scratch_va)
> @@ -2608,6 +2617,12 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	svm->sev_es.ghcb_sa_len = len;
> 
>   	return 0;
> +
> +e_scratch:
> +	ghcb_set_sw_exit_info_1(ghcb, 2);
> +	ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
> +
> +	return 1;
>   }
> 
>   static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
> @@ -2658,7 +2673,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
> 
>   		ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_CPUID);
>   		if (!ret) {
> -			ret = -EINVAL;
> +			/* Error, keep GHCB MSR value as-is */
>   			break;
>   		}
> 
> @@ -2694,10 +2709,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>   						GHCB_MSR_TERM_REASON_POS);
>   		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
>   			reason_set, reason_code);
> -		fallthrough;
> +
> +		ret = -EINVAL;
> +		break;
>   	}
>   	default:
> -		ret = -EINVAL;
> +		/* Error, keep GHCB MSR value as-is */
> +		break;
>   	}
> 
>   	trace_kvm_vmgexit_msr_protocol_exit(svm->vcpu.vcpu_id,
> @@ -2721,14 +2739,18 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
> 
>   	if (!ghcb_gpa) {
>   		vcpu_unimpl(vcpu, "vmgexit: GHCB gpa is not set\n");
> -		return -EINVAL;
> +
> +		/* Without a GHCB, just return right back to the guest */
> +		return 1;
>   	}
> 
>   	if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) {
>   		/* Unable to map GHCB from guest */
>   		vcpu_unimpl(vcpu, "vmgexit: error mapping GHCB [%#llx] from guest\n",
>   			    ghcb_gpa);
> -		return -EINVAL;
> +
> +		/* Without a GHCB, just return right back to the guest */
> +		return 1;
>   	}
> 
>   	svm->sev_es.ghcb = svm->sev_es.ghcb_map.hva;
> @@ -2788,11 +2810,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   		default:
>   			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
>   			       control->exit_info_1);
> -			ghcb_set_sw_exit_info_1(ghcb, 1);
> -			ghcb_set_sw_exit_info_2(ghcb,
> -						X86_TRAP_UD |
> -						SVM_EVTINJ_TYPE_EXEPT |
> -						SVM_EVTINJ_VALID);
> +			ghcb_set_sw_exit_info_1(ghcb, 2);
> +			ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_INPUT);
>   		}
> 
>   		ret = 1;
> 
> base-commit: 70f433c2193fbfb5541ef98f973e087ddf2f9dfb
> --
> 

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Sean Christopherson]

On Thu, Dec 02, 2021, Tom Lendacky wrote:
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 713e3daa9574..322553322202 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2353,24 +2353,29 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>  	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
>  }
>  
> -static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> +static bool sev_es_validate_vmgexit(struct vcpu_svm *svm)
>  {

...

> -	return 0;
> +	return true;
>  
>  vmgexit_err:
>  	vcpu = &svm->vcpu;
>  
> -	if (ghcb->ghcb_usage) {
> +	if (reason == GHCB_ERR_INVALID_USAGE) {
>  		vcpu_unimpl(vcpu, "vmgexit: ghcb usage %#x is not valid\n",
>  			    ghcb->ghcb_usage);
> +	} else if (reason == GHCB_ERR_INVALID_EVENT) {
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
> +			    exit_code);
>  	} else {
> -		vcpu_unimpl(vcpu, "vmgexit: exit reason %#llx is not valid\n",
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
>  			    exit_code);
>  		dump_ghcb(svm);
>  	}
>  
> -	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> -	vcpu->run->internal.ndata = 2;
> -	vcpu->run->internal.data[0] = exit_code;
> -	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
> +	/* Clear the valid entries fields */
> +	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> +
> +	ghcb_set_sw_exit_info_1(ghcb, 2);
> +	ghcb_set_sw_exit_info_2(ghcb, reason);
>  
> -	return -EINVAL;
> +	return false;

I'd really prefer that this helper continue to return 0/-EINVAL, there's no hint
in the function name that this return true/false.  And given the usage, there's
no advantage to returning true/false.  On the contrary, if there's a future
condition where this needs to exit to userspace, we'll end up switching this all
back to int.

>  }
>  
>  void sev_es_unmap_ghcb(struct vcpu_svm *svm)
> @@ -2531,7 +2540,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>  }
>  
>  #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> -static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
> +static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)

Same here, but now there's an actual need to return an int...

>  {
>  	struct vmcb_control_area *control = &svm->vmcb->control;
>  	struct ghcb *ghcb = svm->sev_es.ghcb;
> @@ -2542,14 +2551,14 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>  	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
>  	if (!scratch_gpa_beg) {
>  		pr_err("vmgexit: scratch gpa not provided\n");
> -		return -EINVAL;
> +		goto e_scratch;
>  	}
>  
>  	scratch_gpa_end = scratch_gpa_beg + len;
>  	if (scratch_gpa_end < scratch_gpa_beg) {
>  		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
>  		       len, scratch_gpa_beg);
> -		return -EINVAL;
> +		goto e_scratch;
>  	}
>  
>  	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
> @@ -2567,7 +2576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>  		    scratch_gpa_end > ghcb_scratch_end) {
>  			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
>  			       scratch_gpa_beg, scratch_gpa_end);
> -			return -EINVAL;
> +			goto e_scratch;
>  		}
>  
>  		scratch_va = (void *)svm->sev_es.ghcb;
> @@ -2580,18 +2589,18 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>  		if (len > GHCB_SCRATCH_AREA_LIMIT) {
>  			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
>  			       len, GHCB_SCRATCH_AREA_LIMIT);
> -			return -EINVAL;
> +			goto e_scratch;
>  		}
>  		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
>  		if (!scratch_va)
> -			return -ENOMEM;

...because this is wrong.  Failure to allocate memory should exit to userspace,
not report an error to the guest.

> +			goto e_scratch;
>  
>  		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
>  			/* Unable to copy scratch area from guest */
>  			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
>  
>  			kvfree(scratch_va);
> -			return -EFAULT;
> +			goto e_scratch;

Same here, failure to read guest memory is a userspace issue and needs to be
reported to userspace.

>  		}
>  
>  		/*

IMO, this should be the patch (compile tested only).

---
 arch/x86/include/asm/sev-common.h | 11 +++++
 arch/x86/kvm/svm/sev.c            | 75 +++++++++++++++++++------------
 2 files changed, 58 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 2cef6c5a52c2..6acaf5af0a3d 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -73,4 +73,15 @@

 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)

+/*
+ * Error codes related to GHCB input that can be communicated back to the guest
+ * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
+ */
+#define GHCB_ERR_NOT_REGISTERED		1
+#define GHCB_ERR_INVALID_USAGE		2
+#define GHCB_ERR_INVALID_SCRATCH_AREA	3
+#define GHCB_ERR_MISSING_INPUT		4
+#define GHCB_ERR_INVALID_INPUT		5
+#define GHCB_ERR_INVALID_EVENT		6
+
 #endif
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 713e3daa9574..60c6d7b216eb 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2357,20 +2357,25 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 {
 	struct kvm_vcpu *vcpu;
 	struct ghcb *ghcb;
-	u64 exit_code = 0;
+	u64 exit_code;
+	u64 reason;

 	ghcb = svm->sev_es.ghcb;

-	/* Only GHCB Usage code 0 is supported */
-	if (ghcb->ghcb_usage)
-		goto vmgexit_err;
-
 	/*
-	 * Retrieve the exit code now even though is may not be marked valid
+	 * Retrieve the exit code now even though it may not be marked valid
 	 * as it could help with debugging.
 	 */
 	exit_code = ghcb_get_sw_exit_code(ghcb);

+	/* Only GHCB Usage code 0 is supported */
+	if (ghcb->ghcb_usage) {
+		reason = GHCB_ERR_INVALID_USAGE;
+		goto vmgexit_err;
+	}
+
+	reason = GHCB_ERR_MISSING_INPUT;
+
 	if (!ghcb_sw_exit_code_is_valid(ghcb) ||
 	    !ghcb_sw_exit_info_1_is_valid(ghcb) ||
 	    !ghcb_sw_exit_info_2_is_valid(ghcb))
@@ -2449,6 +2454,7 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 		break;
 	default:
+		reason = GHCB_ERR_INVALID_EVENT;
 		goto vmgexit_err;
 	}

@@ -2457,22 +2463,25 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 vmgexit_err:
 	vcpu = &svm->vcpu;

-	if (ghcb->ghcb_usage) {
+	if (reason == GHCB_ERR_INVALID_USAGE) {
 		vcpu_unimpl(vcpu, "vmgexit: ghcb usage %#x is not valid\n",
 			    ghcb->ghcb_usage);
+	} else if (reason == GHCB_ERR_INVALID_EVENT) {
+		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
+			    exit_code);
 	} else {
-		vcpu_unimpl(vcpu, "vmgexit: exit reason %#llx is not valid\n",
+		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
 			    exit_code);
 		dump_ghcb(svm);
 	}

-	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
-	vcpu->run->internal.ndata = 2;
-	vcpu->run->internal.data[0] = exit_code;
-	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+	/* Clear the valid entries fields */
+	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));

-	return -EINVAL;
+	ghcb_set_sw_exit_info_1(ghcb, 2);
+	ghcb_set_sw_exit_info_2(ghcb, reason);
+
+	return 1;
 }

 void sev_es_unmap_ghcb(struct vcpu_svm *svm)
@@ -2542,14 +2551,14 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
 	if (!scratch_gpa_beg) {
 		pr_err("vmgexit: scratch gpa not provided\n");
-		return -EINVAL;
+		goto e_scratch;
 	}

 	scratch_gpa_end = scratch_gpa_beg + len;
 	if (scratch_gpa_end < scratch_gpa_beg) {
 		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
 		       len, scratch_gpa_beg);
-		return -EINVAL;
+		goto e_scratch;
 	}

 	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
@@ -2567,7 +2576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		    scratch_gpa_end > ghcb_scratch_end) {
 			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
 			       scratch_gpa_beg, scratch_gpa_end);
-			return -EINVAL;
+			goto e_scratch;
 		}

 		scratch_va = (void *)svm->sev_es.ghcb;
@@ -2580,7 +2589,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		if (len > GHCB_SCRATCH_AREA_LIMIT) {
 			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
 			       len, GHCB_SCRATCH_AREA_LIMIT);
-			return -EINVAL;
+			goto e_scratch;
 		}
 		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
 		if (!scratch_va)
@@ -2608,6 +2617,12 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	svm->sev_es.ghcb_sa_len = len;

 	return 0;
+
+e_scratch:
+	ghcb_set_sw_exit_info_1(ghcb, 2);
+	ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
+
+	return 1;
 }

 static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
@@ -2658,7 +2673,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)

 		ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_CPUID);
 		if (!ret) {
-			ret = -EINVAL;
+			/* Error, keep GHCB MSR value as-is */
 			break;
 		}

@@ -2694,10 +2709,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 						GHCB_MSR_TERM_REASON_POS);
 		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
 			reason_set, reason_code);
-		fallthrough;
+
+		ret = -EINVAL;
+		break;
 	}
 	default:
-		ret = -EINVAL;
+		/* Error, keep GHCB MSR value as-is */
+		break;
 	}

 	trace_kvm_vmgexit_msr_protocol_exit(svm->vcpu.vcpu_id,
@@ -2721,14 +2739,18 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)

 	if (!ghcb_gpa) {
 		vcpu_unimpl(vcpu, "vmgexit: GHCB gpa is not set\n");
-		return -EINVAL;
+
+		/* Without a GHCB, just return right back to the guest */
+		return 1;
 	}

 	if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) {
 		/* Unable to map GHCB from guest */
 		vcpu_unimpl(vcpu, "vmgexit: error mapping GHCB [%#llx] from guest\n",
 			    ghcb_gpa);
-		return -EINVAL;
+
+		/* Without a GHCB, just return right back to the guest */
+		return 1;
 	}

 	svm->sev_es.ghcb = svm->sev_es.ghcb_map.hva;
@@ -2788,11 +2810,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		default:
 			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
 			       control->exit_info_1);
-			ghcb_set_sw_exit_info_1(ghcb, 1);
-			ghcb_set_sw_exit_info_2(ghcb,
-						X86_TRAP_UD |
-						SVM_EVTINJ_TYPE_EXEPT |
-						SVM_EVTINJ_VALID);
+			ghcb_set_sw_exit_info_1(ghcb, 2);
+			ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_INPUT);
 		}

 		ret = 1;

base-commit: 70f433c2193fbfb5541ef98f973e087ddf2f9dfb
--

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Paolo Bonzini]

On 12/2/21 20:39, Tom Lendacky wrote:
>> Queued, thanks.  Though it would have been nicer to split the changes 
>> in the return values (e.g. for setup_vmgexit_scratch and 
>> sev_es_validate_vmgexit) from the introduction of the new GHCB exitinfo.
> 
> I can still do that if it will help make things easier. Let me know.

Well, at this point I've already reviewed it. :)

Paolo

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

On 12/2/21 1:19 PM, Paolo Bonzini wrote:
> On 12/2/21 19:52, Tom Lendacky wrote:
> 
> Queued, thanks.  Though it would have been nicer to split the changes in 
> the return values (e.g. for setup_vmgexit_scratch and 
> sev_es_validate_vmgexit) from the introduction of the new GHCB exitinfo.

I can still do that if it will help make things easier. Let me know.

Thanks,
Tom

> 
> Paolo
> 
> Paolo

Re: [PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Paolo Bonzini]

On 12/2/21 19:52, Tom Lendacky wrote:
> Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
> exit code or exit parameters fails.
> 
> The VMGEXIT instruction can be issued from userspace, even though
> userspace (likely) can't update the GHCB. To prevent userspace from being
> able to kill the guest, return an error through the GHCB when validation
> fails rather than terminating the guest. For cases where the GHCB can't be
> updated (e.g. the GHCB can't be mapped, etc.), just return back to the
> guest.
> 
> The new error codes are documented in the lasest update to the GHCB
> specification.
> 
> Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>   arch/x86/include/asm/sev-common.h |  11 ++++
>   arch/x86/kvm/svm/sev.c            | 106 +++++++++++++++++-------------
>   2 files changed, 71 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
> index 2cef6c5a52c2..6acaf5af0a3d 100644
> --- a/arch/x86/include/asm/sev-common.h
> +++ b/arch/x86/include/asm/sev-common.h
> @@ -73,4 +73,15 @@
>   
>   #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
>   
> +/*
> + * Error codes related to GHCB input that can be communicated back to the guest
> + * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
> + */
> +#define GHCB_ERR_NOT_REGISTERED		1
> +#define GHCB_ERR_INVALID_USAGE		2
> +#define GHCB_ERR_INVALID_SCRATCH_AREA	3
> +#define GHCB_ERR_MISSING_INPUT		4
> +#define GHCB_ERR_INVALID_INPUT		5
> +#define GHCB_ERR_INVALID_EVENT		6
> +
>   #endif
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 713e3daa9574..322553322202 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2353,24 +2353,29 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
>   	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
>   }
>   
> -static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> +static bool sev_es_validate_vmgexit(struct vcpu_svm *svm)
>   {
>   	struct kvm_vcpu *vcpu;
>   	struct ghcb *ghcb;
> -	u64 exit_code = 0;
> +	u64 exit_code;
> +	u64 reason;
>   
>   	ghcb = svm->sev_es.ghcb;
>   
> -	/* Only GHCB Usage code 0 is supported */
> -	if (ghcb->ghcb_usage)
> -		goto vmgexit_err;
> -
>   	/*
> -	 * Retrieve the exit code now even though is may not be marked valid
> +	 * Retrieve the exit code now even though it may not be marked valid
>   	 * as it could help with debugging.
>   	 */
>   	exit_code = ghcb_get_sw_exit_code(ghcb);
>   
> +	/* Only GHCB Usage code 0 is supported */
> +	if (ghcb->ghcb_usage) {
> +		reason = GHCB_ERR_INVALID_USAGE;
> +		goto vmgexit_err;
> +	}
> +
> +	reason = GHCB_ERR_MISSING_INPUT;
> +
>   	if (!ghcb_sw_exit_code_is_valid(ghcb) ||
>   	    !ghcb_sw_exit_info_1_is_valid(ghcb) ||
>   	    !ghcb_sw_exit_info_2_is_valid(ghcb))
> @@ -2449,30 +2454,34 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
>   	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
>   		break;
>   	default:
> +		reason = GHCB_ERR_INVALID_EVENT;
>   		goto vmgexit_err;
>   	}
>   
> -	return 0;
> +	return true;
>   
>   vmgexit_err:
>   	vcpu = &svm->vcpu;
>   
> -	if (ghcb->ghcb_usage) {
> +	if (reason == GHCB_ERR_INVALID_USAGE) {
>   		vcpu_unimpl(vcpu, "vmgexit: ghcb usage %#x is not valid\n",
>   			    ghcb->ghcb_usage);
> +	} else if (reason == GHCB_ERR_INVALID_EVENT) {
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
> +			    exit_code);
>   	} else {
> -		vcpu_unimpl(vcpu, "vmgexit: exit reason %#llx is not valid\n",
> +		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
>   			    exit_code);
>   		dump_ghcb(svm);
>   	}
>   
> -	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
> -	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
> -	vcpu->run->internal.ndata = 2;
> -	vcpu->run->internal.data[0] = exit_code;
> -	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
> +	/* Clear the valid entries fields */
> +	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> +
> +	ghcb_set_sw_exit_info_1(ghcb, 2);
> +	ghcb_set_sw_exit_info_2(ghcb, reason);
>   
> -	return -EINVAL;
> +	return false;
>   }
>   
>   void sev_es_unmap_ghcb(struct vcpu_svm *svm)
> @@ -2531,7 +2540,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
>   }
>   
>   #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
> -static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
> +static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   {
>   	struct vmcb_control_area *control = &svm->vmcb->control;
>   	struct ghcb *ghcb = svm->sev_es.ghcb;
> @@ -2542,14 +2551,14 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
>   	if (!scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch gpa not provided\n");
> -		return -EINVAL;
> +		goto e_scratch;
>   	}
>   
>   	scratch_gpa_end = scratch_gpa_beg + len;
>   	if (scratch_gpa_end < scratch_gpa_beg) {
>   		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
>   		       len, scratch_gpa_beg);
> -		return -EINVAL;
> +		goto e_scratch;
>   	}
>   
>   	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
> @@ -2567,7 +2576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		    scratch_gpa_end > ghcb_scratch_end) {
>   			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
>   			       scratch_gpa_beg, scratch_gpa_end);
> -			return -EINVAL;
> +			goto e_scratch;
>   		}
>   
>   		scratch_va = (void *)svm->sev_es.ghcb;
> @@ -2580,18 +2589,18 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   		if (len > GHCB_SCRATCH_AREA_LIMIT) {
>   			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
>   			       len, GHCB_SCRATCH_AREA_LIMIT);
> -			return -EINVAL;
> +			goto e_scratch;
>   		}
>   		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
>   		if (!scratch_va)
> -			return -ENOMEM;
> +			goto e_scratch;
>   
>   		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
>   			/* Unable to copy scratch area from guest */
>   			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
>   
>   			kvfree(scratch_va);
> -			return -EFAULT;
> +			goto e_scratch;
>   		}
>   
>   		/*
> @@ -2607,7 +2616,13 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
>   	svm->sev_es.ghcb_sa = scratch_va;
>   	svm->sev_es.ghcb_sa_len = len;
>   
> -	return 0;
> +	return true;
> +
> +e_scratch:
> +	ghcb_set_sw_exit_info_1(ghcb, 2);
> +	ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
> +
> +	return false;
>   }
>   
>   static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
> @@ -2658,7 +2673,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>   
>   		ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_CPUID);
>   		if (!ret) {
> -			ret = -EINVAL;
> +			/* Error, keep GHCB MSR value as-is */
>   			break;
>   		}
>   
> @@ -2694,10 +2709,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
>   						GHCB_MSR_TERM_REASON_POS);
>   		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
>   			reason_set, reason_code);
> -		fallthrough;
> +
> +		ret = -EINVAL;
> +		break;
>   	}
>   	default:
> -		ret = -EINVAL;
> +		/* Error, keep GHCB MSR value as-is */
> +		break;
>   	}
>   
>   	trace_kvm_vmgexit_msr_protocol_exit(svm->vcpu.vcpu_id,
> @@ -2721,14 +2739,18 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   
>   	if (!ghcb_gpa) {
>   		vcpu_unimpl(vcpu, "vmgexit: GHCB gpa is not set\n");
> -		return -EINVAL;
> +
> +		/* Without a GHCB, just return right back to the guest */
> +		return 1;
>   	}
>   
>   	if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) {
>   		/* Unable to map GHCB from guest */
>   		vcpu_unimpl(vcpu, "vmgexit: error mapping GHCB [%#llx] from guest\n",
>   			    ghcb_gpa);
> -		return -EINVAL;
> +
> +		/* Without a GHCB, just return right back to the guest */
> +		return 1;
>   	}
>   
>   	svm->sev_es.ghcb = svm->sev_es.ghcb_map.hva;
> @@ -2738,18 +2760,17 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   
>   	exit_code = ghcb_get_sw_exit_code(ghcb);
>   
> -	ret = sev_es_validate_vmgexit(svm);
> -	if (ret)
> -		return ret;
> +	if (!sev_es_validate_vmgexit(svm))
> +		return 1;
>   
>   	sev_es_sync_from_ghcb(svm);
>   	ghcb_set_sw_exit_info_1(ghcb, 0);
>   	ghcb_set_sw_exit_info_2(ghcb, 0);
>   
> +	ret = 1;
>   	switch (exit_code) {
>   	case SVM_VMGEXIT_MMIO_READ:
> -		ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
> -		if (ret)
> +		if (!setup_vmgexit_scratch(svm, true, control->exit_info_2))
>   			break;
>   
>   		ret = kvm_sev_es_mmio_read(vcpu,
> @@ -2758,8 +2779,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   					   svm->sev_es.ghcb_sa);
>   		break;
>   	case SVM_VMGEXIT_MMIO_WRITE:
> -		ret = setup_vmgexit_scratch(svm, false, control->exit_info_2);
> -		if (ret)
> +		if (!setup_vmgexit_scratch(svm, false, control->exit_info_2))
>   			break;
>   
>   		ret = kvm_sev_es_mmio_write(vcpu,
> @@ -2788,14 +2808,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
>   		default:
>   			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
>   			       control->exit_info_1);
> -			ghcb_set_sw_exit_info_1(ghcb, 1);
> -			ghcb_set_sw_exit_info_2(ghcb,
> -						X86_TRAP_UD |
> -						SVM_EVTINJ_TYPE_EXEPT |
> -						SVM_EVTINJ_VALID);
> +			ghcb_set_sw_exit_info_1(ghcb, 2);
> +			ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_INPUT);
>   		}
>   
> -		ret = 1;
>   		break;
>   	}
>   	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
> @@ -2815,7 +2831,6 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>   {
>   	int count;
>   	int bytes;
> -	int r;
>   
>   	if (svm->vmcb->control.exit_info_2 > INT_MAX)
>   		return -EINVAL;
> @@ -2824,9 +2839,8 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
>   	if (unlikely(check_mul_overflow(count, size, &bytes)))
>   		return -EINVAL;
>   
> -	r = setup_vmgexit_scratch(svm, in, bytes);
> -	if (r)
> -		return r;
> +	if (!setup_vmgexit_scratch(svm, in, bytes))
> +		return 1;
>   
>   	return kvm_sev_es_string_io(&svm->vcpu, size, port, svm->sev_es.ghcb_sa,
>   				    count, in);
> 

Queued, thanks.  Though it would have been nicer to split the changes in 
the return values (e.g. for setup_vmgexit_scratch and 
sev_es_validate_vmgexit) from the introduction of the new GHCB exitinfo.

Paolo

Paolo

[PATCH] KVM: SVM: Do not terminate SEV-ES guests on GHCB validation failure

[Tom Lendacky]

Currently, an SEV-ES guest is terminated if the validation of the VMGEXIT
exit code or exit parameters fails.

The VMGEXIT instruction can be issued from userspace, even though
userspace (likely) can't update the GHCB. To prevent userspace from being
able to kill the guest, return an error through the GHCB when validation
fails rather than terminating the guest. For cases where the GHCB can't be
updated (e.g. the GHCB can't be mapped, etc.), just return back to the
guest.

The new error codes are documented in the lasest update to the GHCB
specification.

Fixes: 291bd20d5d88 ("KVM: SVM: Add initial support for a VMGEXIT VMEXIT")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
 arch/x86/include/asm/sev-common.h |  11 ++++
 arch/x86/kvm/svm/sev.c            | 106 +++++++++++++++++-------------
 2 files changed, 71 insertions(+), 46 deletions(-)

diff --git a/arch/x86/include/asm/sev-common.h b/arch/x86/include/asm/sev-common.h
index 2cef6c5a52c2..6acaf5af0a3d 100644
--- a/arch/x86/include/asm/sev-common.h
+++ b/arch/x86/include/asm/sev-common.h
@@ -73,4 +73,15 @@
 
 #define GHCB_RESP_CODE(v)		((v) & GHCB_MSR_INFO_MASK)
 
+/*
+ * Error codes related to GHCB input that can be communicated back to the guest
+ * by setting the lower 32-bits of the GHCB SW_EXITINFO1 field to 2.
+ */
+#define GHCB_ERR_NOT_REGISTERED		1
+#define GHCB_ERR_INVALID_USAGE		2
+#define GHCB_ERR_INVALID_SCRATCH_AREA	3
+#define GHCB_ERR_MISSING_INPUT		4
+#define GHCB_ERR_INVALID_INPUT		5
+#define GHCB_ERR_INVALID_EVENT		6
+
 #endif
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 713e3daa9574..322553322202 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2353,24 +2353,29 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
 	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
 }
 
-static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
+static bool sev_es_validate_vmgexit(struct vcpu_svm *svm)
 {
 	struct kvm_vcpu *vcpu;
 	struct ghcb *ghcb;
-	u64 exit_code = 0;
+	u64 exit_code;
+	u64 reason;
 
 	ghcb = svm->sev_es.ghcb;
 
-	/* Only GHCB Usage code 0 is supported */
-	if (ghcb->ghcb_usage)
-		goto vmgexit_err;
-
 	/*
-	 * Retrieve the exit code now even though is may not be marked valid
+	 * Retrieve the exit code now even though it may not be marked valid
 	 * as it could help with debugging.
 	 */
 	exit_code = ghcb_get_sw_exit_code(ghcb);
 
+	/* Only GHCB Usage code 0 is supported */
+	if (ghcb->ghcb_usage) {
+		reason = GHCB_ERR_INVALID_USAGE;
+		goto vmgexit_err;
+	}
+
+	reason = GHCB_ERR_MISSING_INPUT;
+
 	if (!ghcb_sw_exit_code_is_valid(ghcb) ||
 	    !ghcb_sw_exit_info_1_is_valid(ghcb) ||
 	    !ghcb_sw_exit_info_2_is_valid(ghcb))
@@ -2449,30 +2454,34 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
 		break;
 	default:
+		reason = GHCB_ERR_INVALID_EVENT;
 		goto vmgexit_err;
 	}
 
-	return 0;
+	return true;
 
 vmgexit_err:
 	vcpu = &svm->vcpu;
 
-	if (ghcb->ghcb_usage) {
+	if (reason == GHCB_ERR_INVALID_USAGE) {
 		vcpu_unimpl(vcpu, "vmgexit: ghcb usage %#x is not valid\n",
 			    ghcb->ghcb_usage);
+	} else if (reason == GHCB_ERR_INVALID_EVENT) {
+		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
+			    exit_code);
 	} else {
-		vcpu_unimpl(vcpu, "vmgexit: exit reason %#llx is not valid\n",
+		vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
 			    exit_code);
 		dump_ghcb(svm);
 	}
 
-	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
-	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
-	vcpu->run->internal.ndata = 2;
-	vcpu->run->internal.data[0] = exit_code;
-	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+	/* Clear the valid entries fields */
+	memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
+
+	ghcb_set_sw_exit_info_1(ghcb, 2);
+	ghcb_set_sw_exit_info_2(ghcb, reason);
 
-	return -EINVAL;
+	return false;
 }
 
 void sev_es_unmap_ghcb(struct vcpu_svm *svm)
@@ -2531,7 +2540,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
 }
 
 #define GHCB_SCRATCH_AREA_LIMIT		(16ULL * PAGE_SIZE)
-static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
+static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 {
 	struct vmcb_control_area *control = &svm->vmcb->control;
 	struct ghcb *ghcb = svm->sev_es.ghcb;
@@ -2542,14 +2551,14 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	scratch_gpa_beg = ghcb_get_sw_scratch(ghcb);
 	if (!scratch_gpa_beg) {
 		pr_err("vmgexit: scratch gpa not provided\n");
-		return -EINVAL;
+		goto e_scratch;
 	}
 
 	scratch_gpa_end = scratch_gpa_beg + len;
 	if (scratch_gpa_end < scratch_gpa_beg) {
 		pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n",
 		       len, scratch_gpa_beg);
-		return -EINVAL;
+		goto e_scratch;
 	}
 
 	if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) {
@@ -2567,7 +2576,7 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		    scratch_gpa_end > ghcb_scratch_end) {
 			pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n",
 			       scratch_gpa_beg, scratch_gpa_end);
-			return -EINVAL;
+			goto e_scratch;
 		}
 
 		scratch_va = (void *)svm->sev_es.ghcb;
@@ -2580,18 +2589,18 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 		if (len > GHCB_SCRATCH_AREA_LIMIT) {
 			pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n",
 			       len, GHCB_SCRATCH_AREA_LIMIT);
-			return -EINVAL;
+			goto e_scratch;
 		}
 		scratch_va = kvzalloc(len, GFP_KERNEL_ACCOUNT);
 		if (!scratch_va)
-			return -ENOMEM;
+			goto e_scratch;
 
 		if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) {
 			/* Unable to copy scratch area from guest */
 			pr_err("vmgexit: kvm_read_guest for scratch area failed\n");
 
 			kvfree(scratch_va);
-			return -EFAULT;
+			goto e_scratch;
 		}
 
 		/*
@@ -2607,7 +2616,13 @@ static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len)
 	svm->sev_es.ghcb_sa = scratch_va;
 	svm->sev_es.ghcb_sa_len = len;
 
-	return 0;
+	return true;
+
+e_scratch:
+	ghcb_set_sw_exit_info_1(ghcb, 2);
+	ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_SCRATCH_AREA);
+
+	return false;
 }
 
 static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask,
@@ -2658,7 +2673,7 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 
 		ret = svm_invoke_exit_handler(vcpu, SVM_EXIT_CPUID);
 		if (!ret) {
-			ret = -EINVAL;
+			/* Error, keep GHCB MSR value as-is */
 			break;
 		}
 
@@ -2694,10 +2709,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
 						GHCB_MSR_TERM_REASON_POS);
 		pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
 			reason_set, reason_code);
-		fallthrough;
+
+		ret = -EINVAL;
+		break;
 	}
 	default:
-		ret = -EINVAL;
+		/* Error, keep GHCB MSR value as-is */
+		break;
 	}
 
 	trace_kvm_vmgexit_msr_protocol_exit(svm->vcpu.vcpu_id,
@@ -2721,14 +2739,18 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 
 	if (!ghcb_gpa) {
 		vcpu_unimpl(vcpu, "vmgexit: GHCB gpa is not set\n");
-		return -EINVAL;
+
+		/* Without a GHCB, just return right back to the guest */
+		return 1;
 	}
 
 	if (kvm_vcpu_map(vcpu, ghcb_gpa >> PAGE_SHIFT, &svm->sev_es.ghcb_map)) {
 		/* Unable to map GHCB from guest */
 		vcpu_unimpl(vcpu, "vmgexit: error mapping GHCB [%#llx] from guest\n",
 			    ghcb_gpa);
-		return -EINVAL;
+
+		/* Without a GHCB, just return right back to the guest */
+		return 1;
 	}
 
 	svm->sev_es.ghcb = svm->sev_es.ghcb_map.hva;
@@ -2738,18 +2760,17 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 
 	exit_code = ghcb_get_sw_exit_code(ghcb);
 
-	ret = sev_es_validate_vmgexit(svm);
-	if (ret)
-		return ret;
+	if (!sev_es_validate_vmgexit(svm))
+		return 1;
 
 	sev_es_sync_from_ghcb(svm);
 	ghcb_set_sw_exit_info_1(ghcb, 0);
 	ghcb_set_sw_exit_info_2(ghcb, 0);
 
+	ret = 1;
 	switch (exit_code) {
 	case SVM_VMGEXIT_MMIO_READ:
-		ret = setup_vmgexit_scratch(svm, true, control->exit_info_2);
-		if (ret)
+		if (!setup_vmgexit_scratch(svm, true, control->exit_info_2))
 			break;
 
 		ret = kvm_sev_es_mmio_read(vcpu,
@@ -2758,8 +2779,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 					   svm->sev_es.ghcb_sa);
 		break;
 	case SVM_VMGEXIT_MMIO_WRITE:
-		ret = setup_vmgexit_scratch(svm, false, control->exit_info_2);
-		if (ret)
+		if (!setup_vmgexit_scratch(svm, false, control->exit_info_2))
 			break;
 
 		ret = kvm_sev_es_mmio_write(vcpu,
@@ -2788,14 +2808,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu)
 		default:
 			pr_err("svm: vmgexit: unsupported AP jump table request - exit_info_1=%#llx\n",
 			       control->exit_info_1);
-			ghcb_set_sw_exit_info_1(ghcb, 1);
-			ghcb_set_sw_exit_info_2(ghcb,
-						X86_TRAP_UD |
-						SVM_EVTINJ_TYPE_EXEPT |
-						SVM_EVTINJ_VALID);
+			ghcb_set_sw_exit_info_1(ghcb, 2);
+			ghcb_set_sw_exit_info_2(ghcb, GHCB_ERR_INVALID_INPUT);
 		}
 
-		ret = 1;
 		break;
 	}
 	case SVM_VMGEXIT_UNSUPPORTED_EVENT:
@@ -2815,7 +2831,6 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
 {
 	int count;
 	int bytes;
-	int r;
 
 	if (svm->vmcb->control.exit_info_2 > INT_MAX)
 		return -EINVAL;
@@ -2824,9 +2839,8 @@ int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in)
 	if (unlikely(check_mul_overflow(count, size, &bytes)))
 		return -EINVAL;
 
-	r = setup_vmgexit_scratch(svm, in, bytes);
-	if (r)
-		return r;
+	if (!setup_vmgexit_scratch(svm, in, bytes))
+		return 1;
 
 	return kvm_sev_es_string_io(&svm->vcpu, size, port, svm->sev_es.ghcb_sa,
 				    count, in);
-- 
2.33.1