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

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

[Tom Lendacky]

From: Tom Lendacky <thomas.lendacky@amd.com>

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 <thomas.lendacky@amd.com>
---
 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

[Sean Christopherson]

On Wed, Apr 07, 2021, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> 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 <thomas.lendacky@amd.com>

Either way:

Reviewed-by: Sean Christopherson <seanjc@google.com> 

> ---
>  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

[Tom Lendacky]

On 4/7/21 3:08 PM, Sean Christopherson wrote:
> On Wed, Apr 07, 2021, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> 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 <thomas.lendacky@amd.com>
> 
> Either way:
> 
> Reviewed-by: Sean Christopherson <seanjc@google.com> 
> 
>> ---
>>  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

[Sean Christopherson]

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 <thomas.lendacky@amd.com>
> >>
> >> 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

[Tom Lendacky]

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 <thomas.lendacky@amd.com>
>>>>
>>>> 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

[Paolo Bonzini]

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

[Tom Lendacky]

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
> 

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

[Tom Lendacky]

commit a3ba26ecfb569f4aa3f867e80c02aa65f20aadad upstream.

Access to the GHCB is mainly in the VMGEXIT path and it is known that the
GHCB will be mapped. But there are two paths where it is possible the GHCB
might not be mapped.

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 might not be mapped (depending on the previous VMEXIT),
which will result in a NULL pointer dereference.

The svm_complete_emulated_msr() routine will update the GHCB to inform
the caller of a RDMSR/WRMSR operation about any errors. While it is likely
that the GHCB will be mapped in this situation, add a safe guard
in this path to be certain a NULL pointer dereference is not encountered.

Fixes: f1c6366e3043 ("KVM: SVM: Add required changes to support intercepts under SEV-ES")
Fixes: 647daca25d24 ("KVM: SVM: Add support for booting APs in an SEV-ES guest")
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
Cc: stable@vger.kernel.org
Message-Id: <a5d3ebb600a91170fc88599d5a575452b3e31036.1617979121.git.thomas.lendacky@amd.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/svm/sev.c | 3 +++
 arch/x86/kvm/svm/svm.c | 2 +-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 874ea309279f..d6cf74d3b3b2 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2104,5 +2104,8 @@ 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.
 	 */
+	if (!svm->ghcb)
+		return;
+
 	ghcb_set_sw_exit_info_2(svm->ghcb, 1);
 }
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 58a45bb139f8..2b6e7874c491 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2809,7 +2809,7 @@ 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(svm->vcpu.kvm) || !err)
+	if (!err || !sev_es_guest(vcpu->kvm) || WARN_ON_ONCE(!svm->ghcb))
 		return kvm_complete_insn_gp(&svm->vcpu, err);
 
 	ghcb_set_sw_exit_info_1(svm->ghcb, 1);
-- 
2.31.0