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

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

[Tom Lendacky]

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

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>

---

Changes from v1:
- Added the svm_complete_emulated_msr() path as suggested by Sean
  Christopherson
- Add a WARN_ON_ONCE() to the sev_vcpu_deliver_sipi_vector() path
---
 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 83e00e524513..7ac67615c070 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -2105,5 +2105,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 (WARN_ON_ONCE(!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 271196400495..534e52ba6045 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -2759,7 +2759,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(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);
-- 
2.31.0

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

[Sean Christopherson]

On Thu, Apr 08, 2021, Tom Lendacky wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> 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>
> 
> ---
> 
> Changes from v1:
> - Added the svm_complete_emulated_msr() path as suggested by Sean
>   Christopherson
> - Add a WARN_ON_ONCE() to the sev_vcpu_deliver_sipi_vector() path
> ---
>  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 83e00e524513..7ac67615c070 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))

Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
If so, this should not WARN.

> +		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 271196400495..534e52ba6045 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -2759,7 +2759,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(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);
> -- 
> 2.31.0
> 

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

[Tom Lendacky]

On 4/8/21 12:10 PM, Sean Christopherson wrote:
> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> 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>
>>
>> ---
>>
>> Changes from v1:
>> - Added the svm_complete_emulated_msr() path as suggested by Sean
>>   Christopherson
>> - Add a WARN_ON_ONCE() to the sev_vcpu_deliver_sipi_vector() path
>> ---
>>  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 83e00e524513..7ac67615c070 100644
>> --- a/arch/x86/kvm/svm/sev.c
>> +++ b/arch/x86/kvm/svm/sev.c
>> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))
> 
> Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
> If so, this should not WARN.

Yes, it is a guest triggerable event. But a guest shouldn't be doing that,
so I thought adding the WARN_ON_ONCE() just to detect it wasn't bad.
Definitely wouldn't want a WARN_ON().

Thanks,
Tom

> 
>> +		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 271196400495..534e52ba6045 100644
>> --- a/arch/x86/kvm/svm/svm.c
>> +++ b/arch/x86/kvm/svm/svm.c
>> @@ -2759,7 +2759,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(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);
>> -- 
>> 2.31.0
>>

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

[Sean Christopherson]

On Thu, Apr 08, 2021, Tom Lendacky wrote:
> On 4/8/21 12:10 PM, Sean Christopherson wrote:
> > On Thu, Apr 08, 2021, Tom Lendacky wrote:
> >> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >> index 83e00e524513..7ac67615c070 100644
> >> --- a/arch/x86/kvm/svm/sev.c
> >> +++ b/arch/x86/kvm/svm/sev.c
> >> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))
> > 
> > Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
> > If so, this should not WARN.
> 
> Yes, it is a guest triggerable event. But a guest shouldn't be doing that,
> so I thought adding the WARN_ON_ONCE() just to detect it wasn't bad.
> Definitely wouldn't want a WARN_ON().

WARNs are intended only for host issues, e.g. a malicious guest shouldn't be
able to crash the host when running with panic_on_warn.

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

[Tom Lendacky]

On 4/8/21 12:37 PM, Sean Christopherson wrote:
> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>> On 4/8/21 12:10 PM, Sean Christopherson wrote:
>>> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>> index 83e00e524513..7ac67615c070 100644
>>>> --- a/arch/x86/kvm/svm/sev.c
>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))
>>>
>>> Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
>>> If so, this should not WARN.
>>
>> Yes, it is a guest triggerable event. But a guest shouldn't be doing that,
>> so I thought adding the WARN_ON_ONCE() just to detect it wasn't bad.
>> Definitely wouldn't want a WARN_ON().
> 
> WARNs are intended only for host issues, e.g. a malicious guest shouldn't be
> able to crash the host when running with panic_on_warn.
> 

Ah, yeah, forgot about panic_on_warn. I can go back to the original patch
or do a pr_warn_once(), any pref?

Thanks,
Tom

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

[Sean Christopherson]

On Thu, Apr 08, 2021, Tom Lendacky wrote:
> 
> 
> On 4/8/21 12:37 PM, Sean Christopherson wrote:
> > On Thu, Apr 08, 2021, Tom Lendacky wrote:
> >> On 4/8/21 12:10 PM, Sean Christopherson wrote:
> >>> On Thu, Apr 08, 2021, Tom Lendacky wrote:
> >>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> >>>> index 83e00e524513..7ac67615c070 100644
> >>>> --- a/arch/x86/kvm/svm/sev.c
> >>>> +++ b/arch/x86/kvm/svm/sev.c
> >>>> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))
> >>>
> >>> Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
> >>> If so, this should not WARN.
> >>
> >> Yes, it is a guest triggerable event. But a guest shouldn't be doing that,
> >> so I thought adding the WARN_ON_ONCE() just to detect it wasn't bad.
> >> Definitely wouldn't want a WARN_ON().
> > 
> > WARNs are intended only for host issues, e.g. a malicious guest shouldn't be
> > able to crash the host when running with panic_on_warn.
> > 
> 
> Ah, yeah, forgot about panic_on_warn. I can go back to the original patch
> or do a pr_warn_once(), any pref?

No strong preference.  If you think the print would be helpful for ongoing
development, then it's probably worth adding.

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

[Tom Lendacky]

On 4/8/21 2:48 PM, Sean Christopherson wrote:
> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>>
>>
>> On 4/8/21 12:37 PM, Sean Christopherson wrote:
>>> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>>>> On 4/8/21 12:10 PM, Sean Christopherson wrote:
>>>>> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>>>>>> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
>>>>>> index 83e00e524513..7ac67615c070 100644
>>>>>> --- a/arch/x86/kvm/svm/sev.c
>>>>>> +++ b/arch/x86/kvm/svm/sev.c
>>>>>> @@ -2105,5 +2105,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 (WARN_ON_ONCE(!svm->ghcb))
>>>>>
>>>>> Isn't this guest triggerable?  I.e. send a SIPI without doing the reset hold?
>>>>> If so, this should not WARN.
>>>>
>>>> Yes, it is a guest triggerable event. But a guest shouldn't be doing that,
>>>> so I thought adding the WARN_ON_ONCE() just to detect it wasn't bad.
>>>> Definitely wouldn't want a WARN_ON().
>>>
>>> WARNs are intended only for host issues, e.g. a malicious guest shouldn't be
>>> able to crash the host when running with panic_on_warn.
>>>
>>
>> Ah, yeah, forgot about panic_on_warn. I can go back to the original patch
>> or do a pr_warn_once(), any pref?
> 
> No strong preference.  If you think the print would be helpful for ongoing
> development, then it's probably worth adding.

For development, I'd want to see it all the time. But since it is guest
triggerable, the _once() method is really needed in production. So in the
latest version I just dropped the message/notification.

Thanks,
Tom

>