| Message ID | 20241213194137.315304-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | KVM: x86: clear vcpu->run->hypercall.ret before exiting for KVM_EXIT_HYPERCALL | expand |
On Fri, Dec 13, 2024, Paolo Bonzini wrote: > QEMU up to 9.2.0 is assuming that vcpu->run->hypercall.ret is 0 on exit and > it never modifies it when processing KVM_EXIT_HYPERCALL. Make this explicit > in the code, to avoid breakage when KVM starts modifying that field. > > This in principle is not a good idea... It would have been much better if > KVM had set the field to -KVM_ENOSYS from the beginning, so that a dumb > userspace that does nothing on KVM_EXIT_HYPERCALL would tell the guest it > does not support KVM_HC_MAP_GPA_RANGE. However, breaking userspace is > a Very Bad Thing, as everybody should know. > > Reported-by: Binbin Wu <binbin.wu@linux.intel.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > arch/x86/kvm/svm/sev.c | 14 ++++++++++++++ > arch/x86/kvm/x86.c | 7 +++++++ > 2 files changed, 21 insertions(+) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 943bd074a5d3..9ffb0fb5aacd 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -3634,6 +3634,13 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr) > > vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; > vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; > + /* > + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) > + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that > + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting > + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. > + */ > + vcpu->run->hypercall.ret = 0; > vcpu->run->hypercall.args[0] = gpa; > vcpu->run->hypercall.args[1] = 1; > vcpu->run->hypercall.args[2] = (op == SNP_PAGE_STATE_PRIVATE) > @@ -3797,6 +3804,13 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc) > case VMGEXIT_PSC_OP_SHARED: > vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; > vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; > + /* > + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) > + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that > + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting > + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. > + */ > + vcpu->run->hypercall.ret = 0; > vcpu->run->hypercall.args[0] = gfn_to_gpa(gfn); > vcpu->run->hypercall.args[1] = npages; > vcpu->run->hypercall.args[2] = entry_start.operation == VMGEXIT_PSC_OP_PRIVATE > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 2e713480933a..705fa475179f 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -10052,6 +10052,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr, > > vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; > vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; > + /* > + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) > + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that > + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting > + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. > + */ > + vcpu->run->hypercall.ret = 0; As Binbin suggested, please add a helper. Copy-pasting multi-line comment is especially ugly.
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 943bd074a5d3..9ffb0fb5aacd 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3634,6 +3634,13 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr) vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; + /* + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. + */ + vcpu->run->hypercall.ret = 0; vcpu->run->hypercall.args[0] = gpa; vcpu->run->hypercall.args[1] = 1; vcpu->run->hypercall.args[2] = (op == SNP_PAGE_STATE_PRIVATE) @@ -3797,6 +3804,13 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc) case VMGEXIT_PSC_OP_SHARED: vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; + /* + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. + */ + vcpu->run->hypercall.ret = 0; vcpu->run->hypercall.args[0] = gfn_to_gpa(gfn); vcpu->run->hypercall.args[1] = npages; vcpu->run->hypercall.args[2] = entry_start.operation == VMGEXIT_PSC_OP_PRIVATE diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 2e713480933a..705fa475179f 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -10052,6 +10052,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr, vcpu->run->exit_reason = KVM_EXIT_HYPERCALL; vcpu->run->hypercall.nr = KVM_HC_MAP_GPA_RANGE; + /* + * In principle this should have been -KVM_ENOSYS, but userspace (QEMU <=9.2) + * assumed that vcpu->run->hypercall.ret is never changed by KVM and thus that + * it was always zero on KVM_EXIT_HYPERCALL. Since KVM is now overwriting + * vcpu->run->hypercall.ret, ensuring that it is zero to not break QEMU. + */ + vcpu->run->hypercall.ret = 0; vcpu->run->hypercall.args[0] = gpa; vcpu->run->hypercall.args[1] = npages; vcpu->run->hypercall.args[2] = attrs;
QEMU up to 9.2.0 is assuming that vcpu->run->hypercall.ret is 0 on exit and it never modifies it when processing KVM_EXIT_HYPERCALL. Make this explicit in the code, to avoid breakage when KVM starts modifying that field. This in principle is not a good idea... It would have been much better if KVM had set the field to -KVM_ENOSYS from the beginning, so that a dumb userspace that does nothing on KVM_EXIT_HYPERCALL would tell the guest it does not support KVM_HC_MAP_GPA_RANGE. However, breaking userspace is a Very Bad Thing, as everybody should know. Reported-by: Binbin Wu <binbin.wu@linux.intel.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- arch/x86/kvm/svm/sev.c | 14 ++++++++++++++ arch/x86/kvm/x86.c | 7 +++++++ 2 files changed, 21 insertions(+)