diff mbox series

[v4,1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

Message ID 20210930054915.13252-2-dovmurik@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series x86/sev: Measured Linux SEV guest with kernel/initrd/cmdline | expand

Commit Message

Dov Murik Sept. 30, 2021, 5:49 a.m. UTC
Add the sev_add_kernel_loader_hashes function to calculate the hashes of
the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
table area.  For this to work, OVMF must support an encrypted area to
place the data which is advertised via a special GUID in the OVMF reset
table.

The hashes of each of the files is calculated (or the string in the case
of the cmdline with trailing '\0' included).  Each entry in the hashes
table is GUID identified and since they're passed through the
sev_encrypt_flash interface, the hashes will be accumulated by the AMD
PSP measurement (SEV_LAUNCH_MEASURE).

Co-developed-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: James Bottomley <jejb@linux.ibm.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 target/i386/sev_i386.h |  12 ++++
 target/i386/sev-stub.c |   5 ++
 target/i386/sev.c      | 137 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 154 insertions(+)

Comments

Daniel P. Berrangé Sept. 30, 2021, 8:32 a.m. UTC | #1
On Thu, Sep 30, 2021 at 08:49:14AM +0300, Dov Murik wrote:
> Add the sev_add_kernel_loader_hashes function to calculate the hashes of
> the kernel/initrd/cmdline and fill a designated OVMF encrypted hash
> table area.  For this to work, OVMF must support an encrypted area to
> place the data which is advertised via a special GUID in the OVMF reset
> table.
> 
> The hashes of each of the files is calculated (or the string in the case
> of the cmdline with trailing '\0' included).  Each entry in the hashes
> table is GUID identified and since they're passed through the
> sev_encrypt_flash interface, the hashes will be accumulated by the AMD
> PSP measurement (SEV_LAUNCH_MEASURE).
> 
> Co-developed-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  target/i386/sev_i386.h |  12 ++++
>  target/i386/sev-stub.c |   5 ++
>  target/i386/sev.c      | 137 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 154 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
Dov Murik Sept. 30, 2021, 10:13 a.m. UTC | #2
On 30/09/2021 11:32, Daniel P. Berrangé wrote:
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 

Thanks!

-Dov
Tom Lendacky Oct. 18, 2021, 6:02 p.m. UTC | #3
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.

Is that anything we need to be concerned about?

Thanks,
Tom
Dov Murik Oct. 19, 2021, 6:18 a.m. UTC | #4
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.


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



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

Other ideas?


-Dov
Tom Lendacky Oct. 20, 2021, 3:26 p.m. UTC | #5
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
>
Brijesh Singh Oct. 27, 2021, 7:43 p.m. UTC | #6
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
>>
Dov Murik Oct. 28, 2021, 8:41 a.m. UTC | #7
On 27/10/2021 22:43, Brijesh Singh wrote:
> 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.

Yes, I managed to reproduce this.  With the OvmfPkgX64 build I see that
the GUID exists but base=0 and size=0.  That size should be illegal
anyway for QEMU to fill.


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

I'll prepare QEMU fixes:

1. (reported by Tom) when no SEV_HASH_TABLE_RV_GUID entry is found -
just warn and continue boot.
2. (reported by Brijesh) verify that base != 0 (supposedly GPA 0 is a
valid address, but I'm willing to take a chance here and not allow it)
and size is big enough for the hashes table structure+padding. If not,
warn and continue boot.

Separately I'll try to solve the issue in the other OVMF *.fdf's.



Thanks for reporting this.

-Dov


> 
> 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
>>>
Dov Murik Nov. 1, 2021, 10:28 a.m. UTC | #8
On 28/10/2021 11:41, Dov Murik wrote:
> 
> 
> On 27/10/2021 22:43, Brijesh Singh wrote:
>> 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.
> 
> Yes, I managed to reproduce this.  With the OvmfPkgX64 build I see that
> the GUID exists but base=0 and size=0.  That size should be illegal
> anyway for QEMU to fill.
> 
> 
>> 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 ?
> 
> I'll prepare QEMU fixes:
> 
> 1. (reported by Tom) when no SEV_HASH_TABLE_RV_GUID entry is found -
> just warn and continue boot.
> 2. (reported by Brijesh) verify that base != 0 (supposedly GPA 0 is a
> valid address, but I'm willing to take a chance here and not allow it)
> and size is big enough for the hashes table structure+padding. If not,
> warn and continue boot.

I submitted this small series [1].  Tom, Brijesh, I hope this solves
the issues you're experiencing and allows you to boot (and
displays a QEMU warning during launch).

[1] https://lore.kernel.org/qemu-devel/20211101102136.1706421-1-dovmurik@linux.ibm.com/


-Dov


> 
> Separately I'll try to solve the issue in the other OVMF *.fdf's.
> 
> 
> 
> Thanks for reporting this.
> 
> -Dov
> 
> 
>>
>> 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
>>>>
diff mbox series

Patch

diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
index ae6d840478..2afe108069 100644
--- a/target/i386/sev_i386.h
+++ b/target/i386/sev_i386.h
@@ -28,6 +28,17 @@ 
 #define SEV_POLICY_DOMAIN       0x10
 #define SEV_POLICY_SEV          0x20
 
+typedef struct SevKernelLoaderContext {
+    char *setup_data;
+    size_t setup_size;
+    char *kernel_data;
+    size_t kernel_size;
+    char *initrd_data;
+    size_t initrd_size;
+    char *cmdline_data;
+    size_t cmdline_size;
+} SevKernelLoaderContext;
+
 extern bool sev_es_enabled(void);
 extern uint64_t sev_get_me_mask(void);
 extern SevInfo *sev_get_info(void);
@@ -37,5 +48,6 @@  extern char *sev_get_launch_measurement(void);
 extern SevCapability *sev_get_capabilities(Error **errp);
 extern SevAttestationReport *
 sev_get_attestation_report(const char *mnonce, Error **errp);
+extern bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp);
 
 #endif
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
index 0227cb5177..d8e6583171 100644
--- a/target/i386/sev-stub.c
+++ b/target/i386/sev-stub.c
@@ -81,3 +81,8 @@  sev_get_attestation_report(const char *mnonce, Error **errp)
     error_setg(errp, "SEV is not available in this QEMU");
     return NULL;
 }
