All of lore.kernel.org
 help / color / mirror / Atom feed
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: Thu, 21 Apr 2022 11:18:43 +1200	[thread overview]
Message-ID: <b209ee09b74394ab7aed85e0244e2191ee3d4171.camel@intel.com> (raw)
In-Reply-To: <20220415220109.282834-5-sathyanarayanan.kuppuswamy@linux.intel.com>

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.


-- 
Thanks,
-Kai



  parent reply	other threads:[~2022-04-20 23:18 UTC|newest]

Thread overview: 48+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-15 22:01 [PATCH v3 0/4] Add TDX Guest Attestation support Kuppuswamy Sathyanarayanan
2022-04-15 22:01 ` [PATCH v3 1/4] x86/tdx: Add tdx_mcall_tdreport() API support Kuppuswamy Sathyanarayanan
2022-04-19  2:29   ` Kai Huang
2022-04-19  3:37     ` Sathyanarayanan Kuppuswamy
2022-04-19  3:51       ` Kai Huang
2022-04-19  3:53         ` Sathyanarayanan Kuppuswamy
2022-04-15 22:01 ` [PATCH v3 2/4] x86/tdx: Add tdx_hcall_get_quote() " Kuppuswamy Sathyanarayanan
2022-04-19  2:59   ` Kai Huang
2022-04-19  4:04     ` Sathyanarayanan Kuppuswamy
2022-04-19  4:40       ` Kai Huang
2022-04-19  5:28         ` Sathyanarayanan Kuppuswamy
2022-04-19  7:21           ` Kai Huang
2022-04-20  3:39   ` Aubrey Li
2022-04-20  7:16     ` Sathyanarayanan Kuppuswamy
2022-04-20  8:08       ` Aubrey Li
2022-04-22 17:24       ` Isaku Yamahata
2022-04-25  3:06         ` Aubrey Li
2022-04-15 22:01 ` [PATCH v3 3/4] x86/tdx: Add TDX Guest event notify interrupt support Kuppuswamy Sathyanarayanan
2022-04-15 22:01 ` [PATCH v3 4/4] platform/x86: intel_tdx_attest: Add TDX Guest attestation interface driver Kuppuswamy Sathyanarayanan
2022-04-19  7:47   ` Kai Huang
2022-04-19  8:13     ` Borislav Petkov
2022-04-19 12:48       ` Sathyanarayanan Kuppuswamy
2022-04-20 22:00         ` Borislav Petkov
2022-04-20 22:09           ` Sathyanarayanan Kuppuswamy
2022-04-21  9:10             ` Borislav Petkov
2022-04-21 14:54               ` Sathyanarayanan Kuppuswamy
2022-04-19  8:16     ` Kai Huang
2022-04-19 14:00       ` Sathyanarayanan Kuppuswamy
2022-04-19 22:38         ` Kai Huang
2022-04-19 14:13     ` Dave Hansen
2022-04-19 14:19       ` Sathyanarayanan Kuppuswamy
2022-04-19 14:24         ` Dave Hansen
2022-04-19 14:26           ` Sathyanarayanan Kuppuswamy
2022-04-19 22:21       ` Kai Huang
2022-04-19 22:49         ` Dave Hansen
2022-04-19 23:02           ` Kai Huang
2022-04-20  1:20   ` Isaku Yamahata
2022-04-20  1:26     ` Sathyanarayanan Kuppuswamy
2022-04-21  7:04       ` Isaku Yamahata
2022-04-21 14:44         ` Sathyanarayanan Kuppuswamy
2022-04-20 23:18   ` Kai Huang [this message]
2022-04-20 23:45     ` Sathyanarayanan Kuppuswamy
2022-04-21  0:11       ` Kai Huang
2022-04-21  2:42         ` Sathyanarayanan Kuppuswamy
2022-04-21  6:57           ` Isaku Yamahata
2022-04-21 10:33             ` Kai Huang
2022-04-21 14:53             ` Sathyanarayanan Kuppuswamy
2022-04-21 16:53               ` Isaku Yamahata

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=b209ee09b74394ab7aed85e0244e2191ee3d4171.camel@intel.com \
    --to=kai.huang@intel.com \
    --cc=ak@linux.intel.com \
    --cc=bp@alien8.de \
    --cc=dave.hansen@linux.intel.com \
    --cc=hdegoede@redhat.com \
    --cc=hpa@zytor.com \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mgross@linux.intel.com \
    --cc=mingo@redhat.com \
    --cc=platform-driver-x86@vger.kernel.org \
    --cc=sathyanarayanan.kuppuswamy@linux.intel.com \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.