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

  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.

[1]: https://aepicleak.com/aepicleak.pdf

Link: https://lore.kernel.org/lkml/d6ffb489-7024-ff74-bd2f-d1e06573bb82@intel.com/
Link: https://lore.kernel.org/lkml/ba80b303-31bf-d44a-b05d-5c0f83038798@intel.com/
Signed-off-by: Kai Huang <kai.huang@intel.com>
---

v6 -> v7:
 - Changed to use "Link" for the two lore links to get rid of checkpatch
   warning.

---
 arch/x86/Kconfig            |  1 +
 arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
 2 files changed, 12 insertions(+)
  

On 11/20/22 4:26 PM, Kai Huang wrote:
> The MMIO/xAPIC interface has some problems, most notably the APIC LEAK

"some problems" looks more generic. May be we can be specific here. Like
it has security issues?

> [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.
> 
> [1]: https://aepicleak.com/aepicleak.pdf
> 
> Link: https://lore.kernel.org/lkml/d6ffb489-7024-ff74-bd2f-d1e06573bb82@intel.com/
> Link: https://lore.kernel.org/lkml/ba80b303-31bf-d44a-b05d-5c0f83038798@intel.com/
> Signed-off-by: Kai Huang <kai.huang@intel.com>
> ---
> 
> v6 -> v7:
>  - Changed to use "Link" for the two lore links to get rid of checkpatch
>    warning.
> 
> ---
>  arch/x86/Kconfig            |  1 +
>  arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cced4ef3bfb2..dd333b46fafb 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
>  	depends on CPU_SUP_INTEL
>  	depends on X86_64
>  	depends on KVM_INTEL
> +	depends on X86_X2APIC
>  	help
>  	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
>  	  host and certain physical attacks.  This option enables necessary TDX
> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> index 982d9c453b6b..8d943bdc8335 100644
> --- a/arch/x86/virt/vmx/tdx/tdx.c
> +++ b/arch/x86/virt/vmx/tdx/tdx.c
> @@ -12,6 +12,7 @@
>  #include <linux/printk.h>
>  #include <asm/msr-index.h>
>  #include <asm/msr.h>
> +#include <asm/apic.h>
>  #include <asm/tdx.h>
>  #include "tdx.h"
>  
> @@ -81,6 +82,16 @@ static int __init tdx_init(void)
>  		goto no_tdx;
>  	}
>  
> +	/*
> +	 * TDX requires X2APIC being enabled to prevent potential data
> +	 * leak via APIC MMIO registers.  Just disable TDX if not using
> +	 * X2APIC.

Remove the double space.

> +	 */
> +	if (!x2apic_enabled()) {
> +		pr_info("Disable TDX as X2APIC is not enabled.\n");

pr_warn()?

> +		goto no_tdx;
> +	}
> +
>  	return 0;
>  no_tdx:
>  	clear_tdx();
  

On Sun, 2022-11-20 at 19:51 -0800, Sathyanarayanan Kuppuswamy wrote:
> 
> On 11/20/22 4:26 PM, Kai Huang wrote:
> > The MMIO/xAPIC interface has some problems, most notably the APIC LEAK
> 
> "some problems" looks more generic. May be we can be specific here. Like
> it has security issues?

It was quoted from below upstream commit id (I only kept the one that I quoted
to save space):

commit b8d1d163604bd1e600b062fb00de5dc42baa355f (tag: x86_apic_for_v6.1_rc1,
tip/x86/apic)
Author: Daniel Sneddon <daniel.sneddon@linux.intel.com>
Date:   Tue Aug 16 16:19:42 2022 -0700

    x86/apic: Don't disable x2APIC if locked
    
    ....
    
    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.
    
    ....
    
    [1]: https://aepicleak.com/aepicleak.pdf
    
    Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
    Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
    Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
    Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>
    Link:
https://lkml.kernel.org/r/20220816231943.1152579-1-daniel.sneddon@linux.intel.com


