linux-kernel - Re: [PATCH v7 043/102] KVM: x86/mmu: Focibly use TDP MMU for TDX

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: <Ysw6HdGSIECkP5RC@google.com>
Date:   Mon, 11 Jul 2022 14:56:29 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     isaku.yamahata@...el.com
Cc:     kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [PATCH v7 043/102] KVM: x86/mmu: Focibly use TDP MMU for TDX

s/Focibly/Forcibly, but that's a moot point because KVM shouldn't override the
the module param.  KVM should instead _require_ the TDP MMU to be enabled.  E.g.
if userspace disables the TDP MMU to workaround a fatal bug, then forcing the TDP
MMU may silently expose KVM to said bug.

And overriding tdp_enabled is just mind-boggling broken, all of the SPTE masks
will be wrong.

On Mon, Jun 27, 2022, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> In this patch series, TDX supports only TDP MMU and doesn't support legacy
> MMU.  Forcibly use TDP MMU for TDX irrelevant of kernel parameter to
> disable TDP MMU.

Do not refer to the "patch series", instead phrase the statement with respect to
what KVM support.

  Require the TDP MMU for TDX guests, the so called "shadow" MMU does not
  support mapping guest private memory, i.e. does not support Secure-EPT.

> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
>  arch/x86/kvm/mmu/tdp_mmu.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu/tdp_mmu.c b/arch/x86/kvm/mmu/tdp_mmu.c
> index 82f1bfac7ee6..7eb41b176d1e 100644
> --- a/arch/x86/kvm/mmu/tdp_mmu.c
> +++ b/arch/x86/kvm/mmu/tdp_mmu.c
> @@ -18,8 +18,13 @@ int kvm_mmu_init_tdp_mmu(struct kvm *kvm)
>  {
>  	struct workqueue_struct *wq;
>  
> -	if (!tdp_enabled || !READ_ONCE(tdp_mmu_enabled))
> -		return 0;
> +	/*
> +	 *  Because TDX supports only TDP MMU, forcibly use TDP MMU in the case
> +	 *  of TDX.
> +	 */
> +	if (kvm->arch.vm_type != KVM_X86_TDX_VM &&
> +		(!tdp_enabled || !READ_ONCE(tdp_mmu_enabled)))
> +		return false;

Yeah, no.

	if (!tdp_enabled || !READ_ONCE(tdp_mmu_enabled))
		return kvm->arch.vm_type == KVM_X86_TDX_VM ? -EINVAL : 0;

>  
>  	wq = alloc_workqueue("kvm", WQ_UNBOUND|WQ_MEM_RECLAIM|WQ_CPU_INTENSIVE, 0);
>  	if (!wq)
> -- 
> 2.25.1
>