Re: [RFC PATCH v2 07/12] i386/sev: populate secrets and cpuid page and finalize the SNP launch

[Dov Murik]

Hi Michael,

On 27/08/2021 1:26, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> During the SNP guest launch sequence, a special secrets and cpuid page
> needs to be populated by the SEV-SNP firmware. 

Just to be clear: these are two distinct pages.  I suggest rephrasing to
"... a special secrets page and a special cpuid page need to be
populated ..." (or something along these lines).

> The secrets page contains
> the VM Platform Communication Key (VMPCKs) used by the guest to send and
> receive secure messages to the PSP. And CPUID page will contain the CPUID
> value filtered through the PSP.
> 
> The guest BIOS (OVMF) reserves these pages in MEMFD and location of it
> is available through the SNP boot block GUID. While finalizing the guest
> boot flow, lookup for the boot block and call the SNP_LAUNCH_UPDATE
> command to populate secrets and cpuid pages.
> 
> In order to support early boot code, the OVMF may ask hypervisor to
> request the pre-validation of certain memory range. If such range is
> present the call SNP_LAUNCH_UPDATE command to validate those address
> range without affecting the measurement. See the SEV-SNP specification
> for further details.
> 
> Finally, call the SNP_LAUNCH_FINISH to finalize the guest boot.
> 
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>  target/i386/sev.c        | 189 ++++++++++++++++++++++++++++++++++++++-
>  target/i386/trace-events |   2 +
>  2 files changed, 189 insertions(+), 2 deletions(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 867c0cb457..0009c93d28 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -33,6 +33,7 @@
>  #include "monitor/monitor.h"
>  #include "exec/confidential-guest-support.h"
>  #include "hw/i386/pc.h"
> +#include "qemu/range.h"
>  
>  #define TYPE_SEV_COMMON "sev-common"
>  OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
> @@ -107,6 +108,19 @@ typedef struct __attribute__((__packed__)) SevInfoBlock {
>      uint32_t reset_addr;
>  } SevInfoBlock;
>  
> +#define SEV_SNP_BOOT_BLOCK_GUID "bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9"
> +typedef struct __attribute__((__packed__)) SevSnpBootInfoBlock {
> +    /* Prevalidate range address */
> +    uint32_t pre_validated_start;
> +    uint32_t pre_validated_end;
> +    /* Secrets page address */
> +    uint32_t secrets_addr;
> +    uint32_t secrets_len;

Just curious: isn't secrets_len always 4096? (same for cpuid_len)
Though it might be a good future proofing to have a length field.


> +    /* CPUID page address */
> +    uint32_t cpuid_addr;
> +    uint32_t cpuid_len;
> +} SevSnpBootInfoBlock;
> +
>  static Error *sev_mig_blocker;
>  
>  static const char *const sev_fw_errlist[] = {
> @@ -1086,6 +1100,162 @@ static Notifier sev_machine_done_notify = {
>      .notify = sev_launch_get_measure,
>  };
>  
> +static int
> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
> +{
> +    void *hva;
> +    MemoryRegion *mr = NULL;
> +    SevSnpGuestState *sev_snp_guest =
> +        SEV_SNP_GUEST(MACHINE(qdev_get_machine())->cgs);
> +
> +    hva = gpa2hva(&mr, hwaddr, size, NULL);
> +    if (!hva) {
> +        error_report("SEV-SNP failed to get HVA for GPA 0x%x", hwaddr);
> +        return 1;
> +    }
> +
> +    return sev_snp_launch_update(sev_snp_guest, hwaddr, hva, size, type);
> +}
> +
> +static bool
> +detect_first_overlap(uint64_t start, uint64_t end, Range *range_list,
> +                     size_t range_count, Range *overlap_range)
> +{
> +    int i;
> +    bool overlap = false;
> +    Range new;
> +
> +    assert(overlap_range);

Also:
assert(end >= start)
assert(range_count == 0 || range_list)

> +    range_make_empty(overlap_range);
> +    range_init_nofail(&new, start, end - start + 1);
> +
> +    for (i = 0; i < range_count; i++) {
> +        if (range_overlaps_range(&new, &range_list[i]) &&
> +            (range_is_empty(overlap_range) ||
> +             range_lob(&range_list[i]) < range_lob(overlap_range))) {
> +            *overlap_range = range_list[i];
> +            overlap = true;
> +        }
> +    }
> +
> +    return overlap;
> +}
> +
> +static void snp_ovmf_boot_block_setup(void)
> +{
> +    SevSnpBootInfoBlock *info;
> +    uint32_t start, end, sz;
> +    int ret;
> +    Range validated_ranges[2];
> +
> +    /*
> +     * Extract the SNP boot block for the SEV-SNP guests by locating the
> +     * SNP_BOOT GUID. The boot block contains the information such as 
> location
> +     * of secrets and CPUID page, additionaly it may contain the range of
> +     * memory that need to be pre-validated for the boot.
> +     */
> +    if (!pc_system_ovmf_table_find(SEV_SNP_BOOT_BLOCK_GUID,
> +        (uint8_t **)&info, NULL)) {

Fix indent of arguments.


> +        error_report("SEV-SNP: failed to find the SNP boot block");
> +        exit(1);
> +    }
> +
> +    trace_kvm_sev_snp_ovmf_boot_block_info(info->secrets_addr,
> +                                           info->secrets_len, 
> info->cpuid_addr,
> +                                           info->cpuid_len,
> +                                           info->pre_validated_start,
> +                                           info->pre_validated_end);
> +
> +    /* Populate the secrets page */
> +    ret = sev_snp_launch_update_gpa(info->secrets_addr, info->secrets_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_SECRETS);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert secret page GPA 0x%x",
> +                     info->secrets_addr);
> +        exit(1);
> +    }
> +
> +    /* Populate the cpuid page */
> +    ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len,
> +                                    KVM_SEV_SNP_PAGE_TYPE_CPUID);
> +    if (ret) {
> +        error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x",
> +                     info->cpuid_addr);
> +        exit(1);
> +    }
> +
> +    /*
> +     * Pre-validate the range using the LAUNCH_UPDATE_DATA, if the
> +     * pre-validation range contains the CPUID and Secret page GPA then skip
> +     * it. This is because SEV-SNP firmware pre-validates those pages as part
> +     * of adding secrets and cpuid LAUNCH_UPDATE type.
> +     */
> +    range_init_nofail(&validated_ranges[0], info->secrets_addr, 
> info->secrets_len);
> +    range_init_nofail(&validated_ranges[1], info->cpuid_addr, 
> info->cpuid_len);
> +    start = info->pre_validated_start;
> +    end = info->pre_validated_end;
> +
> +    while (start < end) {
> +        Range overlap_range;
> +
> +        /* Check if the requested range overlaps with Secrets and CPUID page 
> */
> +        if (detect_first_overlap(start, end, validated_ranges, 2,

Replace the literal 2 with ARRAY_SIZE(validated_ranges).


> +                                 &overlap_range)) {
> +            if (start < range_lob(&overlap_range)) {
> +                sz = range_lob(&overlap_range) - start;
> +                if (sev_snp_launch_update_gpa(start, sz,
> +                    KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {

Fix indent of arguments (if possible).

> +                    error_report("SEV-SNP: failed to validate gpa 0x%x sz 
> %d",
> +                                 start, sz);
> +                    exit(1);
> +                }
> +            }
> +
> +            start = range_upb(&overlap_range) + 1;
> +            continue;
> +        }
> +
> +        /* Validate the remaining range */
> +        if (sev_snp_launch_update_gpa(start, end - start,

I think the second argument should be:    end - start + 1 .

Consider start=0x8000 end=0x8fff (inclusive). In this case you want to
validate exactly 0x1000 bytes starting at 0x8000.  So the size should be
0x8fff - 0x8000 + 1.

I assume all this doesn't matter for the underlying calls which operate
at whole pages anyway (are there proper asserts in sev_snp_launch_update
(or in KVM) that verify that start and sz are page-size-aligned?)



> +            KVM_SEV_SNP_PAGE_TYPE_UNMEASURED)) {

Fix indent of arguments.


> +            error_report("SEV-SNP: failed to validate gpa 0x%x sz %d",
> +                         start, end - start);
> +            exit(1);
> +        }
> +
> +        start = end;
> +    }
> +}
> +
> +static void
> +sev_snp_launch_finish(SevSnpGuestState *sev_snp)
> +{
> +    int ret, error;
> +    Error *local_err = NULL;
> +    struct kvm_sev_snp_launch_finish *finish = &sev_snp->kvm_finish_conf;
> +
> +    trace_kvm_sev_snp_launch_finish();
> +    ret = sev_ioctl(SEV_COMMON(sev_snp)->sev_fd, KVM_SEV_SNP_LAUNCH_FINISH, 
> finish, &error);
> +    if (ret) {
> +        error_report("%s: SNP_LAUNCH_FINISH ret=%d fw_error=%d '%s'",
> +                     __func__, ret, error, fw_error_to_str(error));
> +        exit(1);
> +    }
> +
> +    sev_set_guest_state(SEV_COMMON(sev_snp), SEV_STATE_RUNNING);
> +
> +    /* add migration blocker */
> +    error_setg(&sev_mig_blocker,
> +               "SEV: Migration is not implemented");
> +    ret = migrate_add_blocker(sev_mig_blocker, &local_err);
> +    if (local_err) {
> +        error_report_err(local_err);
> +        error_free(sev_mig_blocker);
> +        exit(1);
> +    }
> +}
> +
> +
>  static void
>  sev_launch_finish(SevGuestState *sev_guest)
>  {
> @@ -1121,7 +1291,12 @@ sev_vm_state_change(void *opaque, bool running, 
> RunState state)
>  
>      if (running) {
>          if (!sev_check_state(sev_common, SEV_STATE_RUNNING)) {
> -            sev_launch_finish(SEV_GUEST(sev_common));
> +            if (sev_snp_enabled()) {
> +                snp_ovmf_boot_block_setup();
> +                sev_snp_launch_finish(SEV_SNP_GUEST(sev_common));
> +            } else {
> +                sev_launch_finish(SEV_GUEST(sev_common));
> +            }
>          }
>      }
>  }
> @@ -1236,7 +1411,17 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error 
> **errp)
>      }
>  
>      ram_block_notifier_add(&sev_ram_notifier);
> -    qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +
> +    /*
> +     * The machine done notify event is used by the SEV guest to get the
> +     * measurement of the encrypted images. When SEV-SNP is enabled then
> +     * measurement is part of the attestation report and the measurement
> +     * command does not exist. So skip registering the notifier.
> +     */
> +    if (!sev_snp_enabled()) {
> +        qemu_add_machine_init_done_notifier(&sev_machine_done_notify);
> +    }
> +
>      qemu_add_vm_change_state_handler(sev_vm_state_change, sev_common);
>  
>      cgs->ready = true;
> diff --git a/target/i386/trace-events b/target/i386/trace-events
> index 0c2d250206..db91287439 100644
> --- a/target/i386/trace-events
> +++ b/target/i386/trace-events
> @@ -13,3 +13,5 @@ kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t 
> secret, int len) "hpa
>  kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s 
> data %s"
>  kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
>  kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 
> 0x%" PRIx64 " type %d"
> +kvm_sev_snp_launch_finish(void) ""
> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, 
> uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x 
> cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"

In this trace format string you use the notation A+B to indicate addr=A
 len=B.  But for the pre-validated range the arguments are 'start' and
'end' (and not 'addr' and 'len'), so I suggest choosing a different
notation to log that range.

-Dov