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 >
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
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
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
>
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
> >>
> >> 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.
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
>>
>> 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
>
> Either way:
>
> Reviewed-by: Sean Christopherson
>
>> ---
>> 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
>>
Re: [PATCH] KVM: SVM: Make sure GHCB is mapped before updating
On Wed, Apr 07, 2021, Tom Lendacky wrote:
> From: Tom Lendacky
>
> 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
Either way:
Reviewed-by: Sean Christopherson
> ---
> 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
>
[PATCH] KVM: SVM: Make sure GHCB is mapped before updating
From: Tom Lendacky
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
---
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
