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

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'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.

You can only DoS yourself with virsh, and you'll be limited by the RPC protocol wire limit. > > docs/manpages/virsh.rst | 25 ++++++++++ > tools/virsh-domain.c | 107 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 132 insertions(+) With a 1 MB or similar size limit Reviewed-by: Daniel P. Berrangé <berrange(a)redhat.com&gt; Regards, Daniel