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

From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Dov Murik <dovmurik@linux.ibm.com>
Cc: "Tom Lendacky" <thomas.lendacky@amd.com>,
	"Ashish Kalra" <ashish.kalra@amd.com>,
	"Brijesh Singh" <brijesh.singh@amd.com>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"James Bottomley" <jejb@linux.ibm.com>,
	"Marcelo Tosatti" <mtosatti@redhat.com>,
	qemu-devel@nongnu.org,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Tobin Feldman-Fitzthum" <tobin@linux.ibm.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>
Subject: Re: [PATCH 1/3] sev/i386: Allow launching with -kernel if no OVMF hashes table found
Date: Wed, 3 Nov 2021 16:02:37 +0000	[thread overview]
Message-ID: <YYKynfyEpwn3PRHW@redhat.com> (raw)
In-Reply-To: <20211101102136.1706421-2-dovmurik@linux.ibm.com>

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

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