| Message ID | 20211109222350.2266045-2-seanjc@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: SEV: Fall back to __vmalloc() for SEV-ES scratch | expand |
On 11/9/21 4:23 PM, Sean Christopherson wrote: > Return appropriate error codes if setting up the GHCB scratch area for an > SEV-ES guest fails. In particular, returning -EINVAL instead of -ENOMEM > when allocating the kernel buffer could be confusing as userspace would > likely suspect a guest issue. Based on previous feedback and to implement the changes to the GHCB specification, I'm planning on submitting a patch that will return an error code back to the guest, instead of terminating the guest, if the scratch area fails to be setup properly. So you could hold off on this patch if you want. Thanks, Tom > > Fixes: 8f423a80d299 ("KVM: SVM: Support MMIO for an SEV-ES guest") > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Signed-off-by: Sean Christopherson <seanjc@google.com> > --- > arch/x86/kvm/svm/sev.c | 31 ++++++++++++++++++------------- > 1 file changed, 18 insertions(+), 13 deletions(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 3e2769855e51..ea8069c9b5cb 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -2299,7 +2299,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu) > } > > #define GHCB_SCRATCH_AREA_LIMIT (16ULL * PAGE_SIZE) > -static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > +static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > { > struct vmcb_control_area *control = &svm->vmcb->control; > struct ghcb *ghcb = svm->ghcb; > @@ -2310,14 +2310,14 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > scratch_gpa_beg = ghcb_get_sw_scratch(ghcb); > if (!scratch_gpa_beg) { > pr_err("vmgexit: scratch gpa not provided\n"); > - return false; > + return -EINVAL; > } > > scratch_gpa_end = scratch_gpa_beg + len; > if (scratch_gpa_end < scratch_gpa_beg) { > pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n", > len, scratch_gpa_beg); > - return false; > + return -EINVAL; > } > > if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) { > @@ -2335,7 +2335,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > scratch_gpa_end > ghcb_scratch_end) { > pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n", > scratch_gpa_beg, scratch_gpa_end); > - return false; > + return -EINVAL; > } > > scratch_va = (void *)svm->ghcb; > @@ -2348,18 +2348,18 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > if (len > GHCB_SCRATCH_AREA_LIMIT) { > pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n", > len, GHCB_SCRATCH_AREA_LIMIT); > - return false; > + return -EINVAL; > } > scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT); > if (!scratch_va) > - return false; > + return -ENOMEM; > > if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) { > /* Unable to copy scratch area from guest */ > pr_err("vmgexit: kvm_read_guest for scratch area failed\n"); > > kfree(scratch_va); > - return false; > + return -EFAULT; > } > > /* > @@ -2375,7 +2375,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) > svm->ghcb_sa = scratch_va; > svm->ghcb_sa_len = len; > > - return true; > + return 0; > } > > static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask, > @@ -2514,10 +2514,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) > ghcb_set_sw_exit_info_1(ghcb, 0); > ghcb_set_sw_exit_info_2(ghcb, 0); > > - ret = -EINVAL; > switch (exit_code) { > case SVM_VMGEXIT_MMIO_READ: > - if (!setup_vmgexit_scratch(svm, true, control->exit_info_2)) > + ret = setup_vmgexit_scratch(svm, true, control->exit_info_2); > + if (ret) > break; > > ret = kvm_sev_es_mmio_read(vcpu, > @@ -2526,7 +2526,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) > svm->ghcb_sa); > break; > case SVM_VMGEXIT_MMIO_WRITE: > - if (!setup_vmgexit_scratch(svm, false, control->exit_info_2)) > + ret = setup_vmgexit_scratch(svm, false, control->exit_info_2); > + if (ret) > break; > > ret = kvm_sev_es_mmio_write(vcpu, > @@ -2569,6 +2570,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) > vcpu_unimpl(vcpu, > "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n", > control->exit_info_1, control->exit_info_2); > + ret = -EINVAL; > break; > default: > ret = svm_invoke_exit_handler(vcpu, exit_code); > @@ -2579,8 +2581,11 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) > > int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in) > { > - if (!setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2)) > - return -EINVAL; > + int r; > + > + r = setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2); > + if (r) > + return r; > > return kvm_sev_es_string_io(&svm->vcpu, size, port, > svm->ghcb_sa, svm->ghcb_sa_len / size, in); >
On 11/11/21 16:14, Tom Lendacky wrote: > >> Return appropriate error codes if setting up the GHCB scratch area for an >> SEV-ES guest fails. In particular, returning -EINVAL instead of -ENOMEM >> when allocating the kernel buffer could be confusing as userspace would >> likely suspect a guest issue. > > Based on previous feedback and to implement the changes to the GHCB > specification, I'm planning on submitting a patch that will return an > error code back to the guest, instead of terminating the guest, if the > scratch area fails to be setup properly. So you could hold off on this > patch if you want. I think we still want these two patches in 5.16. Paolo
On 11/11/21 9:47 AM, Paolo Bonzini wrote: > On 11/11/21 16:14, Tom Lendacky wrote: >> >>> Return appropriate error codes if setting up the GHCB scratch area for an >>> SEV-ES guest fails. In particular, returning -EINVAL instead of -ENOMEM >>> when allocating the kernel buffer could be confusing as userspace would >>> likely suspect a guest issue. >> >> Based on previous feedback and to implement the changes to the GHCB >> specification, I'm planning on submitting a patch that will return an >> error code back to the guest, instead of terminating the guest, if the >> scratch area fails to be setup properly. So you could hold off on this >> patch if you want. > > I think we still want these two patches in 5.16. Ok, I'll rebase my changes on top of these then once you push them. Thanks, Tom > > Paolo >
diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 3e2769855e51..ea8069c9b5cb 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -2299,7 +2299,7 @@ void pre_sev_run(struct vcpu_svm *svm, int cpu) } #define GHCB_SCRATCH_AREA_LIMIT (16ULL * PAGE_SIZE) -static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) +static int setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) { struct vmcb_control_area *control = &svm->vmcb->control; struct ghcb *ghcb = svm->ghcb; @@ -2310,14 +2310,14 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) scratch_gpa_beg = ghcb_get_sw_scratch(ghcb); if (!scratch_gpa_beg) { pr_err("vmgexit: scratch gpa not provided\n"); - return false; + return -EINVAL; } scratch_gpa_end = scratch_gpa_beg + len; if (scratch_gpa_end < scratch_gpa_beg) { pr_err("vmgexit: scratch length (%#llx) not valid for scratch address (%#llx)\n", len, scratch_gpa_beg); - return false; + return -EINVAL; } if ((scratch_gpa_beg & PAGE_MASK) == control->ghcb_gpa) { @@ -2335,7 +2335,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) scratch_gpa_end > ghcb_scratch_end) { pr_err("vmgexit: scratch area is outside of GHCB shared buffer area (%#llx - %#llx)\n", scratch_gpa_beg, scratch_gpa_end); - return false; + return -EINVAL; } scratch_va = (void *)svm->ghcb; @@ -2348,18 +2348,18 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) if (len > GHCB_SCRATCH_AREA_LIMIT) { pr_err("vmgexit: scratch area exceeds KVM limits (%#llx requested, %#llx limit)\n", len, GHCB_SCRATCH_AREA_LIMIT); - return false; + return -EINVAL; } scratch_va = kzalloc(len, GFP_KERNEL_ACCOUNT); if (!scratch_va) - return false; + return -ENOMEM; if (kvm_read_guest(svm->vcpu.kvm, scratch_gpa_beg, scratch_va, len)) { /* Unable to copy scratch area from guest */ pr_err("vmgexit: kvm_read_guest for scratch area failed\n"); kfree(scratch_va); - return false; + return -EFAULT; } /* @@ -2375,7 +2375,7 @@ static bool setup_vmgexit_scratch(struct vcpu_svm *svm, bool sync, u64 len) svm->ghcb_sa = scratch_va; svm->ghcb_sa_len = len; - return true; + return 0; } static void set_ghcb_msr_bits(struct vcpu_svm *svm, u64 value, u64 mask, @@ -2514,10 +2514,10 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) ghcb_set_sw_exit_info_1(ghcb, 0); ghcb_set_sw_exit_info_2(ghcb, 0); - ret = -EINVAL; switch (exit_code) { case SVM_VMGEXIT_MMIO_READ: - if (!setup_vmgexit_scratch(svm, true, control->exit_info_2)) + ret = setup_vmgexit_scratch(svm, true, control->exit_info_2); + if (ret) break; ret = kvm_sev_es_mmio_read(vcpu, @@ -2526,7 +2526,8 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) svm->ghcb_sa); break; case SVM_VMGEXIT_MMIO_WRITE: - if (!setup_vmgexit_scratch(svm, false, control->exit_info_2)) + ret = setup_vmgexit_scratch(svm, false, control->exit_info_2); + if (ret) break; ret = kvm_sev_es_mmio_write(vcpu, @@ -2569,6 +2570,7 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) vcpu_unimpl(vcpu, "vmgexit: unsupported event - exit_info_1=%#llx, exit_info_2=%#llx\n", control->exit_info_1, control->exit_info_2); + ret = -EINVAL; break; default: ret = svm_invoke_exit_handler(vcpu, exit_code); @@ -2579,8 +2581,11 @@ int sev_handle_vmgexit(struct kvm_vcpu *vcpu) int sev_es_string_io(struct vcpu_svm *svm, int size, unsigned int port, int in) { - if (!setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2)) - return -EINVAL; + int r; + + r = setup_vmgexit_scratch(svm, in, svm->vmcb->control.exit_info_2); + if (r) + return r; return kvm_sev_es_string_io(&svm->vcpu, size, port, svm->ghcb_sa, svm->ghcb_sa_len / size, in);
Return appropriate error codes if setting up the GHCB scratch area for an SEV-ES guest fails. In particular, returning -EINVAL instead of -ENOMEM when allocating the kernel buffer could be confusing as userspace would likely suspect a guest issue. Fixes: 8f423a80d299 ("KVM: SVM: Support MMIO for an SEV-ES guest") Cc: Tom Lendacky <thomas.lendacky@amd.com> Signed-off-by: Sean Christopherson <seanjc@google.com> --- arch/x86/kvm/svm/sev.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)