| Message ID | 20211101102136.1706421-3-dovmurik@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SEV: fixes for -kernel launch with incompatible OVMF | expand |
* 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> 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) 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 >
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? -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 >>
* 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 > >> >
On 11/2/21 19:38, Dr. David Alan Gilbert wrote: > * Dov Murik (dovmurik@linux.ibm.com) wrote: >>> 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. IIUC the API the MemTxAttrs argument could help you, but I don't think properly enforced (yet?...).
On Mon, Nov 01, 2021 at 10:21:35AM +0000, Dov Murik 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; > + } I think warn_report is likely a bad idea. If someone's use case is relying on the hashs being populated, then we need to be able to error_report and exit, not carry on with a known broken setup. Regards, Daniel
On 03/11/2021 18:07, Daniel P. Berrangé wrote: > On Mon, Nov 01, 2021 at 10:21:35AM +0000, Dov Murik 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; >> + } > > I think warn_report is likely a bad idea. > > If someone's use case is relying on the hashs being populated, then > we need to be able to error_report and exit, not carry on with a > known broken setup. > Thanks you. As I wrote elsewhere, I will put the whole thing under a flag of sev-guest. If the flag is ON then I will error_report and exit if the GUID is missing or if the address/size is wrong, as you suggest. -Dov
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; + } /* * 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);
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(-)