Re: [PATCH v4 29/66] i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig

[Markus Armbruster]

Xiaoyao Li <xiaoyao.li@intel.com> writes:

> On 2/19/2024 8:48 PM, Markus Armbruster wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>> 
>>> From: Isaku Yamahata <isaku.yamahata@intel.com>
>>>
>>> Three sha384 hash values, mrconfigid, mrowner and mrownerconfig, of a TD
>>> can be provided for TDX attestation. Detailed meaning of them can be
>>> found: 
>>> https://lore.kernel.org/qemu-devel/31d6dbc1-f453-4cef-ab08-4813f4e0ff92@intel.com/
>>>
>>> Allow user to specify those values via property mrconfigid, mrowner and
>>> mrownerconfig. They are all in base64 format.
>>>
>>> example
>>> -object tdx-guest, \
>>>    
>>> mrconfigid=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v,\
>>>    
>>> mrowner=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v,\
>>>    
>>> mrownerconfig=ASNFZ4mrze8BI0VniavN7wEjRWeJq83vASNFZ4mrze8BI0VniavN7wEjRWeJq83v
>>>
>>> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
>>> Co-developed-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>> Signed-off-by: Xiaoyao Li <xiaoyao.li@intel.com>
>>>
>>> ---
>>> Changes in v4:
>>>   - describe more of there fields in qom.json
>>>   - free the old value before set new value to avoid memory leak in
>>>     _setter(); (Daniel)
>>>
>>> Changes in v3:
>>>   - use base64 encoding instread of hex-string;
>>> ---
>>>   qapi/qom.json         | 14 ++++++-
>>>   target/i386/kvm/tdx.c | 87 +++++++++++++++++++++++++++++++++++++++++++
>>>   target/i386/kvm/tdx.h |  3 ++
>>>   3 files changed, 103 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>> index 2177f3101382..15445f9e41fc 100644
>>> --- a/qapi/qom.json
>>> +++ b/qapi/qom.json
>>> @@ -905,10 +905,22 @@
>>>   #     pages.  Some guest OS (e.g., Linux TD guest) may require this to
>>>   #     be set, otherwise they refuse to boot.
>>>   #
>>> +# @mrconfigid: ID for non-owner-defined configuration of the guest TD,
>>> +#     e.g., run-time or OS configuration.  base64 encoded SHA384 digest.
>> 
>> "base64 encoded SHA384" is not a sentence.
>> 
>> Double-checking: the data being hashed here is the "non-owner-defined
>> configuration of the guest TD", and the resulting hash is the "ID"?
>
> yes. The "ID" here means the resulting hash.
>
> The reason to use "ID" here because in the TDX spec, it's description is
>
>    Software-defined ID for non-owner-defined configuration of the guest
>    TD - e.g., run-time or OS configuration.
>
> If ID is confusing, how about
>
>    SHA384 hash of non-owner-defined configuration of the guest TD, e.g.,
>    run-time of OS configuration.  It's base64 encoded.

I guess staying close to the TDX spec makes sense.

We still need to mention the base64 encoding.

What about something like

     ID for non-owner-defined configuration of the guest TD, e.g.,
     run-time or OS configuration (base64 encoded SHA384 digest)

or, if we decide that the fact it's SHA384 digest is irrelevant for QMP

     ID for non-owner-defined configuration of the guest TD, e.g.,
     run-time or OS configuration (base64 encoded)

>>> +#
>>> +# @mrowner: ID for the guest TD’s owner.  base64 encoded SHA384 digest.
>> 
>> Likewise.
>> 
>>> +#
>>> +# @mrownerconfig: ID for owner-defined configuration of the guest TD,
>>> +#     e.g., specific to the workload rather than the run-time or OS.
>>> +#     base64 encoded SHA384 digest.
>> 
>> Likewise.
>> 
>>> +#
>>>   # Since: 9.0
>>>   ##
>>>   { 'struct': 'TdxGuestProperties',
>>> -  'data': { '*sept-ve-disable': 'bool' } }
>>> +  'data': { '*sept-ve-disable': 'bool',
>>> +            '*mrconfigid': 'str',
>>> +            '*mrowner': 'str',
>>> +            '*mrownerconfig': 'str' } }
>> 
>> The new members are optional, but their description in the doc comment
>> doesn't explain behavior when present vs. behavior when absent.
>> 
>>>   
>>>   ##
>>>   # @ThreadContextProperties:
>> 
>> [...]
>> 
>>