Terminate the VM if userspace attempts to run an SEV-SNP (or -ES) vCPU
that has an invalid VMSA. With SNP's AP Create/Destroy hypercalls, it's
possible for an SNP vCPU to end up with an invalid VMSA, e.g. through a
deliberate Destroy or a failed Create event. KVM marks the vCPU HALTED so
that *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM
from manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
Cc: stable@vger.kernel.org
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
arch/x86/kvm/svm/sev.c | 18 +++++++++++++++---
arch/x86/kvm/svm/svm.c | 7 +++++--
arch/x86/kvm/svm/svm.h | 2 +-
3 files changed, 21 insertions(+), 6 deletions(-)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 6c6d45e13858..e14a37dbc6ea 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3452,10 +3452,21 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
svm->sev_es.ghcb = NULL;
}
-void pre_sev_run(struct vcpu_svm *svm, int cpu)
+int pre_sev_run(struct vcpu_svm *svm, int cpu)
{
struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
- unsigned int asid = sev_get_asid(svm->vcpu.kvm);
+ struct kvm *kvm = svm->vcpu.kvm;
+ unsigned int asid = sev_get_asid(kvm);
+
+ /*
+ * Terminate the VM if userspace attempts to run the vCPU with an
+ * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
+ * an SNP AP Destroy event.
+ */
+ if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
+ kvm_vm_dead(kvm);
+ return -EIO;
+ }
/* Assign the asid allocated with this SEV guest */
svm->asid = asid;
@@ -3468,11 +3479,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
*/
if (sd->sev_vmcbs[asid] == svm->vmcb &&
svm->vcpu.arch.last_vmentry_cpu == cpu)
- return;
+ return 0;
sd->sev_vmcbs[asid] = svm->vmcb;
svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
+ return 0;
}
#define GHCB_SCRATCH_AREA_LIMIT (16ULL * PAGE_SIZE)
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index b8aa0f36850f..46e0b65a9fec 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
return svm_invoke_exit_handler(vcpu, exit_code);
}
-static void pre_svm_run(struct kvm_vcpu *vcpu)
+static int pre_svm_run(struct kvm_vcpu *vcpu)
{
struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
struct vcpu_svm *svm = to_svm(vcpu);
@@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
/* FIXME: handle wraparound of asid_generation */
if (svm->current_vmcb->asid_generation != sd->asid_generation)
new_asid(svm, sd);
+
+ return 0;
}
static void svm_inject_nmi(struct kvm_vcpu *vcpu)
@@ -4231,7 +4233,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
if (force_immediate_exit)
smp_send_reschedule(vcpu->cpu);
- pre_svm_run(vcpu);
+ if (pre_svm_run(vcpu))
+ return EXIT_FASTPATH_EXIT_USERSPACE;
sync_lapic_to_cr8(vcpu);
diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
index 5b159f017055..e51852977b70 100644
--- a/arch/x86/kvm/svm/svm.h
+++ b/arch/x86/kvm/svm/svm.h
@@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
/* sev.c */
-void pre_sev_run(struct vcpu_svm *svm, int cpu);
+int pre_sev_run(struct vcpu_svm *svm, int cpu);
void sev_init_vmcb(struct vcpu_svm *svm);
void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);
--
2.48.1.601.g30ceb7b040-googOn 2/18/25 19:26, Sean Christopherson wrote:
> Terminate the VM if userspace attempts to run an SEV-SNP (or -ES) vCPU
> that has an invalid VMSA. With SNP's AP Create/Destroy hypercalls, it's
> possible for an SNP vCPU to end up with an invalid VMSA, e.g. through a
> deliberate Destroy or a failed Create event. KVM marks the vCPU HALTED so
> that *KVM* doesn't run the vCPU, but nothing prevents a misbehaving VMM
> from manually making the vCPU RUNNABLE via KVM_SET_MP_STATE.
>
> Fixes: e366f92ea99e ("KVM: SEV: Support SEV-SNP AP Creation NAE event")
> Cc: stable@vger.kernel.org
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/svm/sev.c | 18 +++++++++++++++---
> arch/x86/kvm/svm/svm.c | 7 +++++--
> arch/x86/kvm/svm/svm.h | 2 +-
> 3 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 6c6d45e13858..e14a37dbc6ea 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3452,10 +3452,21 @@ void sev_es_unmap_ghcb(struct vcpu_svm *svm)
> svm->sev_es.ghcb = NULL;
> }
>
> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
> {
> struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> - unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> + struct kvm *kvm = svm->vcpu.kvm;
> + unsigned int asid = sev_get_asid(kvm);
> +
> + /*
> + * Terminate the VM if userspace attempts to run the vCPU with an
> + * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
> + * an SNP AP Destroy event.
> + */
> + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> + kvm_vm_dead(kvm);
> + return -EIO;
> + }
If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
VMRUN will fail and KVM will dump the VMCB and exit back to userspace
with KVM_EXIT_INTERNAL_ERROR.
Is doing this preferrable to that? If so, should a vcpu_unimpl() message
be issued, too, to better identify the reason for marking the VM dead?
>
> /* Assign the asid allocated with this SEV guest */
> svm->asid = asid;
> @@ -3468,11 +3479,12 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu)
> */
> if (sd->sev_vmcbs[asid] == svm->vmcb &&
> svm->vcpu.arch.last_vmentry_cpu == cpu)
> - return;
> + return 0;
>
> sd->sev_vmcbs[asid] = svm->vmcb;
> svm->vmcb->control.tlb_ctl = TLB_CONTROL_FLUSH_ASID;
> vmcb_mark_dirty(svm->vmcb, VMCB_ASID);
> + return 0;
> }
>
> #define GHCB_SCRATCH_AREA_LIMIT (16ULL * PAGE_SIZE)
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index b8aa0f36850f..46e0b65a9fec 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -3587,7 +3587,7 @@ static int svm_handle_exit(struct kvm_vcpu *vcpu, fastpath_t exit_fastpath)
> return svm_invoke_exit_handler(vcpu, exit_code);
> }
>
> -static void pre_svm_run(struct kvm_vcpu *vcpu)
> +static int pre_svm_run(struct kvm_vcpu *vcpu)
> {
> struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, vcpu->cpu);
> struct vcpu_svm *svm = to_svm(vcpu);
> @@ -3609,6 +3609,8 @@ static void pre_svm_run(struct kvm_vcpu *vcpu)
> /* FIXME: handle wraparound of asid_generation */
> if (svm->current_vmcb->asid_generation != sd->asid_generation)
> new_asid(svm, sd);
> +
> + return 0;
> }
>
> static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> @@ -4231,7 +4233,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> if (force_immediate_exit)
> smp_send_reschedule(vcpu->cpu);
>
> - pre_svm_run(vcpu);
> + if (pre_svm_run(vcpu))
> + return EXIT_FASTPATH_EXIT_USERSPACE;
Since the return code from pre_svm_run() is never used, should it just
be a bool function, then?
Thanks,
Tom
>
> sync_lapic_to_cr8(vcpu);
>
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 5b159f017055..e51852977b70 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -713,7 +713,7 @@ void avic_refresh_virtual_apic_mode(struct kvm_vcpu *vcpu);
>
> /* sev.c */
>
> -void pre_sev_run(struct vcpu_svm *svm, int cpu);
> +int pre_sev_run(struct vcpu_svm *svm, int cpu);
> void sev_init_vmcb(struct vcpu_svm *svm);
> void sev_vcpu_after_set_cpuid(struct vcpu_svm *svm);
> int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in);
On Mon, Feb 24, 2025, Tom Lendacky wrote:
> On 2/18/25 19:26, Sean Christopherson wrote:
> > -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> > +int pre_sev_run(struct vcpu_svm *svm, int cpu)
> > {
> > struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> > - unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> > + struct kvm *kvm = svm->vcpu.kvm;
> > + unsigned int asid = sev_get_asid(kvm);
> > +
> > + /*
> > + * Terminate the VM if userspace attempts to run the vCPU with an
> > + * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
> > + * an SNP AP Destroy event.
> > + */
> > + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> > + kvm_vm_dead(kvm);
> > + return -EIO;
> > + }
>
> If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
> VMRUN will fail and KVM will dump the VMCB and exit back to userspace
I haven't tested, but based on what the APM says, I'm pretty sure this would crash
the host due to a #GP on VMRUN, i.e. due to the resulting kvm_spurious_fault().
IF (rAX contains an unsupported physical address)
EXCEPTION [#GP]
> with KVM_EXIT_INTERNAL_ERROR.
>
> Is doing this preferrable to that?
Even if AMD guaranteed that the absolute worst case scenario is a failed VMRUN
with zero side effects, doing VMRUN with a bad address should be treated as a
KVM bug.
> If so, should a vcpu_unimpl() message be issued, too, to better identify the
> reason for marking the VM dead?
My vote is no. At some point we need to assume userspace possesess a reasonable
level of competency and sanity.
> > static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> > @@ -4231,7 +4233,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> > if (force_immediate_exit)
> > smp_send_reschedule(vcpu->cpu);
> >
> > - pre_svm_run(vcpu);
> > + if (pre_svm_run(vcpu))
> > + return EXIT_FASTPATH_EXIT_USERSPACE;
>
> Since the return code from pre_svm_run() is never used, should it just
> be a bool function, then?
Hard no. I strongly dislike boolean returns for functions that aren't obviously
predicates, because it's impossible to determine the polarity of the return value
based solely on the prototype. This leads to bugs that are easier to detect with
0/-errno return, e.g. returning -EINVAL in a happy path stands out more than
returning the wrong false/true value.
Case in point (because I just responded to another emain about this function),
what's the polarity of this helper? :-)
static bool sanity_check_entries(struct kvm_cpuid_entry2 __user *entries,
__u32 num_entries, unsigned int ioctl_type)
On 2/24/25 16:55, Sean Christopherson wrote:
> On Mon, Feb 24, 2025, Tom Lendacky wrote:
>> On 2/18/25 19:26, Sean Christopherson wrote:
>>> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
>>> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
>>> {
>>> struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
>>> - unsigned int asid = sev_get_asid(svm->vcpu.kvm);
>>> + struct kvm *kvm = svm->vcpu.kvm;
>>> + unsigned int asid = sev_get_asid(kvm);
>>> +
>>> + /*
>>> + * Terminate the VM if userspace attempts to run the vCPU with an
>>> + * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
>>> + * an SNP AP Destroy event.
>>> + */
>>> + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
>>> + kvm_vm_dead(kvm);
>>> + return -EIO;
>>> + }
>>
>> If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
>> VMRUN will fail and KVM will dump the VMCB and exit back to userspace
>
> I haven't tested, but based on what the APM says, I'm pretty sure this would crash
> the host due to a #GP on VMRUN, i.e. due to the resulting kvm_spurious_fault().
>
> IF (rAX contains an unsupported physical address)
> EXCEPTION [#GP]
Well that's for the VMCB, the VMSA is pointed to by the VMCB and results
in a VMEXIT code of -1 if you don't supply a proper page-aligned,
physical address.
>
>> with KVM_EXIT_INTERNAL_ERROR.
>>
>> Is doing this preferrable to that?
>
> Even if AMD guaranteed that the absolute worst case scenario is a failed VMRUN
> with zero side effects, doing VMRUN with a bad address should be treated as a
> KVM bug.
Fair.
>
>> If so, should a vcpu_unimpl() message be issued, too, to better identify the
>> reason for marking the VM dead?
>
> My vote is no. At some point we need to assume userspace possesess a reasonable
> level of competency and sanity.
>
>>> static void svm_inject_nmi(struct kvm_vcpu *vcpu)
>>> @@ -4231,7 +4233,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
>>> if (force_immediate_exit)
>>> smp_send_reschedule(vcpu->cpu);
>>>
>>> - pre_svm_run(vcpu);
>>> + if (pre_svm_run(vcpu))
>>> + return EXIT_FASTPATH_EXIT_USERSPACE;
In testing this out, I think userspace continues on because I eventually
get:
KVM_GET_PIT2 failed: Input/output error
/tmp/cmdline.98112: line 1: 98163 Aborted (core dumped) ...
Haven't looked too close, but maybe an exit_reason needs to be set to
get qemu to quit sooner?
Thanks,
Tom
>>
>> Since the return code from pre_svm_run() is never used, should it just
>> be a bool function, then?
>
> Hard no. I strongly dislike boolean returns for functions that aren't obviously
> predicates, because it's impossible to determine the polarity of the return value
> based solely on the prototype. This leads to bugs that are easier to detect with
> 0/-errno return, e.g. returning -EINVAL in a happy path stands out more than
> returning the wrong false/true value.
>
> Case in point (because I just responded to another emain about this function),
> what's the polarity of this helper? :-)
>
> static bool sanity_check_entries(struct kvm_cpuid_entry2 __user *entries,
> __u32 num_entries, unsigned int ioctl_type)
On Mon, Feb 24, 2025, Tom Lendacky wrote:
> On 2/24/25 16:55, Sean Christopherson wrote:
> > On Mon, Feb 24, 2025, Tom Lendacky wrote:
> >> On 2/18/25 19:26, Sean Christopherson wrote:
> >>> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> >>> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
> >>> {
> >>> struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> >>> - unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> >>> + struct kvm *kvm = svm->vcpu.kvm;
> >>> + unsigned int asid = sev_get_asid(kvm);
> >>> +
> >>> + /*
> >>> + * Terminate the VM if userspace attempts to run the vCPU with an
> >>> + * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
> >>> + * an SNP AP Destroy event.
> >>> + */
> >>> + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> >>> + kvm_vm_dead(kvm);
> >>> + return -EIO;
> >>> + }
> >>
> >> If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
> >> VMRUN will fail and KVM will dump the VMCB and exit back to userspace
> >
> > I haven't tested, but based on what the APM says, I'm pretty sure this would crash
> > the host due to a #GP on VMRUN, i.e. due to the resulting kvm_spurious_fault().
> >
> > IF (rAX contains an unsupported physical address)
> > EXCEPTION [#GP]
>
> Well that's for the VMCB, the VMSA is pointed to by the VMCB and results
> in a VMEXIT code of -1 if you don't supply a proper page-aligned,
> physical address.
Ah, good to know (and somewhat of a relief :-) ).
> >>> static void svm_inject_nmi(struct kvm_vcpu *vcpu)
> >>> @@ -4231,7 +4233,8 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> >>> if (force_immediate_exit)
> >>> smp_send_reschedule(vcpu->cpu);
> >>>
> >>> - pre_svm_run(vcpu);
> >>> + if (pre_svm_run(vcpu))
> >>> + return EXIT_FASTPATH_EXIT_USERSPACE;
>
> In testing this out, I think userspace continues on because I eventually
> get:
>
> KVM_GET_PIT2 failed: Input/output error
> /tmp/cmdline.98112: line 1: 98163 Aborted (core dumped) ...
>
> Haven't looked too close, but maybe an exit_reason needs to be set to
> get qemu to quit sooner?
Oh, the irony. In trying to do the right thing (exit to userspace), I managed to
do the wrong thing.
If KVM tried to re-enter the guest, vcpu_enter_guest() would have encountered
the KVM_REQ_DEAD and exited with -EIO.
if (kvm_check_request(KVM_REQ_VM_DEAD, vcpu)) {
r = -EIO;
goto out;
}
By returning EXIT_FASTPATH_EXIT_USERSPACE, KVM exited to userspace more directly
and returned '0' instead of -EIO.
Getting KVM to return -EIO is easy, but doing so feels wrong, especially if we
take the quick-and-dirty route like so:
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 454fd6b8f3db..9c8b400e04f2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -11102,7 +11102,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu)
kvm_lapic_sync_from_vapic(vcpu);
if (unlikely(exit_fastpath == EXIT_FASTPATH_EXIT_USERSPACE))
- return 0;
+ return kvm_test_request(KVM_REQ_VM_DEAD, vcpu) ? -EIO : 0;
r = kvm_x86_call(handle_exit)(vcpu, exit_fastpath);
return r;
Given that, IIUC, KVM would eventually return KVM_EXIT_FAIL_ENTRY, I like your
idea of returning meaningful information. And unless I'm missing something, that
would obviate any need to terminate the VM, which would address your earlier point
of whether terminating the VM is truly better than returning than returning a
familiar error code.
So this? (completely untested)
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 7345cac6f93a..71b340cbe561 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3463,10 +3463,8 @@ int pre_sev_run(struct vcpu_svm *svm, int cpu)
* invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
* an SNP AP Destroy event.
*/
- if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
- kvm_vm_dead(kvm);
- return -EIO;
- }
+ if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
+ return -EINVAL;
/* Assign the asid allocated with this SEV guest */
svm->asid = asid;
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 46e0b65a9fec..f72bcf2e590e 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4233,8 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
if (force_immediate_exit)
smp_send_reschedule(vcpu->cpu);
- if (pre_svm_run(vcpu))
+ if (pre_svm_run(vcpu)) {
+ vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
+ vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
+ vcpu->run->fail_entry.cpu = vcpu->cpu;
return EXIT_FASTPATH_EXIT_USERSPACE;
+ }
sync_lapic_to_cr8(vcpu);
On 2/24/25 18:54, Sean Christopherson wrote:
> On Mon, Feb 24, 2025, Tom Lendacky wrote:
>> On 2/24/25 16:55, Sean Christopherson wrote:
>>> On Mon, Feb 24, 2025, Tom Lendacky wrote:
>>>> On 2/18/25 19:26, Sean Christopherson wrote:
>
> Given that, IIUC, KVM would eventually return KVM_EXIT_FAIL_ENTRY, I like your
> idea of returning meaningful information. And unless I'm missing something, that
> would obviate any need to terminate the VM, which would address your earlier point
> of whether terminating the VM is truly better than returning than returning a
> familiar error code.
>
> So this? (completely untested)
This works nicely and qemu terminates quickly with:
KVM: entry failed, hardware error 0xffffffffffffffff
Thanks,
Tom
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 7345cac6f93a..71b340cbe561 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3463,10 +3463,8 @@ int pre_sev_run(struct vcpu_svm *svm, int cpu)
> * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
> * an SNP AP Destroy event.
> */
> - if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> - kvm_vm_dead(kvm);
> - return -EIO;
> - }
> + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa))
> + return -EINVAL;
>
> /* Assign the asid allocated with this SEV guest */
> svm->asid = asid;
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 46e0b65a9fec..f72bcf2e590e 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4233,8 +4233,12 @@ static __no_kcsan fastpath_t svm_vcpu_run(struct kvm_vcpu *vcpu,
> if (force_immediate_exit)
> smp_send_reschedule(vcpu->cpu);
>
> - if (pre_svm_run(vcpu))
> + if (pre_svm_run(vcpu)) {
> + vcpu->run->exit_reason = KVM_EXIT_FAIL_ENTRY;
> + vcpu->run->fail_entry.hardware_entry_failure_reason = SVM_EXIT_ERR;
> + vcpu->run->fail_entry.cpu = vcpu->cpu;
> return EXIT_FASTPATH_EXIT_USERSPACE;
> + }
>
> sync_lapic_to_cr8(vcpu);
On Mon, Feb 24, 2025, Sean Christopherson wrote:
> On Mon, Feb 24, 2025, Tom Lendacky wrote:
> > On 2/24/25 16:55, Sean Christopherson wrote:
> > > On Mon, Feb 24, 2025, Tom Lendacky wrote:
> > >> On 2/18/25 19:26, Sean Christopherson wrote:
> > >>> -void pre_sev_run(struct vcpu_svm *svm, int cpu)
> > >>> +int pre_sev_run(struct vcpu_svm *svm, int cpu)
> > >>> {
> > >>> struct svm_cpu_data *sd = per_cpu_ptr(&svm_data, cpu);
> > >>> - unsigned int asid = sev_get_asid(svm->vcpu.kvm);
> > >>> + struct kvm *kvm = svm->vcpu.kvm;
> > >>> + unsigned int asid = sev_get_asid(kvm);
> > >>> +
> > >>> + /*
> > >>> + * Terminate the VM if userspace attempts to run the vCPU with an
> > >>> + * invalid VMSA, e.g. if userspace forces the vCPU to be RUNNABLE after
> > >>> + * an SNP AP Destroy event.
> > >>> + */
> > >>> + if (sev_es_guest(kvm) && !VALID_PAGE(svm->vmcb->control.vmsa_pa)) {
> > >>> + kvm_vm_dead(kvm);
> > >>> + return -EIO;
> > >>> + }
> > >>
> > >> If a VMRUN is performed with the vmsa_pa value set to INVALID_PAGE, the
> > >> VMRUN will fail and KVM will dump the VMCB and exit back to userspace
> > >
> > > I haven't tested, but based on what the APM says, I'm pretty sure this would crash
> > > the host due to a #GP on VMRUN, i.e. due to the resulting kvm_spurious_fault().
> > >
> > > IF (rAX contains an unsupported physical address)
> > > EXCEPTION [#GP]
> >
> > Well that's for the VMCB, the VMSA is pointed to by the VMCB and results
> > in a VMEXIT code of -1 if you don't supply a proper page-aligned,
> > physical address.
>
> Ah, good to know (and somewhat of a relief :-) ).
If anyone else was wondering, the behavior is described in the "VMRUN Page Checks"
table, which "Table 15-4" in the March 2024 version of the APM.
FWIW, knowing that VMRUN is supposed to handle this scenario does make me somewhat
tempted to skip this patch entirely. But I still don't like the idea of doing
VMRUN with a known bad address.
© 2016 - 2025 Red Hat, Inc.