Re: [RFC PATCH v2 11/12] i386/sev: sev-snp: add support for CPUID validation

[Michael Roth]

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!