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
>>
next prev parent 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 \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).