Re: [PATCH 3/3] sev: update sev-inject-launch-secret to make gpa optional

[James Bottomley]

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