On Wed, Jan 18, 2023 at 7:27 AM Jeremi Piotrowski
<jpiotrowski@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 14, 2022 at 01:40:36PM -0600, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@xxxxxxx>
> >
> > On VMGEXIT, sev_handle_vmgexit() creates a host mapping for the GHCB GPA,
> > and unmaps it just before VM-entry. This long-lived GHCB map is used by
> > the VMGEXIT handler through accessors such as ghcb_{set_get}_xxx().
> >
> > A long-lived GHCB map can cause issue when SEV-SNP is enabled. When
> > SEV-SNP is enabled the mapped GPA needs to be protected against a page
> > state change.
> >
> > To eliminate the long-lived GHCB mapping, update the GHCB sync operations
> > to explicitly map the GHCB before access and unmap it after access is
> > complete. This requires that the setting of the GHCBs sw_exit_info_{1,2}
> > fields be done during sev_es_sync_to_ghcb(), so create two new fields in
> > the vcpu_svm struct to hold these values when required to be set outside
> > of the GHCB mapping.
> >
> > Signed-off-by: Brijesh Singh <brijesh.singh@xxxxxxx>
> > Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>
> > [mdr: defer per_cpu() assignment and order it with barrier() to fix case
> > where kvm_vcpu_map() causes reschedule on different CPU]
> > Signed-off-by: Michael Roth <michael.roth@xxxxxxx>
> > ---
> > arch/x86/kvm/svm/sev.c | 131 ++++++++++++++++++++++++++---------------
> > arch/x86/kvm/svm/svm.c | 18 +++---
> > arch/x86/kvm/svm/svm.h | 24 +++++++-
> > 3 files changed, 116 insertions(+), 57 deletions(-)
> >
> > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > index d5c6e48055fb..6ac0cb6e3484 100644
> > --- a/arch/x86/kvm/svm/sev.c
> > +++ b/arch/x86/kvm/svm/sev.c
> > @@ -2921,15 +2921,40 @@ void sev_free_vcpu(struct kvm_vcpu *vcpu)
> > kvfree(svm->sev_es.ghcb_sa);
> > }
> >
> > +static inline int svm_map_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > +{
> > + struct vmcb_control_area *control = &svm->vmcb->control;
> > + u64 gfn = gpa_to_gfn(control->ghcb_gpa);
> > +
> > + if (kvm_vcpu_map(&svm->vcpu, gfn, map)) {
> > + /* Unable to map GHCB from guest */
> > + pr_err("error mapping GHCB GFN [%#llx] from guest\n", gfn);
> > + return -EFAULT;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static inline void svm_unmap_ghcb(struct vcpu_svm *svm, struct kvm_host_map *map)
> > +{
> > + kvm_vcpu_unmap(&svm->vcpu, map, true);
> > +}
> > +
> > static void dump_ghcb(struct vcpu_svm *svm)
> > {
> > - struct ghcb *ghcb = svm->sev_es.ghcb;
> > + struct kvm_host_map map;
> > unsigned int nbits;
> > + struct ghcb *ghcb;
> > +
> > + if (svm_map_ghcb(svm, &map))
> > + return;
> > +
> > + ghcb = map.hva;
>
> dump_ghcb() is called from sev_es_validate_vmgexit() with the ghcb already
> mapped. How about passing 'struct kvm_host_map *' (or struct ghcb *) as a
> param to avoid double mapping?
This also causes a soft lockup, PSC spin lock is already acquired in
sev_es_validate_vmgexit. dump_ghcb will try to acquire the same lock
again. So a guest can send an invalid ghcb page and cause a host soft
lockup.
>
> >
> > /* Re-use the dump_invalid_vmcb module parameter */
> > if (!dump_invalid_vmcb) {
> > pr_warn_ratelimited("set kvm_amd.dump_invalid_vmcb=1 to dump internal KVM state.\n");
> > - return;
> > + goto e_unmap;
> > }
> >
> > nbits = sizeof(ghcb->save.valid_bitmap) * 8;
> > @@ -2944,12 +2969,21 @@ static void dump_ghcb(struct vcpu_svm *svm)
> > pr_err("%-20s%016llx is_valid: %u\n", "sw_scratch",
> > ghcb->save.sw_scratch, ghcb_sw_scratch_is_valid(ghcb));
> > pr_err("%-20s%*pb\n", "valid_bitmap", nbits, ghcb->save.valid_bitmap);
> > +
> > +e_unmap:
> > + svm_unmap_ghcb(svm, &map);
> > }
> >
> > -static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> > +static bool sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> > {
> > struct kvm_vcpu *vcpu = &svm->vcpu;
> > - struct ghcb *ghcb = svm->sev_es.ghcb;
> > + struct kvm_host_map map;
> > + struct ghcb *ghcb;
> > +
> > + if (svm_map_ghcb(svm, &map))
> > + return false;
> > +
> > + ghcb = map.hva;
> >
> > /*
> > * The GHCB protocol so far allows for the following data
> > @@ -2963,13 +2997,24 @@ static void sev_es_sync_to_ghcb(struct vcpu_svm *svm)
> > ghcb_set_rbx(ghcb, vcpu->arch.regs[VCPU_REGS_RBX]);
> > ghcb_set_rcx(ghcb, vcpu->arch.regs[VCPU_REGS_RCX]);
> > ghcb_set_rdx(ghcb, vcpu->arch.regs[VCPU_REGS_RDX]);
> > +
> > + /*
> > + * Copy the return values from the exit_info_{1,2}.
> > + */
> > + ghcb_set_sw_exit_info_1(ghcb, svm->sev_es.ghcb_sw_exit_info_1);
> > + ghcb_set_sw_exit_info_2(ghcb, svm->sev_es.ghcb_sw_exit_info_2);
> > +
> > + trace_kvm_vmgexit_exit(svm->vcpu.vcpu_id, ghcb);
> > +
> > + svm_unmap_ghcb(svm, &map);
> > +
> > + return true;
> > }
> >
> > -static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> > +static void sev_es_sync_from_ghcb(struct vcpu_svm *svm, struct ghcb *ghcb)
> > {
> > struct vmcb_control_area *control = &svm->vmcb->control;
> > struct kvm_vcpu *vcpu = &svm->vcpu;
> > - struct ghcb *ghcb = svm->sev_es.ghcb;
> > u64 exit_code;
> >
> > /*
> > @@ -3013,20 +3058,25 @@ static void sev_es_sync_from_ghcb(struct vcpu_svm *svm)
> > memset(ghcb->save.valid_bitmap, 0, sizeof(ghcb->save.valid_bitmap));
> > }
> >
> > -static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> > +static int sev_es_validate_vmgexit(struct vcpu_svm *svm, u64 *exit_code)
> > {
> > - struct kvm_vcpu *vcpu;
> > + struct kvm_vcpu *vcpu = &svm->vcpu;
> > + struct kvm_host_map map;
> > struct ghcb *ghcb;
> > - u64 exit_code;
> > u64 reason;
> >
> > - ghcb = svm->sev_es.ghcb;
> > + if (svm_map_ghcb(svm, &map))
> > + return -EFAULT;
> > +
> > + ghcb = map.hva;
> > +
> > + trace_kvm_vmgexit_enter(vcpu->vcpu_id, ghcb);
> >
> > /*
> > * Retrieve the exit code now even though it may not be marked valid
> > * as it could help with debugging.
> > */
> > - exit_code = ghcb_get_sw_exit_code(ghcb);
> > + *exit_code = ghcb_get_sw_exit_code(ghcb);
> >
> > /* Only GHCB Usage code 0 is supported */
> > if (ghcb->ghcb_usage) {
> > @@ -3119,6 +3169,9 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> > goto vmgexit_err;
> > }
> >
> > + sev_es_sync_from_ghcb(svm, ghcb);
> > +
> > + svm_unmap_ghcb(svm, &map);
> > return 0;
> >
> > vmgexit_err:
> > @@ -3129,10 +3182,10 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> > ghcb->ghcb_usage);
> > } else if (reason == GHCB_ERR_INVALID_EVENT) {
> > vcpu_unimpl(vcpu, "vmgexit: exit code %#llx is not valid\n",
> > - exit_code);
> > + *exit_code);
> > } else {
> > vcpu_unimpl(vcpu, "vmgexit: exit code %#llx input is not valid\n",
> > - exit_code);
> > + *exit_code);
> > dump_ghcb(svm);
> > }
> >
> > @@ -3142,6 +3195,8 @@ static int sev_es_validate_vmgexit(struct vcpu_svm *svm)
> > ghcb_set_sw_exit_info_1(ghcb, 2);
> > ghcb_set_sw_exit_info_2(ghcb, reason);
> >
> > + svm_unmap_ghcb(svm, &map);
> > +
> > /* Resume the guest to "return" the error code. */
> > return 1;
> > }
>