+
+bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
+{
+    g_assert_not_reached();
+}
diff --git a/target/i386/sev.c b/target/i386/sev.c
index 0b2c8f594a..8b98e184c2 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -23,6 +23,7 @@ 
 #include "qemu/base64.h"
 #include "qemu/module.h"
 #include "qemu/uuid.h"
+#include "crypto/hash.h"
 #include "sysemu/kvm.h"
 #include "sev_i386.h"
 #include "sysemu/sysemu.h"
@@ -83,6 +84,32 @@  typedef struct __attribute__((__packed__)) SevInfoBlock {
     uint32_t reset_addr;
 } SevInfoBlock;
 
+#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
+typedef struct QEMU_PACKED SevHashTableDescriptor {
+    /* SEV hash table area guest address */
+    uint32_t base;
+    /* SEV hash table area size (in bytes) */
+    uint32_t size;
+} SevHashTableDescriptor;
+
+/* hard code sha256 digest size */
+#define HASH_SIZE 32
+
+typedef struct QEMU_PACKED SevHashTableEntry {
+    QemuUUID guid;
+    uint16_t len;
+    uint8_t hash[HASH_SIZE];
+} SevHashTableEntry;
+
+typedef struct QEMU_PACKED SevHashTable {
+    QemuUUID guid;
+    uint16_t len;
+    SevHashTableEntry cmdline;
+    SevHashTableEntry initrd;
+    SevHashTableEntry kernel;
+    uint8_t padding[];
+} SevHashTable;
+
 static SevGuestState *sev_guest;
 static Error *sev_mig_blocker;
 
@@ -1071,6 +1098,116 @@  int sev_es_save_reset_vector(void *flash_ptr, uint64_t flash_size)
     return 0;
 }
 
+static const QemuUUID sev_hash_table_header_guid = {
+    .data = UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
+                    0xd4, 0x11, 0xfd, 0x21)
+};
+
+static const QemuUUID sev_kernel_entry_guid = {
+    .data = UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
+                    0x72, 0xd2, 0x04, 0x5b)
+};
+static const QemuUUID sev_initrd_entry_guid = {
+    .data = UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
+                    0x91, 0x69, 0x78, 0x1d)
+};
+static const QemuUUID sev_cmdline_entry_guid = {
+    .data = UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
+                    0x4d, 0x36, 0xab, 0x2a)
+};
+
+/*
+ * 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;
+    }
+    area = (SevHashTableDescriptor *)data;
+
+    /*
+     * Calculate hash of kernel command-line with the terminating null byte. If
+     * the user doesn't supply a command-line via -append, the 1-byte "\0" will
+     * be used.
+     */
+    hashp = cmdline_hash;
+    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
+                           ctx->cmdline_size, &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /*
+     * Calculate hash of initrd. If the user doesn't supply an initrd via
+     * -initrd, an empty buffer will be used (ctx->initrd_size == 0).
+     */
+    hashp = initrd_hash;
+    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
+                           ctx->initrd_size, &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /* Calculate hash of the kernel */
+    hashp = kernel_hash;
+    struct iovec iov[2] = {
+        { .iov_base = ctx->setup_data, .iov_len = ctx->setup_size },
+        { .iov_base = ctx->kernel_data, .iov_len = ctx->kernel_size }
+    };
+    if (qcrypto_hash_bytesv(QCRYPTO_HASH_ALG_SHA256, iov, ARRAY_SIZE(iov),
+                            &hashp, &hash_len, errp) < 0) {
+        return false;
+    }
+    assert(hash_len == HASH_SIZE);
+
+    /*
+     * Populate the hashes table in the guest's memory at the OVMF-designated
+     * area for the SEV hashes table
+     */
+    ht = qemu_map_ram_ptr(NULL, area->base);
+
+    ht->guid = sev_hash_table_header_guid;
+    ht->len = sizeof(*ht);
+
+    ht->cmdline.guid = sev_cmdline_entry_guid;
+    ht->cmdline.len = sizeof(ht->cmdline);
+    memcpy(ht->cmdline.hash, cmdline_hash, sizeof(ht->cmdline.hash));
+
+    ht->initrd.guid = sev_initrd_entry_guid;
+    ht->initrd.len = sizeof(ht->initrd);
+    memcpy(ht->initrd.hash, initrd_hash, sizeof(ht->initrd.hash));
+
+    ht->kernel.guid = sev_kernel_entry_guid;
+    ht->kernel.len = sizeof(ht->kernel);
+    memcpy(ht->kernel.hash, kernel_hash, sizeof(ht->kernel.hash));
+
+    /* When calling sev_encrypt_flash, the length has to be 16 byte aligned */
+    aligned_len = ROUND_UP(ht->len, 16);
+    if (aligned_len != ht->len) {
+        /* zero the excess data so the measurement can be reliably calculated */
+        memset(ht->padding, 0, aligned_len - ht->len);
+    }
+
+    if (sev_encrypt_flash((uint8_t *)ht, aligned_len, errp) < 0) {
+        return false;
+    }
+
+    return true;
+}
+
 static void
 sev_register_types(void)
 {