| Message ID | 20210106105001.449974-6-mlevitsk@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: nSVM: few random fixes | expand |
On Wed, Jan 06, 2021, Maxim Levitsky wrote: > This should prevent bad things from happening if the user calls the > KVM_SET_NESTED_STATE twice. This doesn't exactly inspire confidence, nor does it provide much help to readers that don't already know why KVM should "leave nested" before processing the rest of kvm_state. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > arch/x86/kvm/svm/nested.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > index c1a3d0e996add..3aa18016832d0 100644 > --- a/arch/x86/kvm/svm/nested.c > +++ b/arch/x86/kvm/svm/nested.c > @@ -1154,8 +1154,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > if (is_smm(vcpu) && (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > return -EINVAL; > > + svm_leave_nested(svm); nVMX sets a really bad example in that it does vmx_leave_nested(), and many other things, long before it has vetted the incoming state. That's not the end of the word as the caller is likely going to exit if this ioctl() fails, but it would be nice to avoid such behavior with nSVM, especially since it appears to be trivially easy to do svm_leave_nested() iff the ioctl() will succeed. > + > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) { > - svm_leave_nested(svm); > svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET)); > return 0; > } > -- > 2.26.2 >
On Wed, 2021-01-06 at 09:39 -0800, Sean Christopherson wrote: > On Wed, Jan 06, 2021, Maxim Levitsky wrote: > > This should prevent bad things from happening if the user calls the > > KVM_SET_NESTED_STATE twice. > > This doesn't exactly inspire confidence, nor does it provide much help to > readers that don't already know why KVM should "leave nested" before processing > the rest of kvm_state. > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > arch/x86/kvm/svm/nested.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c > > index c1a3d0e996add..3aa18016832d0 100644 > > --- a/arch/x86/kvm/svm/nested.c > > +++ b/arch/x86/kvm/svm/nested.c > > @@ -1154,8 +1154,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, > > if (is_smm(vcpu) && (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) > > return -EINVAL; > > > > + svm_leave_nested(svm); > > nVMX sets a really bad example in that it does vmx_leave_nested(), and many > other things, long before it has vetted the incoming state. That's not the end > of the word as the caller is likely going to exit if this ioctl() fails, but it > would be nice to avoid such behavior with nSVM, especially since it appears to > be trivially easy to do svm_leave_nested() iff the ioctl() will succeed. I agree with you. So if I understand correctly I should move the unconditional svm_leave_nested(svm) after all the checks are done? I Best regards, Maxim Levitsky > > > + > > if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) { > > - svm_leave_nested(svm); > > svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET)); > > return 0; > > } > > -- > > 2.26.2 > >
diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index c1a3d0e996add..3aa18016832d0 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -1154,8 +1154,9 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu, if (is_smm(vcpu) && (kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) return -EINVAL; + svm_leave_nested(svm); + if (!(kvm_state->flags & KVM_STATE_NESTED_GUEST_MODE)) { - svm_leave_nested(svm); svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET)); return 0; }
This should prevent bad things from happening if the user calls the KVM_SET_NESTED_STATE twice. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- arch/x86/kvm/svm/nested.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)