linux-kernel - Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific parameters

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <c11cd64487f8971f9cfa880bface2076eb5b8b6d.camel@intel.com>
Date: Mon, 8 Apr 2024 18:38:56 +0000
From: "Edgecombe, Rick P" <rick.p.edgecombe@...el.com>
To: "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "Yamahata,
 Isaku" <isaku.yamahata@...el.com>
CC: "Zhang, Tina" <tina.zhang@...el.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "Yuan, Hang" <hang.yuan@...el.com>, "Huang, Kai"
	<kai.huang@...el.com>, "Chen, Bo2" <chen.bo@...el.com>, "sagis@...gle.com"
	<sagis@...gle.com>, "isaku.yamahata@...il.com" <isaku.yamahata@...il.com>,
	"Aktas, Erdem" <erdemaktas@...gle.com>, "Li, Xiaoyao" <xiaoyao.li@...el.com>,
	"pbonzini@...hat.com" <pbonzini@...hat.com>
Subject: Re: [PATCH v19 039/130] KVM: TDX: initialize VM with TDX specific
 parameters

On Mon, 2024-02-26 at 00:25 -0800, isaku.yamahata@...el.com wrote:
> +static int setup_tdparams_xfam(struct kvm_cpuid2 *cpuid, struct td_params *td_params)
> +{
> +       const struct kvm_cpuid_entry2 *entry;
> +       u64 guest_supported_xcr0;
> +       u64 guest_supported_xss;
> +
> +       /* Setup td_params.xfam */
> +       entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 0);
> +       if (entry)
> +               guest_supported_xcr0 = (entry->eax | ((u64)entry->edx << 32));
> +       else
> +               guest_supported_xcr0 = 0;
> +       guest_supported_xcr0 &= kvm_caps.supported_xcr0;
> +
> +       entry = kvm_find_cpuid_entry2(cpuid->entries, cpuid->nent, 0xd, 1);
> +       if (entry)
> +               guest_supported_xss = (entry->ecx | ((u64)entry->edx << 32));
> +       else
> +               guest_supported_xss = 0;
> +
> +       /*
> +        * PT and CET can be exposed to TD guest regardless of KVM's XSS, PT
> +        * and, CET support.
> +        */
> +       guest_supported_xss &=
> +               (kvm_caps.supported_xss | XFEATURE_MASK_PT | TDX_TD_XFAM_CET);

So this enables features based on xss support in the passed CPUID, but these features are not
dependent xsave. You could have CET without xsave support. And in fact Kernel IBT doesn't use it. To
utilize CPUID leafs to configure features, but diverge from the HW meaning seems like asking for
trouble.

> +
> +       td_params->xfam = guest_supported_xcr0 | guest_supported_xss;
> +       if (td_params->xfam & XFEATURE_MASK_LBR) {
> +               /*
> +                * TODO: once KVM supports LBR(save/restore LBR related
> +                * registers around TDENTER), remove this guard.
> +                */
> +#define MSG_LBR        "TD doesn't support LBR yet. KVM needs to save/restore IA32_LBR_DEPTH
> properly.\n"
> +               pr_warn(MSG_LBR);
> +               return -EOPNOTSUPP;
> +       }
> +
> +       return 0;
> +}
> +
> +static int setup_tdparams(struct kvm *kvm, struct td_params *td_params,
> +                       struct kvm_tdx_init_vm *init_vm)
> +{
> +       struct kvm_cpuid2 *cpuid = &init_vm->cpuid;
> +       int ret;
> +
> +       if (kvm->created_vcpus)
> +               return -EBUSY;
> +
> +       if (init_vm->attributes & TDX_TD_ATTRIBUTE_PERFMON) {
> +               /*
> +                * TODO: save/restore PMU related registers around TDENTER.
> +                * Once it's done, remove this guard.
> +                */
> +#define MSG_PERFMON    "TD doesn't support perfmon yet. KVM needs to save/restore host perf
> registers properly.\n"
> +               pr_warn(MSG_PERFMON);

We need to remove the TODOs and a warn doesn't seem appropriate.

> +               return -EOPNOTSUPP;
> +       }
> +
> +       td_params->max_vcpus = kvm->max_vcpus;
> +       td_params->attributes = init_vm->attributes;

Don't we need to sanitize this for a selection of features known to KVM. For example what if
something else like TDX_TD_ATTRIBUTE_PERFMON is added to a future TDX module and then suddenly
userspace can configure it.

So xfam is how to control features that are tied to save (CET, etc). And ATTRIBUTES are tied to
features without xsave support (PKS, etc).

If we are going to use CPUID for specifying which features should get enabled in the TDX module, we
should match the arch definitions of the leafs. For things like CET whether xfam controls the value
of multiple CPUID leafs, then we need should check that they are all set to some consistent values
and otherwise reject them. So for CET we would need to check the SHSTK and IBT bits, as well as two
XCR0 bits.

If we are going to do that for XFAM based features, then why not do the same for ATTRIBUTE based
features?

We would need something like GET_SUPPORTED_CPUID for TDX, but also since some features can be forced
on we would need to expose something like GET_SUPPORTED_CPUID_REQUIRED as well. 

> +       td_params->exec_controls = TDX_CONTROL_FLAG_NO_RBP_MOD;
> +       td_params->tsc_frequency = TDX_TSC_KHZ_TO_25MHZ(kvm->arch.default_tsc_khz);