qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Brijesh Singh <brijesh.singh@amd.com>
To: Tom Lendacky <thomas.lendacky@amd.com>,
	Dov Murik <dovmurik@linux.ibm.com>,
	qemu-devel@nongnu.org
Cc: brijesh.singh@amd.com, "James Bottomley" <jejb@linux.ibm.com>,
	"Daniel P. Berrangé" <berrange@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	"Marcel Apfelbaum" <marcel.apfelbaum@gmail.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Dr. David Alan Gilbert" <dgilbert@redhat.com>,
	"Richard Henderson" <richard.henderson@linaro.org>,
	"Eduardo Habkost" <ehabkost@redhat.com>,
	"Connor Kuehl" <ckuehl@redhat.com>,
	"Philippe Mathieu-Daudé" <philmd@redhat.com>,
	"Laszlo Ersek" <lersek@redhat.com>,
	"Ashish Kalra" <ashish.kalra@amd.com>,
	"Tobin Feldman-Fitzthum" <tobin@linux.ibm.com>,
	"Jim Cadden" <jcadden@ibm.com>,
	"Hubertus Franke" <frankeh@us.ibm.com>
Subject: Re: [PATCH v4 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot
Date: Wed, 27 Oct 2021 14:43:09 -0500	[thread overview]
Message-ID: <001dd81a-282d-c307-a657-e228480d4af3@amd.com> (raw)
In-Reply-To: <c94afa49-fdfc-0f33-a4d8-2a9c30dd7812@amd.com>

Hi Dov,

Sorry for coming a bit late on it but I am seeing another issue with 
this patch. The hash build logic looks for a SEV_HASH_TABLE_RV_GUID in 
the GUID list. If found, it uses the base address to store the hash'es. 
Looking at the OVMF, it seems that base address for this GUID is zero. 
It seems that by default the Base Address is non-zero for the AmdSev 
Package build only.

Can we add a check in the sev_add_kernel_loader_hashes() to verify that 
base address is non-zero and at the same time improve OVMF to update 
*.fdf to reserve this page in the MEMFD ?

Thanks
Brijesh

On 10/20/21 10:26 AM, Tom Lendacky wrote:
> On 10/19/21 1:18 AM, Dov Murik wrote:
>> On 18/10/2021 21:02, Tom Lendacky wrote:
>>> On 9/30/21 12:49 AM, Dov Murik wrote:
>>>
>>> ...
>>>
>>>> +/*
>>>> + * Add the hashes of the linux kernel/initrd/cmdline to an encrypted
>>>> guest page
>>>> + * which is included in SEV's initial memory measurement.
>>>> + */
>>>> +bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error
>>>> **errp)
>>>> +{
>>>> +    uint8_t *data;
>>>> +    SevHashTableDescriptor *area;
>>>> +    SevHashTable *ht;
>>>> +    uint8_t cmdline_hash[HASH_SIZE];
>>>> +    uint8_t initrd_hash[HASH_SIZE];
>>>> +    uint8_t kernel_hash[HASH_SIZE];
>>>> +    uint8_t *hashp;
>>>> +    size_t hash_len = HASH_SIZE;
>>>> +    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");
>>>> +        return false;
>>>> +    }
>>>
>>> This breaks backwards compatibility with an older OVMF image. Any older
>>> OVMF image with SEV support that doesn't have the hash table GUID will
>>> now fail to boot using -kernel/-initrd/-append, where it used to be able
>>> to boot before.
>>>
>>
>>
>> Thanks Tom for noticing this.
>>
>> Just so we're on the same page: this patch is already merged.
> 
> Right, just not in a release, yet.
> 
>>
>>
>> We're dealing with a scenario of launching a guest with SEV enabled and
>> with -kernel.  The behaviours are:
>>
>>
>> A. With current QEMU:
>>
>> A1. New AmdSev OVMF build: OVMF will verify the hashes and boot 
>> correctly.
>> A2. New Generic OvmfPkgX64 build: No verification but will boot 
>> correctly.
>>
>> A3. Old AmdSev OVMF build: QEMU aborts the launch because there's no
>> hash table GUID.
>> A4. Old Generic OvmfPkgX64 build: QEMU aborts the launch because there's
>> no hash table GUID.
>>
>>
>> B. With older QEMU (before this patch was merged):
>>
>> B1. New AmdSev OVMF build: OVMF will try to verify the hashes but they
>> are not populated; boot aborted.
>> B2. New Generic OvmfPkgX64 build: No verification but will boot 
>> correctly.
>>
>> B3. Old AmdSev OVMF build: OVMF aborts the launch because -kernel is not
>> supported at all.
>> B4. Old Generic OvmfPkgX64 build: No verification but will boot 
>> correctly.
>>
>>
>> So the problem you are raising is scenario A4 (as opposed to previous
>> behaviour B4).
> 
> Correct, scenario A4.
> 
>>
>>
>>
>>> Is that anything we need to be concerned about?
>>>
>>
>> Possible solutions:
>>
>> 1. Do nothing. For users that encounter this: tell them to upgrade OVMF.
>> 2. Modify the code: remove the line: error_setg(errp, "SEV: kernel
>> specified but OVMF has no hash table guid")
>>
>> I think that option 2 will not degrade security *if* the Guest Owner
>> verifies the measurement (which is mandatory anyway; otherwise the
>> untrusted host can replace OVMF with a "malicious" version that doesn't
>> verify the hashes). Skipping silently might make debugging a bit harder.
>> Maybe we can print a warning and return, and then the guest launch will
>> continue?
> 
> That sounds like it might be the best approach if there are no security 
> concerns. I agree with printing a message, either informational or 
> warning is ok by me.
> 
> Lets see if anyone else has some thoughts/ideas.
> 
> Thanks,
> Tom
> 
>>
>> Other ideas?
>>
>>
>> -Dov
>>


  reply	other threads:[~2021-10-27 19:53 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-30  5:49 [PATCH v4 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline Dov Murik
2021-09-30  5:49 ` [PATCH v4 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot Dov Murik
2021-09-30  8:32   ` Daniel P. Berrangé
2021-09-30 10:13     ` Dov Murik
2021-10-18 18:02   ` Tom Lendacky
2021-10-19  6:18     ` Dov Murik
2021-10-20 15:26       ` Tom Lendacky
2021-10-27 19:43         ` Brijesh Singh [this message]
2021-10-28  8:41           ` Dov Murik
2021-11-01 10:28             ` Dov Murik
2021-09-30  5:49 ` [PATCH v4 2/2] x86/sev: generate SEV kernel loader hashes in x86_load_linux Dov Murik
2021-10-04  8:03 ` [PATCH v4 0/2] x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline Paolo Bonzini
2021-10-04 17:23   ` Dov Murik

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=001dd81a-282d-c307-a657-e228480d4af3@amd.com \
    --to=brijesh.singh@amd.com \
    --cc=ashish.kalra@amd.com \
    --cc=berrange@redhat.com \
    --cc=ckuehl@redhat.com \
    --cc=dgilbert@redhat.com \
    --cc=dovmurik@linux.ibm.com \
    --cc=ehabkost@redhat.com \
    --cc=frankeh@us.ibm.com \
    --cc=jcadden@ibm.com \
    --cc=jejb@linux.ibm.com \
    --cc=lersek@redhat.com \
    --cc=marcel.apfelbaum@gmail.com \
    --cc=mst@redhat.com \
    --cc=pbonzini@redhat.com \
    --cc=philmd@redhat.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    --cc=thomas.lendacky@amd.com \
    --cc=tobin@linux.ibm.com \
    --subject='Re: [PATCH v4 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).