| Message ID | 20201214154429.11023-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> > > --- > > v2: fix line length warning, add more comments about sev area > --- > qapi/misc-target.json | 2 +- > target/i386/monitor.c | 27 ++++++++++++++++++++++++++- > 2 files changed, 27 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..11bdb04155 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,33 @@ 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; > + > + /* > + * not checking length means that this area can't be versioned > + * by length and would have to be replaced if updated > + */ Can you just explain that a bit more? > + 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); Other than me not understanding that comment, I think we're fine: Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > } > -- > 2.26.2 > >
On Tue, 2021-01-26 at 12:32 +0000, Dr. David Alan Gilbert wrote: > * 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> > > > > --- > > > > v2: fix line length warning, add more comments about sev area > > --- > > qapi/misc-target.json | 2 +- > > target/i386/monitor.c | 27 ++++++++++++++++++++++++++- > > 2 files changed, 27 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..11bdb04155 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,33 @@ 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; > > + > > + /* > > + * not checking length means that this area can't be > > versioned > > + * by length and would have to be replaced if updated > > + */ > > Can you just explain that a bit more? It's referring back to the original concept that the reset vector length would tell you what version of the thing you were using. So if you were looking for a property at offset 10 and the length came in as 8 the version was too early. If it was 18 you had a later version and your property was present. The current scheme uses guids which can be versioned by length if you think you'll add extra properties to them. This one I don't think would ever get an extra property, so there's no point checking the length. Not checking the length means if I'm wrong and we do need an extra property it will have to be attached to a new guid. That's a bit confusing to add to the comment ... how about I just leave out the comment entirely? > > + 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); > > Other than me not understanding that comment, I think we're fine: Thanks. > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > } > > -- > > 2.26.2 > > > >
* James Bottomley (jejb@linux.ibm.com) wrote: > On Tue, 2021-01-26 at 12:32 +0000, Dr. David Alan Gilbert wrote: > > * 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> > > > > > > --- > > > > > > v2: fix line length warning, add more comments about sev area > > > --- > > > qapi/misc-target.json | 2 +- > > > target/i386/monitor.c | 27 ++++++++++++++++++++++++++- > > > 2 files changed, 27 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..11bdb04155 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,33 @@ 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; > > > + > > > + /* > > > + * not checking length means that this area can't be > > > versioned > > > + * by length and would have to be replaced if updated > > > + */ > > > > Can you just explain that a bit more? > > It's referring back to the original concept that the reset vector > length would tell you what version of the thing you were using. So if > you were looking for a property at offset 10 and the length came in as > 8 the version was too early. If it was 18 you had a later version and > your property was present. > > The current scheme uses guids which can be versioned by length if you > think you'll add extra properties to them. This one I don't think > would ever get an extra property, so there's no point checking the > length. Not checking the length means if I'm wrong and we do need an > extra property it will have to be attached to a new guid. > > That's a bit confusing to add to the comment ... how about I just leave > out the comment entirely? Yes, I don't think it makes much sense unless you knew the history. Dave > > > + 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); > > > > Other than me not understanding that comment, I think we're fine: > > Thanks. > > > Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > > > > > } > > > -- > > > 2.26.2 > > > > > > > >
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..11bdb04155 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,33 @@ 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; + + /* + * not checking length means that this area can't be versioned + * by length and would have to be replaced if updated + */ + 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 --- qapi/misc-target.json | 2 +- target/i386/monitor.c | 27 ++++++++++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-)