| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230112031201.GB2034518@ls.amr.corp.intel.com>
Date: Wed, 11 Jan 2023 19:12:01 -0800
From: Isaku Yamahata <isaku.yamahata@...il.com>
To: "Wang, Lei" <lei4.wang@...el.com>
Cc: isaku.yamahata@...el.com, kvm@...r.kernel.org,
linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
Paolo Bonzini <pbonzini@...hat.com>, erdemaktas@...gle.com,
Sean Christopherson <seanjc@...gle.com>,
Sagi Shahar <sagis@...gle.com>,
David Matlack <dmatlack@...gle.com>,
Xiaoyao Li <xiaoyao.li@...el.com>
Subject: Re: [PATCH v10 021/108] KVM: TDX: initialize VM with TDX specific
parameters
On Wed, Jan 04, 2023 at 03:59:08PM +0800,
"Wang, Lei" <lei4.wang@...el.com> wrote:
> On 10/30/2022 2:22 PM, isaku.yamahata@...el.com wrote:
> > From: Xiaoyao Li <xiaoyao.li@...el.com>
> >
> > TDX requires additional parameters for TDX VM for confidential execution to
> > protect its confidentiality of its memory contents and its CPU state from
> > any other software, including VMM. When creating guest TD VM before
> > creating vcpu, the number of vcpu, TSC frequency (that is same among
> > vcpus. and it can't be changed.) CPUIDs which is emulated by the TDX
> > module. It means guest can trust those CPUIDs. and sha384 values for
> > measurement.
> >
> > Add new subcommand, KVM_TDX_INIT_VM, to pass parameters for TDX guest. It
> > assigns encryption key to the TDX guest for memory encryption. TDX
> > encrypts memory per-guest bases. It assigns device model passes per-VM
> > parameters for the TDX guest. The maximum number of vcpus, tsc frequency
> > (TDX guest has fised VM-wide TSC frequency. not per-vcpu. The TDX guest
> > can not change it.), attributes (production or debug), available extended
> > features (which is reflected into guest XCR0, IA32_XSS MSR), cpuids, sha384
> > measurements, and etc.
> >
> > This subcommand is called before creating vcpu and KVM_SET_CPUID2, i.e.
> > cpuids configurations aren't available yet. So CPUIDs configuration values
> > needs to be passed in struct kvm_init_vm. It's device model responsibility
>
> I suppose this should be kvm_tdx_init_vm.
>
> > to make this cpuid config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
> >
> > Signed-off-by: Xiaoyao Li <xiaoyao.li@...el.com>
> > Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > ---
> > arch/x86/include/asm/tdx.h | 3 +
> > arch/x86/include/uapi/asm/kvm.h | 31 +++
> > arch/x86/kvm/vmx/tdx.c | 296 ++++++++++++++++++++++----
> > arch/x86/kvm/vmx/tdx.h | 22 ++
> > tools/arch/x86/include/uapi/asm/kvm.h | 33 +++
> > 5 files changed, 347 insertions(+), 38 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> > index cd304d323d33..05ac4bfc8f8a 100644
> > --- a/arch/x86/include/asm/tdx.h
> > +++ b/arch/x86/include/asm/tdx.h
> > @@ -131,6 +131,9 @@ static inline long tdx_kvm_hypercall(unsigned int nr, unsigned long p1,
> > #endif /* CONFIG_INTEL_TDX_GUEST && CONFIG_KVM_GUEST */
> >
> > #ifdef CONFIG_INTEL_TDX_HOST
> > +
> > +/* -1 indicates CPUID leaf with no sub-leaves. */
> > +#define TDX_CPUID_NO_SUBLEAF ((u32)-1)
> > struct tdx_cpuid_config {
> > u32 leaf;
> > u32 sub_leaf;
> > diff --git a/arch/x86/include/uapi/asm/kvm.h b/arch/x86/include/uapi/asm/kvm.h
> > index 2ad9666e02a5..26661879c031 100644
> > --- a/arch/x86/include/uapi/asm/kvm.h
> > +++ b/arch/x86/include/uapi/asm/kvm.h
> > @@ -538,6 +538,7 @@ struct kvm_pmu_event_filter {
> > /* Trust Domain eXtension sub-ioctl() commands. */
> > enum kvm_tdx_cmd_id {
> > KVM_TDX_CAPABILITIES = 0,
> > + KVM_TDX_INIT_VM,
> >
> > KVM_TDX_CMD_NR_MAX,
> > };
> > @@ -583,4 +584,34 @@ struct kvm_tdx_capabilities {
> > struct kvm_tdx_cpuid_config cpuid_configs[0];
> > };
> >
> > +struct kvm_tdx_init_vm {
> > + __u64 attributes;
> > + __u64 mrconfigid[6]; /* sha384 digest */
> > + __u64 mrowner[6]; /* sha384 digest */
> > + __u64 mrownerconfig[6]; /* sha348 digest */
> > + union {
> > + /*
> > + * KVM_TDX_INIT_VM is called before vcpu creation, thus before
> > + * KVM_SET_CPUID2. CPUID configurations needs to be passed.
> > + *
> > + * This configuration supersedes KVM_SET_CPUID{,2}.
> > + * The user space VMM, e.g. qemu, should make them consistent
> > + * with this values.
> > + * sizeof(struct kvm_cpuid_entry2) * KVM_MAX_CPUID_ENTRIES(256)
> > + * = 8KB.
> > + */
> > + struct {
> > + struct kvm_cpuid2 cpuid;
> > + /* 8KB with KVM_MAX_CPUID_ENTRIES. */
> > + struct kvm_cpuid_entry2 entries[];
> > + };
> > + /*
> > + * For future extensibility.
> > + * The size(struct kvm_tdx_init_vm) = 16KB.
> > + * This should be enough given sizeof(TD_PARAMS) = 1024
> > + */
> > + __u64 reserved[2029];
> > + };
> > +};
> > +
> > #endif /* _ASM_X86_KVM_H */
> > diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> > index d77709a6da51..54045e0576e7 100644
> > --- a/arch/x86/kvm/vmx/tdx.c
> > +++ b/arch/x86/kvm/vmx/tdx.c
> > @@ -284,6 +284,205 @@ static int tdx_do_tdh_mng_key_config(void *param)
> > int tdx_vm_init(struct kvm *kvm)
> > {
> > struct kvm_tdx *kvm_tdx = to_kvm_tdx(kvm);
> > +
> > + kvm_tdx->hkid = -1;
> > +
> > + /*
> > + * This function initializes only KVM software construct. It doesn't
> > + * initialize TDX stuff, e.g. TDCS, TDR, TDCX, HKID etc.
> > + * It is handled by KVM_TDX_INIT_VM, __tdx_td_init().
> > + */
> > +
> > + return 0;
> > +}
> > +
> > +int tdx_dev_ioctl(void __user *argp)
> > +{
> > + struct kvm_tdx_capabilities __user *user_caps;
> > + struct kvm_tdx_capabilities caps;
> > + struct kvm_tdx_cmd cmd;
> > +
> > + BUILD_BUG_ON(sizeof(struct kvm_tdx_cpuid_config) !=
> > + sizeof(struct tdx_cpuid_config));
> > +
> > + if (copy_from_user(&cmd, argp, sizeof(cmd)))
> > + return -EFAULT;
> > + if (cmd.flags || cmd.error || cmd.unused)
> > + return -EINVAL;
> > + /*
> > + * Currently only KVM_TDX_CAPABILITIES is defined for system-scoped
> > + * mem_enc_ioctl().
> > + */
> > + if (cmd.id != KVM_TDX_CAPABILITIES)
> > + return -EINVAL;
> > +
> > + user_caps = (void __user *)cmd.data;
> > + if (copy_from_user(&caps, user_caps, sizeof(caps)))
> > + return -EFAULT;
> > +
> > + if (caps.nr_cpuid_configs < tdx_caps.nr_cpuid_configs)
> > + return -E2BIG;
> > +
> > + caps = (struct kvm_tdx_capabilities) {
> > + .attrs_fixed0 = tdx_caps.attrs_fixed0,
> > + .attrs_fixed1 = tdx_caps.attrs_fixed1,
> > + .xfam_fixed0 = tdx_caps.xfam_fixed0,
> > + .xfam_fixed1 = tdx_caps.xfam_fixed1,
> > + .nr_cpuid_configs = tdx_caps.nr_cpuid_configs,
> > + .padding = 0,
> > + };
> > +
> > + if (copy_to_user(user_caps, &caps, sizeof(caps)))
> > + return -EFAULT;
> > + if (copy_to_user(user_caps->cpuid_configs, &tdx_caps.cpuid_configs,
> > + tdx_caps.nr_cpuid_configs *
> > + sizeof(struct tdx_cpuid_config)))
> > + return -EFAULT;
> > +
> > + return 0;
> > +}
>
> tdx_dev_ioctl() is introduced in previous patch with the same code added here,
> which means this is just a place change and it will confuse reviewers. Is it
> neccesary to do so?
Right. Somehow diff was also confused. I'll try to twist the previous patch to
introduce __tdx_td_init() with the previous patch.
...
> > +static inline bool is_td_initialized(struct kvm *kvm)
> > +{
> > + return to_kvm_tdx(kvm)->hkid > 0;
> > +}
>
> There is a similar function, is_hkid_assigned(), which is previously defined. Do
> you think redefining a new function here will bring code redundency?
I'll drop this function and replace it with is_hkid_assigned().
--
Isaku Yamahata <isaku.yamahata@...il.com>
Powered by blists - more mailing lists