| Message ID | 1ed85188bee4a602ffad9632cdf5b5b5c0f40957.1617900892.git.thomas.lendacky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] KVM: SVM: Make sure GHCB is mapped before updating | expand |
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 >
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 >>
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.
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
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.
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 >
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);