Re: [PATCH 2/3] sev/i386: Warn if using -kernel with invalid OVMF hashes table area

[Dr. David Alan Gilbert]

* Dov Murik (dovmurik@linux.ibm.com) wrote:
> 
> 
> On 02/11/2021 14:36, Dr. David Alan Gilbert wrote:
> > * Dov Murik (dovmurik@linux.ibm.com) wrote:
> >> Commit cff03145ed3c ("sev/i386: Introduce sev_add_kernel_loader_hashes
> >> for measured linux boot", 2021-09-30) introduced measured direct boot
> >> with -kernel, using an OVMF-designated hashes table which QEMU fills.
> >>
> >> However, no checks are performed on the validity of the hashes area
> >> designated by OVMF.  Specifically, if OVMF publishes the
> >> SEV_HASH_TABLE_RV_GUID entry but it is filled with zeroes, this will
> >> cause QEMU to write the hashes entries over the first page of the
> >> guest's memory (GPA 0).
> >>
> >> Add validity checks to the published area.  If the hashes table area's
> >> base address is zero, or its size is too small to fit the aligned hashes
> >> table, warn and skip the hashes entries addition.  In such case, the
> >> following warning will be displayed:
> >>
> >>     qemu-system-x86_64: warning: SEV: OVMF's hashes table area is invalid 
> >> (base=0x0 size=0x0)
> >>
> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> >> Reported-by: Brijesh Singh <brijesh.singh@amd.com>
> >> ---
> >>  target/i386/sev.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/target/i386/sev.c b/target/i386/sev.c
> >> index 682b8ccf6c..a20ddb545e 100644
> >> --- a/target/i386/sev.c
> >> +++ b/target/i386/sev.c
> >> @@ -1201,13 +1201,18 @@ bool 
> >> sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
> >>      uint8_t kernel_hash[HASH_SIZE];
> >>      uint8_t *hashp;
> >>      size_t hash_len = HASH_SIZE;
> >> -    int aligned_len;
> >> +    int aligned_len = ROUND_UP(sizeof(SevHashTable), 16);
> >>  
> >>      if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) {
> >>          warn_report("SEV: kernel specified but OVMF has no hash table 
> >> guid");
> >>          return false;
> >>      }
> >>      area = (SevHashTableDescriptor *)data;
> >> +    if (!area->base || area->size < aligned_len) {
> >> +        warn_report("SEV: OVMF's hashes table area is invalid (base=0x%x 
> >> size=0x%x)",
> >> +                    area->base, area->size);
> >> +        return false;
> >> +    }
> > 
> > That's probably a useful check, so
> > 
> > 
> > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> 
> Thanks.
> 
> 
> > however, maybe it needs to be more thorough before using area->base to
> > qemu_map_ram_ptr? - I think it'll get unhappy if it's a bad address not
> > in a ram block. (Or check you're running over the end of a ramblock)
> >
> 
> Does address_space_write perform these checks? Or maybe another API for
> accessing the guest's RAM?

I'm not sure; for example in address_space_map I don't see an check that
flatview_translate gives a valid mr.

Dave

> -Dov
> 
> > Dave
> > 
> >>  
> >>      /*
> >>       * Calculate hash of kernel command-line with the terminating null 
> >> byte. If
> >> @@ -1266,7 +1271,6 @@ bool 
> >> sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp)
> >>      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);
> >> -- 
> >> 2.25.1
> >>
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK