Xiaoyao Li <xiaoyao...@intel.com> writes: > On 3/7/2024 4:39 PM, Markus Armbruster wrote: >> Xiaoyao Li <xiaoyao...@intel.com> writes: >> >>> On 2/29/2024 9:25 PM, Markus Armbruster wrote: >>>> Xiaoyao Li <xiaoyao...@intel.com> writes: >>>> >>>>> On 2/29/2024 4:37 PM, Markus Armbruster wrote: >>>>>> Xiaoyao Li <xiaoyao...@intel.com> writes: >>>>>> >>>>>>> From: Isaku Yamahata <isaku.yamah...@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-4813f4e0f...@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.yamah...@intel.com> >>>>>>> Co-developed-by: Xiaoyao Li <xiaoyao...@intel.com> >>>>>>> Signed-off-by: Xiaoyao Li <xiaoyao...@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.