| Message ID | 20201209172334.14100-4-jejb@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sev: enable seret injection to a self described area in OVMF | expand |
On 12/9/20 11:23 AM, 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> > --- > qapi/misc-target.json | 2 +- > target/i386/monitor.c | 22 +++++++++++++++++++++- > 2 files changed, 22 insertions(+), 2 deletions(-) > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json > index 4486a543ae..1ee4e62f85 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..a99e3dd2b3 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,28 @@ 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; > +}; > + Originally, the idea was to allow expanding of these GUID based structures by pre-pending data to them, but based on how pc_system_ovmf_table_find() returns the pointer to the start of the structure (based on the length found in the structure), I believe that expansion could be done by appending to the structure, which seems more logical. For example, if this structure is ever expanded, it can use the third parameter of pc_system_ovmf_table_find() to get the length and compare that to the size of the structure to determine if new version of the structure is present in the firmware. Otherwise you can't do the nice easy assignment below: area = (struct sev_secret_area *)data; You actually have to do some math: area = (struct sev_secret_area *)(data + data_len - sizeof(QemuUUID) - sizeof(uint16_t) - sizeof(*area)); or add the QemuUUID and uint16_t fields to sev_secret_area and: area = (struct sev_secret_area *)(data + data_len - sizeof(*area)); Or we make the decision that these GUID structs should never change, just add a new one to the table if more info is needed. Whatever we decide should probably be documented in both the OVMF patches and the Qemu patches. Thanks, Tom > 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); > } >
On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote: > On 12/9/20 11:23 AM, 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> > > --- > > qapi/misc-target.json | 2 +- > > target/i386/monitor.c | 22 +++++++++++++++++++++- > > 2 files changed, 22 insertions(+), 2 deletions(-) > > > > diff --git a/qapi/misc-target.json b/qapi/misc-target.json > > index 4486a543ae..1ee4e62f85 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..a99e3dd2b3 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,28 @@ 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; > > +}; > > + > > Originally, the idea was to allow expanding of these GUID based > structures by pre-pending data to them, but based on how > pc_system_ovmf_table_find() returns the pointer to the start of the > structure (based on the length found in the structure), I believe > that expansion could be done by appending to the structure, which > seems more logical. For example, if this structure is ever expanded, > it can use the third parameter of pc_system_ovmf_table_find() to get > the length and compare that to the size of the structure to > determine if new version of the structure is present in the firmware. Actually, I don't think it much matters. It looks like the len it would return is wrong ... it should be the length of just the returned data pointer (without the length or guid), so ptr+len would point to the foot of the data if that's what you want. > Otherwise you can't do the nice easy assignment below: > area = (struct sev_secret_area *)data; > > You actually have to do some math: > area = (struct sev_secret_area *)(data + data_len - > sizeof(QemuUUID) - > sizeof(uint16_t) - > sizeof(*area)); > > or add the QemuUUID and uint16_t fields to sev_secret_area and: > area = (struct sev_secret_area *)(data + data_len - > sizeof(*area)); Right, that's why I think patch 2/3 should do *data_len = len - sizeof(QemuUUID) - sizeof(uint16_t) > Or we make the decision that these GUID structs should never change, > just add a new one to the table if more info is needed. Actually, the fact that the only guid the table depends on is the table footer GUID, you can always remove guids and add new ones. So I think it's up to whoever is using the GUID to decide the policy. So for this one I'm not checking the length, which argues it wouldn't be subject to the added length new data rule and I'd have to use a new guid for new information. However, I could also see situations where you would check the length and thus would have the ability to add fields (either at the beginning or the end). > Whatever we decide should probably be documented in both the OVMF > patches and the Qemu patches. OK, I can add a comment about my use case and you can add one documenting your length based use case. James
On 12/11/20 4:45 PM, James Bottomley wrote: > On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote: >> On 12/9/20 11:23 AM, James Bottomley wrote: > > So for this one I'm not checking the length, which argues it wouldn't > be subject to the added length new data rule and I'd have to use a new > guid for new information. However, I could also see situations where > you would check the length and thus would have the ability to add > fields (either at the beginning or the end). I think this paragraph explains it nicely and a slightly expanded comment with this information would be enough. Thanks, Tom > >> Whatever we decide should probably be documented in both the OVMF >> patches and the Qemu patches. > > OK, I can add a comment about my use case and you can add one > documenting your length based use case. > > James > >
On 12/11/20 23:54, Tom Lendacky wrote: > On 12/11/20 4:45 PM, James Bottomley wrote: >> On Fri, 2020-12-11 at 16:00 -0600, Tom Lendacky wrote: >>> On 12/9/20 11:23 AM, James Bottomley wrote: >> >> So for this one I'm not checking the length, which argues it wouldn't >> be subject to the added length new data rule and I'd have to use a new >> guid for new information. However, I could also see situations where >> you would check the length and thus would have the ability to add >> fields (either at the beginning or the end). > > I think this paragraph explains it nicely and a slightly expanded > comment with this information would be enough. Sounds good to me as well, thank you. Laszlo
diff --git a/qapi/misc-target.json b/qapi/misc-target.json index 4486a543ae..1ee4e62f85 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..a99e3dd2b3 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,28 @@ 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> --- qapi/misc-target.json | 2 +- target/i386/monitor.c | 22 +++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-)