| Message ID | 20210826222627.3556-12-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) support | expand |
Hi Michael, On 27/08/2021 1:26, Michael Roth wrote: > SEV-SNP firmware allows a special guest page to be populated with a > table of guest CPUID values so that they can be validated through > firmware before being loaded into encrypted guest memory where they can > be used in place of hypervisor-provided values[1]. > > As part of SEV-SNP guest initialization, use this process to validate > the CPUID entries reported by KVM_GET_CPUID2 prior to initial guest > start. > > [1]: SEV SNP Firmware ABI Specification, Rev. 0.8, 8.13.2.6 > > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > target/i386/sev.c | 146 +++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 143 insertions(+), 3 deletions(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 0009c93d28..72a6146295 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -153,6 +153,36 @@ static const char *const sev_fw_errlist[] = { > > #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) > > +/* <linux/kvm.h> doesn't expose this, so re-use the max from kvm.c */ > +#define KVM_MAX_CPUID_ENTRIES 100 > + > +typedef struct KvmCpuidInfo { > + struct kvm_cpuid2 cpuid; > + struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES]; > +} KvmCpuidInfo; > + > +#define SNP_CPUID_FUNCTION_MAXCOUNT 64 > +#define SNP_CPUID_FUNCTION_UNKNOWN 0xFFFFFFFF > + > +typedef struct { > + uint32_t eax_in; > + uint32_t ecx_in; > + uint64_t xcr0_in; > + uint64_t xss_in; > + uint32_t eax; > + uint32_t ebx; > + uint32_t ecx; > + uint32_t edx; > + uint64_t reserved; > +} __attribute__((packed)) SnpCpuidFunc; > + > +typedef struct { > + uint32_t count; > + uint32_t reserved1; > + uint64_t reserved2; > + SnpCpuidFunc entries[SNP_CPUID_FUNCTION_MAXCOUNT]; > +} __attribute__((packed)) SnpCpuidInfo; > + > static int > sev_ioctl(int fd, int cmd, void *data, int *error) > { > @@ -1141,6 +1171,117 @@ detect_first_overlap(uint64_t start, uint64_t end, Range *range_list, > return overlap; > } > > +static int > +sev_snp_cpuid_info_fill(SnpCpuidInfo *snp_cpuid_info, > + const KvmCpuidInfo *kvm_cpuid_info) > +{ > + size_t i; > + > + memset(snp_cpuid_info, 0, sizeof(*snp_cpuid_info)); > + > + for (i = 0; kvm_cpuid_info->entries[i].function != 0xFFFFFFFF; i++) { Maybe iterate only while i < kvm_cpuid_info.cpuid.nent ? > + const struct kvm_cpuid_entry2 *kvm_cpuid_entry; > + SnpCpuidFunc *snp_cpuid_entry; > + > + kvm_cpuid_entry = &kvm_cpuid_info->entries[i]; > + snp_cpuid_entry = &snp_cpuid_info->entries[i]; There's no explicit check that i < KVM_MAX_CPUID_ENTRIES and i < SNP_CPUID_FUNCTION_MAXCOUNT. The !=0xFFFFFFFF condition might protect against this but this is not really clear (the memset to 0xFF is done in another function). Since KVM_MAX_CPUID_ENTRIES is 100 and SNP_CPUID_FUNCTION_MAXCOUNT is 64, it seems possible that i will be 65 for example and then snp_cpuid_info->entries[i] is an out-of-bounds read access. > + > + snp_cpuid_entry->eax_in = kvm_cpuid_entry->function; > + if (kvm_cpuid_entry->flags == KVM_CPUID_FLAG_SIGNIFCANT_INDEX) { > + snp_cpuid_entry->ecx_in = kvm_cpuid_entry->index; > + } > + snp_cpuid_entry->eax = kvm_cpuid_entry->eax; > + snp_cpuid_entry->ebx = kvm_cpuid_entry->ebx; > + snp_cpuid_entry->ecx = kvm_cpuid_entry->ecx; > + snp_cpuid_entry->edx = kvm_cpuid_entry->edx; > + > + if (snp_cpuid_entry->eax_in == 0xD && > + (snp_cpuid_entry->ecx_in == 0x0 || snp_cpuid_entry->ecx_in == 0x1)) { > + snp_cpuid_entry->ebx = 0x240; > + } Can you please add a comment explaining this special case? > + } > + > + if (i > SNP_CPUID_FUNCTION_MAXCOUNT) { This can be checked at the top (before the for loop): compare kvm_cpuid_info.cpuid.nent with SNP_CPUID_FUNCTION_MAXCOUNT. > + error_report("SEV-SNP: CPUID count '%lu' exceeds max '%u'", > + i, SNP_CPUID_FUNCTION_MAXCOUNT); > + return -1; > + } > + > + snp_cpuid_info->count = i; > + > + return 0; > +} > + > +static void > +sev_snp_cpuid_report_mismatches(SnpCpuidInfo *old, > + SnpCpuidInfo *new) > +{ > + size_t i; > + Add check that new->count == old->count. > + for (i = 0; i < old->count; i++) { > + SnpCpuidFunc *old_func, *new_func; > + > + old_func = &old->entries[i]; > + new_func = &new->entries[i]; > + > + if (memcmp(old_func, new_func, sizeof(SnpCpuidFunc))) { Maybe clearer: if (*old_func != *new_func) ... > + error_report("SEV-SNP: CPUID validation failed for function %x, index: %x.\n" Add "0x" prefixes before printing hex values (%x), otherwise we might have confusing outputs such as "failed for function 13, index: 25" which is unclear whether it's decimal or hex. > + "provided: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x\n" > + "expected: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x", > + old_func->eax_in, old_func->ecx_in, > + old_func->eax, old_func->ebx, old_func->ecx, old_func->edx, > + new_func->eax, new_func->ebx, new_func->ecx, new_func->edx); > + } > + } > +} > + > +static int > +sev_snp_launch_update_cpuid(uint32_t cpuid_addr, uint32_t cpuid_len) > +{ > + KvmCpuidInfo kvm_cpuid_info; > + SnpCpuidInfo snp_cpuid_info; > + CPUState *cs = first_cpu; > + MemoryRegion *mr = NULL; > + void *snp_cpuid_hva; > + int ret; > + > + snp_cpuid_hva = gpa2hva(&mr, cpuid_addr, cpuid_len, NULL); > + if (!snp_cpuid_hva) { > + error_report("SEV-SNP: unable to access CPUID memory range at GPA %d", > + cpuid_addr); > + return 1; > + } I think that moving this section just before the memcpy(snp_cpuid_hva, ...) below would make the flow of this function clearer to the reader (no functional difference, I believe). > + > + /* get the cpuid list from KVM */ > + memset(&kvm_cpuid_info.entries, 0xFF, > + KVM_MAX_CPUID_ENTRIES * sizeof(struct kvm_cpuid_entry2)); The third argument can be: sizeof(kvm_cpuid_info.entries) > + kvm_cpuid_info.cpuid.nent = KVM_MAX_CPUID_ENTRIES; > + > + ret = kvm_vcpu_ioctl(cs, KVM_GET_CPUID2, &kvm_cpuid_info); > + if (ret) { > + error_report("SEV-SNP: unable to query CPUID values for CPU: '%s'", > + strerror(-ret)); Missing return 1 or exit(1) here? -Dov > + } > + > + ret = sev_snp_cpuid_info_fill(&snp_cpuid_info, &kvm_cpuid_info); > + if (ret) { > + error_report("SEV-SNP: failed to generate CPUID table information"); > + exit(1); > + } > + > + memcpy(snp_cpuid_hva, &snp_cpuid_info, sizeof(snp_cpuid_info)); Before memcpy, maybe add sanity test (assert?) that sizeof(snp_cpuid_info) <= cpuid_len . > + > + ret = sev_snp_launch_update_gpa(cpuid_addr, cpuid_len, > + KVM_SEV_SNP_PAGE_TYPE_CPUID); > + if (ret) { > + sev_snp_cpuid_report_mismatches(&snp_cpuid_info, snp_cpuid_hva); > + error_report("SEV-SNP: failed update CPUID page"); > + exit(1); > + } > + > + return 0; > +} > + > static void snp_ovmf_boot_block_setup(void) > { > SevSnpBootInfoBlock *info; > @@ -1176,10 +1317,9 @@ static void snp_ovmf_boot_block_setup(void) > } > > /* Populate the cpuid page */ > - ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len, > - KVM_SEV_SNP_PAGE_TYPE_CPUID); > + ret = sev_snp_launch_update_cpuid(info->cpuid_addr, info->cpuid_len); > if (ret) { > - error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x", > + error_report("SEV-SNP: failed to populate cpuid tables GPA 0x%x", > info->cpuid_addr); > exit(1); > } >
On Sun, Sep 05, 2021 at 01:02:12PM +0300, Dov Murik wrote: > Hi Michael, > > On 27/08/2021 1:26, Michael Roth wrote: > > SEV-SNP firmware allows a special guest page to be populated with a > > table of guest CPUID values so that they can be validated through > > firmware before being loaded into encrypted guest memory where they can > > be used in place of hypervisor-provided values[1]. > > > > As part of SEV-SNP guest initialization, use this process to validate > > the CPUID entries reported by KVM_GET_CPUID2 prior to initial guest > > start. > > > > [1]: SEV SNP Firmware ABI Specification, Rev. 0.8, 8.13.2.6 > > > > Signed-off-by: Michael Roth <michael.roth@amd.com> > > --- > > target/i386/sev.c | 146 +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 143 insertions(+), 3 deletions(-) > > > > diff --git a/target/i386/sev.c b/target/i386/sev.c > > index 0009c93d28..72a6146295 100644 > > --- a/target/i386/sev.c > > +++ b/target/i386/sev.c > > @@ -153,6 +153,36 @@ static const char *const sev_fw_errlist[] = { > > > > #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) > > > > +/* <linux/kvm.h> doesn't expose this, so re-use the max from kvm.c */ > > +#define KVM_MAX_CPUID_ENTRIES 100 > > + > > +typedef struct KvmCpuidInfo { > > + struct kvm_cpuid2 cpuid; > > + struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES]; > > +} KvmCpuidInfo; > > + > > +#define SNP_CPUID_FUNCTION_MAXCOUNT 64 > > +#define SNP_CPUID_FUNCTION_UNKNOWN 0xFFFFFFFF > > + > > +typedef struct { > > + uint32_t eax_in; > > + uint32_t ecx_in; > > + uint64_t xcr0_in; > > + uint64_t xss_in; > > + uint32_t eax; > > + uint32_t ebx; > > + uint32_t ecx; > > + uint32_t edx; > > + uint64_t reserved; > > +} __attribute__((packed)) SnpCpuidFunc; > > + > > +typedef struct { > > + uint32_t count; > > + uint32_t reserved1; > > + uint64_t reserved2; > > + SnpCpuidFunc entries[SNP_CPUID_FUNCTION_MAXCOUNT]; > > +} __attribute__((packed)) SnpCpuidInfo; > > + > > static int > > sev_ioctl(int fd, int cmd, void *data, int *error) > > { > > @@ -1141,6 +1171,117 @@ detect_first_overlap(uint64_t start, uint64_t end, Range *range_list, > > return overlap; > > } > > > > +static int > > +sev_snp_cpuid_info_fill(SnpCpuidInfo *snp_cpuid_info, > > + const KvmCpuidInfo *kvm_cpuid_info) > > +{ > > + size_t i; > > + > > + memset(snp_cpuid_info, 0, sizeof(*snp_cpuid_info)); > > + > > + for (i = 0; kvm_cpuid_info->entries[i].function != 0xFFFFFFFF; i++) { > > Maybe iterate only while i < kvm_cpuid_info.cpuid.nent ? Unfortunately kvm_vcpu_ioctl_get_cpuid2() in kernel only checks kvm_cpuid_info.cpuid.nent as an input to verify there's enough room in in kvm_cpuid_info.cpuid.entries array to copy the values over. It doesn't update kvm_cpuid_info.cpuid.nent to indicate how many entries are actually present, so I've been relying on the 0xFFFFFFFF marker stuff. An alternative approach is to keep incrementing cpuid.nent until the KVM ioctl stops reporting E2BIG, I think the KVM selftests take this approach as well so that's probably the way to go. > > > + const struct kvm_cpuid_entry2 *kvm_cpuid_entry; > > + SnpCpuidFunc *snp_cpuid_entry; > > + > > + kvm_cpuid_entry = &kvm_cpuid_info->entries[i]; > > + snp_cpuid_entry = &snp_cpuid_info->entries[i]; > > There's no explicit check that i < KVM_MAX_CPUID_ENTRIES and i < > SNP_CPUID_FUNCTION_MAXCOUNT. The !=0xFFFFFFFF condition might protect > against this but this is not really clear (the memset to 0xFF is done in > another function). > > Since KVM_MAX_CPUID_ENTRIES is 100 and SNP_CPUID_FUNCTION_MAXCOUNT is > 64, it seems possible that i will be 65 for example and then > snp_cpuid_info->entries[i] is an out-of-bounds read access. Indeed, and I don't think this is guarded against currently. I'll make sure to add a check for this. > > > > > > + > > + snp_cpuid_entry->eax_in = kvm_cpuid_entry->function; > > + if (kvm_cpuid_entry->flags == KVM_CPUID_FLAG_SIGNIFCANT_INDEX) { > > + snp_cpuid_entry->ecx_in = kvm_cpuid_entry->index; > > + } > > + snp_cpuid_entry->eax = kvm_cpuid_entry->eax; > > + snp_cpuid_entry->ebx = kvm_cpuid_entry->ebx; > > + snp_cpuid_entry->ecx = kvm_cpuid_entry->ecx; > > + snp_cpuid_entry->edx = kvm_cpuid_entry->edx; > > + > > + if (snp_cpuid_entry->eax_in == 0xD && > > + (snp_cpuid_entry->ecx_in == 0x0 || snp_cpuid_entry->ecx_in == 0x1)) { > > + snp_cpuid_entry->ebx = 0x240; > > + } > > Can you please add a comment explaining this special case? Will do, meant to add one previously. > > > > > > + } > > + > > + if (i > SNP_CPUID_FUNCTION_MAXCOUNT) { > > This can be checked at the top (before the for loop): compare > kvm_cpuid_info.cpuid.nent with SNP_CPUID_FUNCTION_MAXCOUNT. Was possible previously because cpuid.nent doesn't reflect the actual entry count, but with the proposed change above I think I should be able to handle this as suggested. > > > + error_report("SEV-SNP: CPUID count '%lu' exceeds max '%u'", > > + i, SNP_CPUID_FUNCTION_MAXCOUNT); > > + return -1; > > + } > > + > > + snp_cpuid_info->count = i; > > + > > + return 0; > > +} > > + > > +static void > > +sev_snp_cpuid_report_mismatches(SnpCpuidInfo *old, > > + SnpCpuidInfo *new) > > +{ > > + size_t i; > > + > > Add check that new->count == old->count. Ah, of course. > > > > + for (i = 0; i < old->count; i++) { > > + SnpCpuidFunc *old_func, *new_func; > > + > > + old_func = &old->entries[i]; > > + new_func = &new->entries[i]; > > + > > + if (memcmp(old_func, new_func, sizeof(SnpCpuidFunc))) { > > Maybe clearer: > > if (*old_func != *new_func) ... Not 2 structs can be compared this way, maybe I'll just compare the individual members. > > > > + error_report("SEV-SNP: CPUID validation failed for function %x, index: %x.\n" > > Add "0x" prefixes before printing hex values (%x), otherwise we might > have confusing outputs such as "failed for function 13, index: 25" which > is unclear whether it's decimal or hex. Of course, will fix. > > > > + "provided: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x\n" > > + "expected: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x", > > + old_func->eax_in, old_func->ecx_in, > > + old_func->eax, old_func->ebx, old_func->ecx, old_func->edx, > > + new_func->eax, new_func->ebx, new_func->ecx, new_func->edx); > > + } > > + } > > +} > > + > > +static int > > +sev_snp_launch_update_cpuid(uint32_t cpuid_addr, uint32_t cpuid_len) > > +{ > > + KvmCpuidInfo kvm_cpuid_info; > > + SnpCpuidInfo snp_cpuid_info; > > + CPUState *cs = first_cpu; > > + MemoryRegion *mr = NULL; > > + void *snp_cpuid_hva; > > + int ret; > > + > > + snp_cpuid_hva = gpa2hva(&mr, cpuid_addr, cpuid_len, NULL); > > + if (!snp_cpuid_hva) { > > + error_report("SEV-SNP: unable to access CPUID memory range at GPA %d", > > + cpuid_addr); > > + return 1; > > + } > > I think that moving this section just before the memcpy(snp_cpuid_hva, > ...) below would make the flow of this function clearer to the reader > (no functional difference, I believe). I think I put this here to avoid the extra KVM ioctls if this case is hit, but it shouldn't hurt to move it. > > > > + > > + /* get the cpuid list from KVM */ > > + memset(&kvm_cpuid_info.entries, 0xFF, > > + KVM_MAX_CPUID_ENTRIES * sizeof(struct kvm_cpuid_entry2)); > > The third argument can be: sizeof(kvm_cpuid_info.entries) Much nicer. > > > > + kvm_cpuid_info.cpuid.nent = KVM_MAX_CPUID_ENTRIES; > > + > > + ret = kvm_vcpu_ioctl(cs, KVM_GET_CPUID2, &kvm_cpuid_info); > > + if (ret) { > > + error_report("SEV-SNP: unable to query CPUID values for CPU: '%s'", > > + strerror(-ret)); > > Missing return 1 or exit(1) here? Yes indeed, will get this fixed up. > > > -Dov > > > + } > > + > > + ret = sev_snp_cpuid_info_fill(&snp_cpuid_info, &kvm_cpuid_info); > > + if (ret) { > > + error_report("SEV-SNP: failed to generate CPUID table information"); > > + exit(1); > > + } > > + > > + memcpy(snp_cpuid_hva, &snp_cpuid_info, sizeof(snp_cpuid_info)); > > Before memcpy, maybe add sanity test (assert?) that > sizeof(snp_cpuid_info) <= cpuid_len . Makes sense. Thanks!
On 07/09/2021 19:50, Michael Roth wrote: > On Sun, Sep 05, 2021 at 01:02:12PM +0300, Dov Murik wrote: >> On 27/08/2021 1:26, Michael Roth wrote: [...] >>> + for (i = 0; i < old->count; i++) { >>> + SnpCpuidFunc *old_func, *new_func; >>> + >>> + old_func = &old->entries[i]; >>> + new_func = &new->entries[i]; >>> + >>> + if (memcmp(old_func, new_func, sizeof(SnpCpuidFunc))) { >> >> Maybe clearer: >> >> if (*old_func != *new_func) ... > > Not 2 structs can be compared this way, maybe I'll just compare the > individual members. > Oops, my bad; I was confusing it with struct assignment. I guess memcmp is fine as-is in this case. -Dov
diff --git a/target/i386/sev.c b/target/i386/sev.c index 0009c93d28..72a6146295 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -153,6 +153,36 @@ static const char *const sev_fw_errlist[] = { #define SEV_FW_MAX_ERROR ARRAY_SIZE(sev_fw_errlist) +/* <linux/kvm.h> doesn't expose this, so re-use the max from kvm.c */ +#define KVM_MAX_CPUID_ENTRIES 100 + +typedef struct KvmCpuidInfo { + struct kvm_cpuid2 cpuid; + struct kvm_cpuid_entry2 entries[KVM_MAX_CPUID_ENTRIES]; +} KvmCpuidInfo; + +#define SNP_CPUID_FUNCTION_MAXCOUNT 64 +#define SNP_CPUID_FUNCTION_UNKNOWN 0xFFFFFFFF + +typedef struct { + uint32_t eax_in; + uint32_t ecx_in; + uint64_t xcr0_in; + uint64_t xss_in; + uint32_t eax; + uint32_t ebx; + uint32_t ecx; + uint32_t edx; + uint64_t reserved; +} __attribute__((packed)) SnpCpuidFunc; + +typedef struct { + uint32_t count; + uint32_t reserved1; + uint64_t reserved2; + SnpCpuidFunc entries[SNP_CPUID_FUNCTION_MAXCOUNT]; +} __attribute__((packed)) SnpCpuidInfo; + static int sev_ioctl(int fd, int cmd, void *data, int *error) { @@ -1141,6 +1171,117 @@ detect_first_overlap(uint64_t start, uint64_t end, Range *range_list, return overlap; } +static int +sev_snp_cpuid_info_fill(SnpCpuidInfo *snp_cpuid_info, + const KvmCpuidInfo *kvm_cpuid_info) +{ + size_t i; + + memset(snp_cpuid_info, 0, sizeof(*snp_cpuid_info)); + + for (i = 0; kvm_cpuid_info->entries[i].function != 0xFFFFFFFF; i++) { + const struct kvm_cpuid_entry2 *kvm_cpuid_entry; + SnpCpuidFunc *snp_cpuid_entry; + + kvm_cpuid_entry = &kvm_cpuid_info->entries[i]; + snp_cpuid_entry = &snp_cpuid_info->entries[i]; + + snp_cpuid_entry->eax_in = kvm_cpuid_entry->function; + if (kvm_cpuid_entry->flags == KVM_CPUID_FLAG_SIGNIFCANT_INDEX) { + snp_cpuid_entry->ecx_in = kvm_cpuid_entry->index; + } + snp_cpuid_entry->eax = kvm_cpuid_entry->eax; + snp_cpuid_entry->ebx = kvm_cpuid_entry->ebx; + snp_cpuid_entry->ecx = kvm_cpuid_entry->ecx; + snp_cpuid_entry->edx = kvm_cpuid_entry->edx; + + if (snp_cpuid_entry->eax_in == 0xD && + (snp_cpuid_entry->ecx_in == 0x0 || snp_cpuid_entry->ecx_in == 0x1)) { + snp_cpuid_entry->ebx = 0x240; + } + } + + if (i > SNP_CPUID_FUNCTION_MAXCOUNT) { + error_report("SEV-SNP: CPUID count '%lu' exceeds max '%u'", + i, SNP_CPUID_FUNCTION_MAXCOUNT); + return -1; + } + + snp_cpuid_info->count = i; + + return 0; +} + +static void +sev_snp_cpuid_report_mismatches(SnpCpuidInfo *old, + SnpCpuidInfo *new) +{ + size_t i; + + for (i = 0; i < old->count; i++) { + SnpCpuidFunc *old_func, *new_func; + + old_func = &old->entries[i]; + new_func = &new->entries[i]; + + if (memcmp(old_func, new_func, sizeof(SnpCpuidFunc))) { + error_report("SEV-SNP: CPUID validation failed for function %x, index: %x.\n" + "provided: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x\n" + "expected: eax:0x%08x, ebx: 0x%08x, ecx: 0x%08x, edx: 0x%08x", + old_func->eax_in, old_func->ecx_in, + old_func->eax, old_func->ebx, old_func->ecx, old_func->edx, + new_func->eax, new_func->ebx, new_func->ecx, new_func->edx); + } + } +} + +static int +sev_snp_launch_update_cpuid(uint32_t cpuid_addr, uint32_t cpuid_len) +{ + KvmCpuidInfo kvm_cpuid_info; + SnpCpuidInfo snp_cpuid_info; + CPUState *cs = first_cpu; + MemoryRegion *mr = NULL; + void *snp_cpuid_hva; + int ret; + + snp_cpuid_hva = gpa2hva(&mr, cpuid_addr, cpuid_len, NULL); + if (!snp_cpuid_hva) { + error_report("SEV-SNP: unable to access CPUID memory range at GPA %d", + cpuid_addr); + return 1; + } + + /* get the cpuid list from KVM */ + memset(&kvm_cpuid_info.entries, 0xFF, + KVM_MAX_CPUID_ENTRIES * sizeof(struct kvm_cpuid_entry2)); + kvm_cpuid_info.cpuid.nent = KVM_MAX_CPUID_ENTRIES; + + ret = kvm_vcpu_ioctl(cs, KVM_GET_CPUID2, &kvm_cpuid_info); + if (ret) { + error_report("SEV-SNP: unable to query CPUID values for CPU: '%s'", + strerror(-ret)); + } + + ret = sev_snp_cpuid_info_fill(&snp_cpuid_info, &kvm_cpuid_info); + if (ret) { + error_report("SEV-SNP: failed to generate CPUID table information"); + exit(1); + } + + memcpy(snp_cpuid_hva, &snp_cpuid_info, sizeof(snp_cpuid_info)); + + ret = sev_snp_launch_update_gpa(cpuid_addr, cpuid_len, + KVM_SEV_SNP_PAGE_TYPE_CPUID); + if (ret) { + sev_snp_cpuid_report_mismatches(&snp_cpuid_info, snp_cpuid_hva); + error_report("SEV-SNP: failed update CPUID page"); + exit(1); + } + + return 0; +} + static void snp_ovmf_boot_block_setup(void) { SevSnpBootInfoBlock *info; @@ -1176,10 +1317,9 @@ static void snp_ovmf_boot_block_setup(void) } /* Populate the cpuid page */ - ret = sev_snp_launch_update_gpa(info->cpuid_addr, info->cpuid_len, - KVM_SEV_SNP_PAGE_TYPE_CPUID); + ret = sev_snp_launch_update_cpuid(info->cpuid_addr, info->cpuid_len); if (ret) { - error_report("SEV-SNP: failed to insert cpuid page GPA 0x%x", + error_report("SEV-SNP: failed to populate cpuid tables GPA 0x%x", info->cpuid_addr); exit(1); }
SEV-SNP firmware allows a special guest page to be populated with a table of guest CPUID values so that they can be validated through firmware before being loaded into encrypted guest memory where they can be used in place of hypervisor-provided values[1]. As part of SEV-SNP guest initialization, use this process to validate the CPUID entries reported by KVM_GET_CPUID2 prior to initial guest start. [1]: SEV SNP Firmware ABI Specification, Rev. 0.8, 8.13.2.6 Signed-off-by: Michael Roth <michael.roth@amd.com> --- target/i386/sev.c | 146 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 143 insertions(+), 3 deletions(-)