Re: [PATCH v1 3/3] i386/sev: Add KVM_EXIT_SNP_REQ_CERTS support for certificate-fetching

[Michael Roth]

On Wed, Dec 18, 2024 at 06:32:05PM +0100, Markus Armbruster wrote:
> Michael Roth <michael.roth@amd.com> writes:
> 
> > The GHCB specification[1] defines a VMGEXIT-based Guest Request
> > hypercall to allow an SNP guest to issue encrypted requests directly to
> > SNP firmware to do things like query the attestation report for the
> > guest. These are generally handled purely in the kernel.
> >
> > In some some cases, it's useful for the host to be able to additionally
> > supply the certificate chain for the signing key that SNP firmware uses
> > to sign these attestation reports. To allow for this, the GHCB
> > specification defines an Extended Guest Request where this certificate
> > data can be provided in a special format described in the GHCB spec.
> > This certificate data may be global or guest-specific depending on how
> > the guest was configured. Rather than providing interfaces to manage
> > these within the kernel, KVM provides a new KVM_EXIT_SNP_REQ_CERTS exit
> > to request the certificate contents from userspace. Implement support
> > for that here.
> >
> > To synchronize delivery of the certificates to the guest in a way where
> > they will not be rendered invalid by updates to SNP firmware or
> > attestation singing/endorsement keys by management tools outside the
> > purview of QEMU, it is expected by users of KVM_EXIT_SNP_REQ_CERTS to
> > obtain a shared/read lock on the certificate file prior to delivering
> > them back to KVM. Only after this will the attestation report be
> > retrieved from firmware and bundled with the certificate data, so QEMU
> > must continue to hold the file lock until KVM confirms that the
> > attestation report has been retrieved/bundled. This confirmation is done
> > by way of the kvm_immediate_exit callback infrastructure that was
> > introduced in a previous patch.
> 
> The "management tools outside the purview of QEMU" will all obtain the
> same kind of file lock?

Yes, this is meant for a cooperative environment where the cloud provider
has opted to provide guest certificates in this manner and needs a way
to synchronize guest attestation requests with other parts of their
stack handling management duties like updating firmware/endorsement
keys/etc. which might occur between the time a certificate is fetched
and the time the attestation report is generated by firmware.

The idea is to provide a path of least resistance using a common
framework like filesystem locks, then heavily suggest that approach on
the kernel documentation side, so that tools purposely or naturally opt
to use this mechanism rather than every service provider coming up with
some separate thing that they'll need to work into some custom QEMU/VMM
to solve the same problem.

Of course, userspace is free to implement their own completely separate
mechanism for handling all this and completely ignore file-locking. But
QEMU is only trying to play nice with this above-mentioned reference
implementation and cooperative management tools, and not trying to
profess to provide any sort of synchronization for cases where those
sorts of management-level updates are performed without utilizing this
reference implementation for synchronization.

I should have touched on this in the schema documentation. I'll make
sure to add that in the next spin.

> 
> > [1] "Guest Hypervisor Communication Block (GHCB) Standardization",
> >     https://www.amd.com/en/developer/sev.html
> >
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >  qapi/qom.json                 |  23 +++-
> >  target/i386/kvm/kvm.c         |  10 ++
> >  target/i386/sev-sysemu-stub.c |   5 +
> >  target/i386/sev.c             | 249 ++++++++++++++++++++++++++++++++++
> >  target/i386/sev.h             |   2 +
> >  5 files changed, 288 insertions(+), 1 deletion(-)
> >
> > diff --git a/qapi/qom.json b/qapi/qom.json
> > index 28ce24cd8d..6eaf0e7721 100644
> > --- a/qapi/qom.json
> > +++ b/qapi/qom.json
> > @@ -1034,6 +1034,25 @@
> >  #     firmware.  Set this to true to disable the use of VCEK.
> >  #     (default: false) (since: 9.1)
> >  #
> > +# @certs-path: Path to certificate data that can be passed to guests via
> > +#              SNP Extended Guest Requests. File should be in the format
> > +#              described in the GHCB specification. (default: none)
> > +#              (since: 10.0)
> 
> I prefer "filename" to "path".  We have many kinds of paths: pathnames
> (denoting files), QOM paths (denoting objects), qdev paths, search
> paths, ...  With "filename", your readers immediately know what you're
> talking about.
> 
> SevGuestProperties has a member 'dh-cert-file'.  Whether that's related
> to your file I can't tell from its documentation.
> 
> > +#
> > +# @certs-timeout: Max time in milliseconds to wait to obtain a read lock
> 
> Please don't abbreviate "Maximum" here.
> 
> Confident millisecond granularity will suffice forever?

I believe so.

There will likely be some form of rate-limiting added to KVM along the lines
of this patch, with proposals on the order of 2 requests per second for guest
requests to avoid DoS'ing each other:

  https://lore.kernel.org/all/20230119213426.379312-1-dionnaglaze@google.com/

I believe guest kernels already voluntarily limit themselves to
something similar before retrying requests.

So the only case I can think of where a sub-1ms interval would be
desired would be cases where the guest has very aggressive soft-lockup
detection settings, in which case we might as well just set the timeout
to 0 and not busy-wait at all, which should be allowed for here.
(There's more documentation in the related handling in
target/i386/sev.c, but in such cases the guest will get an EAGAIN and
generally retry the request later).

> 
> > +#                 on the certificate file specified by @certs-path. This
> > +#                 is not a cumulative value and only affects how long
> > +#                 QEMU waits before returning execution to the vCPU and
> > +#                 informing the guest of the timeout, so the guest can
> > +#                 still continuing retrying for as long as it likes
> > +#                 (which will be about 60 seconds for linux guests at
> > +#                 the time of this writing). If the guest-side timeout
> > +#                 is insufficient, set this higher to allow more time to
> > +#                 fetch the certificate. If the guest-side timeout is
> > +#                 sufficient, set this lower to reduce the likelihood of
> > +#                 soft lockups in the guest.
> > +#                 (default: 100) (since: 10.0)
> > +#
> >  # Since: 9.1
> >  ##
> 
> Please format like
> 
>    # @certs-path: Path to certificate data that can be passed to guests
>    #     via SNP Extended Guest Requests.  File should be in the format
>    #     described in the GHCB specification.
>    #     (default: none) (since: 10.0)
>    #
>    # @certs-timeout: Max time in milliseconds to wait to obtain a read
>    #     lock on the certificate file specified by @certs-path.  This is
>    #     not a cumulative value and only affects how long QEMU waits
>    #     before returning execution to the vCPU and informing the guest
>    #     of the timeout, so the guest can still continuing retrying for
>    #     as long as it likes (which will be about 60 seconds for linux
>    #     guests at the time of this writing).  If the guest-side timeout
>    #     is insufficient, set this higher to allow more time to fetch the
>    #     certificate.  If the guest-side timeout is sufficient, set this
>    #     lower to reduce the likelihood of soft lockups in the guest.
>    #     (default: 100) (since: 10.0)
> 
> to blend in with commit a937b6aa739 (qapi: Reformat doc comments to
> conform to current conventions).

Will do!

-Mike

> 
> >  { 'struct': 'SevSnpGuestProperties',
> > @@ -1045,7 +1064,9 @@
> >              '*id-auth': 'str',
> >              '*author-key-enabled': 'bool',
> >              '*host-data': 'str',
> > -            '*vcek-disabled': 'bool' } }
> > +            '*vcek-disabled': 'bool',
> > +            '*certs-path': 'str',
> > +            '*certs-timeout': 'uint32' } }
> >  
> >  ##
> >  # @ThreadContextProperties:
> 
> [...]
>