linux-kernel - Re: [PATCH v17 092/116] KVM: TDX: Handle TDX PV HLT hypercall

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]

Date: Fri, 5 Jan 2024 15:05:12 -0800
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>, erdemaktas@...gle.com, 
	Sagi Shahar <sagis@...gle.com>, David Matlack <dmatlack@...gle.com>, Kai Huang <kai.huang@...el.com>, 
	Zhi Wang <zhi.wang.linux@...il.com>, chen.bo@...el.com, hang.yuan@...el.com, 
	tina.zhang@...el.com
Subject: Re: [PATCH v17 092/116] KVM: TDX: Handle TDX PV HLT hypercall

On Tue, Nov 07, 2023, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> Wire up TDX PV HLT hypercall to the KVM backend function.
> 
> Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> ---
>  arch/x86/kvm/vmx/tdx.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  arch/x86/kvm/vmx/tdx.h |  3 +++
>  2 files changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
> index 3a1fe74b95c3..4e48989d364f 100644
> --- a/arch/x86/kvm/vmx/tdx.c
> +++ b/arch/x86/kvm/vmx/tdx.c
> @@ -662,7 +662,32 @@ void tdx_vcpu_load(struct kvm_vcpu *vcpu, int cpu)
>  
>  bool tdx_protected_apic_has_interrupt(struct kvm_vcpu *vcpu)
>  {
> -	return pi_has_pending_interrupt(vcpu);
> +	bool ret = pi_has_pending_interrupt(vcpu);
> +	struct vcpu_tdx *tdx = to_tdx(vcpu);
> +
> +	if (ret || vcpu->arch.mp_state != KVM_MP_STATE_HALTED)
> +		return true;
> +
> +	if (tdx->interrupt_disabled_hlt)
> +		return false;
> +
> +	/*
> +	 * This is for the case where the virtual interrupt is recognized,
> +	 * i.e. set in vmcs.RVI, between the STI and "HLT".  KVM doesn't have
> +	 * access to RVI and the interrupt is no longer in the PID (because it
> +	 * was "recognized".  It doesn't get delivered in the guest because the
> +	 * TDCALL completes before interrupts are enabled.
> +	 *
> +	 * TDX modules sets RVI while in an STI interrupt shadow.
> +	 * - TDExit(typically TDG.VP.VMCALL<HLT>) from the guest to TDX module.
> +	 *   The interrupt shadow at this point is gone.
> +	 * - It knows that there is an interrupt that can be delivered
> +	 *   (RVI > PPR && EFLAGS.IF=1, the other conditions of 29.2.2 don't
> +	 *    matter)
> +	 * - It forwards the TDExit nevertheless, to a clueless hypervisor that
> +	 *   has no way to glean either RVI or PPR.

WTF.  Seriously, what in the absolute hell is going on.  I reported this internally
four ***YEARS*** ago.  This is not some obscure theoretical edge case, this is core
functionality and it's completely broken garbage.

NAK.  Hard NAK.  Fix the TDX module, full stop.

Even worse, TDX 1.5 apparently _already_ has the necessary logic for dealing with
interrupts that are pending in RVI when handling NESTED VM-Enter.  Really!?!?!
Y'all went and added nested virtualization support of some kind, but can't find
the time to get the basics right?