> 
> > [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.
> > 
> > [1]: https://aepicleak.com/aepicleak.pdf
> > 
> > Link: https://lore.kernel.org/lkml/d6ffb489-7024-ff74-bd2f-d1e06573bb82@intel.com/
> > Link: https://lore.kernel.org/lkml/ba80b303-31bf-d44a-b05d-5c0f83038798@intel.com/
> > Signed-off-by: Kai Huang <kai.huang@intel.com>
> > ---
> > 
> > v6 -> v7:
> >  - Changed to use "Link" for the two lore links to get rid of checkpatch
> >    warning.
> > 
> > ---
> >  arch/x86/Kconfig            |  1 +
> >  arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index cced4ef3bfb2..dd333b46fafb 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
> >  	depends on CPU_SUP_INTEL
> >  	depends on X86_64
> >  	depends on KVM_INTEL
> > +	depends on X86_X2APIC
> >  	help
> >  	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
> >  	  host and certain physical attacks.  This option enables necessary TDX
> > diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
> > index 982d9c453b6b..8d943bdc8335 100644
> > --- a/arch/x86/virt/vmx/tdx/tdx.c
> > +++ b/arch/x86/virt/vmx/tdx/tdx.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/printk.h>
> >  #include <asm/msr-index.h>
> >  #include <asm/msr.h>
> > +#include <asm/apic.h>
> >  #include <asm/tdx.h>
> >  #include "tdx.h"
> >  
> > @@ -81,6 +82,16 @@ static int __init tdx_init(void)
> >  		goto no_tdx;
> >  	}
> >  
> > +	/*
> > +	 * TDX requires X2APIC being enabled to prevent potential data
> > +	 * leak via APIC MMIO registers.  Just disable TDX if not using
> > +	 * X2APIC.
> 
> Remove the double space.

Sorry which "double space"?

> 
> > +	 */
> > +	if (!x2apic_enabled()) {
> > +		pr_info("Disable TDX as X2APIC is not enabled.\n");
> 
> pr_warn()?

Why?
  

On 11/21/22 1:44 AM, Huang, Kai wrote:
> On Sun, 2022-11-20 at 19:51 -0800, Sathyanarayanan Kuppuswamy wrote:
>>
>> On 11/20/22 4:26 PM, Kai Huang wrote:
>>> The MMIO/xAPIC interface has some problems, most notably the APIC LEAK
>>
>> "some problems" looks more generic. May be we can be specific here. Like
>> it has security issues?
> 
> It was quoted from below upstream commit id (I only kept the one that I quoted
> to save space):

Ok.

> 
> commit b8d1d163604bd1e600b062fb00de5dc42baa355f (tag: x86_apic_for_v6.1_rc1,
> tip/x86/apic)
> Author: Daniel Sneddon <daniel.sneddon@linux.intel.com>
> Date:   Tue Aug 16 16:19:42 2022 -0700
> 
>     x86/apic: Don't disable x2APIC if locked
>     
>     ....
>     
>     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.
>     
>     ....
>     
>     [1]: https://aepicleak.com/aepicleak.pdf
>     
>     Signed-off-by: Daniel Sneddon <daniel.sneddon@linux.intel.com>
>     Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
>     Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
>     Tested-by: Neelima Krishnan <neelima.krishnan@intel.com>
>     Link:
> https://lkml.kernel.org/r/20220816231943.1152579-1-daniel.sneddon@linux.intel.com
> 
> 
>>
>>> [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.
>>>
>>> [1]: https://aepicleak.com/aepicleak.pdf
>>>
>>> Link: https://lore.kernel.org/lkml/d6ffb489-7024-ff74-bd2f-d1e06573bb82@intel.com/
>>> Link: https://lore.kernel.org/lkml/ba80b303-31bf-d44a-b05d-5c0f83038798@intel.com/
>>> Signed-off-by: Kai Huang <kai.huang@intel.com>
>>> ---
>>>
>>> v6 -> v7:
>>>  - Changed to use "Link" for the two lore links to get rid of checkpatch
>>>    warning.
>>>
>>> ---
>>>  arch/x86/Kconfig            |  1 +
>>>  arch/x86/virt/vmx/tdx/tdx.c | 11 +++++++++++
>>>  2 files changed, 12 insertions(+)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index cced4ef3bfb2..dd333b46fafb 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -1958,6 +1958,7 @@ config INTEL_TDX_HOST
>>>  	depends on CPU_SUP_INTEL
>>>  	depends on X86_64
>>>  	depends on KVM_INTEL
>>> +	depends on X86_X2APIC
>>>  	help
>>>  	  Intel Trust Domain Extensions (TDX) protects guest VMs from malicious
>>>  	  host and certain physical attacks.  This option enables necessary TDX
>>> diff --git a/arch/x86/virt/vmx/tdx/tdx.c b/arch/x86/virt/vmx/tdx/tdx.c
>>> index 982d9c453b6b..8d943bdc8335 100644
>>> --- a/arch/x86/virt/vmx/tdx/tdx.c
>>> +++ b/arch/x86/virt/vmx/tdx/tdx.c
>>> @@ -12,6 +12,7 @@
>>>  #include <linux/printk.h>
>>>  #include <asm/msr-index.h>
>>>  #include <asm/msr.h>
>>> +#include <asm/apic.h>
>>>  #include <asm/tdx.h>
>>>  #include "tdx.h"
>>>  
>>> @@ -81,6 +82,16 @@ static int __init tdx_init(void)
>>>  		goto no_tdx;
>>>  	}
>>>  
>>> +	/*
>>> +	 * TDX requires X2APIC being enabled to prevent potential data
>>> +	 * leak via APIC MMIO registers.  Just disable TDX if not using
>>> +	 * X2APIC.
>>
>> Remove the double space.
> 
> Sorry which "double space"?

Before Just disable. It looked like double space. Is it not?

> 
>>
>>> +	 */
>>> +	if (!x2apic_enabled()) {
>>> +		pr_info("Disable TDX as X2APIC is not enabled.\n");
>>
>> pr_warn()?
> 
> Why?

Since it is a failure case, I thought pr_warn would be better. It is up
to you.

>
  

On Mon, 2022-11-21 at 14:00 -0800, Sathyanarayanan Kuppuswamy wrote:
> > > >   
> > > > +	/*
> > > > +	 * TDX requires X2APIC being enabled to prevent potential data
> > > > +	 * leak via APIC MMIO registers.  Just disable TDX if not using
> > > > +	 * X2APIC.
> > > 
> > > Remove the double space.
> > 
> > Sorry which "double space"?
> 
> Before Just disable. It looked like double space. Is it not?

There are bunch of examples in the upstream kernel using "double space" both in
changelog and comment.
  

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.

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?
  

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@intel.com/

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.
"
  

On 11/21/22 16:30, Huang, Kai wrote:
> 
> 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.

That's fine and it makes logical sense as a dependency.  TDX host
support requires x2APIC.  Period.
  

On Mon, 2022-11-21 at 16:44 -0800, Dave Hansen wrote:
> On 11/21/22 16:30, Huang, Kai wrote:
> > 
> > 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.
> 
> That's fine and it makes logical sense as a dependency.  TDX host
> support requires x2APIC.  Period.
> 
Thanks.  Perhaps I can reuse your second sentence 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.  TDX host support requires
x2APIC.  Make INTEL_TDX_HOST depend on X86_X2APIC.
"