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

[Xiaoyao Li <xiaoyao.li@intel.com>]

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.