linux-kernel - Re: [PATCH v4 1/1] KVM: Introduce KVM_EXIT_SNP_REQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250219173628.hlbrp4c7uf3wtv4r@amd.com>
Date: Wed, 19 Feb 2025 11:36:28 -0600
From: Michael Roth <michael.roth@....com>
To: Dionna Amalie Glaze <dionnaglaze@...gle.com>
CC: Sean Christopherson <seanjc@...gle.com>, Melody Wang <huibo.wang@....com>,
	<kvm@...r.kernel.org>, <linux-coco@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, Paolo Bonzini <pbonzini@...hat.com>,
	<roedel@...e.de>, Tom Lendacky <thomas.lendacky@....com>,
	<ashish.kalra@....com>, <liam.merwick@...cle.com>, <pankaj.gupta@....com>
Subject: Re: [PATCH v4 1/1] KVM: Introduce KVM_EXIT_SNP_REQ_CERTS for SNP
 certificate-fetching

On Tue, Jan 21, 2025 at 09:19:08AM -0800, Dionna Amalie Glaze wrote:
> On Tue, Jan 21, 2025 at 8:52 AM Sean Christopherson <seanjc@...gle.com> wrote:
> >
> > On Tue, Jan 21, 2025, Dionna Amalie Glaze wrote:
> > > On Mon, Jan 20, 2025 at 1:58 PM Melody Wang <huibo.wang@....com> wrote:
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index 943bd074a5d3..4896c34ed318 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -4064,6 +4064,30 @@ static int snp_handle_guest_req(struct vcpu_svm *svm, gpa_t req_gpa, gpa_t resp_
> > > >         return ret;
> > > >  }
> > > >
> > > > +static int snp_complete_req_certs(struct kvm_vcpu *vcpu)
> > > > +{
> > > > +       struct vcpu_svm *svm = to_svm(vcpu);
> > > > +       struct vmcb_control_area *control = &svm->vmcb->control;
> > > > +
> > > > +       if (vcpu->run->snp_req_certs.ret) {
> > > > +               if (vcpu->run->snp_req_certs.ret == ENOSPC) {
> > > > +                       vcpu->arch.regs[VCPU_REGS_RBX] = vcpu->run->snp_req_certs.npages;
> > > > +                       ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> > > > +                                               SNP_GUEST_ERR(SNP_GUEST_VMM_ERR_INVALID_LEN, 0));
> > > > +               } else if (vcpu->run->snp_req_certs.ret == EAGAIN) {
> > > > +                       ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> > > > +                                               SNP_GUEST_ERR(SNP_GUEST_VMM_ERR_BUSY, 0));
> > >
> > > Discussion, not a change request: given that my proposed patch [1] to
> > > add rate-limiting for guest messages to the PSP generally was
> > > rejected,
> >
> > For the record, it wasn't rejected outright.  I pointed out flaws in the proposed
> > behavior[*], and AFAICT no one ever responded.  If I fully reject something, I
> > promise I will make it abundantly clear :-)
> >
> 
> Okay, well it was a no to the implementation strategy I had chosen and
> did not have bandwidth to change. Your suggestion to exit to userspace
> is more aligned with the topic of discussion now.
> 
> > [*] https://lore.kernel.org/all/Y8rEFpbMV58yJIKy@google.com
> >
> > > do we think it'd be proper to add a KVM_EXIT_SNP_REQ_MSG or
> > > some such for the VMM to decide if the guest should have access to the
> > > globally shared resource (PSP) via EAGAIN or 0?
> >
> > Can you elaborate?  I don't quite understand what you're suggesting.
> >
> 
> I just mean that instead of only exiting to the VMM on an extended
> guest request and this capability enabled, all guest requests exit to
> the VMM to make the decision to permit access to the device. EAGAIN
> means busy, try again later, and 0 means permit the request. That
> allows for implementation-specific throttling policies to be
> implemented.

Discussed this with Sean on the PUCK call last week and our thinking was
that tying throttling support to a new KVM_EXIT_* type would not be
compatible with existing userspaces unless it is opt-in, and requiring
userspace to opt-in would not be ideal if DoS on the part of guest-owner is
a concern.

So most likely the more generalized exit-to-userspace throttling behavior
would be tied to something pre-existing, like existing EAGAIN/EINTR handling
for the KVM_RUN ioctl or something along that line.

And if the throttling occurs as part of this particular path, we still have
the option of using the SNP_GUEST_VMM_ERR_BUSY response to guest to force a
retry while still giving userspace the immediate_exit-triggered exit it
needs to release the lock and complete things on its end.

So, since throttling support doesn't seem like a blocker for this series, I
went ahead and sent a v5 with some minor documentation updates:

  https://lore.kernel.org/kvm/20250219151505.3538323-1-michael.roth@amd.com/

Thanks!

-Mike

> 
> > > [1] https://patchwork.kernel.org/project/kvm/cover/20230119213426.379312-1-dionnaglaze@google.com/
> > >
> > > > +               } else {
> > > > +                       ghcb_set_sw_exit_info_2(svm->sev_es.ghcb,
> > > > +                                               SNP_GUEST_ERR(SNP_GUEST_VMM_ERR_GENERIC, 0));
> > > > +               }
> > > > +
> > > > +               return 1; /* resume guest */
> > > > +       }
> > > > +
> > > > +       return snp_handle_guest_req(svm, control->exit_info_1, control->exit_info_2);
> > > > +}
> 
> 
> 
> -- 
> -Dionna Glaze, PhD, CISSP, CCSP (she/her)
>