* Dov Murik (dovmu...@linux.ibm.com) wrote: > > > On 02/11/2021 14:36, Dr. David Alan Gilbert wrote: > > * Dov Murik (dovmu...@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 <dovmu...@linux.ibm.com> > >> Reported-by: Brijesh Singh <brijesh.si...@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 <dgilb...@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 / dgilb...@redhat.com / Manchester, UK