2021-04-07 21:53:39

by Tom Lendacky

[permalink] [raw]
Subject: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

From: Tom Lendacky <[email protected]>

The sev_vcpu_deliver_sipi_vector() routine will update the GHCB to inform
the caller of the AP Reset Hold NAE event that a SIPI has been delivered.
However, if a SIPI is performed without a corresponding AP Reset Hold,
then the GHCB may not be mapped, which will result in a NULL pointer
dereference.

Check that the GHCB is mapped before attempting the update.

Fixes: 647daca25d24 ("KVM: SVM: Add support for booting APs in an SEV-ES guest")
Signed-off-by: Tom Lendacky <[email protected]>
---
arch/x86/kvm/svm/sev.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 83e00e524513..13758e3b106d 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2105,5 +2105,6 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
* the guest will set the CS and RIP. Set SW_EXIT_INFO_2 to a
* non-zero value.
*/
- ghcb_set_sw_exit_info_2(svm->ghcb, 1);
+ if (svm->ghcb)
+ ghcb_set_sw_exit_info_2(svm->ghcb, 1);
}
--
2.31.0


2021-04-07 22:01:08

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

On Wed, Apr 07, 2021, Tom Lendacky wrote:
> From: Tom Lendacky <[email protected]>
>
> The sev_vcpu_deliver_sipi_vector() routine will update the GHCB to inform
> the caller of the AP Reset Hold NAE event that a SIPI has been delivered.
> However, if a SIPI is performed without a corresponding AP Reset Hold,
> then the GHCB may not be mapped, which will result in a NULL pointer
> dereference.
>
> Check that the GHCB is mapped before attempting the update.

It's tempting to say the ghcb_set_*() helpers should guard against this, but
that would add a lot of pollution and the vast majority of uses are very clearly
in the vmgexit path. svm_complete_emulated_msr() is the only other case that
is non-obvious; would it make sense to sanity check svm->ghcb there as well?

diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 019ac836dcd0..abe9c765628f 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2728,7 +2728,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
{
struct vcpu_svm *svm = to_svm(vcpu);
- if (!sev_es_guest(vcpu->kvm) || !err)
+
+ if (!err || !sev_es_guest(vcpu->kvm) || !WARN_ON_ONCE(svm->ghcb))
return kvm_complete_insn_gp(vcpu, err);

ghcb_set_sw_exit_info_1(svm->ghcb, 1);

> Fixes: 647daca25d24 ("KVM: SVM: Add support for booting APs in an SEV-ES guest")
> Signed-off-by: Tom Lendacky <[email protected]>

Either way:

Reviewed-by: Sean Christopherson <[email protected]>

> ---
> arch/x86/kvm/svm/sev.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 83e00e524513..13758e3b106d 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2105,5 +2105,6 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
> * the guest will set the CS and RIP. Set SW_EXIT_INFO_2 to a
> * non-zero value.
> */
> - ghcb_set_sw_exit_info_2(svm->ghcb, 1);
> + if (svm->ghcb)
> + ghcb_set_sw_exit_info_2(svm->ghcb, 1);
> }
> --
> 2.31.0
>

2021-04-07 22:04:59

by Sean Christopherson

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

On Wed, Apr 07, 2021, Tom Lendacky wrote:
> On 4/7/21 3:08 PM, Sean Christopherson wrote:
> > On Wed, Apr 07, 2021, Tom Lendacky wrote:
> >> From: Tom Lendacky <[email protected]>
> >>
> >> The sev_vcpu_deliver_sipi_vector() routine will update the GHCB to inform
> >> the caller of the AP Reset Hold NAE event that a SIPI has been delivered.
> >> However, if a SIPI is performed without a corresponding AP Reset Hold,
> >> then the GHCB may not be mapped, which will result in a NULL pointer
> >> dereference.
> >>
> >> Check that the GHCB is mapped before attempting the update.
> >
> > It's tempting to say the ghcb_set_*() helpers should guard against this, but
> > that would add a lot of pollution and the vast majority of uses are very clearly
> > in the vmgexit path. svm_complete_emulated_msr() is the only other case that
> > is non-obvious; would it make sense to sanity check svm->ghcb there as well?
>
> Hmm... I'm not sure if we can get here without having taken the VMGEXIT
> path to start, but it certainly couldn't hurt to add it.

Yeah, AFAICT it should be impossible to reach the callback without a valid ghcb,
it'd be purely be a sanity check.

> I can submit a v2 with that unless you want to submit it (with one small
> change below).

I'd say just throw it into v2.

> > diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> > index 019ac836dcd0..abe9c765628f 100644
> > --- a/arch/x86/kvm/svm/svm.c
> > +++ b/arch/x86/kvm/svm/svm.c
> > @@ -2728,7 +2728,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> > static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
> > {
> > struct vcpu_svm *svm = to_svm(vcpu);
> > - if (!sev_es_guest(vcpu->kvm) || !err)
> > +
> > + if (!err || !sev_es_guest(vcpu->kvm) || !WARN_ON_ONCE(svm->ghcb))
>
> This should be WARN_ON_ONCE(!svm->ghcb), otherwise you'll get the right
> result, but get a stack trace immediately.

Doh, yep.

2021-04-07 22:05:04

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

On 4/7/21 3:08 PM, Sean Christopherson wrote:
> On Wed, Apr 07, 2021, Tom Lendacky wrote:
>> From: Tom Lendacky <[email protected]>
>>
>> The sev_vcpu_deliver_sipi_vector() routine will update the GHCB to inform
>> the caller of the AP Reset Hold NAE event that a SIPI has been delivered.
>> However, if a SIPI is performed without a corresponding AP Reset Hold,
>> then the GHCB may not be mapped, which will result in a NULL pointer
>> dereference.
>>
>> Check that the GHCB is mapped before attempting the update.
>
> It's tempting to say the ghcb_set_*() helpers should guard against this, but
> that would add a lot of pollution and the vast majority of uses are very clearly
> in the vmgexit path. svm_complete_emulated_msr() is the only other case that
> is non-obvious; would it make sense to sanity check svm->ghcb there as well?

Hmm... I'm not sure if we can get here without having taken the VMGEXIT
path to start, but it certainly couldn't hurt to add it.

I can submit a v2 with that unless you want to submit it (with one small
change below).

>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 019ac836dcd0..abe9c765628f 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2728,7 +2728,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
> {
> struct vcpu_svm *svm = to_svm(vcpu);
> - if (!sev_es_guest(vcpu->kvm) || !err)
> +
> + if (!err || !sev_es_guest(vcpu->kvm) || !WARN_ON_ONCE(svm->ghcb))

This should be WARN_ON_ONCE(!svm->ghcb), otherwise you'll get the right
result, but get a stack trace immediately.

Thanks,
Tom

> return kvm_complete_insn_gp(vcpu, err);
>
> ghcb_set_sw_exit_info_1(svm->ghcb, 1);
>
>> Fixes: 647daca25d24 ("KVM: SVM: Add support for booting APs in an SEV-ES guest")
>> Signed-off-by: Tom Lendacky <[email protected]>
>
> Either way:
>
> Reviewed-by: Sean Christopherson <[email protected]>
>
>> ---
>> arch/x86/kvm/svm/sev.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>> index 83e00e524513..13758e3b106d 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2105,5 +2105,6 @@ void sev_vcpu_deliver_sipi_vector(struct kvm_vcpu *vcpu, u8 vector)
>> * the guest will set the CS and RIP. Set SW_EXIT_INFO_2 to a
>> * non-zero value.
>> */
>> - ghcb_set_sw_exit_info_2(svm->ghcb, 1);
>> + if (svm->ghcb)
>> + ghcb_set_sw_exit_info_2(svm->ghcb, 1);
>> }
>> --
>> 2.31.0
>>

2021-04-08 16:06:41

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating



On 4/7/21 4:07 PM, Sean Christopherson wrote:
> On Wed, Apr 07, 2021, Tom Lendacky wrote:
>> On 4/7/21 3:08 PM, Sean Christopherson wrote:
>>> On Wed, Apr 07, 2021, Tom Lendacky wrote:
>>>> From: Tom Lendacky <[email protected]>
>>>>
>>>> The sev_vcpu_deliver_sipi_vector() routine will update the GHCB to inform
>>>> the caller of the AP Reset Hold NAE event that a SIPI has been delivered.
>>>> However, if a SIPI is performed without a corresponding AP Reset Hold,
>>>> then the GHCB may not be mapped, which will result in a NULL pointer
>>>> dereference.
>>>>
>>>> Check that the GHCB is mapped before attempting the update.
>>>
>>> It's tempting to say the ghcb_set_*() helpers should guard against this, but
>>> that would add a lot of pollution and the vast majority of uses are very clearly
>>> in the vmgexit path. svm_complete_emulated_msr() is the only other case that
>>> is non-obvious; would it make sense to sanity check svm->ghcb there as well?
>>
>> Hmm... I'm not sure if we can get here without having taken the VMGEXIT
>> path to start, but it certainly couldn't hurt to add it.
>
> Yeah, AFAICT it should be impossible to reach the callback without a valid ghcb,
> it'd be purely be a sanity check.
>
>> I can submit a v2 with that unless you want to submit it (with one small
>> change below).
>
> I'd say just throw it into v2.
>
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 019ac836dcd0..abe9c765628f 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -2728,7 +2728,8 @@ static int svm_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
>>> static int svm_complete_emulated_msr(struct kvm_vcpu *vcpu, int err)
>>> {
>>> struct vcpu_svm *svm = to_svm(vcpu);
>>> - if (!sev_es_guest(vcpu->kvm) || !err)
>>> +
>>> + if (!err || !sev_es_guest(vcpu->kvm) || !WARN_ON_ONCE(svm->ghcb))
>>
>> This should be WARN_ON_ONCE(!svm->ghcb), otherwise you'll get the right
>> result, but get a stack trace immediately.
>
> Doh, yep.

Actually, because of the "or's", this needs to be:

if (!err || !sev_es_guest(vcpu->kvm) || (sev_es_guest(vcpu->kvm) && WARN_ON_ONCE(!svm->ghcb)))

Thanks,
Tom

>

2021-04-08 16:15:24

by Paolo Bonzini

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

On 08/04/21 18:04, Tom Lendacky wrote:
>>>> + if (!err || !sev_es_guest(vcpu->kvm) || !WARN_ON_ONCE(svm->ghcb))
>>> This should be WARN_ON_ONCE(!svm->ghcb), otherwise you'll get the right
>>> result, but get a stack trace immediately.
>> Doh, yep.
> Actually, because of the "or's", this needs to be:
>
> if (!err || !sev_es_guest(vcpu->kvm) || (sev_es_guest(vcpu->kvm) && WARN_ON_ONCE(!svm->ghcb)))

No, || cuts the right-hand side if the left-hand side is true. So:

- if err == 0, the rest is not evaluated

- if !sev_es_guest(vcpu->kvm), WARN_ON_ONCE(!svm->ghcb) is not evaluated

Paolo

2021-04-08 16:22:57

by Tom Lendacky

[permalink] [raw]
Subject: Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating

On 4/8/21 11:14 AM, Paolo Bonzini wrote:
> On 08/04/21 18:04, Tom Lendacky wrote:
>>>>> +       if (!err || !sev_es_guest(vcpu->kvm) ||
>>>>> !WARN_ON_ONCE(svm->ghcb))
>>>> This should be WARN_ON_ONCE(!svm->ghcb), otherwise you'll get the right
>>>> result, but get a stack trace immediately.
>>> Doh, yep.
>> Actually, because of the "or's", this needs to be:
>>
>> if (!err || !sev_es_guest(vcpu->kvm) || (sev_es_guest(vcpu->kvm) &&
>> WARN_ON_ONCE(!svm->ghcb)))
>
> No, || cuts the right-hand side if the left-hand side is true.  So:
>
> - if err == 0, the rest is not evaluated
>
> - if !sev_es_guest(vcpu->kvm), WARN_ON_ONCE(!svm->ghcb) is not evaluated

That's what I was doing in my head, but I guess I need more coffee... :)

Thanks,
Tom

>
> Paolo
>