Re: [PATCH v2 1/2] sev/i386: Introduce sev_add_kernel_loader_hashes for measured linux boot

[Dov Murik]

I found an issue with this patch when no -initrd is passed, see below.



On 21/06/2021 22:05, 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 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-stub.c |   5 ++
>  target/i386/sev.c      | 121 +++++++++++++++++++++++++++++++++++++++++
>  target/i386/sev_i386.h |  12 ++++
>  3 files changed, 138 insertions(+)
> 
> diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c
> index 0227cb5177..2b5e42d644 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(KernelLoaderContext *ctx, Error **errp)
> +{
> +    return false;
> +}
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 83df8c09f6..8e3f601bb6 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,29 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_HASH_TABLE_RV_GUID  "7255371f-3a3b-4b04-927b-1da6efa8d454"
> +typedef struct __attribute__((__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 __attribute__((__packed__)) SevHashTableEntry {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    uint8_t hash[HASH_SIZE];
> +} SevHashTableEntry;
> +
> +typedef struct __attribute__((__packed__)) SevHashTable {
> +    uint8_t guid[16];
> +    uint16_t len;
> +    SevHashTableEntry entries[];
> +} SevHashTable;
> +
>  static SevGuestState *sev_guest;
>  static Error *sev_mig_blocker;
>  
> @@ -1077,6 +1101,103 @@ int sev_es_save_reset_vector(void *flash_ptr, 
> uint64_t flash_size)
>      return 0;
>  }
>  
> +static const uint8_t sev_hash_table_header_guid[] =
> +        UUID_LE(0x9438d606, 0x4f22, 0x4cc9, 0xb4, 0x79, 0xa7, 0x93,
> +                0xd4, 0x11, 0xfd, 0x21);
> +
> +static const uint8_t sev_kernel_entry_guid[] =
> +        UUID_LE(0x4de79437, 0xabd2, 0x427f, 0xb8, 0x35, 0xd5, 0xb1,
> +                0x72, 0xd2, 0x04, 0x5b);
> +static const uint8_t sev_initrd_entry_guid[] =
> +        UUID_LE(0x44baf731, 0x3a2f, 0x4bd7, 0x9a, 0xf1, 0x41, 0xe2,
> +                0x91, 0x69, 0x78, 0x1d);
> +static const uint8_t sev_cmdline_entry_guid[] =
> +        UUID_LE(0x97d02dd8, 0xbd20, 0x4c94, 0xaa, 0x78, 0xe7, 0x71,
> +                0x4d, 0x36, 0xab, 0x2a);
> +
> +static void fill_sev_hash_table_entry(SevHashTableEntry *e, const uint8_t 
> *guid,
> +                                      const uint8_t *hash, size_t hash_len)
> +{
> +    memcpy(e->guid, guid, sizeof(e->guid));
> +    e->len = sizeof(*e);
> +    memcpy(e->hash, hash, hash_len);
> +}
> +
> +/*
> + * 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(KernelLoaderContext *ctx, Error **errp)
> +{
> +    uint8_t *data;
> +    SevHashTableDescriptor *area;
> +    SevHashTable *ht;
> +    SevHashTableEntry *e;
> +    uint8_t hash_buf[HASH_SIZE];
> +    uint8_t *hash = hash_buf;
> +    size_t hash_len = sizeof(hash_buf);
> +    int ht_index = 0;
> +    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;
> +
> +    ht = qemu_map_ram_ptr(NULL, area->base);
> +
> +    /* Populate the hashes table header */
> +    memcpy(ht->guid, sev_hash_table_header_guid, sizeof(ht->guid));
> +    ht->len = sizeof(*ht);
> +
> +    /* Calculate hash of kernel command-line */
> +    if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->cmdline_data,
> +                           ctx->cmdline_size,
> +                           &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }
> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_cmdline_entry_guid, hash, hash_len);
> +
> +    /* Calculate hash of initrd */
> +    if (ctx->initrd_data) {
> +        if (qcrypto_hash_bytes(QCRYPTO_HASH_ALG_SHA256, ctx->initrd_data,
> +                               ctx->initrd_size, &hash, &hash_len, errp) < 
> 0) {
> +            return false;
> +        }
> +        e = &ht->entries[ht_index++];
> +        fill_sev_hash_table_entry(e, sev_initrd_entry_guid, hash, hash_len);
> +    }


As can be seen, we conditionally add the hash table entry for initrd,
depending on whether the user passed the -initrd switch to QEMU.
However, in the OVMF side, initrd hash is *always* verified; in this
case it will be verified against a the hash of an empty buffer (sha256
starts with e3b0c44298fc...).  Since QEMU (this patch) doesn't add that
entry in the hashes table, we get an an access denied error in OVMF.

I think OVMF is doing the correct thing: the Guest Owner wishes to have
no initrd, and OVMF should verify that the initrd it reads is indeed
empty.  Therefore the measured SEV hashes table should include an entry
for initrd with the sha256 of an empty buffer.

(James - do you agree?)


I think we should remove this condition, and always fill the three
entries in the hashes table: cmdline, initrd, and kernel.  Here's the
treatment for missing arguments:

* No -cmdline: use the hash of the 1-byte "\0" string (this is already
  the case, no code changes needed).
* No -initrd: use the hash of the empty buffer.
* No -kernel: this code will not be reached at all (this is already the
  case, no code changes needed).


This leads to another possible modification: instead of array of
SevHashTableEntry entries[] in SevHashTable, we can have:

typedef struct __attribute__((__packed__)) SevHashTable {
    uint8_t guid[16];
    uint16_t len;
    SevHashTableEntry cmdline;
    SevHashTableEntry initrd;
    SevHashTableEntry kernel;
    uint8_t zero_padding[];  /* padded to 16-byte boundary */
} SevHashTable;


This could clear up things here; keeping the GUID+length in each entry
leaves the future option for a more modular table.

It will also make the order of entries clear, which is required for
calculating the expected measurement on the Guest Owner side.

This also means that the total length of the entire table is known in
advance (168 bytes; aligned to 176 bytes with zero padding).  Not sure
if this helps.


-Dov


> +
> +    /* Calculate hash of the kernel */
> +    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, 2,
> +                            &hash, &hash_len, errp) < 0) {
> +        return false;
> +    }
> +    e = &ht->entries[ht_index++];
> +    fill_sev_hash_table_entry(e, sev_kernel_entry_guid, hash, hash_len);
> +
> +    /* now we have all the possible entries, finalize the hashes table */
> +    ht->len += ht_index * sizeof(*e);
> +    /* SEV len 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->entries[ht_index], 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)
>  {
> diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h
> index ae6d840478..deb3eec409 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 KernelLoaderContext {
> +    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;
> +} KernelLoaderContext;
> +
>  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(KernelLoaderContext *ctx, Error 
> **errp);
>  
>  #endif
>