| Message ID | 20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
| Series | Add TDX Guest Attestation support | expand |
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.
On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote: > 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. Similar story with the unaccepted memory gunk. If it is not going to be used outside of encrypted guests, why are we polluting our already insanely humongous Kconfig space with more symbols?
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.
On 4/19/22 1:13 AM, Borislav Petkov wrote: > On Tue, Apr 19, 2022 at 07:47:33PM +1200, Kai Huang wrote: >> 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. > > Similar story with the unaccepted memory gunk. If it is not going to > be used outside of encrypted guests, why are we polluting our already > insanely humongous Kconfig space with more symbols? > Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION. Boris, this is a simple platform driver which adds IOCTL interfaces to allow user space to get TDREPORT and TDQuote support. So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ?
On 4/19/22 1:16 AM, Kai Huang wrote: > 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. IMO, we should not use a usage model to categorize "GetQuote" support as a mandatory or non-mandatory requirement. For customers who use VSOCK, they can get away without GetQuote TDVMCALL support. But for customers who do not want to use VSOCK model, this is a required support. AFAIK, our current customer requirement is to use TDVMCALL approach for attestation support. If your suggestion is to split GetQuote support as separate patch to make it easier for review, I am fine with such suggestion. Maintainers, any opinion? Would you prefer to split the driver into two patches? > > 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 GetQuote IOCTL support is a very simple feature support, so, IMO, we don't need to complicate it with additional config. > complexity things in terms of review.
On 4/19/22 00:47, Kai Huang wrote: >>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. Are you saying you can't think of a single threat model where there's a benefit to running a TDX guest without attestation? Will TDX only be used in environments where secrets are provisioned to guests on the basis of attestation? >>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? How much code are we talking about? What's the difference in the size of the binaries with this compiled in?
On 4/19/22 7:13 AM, Dave Hansen wrote: >> >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? > How much code are we talking about? What's the difference in the size > of the binaries with this compiled in? Current driver size is ~300 lines. It adds ~500 bytes to the kernel binary if it is built-in.
On 4/19/22 07:19, Sathyanarayanan Kuppuswamy wrote: > On 4/19/22 7:13 AM, Dave Hansen wrote: >>> >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? >> How much code are we talking about? What's the difference in the size >> of the binaries with this compiled in? > > Current driver size is ~300 lines. It adds ~500 bytes to the kernel > binary if it is built-in. That doesn't sound like good use of a Kconfig option to me. Just explain in the cover letter: Any distribution enabling TDX is also expected to need attestation. The compiled size is quite small (500 bytes).
On 4/19/22 7:24 AM, Dave Hansen wrote: >> Current driver size is ~300 lines. It adds ~500 bytes to the kernel >> binary if it is built-in. > That doesn't sound like good use of a Kconfig option to me. Just > explain in the cover letter: > > Any distribution enabling TDX is also expected to need > attestation. The compiled size is quite small (500 bytes). Ok. I will add it.
On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote: > On 4/19/22 00:47, Kai Huang wrote: > > > 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. > > Are you saying you can't think of a single threat model where there's a > benefit to running a TDX guest without attestation? Will TDX only be > used in environments where secrets are provisioned to guests on the > basis of attestation? > > > I don't think anyone should provision secret to a TD before it get attested that it is a genuine TD that he/she expected. If someone does that, he/she takes the risk of losing the secret. Of course if someone just want to try a TD then w/o attestation is totally fine.
On Tue, 2022-04-19 at 07:00 -0700, Sathyanarayanan Kuppuswamy wrote: > > On 4/19/22 1:16 AM, Kai Huang wrote: > > 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. > > IMO, we should not use a usage model to categorize "GetQuote" support > as a mandatory or non-mandatory requirement. > > For customers who use VSOCK, they can get away without GetQuote > TDVMCALL support. But for customers who do not want to use > VSOCK model, this is a required support. AFAIK, our current customer > requirement is to use TDVMCALL approach for attestation support. > > If your suggestion is to split GetQuote support as separate > patch to make it easier for review, I am fine with such > suggestion. > I am not saying we should get rid of GetQuote support. If there's customer wants this with a good reason, we can certainly support it. I understand that some customer wants to deploy QE in host and don't want additional communication channel (i.e. vsock) between guest and host, which may add additional attack window and/or customer's validation resource. My point is regardless whether we need to support GetQuote, logically this driver can be split to two parts as I said: 1) basic TDREPORT support to userspace; 2) additional GetQuote support. And I think there are many benefits if you do in this way as I commented below. > > > > 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 > > > GetQuote IOCTL support is a very simple feature support, so, IMO, we > don't need to complicate it with additional config. > > > Additional Kconfig can reduce attack window by turning it off for people don't need it. Anyway no strong opinion here.
On 4/19/22 15:21, Kai Huang wrote: > On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote: >> On 4/19/22 00:47, Kai Huang wrote: >>>> 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. >> Are you saying you can't think of a single threat model where there's a >> benefit to running a TDX guest without attestation? Will TDX only be >> used in environments where secrets are provisioned to guests on the >> basis of attestation? >> > I don't think anyone should provision secret to a TD before it get attested that > it is a genuine TD that he/she expected. If someone does that, he/she takes the > risk of losing the secret. Of course if someone just want to try a TD then w/o > attestation is totally fine. Yeah, but you said: w/o attestation support in TD guest, I guess nobody will seriously use TD guest. I'm trying to get to the bottom of that. That's a much more broad statement than something about when it's safe to deploy secrets. There are lots of secrets deployed in (serious) VMs today. There are lots of secrets deployed in (serious) SEV VMs that don't have attestation. Yet, the world somehow hasn't come crashing down. I think it's crazy to say that nobody will deploy secrets to TDX VMs without attestation. I think it's a step father into crazy land to say that no one will "seriously" use TDX guests without attestation. Let's be honest about this and not live in some fantasy world, please.
On Tue, 2022-04-19 at 15:49 -0700, Dave Hansen wrote: > On 4/19/22 15:21, Kai Huang wrote: > > On Tue, 2022-04-19 at 07:13 -0700, Dave Hansen wrote: > > > On 4/19/22 00:47, Kai Huang wrote: > > > > > 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. > > > Are you saying you can't think of a single threat model where there's a > > > benefit to running a TDX guest without attestation? Will TDX only be > > > used in environments where secrets are provisioned to guests on the > > > basis of attestation? > > > > > I don't think anyone should provision secret to a TD before it get attested that > > it is a genuine TD that he/she expected. If someone does that, he/she takes the > > risk of losing the secret. Of course if someone just want to try a TD then w/o > > attestation is totally fine. > > Yeah, but you said: > > w/o attestation support in TD guest, I guess nobody will > seriously use TD guest. > > I'm trying to get to the bottom of that. That's a much more broad > statement than something about when it's safe to deploy secrets. > > There are lots of secrets deployed in (serious) VMs today. There are > lots of secrets deployed in (serious) SEV VMs that don't have > attestation. Yet, the world somehow hasn't come crashing down. > > I think it's crazy to say that nobody will deploy secrets to TDX VMs > without attestation. I think it's a step father into crazy land to say > that no one will "seriously" use TDX guests without attestation. > > Let's be honest about this and not live in some fantasy world, please. OK agree. No argument about this.
On Fri, Apr 15, 2022 at 03:01:09PM -0700, Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: ... > 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 ... > +static long tdx_attest_ioctl(struct file *file, unsigned int cmd, > + unsigned long arg) > +{ > + struct attest_dev *adev = platform_get_drvdata(pdev); > + void __user *argp = (void __user *)arg; > + struct tdx_gen_quote tdquote_req; > + long ret = 0, err; > + > + mutex_lock(&adev->lock); > + > + switch (cmd) { > + case TDX_CMD_GET_TDREPORT: > + if (copy_from_user(adev->report_buf, argp, > + TDX_REPORT_DATA_LEN)) { > + ret = -EFAULT; > + break; > + } > + > + /* Generate TDREPORT_STRUCT */ > + err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf); > + if (err) { > + ret = put_user(err, (long __user *)argp); > + ret = -EIO; > + break; > + } > + > + if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN)) > + ret = -EFAULT; > + break; > + case TDX_CMD_GEN_QUOTE: > + reinit_completion(&adev->req_compl); > + > + /* Copy TDREPORT data from user buffer */ > + if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) { > + ret = -EFAULT; > + break; > + } > + > + if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) { > + ret = -EINVAL; > + break; > + } > + > + if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf, > + tdquote_req.len)) { > + ret = -EFAULT; > + break; > + } > + > + /* Submit GetQuote Request */ > + err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE); > + if (err) { > + ret = put_user(err, (long __user *)argp); > + ret = -EIO; > + break; > + } > + > + /* Wait for attestation completion */ > + ret = wait_for_completion_interruptible_timeout( > + &adev->req_compl, > + msecs_to_jiffies(GET_QUOTE_TIMEOUT)); If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe to continue to using adev->tdquote_buf. VMM would continue to processing getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again, and tdquote_buf is re-used?
On 4/19/22 6:20 PM, Isaku Yamahata wrote: > If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe > to continue to using adev->tdquote_buf. VMM would continue to processing > getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again, > and tdquote_buf is re-used? This part is not clearly discussed in the specification. May be spec should define some reasonable timeout and teardown details. Regarding not using this buffer again, what happens if we de-allocate it on timeout and the host still updates it?
On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote: > Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION. Sounds like you didn't read my mail. Kai was questioning the need for a Kconfig symbol at all. And me too. If this thing is not going to be used by anything else besides TDX guests, why does it need a Kconfig option at all?! > Boris, this is a simple platform driver which adds IOCTL interfaces to allow > user space to get TDREPORT and TDQuote support. > > So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ? See above.
Hi Boris, On 4/20/22 3:00 PM, Borislav Petkov wrote: > On Tue, Apr 19, 2022 at 05:48:57AM -0700, Sathyanarayanan Kuppuswamy wrote: >> Make sense. We can just go with CONFIG_INTEL_TDX_ATTESTATION. > > Sounds like you didn't read my mail. Kai was questioning the need for a > Kconfig symbol at all. And me too. > > If this thing is not going to be used by anything else besides TDX > guests, why does it need a Kconfig option at all?! Sorry, it is a typo. I meant we can just compile it with TDX guest config (CONFIG_INTEL_TDX_GUEST). > >> Boris, this is a simple platform driver which adds IOCTL interfaces to allow >> user space to get TDREPORT and TDQuote support. >> >> So, would prefer to leave in platform/x86 or move it to arch/x86/coco/tdx/ ? > > See above. I agree with dropping the new CONFIG option. But regarding driver location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)? >
On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote: > TDX guest supports encrypted disk as root or secondary drives. > Decryption keys required to access such drives are usually maintained > by 3rd party key servers. Attestation is required by 3rd party key > servers to get the key for an encrypted disk volume, or possibly other > encrypted services. Attestation is used to prove to the key server that > the TD guest is running in a valid TD and the kernel and virtual BIOS > and other environment are secure. > > During the boot process various components before the kernel accumulate > hashes in the TDX module, which can then combined into a report. This > would typically include a hash of the bios, bios configuration, boot > loader, command line, kernel, initrd. After checking the hashes the > key server will securely release the keys. > > The actual details of the attestation protocol depend on the particular > key server configuration, but some parts are common and need to > communicate with the TDX module. As we discussed "key provisioning from key server to support disk decryption" is only one use case of attestation, so I don't think you need 3 paragraphs to talk about details of this use case here. The attestation flow is documented in GHCI so it's clear. The attestation flow (and what this patch does) does not have any direct relation to the "disk decryption" details above. I think you can discard above entirely or using one or two simple sentences to explain. Also, as you agreed you will remove the 8K shared memory assumption: https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/ and if you agree with my approach (again, I recommend) to split the driver to two parts (reorganize your patches essentially): https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8 I'll review again once you finish updating the driver. Btw some minor comments below. [...] > --- /dev/null > +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c Perhaps attest.c is enough, no matter where the file will reside. > @@ -0,0 +1,302 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * intel_tdx_attest.c - TDX guest attestation interface driver. > + * > + * Implements user interface to trigger attestation process and > + * read the TD Quote result. > + * > + * Copyright (C) 2021-2022 Intel Corporation For upstream I guess just need 2022. [...] > +struct attest_dev { > + /* Mutex to serialize attestation requests */ > + struct mutex lock; I think we need a comment to explain why the driver doesn't support multiple GetQuote requests in parallel. In fact the updated GHCI spec doesn't explicitly say GetQuote cannot be done in parallel. It has a new "GET_QUOTE_IN_FLIGHT" flag introduced, which can be used to determine which Quote is finished I think. I am fine with only supporting GetQuote in serialized way, but perhaps we need to call out the reason somewhere. [...] > + > + /* Allocate DMA buffer to get TDQUOTE data from the VMM */ > + adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE, > + &adev->handle, > + GFP_KERNEL | __GFP_ZERO); > + if (!adev->tdquote_buf) { > + ret = -ENOMEM; > + goto failed; > + } The buffer needs to be shared. Guest must have called MapGPA to convert the buffer to shared. Is this guaranteed by calling dma_alloc_coherent(), since seems I didn't see any MapGPA in the driver? Anyway this deserves a comment I think.
Hi, On 4/20/22 4:18 PM, Kai Huang wrote: > On Fri, 2022-04-15 at 15:01 -0700, Kuppuswamy Sathyanarayanan wrote: >> TDX guest supports encrypted disk as root or secondary drives. >> Decryption keys required to access such drives are usually maintained >> by 3rd party key servers. Attestation is required by 3rd party key >> servers to get the key for an encrypted disk volume, or possibly other >> encrypted services. Attestation is used to prove to the key server that >> the TD guest is running in a valid TD and the kernel and virtual BIOS >> and other environment are secure. >> >> During the boot process various components before the kernel accumulate >> hashes in the TDX module, which can then combined into a report. This >> would typically include a hash of the bios, bios configuration, boot >> loader, command line, kernel, initrd. After checking the hashes the >> key server will securely release the keys. >> >> The actual details of the attestation protocol depend on the particular >> key server configuration, but some parts are common and need to >> communicate with the TDX module. > > As we discussed "key provisioning from key server to support disk decryption" is > only one use case of attestation, so I don't think you need 3 paragraphs to talk > about details of this use case here. The attestation flow is documented in GHCI Not everyone understands the attestation context and usage. So I have attempted to explain one of the use-case in detail. > so it's clear. The attestation flow (and what this patch does) does not have > any direct relation to the "disk decryption" details above. I think you can > discard above entirely or using one or two simple sentences to explain. Ok. I will try to summarize the details in few lines > > Also, as you agreed you will remove the 8K shared memory assumption: > > https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/ Yes. I have already removed this restriction. This will be part of my next submission. > > and if you agree with my approach (again, I recommend) to split the driver to > two parts (reorganize your patches essentially): Yes. I have moved the GetQuote support to a new patch (but without new config). > > https://lore.kernel.org/lkml/20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com/T/#m9e3c5115df0be0b53d41f987e1eda1715255d1d8 > > I'll review again once you finish updating the driver. Thanks. > > Btw some minor comments below. > > > [...] > >> --- /dev/null >> +++ b/drivers/platform/x86/intel/tdx/intel_tdx_attest.c > > Perhaps attest.c is enough, no matter where the file will reside. Noted. I will change it. > >> @@ -0,0 +1,302 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * intel_tdx_attest.c - TDX guest attestation interface driver. >> + * >> + * Implements user interface to trigger attestation process and >> + * read the TD Quote result. >> + * >> + * Copyright (C) 2021-2022 Intel Corporation > > For upstream I guess just need 2022. > > [...] Noted. I will change it. > >> +struct attest_dev { >> + /* Mutex to serialize attestation requests */ >> + struct mutex lock; > > I think we need a comment to explain why the driver doesn't support multiple > GetQuote requests in parallel. In fact the updated GHCI spec doesn't explicitly > say GetQuote cannot be done in parallel. It has a new "GET_QUOTE_IN_FLIGHT" > flag introduced, which can be used to determine which Quote is finished I think. > > I am fine with only supporting GetQuote in serialized way, but perhaps we need > to call out the reason somewhere. If we want to support multiple GetQuote requests in parallel, then we need some way to uniquely identify the GetQuote requests. So that when we get completion notification, we can understand which request is completed. This part is not mentioned/discussed in ABI spec. So we want to serialize the requests for now. I will include the details about it in the commit log. > > [...] > >> + >> + /* Allocate DMA buffer to get TDQUOTE data from the VMM */ >> + adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE, >> + &adev->handle, >> + GFP_KERNEL | __GFP_ZERO); >> + if (!adev->tdquote_buf) { >> + ret = -ENOMEM; >> + goto failed; >> + } > > The buffer needs to be shared. Guest must have called MapGPA to convert the > buffer to shared. Is this guaranteed by calling dma_alloc_coherent(), since > seems I didn't see any MapGPA in the driver? Anyway this deserves a comment I > think. Yes. It is taken care by dma_alloc_*() API. DMA memory is marked by default as shared in TDX guest. I will add comment about it. > >
On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote: > If we want to support multiple GetQuote requests in parallel, then we > need some way to uniquely identify the GetQuote requests. So that when > we get completion notification, we can understand which request is > completed. This part is not mentioned/discussed in ABI spec. So we want > to serialize the requests for now. > Yes it's unfortunate that this part (whether concurrent GetQuote requests are supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am fine with only supporting GetQuote requests one by one. AFAICT there's no request to support concurrent GetQuote requests anyway. What concerns me is exactly how explain this. As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue multiple GetQuote requests, and when you receive the interrupt, you check which buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote ready. However I am not 100% sure whether above will always work. Interrupt can get lost when there are multiple Quotes ready in multiple buffer in very short time period, etc? Perhaps Isaku can provide more input here. Anyway, how about explaining in this way: "The GHCI spec doesn't clearly say whether TDX can support or how to support multiple GetQuote requests in parallel. Attestation request is not supposed to be frequent and should not be in performance critical path. Only support GetQuote requests in serialized way for now."
On 4/20/22 5:11 PM, Kai Huang wrote: > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote: >> If we want to support multiple GetQuote requests in parallel, then we >> need some way to uniquely identify the GetQuote requests. So that when >> we get completion notification, we can understand which request is >> completed. This part is not mentioned/discussed in ABI spec. So we want >> to serialize the requests for now. >> > > Yes it's unfortunate that this part (whether concurrent GetQuote requests are > supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am > fine with only supporting GetQuote requests one by one. AFAICT there's no > request to support concurrent GetQuote requests anyway. What concerns me is > exactly how explain this. > > As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue > multiple GetQuote requests, and when you receive the interrupt, you check which > buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote > ready. However I am not 100% sure whether above will always work. Interrupt > can get lost when there are multiple Quotes ready in multiple buffer in very > short time period, etc? Perhaps Isaku can provide more input here. Either supported or not, it should be mentioned in the GHCI spec. Currently, there are no details related to it. If it is supported, the specification should include the protocol to use. I will check with Isaku about it. > > Anyway, how about explaining in this way: > > "The GHCI spec doesn't clearly say whether TDX can support or how to support > multiple GetQuote requests in parallel. Attestation request is not supposed to > be frequent and should not be in performance critical path. Only support > GetQuote requests in serialized way for now." It seems good enough. I will use it. > >
On Wed, Apr 20, 2022 at 07:42:06PM -0700, Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > On 4/20/22 5:11 PM, Kai Huang wrote: > > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote: > > > If we want to support multiple GetQuote requests in parallel, then we > > > need some way to uniquely identify the GetQuote requests. So that when > > > we get completion notification, we can understand which request is > > > completed. This part is not mentioned/discussed in ABI spec. So we want > > > to serialize the requests for now. > > > > > > > Yes it's unfortunate that this part (whether concurrent GetQuote requests are > > supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am > > fine with only supporting GetQuote requests one by one. AFAICT there's no > > request to support concurrent GetQuote requests anyway. What concerns me is > > exactly how explain this. > > > > As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue > > multiple GetQuote requests, and when you receive the interrupt, you check which > > buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote > > ready. However I am not 100% sure whether above will always work. Interrupt > > can get lost when there are multiple Quotes ready in multiple buffer in very > > short time period, etc? Perhaps Isaku can provide more input here. > > Either supported or not, it should be mentioned in the GHCI spec. Currently, > there are no details related to it. If it is supported, the specification > should include the protocol to use. > > I will check with Isaku about it. The spec says that TD can call multiple GetQuote requests in parallel. TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's implementation specific that how many concurrent requests are allowed. The TD should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple requests simultaneously As Kai said, there is no requirement for multiple GetQuote in parallel, it's okay to support only single request at the same time. While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before sending next request.
On Tue, Apr 19, 2022 at 06:26:43PM -0700, Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > On 4/19/22 6:20 PM, Isaku Yamahata wrote: > > If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe > > to continue to using adev->tdquote_buf. VMM would continue to processing > > getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again, > > and tdquote_buf is re-used? > > This part is not clearly discussed in the specification. May be spec > should define some reasonable timeout and teardown details. > > Regarding not using this buffer again, what happens if we de-allocate > it on timeout and the host still updates it? Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD attestation driver shouldn't reuse/free the pages. In the case of this driver, I think of two options - don't timeout. wait for interrupt to arrive and check the shared GPA state. - allow timeout. When the next request comes, check the shared GPA state. If it's still GET_QUOTE_IN_FLIGHT, return EBUSY. It's possible for VMM to keep the shared GPA forever maliciously(DoS) or unintentionally due to bug. TD can't do much about it.
On Wed, Apr 20, 2022 at 03:09:44PM -0700, Sathyanarayanan Kuppuswamy wrote: > I agree with dropping the new CONFIG option. But regarding driver > location, which is preferred (platform/x86/intel/tdx or arch/x86/coco/tdx/)? Well, since this is not really a driver but the attestation part of TDX, then arch/x86/coco/tdx/ sounds like the place. Thx.
On Wed, 2022-04-20 at 23:57 -0700, Isaku Yamahata wrote: > On Wed, Apr 20, 2022 at 07:42:06PM -0700, > Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > > > > > On 4/20/22 5:11 PM, Kai Huang wrote: > > > On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote: > > > > If we want to support multiple GetQuote requests in parallel, then we > > > > need some way to uniquely identify the GetQuote requests. So that when > > > > we get completion notification, we can understand which request is > > > > completed. This part is not mentioned/discussed in ABI spec. So we want > > > > to serialize the requests for now. > > > > > > > > > > Yes it's unfortunate that this part (whether concurrent GetQuote requests are > > > supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am > > > fine with only supporting GetQuote requests one by one. AFAICT there's no > > > request to support concurrent GetQuote requests anyway. What concerns me is > > > exactly how explain this. > > > > > > As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue > > > multiple GetQuote requests, and when you receive the interrupt, you check which > > > buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote > > > ready. However I am not 100% sure whether above will always work. Interrupt > > > can get lost when there are multiple Quotes ready in multiple buffer in very > > > short time period, etc? Perhaps Isaku can provide more input here. > > > > Either supported or not, it should be mentioned in the GHCI spec. Currently, > > there are no details related to it. If it is supported, the specification > > should include the protocol to use. > > > > I will check with Isaku about it. > > The spec says that TD can call multiple GetQuote requests in parallel. > > TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's > implementation specific that how many concurrent requests are allowed. The TD > should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple > requests simultaneously > > As Kai said, there is no requirement for multiple GetQuote in parallel, it's > okay to support only single request at the same time. > > While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The > attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before > sending next request. Sorry I missed this in the spec. Then as I mentioned above, TD should check which buffer has GET_QUOTE_IN_FLIGHT bit cleared to determine which GetQuote request is done? I guess this is the only way. Anyway, supporting single request only is fine to me. Just needs some explanation in comments or commit message.
On 4/21/22 12:04 AM, Isaku Yamahata wrote: > On Tue, Apr 19, 2022 at 06:26:43PM -0700, > Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >> On 4/19/22 6:20 PM, Isaku Yamahata wrote: >>> If timeout occurs, the state of adev->tdquote_buf is unknown. It's not safe >>> to continue to using adev->tdquote_buf. VMM would continue to processing >>> getquote request with this buffer. What if TDX_CMD_GEN_QUOTE is issued again, >>> and tdquote_buf is re-used? >> >> This part is not clearly discussed in the specification. May be spec >> should define some reasonable timeout and teardown details. >> >> Regarding not using this buffer again, what happens if we de-allocate >> it on timeout and the host still updates it? > > Until GET_QUOTE_IN_FLIGHT is cleared, the shared page is owned by VMM, TD > attestation driver shouldn't reuse/free the pages. > > In the case of this driver, I think of two options > - don't timeout. wait for interrupt to arrive and check the shared GPA state. > - allow timeout. When the next request comes, check the shared GPA state. > If it's still GET_QUOTE_IN_FLIGHT, return EBUSY. Out of the above two options, I think option 1 is better. It is simpler to serialize them using mutex compared to checking the shared buffer of previous request. > > It's possible for VMM to keep the shared GPA forever maliciously(DoS) or > unintentionally due to bug. TD can't do much about it. I will add a note about it in commit log.
On 4/20/22 11:57 PM, Isaku Yamahata wrote: > On Wed, Apr 20, 2022 at 07:42:06PM -0700, > Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > >> >> >> On 4/20/22 5:11 PM, Kai Huang wrote: >>> On Wed, 2022-04-20 at 16:45 -0700, Sathyanarayanan Kuppuswamy wrote: >>>> If we want to support multiple GetQuote requests in parallel, then we >>>> need some way to uniquely identify the GetQuote requests. So that when >>>> we get completion notification, we can understand which request is >>>> completed. This part is not mentioned/discussed in ABI spec. So we want >>>> to serialize the requests for now. >>>> >>> >>> Yes it's unfortunate that this part (whether concurrent GetQuote requests are >>> supported by TDX architecture) is not explicitly mentioned in GHCI spec. I am >>> fine with only supporting GetQuote requests one by one. AFAICT there's no >>> request to support concurrent GetQuote requests anyway. What concerns me is >>> exactly how explain this. >>> >>> As I said, we have GET_QUOTE_IN_FLIGHT flag now. Theoretically, you can queue >>> multiple GetQuote requests, and when you receive the interrupt, you check which >>> buffer has GET_QUOTE_IN_FLIGHT cleared. That buffer is the one with Quote >>> ready. However I am not 100% sure whether above will always work. Interrupt >>> can get lost when there are multiple Quotes ready in multiple buffer in very >>> short time period, etc? Perhaps Isaku can provide more input here. >> >> Either supported or not, it should be mentioned in the GHCI spec. Currently, >> there are no details related to it. If it is supported, the specification >> should include the protocol to use. >> >> I will check with Isaku about it. > > The spec says that TD can call multiple GetQuote requests in parallel. Sorry, I missed the above content. Thanks for pointing out. > > TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's > implementation specific that how many concurrent requests are allowed. The TD > should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple > requests simultaneously Do you know why we should handle VMCALL_RETRY case? IIUC, as per above spec, if each request we send uses different GPA buffer, then we should not even worry about checking for IN_FLIGHT status. right? > > As Kai said, there is no requirement for multiple GetQuote in parallel, it's > okay to support only single request at the same time. For now I will leave it as single request at a time. > > While the status is GET_QUOTE_IN_FLIGHT, VMM owns the shared GPA. The > attestation driver should wait for GET_QUOTE_IN_FLIGHT to be cleared before > sending next request.
On 4/21/22 2:10 AM, Borislav Petkov wrote: > Well, since this is not really a driver but the attestation part of TDX, > then arch/x86/coco/tdx/ sounds like the place. Ok. I will move it to arch/x86/coco/tdx/attest.c
On Thu, Apr 21, 2022 at 07:53:39AM -0700, Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > On 4/20/22 11:57 PM, Isaku Yamahata wrote: > > On Wed, Apr 20, 2022 at 07:42:06PM -0700, > > Sathyanarayanan Kuppuswamy <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > > > TDG.VP.VMCALL<GetQuote> API allows one TD to issue multiple requests. It's > > implementation specific that how many concurrent requests are allowed. The TD > > should be able to handle TDG.VP.VMCALL_RETRY if it chooses to issue multiple > > requests simultaneously > > Do you know why we should handle VMCALL_RETRY case? IIUC, as per > above spec, if each request we send uses different GPA buffer, then we > should not even worry about checking for IN_FLIGHT status. right? Not correct. User space VMM, e.g. qemu, may return RETRY error for various reasons even if different GPAs are used or even if only single request is issued at the same time. Other user space VMM (there are severals alternatives to qemu) would support TDX in future. They would choose different way to implement. Attestation driver should check IN_FLIGHT always before processing shared GPA. Interrupt notifies only that it needs attention from attestation driver. It doesn't guarantee that IN_FLIGHT is cleared. Interrupt indicates only that the state may be changed. It may not be changed. VMM inject the iterrupt spuriously.
diff --git a/drivers/platform/x86/intel/Kconfig b/drivers/platform/x86/intel/Kconfig index 1f01a8a23c57..a2e2a5a29bde 100644 --- a/drivers/platform/x86/intel/Kconfig +++ b/drivers/platform/x86/intel/Kconfig @@ -12,7 +12,7 @@ source "drivers/platform/x86/intel/speed_select_if/Kconfig" source "drivers/platform/x86/intel/telemetry/Kconfig" source "drivers/platform/x86/intel/wmi/Kconfig" source "drivers/platform/x86/intel/uncore-frequency/Kconfig" - +source "drivers/platform/x86/intel/tdx/Kconfig" config INTEL_HID_EVENT tristate "Intel HID Event" diff --git a/drivers/platform/x86/intel/Makefile b/drivers/platform/x86/intel/Makefile index c61bc3e97121..6b7c94051519 100644 --- a/drivers/platform/x86/intel/Makefile +++ b/drivers/platform/x86/intel/Makefile @@ -10,6 +10,7 @@ obj-$(CONFIG_INTEL_SKL_INT3472) += int3472/ obj-$(CONFIG_INTEL_PMC_CORE) += pmc/ obj-$(CONFIG_INTEL_PMT_CLASS) += pmt/ obj-$(CONFIG_INTEL_SPEED_SELECT_INTERFACE) += speed_select_if/ +obj-$(CONFIG_INTEL_TDX_GUEST) += tdx/ obj-$(CONFIG_INTEL_TELEMETRY) += telemetry/ obj-$(CONFIG_INTEL_WMI) += wmi/ obj-$(CONFIG_INTEL_UNCORE_FREQ_CONTROL) += uncore-frequency/ diff --git a/drivers/platform/x86/intel/tdx/Kconfig b/drivers/platform/x86/intel/tdx/Kconfig new file mode 100644 index 000000000000..332a10313b49 --- /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 @@ -0,0 +1,302 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * intel_tdx_attest.c - TDX guest attestation interface driver. + * + * Implements user interface to trigger attestation process and + * read the TD Quote result. + * + * Copyright (C) 2021-2022 Intel Corporation + * + * Author: + * Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> + */ + +#define pr_fmt(fmt) "x86/tdx: attest: " fmt + +#include <linux/module.h> +#include <linux/miscdevice.h> +#include <linux/uaccess.h> +#include <linux/fs.h> +#include <linux/mm.h> +#include <linux/slab.h> +#include <linux/set_memory.h> +#include <linux/dma-mapping.h> +#include <linux/platform_device.h> +#include <linux/jiffies.h> +#include <linux/io.h> +#include <asm/apic.h> +#include <asm/tdx.h> +#include <asm/irq_vectors.h> +#include <uapi/misc/tdx.h> + +#define DRIVER_NAME "tdx-attest" + +/* Used in Quote memory allocation */ +#define QUOTE_SIZE (2 * PAGE_SIZE) +/* Used in Get Quote request memory allocation */ +#define GET_QUOTE_MAX_SIZE (4 * PAGE_SIZE) +/* Get Quote timeout in msec */ +#define GET_QUOTE_TIMEOUT (5000) + +struct attest_dev { + /* Mutex to serialize attestation requests */ + struct mutex lock; + /* Completion object to track GetQuote completion status */ + struct completion req_compl; + /* Buffer used to copy report data in attestation handler */ + u8 report_buf[TDX_REPORT_DATA_LEN] __aligned(64); + /* Data pointer used to get TD Quote data in attestation handler */ + void *tdquote_buf; + /* Data pointer used to get TDREPORT data in attestation handler */ + void *tdreport_buf; + /* DMA handle used to allocate and free tdquote DMA buffer */ + dma_addr_t handle; + struct miscdevice miscdev; +}; + +static struct platform_device *pdev; + +static void attestation_callback_handler(void) +{ + struct attest_dev *adev = platform_get_drvdata(pdev); + + complete(&adev->req_compl); +} + +static long tdx_attest_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) +{ + struct attest_dev *adev = platform_get_drvdata(pdev); + void __user *argp = (void __user *)arg; + struct tdx_gen_quote tdquote_req; + long ret = 0, err; + + mutex_lock(&adev->lock); + + switch (cmd) { + case TDX_CMD_GET_TDREPORT: + if (copy_from_user(adev->report_buf, argp, + TDX_REPORT_DATA_LEN)) { + ret = -EFAULT; + break; + } + + /* Generate TDREPORT_STRUCT */ + err = tdx_mcall_tdreport(adev->tdreport_buf, adev->report_buf); + if (err) { + ret = put_user(err, (long __user *)argp); + ret = -EIO; + break; + } + + if (copy_to_user(argp, adev->tdreport_buf, TDX_TDREPORT_LEN)) + ret = -EFAULT; + break; + case TDX_CMD_GEN_QUOTE: + reinit_completion(&adev->req_compl); + + /* Copy TDREPORT data from user buffer */ + if (copy_from_user(&tdquote_req, argp, sizeof(struct tdx_gen_quote))) { + ret = -EFAULT; + break; + } + + if (tdquote_req.len <= 0 || tdquote_req.len > GET_QUOTE_MAX_SIZE) { + ret = -EINVAL; + break; + } + + if (copy_from_user(adev->tdquote_buf, (void __user *)tdquote_req.buf, + tdquote_req.len)) { + ret = -EFAULT; + break; + } + + /* Submit GetQuote Request */ + err = tdx_hcall_get_quote(adev->tdquote_buf, GET_QUOTE_MAX_SIZE); + if (err) { + ret = put_user(err, (long __user *)argp); + ret = -EIO; + break; + } + + /* Wait for attestation completion */ + ret = wait_for_completion_interruptible_timeout( + &adev->req_compl, + msecs_to_jiffies(GET_QUOTE_TIMEOUT)); + if (ret <= 0) { + ret = -EIO; + break; + } + + /* ret will be positive if completed. */ + ret = 0; + + if (copy_to_user((void __user *)tdquote_req.buf, adev->tdquote_buf, + tdquote_req.len)) + ret = -EFAULT; + + break; + case TDX_CMD_GET_QUOTE_SIZE: + ret = put_user(QUOTE_SIZE, (u64 __user *)argp); + break; + default: + pr_err("cmd %d not supported\n", cmd); + break; + } + + mutex_unlock(&adev->lock); + + return ret; +} + +static const struct file_operations tdx_attest_fops = { + .owner = THIS_MODULE, + .unlocked_ioctl = tdx_attest_ioctl, + .llseek = no_llseek, +}; + +/* Helper function to cleanup attestation related allocations */ +static void _tdx_attest_remove(struct attest_dev *adev) +{ + misc_deregister(&adev->miscdev); + + tdx_remove_ev_notify_handler(); + + if (adev->tdquote_buf) + dma_free_coherent(&pdev->dev, GET_QUOTE_MAX_SIZE, + adev->tdquote_buf, adev->handle); + + if (adev->tdreport_buf) + free_pages((unsigned long)adev->tdreport_buf, 0); + + kfree(adev); +} + +static int tdx_attest_probe(struct platform_device *attest_pdev) +{ + struct device *dev = &attest_pdev->dev; + struct attest_dev *adev; + long ret = 0; + + /* Only single device is allowed */ + if (pdev) + return -EBUSY; + + adev = kzalloc(sizeof(*adev), GFP_KERNEL); + if (!adev) + return -ENOMEM; + + mutex_init(&adev->lock); + init_completion(&adev->req_compl); + pdev = attest_pdev; + platform_set_drvdata(pdev, adev); + + /* + * tdreport_data needs to be 64-byte aligned. + * Full page alignment is more than enough. + */ + adev->tdreport_buf = (void *)__get_free_pages(GFP_KERNEL | __GFP_ZERO, + 0); + if (!adev->tdreport_buf) { + ret = -ENOMEM; + goto failed; + } + + ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(64)); + if (ret) { + pr_err("dma set coherent mask failed\n"); + goto failed; + } + + /* Allocate DMA buffer to get TDQUOTE data from the VMM */ + adev->tdquote_buf = dma_alloc_coherent(dev, GET_QUOTE_MAX_SIZE, + &adev->handle, + GFP_KERNEL | __GFP_ZERO); + if (!adev->tdquote_buf) { + ret = -ENOMEM; + goto failed; + } + + /* Register attestation event notify handler */ + tdx_setup_ev_notify_handler(attestation_callback_handler); + + adev->miscdev.name = DRIVER_NAME; + adev->miscdev.minor = MISC_DYNAMIC_MINOR; + adev->miscdev.fops = &tdx_attest_fops; + adev->miscdev.parent = dev; + + ret = misc_register(&adev->miscdev); + if (ret) { + pr_err("misc device registration failed\n"); + goto failed; + } + + pr_debug("module initialization success\n"); + + return 0; + +failed: + _tdx_attest_remove(adev); + + pr_debug("module initialization failed\n"); + + return ret; +} + +static int tdx_attest_remove(struct platform_device *attest_pdev) +{ + struct attest_dev *adev = platform_get_drvdata(attest_pdev); + + mutex_lock(&adev->lock); + _tdx_attest_remove(adev); + mutex_unlock(&adev->lock); + pr_debug("module is successfully removed\n"); + return 0; +} + +static struct platform_driver tdx_attest_driver = { + .probe = tdx_attest_probe, + .remove = tdx_attest_remove, + .driver = { + .name = DRIVER_NAME, + }, +}; + +static int __init tdx_attest_init(void) +{ + int ret; + + /* Make sure we are in a valid TDX platform */ + if (!cpu_feature_enabled(X86_FEATURE_TDX_GUEST)) + return -EIO; + + ret = platform_driver_register(&tdx_attest_driver); + if (ret) { + pr_err("failed to register driver, err=%d\n", ret); + return ret; + } + + pdev = platform_device_register_simple(DRIVER_NAME, -1, NULL, 0); + if (IS_ERR(pdev)) { + ret = PTR_ERR(pdev); + pr_err("failed to allocate device, err=%d\n", ret); + platform_driver_unregister(&tdx_attest_driver); + return ret; + } + + return 0; +} + +static void __exit tdx_attest_exit(void) +{ + platform_device_unregister(pdev); + platform_driver_unregister(&tdx_attest_driver); +} + +module_init(tdx_attest_init); +module_exit(tdx_attest_exit); + +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>"); +MODULE_DESCRIPTION("TDX attestation driver"); +MODULE_LICENSE("GPL v2"); diff --git a/include/uapi/misc/tdx.h b/include/uapi/misc/tdx.h new file mode 100644 index 000000000000..9920f36c79fe --- /dev/null +++ b/include/uapi/misc/tdx.h @@ -0,0 +1,42 @@ +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ +#ifndef _UAPI_MISC_TDX_H +#define _UAPI_MISC_TDX_H + +#include <linux/types.h> +#include <linux/ioctl.h> + +/* Input report data length for TDX_CMD_GET_TDREPORT IOCTL request */ +#define TDX_REPORT_DATA_LEN 64 + +/* Output TD report data length after TDX_CMD_GET_TDREPORT IOCTL execution */ +#define TDX_TDREPORT_LEN 1024 + +/* + * TDX_CMD_GET_TDREPORT IOCTL is used to get TDREPORT data from the TDX + * Module. Users should pass report data of size TDX_REPORT_DATA_LEN bytes + * via user input buffer of size TDX_TDREPORT_LEN. Once IOCTL is successful + * TDREPORT data is copied to the user buffer. + */ +#define TDX_CMD_GET_TDREPORT _IOWR('T', 0x01, __u64) + +/* + * TDX_CMD_GEN_QUOTE IOCTL is used to request TD QUOTE from the VMM. User + * should pass TD report data of size TDX_TDREPORT_LEN bytes via user input + * buffer of quote size. Once IOCTL is successful quote data is copied back to + * the user buffer. + */ +#define TDX_CMD_GEN_QUOTE _IOR('T', 0x02, __u64) + +/* + * TDX_CMD_GET_QUOTE_SIZE IOCTL is used to get the TD Quote size info in bytes. + * This will be used for determining the input buffer allocation size when + * using TDX_CMD_GEN_QUOTE IOCTL. + */ +#define TDX_CMD_GET_QUOTE_SIZE _IOR('T', 0x03, __u64) + +struct tdx_gen_quote { + __u64 buf; + __u64 len; +}; + +#endif /* _UAPI_MISC_TDX_H */
TDX guest supports encrypted disk as root or secondary drives. Decryption keys required to access such drives are usually maintained by 3rd party key servers. Attestation is required by 3rd party key servers to get the key for an encrypted disk volume, or possibly other encrypted services. Attestation is used to prove to the key server that the TD guest is running in a valid TD and the kernel and virtual BIOS and other environment are secure. During the boot process various components before the kernel accumulate hashes in the TDX module, which can then combined into a report. This would typically include a hash of the bios, bios configuration, boot loader, command line, kernel, initrd. After checking the hashes the key server will securely release the keys. The actual details of the attestation protocol depend on the particular key server configuration, but some parts are common and need to communicate with the TDX module. This communication is implemented in the attestation driver. The supported steps are: 1. TD guest generates the TDREPORT that contains version information about the Intel TDX module, measurement of the TD, along with a TD-specified nonce. 2. TD guest shares the TDREPORT with TD host via GetQuote hypercall which is used by the host to generate a quote via quoting enclave (QE). 3. Quote generation completion notification is sent to TD OS via callback interrupt vector configured by TD using SetupEventNotifyInterrupt hypercall. 4. After receiving the generated TDQUOTE, a remote verifier can be used to verify the quote and confirm the trustworthiness of the TD. Attestation agent uses IOCTLs implemented by the attestation driver to complete the various steps of the attestation process. Also note that, explicit access permissions are not enforced in this driver because the quote and measurements are not a secret. However the access permissions of the device node can be used to set any desired access policy. The udev default is usually root access only. TDX_CMD_GEN_QUOTE IOCTL can be used to create an computation on the host, but TDX assumes that the host is able to deal with malicious guest flooding it anyways. The interaction with the TDX module is like a RPM protocol here. There are several operations (get tdreport, get quote) that need to input a blob, and then output another blob. It was considered to use a sysfs interface for this, but it doesn't fit well into the standard sysfs model for configuring values. It would be possible to do read/write on files, but it would need multiple file descriptors, which would be somewhat messy. ioctls seems to be the best fitting and simplest model here. There is one ioctl per operation, that takes the input blob and returns the output blob, and as well as auxiliary ioctls to return the blob lengths. The ioctls are documented in the header file. [Chenyi Qiang: Proposed struct tdx_gen_quote for passing user buffer] Reviewed-by: Tony Luck <tony.luck@intel.com> Reviewed-by: Andi Kleen <ak@linux.intel.com> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Acked-by: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> --- Changes since v2: * Combined attestation related global variables into struct attest_dev to make the code clean. * Added support to pass TDREPORT and GetQuote TDCALLs error code back to user on failure. * Modified the driver to use platform device driver model and added check to ensure only one device is allowed. drivers/platform/x86/intel/Kconfig | 2 +- drivers/platform/x86/intel/Makefile | 1 + drivers/platform/x86/intel/tdx/Kconfig | 13 + drivers/platform/x86/intel/tdx/Makefile | 3 + .../platform/x86/intel/tdx/intel_tdx_attest.c | 302 ++++++++++++++++++ include/uapi/misc/tdx.h | 42 +++ 6 files changed, 362 insertions(+), 1 deletion(-) create mode 100644 drivers/platform/x86/intel/tdx/Kconfig create mode 100644 drivers/platform/x86/intel/tdx/Makefile create mode 100644 drivers/platform/x86/intel/tdx/intel_tdx_attest.c create mode 100644 include/uapi/misc/tdx.h