Re: [PATCH v2] KVM: SVM: Make sure GHCB is mapped before updating
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 >
Re: [PATCH v2] KVM: SVM: Make sure GHCB is mapped before updating
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
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
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
On 4/8/21 12:10 PM, Sean Christopherson wrote:
> On Thu, Apr 08, 2021, Tom Lendacky wrote:
>> From: Tom Lendacky
>>
>> 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
>>
>> ---
>>
>> 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
On Thu, Apr 08, 2021, Tom Lendacky wrote:
> From: Tom Lendacky
>
> 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
>
> ---
>
> 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
>
