| Message ID | 20210204193939.16617-3-jejb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sev: enable secret injection to a self described area in OVMF | expand |
* James Bottomley (jejb@linux.ibm.com) wrote: > If the gpa isn't specified, it's value is extracted from the OVMF > properties table located below the reset vector (and if this doesn't > exist, an error is returned). OVMF has defined the GUID for the SEV > secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format of > the <data> is: <base>|<size> where both are uint32_t. We extract > <base> and use it as the gpa for the injection. > > Note: it is expected that the injected secret will also be GUID > described but since qemu can't interpret it, the format is left > undefined here. > > Signed-off-by: James Bottomley <jejb@linux.ibm.com> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > --- > > v2: fix line length warning, add more comments about sev area > v2: remove misleading comment > --- > qapi/misc-target.json | 2 +- > target/i386/monitor.c | 23 ++++++++++++++++++++++- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json > index 06ef8757f0..0c7491cd82 100644 > --- a/qapi/misc-target.json > +++ b/qapi/misc-target.json > @@ -216,7 +216,7 @@ > # > ## > { 'command': 'sev-inject-launch-secret', > - 'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' }, > + 'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' }, > 'if': 'defined(TARGET_I386)' } > > ## > diff --git a/target/i386/monitor.c b/target/i386/monitor.c > index 1bc91442b1..5994408bee 100644 > --- a/target/i386/monitor.c > +++ b/target/i386/monitor.c > @@ -34,6 +34,7 @@ > #include "sev_i386.h" > #include "qapi/qapi-commands-misc-target.h" > #include "qapi/qapi-commands-misc.h" > +#include "hw/i386/pc.h" > > /* Perform linear address sign extension */ > static hwaddr addr_canonical(CPUArchState *env, hwaddr addr) > @@ -730,9 +731,29 @@ SevCapability *qmp_query_sev_capabilities(Error **errp) > return sev_get_capabilities(errp); > } > > +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294" > +struct sev_secret_area { > + uint32_t base; > + uint32_t size; > +}; > + > void qmp_sev_inject_launch_secret(const char *packet_hdr, > - const char *secret, uint64_t gpa, > + const char *secret, > + bool has_gpa, uint64_t gpa, > Error **errp) > { > + if (!has_gpa) { > + uint8_t *data; > + struct sev_secret_area *area; > + > + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { > + error_setg(errp, "SEV: no secret area found in OVMF," > + " gpa must be specified."); > + return; > + } > + area = (struct sev_secret_area *)data; > + gpa = area->base; > + } > + > sev_inject_launch_secret(packet_hdr, secret, gpa, errp); > } > -- > 2.26.2 >
On Thu, Feb 04, 2021 at 11:39:39AM -0800, James Bottomley wrote: > If the gpa isn't specified, it's value is extracted from the OVMF > properties table located below the reset vector (and if this doesn't > exist, an error is returned). OVMF has defined the GUID for the SEV > secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format of > the <data> is: <base>|<size> where both are uint32_t. We extract > <base> and use it as the gpa for the injection. > > Note: it is expected that the injected secret will also be GUID > described but since qemu can't interpret it, the format is left > undefined here. > > Signed-off-by: James Bottomley <jejb@linux.ibm.com> > > --- > > v2: fix line length warning, add more comments about sev area > v2: remove misleading comment > --- > qapi/misc-target.json | 2 +- > target/i386/monitor.c | 23 ++++++++++++++++++++++- > 2 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json > index 06ef8757f0..0c7491cd82 100644 > --- a/qapi/misc-target.json > +++ b/qapi/misc-target.json > @@ -216,7 +216,7 @@ > # > ## > { 'command': 'sev-inject-launch-secret', > - 'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' }, > + 'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' }, > 'if': 'defined(TARGET_I386)' } > > ## > diff --git a/target/i386/monitor.c b/target/i386/monitor.c > index 1bc91442b1..5994408bee 100644 > --- a/target/i386/monitor.c > +++ b/target/i386/monitor.c > @@ -34,6 +34,7 @@ > #include "sev_i386.h" > #include "qapi/qapi-commands-misc-target.h" > #include "qapi/qapi-commands-misc.h" > +#include "hw/i386/pc.h" > > /* Perform linear address sign extension */ > static hwaddr addr_canonical(CPUArchState *env, hwaddr addr) > @@ -730,9 +731,29 @@ SevCapability *qmp_query_sev_capabilities(Error **errp) > return sev_get_capabilities(errp); > } > > +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294" > +struct sev_secret_area { > + uint32_t base; > + uint32_t size; > +}; > + > void qmp_sev_inject_launch_secret(const char *packet_hdr, > - const char *secret, uint64_t gpa, > + const char *secret, > + bool has_gpa, uint64_t gpa, > Error **errp) > { > + if (!has_gpa) { > + uint8_t *data; > + struct sev_secret_area *area; > + > + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { > + error_setg(errp, "SEV: no secret area found in OVMF," > + " gpa must be specified."); > + return; > + } IIUC, historically QEMU has gone out of its way to avoid creating a direct dependancy on specific firmware implementation details such as this, so this whole approach makes me feel really uneasy. We have the fw_cfg interface which lets QEMU put well known data items at specific places in memory, which the firmware can then access and do sometime with. That all happens at startup though before CPUs are running, so this situation at runtime is more complex. None the less I still wonder if we can take a better approach such that the firmware explicitly tells us this location to use, rather than QEMU parsing data tables to reverse engineer it. > + area = (struct sev_secret_area *)data; > + gpa = area->base; > + } > + > sev_inject_launch_secret(packet_hdr, secret, gpa, errp); > } > -- > 2.26.2 > > Regards, Daniel
On 05/02/21 10:51, Daniel P. Berrangé wrote: >> + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { >> + error_setg(errp, "SEV: no secret area found in OVMF," >> + " gpa must be specified."); >> + return; >> + } > IIUC, historically QEMU has gone out of its way to avoid creating a > direct dependancy on specific firmware implementation details such > as this, so this whole approach makes me feel really uneasy. The problem here is that this secret must be measured and therefore cannot be extracted by the guest out of fw_cfg. Note that there's no reason why other firmware than OVMF could not adopt the same interface. Paolo
On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote: > On 05/02/21 10:51, Daniel P. Berrangé wrote: > > > + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { > > > + error_setg(errp, "SEV: no secret area found in OVMF," > > > + " gpa must be specified."); > > > + return; > > > + } > > IIUC, historically QEMU has gone out of its way to avoid creating a > > direct dependancy on specific firmware implementation details such > > as this, so this whole approach makes me feel really uneasy. > > The problem here is that this secret must be measured and therefore cannot > be extracted by the guest out of fw_cfg. Note that there's no reason why > other firmware than OVMF could not adopt the same interface. I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a way for OVMF to tell QEMU where to store it Regards, Daniel
On 05/02/21 12:37, Daniel P. Berrangé wrote: > On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote: >> On 05/02/21 10:51, Daniel P. Berrangé wrote: >>>> + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { >>>> + error_setg(errp, "SEV: no secret area found in OVMF," >>>> + " gpa must be specified."); >>>> + return; >>>> + } >>> IIUC, historically QEMU has gone out of its way to avoid creating a >>> direct dependancy on specific firmware implementation details such >>> as this, so this whole approach makes me feel really uneasy. >> >> The problem here is that this secret must be measured and therefore cannot >> be extracted by the guest out of fw_cfg. Note that there's no reason why >> other firmware than OVMF could not adopt the same interface. > > I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a > way for OVMF to tell QEMU where to store it I may be misunderstanding, but I think QEMU has to store it before OVMF runs, because the measurement is "sealed" when the VM starts. Paolo
On Fri, Feb 05, 2021 at 12:45:18PM +0100, Paolo Bonzini wrote: > On 05/02/21 12:37, Daniel P. Berrangé wrote: > > On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote: > > > On 05/02/21 10:51, Daniel P. Berrangé wrote: > > > > > + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { > > > > > + error_setg(errp, "SEV: no secret area found in OVMF," > > > > > + " gpa must be specified."); > > > > > + return; > > > > > + } > > > > IIUC, historically QEMU has gone out of its way to avoid creating a > > > > direct dependancy on specific firmware implementation details such > > > > as this, so this whole approach makes me feel really uneasy. > > > > > > The problem here is that this secret must be measured and therefore cannot > > > be extracted by the guest out of fw_cfg. Note that there's no reason why > > > other firmware than OVMF could not adopt the same interface. > > > > I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a > > way for OVMF to tell QEMU where to store it > > I may be misunderstanding, but I think QEMU has to store it before OVMF > runs, because the measurement is "sealed" when the VM starts. Oh, right, I see what you mean Regards, Daniel
* Paolo Bonzini (pbonzini@redhat.com) wrote: > On 05/02/21 12:37, Daniel P. Berrangé wrote: > > On Fri, Feb 05, 2021 at 11:58:26AM +0100, Paolo Bonzini wrote: > > > On 05/02/21 10:51, Daniel P. Berrangé wrote: > > > > > + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { > > > > > + error_setg(errp, "SEV: no secret area found in OVMF," > > > > > + " gpa must be specified."); > > > > > + return; > > > > > + } > > > > IIUC, historically QEMU has gone out of its way to avoid creating a > > > > direct dependancy on specific firmware implementation details such > > > > as this, so this whole approach makes me feel really uneasy. > > > > > > The problem here is that this secret must be measured and therefore cannot > > > be extracted by the guest out of fw_cfg. Note that there's no reason why > > > other firmware than OVMF could not adopt the same interface. > > > > I didn't mean to store the secret in fw_cfg. Rather to use fw_cfg as a > > way for OVMF to tell QEMU where to store it > > I may be misunderstanding, but I think QEMU has to store it before OVMF > runs, because the measurement is "sealed" when the VM starts. Yes, that's correct, it's a feature of SEV and SEV-ES that the flow is: * measure data * attest * insert some encrypted data * Start execution I would agree this code is pretty much tied to OVMF's weird GUID based system when it's not needed; but there's no standard to follow here. Dave > Paolo >
diff --git a/qapi/misc-target.json b/qapi/misc-target.json index 06ef8757f0..0c7491cd82 100644 --- a/qapi/misc-target.json +++ b/qapi/misc-target.json @@ -216,7 +216,7 @@ # ## { 'command': 'sev-inject-launch-secret', - 'data': { 'packet-header': 'str', 'secret': 'str', 'gpa': 'uint64' }, + 'data': { 'packet-header': 'str', 'secret': 'str', '*gpa': 'uint64' }, 'if': 'defined(TARGET_I386)' } ## diff --git a/target/i386/monitor.c b/target/i386/monitor.c index 1bc91442b1..5994408bee 100644 --- a/target/i386/monitor.c +++ b/target/i386/monitor.c @@ -34,6 +34,7 @@ #include "sev_i386.h" #include "qapi/qapi-commands-misc-target.h" #include "qapi/qapi-commands-misc.h" +#include "hw/i386/pc.h" /* Perform linear address sign extension */ static hwaddr addr_canonical(CPUArchState *env, hwaddr addr) @@ -730,9 +731,29 @@ SevCapability *qmp_query_sev_capabilities(Error **errp) return sev_get_capabilities(errp); } +#define SEV_SECRET_GUID "4c2eb361-7d9b-4cc3-8081-127c90d3d294" +struct sev_secret_area { + uint32_t base; + uint32_t size; +}; + void qmp_sev_inject_launch_secret(const char *packet_hdr, - const char *secret, uint64_t gpa, + const char *secret, + bool has_gpa, uint64_t gpa, Error **errp) { + if (!has_gpa) { + uint8_t *data; + struct sev_secret_area *area; + + if (!pc_system_ovmf_table_find(SEV_SECRET_GUID, &data, NULL)) { + error_setg(errp, "SEV: no secret area found in OVMF," + " gpa must be specified."); + return; + } + area = (struct sev_secret_area *)data; + gpa = area->base; + } + sev_inject_launch_secret(packet_hdr, secret, gpa, errp); }
If the gpa isn't specified, it's value is extracted from the OVMF properties table located below the reset vector (and if this doesn't exist, an error is returned). OVMF has defined the GUID for the SEV secret area as 4c2eb361-7d9b-4cc3-8081-127c90d3d294 and the format of the <data> is: <base>|<size> where both are uint32_t. We extract <base> and use it as the gpa for the injection. Note: it is expected that the injected secret will also be GUID described but since qemu can't interpret it, the format is left undefined here. Signed-off-by: James Bottomley <jejb@linux.ibm.com> --- v2: fix line length warning, add more comments about sev area v2: remove misleading comment --- qapi/misc-target.json | 2 +- target/i386/monitor.c | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-)