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

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(a)suse.com&gt; > > --- > > > > 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?