| Message ID | 20240229063726.610065-31-xiaoyao.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | QEMU Guest memfd + QEMU TDX support | expand |
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> > > --- > Changes in v5: > - refine the description of QAPI properties and add description of > default value when not specified; > > Changes in v4: > - describe more of there fields in qom.json > - free the old value before set new value to avoid memory leak in > _setter(); (Daniel) > > Changes in v3: > - use base64 encoding instread of hex-string; > --- > qapi/qom.json | 17 ++++++++- > target/i386/kvm/tdx.c | 87 +++++++++++++++++++++++++++++++++++++++++++ > target/i386/kvm/tdx.h | 3 ++ > 3 files changed, 106 insertions(+), 1 deletion(-) > > 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". 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? > + > + if (tdx_guest->mrowner) { > + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrowner, > + strlen(tdx_guest->mrowner), &data_len, errp); > + if (!data || data_len != SHA384_DIGEST_SIZE) { > + error_setg(errp, "TDX: failed to decode mrowner"); > + return -1; > + } > + memcpy(init_vm->mrowner, data, data_len); > + } > + > + if (tdx_guest->mrownerconfig) { > + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrownerconfig, > + strlen(tdx_guest->mrownerconfig), &data_len, errp); > + if (!data || data_len != SHA384_DIGEST_SIZE) { > + error_setg(errp, "TDX: failed to decode mrownerconfig"); > + return -1; > + } > + memcpy(init_vm->mrownerconfig, data, data_len); > + } > + > r = kvm_vm_enable_cap(kvm_state, KVM_CAP_MAX_VCPUS, 0, ms->smp.cpus); > if (r < 0) { > error_setg(errp, "Unable to set MAX VCPUS to %d", ms->smp.cpus); [...]
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> >> >> --- >> Changes in v5: >> - refine the description of QAPI properties and add description of >> default value when not specified; >> >> Changes in v4: >> - describe more of there fields in qom.json >> - free the old value before set new value to avoid memory leak in >> _setter(); (Daniel) >> >> Changes in v3: >> - use base64 encoding instread of hex-string; >> --- >> qapi/qom.json | 17 ++++++++- >> target/i386/kvm/tdx.c | 87 +++++++++++++++++++++++++++++++++++++++++++ >> target/i386/kvm/tdx.h | 3 ++ >> 3 files changed, 106 insertions(+), 1 deletion(-) >> >> 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. > 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. >> + >> + if (tdx_guest->mrowner) { >> + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrowner, >> + strlen(tdx_guest->mrowner), &data_len, errp); >> + if (!data || data_len != SHA384_DIGEST_SIZE) { >> + error_setg(errp, "TDX: failed to decode mrowner"); >> + return -1; >> + } >> + memcpy(init_vm->mrowner, data, data_len); >> + } >> + >> + if (tdx_guest->mrownerconfig) { >> + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrownerconfig, >> + strlen(tdx_guest->mrownerconfig), &data_len, errp); >> + if (!data || data_len != SHA384_DIGEST_SIZE) { >> + error_setg(errp, "TDX: failed to decode mrownerconfig"); >> + return -1; >> + } >> + memcpy(init_vm->mrownerconfig, data, data_len); >> + } >> + >> r = kvm_vm_enable_cap(kvm_state, KVM_CAP_MAX_VCPUS, 0, ms->smp.cpus); >> if (r < 0) { >> error_setg(errp, "Unable to set MAX VCPUS to %d", ms->smp.cpus); > > [...] >
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? [...]
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.
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?
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.
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.
On 3/7/2024 9:56 PM, Markus Armbruster wrote: > 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. I'm pleased to help you (on or off the maillist) if you have any question on 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. > I will update to this in next version. Huge thanks for your help!
Xiaoyao Li <xiaoyao.li@intel.com> writes: > On 3/7/2024 9:56 PM, Markus Armbruster wrote: [...] >> Alright, the doc comment is not the place to educate me about TDX. > > I'm pleased to help you (on or off the maillist) if you have any question on TDX. :) I can't possibly become knowledgable about QEMU's entire external interface, it's simply too much. Where I'm ignorant, I try to learn just enough to ensure the documentation is useful. I appreciate your kind & patient help with that. >> 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. >> > > I will update to this in next version. > Huge thanks for your help! You're welcome!
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). +# +# @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); + } + + if (tdx_guest->mrowner) { + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrowner, + strlen(tdx_guest->mrowner), &data_len, errp); + if (!data || data_len != SHA384_DIGEST_SIZE) { + error_setg(errp, "TDX: failed to decode mrowner"); + return -1; + } + memcpy(init_vm->mrowner, data, data_len); + } + + if (tdx_guest->mrownerconfig) { + g_autofree uint8_t *data = qbase64_decode(tdx_guest->mrownerconfig, + strlen(tdx_guest->mrownerconfig), &data_len, errp); + if (!data || data_len != SHA384_DIGEST_SIZE) { + error_setg(errp, "TDX: failed to decode mrownerconfig"); + return -1; + } + memcpy(init_vm->mrownerconfig, data, data_len); + } + r = kvm_vm_enable_cap(kvm_state, KVM_CAP_MAX_VCPUS, 0, ms->smp.cpus); if (r < 0) { error_setg(errp, "Unable to set MAX VCPUS to %d", ms->smp.cpus); @@ -574,6 +608,51 @@ static void tdx_guest_set_sept_ve_disable(Object *obj, bool value, Error **errp) } } +static char * tdx_guest_get_mrconfigid(Object *obj, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + return g_strdup(tdx->mrconfigid); +} + +static void tdx_guest_set_mrconfigid(Object *obj, const char *value, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + g_free(tdx->mrconfigid); + tdx->mrconfigid = g_strdup(value); +} + +static char * tdx_guest_get_mrowner(Object *obj, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + return g_strdup(tdx->mrowner); +} + +static void tdx_guest_set_mrowner(Object *obj, const char *value, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + g_free(tdx->mrowner); + tdx->mrowner = g_strdup(value); +} + +static char * tdx_guest_get_mrownerconfig(Object *obj, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + return g_strdup(tdx->mrownerconfig); +} + +static void tdx_guest_set_mrownerconfig(Object *obj, const char *value, Error **errp) +{ + TdxGuest *tdx = TDX_GUEST(obj); + + g_free(tdx->mrownerconfig); + tdx->mrownerconfig = g_strdup(value); +} + /* tdx guest */ OBJECT_DEFINE_TYPE_WITH_INTERFACES(TdxGuest, tdx_guest, @@ -593,6 +672,14 @@ static void tdx_guest_init(Object *obj) object_property_add_bool(obj, "sept-ve-disable", tdx_guest_get_sept_ve_disable, tdx_guest_set_sept_ve_disable); + object_property_add_str(obj, "mrconfigid", + tdx_guest_get_mrconfigid, + tdx_guest_set_mrconfigid); + object_property_add_str(obj, "mrowner", + tdx_guest_get_mrowner, tdx_guest_set_mrowner); + object_property_add_str(obj, "mrownerconfig", + tdx_guest_get_mrownerconfig, + tdx_guest_set_mrownerconfig); } static void tdx_guest_finalize(Object *obj) diff --git a/target/i386/kvm/tdx.h b/target/i386/kvm/tdx.h index 0df910725b52..2697e6bdfb1d 100644 --- a/target/i386/kvm/tdx.h +++ b/target/i386/kvm/tdx.h @@ -21,6 +21,9 @@ typedef struct TdxGuest { bool initialized; uint64_t attributes; /* TD attributes */ + char *mrconfigid; /* base64 encoded sha348 digest */ + char *mrowner; /* base64 encoded sha348 digest */ + char *mrownerconfig; /* base64 encoded sha348 digest */ } TdxGuest; #ifdef CONFIG_TDX