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

[Markus Armbruster]

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.

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?