Re: [PATCH v5 30/65] i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig

[Markus Armbruster]

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

> On 3/7/2024 4:39 PM, Markus Armbruster wrote:
>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>> 
>>> On 2/29/2024 9:25 PM, Markus Armbruster wrote:
>>>> Xiaoyao Li <xiaoyao.li@intel.com> writes:
>>>>
>>>>> On 2/29/2024 4:37 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>
>>>> [...]
>>>>
>>>>>>> diff --git a/qapi/qom.json b/qapi/qom.json
>>>>>>> index 89ed89b9b46e..cac875349a3a 100644
>>>>>>> --- a/qapi/qom.json
>>>>>>> +++ b/qapi/qom.json
>>>>>>> @@ -905,10 +905,25 @@
>>>>>>>    #     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).
>>>>>>> +#     (A default value 0 of SHA384 is used when absent).
>>>>>>
>>>>>> Suggest to drop the parenthesis in the last sentence.
>>>>>>
>>>>>> @mrconfigid is a string, so the default value can't be 0.  Actually,
>>>>>> it's not just any string, but a base64 encoded SHA384 digest, which
>>>>>> means it must be exactly 96 hex digits.  So it can't be "0", either.  It
>>>>>> could be
>>>>>> "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000".
>>>>>
>>>>> I thought value 0 of SHA384 just means it.
>>>>>
>>>>> That's my fault and my poor english.
>>>>
>>>> "Fault" is too harsh :)  It's not as precise as I want our interface
>>>> documentation to be.  We work together to get there.
>>>>
>>>>>> More on this below.
>>>>>>
>>>>>>> +#
>>>>>>> +# @mrowner: ID for the guest TD’s owner (base64 encoded SHA384 digest).
>>>>>>> +#     (A default value 0 of SHA384 is used when absent).
>>>>>>> +#
>>>>>>> +# @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). (A default value 0 of SHA384 is
>>>>>>> +#     used when absent).
>>>>>>> +#
>>>>>>>    # Since: 9.0
>>>>>>>    ##
>>>>>>>    { 'struct': 'TdxGuestProperties',
>>>>>>> -  'data': { '*sept-ve-disable': 'bool' } }
>>>>>>> +  'data': { '*sept-ve-disable': 'bool',
>>>>>>> +            '*mrconfigid': 'str',
>>>>>>> +            '*mrowner': 'str',
>>>>>>> +            '*mrownerconfig': 'str' } }
>>>>>>>    ##
>>>>>>>    # @ThreadContextProperties:
>>>>>>> diff --git a/target/i386/kvm/tdx.c b/target/i386/kvm/tdx.c
>>>>>>> index d0ad4f57b5d0..4ce2f1d082ce 100644
>>>>>>> --- a/target/i386/kvm/tdx.c
>>>>>>> +++ b/target/i386/kvm/tdx.c
>>>>>>> @@ -13,6 +13,7 @@
>>>>>>>    #include "qemu/osdep.h"
>>>>>>>    #include "qemu/error-report.h"
>>>>>>> +#include "qemu/base64.h"
>>>>>>>    #include "qapi/error.h"
>>>>>>>    #include "qom/object_interfaces.h"
>>>>>>>    #include "standard-headers/asm-x86/kvm_para.h"
>>>>>>> @@ -516,6 +517,7 @@ int tdx_pre_create_vcpu(CPUState *cpu, Error **errp)
>>>>>>>        X86CPU *x86cpu = X86_CPU(cpu);
>>>>>>>        CPUX86State *env = &x86cpu->env;
>>>>>>>        g_autofree struct kvm_tdx_init_vm *init_vm = NULL;
>>>>>>> +    size_t data_len;
>>>>>>>        int r = 0;
>>>>>>>        object_property_set_bool(OBJECT(cpu), "pmu", false, 
>>>>>>> &error_abort);
>>>>>>> @@ -528,6 +530,38 @@ int tdx_pre_create_vcpu(CPUState *cpu, Error 
>>>>>>> **errp)
>>>>>>>        init_vm = g_malloc0(sizeof(struct kvm_tdx_init_vm) +
>>>>>>>                            sizeof(struct kvm_cpuid_entry2) * 
>>>>>>> KVM_MAX_CPUID_ENTRIES);
>>>>>>> +#define SHA384_DIGEST_SIZE  48
>>>>>>> +
>>>>>>> +    if (tdx_guest->mrconfigid) {
>>>>>>> +        g_autofree uint8_t *data = 
>>>>>>> qbase64_decode(tdx_guest->mrconfigid,
>>>>>>> +                              strlen(tdx_guest->mrconfigid), 
>>>>>>> &data_len, errp);
>>>>>>> +        if (!data || data_len != SHA384_DIGEST_SIZE) {
>>>>>>> +            error_setg(errp, "TDX: failed to decode mrconfigid");
>>>>>>> +            return -1;
>>>>>>> +        }
>>>>>>> +        memcpy(init_vm->mrconfigid, data, data_len);
>>>>>>> +    }
>>>>>>
>>>>>> When @mrconfigid is absent, the property remains null, and this
>>>>>> conditional is not executed.  init_vm->mrconfigid[], an array of 6
>>>>>> __u64, remains all zero.  How does the kernel treat that?
>>>>>
>>>>> A all-zero SHA384 value is still a valid value, isn't it?
>>>>>
>>>>> KVM treats it with no difference.
>>>>
>>>> Can you point me to the spot in the kernel where mrconfigid is used?
>>>
>>> https://github.com/intel/tdx/blob/66a10e258636fa8ec9f5ce687607bf2196a92341/arch/x86/kvm/vmx/tdx.c#L2322
>>>
>>> KVM just copy what QEMU provides into its own data structure @td_params. 
>>> The format @of td_params is defined by TDX spec, and @td_params needs to be 
>>> passed to TDX module when initialize the context of TD via 
>>> SEAMCALL(TDH.MNG.INIT): 
>>> https://github.com/intel/tdx/blob/66a10e258636fa8ec9f5ce687607bf2196a92341/arch/x86/kvm/vmx/tdx.c#L2450
>>>
>>>
>>> In fact, all the three SHA384 fields, will be hashed together with some 
>>> other fields (in td_params and other content of TD) to compromise the 
>>> initial measurement of TD.
>>>
>>> TDX module doesn't care the value of td_params->mrconfigid.
>>
>> My problem is that I don't understand when and why users would omit the
>> optional @mrFOO.
>
> When users don't care it and don't have an explicit value for them, they can 
> omit it. Then a default all-zero value is used.
>
> If making it mandatory field, then users have to explicit pass a all-zero 
> value when they don't care it.
>
>> I naively expected absent @mrFOO to mean something like "no attestation
>> of FOO".
>>
>> But I see that they default to
>> "000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000".
>>
>> If this zero value is special and means "no attestation", then we
>> accidentally get no attestation when whatever is being hashed happens to
>> hash to this zero value.  Unlikely, but possible.
>>
>> If it's not special, then when and why is the ability to omit it useful?
>
> At some point, the zero value is special, because it is the default value if 
> no explicit one provided by user. But for TDX point of view, it is not 
> special. The field is a must for any TD, and whatever value it is, it will be 
> hashed into MRTD (Build-time Measurement Register) for later attestation.
>
> TDX architecture defines what fields are always hashed into measurement and 
> also provide other mechanism to hash optional field into measurement. All 
> this is known to users of TDX, and users can calculate the final measurement 
> by itself and compare to what gets reported by TDX to see they are identical.
>
> For these three fields, they are must-to-have fields to be hashed into 
> measurement. For user's convenience, we don't want to make it mandatory input 
> because not everyone cares it and have a specific value to input.
> What people needs to know is that, when no explicit value is provided for 
> these three field, a all-zero value is used.

Alright, the doc comment is not the place to educate me about TDX.
Perhaps we can go with

# @mrconfigid: ID for non-owner-defined configuration of the guest TD,
#     e.g., run-time or OS configuration (base64 encoded SHA384 digest).
#     Defaults to all zeroes.