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

[Markus Armbruster <armbru@redhat.com>]

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.