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

[Dr. David Alan Gilbert]

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

Dave

> 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 :|
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK