Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver

From: Kai Huang <kai.huang@intel.com>
To: Kuppuswamy Sathyanarayanan 
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, Hans de Goede <hdegoede@redhat.com>,
	Mark Gross <mgross@linux.intel.com>
Cc: "H . Peter Anvin" <hpa@zytor.com>,
	"Kirill A . Shutemov" <kirill.shutemov@linux.intel.com>,
	Tony Luck <tony.luck@intel.com>, Andi Kleen <ak@linux.intel.com>,
	linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver
Date: Tue, 19 Apr 2022 20:16:48 +1200	[thread overview]
Message-ID: <0d532b0ce1155bf7778366b14c5d1311c45fef01.camel@intel.com> (raw)
In-Reply-To: <bd83067542a3519ee4c91f9d50e9bd4fac27e4bb.camel@intel.com>

On Tue, 2022-04-19 at 19:47 +1200, Kai Huang wrote:
> On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote:
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Kconfig
> > @@ -0,0 +1,13 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +#
> > +# X86 TDX Platform Specific Drivers
> > +#
> > +
> > +config INTEL_TDX_ATTESTATION
> > +	tristate "Intel TDX attestation driver"
> > +	depends on INTEL_TDX_GUEST
> > +	help
> > +	  The TDX attestation driver provides IOCTL interfaces to the user to
> > +	  request TDREPORT from the TDX module or request quote from the VMM
> > +	  or to get quote buffer size. It is mainly used to get secure disk
> > +	  decryption keys from the key server.
> > diff --git a/drivers/platform/x86/intel/tdx/Makefile b/drivers/platform/x86/intel/tdx/Makefile
> > new file mode 100644
> > index 000000000000..94eea6108fbd
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License-Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_INTEL_TDX_ATTESTATION)	+= intel_tdx_attest.o
> > diff --git a/drivers/platform/x86/intel/tdx/intel_tdx_attest.c b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> > new file mode 100644
> > index 000000000000..9124db800d4f
> > --- /dev/null
> > +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c
> 
> 
> From security's perspective, attestation is an essential part of TDX.  That
> being said, w/o attestation support in TD guest, I guess nobody will seriously
> use TD guest.
> 
> From this perspective, I am not sure what's the value of having a dedicated
> INTEL_TDX_ATTESTATION Kconfig.  The attestation support code should be turned on
> unconditionally when CONFIG_INTEL_TDX_GUEST is on.  The code can also be just
> under arch/x86/coco/tdx/ I guess?
> 
> But I'll leave this to maintainers.

In fact after slightly thinking more, I think you can split TDREPORT TDCALL
support with GetQuote/SetupEventNotifyInterrupt support.  The reason is as I
said, GetQuote isn't mandatory to support attestation.  TD attestation agent can
use i.e. vsock, tcp/ip, to communicate to QE directly.  Whether kernel needs to
support GetQuote is actually arguable.

So IMHO you can split this attestation driver into two parts:

1) A "basic" driver which supports reporting TDREPORT to userspace
2) Additional support of GetQuote/SetupEventNotifyInterrupt.

The 1) can even be in a single patch (I guess it won't be complicated).  It is
easy to review (and i.e. can be merged separately), and with it, you will
immediately have one way to support attestation.

2) can be reviewed separately, perhaps with one additional Kconfig option (i.e.
CONFIG_INTEL_TDX_ATTESTATION_GET_QUOTE).  I think this part has most of the
complexity things in terms of review.

-- 
Thanks,
-Kai