On Thu, Mar 31, 2022 at 11:48 AM Marc Orr <marcorr@xxxxxxxxxx> wrote:
>
> On Thu, Mar 31, 2022 at 10:40 AM Marc Orr <marcorr@xxxxxxxxxx> wrote:
> >
> > On Thu, Mar 31, 2022 at 10:11 AM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > >
> > > +Paolo and Vitaly
> > >
> > > In the future, I highly recommend using scripts/get_maintainers.pl.
> > >
> > > On Wed, Mar 30, 2022, Peter Gonda wrote:
> > > > SEV-ES guests can request termination using the GHCB's MSR protocol. See
> > > > AMD's GHCB spec section '4.1.13 Termination Request'. Currently when a
> > > > guest does this the userspace VMM sees an KVM_EXIT_UNKNOWN (-EVINAL)
> > > > return code from KVM_RUN. By adding a KVM_EXIT_SHUTDOWN_ENTRY to kvm_run
> > > > struct the userspace VMM can clearly see the guest has requested a SEV-ES
> > > > termination including the termination reason code set and reason code.
> > > >
> > > > Signed-off-by: Peter Gonda <pgonda@xxxxxxxxxx>
> > > >
> > > > ---
> > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> > > > index 75fa6dd268f0..5f9d37dd3f6f 100644
> > > > --- a/arch/x86/kvm/svm/sev.c
> > > > +++ b/arch/x86/kvm/svm/sev.c
> > > > @@ -2735,8 +2735,13 @@ static int sev_handle_vmgexit_msr_protocol(struct vcpu_svm *svm)
> > > > pr_info("SEV-ES guest requested termination: %#llx:%#llx\n",
> > > > reason_set, reason_code);
> > >
> > > This pr_info() should be removed. A malicious usersepace could spam the kernel
> > > by constantly running a vCPU that requests termination.
>
> Though... this patch makes this pr_info redundant! Since we'll now
> report this in userspace. Actually, I'd be OK to remove it.
I'll make this 2 patches. This current patch and another to rate limit
this pr_info() I think this patch is doing a lot already so would
prefer to just add a second. Is that reasonable?
>
> > Ah, good catch. But actually, I've found this specific pr_info _very_
> > useful in debugging. Sean, would you be OK to convert it to a rate
> > limited print?
> >
> > > > - ret = -EINVAL;
> > > > - break;
> > > > + vcpu->run->exit_reason = KVM_EXIT_SHUTDOWN;
> > > > + vcpu->run->shutdown.reason = KVM_SHUTDOWN_SEV_TERM;
> > > > + vcpu->run->shutdown.ndata = 2;
> > > > + vcpu->run->shutdown.data[0] = reason_set;
> > > > + vcpu->run->shutdown.data[1] = reason_code;
> > >
> > > Does KVM really need to split the reason_set and reason_code? Without reading
> > > the spec, it's not even clear what "set" means. Assuming it's something like
> > > "the reason code is valid", then I don't see any reason (lol) to split the two.
> > > If we do split them, then arguably the reason_code should be filled if and only
> > > if reason_set is true, and that's just extra work.
> >
> > I think KVM should split the reason_set and reason_code. This code is
> > based on the GHCB spec after all. And reason_set and reason_code are
> > both a part of the GHCB spec. But I agree, folks shouldn't have to go
> > to the spec to understand what reason_set and reason_code are. Rather
> > than not splitting them up, can we add comments in the source to
> > explain what they mean?
> >
> > Also, my understanding from reading the spec is that reason_code
> > should always be filled, even when reason_set is 0. See below. But
> > basically, you can have reason_set: 0 and reason_code: non-zero.
> >
> > Quoting the spec:
> > The reason code set is meant to provide hypervisors with their own
> > termination SEV-ES Guest-Hypervisor Communication Block
> > Standardization reason codes. This document defines and owns reason
> > code set 0x0 and the following reason codes (GHCBData[23:16]):
> > 0x00 – General termination request
> > 0x01 – SEV-ES / GHCB Protocol range is not supported.
> > 0x02 – SEV-SNP features not supported
>
> Reading this again, I now see that "reason_set" sounds like "The
> reason code is set". I bet that's how Sean read it during his review.
> So yeah, this needs comments :-)!
I'll add comments but I agree with Marc. These are part of the GHCB
spec so for the very specific SEV-ES termination reason we should
include all the data the spec allows. Sounds OK?
