[PATCH V3 4/4] tools: Add domsetlaunchsecstate virsh command

[Daniel P. Berrangé]

On Thu, Dec 16, 2021 at 12:06:22PM -0700, Jim Fehlig wrote:
> On 12/16/21 08:02, Daniel P. Berrangé wrote:
> > On Tue, Dec 14, 2021 at 09:46:06PM -0700, Jim Fehlig wrote:
> > > After attesting a domain with the help of domlaunchsecinfo,
> > > domsetlaunchsecstate can be used to set a secret in the guest
> > > domain's memory prior to running the vcpus.
> > > 
> > > Signed-off-by: Jim Fehlig <jfehlig at suse.com>
> > > ---
> > > 
> > > Some questions and RFC regarding this patch:
> > > 
> > > I'm not really fond of the command and function names and would appreciate
> > > suggestions :-).
> > 
> > I'm honestly not too fussed about the naming. THis command is really
> > just about feature complete API coverage. I doubt many people will
> > actually use virsh for this, instead they'll want a program that
> > queries the measurement, verifies it and injects secret all in one
> > go.
> > 
> > > Also, is reading the secret header and secret from a file sufficient? The
> > > sev-tool 'package_secret' command writes the secret to a file.
> > 
> > Fine IMHO.
> > 
> > > Lastly, I'm not sure what sizes to expect for secret and secret header. I
> > > may have overlooked it, but didn't find anything related to the size in the
> > > docs. I've temporarily set it to VSH_MAX_XML_FILE until we know a reasonable
> > > value.
> 
> I took another look at the API spec and section "6.6 LAUNCH_SECRET",
> subsection "6.6.2 Parameters" describes the parameters to the LUANCH_SECRET
> command and the layout the the secret header. IIUC, GUEST_PADDR and
> GUEST_LENGTH tell where to put the secret. GUEST_LENGTH cannot be greater
> than 16KB. And the secret header looks to be 104 bytes.
> 
> https://www.amd.com/system/files/TechDocs/55766_SEV-KM_API_Specification.pdf
> 
> > I'm not too sure either, but as a general point, IMHO, almost all
> > our use of virFileReadAll has no functional or security need for
> > us to supply a limit at all.
> > 
> > We really ought to just make it accept '-1' as a limit and treat
> > that as unlimited.  Meanwhile I'd suggest just letting it be
> > 1 MB which is way bigger than i expect this data will be.
> 
> Given the above, 1MB is overkill. But still go with it as a safer, more
> future-proof value?

Given the docs I'm happy if we drop it down to 64kb, as that
still gives lots of headroom.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|