Re: [PATCH v7 03/20] x86/virt/tdx: Disable TDX if X2APIC is not enabled

[Huang, Kai]

On Mon, 2022-11-21 at 15:46 -0800, Dave Hansen wrote:
> On 11/20/22 16:26, Kai Huang wrote:
> > The MMIO/xAPIC interface has some problems, most notably the APIC LEAK
> > [1].  This bug allows an attacker to use the APIC MMIO interface to
> > extract data from the SGX enclave.
> > 
> > TDX is not immune from this either.  Early check X2APIC and disable TDX
> > if X2APIC is not enabled, and make INTEL_TDX_HOST depend on X86_X2APIC.
> 
> This makes no sense.
> 
> This is TDX host code.  TDX hosts are untrusted.  Zero of the TDX
> security guarantees are provided by the host.
> 
> What is the benefit of disabling TDX from the host if x2APIC is not
> enabled?  It can't be for security reasons since the host does not help
> provide TDX security guarantees.  It also can't be for SGX because SGX
> doesn't depend on the OS doing anything in order to be secure.

Agreed.  Although in practice I think if we do some hardening in the kernel, it
would raise some attack bar.

> 
> So, this boils down to the most fundamental of questions you need to
> answer about every patch:
> 
> What does this code do?
> 
> What end-user-visible effect is there if this code is not present?

Considering TDX host cannot be trusted (i.e. can be attacked/modified), I agree
the check isn't needed.

I was following your suggestion in the patch which handles "x2apic locked" case:

https://lore.kernel.org/lkml/ba80b303-31bf-d44a-b05d-5c0f83038798@xxxxxxxxx/

I guess I misunderstood your point.

Reading that discussion again, if I understand correctly, you just want to make
INTEL_TDX_HOST depend on X86_X2APIC?

How about still having a patch to make INTEL_TDX_HOST depend on X86_X2APIC but
with something below in the changelog?

"
TDX capable platforms are locked to X2APIC mode and cannot fall back to the
legacy xAPIC mode when TDX is enabled by the BIOS.   It doesn't make sense to
turn on INTEL_TDX_HOST while X86_X2APIC is not enabled.  Make INTEL_TDX_HOST
depend on X86_X2APIC.
"