| Message ID | 20211101102136.1706421-2-dovmurik@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | SEV: fixes for -kernel launch with incompatible OVMF | expand |
On 11/1/21 5:21 AM, 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, if OVMF doesn't designate such an area, QEMU would completely > abort the VM launch. This breaks launching with -kernel using older > OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. > > Instead, just warn the user that -kernel was supplied by OVMF doesn't > specify the GUID for the hashes table. The following warning will be > displayed during VM launch: > > qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > Reported-by: Tom Lendacky <thomas.lendacky@amd.com> Just a few minor comments/questions below, otherwise: Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > target/i386/sev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index eede07f11d..682b8ccf6c 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) > int aligned_len; > > if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { > - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); > + warn_report("SEV: kernel specified but OVMF has no hash table guid"); Not sure if warn or info would be better, either way works for me, though, since this allows the guest to boot now. Unrelated to this change, but do we explicitly want to say OVMF? Can't another BIOS/UEFI implementation have this support? and should guid be GUID? Thanks, Tom > return false; > } > area = (SevHashTableDescriptor *)data; >
On 01/11/2021 16:25, Tom Lendacky wrote: > On 11/1/21 5:21 AM, 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, if OVMF doesn't designate such an area, QEMU would completely >> abort the VM launch. This breaks launching with -kernel using older >> OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. >> >> Instead, just warn the user that -kernel was supplied by OVMF doesn't >> specify the GUID for the hashes table. The following warning will be >> displayed during VM launch: >> >> qemu-system-x86_64: warning: SEV: kernel specified but OVMF has >> no hash table guid >> >> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >> Reported-by: Tom Lendacky <thomas.lendacky@amd.com> > > Just a few minor comments/questions below, otherwise: > > Acked-by: Tom Lendacky <thomas.lendacky@amd.com> > >> --- >> target/i386/sev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/target/i386/sev.c b/target/i386/sev.c >> index eede07f11d..682b8ccf6c 100644 >> --- a/target/i386/sev.c >> +++ b/target/i386/sev.c >> @@ -1204,7 +1204,7 @@ bool >> sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) >> int aligned_len; >> if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, >> NULL)) { >> - error_setg(errp, "SEV: kernel specified but OVMF has no hash >> table guid"); >> + warn_report("SEV: kernel specified but OVMF has no hash table >> guid"); > > Not sure if warn or info would be better, either way works for me, > though, since this allows the guest to boot now. OK. > > Unrelated to this change, but do we explicitly want to say OVMF? Can't > another BIOS/UEFI implementation have this support? > I don't mind saying "guest firmware" or "VM firmware" or "guest flash" instead. Note that the function name has "ovmf" in it... > and should guid be GUID? I agree; if we change "OVMF" I'll fix that as well. -Dov > > Thanks, > Tom > >> return false; >> } >> area = (SevHashTableDescriptor *)data; >>
On Mon, Nov 01, 2021 at 10:21:34AM +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, if OVMF doesn't designate such an area, QEMU would completely > abort the VM launch. This breaks launching with -kernel using older > OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. > > Instead, just warn the user that -kernel was supplied by OVMF doesn't > specify the GUID for the hashes table. The following warning will be > displayed during VM launch: > > qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > Reported-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > target/i386/sev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index eede07f11d..682b8ccf6c 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) > int aligned_len; > > if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { > - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); > + warn_report("SEV: kernel specified but OVMF has no hash table guid"); > return false; I'm pretty wary of doing this kind of thing. If someone is using QEMU and they required to have the hashes populated for their use case, they now don't get a fatal error if something goes wrong with the process. This is bad as it hides a serious mistake. If someone is using QEMU and they don't require to have the hashes populated and they knowingly use a firmware that doesn't support this, they'll now get a irrelevant warning every time they boot QEMU. This is bad because IME users will file bugs complaining about this bogus warning. If we genuinely need to support both uses cases, then we should have an explicit command line flag to request the desired behaviour. This could be a -machine option to indicate that the hashes must be populated. - unset: try to populate hashes, *silently* ignore missing table in ovmf - set == on: try to populate hashes, report error on missing table in ovmf -set == off: never try to populate hashes, nor look for the table in ovmf Regards, Daniel
* Daniel P. Berrangé (berrange@redhat.com) wrote: > On Mon, Nov 01, 2021 at 10:21:34AM +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, if OVMF doesn't designate such an area, QEMU would completely > > abort the VM launch. This breaks launching with -kernel using older > > OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. > > > > Instead, just warn the user that -kernel was supplied by OVMF doesn't > > specify the GUID for the hashes table. The following warning will be > > displayed during VM launch: > > > > qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid > > > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > > Reported-by: Tom Lendacky <thomas.lendacky@amd.com> > > --- > > target/i386/sev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/target/i386/sev.c b/target/i386/sev.c > > index eede07f11d..682b8ccf6c 100644 > > --- a/target/i386/sev.c > > +++ b/target/i386/sev.c > > @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) > > int aligned_len; > > > > if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { > > - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); > > + warn_report("SEV: kernel specified but OVMF has no hash table guid"); > > return false; > > I'm pretty wary of doing this kind of thing. > > If someone is using QEMU and they required to have the hashes populated > for their use case, they now don't get a fatal error if something goes > wrong with the process. This is bad as it hides a serious mistake. > > If someone is using QEMU and they don't require to have the hashes > populated and they knowingly use a firmware that doesn't support > this, they'll now get a irrelevant warning every time they boot > QEMU. This is bad because IME users will file bugs complaining > about this bogus warning. > > > If we genuinely need to support both uses cases, then we should have > an explicit command line flag to request the desired behaviour. > > This could be a -machine option to indicate that the hashes must > be populated. > > - unset: try to populate hashes, *silently* ignore missing table > in ovmf > - set == on: try to populate hashes, report error on missing > table in ovmf > -set == off: never try to populate hashes, nor look for the > table in ovmf Or as a property on the sev-guest object. Dave > Regards, > Daniel > -- > |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| > |: https://libvirt.org -o- https://fstop138.berrange.com :| > |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :| >
On Thu, Nov 04, 2021 at 06:18:10PM +0000, Dr. David Alan Gilbert wrote: > * Daniel P. Berrangé (berrange@redhat.com) wrote: > > On Mon, Nov 01, 2021 at 10:21:34AM +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, if OVMF doesn't designate such an area, QEMU would completely > > > abort the VM launch. This breaks launching with -kernel using older > > > OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. > > > > > > Instead, just warn the user that -kernel was supplied by OVMF doesn't > > > specify the GUID for the hashes table. The following warning will be > > > displayed during VM launch: > > > > > > qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid > > > > > > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> > > > Reported-by: Tom Lendacky <thomas.lendacky@amd.com> > > > --- > > > target/i386/sev.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/target/i386/sev.c b/target/i386/sev.c > > > index eede07f11d..682b8ccf6c 100644 > > > --- a/target/i386/sev.c > > > +++ b/target/i386/sev.c > > > @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) > > > int aligned_len; > > > > > > if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { > > > - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); > > > + warn_report("SEV: kernel specified but OVMF has no hash table guid"); > > > return false; > > > > I'm pretty wary of doing this kind of thing. > > > > If someone is using QEMU and they required to have the hashes populated > > for their use case, they now don't get a fatal error if something goes > > wrong with the process. This is bad as it hides a serious mistake. > > > > If someone is using QEMU and they don't require to have the hashes > > populated and they knowingly use a firmware that doesn't support > > this, they'll now get a irrelevant warning every time they boot > > QEMU. This is bad because IME users will file bugs complaining > > about this bogus warning. > > > > > > If we genuinely need to support both uses cases, then we should have > > an explicit command line flag to request the desired behaviour. > > > > This could be a -machine option to indicate that the hashes must > > be populated. > > > > - unset: try to populate hashes, *silently* ignore missing table > > in ovmf > > - set == on: try to populate hashes, report error on missing > > table in ovmf > > -set == off: never try to populate hashes, nor look for the > > table in ovmf > > Or as a property on the sev-guest object. Yep, I thought of that too, and I'm pretty undecided which is "best". -machine makes sense as 'kernel' and 'initrd' are properties of the '-machine' and we're doing stuff related to the. -object sev-guest makes sense as this is behaviour that's (currently) specific to SEV. Regards, Daniel
On 04/11/2021 20:22, Daniel P. Berrangé wrote: > On Thu, Nov 04, 2021 at 06:18:10PM +0000, Dr. David Alan Gilbert wrote: >> * Daniel P. Berrangé (berrange@redhat.com) wrote: >>> On Mon, Nov 01, 2021 at 10:21:34AM +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, if OVMF doesn't designate such an area, QEMU would completely >>>> abort the VM launch. This breaks launching with -kernel using older >>>> OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. >>>> >>>> Instead, just warn the user that -kernel was supplied by OVMF doesn't >>>> specify the GUID for the hashes table. The following warning will be >>>> displayed during VM launch: >>>> >>>> qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid >>>> >>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> >>>> Reported-by: Tom Lendacky <thomas.lendacky@amd.com> >>>> --- >>>> target/i386/sev.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/target/i386/sev.c b/target/i386/sev.c >>>> index eede07f11d..682b8ccf6c 100644 >>>> --- a/target/i386/sev.c >>>> +++ b/target/i386/sev.c >>>> @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) >>>> int aligned_len; >>>> >>>> if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { >>>> - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); >>>> + warn_report("SEV: kernel specified but OVMF has no hash table guid"); >>>> return false; >>> >>> I'm pretty wary of doing this kind of thing. >>> >>> If someone is using QEMU and they required to have the hashes populated >>> for their use case, they now don't get a fatal error if something goes >>> wrong with the process. This is bad as it hides a serious mistake. >>> >>> If someone is using QEMU and they don't require to have the hashes >>> populated and they knowingly use a firmware that doesn't support >>> this, they'll now get a irrelevant warning every time they boot >>> QEMU. This is bad because IME users will file bugs complaining >>> about this bogus warning. >>> >>> >>> If we genuinely need to support both uses cases, then we should have >>> an explicit command line flag to request the desired behaviour. >>> >>> This could be a -machine option to indicate that the hashes must >>> be populated. >>> >>> - unset: try to populate hashes, *silently* ignore missing table >>> in ovmf >>> - set == on: try to populate hashes, report error on missing >>> table in ovmf >>> -set == off: never try to populate hashes, nor look for the >>> table in ovmf >> >> Or as a property on the sev-guest object. > > Yep, I thought of that too, and I'm pretty undecided which is "best". > > -machine makes sense as 'kernel' and 'initrd' are properties of > the '-machine' and we're doing stuff related to the. > > -object sev-guest makes sense as this is behaviour that's (currently) > specific to SEV. Thanks for the suggestions. I'm going to go with '-object sev-guest,...,kernel_hashes=on' because this whole function is defined in sev.c and only works with the AmdSev OVMF target. -Dov
diff --git a/target/i386/sev.c b/target/i386/sev.c index eede07f11d..682b8ccf6c 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -1204,7 +1204,7 @@ bool sev_add_kernel_loader_hashes(SevKernelLoaderContext *ctx, Error **errp) int aligned_len; if (!pc_system_ovmf_table_find(SEV_HASH_TABLE_RV_GUID, &data, NULL)) { - error_setg(errp, "SEV: kernel specified but OVMF has no hash table guid"); + warn_report("SEV: kernel specified but OVMF has no hash table guid"); return false; } area = (SevHashTableDescriptor *)data;
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, if OVMF doesn't designate such an area, QEMU would completely abort the VM launch. This breaks launching with -kernel using older OVMF images which don't publish the SEV_HASH_TABLE_RV_GUID. Instead, just warn the user that -kernel was supplied by OVMF doesn't specify the GUID for the hashes table. The following warning will be displayed during VM launch: qemu-system-x86_64: warning: SEV: kernel specified but OVMF has no hash table guid Signed-off-by: Dov Murik <dovmurik@linux.ibm.com> Reported-by: Tom Lendacky <thomas.lendacky@amd.com> --- target/i386/sev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)