Re: [PATCH 1/3] sev/i386: Allow launching with -kernel if no OVMF hashes table found

[Daniel P . Berrangé]

On Thu, Nov 04, 2021 at 06:18:10PM +0000, Dr. David Alan Gilbert wrote:
> * Daniel P. Berrangé (berrange@redhat.com) wrote:
> > On Mon, Nov 01, 2021 at 10:21:34AM +0000, Dov Murik wrote:
> > > Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> > > for measured linux boot", 2021-09-30) introduced measured direct boot
> > > with -kernel, using an OVMF-designated hashes table which QEMU fills.
> > > 
> > > However, if OVMF doesn't designate such an area, QEMU would completely
> > > abort the VM launch.  This breaks launching with -kernel using older
> > > OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID.
> > > 
> > > Instead, just warn the user that -kernel was supplied by OVMF doesn't
> > > specify the GUID for the hashes table.  The following warning will be
> > > displayed during VM launch:
> > > 
> > >     qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no 
> > > hash table guid
> > > 
> > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> > > Reported-by: Tom Lendacky <thomas.lendacky@amd.com>
> > > ---
> > >  target/i386/sev.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > > index eede07f11d..682b8ccf6c 100644
> > > --- a/target/i386/sev.c
> > > +++ b/target/i386/sev.c
> > > @@ -1204,7 +1204,7 @@ bool 
> > > sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
> > >      int aligned_len;
> > >  
> > >      if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) 
> > > {
> > > -        error_setg(errp, "SEV: kernel specified but OVMF has no hash 
> > > table guid");
> > > +        warn_report("SEV: kernel specified but OVMF has no hash table 
> > > guid");
> > >          return false;
> > 
> > I'm pretty wary of doing this kind of thing.
> > 
> > If someone is using QEMU and they required to have the hashes populated
> > for their use case, they now don't get a fatal error if something goes
> > wrong with the process. This is bad as it hides a serious mistake.
> > 
> > If someone is using QEMU and they don't require to have the hashes
> > populated and they knowingly use a firmware that doesn't support
> > this, they'll now get a irrelevant warning every time they boot
> > QEMU. This is bad because IME users will file bugs complaining
> > about this bogus warning.
> > 
> > 
> > If we genuinely need to support both uses cases, then we should have
> > an explicit command line flag to request the desired behaviour.
> > 
> > This could be a -machine option to indicate that the hashes must
> > be populated.
> > 
> >  - unset: try to populate hashes, *silently* ignore missing table
> >           in ovmf
> >  - set == on: try to populate hashes, report error on missing
> >              table in ovmf
> >   -set == off: never try to populate hashes, nor look for the
> >                table in ovmf
> 
> Or as a property on the sev-guest object.

Yep, I thought of that too, and I'm pretty undecided which is "best".

-machine makes sense as 'kernel' and 'initrd' are properties of
the '-machine' and we're doing stuff related to the.

-object sev-guest makes sense as this is behaviour that's (currently)
specific to SEV.


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 :|