[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 30/65] i386/tdx: Support user configurable mrconfigid/mrow
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v5 30/65] i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig |
Date: |
Thu, 29 Feb 2024 14:25:38 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) |
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?
[...]
- [PATCH v5 20/65] i386/tdx: Integrate tdx_caps->xfam_fixed0/1 into tdx_cpuid_lookup, (continued)
- [PATCH v5 20/65] i386/tdx: Integrate tdx_caps->xfam_fixed0/1 into tdx_cpuid_lookup, Xiaoyao Li, 2024/02/29
- [PATCH v5 24/65] i386/tdx: Initialize TDX before creating TD vcpus, Xiaoyao Li, 2024/02/29
- [PATCH v5 25/65] i386/tdx: Add property sept-ve-disable for tdx-guest object, Xiaoyao Li, 2024/02/29
- [PATCH v5 22/65] i386/kvm: Move architectural CPUID leaf generation to separate helper, Xiaoyao Li, 2024/02/29
- [PATCH v5 23/65] kvm: Introduce kvm_arch_pre_create_vcpu(), Xiaoyao Li, 2024/02/29
- [PATCH v5 32/65] i386/tdx: Set kvm_readonly_mem_enabled to false for TDX VM, Xiaoyao Li, 2024/02/29
- [PATCH v5 31/65] i386/tdx: Implement user specified tsc frequency, Xiaoyao Li, 2024/02/29
- [PATCH v5 30/65] i386/tdx: Support user configurable mrconfigid/mrowner/mrownerconfig, Xiaoyao Li, 2024/02/29
[PATCH v5 26/65] i386/tdx: Make sept_ve_disable set by default, Xiaoyao Li, 2024/02/29
[PATCH v5 33/65] kvm/tdx: Don't complain when converting vMMIO region to shared, Xiaoyao Li, 2024/02/29
[PATCH v5 27/65] i386/tdx: Wire CPU features up with attributes of TD guest, Xiaoyao Li, 2024/02/29
[PATCH v5 34/65] kvm/tdx: Ignore memory conversion to shared of unassigned region, Xiaoyao Li, 2024/02/29
[PATCH v5 35/65] memory: Introduce memory_region_init_ram_guest_memfd(), Xiaoyao Li, 2024/02/29
[PATCH v5 29/65] i386/tdx: Validate TD attributes, Xiaoyao Li, 2024/02/29
[PATCH v5 28/65] i386/tdx: Disable pmu for TD guest, Xiaoyao Li, 2024/02/29
[PATCH v5 37/65] i386/tdvf: Introduce function to parse TDVF metadata, Xiaoyao Li, 2024/02/29
[PATCH v5 39/65] i386/tdx: Skip BIOS shadowing setup, Xiaoyao Li, 2024/02/29