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