* [PATCH v4 0/6] configfs-tsm: Attestation Report ABI
@ 2023-09-26 4:16 Dan Williams
2023-09-26 4:17 ` [PATCH v4 1/6] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
` (5 more replies)
0 siblings, 6 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:16 UTC (permalink / raw)
To: linux-coco
Cc: Brijesh Singh, Kuppuswamy Sathyanarayanan, Erdem Aktas,
Peter Zijlstra, Tom Lendacky, Peter Gonda, Borislav Petkov,
Dionna Amalie Glaze, Jeremi Piotrowski, Thomas Gleixner,
Samuel Ortiz, Dionna Glaze, Pankaj Gupta, Greg Kroah-Hartman,
Andrew Morton, James Bottomley, linux-kernel, x86, dave.hansen
Changes since v3: [1]:
- Combine configfs-tsm + sev-guest conversion with the tdx-guest
extension
- Split PEM formatted certificate data to its own output attribute
(Jeremi)
- Parse the sev-guest output payload and emit the raw report without the
header (Jeremi)
- Drop @format as an input parameter and always request "extended"
reports in the sev-guest case with certificate data optionally
included (inspired by creation of separate @certs attribute)
- Drop usage of cleanup helpers in tdx_report_new() until
mutex_lock_interruptible() grows a guard() helper in v6.7. (Daniel and
Dave)
- Changelog grammar fixes for tdx-guest change (Kirill)
- Defer tdx-guest emitting its cert-chain through @certs pending
question on output payload versioning (i.e. kernel should only support
one). In the meantime zero-sized @certs is a valid output condition.
[1]: http://lore.kernel.org/r/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com
An attestation report is signed evidence of how a Trusted Virtual
Machine (TVM) was launched and its current state. A verifying party uses
the report to make judgements of the confidentiality and integrity of
that execution environment. Upon successful attestation the verifying
party may, for example, proceed to deploy secrets to the TVM to carry
out a workload. Multiple confidential computing platforms share this
similar flow.
The approach of adding adding new char devs and new ioctls, for what
amounts to the same logical functionality with minor formatting
differences across vendors [2], is untenable. Common concepts and the
community benefit from common infrastructure.
Use configfs for this facility for maintainability compared to ioctl(),
and for its scalability compared to sysfs. Atomicity can be enforced at
item creation time, and a conflict detection mechanism is included for
scenarios where multiple threads may share a single configuration
instance.
[2]: http://lore.kernel.org/r/cover.1684048511.git.sathyanarayanan.kuppuswamy@linux.intel.com
---
Dan Williams (5):
virt: coco: Add a coco/Makefile and coco/Kconfig
configfs-tsm: Introduce a shared ABI for attestation reports
virt: sevguest: Prep for kernel internal {get,get_ext}_report()
mm/slab: Add __free() support for kvfree
virt: sevguest: Add TSM_REPORTS support for SNP_{GET,GET_EXT}_REPORT
Kuppuswamy Sathyanarayanan (1):
virt: tdx-guest: Add Quote generation support using TSM_REPORTS
Documentation/ABI/testing/configfs-tsm | 67 +++++
MAINTAINERS | 8 +
arch/x86/coco/tdx/tdx.c | 21 ++
arch/x86/include/asm/shared/tdx.h | 1
arch/x86/include/asm/tdx.h | 2
drivers/virt/Kconfig | 6
drivers/virt/Makefile | 4
drivers/virt/coco/Kconfig | 14 +
drivers/virt/coco/Makefile | 8 +
drivers/virt/coco/sev-guest/Kconfig | 1
drivers/virt/coco/sev-guest/sev-guest.c | 180 ++++++++++++--
drivers/virt/coco/tdx-guest/Kconfig | 1
drivers/virt/coco/tdx-guest/tdx-guest.c | 229 +++++++++++++++++
drivers/virt/coco/tsm.c | 411 +++++++++++++++++++++++++++++++
include/linux/slab.h | 2
include/linux/tsm.h | 63 +++++
16 files changed, 992 insertions(+), 26 deletions(-)
create mode 100644 Documentation/ABI/testing/configfs-tsm
create mode 100644 drivers/virt/coco/Kconfig
create mode 100644 drivers/virt/coco/Makefile
create mode 100644 drivers/virt/coco/tsm.c
create mode 100644 include/linux/tsm.h
base-commit: 6465e260f48790807eef06b583b38ca9789b6072
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH v4 1/6] virt: coco: Add a coco/Makefile and coco/Kconfig
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-09-26 4:17 ` [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
` (4 subsequent siblings)
5 siblings, 0 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, peterz, linux-kernel, x86, dave.hansen
In preparation for adding another coco build target, relieve
drivers/virt/Makefile of the responsibility to track new compilation
unit additions to drivers/virt/coco/, and do the same for
drivers/virt/Kconfig.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/virt/Kconfig | 6 +-----
drivers/virt/Makefile | 4 +---
drivers/virt/coco/Kconfig | 9 +++++++++
drivers/virt/coco/Makefile | 7 +++++++
4 files changed, 18 insertions(+), 8 deletions(-)
create mode 100644 drivers/virt/coco/Kconfig
create mode 100644 drivers/virt/coco/Makefile
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index f79ab13a5c28..40129b6f0eca 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -48,10 +48,6 @@ source "drivers/virt/nitro_enclaves/Kconfig"
source "drivers/virt/acrn/Kconfig"
-source "drivers/virt/coco/efi_secret/Kconfig"
-
-source "drivers/virt/coco/sev-guest/Kconfig"
-
-source "drivers/virt/coco/tdx-guest/Kconfig"
+source "drivers/virt/coco/Kconfig"
endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index e9aa6fc96fab..f29901bd7820 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -9,6 +9,4 @@ obj-y += vboxguest/
obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/
obj-$(CONFIG_ACRN_HSM) += acrn/
-obj-$(CONFIG_EFI_SECRET) += coco/efi_secret/
-obj-$(CONFIG_SEV_GUEST) += coco/sev-guest/
-obj-$(CONFIG_INTEL_TDX_GUEST) += coco/tdx-guest/
+obj-y += coco/
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
new file mode 100644
index 000000000000..fc5c64f04c4a
--- /dev/null
+++ b/drivers/virt/coco/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+source "drivers/virt/coco/efi_secret/Kconfig"
+
+source "drivers/virt/coco/sev-guest/Kconfig"
+
+source "drivers/virt/coco/tdx-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
new file mode 100644
index 000000000000..55302ef719ad
--- /dev/null
+++ b/drivers/virt/coco/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+obj-$(CONFIG_EFI_SECRET) += efi_secret/
+obj-$(CONFIG_SEV_GUEST) += sev-guest/
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
2023-09-26 4:17 ` [PATCH v4 1/6] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
2023-09-27 2:10 ` Kuppuswamy Sathyanarayanan
2023-09-26 4:17 ` [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
` (3 subsequent siblings)
5 siblings, 2 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley,
Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz,
Greg Kroah-Hartman, Thomas Gleixner, peterz, linux-kernel, x86,
dave.hansen
One of the common operations of a TSM (Trusted Security Module) is to
provide a way for a TVM (confidential computing guest execution
environment) to take a measurement of its launch state, sign it and
submit it to a verifying party. Upon successful attestation that
verifies the integrity of the TVM additional secrets may be deployed.
The concept is common across TSMs, but the implementations are
unfortunately vendor specific. While the industry grapples with a common
definition of this attestation format [1], Linux need not make this
problem worse by defining a new ABI per TSM that wants to perform a
similar operation. The current momentum has been to invent new ioctl-ABI
per TSM per function which at best is an abdication of the kernel's
responsibility to make common infrastructure concepts share common ABI.
The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
not more, is to define a configfs interface to retrieve the TSM-specific
blob.
report=/sys/kernel/config/tsm/report/report0
mkdir $report
dd if=binary_userdata_plus_nonce > $report/inblob
hexdump $report/outblob
This approach later allows for the standardization of the attestation
blob format without needing to invent a new ABI. Once standardization
happens the standard format can be emitted by $report/outblob and
indicated by $report/provider, or a new attribute like
"$report/tcg_coco_report" can emit the standard format alongside the
vendor format.
Review of previous iterations of this interface identified that there is
a need to scale report generation for multiple container environments
[2]. Configfs enables a model where each container can bind mount one or
more report generation item instances. Still, within a container only a
single thread can be manipulating a given configuration instance at a
time. A 'generation' count is provided to detect conflicts between
multiple threads racing to configure a report instance.
The SEV-SNP concepts of "extended reports" and "privilege levels" are
optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
time. The expectation is that those concepts are generic enough that
they may be adopted by other TSM implementations. In other words,
configfs-tsm aims to address a superset of TSM specific functionality
with a common ABI where attributes may appear, or not appear, based on the set
of concepts the implementation supports.
Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Peter Gonda <pgonda@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Samuel Ortiz <sameo@rivosinc.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Documentation/ABI/testing/configfs-tsm | 67 +++++
MAINTAINERS | 8 +
drivers/virt/coco/Kconfig | 5
drivers/virt/coco/Makefile | 1
drivers/virt/coco/tsm.c | 411 ++++++++++++++++++++++++++++++++
include/linux/tsm.h | 63 +++++
6 files changed, 555 insertions(+)
create mode 100644 Documentation/ABI/testing/configfs-tsm
create mode 100644 drivers/virt/coco/tsm.c
create mode 100644 include/linux/tsm.h
diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
new file mode 100644
index 000000000000..ba81083046d3
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-tsm
@@ -0,0 +1,67 @@
+What: /sys/kernel/config/tsm/report/$name/inblob
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (WO) Up to 64 bytes of user specified binary data. For replay
+ protection this should include a nonce, but the kernel does not
+ place any restrictions on the content.
+
+What: /sys/kernel/config/tsm/report/$name/outblob
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Binary attestation report generated from @inblob and other
+ options The format of the report is implementation specific
+ where the implementation is conveyed via the @provider
+ attribute.
+
+What: /sys/kernel/config/tsm/report/$name/certs
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Zero or more certificates in concatenated PEM format. Refer
+ to implementation specific documentation on which certificates
+ might be returned.
+
+What: /sys/kernel/config/tsm/report/$name/provider
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) A name for the format-specification of @outblob like
+ "sev-snp" or "tdx" in the near term, or a common standard format
+ in the future.
+
+What: /sys/kernel/config/tsm/report/$name/generation
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) The value in this attribute increments each time @inblob or
+ any option is written. Userspace can detect conflicts by
+ checking generation before writing to any attribute and making
+ sure the number of writes matches expectations after reading
+ @outblob, or it can prevent conflicts by creating a report
+ instance per requesting context.
+
+What: /sys/kernel/config/tsm/report/$name/privlevel
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (WO) If a TSM implementation supports the concept of attestation
+ reports for TVMs running at different privilege levels, like
+ SEV-SNP "VMPL", specify the privilege level via this attribute.
+ The minimum acceptable value is conveyed via @privlevel_floor
+ and the maximum acceptable value is TSM_PRIVLEVEL_MAX (3).
+
+What: /sys/kernel/config/tsm/report/$name/privlevel_floor
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Indicates the minimum permissible value that can be written
+ to @privlevel.
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..8acbeb029ba1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21889,6 +21889,14 @@ W: https://github.com/srcres258/linux-doc
T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
F: Documentation/translations/zh_TW/
+TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
+M: Dan Williams <dan.j.williams@intel.com>
+L: linux-coco@lists.linux.dev
+S: Maintained
+F: Documentation/ABI/testing/configfs-tsm
+F: drivers/virt/coco/tsm.c
+F: include/linux/tsm.h
+
TTY LAYER AND SERIAL DRIVERS
M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
M: Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index fc5c64f04c4a..87d142c1f932 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -2,6 +2,11 @@
#
# Confidential computing related collateral
#
+
+config TSM_REPORTS
+ select CONFIGFS_FS
+ tristate
+
source "drivers/virt/coco/efi_secret/Kconfig"
source "drivers/virt/coco/sev-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index 55302ef719ad..18c1aba5edb7 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -2,6 +2,7 @@
#
# Confidential computing related collateral
#
+obj-$(CONFIG_TSM_REPORTS) += tsm.o
obj-$(CONFIG_EFI_SECRET) += efi_secret/
obj-$(CONFIG_SEV_GUEST) += sev-guest/
obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
new file mode 100644
index 000000000000..343fc77d0509
--- /dev/null
+++ b/drivers/virt/coco/tsm.c
@@ -0,0 +1,411 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/tsm.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/rwsem.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/cleanup.h>
+#include <linux/configfs.h>
+
+static struct tsm_provider {
+ const struct tsm_ops *ops;
+ const struct config_item_type *type;
+ void *data;
+} provider;
+static DECLARE_RWSEM(tsm_rwsem);
+
+/**
+ * DOC: Trusted Security Module (TSM) Attestation Report Interface
+ *
+ * The TSM report interface is a common provider of blobs that facilitate
+ * attestation of a TVM (confidential computing guest) by an attestation
+ * service. A TSM report combines a user-defined blob (likely a public-key with
+ * a nonce for a key-exchange protocol) with a signed attestation report. That
+ * combined blob is then used to obtain secrets provided by an agent that can
+ * validate the attestation report. The expectation is that this interface is
+ * invoked infrequently, however configfs allows for multiple agents to
+ * own their own report generation instances to generate reports as
+ * often as needed.
+ *
+ * The attestation report format is TSM provider specific, when / if a standard
+ * materializes that can be published instead of the vendor layout. Until then
+ * the 'provider' attribute indicates the format of 'outblob'. However,
+ * the common "return a list of certs" capability across multiple TSM
+ * implementations is returned in a unified @certs attribute.
+ */
+
+struct tsm_report_state {
+ struct tsm_report report;
+ unsigned long write_generation;
+ unsigned long read_generation;
+ struct config_item cfg;
+};
+
+enum tsm_data_select {
+ TSM_REPORT,
+ TSM_CERTS,
+};
+
+static struct tsm_report *to_tsm_report(struct config_item *cfg)
+{
+ struct tsm_report_state *state =
+ container_of(cfg, struct tsm_report_state, cfg);
+
+ return &state->report;
+}
+
+static struct tsm_report_state *to_state(struct tsm_report *report)
+{
+ return container_of(report, struct tsm_report_state, report);
+}
+
+static int try_advance_write_generation(struct tsm_report *report)
+{
+ struct tsm_report_state *state = to_state(report);
+
+ lockdep_assert_held_write(&tsm_rwsem);
+
+ /*
+ * Malicious or broken userspace has written enough times for
+ * read_generation == write_generation by modular arithmetic without an
+ * interim read. Stop accepting updates until the current report
+ * configuration is read.
+ */
+ if (state->write_generation == state->read_generation - 1)
+ return -EBUSY;
+ state->write_generation++;
+ return 0;
+}
+
+static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
+ const char *buf, size_t len)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ unsigned int val;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &val);
+ if (rc)
+ return rc;
+
+ /*
+ * The valid privilege levels that a TSM might accept, if it accepts a
+ * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
+ * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
+ * than 0.
+ */
+ if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
+ return -EINVAL;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ rc = try_advance_write_generation(report);
+ if (rc)
+ return rc;
+ report->desc.privlevel = val;
+
+ return len;
+}
+CONFIGFS_ATTR_WO(tsm_report_, privlevel);
+
+static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
+ char *buf)
+{
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
+}
+CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
+
+static ssize_t tsm_report_inblob_write(struct config_item *cfg,
+ const void *buf, size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ int rc;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ rc = try_advance_write_generation(report);
+ if (rc)
+ return rc;
+
+ report->desc.inblob_len = count;
+ memcpy(report->desc.inblob, buf, count);
+ return count;
+}
+CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
+
+static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ struct tsm_report_state *state = to_state(report);
+
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%lu\n", state->write_generation);
+}
+CONFIGFS_ATTR_RO(tsm_report_, generation);
+
+static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
+{
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%s\n", provider.ops->name);
+}
+CONFIGFS_ATTR_RO(tsm_report_, provider);
+
+static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
+ enum tsm_data_select select)
+{
+ loff_t offset = 0;
+ u8 *out, len;
+
+ if (select == TSM_REPORT) {
+ out = report->outblob;
+ len = report->outblob_len;
+ } else {
+ out = report->certs;
+ len = report->certs_len;
+ }
+
+ if (!buf)
+ return len;
+ return memory_read_from_buffer(buf, count, &offset, out, len);
+}
+
+static ssize_t read_cached_report(struct tsm_report *report, void *buf,
+ size_t count, enum tsm_data_select select)
+{
+ struct tsm_report_state *state = to_state(report);
+
+ guard(rwsem_read)(&tsm_rwsem);
+ if (!report->desc.inblob_len)
+ return -EINVAL;
+
+ /*
+ * A given TSM backend always fills in ->outblob regardless of
+ * whether the report includes certs or not.
+ */
+ if (!report->outblob ||
+ state->read_generation != state->write_generation)
+ return -EWOULDBLOCK;
+
+ return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
+ size_t count, enum tsm_data_select select)
+{
+ struct tsm_report_state *state = to_state(report);
+ const struct tsm_ops *ops;
+ ssize_t rc;
+
+ /* try to read from the existing report if present and valid... */
+ rc = read_cached_report(report, buf, count, select);
+ if (rc >= 0 || rc != -EWOULDBLOCK)
+ return rc;
+
+ /* slow path, report may need to be regenerated... */
+ guard(rwsem_write)(&tsm_rwsem);
+ ops = provider.ops;
+ if (!report->desc.inblob_len)
+ return -EINVAL;
+
+ /* did another thread already generate this report? */
+ if (report->outblob &&
+ state->read_generation == state->write_generation)
+ goto out;
+
+ kvfree(report->outblob);
+ kvfree(report->certs);
+ report->outblob = NULL;
+ report->certs = NULL;
+ rc = ops->report_new(report, provider.data);
+ if (rc < 0)
+ return rc;
+ state->read_generation = state->write_generation;
+out:
+ return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
+ size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+
+ return tsm_report_read(report, buf, count, TSM_REPORT);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
+
+static ssize_t tsm_report_certs_read(struct config_item *cfg, void *buf,
+ size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+
+ return tsm_report_read(report, buf, count, TSM_CERTS);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, certs, NULL, TSM_OUTBLOB_MAX);
+
+#define TSM_DEFAULT_ATTRS() \
+ &tsm_report_attr_generation, \
+ &tsm_report_attr_provider
+
+static struct configfs_attribute *tsm_report_attrs[] = {
+ TSM_DEFAULT_ATTRS(),
+ NULL,
+};
+
+static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
+ &tsm_report_attr_inblob,
+ &tsm_report_attr_outblob,
+ &tsm_report_attr_certs,
+ NULL,
+};
+
+static struct configfs_attribute *tsm_report_extra_attrs[] = {
+ TSM_DEFAULT_ATTRS(),
+ &tsm_report_attr_privlevel,
+ &tsm_report_attr_privlevel_floor,
+ NULL,
+};
+
+static void tsm_report_item_release(struct config_item *cfg)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ struct tsm_report_state *state = to_state(report);
+
+ kvfree(report->certs);
+ kvfree(report->outblob);
+ kfree(state);
+}
+
+static struct configfs_item_operations tsm_report_item_ops = {
+ .release = tsm_report_item_release,
+};
+
+const struct config_item_type tsm_report_default_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_bin_attrs = tsm_report_bin_attrs,
+ .ct_attrs = tsm_report_attrs,
+ .ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_default_type);
+
+const struct config_item_type tsm_report_ext_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_bin_attrs = tsm_report_bin_attrs,
+ .ct_attrs = tsm_report_extra_attrs,
+ .ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_ext_type);
+
+static struct config_item *tsm_report_make_item(struct config_group *group,
+ const char *name)
+{
+ struct tsm_report_state *state;
+
+ guard(rwsem_read)(&tsm_rwsem);
+ if (!provider.ops)
+ return ERR_PTR(-ENXIO);
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return ERR_PTR(-ENOMEM);
+
+ config_item_init_type_name(&state->cfg, name, provider.type);
+ return &state->cfg;
+}
+
+static struct configfs_group_operations tsm_report_group_ops = {
+ .make_item = tsm_report_make_item,
+};
+
+static const struct config_item_type tsm_reports_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_group_ops = &tsm_report_group_ops,
+};
+
+static const struct config_item_type tsm_root_group_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem tsm_configfs = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = "tsm",
+ .ci_type = &tsm_root_group_type,
+ },
+ },
+ .su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
+};
+
+static struct config_group *tsm_report_group;
+
+int tsm_register(const struct tsm_ops *ops, void *priv,
+ const struct config_item_type *type)
+{
+ const struct tsm_ops *conflict;
+
+ if (!type)
+ type = &tsm_report_default_type;
+ if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
+ return -EINVAL;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ conflict = provider.ops;
+ if (conflict) {
+ pr_err("\"%s\" ops already registered\n", conflict->name);
+ return -EBUSY;
+ }
+
+ provider.ops = ops;
+ provider.data = priv;
+ provider.type = type;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_register);
+
+int tsm_unregister(const struct tsm_ops *ops)
+{
+ guard(rwsem_write)(&tsm_rwsem);
+ if (ops != provider.ops)
+ return -EBUSY;
+ provider.ops = NULL;
+ provider.data = NULL;
+ provider.type = NULL;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_unregister);
+
+static int __init tsm_init(void)
+{
+ struct config_group *root = &tsm_configfs.su_group;
+ struct config_group *tsm;
+ int rc;
+
+ config_group_init(root);
+ rc = configfs_register_subsystem(&tsm_configfs);
+ if (rc)
+ return rc;
+
+ tsm = configfs_register_default_group(root, "report",
+ &tsm_reports_type);
+ if (IS_ERR(tsm)) {
+ configfs_unregister_subsystem(&tsm_configfs);
+ return PTR_ERR(tsm);
+ }
+ tsm_report_group = tsm;
+
+ return 0;
+}
+module_init(tsm_init);
+
+static void __exit tsm_exit(void)
+{
+ configfs_unregister_default_group(tsm_report_group);
+ configfs_unregister_subsystem(&tsm_configfs);
+}
+module_exit(tsm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
new file mode 100644
index 000000000000..1fe1dba3a912
--- /dev/null
+++ b/include/linux/tsm.h
@@ -0,0 +1,63 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __TSM_H
+#define __TSM_H
+
+#include <linux/sizes.h>
+#include <linux/types.h>
+#include <linux/device.h>
+
+#define TSM_INBLOB_MAX 64
+#define TSM_OUTBLOB_MAX SZ_32K
+
+/*
+ * Privilege level is a nested permission concept to allow confidential
+ * guests to partition address space, 4-levels are supported.
+ */
+#define TSM_PRIVLEVEL_MAX 3
+
+/**
+ * struct tsm_desc - option descriptor for generating tsm report blobs
+ * @privlevel: optional privilege level to associate with @outblob
+ * @inblob_len: sizeof @inblob
+ * @inblob: arbitrary input data
+ */
+struct tsm_desc {
+ unsigned int privlevel;
+ size_t inblob_len;
+ u8 inblob[TSM_INBLOB_MAX];
+};
+
+/**
+ * struct tsm_report - track state of report generation relative to options
+ * @desc: report generation options / cached report state
+ * @outblob: generated evidence to provider to the attestation agent
+ * @outblob_len: sizeof(outblob)
+ * @write_generation: conflict detection, and report regeneration tracking
+ * @read_generation: cached report invalidation tracking
+ * @cfg: configfs interface
+ */
+struct tsm_report {
+ struct tsm_desc desc;
+ size_t outblob_len;
+ u8 *outblob;
+ size_t certs_len;
+ u8 *certs;
+};
+
+/*
+ * arch specific ops, only one is expected to be registered at a time
+ * i.e. only one of SEV, TDX, COVE, etc.
+ */
+struct tsm_ops {
+ const char *name;
+ const int privlevel_floor;
+ int (*report_new)(struct tsm_report *desc, void *data);
+};
+
+extern const struct config_item_type tsm_report_ext_type;
+extern const struct config_item_type tsm_report_default_type;
+
+int tsm_register(const struct tsm_ops *ops, void *priv,
+ const struct config_item_type *type);
+int tsm_unregister(const struct tsm_ops *ops);
+#endif /* __TSM_H */
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report()
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
2023-09-26 4:17 ` [PATCH v4 1/6] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-09-26 4:17 ` [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-09-26 18:51 ` Kuppuswamy Sathyanarayanan
2023-09-26 4:17 ` [PATCH v4 4/6] mm/slab: Add __free() support for kvfree Dan Williams
` (2 subsequent siblings)
5 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh,
peterz, linux-kernel, x86, dave.hansen
In preparation for using the configs-tsm facility to convey attestation
blobs to userspace, switch to using the 'sockptr' api for copying
payloads to provided buffers where 'sockptr' handles user vs kernel
buffers.
While configfs-tsm is meant to replace existing confidential computing
ioctl() implementations for attestation report retrieval the old ioctl()
path needs to stick around for a deprecation period.
No behavior change intended.
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/virt/coco/sev-guest/sev-guest.c | 50 ++++++++++++++++++++-----------
1 file changed, 33 insertions(+), 17 deletions(-)
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 97dbe715e96a..c3c9e9ea691f 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -19,6 +19,7 @@
#include <crypto/aead.h>
#include <linux/scatterlist.h>
#include <linux/psp-sev.h>
+#include <linux/sockptr.h>
#include <uapi/linux/sev-guest.h>
#include <uapi/linux/psp-sev.h>
@@ -470,7 +471,13 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
return 0;
}
-static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+struct snp_req_resp {
+ sockptr_t req_data;
+ sockptr_t resp_data;
+};
+
+static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg,
+ struct snp_req_resp *io)
{
struct snp_guest_crypto *crypto = snp_dev->crypto;
struct snp_report_resp *resp;
@@ -479,10 +486,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
lockdep_assert_held(&snp_cmd_mutex);
- if (!arg->req_data || !arg->resp_data)
+ if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
return -EINVAL;
- if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
return -EFAULT;
/*
@@ -501,7 +508,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
if (rc)
goto e_free;
- if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+ if (copy_to_sockptr(io->resp_data, resp, sizeof(*resp)))
rc = -EFAULT;
e_free:
@@ -550,22 +557,25 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
return rc;
}
-static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
+static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg,
+ struct snp_req_resp *io)
+
{
struct snp_guest_crypto *crypto = snp_dev->crypto;
struct snp_ext_report_req req;
struct snp_report_resp *resp;
int ret, npages = 0, resp_len;
+ sockptr_t certs_address;
lockdep_assert_held(&snp_cmd_mutex);
- if (!arg->req_data || !arg->resp_data)
+ if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
return -EINVAL;
- if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+ if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
return -EFAULT;
- /* userspace does not want certificate data */
+ /* caller does not want certificate data */
if (!req.certs_len || !req.certs_address)
goto cmd;
@@ -573,8 +583,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
!IS_ALIGNED(req.certs_len, PAGE_SIZE))
return -EINVAL;
- if (!access_ok((const void __user *)req.certs_address, req.certs_len))
- return -EFAULT;
+ if (sockptr_is_kernel(io->resp_data)) {
+ certs_address = KERNEL_SOCKPTR((void *)req.certs_address);
+ } else {
+ certs_address = USER_SOCKPTR((void __user *)req.certs_address);
+ if (!access_ok(certs_address.user, req.certs_len))
+ return -EFAULT;
+ }
/*
* Initialize the intermediate buffer with all zeros. This buffer
@@ -604,21 +619,19 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
- if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
+ if (copy_to_sockptr(io->req_data, &req, sizeof(req)))
ret = -EFAULT;
}
if (ret)
goto e_free;
- if (npages &&
- copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
- req.certs_len)) {
+ if (npages && copy_to_sockptr(certs_address, snp_dev->certs_data, req.certs_len)) {
ret = -EFAULT;
goto e_free;
}
- if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
+ if (copy_to_sockptr(io->resp_data, resp, sizeof(*resp)))
ret = -EFAULT;
e_free:
@@ -631,6 +644,7 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
struct snp_guest_dev *snp_dev = to_snp_dev(file);
void __user *argp = (void __user *)arg;
struct snp_guest_request_ioctl input;
+ struct snp_req_resp io;
int ret = -ENOTTY;
if (copy_from_user(&input, argp, sizeof(input)))
@@ -651,15 +665,17 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
return -ENOTTY;
}
+ io.req_data = USER_SOCKPTR((void __user *)input.req_data);
+ io.resp_data = USER_SOCKPTR((void __user *)input.resp_data);
switch (ioctl) {
case SNP_GET_REPORT:
- ret = get_report(snp_dev, &input);
+ ret = get_report(snp_dev, &input, &io);
break;
case SNP_GET_DERIVED_KEY:
ret = get_derived_key(snp_dev, &input);
break;
case SNP_GET_EXT_REPORT:
- ret = get_ext_report(snp_dev, &input);
+ ret = get_ext_report(snp_dev, &input, &io);
break;
default:
break;
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 4/6] mm/slab: Add __free() support for kvfree
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
` (2 preceding siblings ...)
2023-09-26 4:17 ` [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-09-26 4:17 ` [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
2023-09-26 4:17 ` [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
5 siblings, 0 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Andrew Morton, Peter Zijlstra, Greg Kroah-Hartman, Pankaj Gupta,
Greg Kroah-Hartman, linux-kernel, x86, dave.hansen
Allow for the declaration of variables that trigger kvfree() when they
go out of scope. The check for NULL and call to kvfree() can be elided
by the compiler in most cases, otherwise without the NULL check an
unnecessary call to kvfree() may be emitted. Peter proposed a comment
for this detail [1].
Link: http://lore.kernel.org/r/20230816103102.GF980931@hirez.programming.kicks-ass.net [1]
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
include/linux/slab.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8228d1276a2f..df4c2d45bb86 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -763,6 +763,8 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla
extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
__realloc_size(3);
extern void kvfree(const void *addr);
+DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
+
extern void kvfree_sensitive(const void *addr, size_t len);
unsigned int kmem_cache_size(struct kmem_cache *s);
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
` (3 preceding siblings ...)
2023-09-26 4:17 ` [PATCH v4 4/6] mm/slab: Add __free() support for kvfree Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-10-04 8:22 ` Dan Carpenter
2023-09-26 4:17 ` [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
5 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh,
Jeremi Piotrowski, peterz, linux-kernel, x86, dave.hansen
The sevguest driver was a first mover in the confidential computing
space. As a first mover that afforded some leeway to build the driver
without concern for common infrastructure.
Now that sevguest is no longer a singleton [1] the common operation of
building and transmitting attestation report blobs can / should be made
common. In this model the so called "TSM-provider" implementations can
share a common envelope ABI even if the contents of that envelope remain
vendor-specific. When / if the industry agrees on an attestation record
format, that definition can also fit in the same ABI. In the meantime
the kernel's maintenance burden is reduced and collaboration on the
commons is increased.
Convert sevguest to use CONFIG_TSM_REPORTS to retrieve the data that
the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
retrieving the report blob via the TSM interface utility,
assuming no nonce and VMPL==2:
report=/sys/kernel/config/tsm/report/report0
mkdir $report
echo 2 > $report/privlevel
dd if=/dev/urandom bs=64 count=1 > $report/inblob
hexdump -C $report/outblob
cat $report/certs
rmdir $report
Given that the platform implementation is free to return empty certificate data
if none is available it lets configfs-tsm be simplified if it only needs
to worry about one output format.
The old ioctls can be lazily deprecated, the main motivation of this
effort is to stop the proliferation of new ioctls, and to increase
cross-vendor collaboration.
Note, only compile-tested.
Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/virt/coco/sev-guest/Kconfig | 1
drivers/virt/coco/sev-guest/sev-guest.c | 130 +++++++++++++++++++++++++++++++
2 files changed, 131 insertions(+)
diff --git a/drivers/virt/coco/sev-guest/Kconfig b/drivers/virt/coco/sev-guest/Kconfig
index da2d7ca531f0..1cffc72c41cb 100644
--- a/drivers/virt/coco/sev-guest/Kconfig
+++ b/drivers/virt/coco/sev-guest/Kconfig
@@ -5,6 +5,7 @@ config SEV_GUEST
select CRYPTO
select CRYPTO_AEAD2
select CRYPTO_GCM
+ select TSM_REPORTS
help
SEV-SNP firmware provides the guest a mechanism to communicate with
the PSP without risk from a malicious hypervisor who wishes to read,
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index c3c9e9ea691f..646feb433b1c 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -16,10 +16,12 @@
#include <linux/miscdevice.h>
#include <linux/set_memory.h>
#include <linux/fs.h>
+#include <linux/tsm.h>
#include <crypto/aead.h>
#include <linux/scatterlist.h>
#include <linux/psp-sev.h>
#include <linux/sockptr.h>
+#include <linux/cleanup.h>
#include <uapi/linux/sev-guest.h>
#include <uapi/linux/psp-sev.h>
@@ -759,6 +761,126 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
return key;
}
+struct snp_msg_report_resp_hdr {
+ u32 status;
+ u32 report_size;
+ u8 rsvd[24];
+};
+#define SNP_REPORT_INVALID_PARAM 0x16
+#define SNP_REPORT_INVALID_KEY_SEL 0x27
+
+struct snp_msg_cert_entry {
+ unsigned char guid[16];
+ u32 offset;
+ u32 length;
+};
+
+static int sev_report_new(struct tsm_report *report, void *data)
+{
+ static const struct snp_msg_cert_entry zero_ent = { 0 };
+ struct tsm_desc *desc = &report->desc;
+ struct snp_guest_dev *snp_dev = data;
+ struct snp_msg_report_resp_hdr hdr;
+ const int report_size = SZ_4K;
+ const int ext_size = SZ_16K;
+ int ret, size = report_size + ext_size;
+ int certs_size, cert_count, i, offset;
+ u8 *certs_address;
+
+ if (desc->inblob_len != 64)
+ return -EINVAL;
+
+ void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ guard(mutex)(&snp_cmd_mutex);
+ certs_address = buf + report_size;
+ struct snp_ext_report_req ext_req = {
+ .data = { .vmpl = desc->privlevel },
+ .certs_address = (__u64)certs_address,
+ .certs_len = ext_size,
+ };
+ memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
+
+ struct snp_guest_request_ioctl input = {
+ .msg_version = 1,
+ .req_data = (__u64)&ext_req,
+ .resp_data = (__u64)buf,
+ };
+ struct snp_req_resp io = {
+ .req_data = KERNEL_SOCKPTR(&ext_req),
+ .resp_data = KERNEL_SOCKPTR(buf),
+ };
+
+ ret = get_ext_report(snp_dev, &input, &io);
+
+ if (ret)
+ return ret;
+
+ memcpy(&hdr, buf, sizeof(hdr));
+ if (hdr.status == SNP_REPORT_INVALID_PARAM)
+ return -EINVAL;
+ if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
+ return -EINVAL;
+ if (hdr.status)
+ return -ENXIO;
+ if ((hdr.report_size + sizeof(hdr)) > report_size)
+ return -ENOMEM;
+
+ void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
+ if (!rbuf)
+ return -ENOMEM;
+
+ memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
+ report->outblob = no_free_ptr(rbuf);
+ report->outblob_len = hdr.report_size;
+
+ for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
+ struct snp_msg_cert_entry *certs = buf + report_size;
+
+ if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
+ break;
+ certs_size += certs[i].length;
+ }
+ cert_count = i;
+
+ /* No certs to report */
+ if (cert_count == 0)
+ return 0;
+
+ /* sanity check that the entire certs table with metadata fits */
+ if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
+ return -ENXIO;
+
+ void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
+ if (!cbuf)
+ return -ENOMEM;
+
+ /* Concatenate returned certs */
+ for (i = 0, offset = 0; i < cert_count; i++) {
+ struct snp_msg_cert_entry *certs = buf + report_size;
+
+ memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
+ offset += certs[i].length;
+ }
+
+ report->certs = no_free_ptr(cbuf);
+ report->certs_len = certs_size;
+
+ return 0;
+}
+
+static const struct tsm_ops sev_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .report_new = sev_report_new,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+ tsm_unregister(&sev_tsm_ops);
+}
+
static int __init sev_guest_probe(struct platform_device *pdev)
{
struct snp_secrets_page_layout *layout;
@@ -832,6 +954,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
snp_dev->input.resp_gpa = __pa(snp_dev->response);
snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
+ ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
+ if (ret)
+ goto e_free_cert_data;
+
+ ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
+ if (ret)
+ goto e_free_cert_data;
+
ret = misc_register(misc);
if (ret)
goto e_free_cert_data;
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
` (4 preceding siblings ...)
2023-09-26 4:17 ` [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
@ 2023-09-26 4:17 ` Dan Williams
2023-09-27 16:14 ` Peter Gonda
5 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-26 4:17 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz, linux-kernel,
x86, dave.hansen
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In TDX guest, the attestation process is used to verify the TDX guest
trustworthiness to other entities before provisioning secrets to the
guest. The first step in the attestation process is TDREPORT
generation, which involves getting the guest measurement data in the
format of TDREPORT, which is further used to validate the authenticity
of the TDX guest. TDREPORT by design is integrity-protected and can
only be verified on the local machine.
To support remote verification of the TDREPORT in a SGX-based
attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
(QE) to convert it to a remotely verifiable Quote. SGX QE by design can
only run outside of the TDX guest (i.e. in a host process or in a
normal VM) and guest can use communication channels like vsock or
TCP/IP to send the TDREPORT to the QE. But for security concerns, the
TDX guest may not support these communication channels. To handle such
cases, TDX defines a GetQuote hypercall which can be used by the guest
to request the host VMM to communicate with the SGX QE. More details
about GetQuote hypercall can be found in TDX Guest-Host Communication
Interface (GHCI) for Intel TDX 1.0, section titled
"TDG.VP.VMCALL<GetQuote>".
Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
Computing Guest platforms to get the measurement data via ConfigFS.
Extend the TSM framework and add support to allow an attestation agent
to get the TDX Quote data (included usage example below).
report=/sys/kernel/config/tsm/report/report0
mkdir $report
dd if=/dev/urandom bs=64 count=1 > $report/inblob
hexdump -C $report/outblob
rmdir $report
GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer, allocate a large enough memory and mark it shared using
set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
for GetQuote requests in the TDX TSM handler.
Although this method reserves a fixed chunk of memory for GetQuote
requests, such one time allocation can help avoid memory fragmentation
related allocation failures later in the uptime of the guest.
Since the Quote generation process is not time-critical or frequently
used, the current version uses a polling model for Quote requests and
it also does not support parallel GetQuote requests.
Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
arch/x86/coco/tdx/tdx.c | 21 +++
arch/x86/include/asm/shared/tdx.h | 1
arch/x86/include/asm/tdx.h | 2
drivers/virt/coco/tdx-guest/Kconfig | 1
drivers/virt/coco/tdx-guest/tdx-guest.c | 229 +++++++++++++++++++++++++++++++
5 files changed, 253 insertions(+), 1 deletion(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..752867b1d11b 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
}
EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
+/**
+ * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
+ * hypercall.
+ * @buf: Address of the directly mapped shared kernel buffer which
+ * contains TDREPORT. The same buffer will be used by VMM to
+ * store the generated TD Quote output.
+ * @size: size of the tdquote buffer (4KB-aligned).
+ *
+ * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
+ * v1.0 specification for more information on GetQuote hypercall.
+ * It is used in the TDX guest driver module to get the TD Quote.
+ *
+ * Return 0 on success or error code on failure.
+ */
+u64 tdx_hcall_get_quote(u8 *buf, size_t size)
+{
+ /* Since buf is a shared memory, set the shared (decrypted) bits */
+ return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
static void __noreturn tdx_panic(const char *msg)
{
struct tdx_hypercall_args args = {
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 7513b3bb69b7..9eab19950f39 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -22,6 +22,7 @@
/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
+#define TDVMCALL_GET_QUOTE 0x10002
#define TDVMCALL_REPORT_FATAL_ERROR 0x10003
#ifndef __ASSEMBLY__
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 603e6d1e9d4a..ebd1cda4875f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
+u64 tdx_hcall_get_quote(u8 *buf, size_t size);
+
#else
static inline void tdx_early_init(void) { };
diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
index 14246fc2fb02..22dd59e19431 100644
--- a/drivers/virt/coco/tdx-guest/Kconfig
+++ b/drivers/virt/coco/tdx-guest/Kconfig
@@ -1,6 +1,7 @@
config TDX_GUEST_DRIVER
tristate "TDX Guest driver"
depends on INTEL_TDX_GUEST
+ select TSM_REPORTS
help
The driver provides userspace interface to communicate with
the TDX module to request the TDX guest details like attestation
diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 5e44a0fa69bd..1253bf76b570 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -12,12 +12,60 @@
#include <linux/mod_devicetable.h>
#include <linux/string.h>
#include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/tsm.h>
+#include <linux/sizes.h>
#include <uapi/linux/tdx-guest.h>
#include <asm/cpu_device_id.h>
#include <asm/tdx.h>
+/*
+ * Intel's SGX QE implementation generally uses Quote size less
+ * than 8K (2K Quote data + ~5K of certificate blob).
+ */
+#define GET_QUOTE_BUF_SIZE SZ_8K
+
+#define GET_QUOTE_CMD_VER 1
+
+/* TDX GetQuote status codes */
+#define GET_QUOTE_SUCCESS 0
+#define GET_QUOTE_IN_FLIGHT 0xffffffffffffffff
+
+/* struct tdx_quote_buf: Format of Quote request buffer.
+ * @version: Quote format version, filled by TD.
+ * @status: Status code of Quote request, filled by VMM.
+ * @in_len: Length of TDREPORT, filled by TD.
+ * @out_len: Length of Quote data, filled by VMM.
+ * @data: Quote data on output or TDREPORT on input.
+ *
+ * More details of Quote request buffer can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_buf {
+ u64 version;
+ u64 status;
+ u32 in_len;
+ u32 out_len;
+ u8 data[];
+};
+
+/* Quote data buffer */
+static void *quote_data;
+
+/* Lock to streamline quote requests */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * GetQuote request timeout in seconds. Expect that 30 seconds
+ * is enough time for QE to respond to any Quote requests.
+ */
+static u32 getquote_timeout = 30;
+
static long tdx_get_report0(struct tdx_report_req __user *req)
{
u8 *reportdata, *tdreport;
@@ -53,6 +101,154 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
return ret;
}
+static void free_quote_buf(void *buf)
+{
+ size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+ unsigned int count = len >> PAGE_SHIFT;
+
+ if (set_memory_encrypted((unsigned long)buf, count)) {
+ pr_err("Failed to restore encryption mask for Quote buffer, leak it\n");
+ return;
+ }
+
+ free_pages_exact(buf, len);
+}
+
+static void *alloc_quote_buf(void)
+{
+ size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+ unsigned int count = len >> PAGE_SHIFT;
+ void *addr;
+
+ addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
+ if (!addr)
+ return NULL;
+
+ if (set_memory_decrypted((unsigned long)addr, count)) {
+ free_pages_exact(addr, len);
+ return NULL;
+ }
+
+ return addr;
+}
+
+/*
+ * wait_for_quote_completion() - Wait for Quote request completion
+ * @quote_buf: Address of Quote buffer.
+ * @timeout: Timeout in seconds to wait for the Quote generation.
+ *
+ * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
+ * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
+ * while VMM processes the GetQuote request, and will change it to success
+ * or error code after processing is complete. So wait till the status
+ * changes from GET_QUOTE_IN_FLIGHT or the request being timed out.
+ */
+static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
+{
+ int i = 0;
+
+ /*
+ * Quote requests usually take a few seconds to complete, so waking up
+ * once per second to recheck the status is fine for this use case.
+ */
+ while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
+ if (msleep_interruptible(MSEC_PER_SEC))
+ return -EINTR;
+ }
+
+ return (i == timeout) ? -ETIMEDOUT : 0;
+}
+
+static int tdx_report_new(struct tsm_report *report, void *data)
+{
+ u8 *buf, *reportdata = NULL, *tdreport = NULL;
+ struct tdx_quote_buf *quote_buf = quote_data;
+ struct tsm_desc *desc = &report->desc;
+ int ret;
+ u64 err;
+
+ /* TODO: switch to guard(mutex_intr) */
+ if (mutex_lock_interruptible("e_lock))
+ return -EINTR;
+
+ /*
+ * If the previous request is timedout or interrupted, and the
+ * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
+ * VMM), don't permit any new request.
+ */
+ if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
+ ret = -EBUSY;
+ goto done;
+ }
+
+ if (desc->inblob_len != TDX_REPORTDATA_LEN) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+ if (!reportdata) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
+ if (!tdreport) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ memcpy(reportdata, desc->inblob, desc->inblob_len);
+
+ /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
+ ret = tdx_mcall_get_report0(reportdata, tdreport);
+ if (ret) {
+ pr_err("GetReport call failed\n");
+ goto done;
+ }
+
+ memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
+
+ /* Update Quote buffer header */
+ quote_buf->version = GET_QUOTE_CMD_VER;
+ quote_buf->in_len = TDX_REPORT_LEN;
+
+ memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
+
+ err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
+ if (err) {
+ pr_err("GetQuote hypercall failed, status:%llx\n", err);
+ ret = -EIO;
+ goto done;
+ }
+
+ ret = wait_for_quote_completion(quote_buf, getquote_timeout);
+ if (ret) {
+ pr_err("GetQuote request timedout\n");
+ goto done;
+ }
+
+ buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ report->outblob = buf;
+ report->outblob_len = quote_buf->out_len;
+
+ /*
+ * TODO: parse the PEM-formatted cert chain out of the quote buffer when
+ * provided
+ */
+done:
+ mutex_unlock("e_lock);
+ kfree(reportdata);
+ kfree(tdreport);
+
+ return ret;
+}
+
static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -82,17 +278,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
};
MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
+static const struct tsm_ops tdx_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .report_new = tdx_report_new,
+};
+
static int __init tdx_guest_init(void)
{
+ int ret;
+
if (!x86_match_cpu(tdx_guest_ids))
return -ENODEV;
- return misc_register(&tdx_misc_dev);
+ ret = misc_register(&tdx_misc_dev);
+ if (ret)
+ return ret;
+
+ quote_data = alloc_quote_buf();
+ if (!quote_data) {
+ pr_err("Failed to allocate Quote buffer\n");
+ ret = -ENOMEM;
+ goto free_misc;
+ }
+
+ ret = tsm_register(&tdx_tsm_ops, NULL, NULL);
+ if (ret)
+ goto free_quote;
+
+ return 0;
+
+free_quote:
+ free_quote_buf(quote_data);
+free_misc:
+ misc_deregister(&tdx_misc_dev);
+
+ return ret;
}
module_init(tdx_guest_init);
static void __exit tdx_guest_exit(void)
{
+ tsm_unregister(&tdx_tsm_ops);
+ free_quote_buf(quote_data);
misc_deregister(&tdx_misc_dev);
}
module_exit(tdx_guest_exit);
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 4:17 ` [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
@ 2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
2023-09-26 18:59 ` Dan Williams
` (2 more replies)
2023-09-27 2:10 ` Kuppuswamy Sathyanarayanan
1 sibling, 3 replies; 29+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-26 18:49 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Hi Dan,
On 9/25/2023 9:17 PM, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its launch state, sign it and
> submit it to a verifying party. Upon successful attestation that
> verifies the integrity of the TVM additional secrets may be deployed.
> The concept is common across TSMs, but the implementations are
> unfortunately vendor specific. While the industry grapples with a common
> definition of this attestation format [1], Linux need not make this
> problem worse by defining a new ABI per TSM that wants to perform a
> similar operation. The current momentum has been to invent new ioctl-ABI
> per TSM per function which at best is an abdication of the kernel's
> responsibility to make common infrastructure concepts share common ABI.
>
> The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
> not more, is to define a configfs interface to retrieve the TSM-specific
> blob.
>
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> dd if=binary_userdata_plus_nonce > $report/inblob
> hexdump $report/outblob
>
> This approach later allows for the standardization of the attestation
> blob format without needing to invent a new ABI. Once standardization
> happens the standard format can be emitted by $report/outblob and
> indicated by $report/provider, or a new attribute like
> "$report/tcg_coco_report" can emit the standard format alongside the
> vendor format.
>
> Review of previous iterations of this interface identified that there is
> a need to scale report generation for multiple container environments
> [2]. Configfs enables a model where each container can bind mount one or
> more report generation item instances. Still, within a container only a
> single thread can be manipulating a given configuration instance at a
> time. A 'generation' count is provided to detect conflicts between
> multiple threads racing to configure a report instance.
>
> The SEV-SNP concepts of "extended reports" and "privilege levels" are
> optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> time. The expectation is that those concepts are generic enough that
> they may be adopted by other TSM implementations. In other words,
> configfs-tsm aims to address a superset of TSM specific functionality
> with a common ABI where attributes may appear, or not appear, based on the set
> of concepts the implementation supports.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Documentation/ABI/testing/configfs-tsm | 67 +++++
> MAINTAINERS | 8 +
> drivers/virt/coco/Kconfig | 5
> drivers/virt/coco/Makefile | 1
> drivers/virt/coco/tsm.c | 411 ++++++++++++++++++++++++++++++++
> include/linux/tsm.h | 63 +++++
> 6 files changed, 555 insertions(+)
> create mode 100644 Documentation/ABI/testing/configfs-tsm
> create mode 100644 drivers/virt/coco/tsm.c
> create mode 100644 include/linux/tsm.h
>
> diff --git a/Documentation/ABI/testing/configfs-tsm b/Documentation/ABI/testing/configfs-tsm
> new file mode 100644
> index 000000000000..ba81083046d3
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-tsm
> @@ -0,0 +1,67 @@
> +What: /sys/kernel/config/tsm/report/$name/inblob
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (WO) Up to 64 bytes of user specified binary data. For replay
> + protection this should include a nonce, but the kernel does not
> + place any restrictions on the content.
> +
> +What: /sys/kernel/config/tsm/report/$name/outblob
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Binary attestation report generated from @inblob and other
> + options The format of the report is implementation specific
> + where the implementation is conveyed via the @provider
> + attribute.
> +
> +What: /sys/kernel/config/tsm/report/$name/certs
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Zero or more certificates in concatenated PEM format. Refer
> + to implementation specific documentation on which certificates
> + might be returned.
> +
> +What: /sys/kernel/config/tsm/report/$name/provider
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) A name for the format-specification of @outblob like
> + "sev-snp" or "tdx" in the near term, or a common standard format
> + in the future.
> +
> +What: /sys/kernel/config/tsm/report/$name/generation
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) The value in this attribute increments each time @inblob or
> + any option is written. Userspace can detect conflicts by
> + checking generation before writing to any attribute and making
> + sure the number of writes matches expectations after reading
> + @outblob, or it can prevent conflicts by creating a report
> + instance per requesting context.
> +
> +What: /sys/kernel/config/tsm/report/$name/privlevel
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (WO) If a TSM implementation supports the concept of attestation
> + reports for TVMs running at different privilege levels, like
> + SEV-SNP "VMPL", specify the privilege level via this attribute.
> + The minimum acceptable value is conveyed via @privlevel_floor
> + and the maximum acceptable value is TSM_PRIVLEVEL_MAX (3).
> +
> +What: /sys/kernel/config/tsm/report/$name/privlevel_floor
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Indicates the minimum permissible value that can be written
> + to @privlevel.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..8acbeb029ba1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21889,6 +21889,14 @@ W: https://github.com/srcres258/linux-doc
> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
> F: Documentation/translations/zh_TW/
>
> +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
> +M: Dan Williams <dan.j.williams@intel.com>
> +L: linux-coco@lists.linux.dev
> +S: Maintained
> +F: Documentation/ABI/testing/configfs-tsm
> +F: drivers/virt/coco/tsm.c
> +F: include/linux/tsm.h
> +
> TTY LAYER AND SERIAL DRIVERS
> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> M: Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index fc5c64f04c4a..87d142c1f932 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -2,6 +2,11 @@
> #
> # Confidential computing related collateral
> #
> +
> +config TSM_REPORTS
> + select CONFIGFS_FS
> + tristate
> +
> source "drivers/virt/coco/efi_secret/Kconfig"
>
> source "drivers/virt/coco/sev-guest/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index 55302ef719ad..18c1aba5edb7 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -2,6 +2,7 @@
> #
> # Confidential computing related collateral
> #
> +obj-$(CONFIG_TSM_REPORTS) += tsm.o
> obj-$(CONFIG_EFI_SECRET) += efi_secret/
> obj-$(CONFIG_SEV_GUEST) += sev-guest/
> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> new file mode 100644
> index 000000000000..343fc77d0509
> --- /dev/null
> +++ b/drivers/virt/coco/tsm.c
> @@ -0,0 +1,411 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/rwsem.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/configfs.h>
> +
> +static struct tsm_provider {
> + const struct tsm_ops *ops;
> + const struct config_item_type *type;
> + void *data;
> +} provider;
> +static DECLARE_RWSEM(tsm_rwsem);
> +
> +/**
> + * DOC: Trusted Security Module (TSM) Attestation Report Interface
> + *
> + * The TSM report interface is a common provider of blobs that facilitate
> + * attestation of a TVM (confidential computing guest) by an attestation
> + * service. A TSM report combines a user-defined blob (likely a public-key with
> + * a nonce for a key-exchange protocol) with a signed attestation report. That
> + * combined blob is then used to obtain secrets provided by an agent that can
> + * validate the attestation report. The expectation is that this interface is
> + * invoked infrequently, however configfs allows for multiple agents to
> + * own their own report generation instances to generate reports as
> + * often as needed.
> + *
> + * The attestation report format is TSM provider specific, when / if a standard
> + * materializes that can be published instead of the vendor layout. Until then
> + * the 'provider' attribute indicates the format of 'outblob'. However,
> + * the common "return a list of certs" capability across multiple TSM
> + * implementations is returned in a unified @certs attribute.
> + */
> +
> +struct tsm_report_state {
> + struct tsm_report report;
> + unsigned long write_generation;
> + unsigned long read_generation;
> + struct config_item cfg;
> +};
> +
> +enum tsm_data_select {
> + TSM_REPORT,
> + TSM_CERTS,
> +};
> +
> +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> +{
> + struct tsm_report_state *state =
> + container_of(cfg, struct tsm_report_state, cfg);
> +
> + return &state->report;
> +}
> +
> +static struct tsm_report_state *to_state(struct tsm_report *report)
> +{
> + return container_of(report, struct tsm_report_state, report);
> +}
> +
> +static int try_advance_write_generation(struct tsm_report *report)
> +{
> + struct tsm_report_state *state = to_state(report);
> +
> + lockdep_assert_held_write(&tsm_rwsem);
> +
> + /*
> + * Malicious or broken userspace has written enough times for
> + * read_generation == write_generation by modular arithmetic without an
> + * interim read. Stop accepting updates until the current report
> + * configuration is read.
> + */
> + if (state->write_generation == state->read_generation - 1)
> + return -EBUSY;
> + state->write_generation++;
> + return 0;
> +}
> +
> +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> + const char *buf, size_t len)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + unsigned int val;
> + int rc;
> +
> + rc = kstrtouint(buf, 0, &val);
> + if (rc)
> + return rc;
> +
> + /*
> + * The valid privilege levels that a TSM might accept, if it accepts a
> + * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
> + * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
> + * than 0.
> + */
> + if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
> + return -EINVAL;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + rc = try_advance_write_generation(report);
> + if (rc)
> + return rc;
> + report->desc.privlevel = val;
> +
> + return len;
> +}
> +CONFIGFS_ATTR_WO(tsm_report_, privlevel);
> +
> +static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
> + char *buf)
> +{
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
> +
> +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> + const void *buf, size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + int rc;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + rc = try_advance_write_generation(report);
> + if (rc)
> + return rc;
> +
> + report->desc.inblob_len = count;
> + memcpy(report->desc.inblob, buf, count);
> + return count;
> +}
> +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
> +
> +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + struct tsm_report_state *state = to_state(report);
> +
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%lu\n", state->write_generation);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, generation);
> +
> +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
> +{
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%s\n", provider.ops->name);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, provider);
> +
> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> + enum tsm_data_select select)
> +{
> + loff_t offset = 0;
> + u8 *out, len;
> +
> + if (select == TSM_REPORT) {
> + out = report->outblob;
> + len = report->outblob_len;
> + } else {
> + out = report->certs;
> + len = report->certs_len;
> + }
> +
Since we get out and len from arch_ops, I think we can check for null condition before
attempting the memory_read_from_buffer()
> + if (!buf)
> + return len;
buf cannot be NULL, right? Do you want this check? If you want to leave it,
in NULL condition it should return 0 bytes, right?
> + return memory_read_from_buffer(buf, count, &offset, out, len);
> +}
> +
> +static ssize_t read_cached_report(struct tsm_report *report, void *buf,
> + size_t count, enum tsm_data_select select)
> +{
> + struct tsm_report_state *state = to_state(report);
> +
> + guard(rwsem_read)(&tsm_rwsem);
> + if (!report->desc.inblob_len)
> + return -EINVAL;
> +
> + /*
> + * A given TSM backend always fills in ->outblob regardless of
> + * whether the report includes certs or not.
> + */
> + if (!report->outblob ||
> + state->read_generation != state->write_generation)
> + return -EWOULDBLOCK;
> +
> + return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> + size_t count, enum tsm_data_select select)
> +{
> + struct tsm_report_state *state = to_state(report);
> + const struct tsm_ops *ops;
> + ssize_t rc;
> +
> + /* try to read from the existing report if present and valid... */
> + rc = read_cached_report(report, buf, count, select);
> + if (rc >= 0 || rc != -EWOULDBLOCK)
> + return rc;
> +
> + /* slow path, report may need to be regenerated... */
> + guard(rwsem_write)(&tsm_rwsem);
> + ops = provider.ops;
> + if (!report->desc.inblob_len)
> + return -EINVAL;
> +
> + /* did another thread already generate this report? */
> + if (report->outblob &&
> + state->read_generation == state->write_generation)
> + goto out;
> +
> + kvfree(report->outblob);
> + kvfree(report->certs);
> + report->outblob = NULL;
> + report->certs = NULL;
Since you are clearing outblob and certs, do you want to reset the outblob_len and certs_len?
> + rc = ops->report_new(report, provider.data);
> + if (rc < 0)
> + return rc;
> + state->read_generation = state->write_generation;
> +out:
> + return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> + size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> +
> + return tsm_report_read(report, buf, count, TSM_REPORT);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
> +
> +static ssize_t tsm_report_certs_read(struct config_item *cfg, void *buf,
> + size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> +
> + return tsm_report_read(report, buf, count, TSM_CERTS);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, certs, NULL, TSM_OUTBLOB_MAX);
> +
> +#define TSM_DEFAULT_ATTRS() \
> + &tsm_report_attr_generation, \
> + &tsm_report_attr_provider
> +
> +static struct configfs_attribute *tsm_report_attrs[] = {
> + TSM_DEFAULT_ATTRS(),
> + NULL,
> +};
> +
> +static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
> + &tsm_report_attr_inblob,
> + &tsm_report_attr_outblob,
> + &tsm_report_attr_certs,
> + NULL,
> +};
> +
> +static struct configfs_attribute *tsm_report_extra_attrs[] = {
> + TSM_DEFAULT_ATTRS(),
> + &tsm_report_attr_privlevel,
> + &tsm_report_attr_privlevel_floor,
> + NULL,
> +};
> +
> +static void tsm_report_item_release(struct config_item *cfg)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + struct tsm_report_state *state = to_state(report);
> +
> + kvfree(report->certs);
> + kvfree(report->outblob);
> + kfree(state);
> +}
> +
> +static struct configfs_item_operations tsm_report_item_ops = {
> + .release = tsm_report_item_release,
> +};
> +
> +const struct config_item_type tsm_report_default_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_bin_attrs = tsm_report_bin_attrs,
> + .ct_attrs = tsm_report_attrs,
> + .ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_default_type);
> +
> +const struct config_item_type tsm_report_ext_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_bin_attrs = tsm_report_bin_attrs,
> + .ct_attrs = tsm_report_extra_attrs,
> + .ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_ext_type);
> +
> +static struct config_item *tsm_report_make_item(struct config_group *group,
> + const char *name)
> +{
> + struct tsm_report_state *state;
> +
> + guard(rwsem_read)(&tsm_rwsem);
> + if (!provider.ops)
> + return ERR_PTR(-ENXIO);
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&state->cfg, name, provider.type);
> + return &state->cfg;
> +}
> +
> +static struct configfs_group_operations tsm_report_group_ops = {
> + .make_item = tsm_report_make_item,
> +};
> +
> +static const struct config_item_type tsm_reports_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_group_ops = &tsm_report_group_ops,
> +};
> +
> +static const struct config_item_type tsm_root_group_type = {
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem tsm_configfs = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "tsm",
> + .ci_type = &tsm_root_group_type,
> + },
> + },
> + .su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
> +};
> +
> +static struct config_group *tsm_report_group;
> +
> +int tsm_register(const struct tsm_ops *ops, void *priv,
> + const struct config_item_type *type)
> +{
> + const struct tsm_ops *conflict;
> +
> + if (!type)
> + type = &tsm_report_default_type;
> + if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
> + return -EINVAL;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + conflict = provider.ops;
> + if (conflict) {
> + pr_err("\"%s\" ops already registered\n", conflict->name);
> + return -EBUSY;
> + }
> +
> + provider.ops = ops;
> + provider.data = priv;
> + provider.type = type;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +int tsm_unregister(const struct tsm_ops *ops)
> +{
> + guard(rwsem_write)(&tsm_rwsem);
> + if (ops != provider.ops)
> + return -EBUSY;
> + provider.ops = NULL;
> + provider.data = NULL;
> + provider.type = NULL;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
> +
> +static int __init tsm_init(void)
> +{
> + struct config_group *root = &tsm_configfs.su_group;
> + struct config_group *tsm;
> + int rc;
> +
> + config_group_init(root);
> + rc = configfs_register_subsystem(&tsm_configfs);
> + if (rc)
> + return rc;
> +
> + tsm = configfs_register_default_group(root, "report",
> + &tsm_reports_type);
> + if (IS_ERR(tsm)) {
> + configfs_unregister_subsystem(&tsm_configfs);
> + return PTR_ERR(tsm);
> + }
> + tsm_report_group = tsm;
> +
> + return 0;
> +}
> +module_init(tsm_init);
> +
> +static void __exit tsm_exit(void)
> +{
> + configfs_unregister_default_group(tsm_report_group);
> + configfs_unregister_subsystem(&tsm_configfs);
> +}
> +module_exit(tsm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> new file mode 100644
> index 000000000000..1fe1dba3a912
> --- /dev/null
> +++ b/include/linux/tsm.h
> @@ -0,0 +1,63 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_H
> +#define __TSM_H
> +
> +#include <linux/sizes.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +
> +#define TSM_INBLOB_MAX 64
> +#define TSM_OUTBLOB_MAX SZ_32K
> +
> +/*
> + * Privilege level is a nested permission concept to allow confidential
> + * guests to partition address space, 4-levels are supported.
> + */
> +#define TSM_PRIVLEVEL_MAX 3
> +
> +/**
> + * struct tsm_desc - option descriptor for generating tsm report blobs
> + * @privlevel: optional privilege level to associate with @outblob
> + * @inblob_len: sizeof @inblob
> + * @inblob: arbitrary input data
> + */
> +struct tsm_desc {
> + unsigned int privlevel;
> + size_t inblob_len;
> + u8 inblob[TSM_INBLOB_MAX];
> +};
> +
> +/**
> + * struct tsm_report - track state of report generation relative to options
> + * @desc: report generation options / cached report state
> + * @outblob: generated evidence to provider to the attestation agent
> + * @outblob_len: sizeof(outblob)
I think following is incorrect. You might want to add info about certs_len
and certs.
> + * @write_generation: conflict detection, and report regeneration tracking
> + * @read_generation: cached report invalidation tracking
> + * @cfg: configfs interface
> + */
> +struct tsm_report {
> + struct tsm_desc desc;
> + size_t outblob_len;
> + u8 *outblob;
> + size_t certs_len;
> + u8 *certs;
> +};
> +
> +/*
> + * arch specific ops, only one is expected to be registered at a time
> + * i.e. only one of SEV, TDX, COVE, etc.
> + */
Since it is ARCH specific ops, I think adding some info about its members
will be helpful. Like what is report_new callback and its acceptable
return values.
> +struct tsm_ops {
> + const char *name;
> + const int privlevel_floor;
> + int (*report_new)(struct tsm_report *desc, void *data);
> +};
> +
> +extern const struct config_item_type tsm_report_ext_type;
> +extern const struct config_item_type tsm_report_default_type;
> +
> +int tsm_register(const struct tsm_ops *ops, void *priv,
> + const struct config_item_type *type);
> +int tsm_unregister(const struct tsm_ops *ops);
> +#endif /* __TSM_H */
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report()
2023-09-26 4:17 ` [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
@ 2023-09-26 18:51 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 29+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-26 18:51 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh,
peterz, linux-kernel, x86, dave.hansen
On 9/25/2023 9:17 PM, Dan Williams wrote:
> In preparation for using the configs-tsm facility to convey attestation
> blobs to userspace, switch to using the 'sockptr' api for copying
> payloads to provided buffers where 'sockptr' handles user vs kernel
> buffers.
>
> While configfs-tsm is meant to replace existing confidential computing
> ioctl() implementations for attestation report retrieval the old ioctl()
> path needs to stick around for a deprecation period.
>
> No behavior change intended.
>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> drivers/virt/coco/sev-guest/sev-guest.c | 50 ++++++++++++++++++++-----------
> 1 file changed, 33 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
> index 97dbe715e96a..c3c9e9ea691f 100644
> --- a/drivers/virt/coco/sev-guest/sev-guest.c
> +++ b/drivers/virt/coco/sev-guest/sev-guest.c
> @@ -19,6 +19,7 @@
> #include <crypto/aead.h>
> #include <linux/scatterlist.h>
> #include <linux/psp-sev.h>
> +#include <linux/sockptr.h>
> #include <uapi/linux/sev-guest.h>
> #include <uapi/linux/psp-sev.h>
>
> @@ -470,7 +471,13 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
> return 0;
> }
>
> -static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> +struct snp_req_resp {
> + sockptr_t req_data;
> + sockptr_t resp_data;
> +};
> +
> +static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg,
> + struct snp_req_resp *io)
> {
> struct snp_guest_crypto *crypto = snp_dev->crypto;
> struct snp_report_resp *resp;
> @@ -479,10 +486,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
>
> lockdep_assert_held(&snp_cmd_mutex);
>
> - if (!arg->req_data || !arg->resp_data)
> + if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
> return -EINVAL;
>
> - if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> + if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
> return -EFAULT;
>
> /*
> @@ -501,7 +508,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
> if (rc)
> goto e_free;
>
> - if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> + if (copy_to_sockptr(io->resp_data, resp, sizeof(*resp)))
> rc = -EFAULT;
>
> e_free:
> @@ -550,22 +557,25 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
> return rc;
> }
>
> -static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
> +static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg,
> + struct snp_req_resp *io)
> +
> {
> struct snp_guest_crypto *crypto = snp_dev->crypto;
> struct snp_ext_report_req req;
> struct snp_report_resp *resp;
> int ret, npages = 0, resp_len;
> + sockptr_t certs_address;
>
> lockdep_assert_held(&snp_cmd_mutex);
>
> - if (!arg->req_data || !arg->resp_data)
> + if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data))
> return -EINVAL;
>
> - if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
> + if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
> return -EFAULT;
>
> - /* userspace does not want certificate data */
> + /* caller does not want certificate data */
> if (!req.certs_len || !req.certs_address)
> goto cmd;
>
> @@ -573,8 +583,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
> !IS_ALIGNED(req.certs_len, PAGE_SIZE))
> return -EINVAL;
>
> - if (!access_ok((const void __user *)req.certs_address, req.certs_len))
> - return -EFAULT;
> + if (sockptr_is_kernel(io->resp_data)) {
> + certs_address = KERNEL_SOCKPTR((void *)req.certs_address);
> + } else {
> + certs_address = USER_SOCKPTR((void __user *)req.certs_address);
> + if (!access_ok(certs_address.user, req.certs_len))
> + return -EFAULT;
> + }
>
> /*
> * Initialize the intermediate buffer with all zeros. This buffer
> @@ -604,21 +619,19 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
> if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
> req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
>
> - if (copy_to_user((void __user *)arg->req_data, &req, sizeof(req)))
> + if (copy_to_sockptr(io->req_data, &req, sizeof(req)))
> ret = -EFAULT;
> }
>
> if (ret)
> goto e_free;
>
> - if (npages &&
> - copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
> - req.certs_len)) {
> + if (npages && copy_to_sockptr(certs_address, snp_dev->certs_data, req.certs_len)) {
> ret = -EFAULT;
> goto e_free;
> }
>
> - if (copy_to_user((void __user *)arg->resp_data, resp, sizeof(*resp)))
> + if (copy_to_sockptr(io->resp_data, resp, sizeof(*resp)))
> ret = -EFAULT;
>
> e_free:
> @@ -631,6 +644,7 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
> struct snp_guest_dev *snp_dev = to_snp_dev(file);
> void __user *argp = (void __user *)arg;
> struct snp_guest_request_ioctl input;
> + struct snp_req_resp io;
> int ret = -ENOTTY;
>
> if (copy_from_user(&input, argp, sizeof(input)))
> @@ -651,15 +665,17 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
> return -ENOTTY;
> }
>
> + io.req_data = USER_SOCKPTR((void __user *)input.req_data);
> + io.resp_data = USER_SOCKPTR((void __user *)input.resp_data);
> switch (ioctl) {
> case SNP_GET_REPORT:
> - ret = get_report(snp_dev, &input);
> + ret = get_report(snp_dev, &input, &io);
> break;
> case SNP_GET_DERIVED_KEY:
> ret = get_derived_key(snp_dev, &input);
> break;
> case SNP_GET_EXT_REPORT:
> - ret = get_ext_report(snp_dev, &input);
> + ret = get_ext_report(snp_dev, &input, &io);
> break;
> default:
> break;
>
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
@ 2023-09-26 18:59 ` Dan Williams
2023-09-27 0:43 ` Kuppuswamy Sathyanarayanan
2023-09-27 8:04 ` Thomas Fossati
2023-09-27 8:43 ` Thomas Fossati
2 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-26 18:59 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Kuppuswamy Sathyanarayanan wrote:
> Hi Dan,
>
> On 9/25/2023 9:17 PM, Dan Williams wrote:
> > One of the common operations of a TSM (Trusted Security Module) is to
> > provide a way for a TVM (confidential computing guest execution
> > environment) to take a measurement of its launch state, sign it and
> > submit it to a verifying party. Upon successful attestation that
> > verifies the integrity of the TVM additional secrets may be deployed.
> > The concept is common across TSMs, but the implementations are
> > unfortunately vendor specific. While the industry grapples with a common
> > definition of this attestation format [1], Linux need not make this
> > problem worse by defining a new ABI per TSM that wants to perform a
> > similar operation. The current momentum has been to invent new ioctl-ABI
> > per TSM per function which at best is an abdication of the kernel's
> > responsibility to make common infrastructure concepts share common ABI.
> >
> > The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
> > not more, is to define a configfs interface to retrieve the TSM-specific
> > blob.
> >
> > report=/sys/kernel/config/tsm/report/report0
> > mkdir $report
> > dd if=binary_userdata_plus_nonce > $report/inblob
> > hexdump $report/outblob
> >
> > This approach later allows for the standardization of the attestation
> > blob format without needing to invent a new ABI. Once standardization
> > happens the standard format can be emitted by $report/outblob and
> > indicated by $report/provider, or a new attribute like
> > "$report/tcg_coco_report" can emit the standard format alongside the
> > vendor format.
> >
> > Review of previous iterations of this interface identified that there is
> > a need to scale report generation for multiple container environments
> > [2]. Configfs enables a model where each container can bind mount one or
> > more report generation item instances. Still, within a container only a
> > single thread can be manipulating a given configuration instance at a
> > time. A 'generation' count is provided to detect conflicts between
> > multiple threads racing to configure a report instance.
> >
> > The SEV-SNP concepts of "extended reports" and "privilege levels" are
> > optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> > time. The expectation is that those concepts are generic enough that
> > they may be adopted by other TSM implementations. In other words,
> > configfs-tsm aims to address a superset of TSM specific functionality
> > with a common ABI where attributes may appear, or not appear, based on the set
> > of concepts the implementation supports.
> >
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> > Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> > Cc: Peter Gonda <pgonda@google.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Samuel Ortiz <sameo@rivosinc.com>
> > Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Acked-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
[..]
> > +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> > + enum tsm_data_select select)
> > +{
> > + loff_t offset = 0;
> > + u8 *out, len;
> > +
> > + if (select == TSM_REPORT) {
> > + out = report->outblob;
> > + len = report->outblob_len;
> > + } else {
> > + out = report->certs;
> > + len = report->certs_len;
> > + }
> > +
>
> Since we get out and len from arch_ops, I think we can check for null condition before
> attempting the memory_read_from_buffer()
>
> > + if (!buf)
> > + return len;
>
> buf cannot be NULL, right? Do you want this check? If you want to leave it,
> in NULL condition it should return 0 bytes, right?
No, and this might deserve a comment for folks that are not familiar
with how configfs works, but configfs calls an attribute's ->read()
helper with @buf == NULL to say "please tell me how many bytes are
available, and I will call you back again to fill in the buffer at that
size".
>
> > + return memory_read_from_buffer(buf, count, &offset, out, len);
> > +}
> > +
> > +static ssize_t read_cached_report(struct tsm_report *report, void *buf,
> > + size_t count, enum tsm_data_select select)
> > +{
> > + struct tsm_report_state *state = to_state(report);
> > +
> > + guard(rwsem_read)(&tsm_rwsem);
> > + if (!report->desc.inblob_len)
> > + return -EINVAL;
> > +
> > + /*
> > + * A given TSM backend always fills in ->outblob regardless of
> > + * whether the report includes certs or not.
> > + */
> > + if (!report->outblob ||
> > + state->read_generation != state->write_generation)
> > + return -EWOULDBLOCK;
> > +
> > + return __read_report(report, buf, count, select);
> > +}
> > +
> > +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> > + size_t count, enum tsm_data_select select)
> > +{
> > + struct tsm_report_state *state = to_state(report);
> > + const struct tsm_ops *ops;
> > + ssize_t rc;
> > +
> > + /* try to read from the existing report if present and valid... */
> > + rc = read_cached_report(report, buf, count, select);
> > + if (rc >= 0 || rc != -EWOULDBLOCK)
> > + return rc;
> > +
> > + /* slow path, report may need to be regenerated... */
> > + guard(rwsem_write)(&tsm_rwsem);
> > + ops = provider.ops;
> > + if (!report->desc.inblob_len)
> > + return -EINVAL;
> > +
> > + /* did another thread already generate this report? */
> > + if (report->outblob &&
> > + state->read_generation == state->write_generation)
> > + goto out;
> > +
> > + kvfree(report->outblob);
> > + kvfree(report->certs);
> > + report->outblob = NULL;
> > + report->certs = NULL;
>
> Since you are clearing outblob and certs, do you want to reset the outblob_len and certs_len?
Not strictly necessary, nothing in the code is checking _len for whether
the report is ready or not.
[..]
> > +/**
> > + * struct tsm_desc - option descriptor for generating tsm report blobs
> > + * @privlevel: optional privilege level to associate with @outblob
> > + * @inblob_len: sizeof @inblob
> > + * @inblob: arbitrary input data
> > + */
> > +struct tsm_desc {
> > + unsigned int privlevel;
> > + size_t inblob_len;
> > + u8 inblob[TSM_INBLOB_MAX];
> > +};
> > +
> > +/**
> > + * struct tsm_report - track state of report generation relative to options
> > + * @desc: report generation options / cached report state
> > + * @outblob: generated evidence to provider to the attestation agent
> > + * @outblob_len: sizeof(outblob)
>
> I think following is incorrect. You might want to add info about certs_len
> and certs.
Yeah, missed updating this with certs addition. The outblob_len
definition is correct, or do you mean the kdoc is out of order with
respect to the struct?
>
> > + * @write_generation: conflict detection, and report regeneration tracking
> > + * @read_generation: cached report invalidation tracking
> > + * @cfg: configfs interface
> > + */
> > +struct tsm_report {
> > + struct tsm_desc desc;
> > + size_t outblob_len;
> > + u8 *outblob;
> > + size_t certs_len;
> > + u8 *certs;
> > +};
> > +
> > +/*
> > + * arch specific ops, only one is expected to be registered at a time
> > + * i.e. only one of SEV, TDX, COVE, etc.
> > + */
>
> Since it is ARCH specific ops, I think adding some info about its members
> will be helpful. Like what is report_new callback and its acceptable
> return values.
Sure.
Will wait for positive test feedback about the sev-guest changes before
spinning this series again.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 18:59 ` Dan Williams
@ 2023-09-27 0:43 ` Kuppuswamy Sathyanarayanan
2023-09-27 3:17 ` Dan Williams
0 siblings, 1 reply; 29+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-27 0:43 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
On 9/26/2023 11:59 AM, Dan Williams wrote:
> Kuppuswamy Sathyanarayanan wrote:
>> Hi Dan,
>>
>> On 9/25/2023 9:17 PM, Dan Williams wrote:
>>> One of the common operations of a TSM (Trusted Security Module) is to
>>> provide a way for a TVM (confidential computing guest execution
>>> environment) to take a measurement of its launch state, sign it and
>>> submit it to a verifying party. Upon successful attestation that
>>> verifies the integrity of the TVM additional secrets may be deployed.
>>> The concept is common across TSMs, but the implementations are
>>> unfortunately vendor specific. While the industry grapples with a common
>>> definition of this attestation format [1], Linux need not make this
>>> problem worse by defining a new ABI per TSM that wants to perform a
>>> similar operation. The current momentum has been to invent new ioctl-ABI
>>> per TSM per function which at best is an abdication of the kernel's
>>> responsibility to make common infrastructure concepts share common ABI.
>>>
>>> The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
>>> not more, is to define a configfs interface to retrieve the TSM-specific
>>> blob.
>>>
>>> report=/sys/kernel/config/tsm/report/report0
>>> mkdir $report
>>> dd if=binary_userdata_plus_nonce > $report/inblob
>>> hexdump $report/outblob
>>>
>>> This approach later allows for the standardization of the attestation
>>> blob format without needing to invent a new ABI. Once standardization
>>> happens the standard format can be emitted by $report/outblob and
>>> indicated by $report/provider, or a new attribute like
>>> "$report/tcg_coco_report" can emit the standard format alongside the
>>> vendor format.
>>>
>>> Review of previous iterations of this interface identified that there is
>>> a need to scale report generation for multiple container environments
>>> [2]. Configfs enables a model where each container can bind mount one or
>>> more report generation item instances. Still, within a container only a
>>> single thread can be manipulating a given configuration instance at a
>>> time. A 'generation' count is provided to detect conflicts between
>>> multiple threads racing to configure a report instance.
>>>
>>> The SEV-SNP concepts of "extended reports" and "privilege levels" are
>>> optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
>>> time. The expectation is that those concepts are generic enough that
>>> they may be adopted by other TSM implementations. In other words,
>>> configfs-tsm aims to address a superset of TSM specific functionality
>>> with a common ABI where attributes may appear, or not appear, based on the set
>>> of concepts the implementation supports.
>>>
>>> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
>>> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
>>> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
>>> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
>>> Cc: Peter Gonda <pgonda@google.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: Samuel Ortiz <sameo@rivosinc.com>
>>> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Acked-by: Thomas Gleixner <tglx@linutronix.de>
>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> [..]
>>> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
>>> + enum tsm_data_select select)
>>> +{
>>> + loff_t offset = 0;
>>> + u8 *out, len;
>>> +
>>> + if (select == TSM_REPORT) {
>>> + out = report->outblob;
>>> + len = report->outblob_len;
>>> + } else {
>>> + out = report->certs;
>>> + len = report->certs_len;
>>> + }
>>> +
>>
>> Since we get out and len from arch_ops, I think we can check for null condition before
>> attempting the memory_read_from_buffer()
>>
>>> + if (!buf)
>>> + return len;
>>
>> buf cannot be NULL, right? Do you want this check? If you want to leave it,
>> in NULL condition it should return 0 bytes, right?
>
> No, and this might deserve a comment for folks that are not familiar
> with how configfs works, but configfs calls an attribute's ->read()
> helper with @buf == NULL to say "please tell me how many bytes are
> available, and I will call you back again to fill in the buffer at that
> size".
>
Got it. Thanks for clarifying it.
>>
>>> + return memory_read_from_buffer(buf, count, &offset, out, len);
>>> +}
>>> +
>>> +static ssize_t read_cached_report(struct tsm_report *report, void *buf,
>>> + size_t count, enum tsm_data_select select)
>>> +{
>>> + struct tsm_report_state *state = to_state(report);
>>> +
>>> + guard(rwsem_read)(&tsm_rwsem);
>>> + if (!report->desc.inblob_len)
>>> + return -EINVAL;
>>> +
>>> + /*
>>> + * A given TSM backend always fills in ->outblob regardless of
>>> + * whether the report includes certs or not.
>>> + */
>>> + if (!report->outblob ||
>>> + state->read_generation != state->write_generation)
>>> + return -EWOULDBLOCK;
>>> +
>>> + return __read_report(report, buf, count, select);
>>> +}
>>> +
>>> +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
>>> + size_t count, enum tsm_data_select select)
>>> +{
>>> + struct tsm_report_state *state = to_state(report);
>>> + const struct tsm_ops *ops;
>>> + ssize_t rc;
>>> +
>>> + /* try to read from the existing report if present and valid... */
>>> + rc = read_cached_report(report, buf, count, select);
>>> + if (rc >= 0 || rc != -EWOULDBLOCK)
>>> + return rc;
>>> +
>>> + /* slow path, report may need to be regenerated... */
>>> + guard(rwsem_write)(&tsm_rwsem);
>>> + ops = provider.ops;
>>> + if (!report->desc.inblob_len)
>>> + return -EINVAL;
>>> +
>>> + /* did another thread already generate this report? */
>>> + if (report->outblob &&
>>> + state->read_generation == state->write_generation)
>>> + goto out;
>>> +
>>> + kvfree(report->outblob);
>>> + kvfree(report->certs);
>>> + report->outblob = NULL;
>>> + report->certs = NULL;
>>
>> Since you are clearing outblob and certs, do you want to reset the outblob_len and certs_len?
>
> Not strictly necessary, nothing in the code is checking _len for whether
> the report is ready or not.
ok.
>
> [..]
>>> +/**
>>> + * struct tsm_desc - option descriptor for generating tsm report blobs
>>> + * @privlevel: optional privilege level to associate with @outblob
>>> + * @inblob_len: sizeof @inblob
>>> + * @inblob: arbitrary input data
>>> + */
>>> +struct tsm_desc {
>>> + unsigned int privlevel;
>>> + size_t inblob_len;
>>> + u8 inblob[TSM_INBLOB_MAX];
>>> +};
>>> +
>>> +/**
>>> + * struct tsm_report - track state of report generation relative to options
>>> + * @desc: report generation options / cached report state
>>> + * @outblob: generated evidence to provider to the attestation agent
>>> + * @outblob_len: sizeof(outblob)
>>
>> I think following is incorrect. You might want to add info about certs_len
>> and certs.
>
> Yeah, missed updating this with certs addition. The outblob_len
> definition is correct, or do you mean the kdoc is out of order with
> respect to the struct?
No, I am talking about the write_generation, read_generation and cfg options.
They are part of struct tsm_report_state, so why document it here?
>
>>
>>> + * @write_generation: conflict detection, and report regeneration tracking
>>> + * @read_generation: cached report invalidation tracking
>>> + * @cfg: configfs interface
>>> + */
>>> +struct tsm_report {
>>> + struct tsm_desc desc;
>>> + size_t outblob_len;
>>> + u8 *outblob;
>>> + size_t certs_len;
>>> + u8 *certs;
>>> +};
>>> +
>>> +/*
>>> + * arch specific ops, only one is expected to be registered at a time
>>> + * i.e. only one of SEV, TDX, COVE, etc.
>>> + */
>>
>> Since it is ARCH specific ops, I think adding some info about its members
>> will be helpful. Like what is report_new callback and its acceptable
>> return values.
>
> Sure.
>
> Will wait for positive test feedback about the sev-guest changes before
> spinning this series again.
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 4:17 ` [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
@ 2023-09-27 2:10 ` Kuppuswamy Sathyanarayanan
1 sibling, 0 replies; 29+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-09-27 2:10 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
On 9/25/2023 9:17 PM, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its launch state, sign it and
> submit it to a verifying party. Upon successful attestation that
> verifies the integrity of the TVM additional secrets may be deployed.
> The concept is common across TSMs, but the implementations are
> unfortunately vendor specific. While the industry grapples with a common
> definition of this attestation format [1], Linux need not make this
> problem worse by defining a new ABI per TSM that wants to perform a
> similar operation. The current momentum has been to invent new ioctl-ABI
> per TSM per function which at best is an abdication of the kernel's
> responsibility to make common infrastructure concepts share common ABI.
>
> The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
> not more, is to define a configfs interface to retrieve the TSM-specific
> blob.
>
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> dd if=binary_userdata_plus_nonce > $report/inblob
> hexdump $report/outblob
>
> This approach later allows for the standardization of the attestation
> blob format without needing to invent a new ABI. Once standardization
> happens the standard format can be emitted by $report/outblob and
> indicated by $report/provider, or a new attribute like
> "$report/tcg_coco_report" can emit the standard format alongside the
> vendor format.
>
> Review of previous iterations of this interface identified that there is
> a need to scale report generation for multiple container environments
> [2]. Configfs enables a model where each container can bind mount one or
> more report generation item instances. Still, within a container only a
> single thread can be manipulating a given configuration instance at a
> time. A 'generation' count is provided to detect conflicts between
> multiple threads racing to configure a report instance.
>
> The SEV-SNP concepts of "extended reports" and "privilege levels" are
> optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> time. The expectation is that those concepts are generic enough that
> they may be adopted by other TSM implementations. In other words,
> configfs-tsm aims to address a superset of TSM specific functionality
> with a common ABI where attributes may appear, or not appear, based on the set
> of concepts the implementation supports.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Documentation/ABI/testing/configfs-tsm | 67 +++++
> MAINTAINERS | 8 +
> drivers/virt/coco/Kconfig | 5
> drivers/virt/coco/Makefile | 1
> drivers/virt/coco/tsm.c | 411 ++++++++++++++++++++++++++++++++
> include/linux/tsm.h | 63 +++++
> 6 files changed, 555 insertions(+)
> create mode 100644 Documentation/ABI/testing/configfs-tsm
> create mode 100644 drivers/virt/coco/tsm.c
> create mode 100644 include/linux/tsm.h
>
[...]
> +
> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> + enum tsm_data_select select)
> +{
> + loff_t offset = 0;
> + u8 *out, len;
When testing I noticed that it reports incorrect buf len. After debugging, noticed
that using u8 for len is incorrect. It should be size_t.
> +
> + if (select == TSM_REPORT) {
> + out = report->outblob;
> + len = report->outblob_len;
> + } else {
> + out = report->certs;
> + len = report->certs_len;
> + }
> +
> + if (!buf)
> + return len;
> + return memory_read_from_buffer(buf, count, &offset, out, len);
> +}
> +
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-27 0:43 ` Kuppuswamy Sathyanarayanan
@ 2023-09-27 3:17 ` Dan Williams
0 siblings, 0 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-27 3:17 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan, Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Kuppuswamy Sathyanarayanan wrote:
[..]
> >>> +/**
> >>> + * struct tsm_desc - option descriptor for generating tsm report blobs
> >>> + * @privlevel: optional privilege level to associate with @outblob
> >>> + * @inblob_len: sizeof @inblob
> >>> + * @inblob: arbitrary input data
> >>> + */
> >>> +struct tsm_desc {
> >>> + unsigned int privlevel;
> >>> + size_t inblob_len;
> >>> + u8 inblob[TSM_INBLOB_MAX];
> >>> +};
> >>> +
> >>> +/**
> >>> + * struct tsm_report - track state of report generation relative to options
> >>> + * @desc: report generation options / cached report state
> >>> + * @outblob: generated evidence to provider to the attestation agent
> >>> + * @outblob_len: sizeof(outblob)
> >>
> >> I think following is incorrect. You might want to add info about certs_len
> >> and certs.
> >
> > Yeah, missed updating this with certs addition. The outblob_len
> > definition is correct, or do you mean the kdoc is out of order with
> > respect to the struct?
>
> No, I am talking about the write_generation, read_generation and cfg options.
> They are part of struct tsm_report_state, so why document it here?
Ah yup, will fix.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
2023-09-26 18:59 ` Dan Williams
@ 2023-09-27 8:04 ` Thomas Fossati
2023-09-27 8:21 ` Dan Williams
2023-09-27 8:43 ` Thomas Fossati
2 siblings, 1 reply; 29+ messages in thread
From: Thomas Fossati @ 2023-09-27 8:04 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Hi Dan,
> On 9/25/2023 9:17 PM, Dan Williams wrote:
> > +++ b/include/linux/tsm.h
> > @@ -0,0 +1,63 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_H
> > +#define __TSM_H
> > +
> > +#include <linux/sizes.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +
> > +#define TSM_INBLOB_MAX 64
I guess @inblob is supposed to (possibly) accommodate nonces from a
challenger, correct?
If so, 64 bytes may not be enough for attesters that produce
EAT-formatted reports -- see [1], and [2].
[1] https://www.ietf.org/archive/id/draft-ietf-rats-eat-21.html#section-4.1-5
[2] https://github.com/ietf-rats-wg/eat/pull/421/files
cheers, thanks
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-27 8:04 ` Thomas Fossati
@ 2023-09-27 8:21 ` Dan Williams
2023-09-27 8:25 ` Thomas Fossati
0 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-27 8:21 UTC (permalink / raw)
To: Thomas Fossati, Dan Williams
Cc: linux-coco, Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Thomas Fossati wrote:
> Hi Dan,
>
> > On 9/25/2023 9:17 PM, Dan Williams wrote:
> > > +++ b/include/linux/tsm.h
> > > @@ -0,0 +1,63 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __TSM_H
> > > +#define __TSM_H
> > > +
> > > +#include <linux/sizes.h>
> > > +#include <linux/types.h>
> > > +#include <linux/device.h>
> > > +
> > > +#define TSM_INBLOB_MAX 64
>
> I guess @inblob is supposed to (possibly) accommodate nonces from a
> challenger, correct?
> If so, 64 bytes may not be enough for attesters that produce
> EAT-formatted reports -- see [1], and [2].
>
> [1] https://www.ietf.org/archive/id/draft-ietf-rats-eat-21.html#section-4.1-5
> [2] https://github.com/ietf-rats-wg/eat/pull/421/files
Hmm, but 64-bytes is the hard limit of the supported platform
implementations.
It can be expanded when/if those platforms expand the
size of the supported user data, or another configfs-tsm backend arrives
that needs that capability.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-27 8:21 ` Dan Williams
@ 2023-09-27 8:25 ` Thomas Fossati
2023-09-27 14:38 ` Peter Gonda
0 siblings, 1 reply; 29+ messages in thread
From: Thomas Fossati @ 2023-09-27 8:25 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
On Wed, 27 Sept 2023 at 10:21, Dan Williams <dan.j.williams@intel.com> wrote:
> It can be expanded when/if those platforms expand the
> size of the supported user data, or another configfs-tsm backend arrives
> that needs that capability.
Makes sense, thanks.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
2023-09-26 18:59 ` Dan Williams
2023-09-27 8:04 ` Thomas Fossati
@ 2023-09-27 8:43 ` Thomas Fossati
2 siblings, 0 replies; 29+ messages in thread
From: Thomas Fossati @ 2023-09-27 8:43 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Caveat: newbie here (just subscribed the linux-coco list) trying to
bridge the abstract language of IETF RATS with an actual
implementation of the architecture.
> +struct tsm_report {
> + struct tsm_desc desc;
> + size_t outblob_len;
> + u8 *outblob;
> + size_t certs_len;
> + u8 *certs;
> +};
Could you clarify the semantics of @certs?
Are these what the IETF calls platform "endorsements" [1], [2]?
Or could a DICE report (which is a cert chain) fall into this bucket too?
If the former, maybe @endorsement_certs (or similar) would minimise ambiguity.
[1] https://www.rfc-editor.org/rfc/rfc9334.html#name-endorsements
[2] https://www.ietf.org/archive/id/draft-dthaler-rats-endorsements-02.html
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-27 8:25 ` Thomas Fossati
@ 2023-09-27 14:38 ` Peter Gonda
2023-09-27 19:05 ` Thomas Fossati
0 siblings, 1 reply; 29+ messages in thread
From: Peter Gonda @ 2023-09-27 14:38 UTC (permalink / raw)
To: Thomas Fossati
Cc: Dan Williams, linux-coco, Dionna Amalie Glaze, James Bottomley,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
On Wed, Sep 27, 2023 at 2:25 AM Thomas Fossati
<thomas.fossati@linaro.org> wrote:
>
> On Wed, 27 Sept 2023 at 10:21, Dan Williams <dan.j.williams@intel.com> wrote:
> > It can be expanded when/if those platforms expand the
> > size of the supported user data, or another configfs-tsm backend arrives
> > that needs that capability.
>
> Makes sense, thanks.
I'm not familiar with the rats eat spec but I would assume the
protocol would acquire more than just the nonce in the inblob.
Probably some combination of claims, nonce, and information about a
public key? Does the specification allow for the data needing to be
signed by the TSM to be hashed first?
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-26 4:17 ` [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
@ 2023-09-27 16:14 ` Peter Gonda
2023-09-27 16:53 ` Dan Williams
2023-09-28 22:49 ` Dan Williams
0 siblings, 2 replies; 29+ messages in thread
From: Peter Gonda @ 2023-09-27 16:14 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
linux-kernel, x86, dave.hansen
On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> In TDX guest, the attestation process is used to verify the TDX guest
> trustworthiness to other entities before provisioning secrets to the
> guest. The first step in the attestation process is TDREPORT
> generation, which involves getting the guest measurement data in the
> format of TDREPORT, which is further used to validate the authenticity
> of the TDX guest. TDREPORT by design is integrity-protected and can
> only be verified on the local machine.
>
> To support remote verification of the TDREPORT in a SGX-based
> attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> only run outside of the TDX guest (i.e. in a host process or in a
> normal VM) and guest can use communication channels like vsock or
> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> TDX guest may not support these communication channels. To handle such
> cases, TDX defines a GetQuote hypercall which can be used by the guest
> to request the host VMM to communicate with the SGX QE. More details
> about GetQuote hypercall can be found in TDX Guest-Host Communication
> Interface (GHCI) for Intel TDX 1.0, section titled
> "TDG.VP.VMCALL<GetQuote>".
>
> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> Computing Guest platforms to get the measurement data via ConfigFS.
> Extend the TSM framework and add support to allow an attestation agent
> to get the TDX Quote data (included usage example below).
>
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> dd if=/dev/urandom bs=64 count=1 > $report/inblob
> hexdump -C $report/outblob
> rmdir $report
>
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate a large enough memory and mark it shared using
> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> for GetQuote requests in the TDX TSM handler.
>
> Although this method reserves a fixed chunk of memory for GetQuote
> requests, such one time allocation can help avoid memory fragmentation
> related allocation failures later in the uptime of the guest.
>
> Since the Quote generation process is not time-critical or frequently
> used, the current version uses a polling model for Quote requests and
> it also does not support parallel GetQuote requests.
>
> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
Hey Dan,
I tried running your test commands on an SNP enabled guest. To build
the kernel I just checked out linus/master and applied your series. I
haven't done any debugging yet, so I will update with what I find.
root@Ubuntu2004:~# hexdump -C $report/outblob
[ 219.871875] ------------[ cut here ]------------
[ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
[ 219.882280] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
[ 219.887628] CPU: 0 PID: 1317 Comm: hexdump Not tainted 6.6.0-rc3-xfstests-000
44-gf636850ddfc7 #1
[ 219.896530] Hardware name: Google Google Compute Engine/Google Compute Engine
, BIOS Google 09/26/2023
[ 219.905859] RIP: 0010:enc_dec_message+0x4ed/0x570
[ 219.910673] Code: c7 c0 00 00 00 80 48 2b 05 b8 4f 99 00 e9 dc fd ff ff 0f 0b
bb f4 ff ff ff eb b5 0f 0b 0f 0b 0f 0b e8 e7 47 ce ff 89 c3 eb 94 <0f> 0b 0f 0b
0f 0b 0f 0b 48 8d 7c 24 38 e8 11 6b 23 00 8b 5c 24 58
[ 219.929547] RSP: 0018:ffffc90000e27a18 EFLAGS: 00010246
[ 219.934893] RAX: 0000000000000000 RBX: ffffc90000e27bf8 RCX: 0000000000081000
[ 219.942134] RDX: 0000000000000000 RSI: 0000000000080000 RDI: ffffc90080e27bf8
[ 219.949378] RBP: ffff8881018980a0 R08: 0000000000000000 R09: ffffc90000e27a78
[ 219.956621] R10: 0000000000026680 R11: 0000000000000008 R12: ffff888111a3c400
[ 219.963864] R13: ffff8881018980d0 R14: ffff8881003e7000 R15: 0000000000000060
[ 219.971106] FS: 00007fd7e75f5740(0000) GS:ffff888237c00000(0000) knlGS:00000
00000000000
[ 219.979303] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 219.985160] CR2: 00005563500e8808 CR3: 0008000111986001 CR4: 0000000000370ef0
[ 219.992401] Call Trace:
[ 219.994955] <TASK>
[ 219.997160] ? die+0x36/0x80
[ 220.000149] ? do_trap+0xf4/0x100
[ 220.003574] ? enc_dec_message+0x4ed/0x570
[ 220.007777] ? do_error_trap+0x65/0x80
[ 220.011632] ? enc_dec_message+0x4ed/0x570
[ 220.015842] ? exc_invalid_op+0x50/0x70
[ 220.019788] ? enc_dec_message+0x4ed/0x570
[ 220.023994] ? asm_exc_invalid_op+0x1a/0x20
[ 220.028288] ? enc_dec_message+0x4ed/0x570
[ 220.032505] ? enc_dec_message+0x16f/0x570
[ 220.036711] ? srso_alias_return_thunk+0x5/0x7f
[ 220.041352] ? srso_alias_return_thunk+0x5/0x7f
[ 220.045994] handle_guest_request+0xc6/0x330
[ 220.050375] get_ext_report+0x1e0/0x3d0
[ 220.054323] sev_report_new+0x159/0x460
[ 220.058267] tsm_report_read.part.0+0x96/0x120
[ 220.062818] configfs_bin_read_iter+0xe1/0x1e0
[ 220.067377] vfs_read+0x1db/0x310
[ 220.070813] ksys_read+0x6f/0xf0
[ 220.074152] do_syscall_64+0x3f/0x90
[ 220.077843] entry_SYSCALL_64_after_hwframe+0x6e/0xd8
[ 220.083007] RIP: 0033:0x7fd7e7705fd2
[ 220.086695] Code: c0 e9 c2 fe ff ff 50 48 8d 3d aa cb 0a 00 e8 d5 1a 02 00 0f
1f 44 00 00 f3 0f 1e fa 64 8b 04 25 18 00 00 00 85 c0 75 10 0f 05 <48> 3d 00 f0
ff ff 77 56 c3 0f 1f 44 00 00 48 83 ec 28 48 89 54 24
[ 220.105572] RSP: 002b:00007fff9bc8fc18 EFLAGS: 00000246 ORIG_RAX: 00000000000
00000
[ 220.113252] RAX: ffffffffffffffda RBX: 00007fd7e77e4980 RCX: 00007fd7e7705fd2
[ 220.120496] RDX: 0000000000001000 RSI: 00005563500e7800 RDI: 0000000000000000
[ 220.127742] RBP: 00007fd7e77e14a0 R08: 0000000000000000 R09: 000000000000007c
[ 220.134988] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000010
[ 220.142232] R13: 00007fd7e77e08a0 R14: 0000000000000d68 R15: 0000000000000d68
[ 220.149479] </TASK>
[ 220.151865] ---[ end trace 0000000000000000 ]---
[ 220.156629] RIP: 0010:enc_dec_message+0x4ed/0x570
[ 220.161479] Code: c7 c0 00 00 00 80 48 2b 05 b8 4f 99 00 e9 dc fd ff ff 0f 0b
bb f4 ff ff ff eb b5 0f 0b 0f 0b 0f 0b e8 e7 47 ce ff 89 c3 eb 94 <0f> 0b 0f 0b
0f 0b 0f 0b 48 8d 7c 24 38 e8 11 6b 23 00 8b 5c 24 58
[ 220.180379] RSP: 0018:ffffc90000e27a18 EFLAGS: 00010246
[ 220.185743] RAX: 0000000000000000 RBX: ffffc90000e27bf8 RCX: 0000000000081000
[ 220.193012] RDX: 0000000000000000 RSI: 0000000000080000 RDI: ffffc90080e27bf8
[ 220.200280] RBP: ffff8881018980a0 R08: 0000000000000000 R09: ffffc90000e27a78
[ 220.207551] R10: 0000000000026680 R11: 0000000000000008 R12: ffff888111a3c400
[ 220.214822] R13: ffff8881018980d0 R14: ffff8881003e7000 R15: 0000000000000060
[ 220.222094] FS: 00007fd7e75f5740(0000) GS:ffff888237c00000(0000) knlGS:00000
00000000000
[ 220.230329] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 220.236210] CR2: 00005563500e8808 CR3: 0008000111986001 CR4: 0000000000370ef0
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-27 16:14 ` Peter Gonda
@ 2023-09-27 16:53 ` Dan Williams
2023-09-28 22:49 ` Dan Williams
1 sibling, 0 replies; 29+ messages in thread
From: Dan Williams @ 2023-09-27 16:53 UTC (permalink / raw)
To: Peter Gonda, Dan Williams
Cc: linux-coco, Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
linux-kernel, x86, dave.hansen
Peter Gonda wrote:
> On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >
> > In TDX guest, the attestation process is used to verify the TDX guest
> > trustworthiness to other entities before provisioning secrets to the
> > guest. The first step in the attestation process is TDREPORT
> > generation, which involves getting the guest measurement data in the
> > format of TDREPORT, which is further used to validate the authenticity
> > of the TDX guest. TDREPORT by design is integrity-protected and can
> > only be verified on the local machine.
> >
> > To support remote verification of the TDREPORT in a SGX-based
> > attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> > (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > only run outside of the TDX guest (i.e. in a host process or in a
> > normal VM) and guest can use communication channels like vsock or
> > TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > TDX guest may not support these communication channels. To handle such
> > cases, TDX defines a GetQuote hypercall which can be used by the guest
> > to request the host VMM to communicate with the SGX QE. More details
> > about GetQuote hypercall can be found in TDX Guest-Host Communication
> > Interface (GHCI) for Intel TDX 1.0, section titled
> > "TDG.VP.VMCALL<GetQuote>".
> >
> > Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > Computing Guest platforms to get the measurement data via ConfigFS.
> > Extend the TSM framework and add support to allow an attestation agent
> > to get the TDX Quote data (included usage example below).
> >
> > report=/sys/kernel/config/tsm/report/report0
> > mkdir $report
> > dd if=/dev/urandom bs=64 count=1 > $report/inblob
> > hexdump -C $report/outblob
> > rmdir $report
> >
> > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > with TDREPORT data as input, which is further used by the VMM to copy
> > the TD Quote result after successful Quote generation. To create the
> > shared buffer, allocate a large enough memory and mark it shared using
> > set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > for GetQuote requests in the TDX TSM handler.
> >
> > Although this method reserves a fixed chunk of memory for GetQuote
> > requests, such one time allocation can help avoid memory fragmentation
> > related allocation failures later in the uptime of the guest.
> >
> > Since the Quote generation process is not time-critical or frequently
> > used, the current version uses a polling model for Quote requests and
> > it also does not support parallel GetQuote requests.
> >
> > Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Hey Dan,
>
> I tried running your test commands on an SNP enabled guest. To build
> the kernel I just checked out linus/master and applied your series. I
> haven't done any debugging yet, so I will update with what I find.
Ok, not surprised just because I made major changes to the sev-guest interface
and did not have a way to test.
> root@Ubuntu2004:~# hexdump -C $report/outblob
> [ 219.871875] ------------[ cut here ]------------
> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> [ 219.882280] invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
> [ 219.887628] CPU: 0 PID: 1317 Comm: hexdump Not tainted 6.6.0-rc3-xfstests-000
> 44-gf636850ddfc7 #1
> [ 219.896530] Hardware name: Google Google Compute Engine/Google Compute Engine
Might you be able to send me info offlist about how I can spin this up so I can
test without waiting for the community to try it out?
In any event, thanks for testing! Appreciate it.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports
2023-09-27 14:38 ` Peter Gonda
@ 2023-09-27 19:05 ` Thomas Fossati
0 siblings, 0 replies; 29+ messages in thread
From: Thomas Fossati @ 2023-09-27 19:05 UTC (permalink / raw)
To: Peter Gonda
Cc: Dan Williams, linux-coco, Dionna Amalie Glaze, James Bottomley,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
linux-kernel, x86, dave.hansen
Hi Peter,
On Wed, 27 Sept 2023 at 16:38, Peter Gonda <pgonda@google.com> wrote:
>
> On Wed, Sep 27, 2023 at 2:25 AM Thomas Fossati
> <thomas.fossati@linaro.org> wrote:
> >
> > On Wed, 27 Sept 2023 at 10:21, Dan Williams <dan.j.williams@intel.com> wrote:
> > > It can be expanded when/if those platforms expand the
> > > size of the supported user data, or another configfs-tsm backend arrives
> > > that needs that capability.
> >
> > Makes sense, thanks.
>
> I'm not familiar with the rats eat spec but I would assume the
> protocol would acquire more than just the nonce in the inblob.
> Probably some combination of claims, nonce, and information about a
> public key?
Looking at existing EAT-based (or EAT-like) serialisations:
Arm CCA has a single, 64 bytes challenge (see §A7 of “Realm Management
Monitor (RMM) Specification” [1].)
CoVE too, see [2].
Nitro instead is doing something different: GetAttestationDoc() has
optional user-provided public key, custom user data, and a custom
nonce passed in as separate input arguments [3].
So, what @inblob's structure looks like really is a choice of the
attester's vendor.
> Does the specification allow for the data needing to be
> signed by the TSM to be hashed first?
EAT per se is mostly agnostic, it has a flexible and extensible type
system, which can adapt to most attester “shapes”.
Hope this answers your questions.
cheers, t
[1] https://developer.arm.com/documentation/den0137/latest
[2] https://github.com/riscv-non-isa/riscv-ap-tee/blob/main/specification/attestation.adoc#tvm-challenge-claim
[3] https://github.com/aws/aws-nitro-enclaves-nsm-api/blob/4b851f3006c6fa98f23dcffb2cba03b39de9b8af/nsm-lib/src/lib.rs#L218
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-27 16:14 ` Peter Gonda
2023-09-27 16:53 ` Dan Williams
@ 2023-09-28 22:49 ` Dan Williams
2023-09-29 17:26 ` Peter Gonda
1 sibling, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-09-28 22:49 UTC (permalink / raw)
To: Peter Gonda, Dan Williams
Cc: linux-coco, Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
linux-kernel, x86, dave.hansen
Peter Gonda wrote:
> On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >
> > In TDX guest, the attestation process is used to verify the TDX guest
> > trustworthiness to other entities before provisioning secrets to the
> > guest. The first step in the attestation process is TDREPORT
> > generation, which involves getting the guest measurement data in the
> > format of TDREPORT, which is further used to validate the authenticity
> > of the TDX guest. TDREPORT by design is integrity-protected and can
> > only be verified on the local machine.
> >
> > To support remote verification of the TDREPORT in a SGX-based
> > attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> > (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > only run outside of the TDX guest (i.e. in a host process or in a
> > normal VM) and guest can use communication channels like vsock or
> > TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > TDX guest may not support these communication channels. To handle such
> > cases, TDX defines a GetQuote hypercall which can be used by the guest
> > to request the host VMM to communicate with the SGX QE. More details
> > about GetQuote hypercall can be found in TDX Guest-Host Communication
> > Interface (GHCI) for Intel TDX 1.0, section titled
> > "TDG.VP.VMCALL<GetQuote>".
> >
> > Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > Computing Guest platforms to get the measurement data via ConfigFS.
> > Extend the TSM framework and add support to allow an attestation agent
> > to get the TDX Quote data (included usage example below).
> >
> > report=/sys/kernel/config/tsm/report/report0
> > mkdir $report
> > dd if=/dev/urandom bs=64 count=1 > $report/inblob
> > hexdump -C $report/outblob
> > rmdir $report
> >
> > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > with TDREPORT data as input, which is further used by the VMM to copy
> > the TD Quote result after successful Quote generation. To create the
> > shared buffer, allocate a large enough memory and mark it shared using
> > set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > for GetQuote requests in the TDX TSM handler.
> >
> > Although this method reserves a fixed chunk of memory for GetQuote
> > requests, such one time allocation can help avoid memory fragmentation
> > related allocation failures later in the uptime of the guest.
> >
> > Since the Quote generation process is not time-critical or frequently
> > used, the current version uses a polling model for Quote requests and
> > it also does not support parallel GetQuote requests.
> >
> > Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Hey Dan,
>
> I tried running your test commands on an SNP enabled guest. To build
> the kernel I just checked out linus/master and applied your series. I
> haven't done any debugging yet, so I will update with what I find.
>
> root@Ubuntu2004:~# hexdump -C $report/outblob
> [ 219.871875] ------------[ cut here ]------------
> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
Ok, it does not like virtual address of one of the buffers, but my
changes "should" not have affected that as get_ext_report() internally
uses snp_dev->certs_data and snp_dev->response for bounce buffering the
actual request / response memory. First test I want to try once I can
get on an SNP system is compare this to the ioctl path just make sure
that succeeds.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-28 22:49 ` Dan Williams
@ 2023-09-29 17:26 ` Peter Gonda
2023-10-03 18:37 ` Peter Gonda
0 siblings, 1 reply; 29+ messages in thread
From: Peter Gonda @ 2023-09-29 17:26 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
linux-kernel, x86, dave.hansen
On Thu, Sep 28, 2023 at 4:49 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Peter Gonda wrote:
> > On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > >
> > > In TDX guest, the attestation process is used to verify the TDX guest
> > > trustworthiness to other entities before provisioning secrets to the
> > > guest. The first step in the attestation process is TDREPORT
> > > generation, which involves getting the guest measurement data in the
> > > format of TDREPORT, which is further used to validate the authenticity
> > > of the TDX guest. TDREPORT by design is integrity-protected and can
> > > only be verified on the local machine.
> > >
> > > To support remote verification of the TDREPORT in a SGX-based
> > > attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> > > (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > > only run outside of the TDX guest (i.e. in a host process or in a
> > > normal VM) and guest can use communication channels like vsock or
> > > TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > > TDX guest may not support these communication channels. To handle such
> > > cases, TDX defines a GetQuote hypercall which can be used by the guest
> > > to request the host VMM to communicate with the SGX QE. More details
> > > about GetQuote hypercall can be found in TDX Guest-Host Communication
> > > Interface (GHCI) for Intel TDX 1.0, section titled
> > > "TDG.VP.VMCALL<GetQuote>".
> > >
> > > Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > > Computing Guest platforms to get the measurement data via ConfigFS.
> > > Extend the TSM framework and add support to allow an attestation agent
> > > to get the TDX Quote data (included usage example below).
> > >
> > > report=/sys/kernel/config/tsm/report/report0
> > > mkdir $report
> > > dd if=/dev/urandom bs=64 count=1 > $report/inblob
> > > hexdump -C $report/outblob
> > > rmdir $report
> > >
> > > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > > with TDREPORT data as input, which is further used by the VMM to copy
> > > the TD Quote result after successful Quote generation. To create the
> > > shared buffer, allocate a large enough memory and mark it shared using
> > > set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > > for GetQuote requests in the TDX TSM handler.
> > >
> > > Although this method reserves a fixed chunk of memory for GetQuote
> > > requests, such one time allocation can help avoid memory fragmentation
> > > related allocation failures later in the uptime of the guest.
> > >
> > > Since the Quote generation process is not time-critical or frequently
> > > used, the current version uses a polling model for Quote requests and
> > > it also does not support parallel GetQuote requests.
> > >
> > > Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >
> > Hey Dan,
> >
> > I tried running your test commands on an SNP enabled guest. To build
> > the kernel I just checked out linus/master and applied your series. I
> > haven't done any debugging yet, so I will update with what I find.
> >
> > root@Ubuntu2004:~# hexdump -C $report/outblob
> > [ 219.871875] ------------[ cut here ]------------
> > [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
>
> Ok, it does not like virtual address of one of the buffers, but my
> changes "should" not have affected that as get_ext_report() internally
> uses snp_dev->certs_data and snp_dev->response for bounce buffering the
> actual request / response memory. First test I want to try once I can
> get on an SNP system is compare this to the ioctl path just make sure
> that succeeds.
I tried the IOCTL with our attestation test and it seems to be working
fine. I was hoping to debug further next week.
I also mailed you offlist to discuss getting access to SNP for testing.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-09-29 17:26 ` Peter Gonda
@ 2023-10-03 18:37 ` Peter Gonda
2023-10-03 19:29 ` Kuppuswamy Sathyanarayanan
0 siblings, 1 reply; 29+ messages in thread
From: Peter Gonda @ 2023-10-03 18:37 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
linux-kernel, x86, dave.hansen
On Fri, Sep 29, 2023 at 11:26 AM Peter Gonda <pgonda@google.com> wrote:
>
> On Thu, Sep 28, 2023 at 4:49 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > Peter Gonda wrote:
> > > On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > > >
> > > > From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > >
> > > > In TDX guest, the attestation process is used to verify the TDX guest
> > > > trustworthiness to other entities before provisioning secrets to the
> > > > guest. The first step in the attestation process is TDREPORT
> > > > generation, which involves getting the guest measurement data in the
> > > > format of TDREPORT, which is further used to validate the authenticity
> > > > of the TDX guest. TDREPORT by design is integrity-protected and can
> > > > only be verified on the local machine.
> > > >
> > > > To support remote verification of the TDREPORT in a SGX-based
> > > > attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> > > > (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > > > only run outside of the TDX guest (i.e. in a host process or in a
> > > > normal VM) and guest can use communication channels like vsock or
> > > > TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > > > TDX guest may not support these communication channels. To handle such
> > > > cases, TDX defines a GetQuote hypercall which can be used by the guest
> > > > to request the host VMM to communicate with the SGX QE. More details
> > > > about GetQuote hypercall can be found in TDX Guest-Host Communication
> > > > Interface (GHCI) for Intel TDX 1.0, section titled
> > > > "TDG.VP.VMCALL<GetQuote>".
> > > >
> > > > Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > > > Computing Guest platforms to get the measurement data via ConfigFS.
> > > > Extend the TSM framework and add support to allow an attestation agent
> > > > to get the TDX Quote data (included usage example below).
> > > >
> > > > report=/sys/kernel/config/tsm/report/report0
> > > > mkdir $report
> > > > dd if=/dev/urandom bs=64 count=1 > $report/inblob
> > > > hexdump -C $report/outblob
> > > > rmdir $report
> > > >
> > > > GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > > > with TDREPORT data as input, which is further used by the VMM to copy
> > > > the TD Quote result after successful Quote generation. To create the
> > > > shared buffer, allocate a large enough memory and mark it shared using
> > > > set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > > > for GetQuote requests in the TDX TSM handler.
> > > >
> > > > Although this method reserves a fixed chunk of memory for GetQuote
> > > > requests, such one time allocation can help avoid memory fragmentation
> > > > related allocation failures later in the uptime of the guest.
> > > >
> > > > Since the Quote generation process is not time-critical or frequently
> > > > used, the current version uses a polling model for Quote requests and
> > > > it also does not support parallel GetQuote requests.
> > > >
> > > > Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > > > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > > > Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> > > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > >
> > > Hey Dan,
> > >
> > > I tried running your test commands on an SNP enabled guest. To build
> > > the kernel I just checked out linus/master and applied your series. I
> > > haven't done any debugging yet, so I will update with what I find.
> > >
> > > root@Ubuntu2004:~# hexdump -C $report/outblob
> > > [ 219.871875] ------------[ cut here ]------------
> > > [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> >
> > Ok, it does not like virtual address of one of the buffers, but my
> > changes "should" not have affected that as get_ext_report() internally
> > uses snp_dev->certs_data and snp_dev->response for bounce buffering the
> > actual request / response memory. First test I want to try once I can
> > get on an SNP system is compare this to the ioctl path just make sure
> > that succeeds.
>
I think there may be an issue with CONFIG_DEBUG_SG. That was the
warning we were getting in my above stack trace:
> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
This was for this line in enc_dec_message():
sg_set_buf(&src[1], src_buf, hdr->msg_sz);
I am not sure why in sg_set_buf() virt_addr_valid() returns false for
the address given in the sev_report_new() which is from the variable
'ext_req' which is stack allocated?
static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
unsigned int buflen)
{
#ifdef CONFIG_DEBUG_SG
BUG_ON(!virt_addr_valid(buf));
#endif
sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
}
When I disable CONFIG_DEBUG_SG in my config. Your patch seems to work,
well at least it doesn't crash the guest. I haven't checked if the
report is valid yet.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-10-03 18:37 ` Peter Gonda
@ 2023-10-03 19:29 ` Kuppuswamy Sathyanarayanan
2023-10-03 20:06 ` Peter Gonda
0 siblings, 1 reply; 29+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-10-03 19:29 UTC (permalink / raw)
To: Peter Gonda, Dan Williams
Cc: linux-coco, Erdem Aktas, peterz, linux-kernel, x86, dave.hansen
Hi,
On 10/3/2023 11:37 AM, Peter Gonda wrote:
> On Fri, Sep 29, 2023 at 11:26 AM Peter Gonda <pgonda@google.com> wrote:
>>
>> On Thu, Sep 28, 2023 at 4:49 PM Dan Williams <dan.j.williams@intel.com> wrote:
>>>
>>> Peter Gonda wrote:
>>>> On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
>>>>>
>>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>>
>>>>> In TDX guest, the attestation process is used to verify the TDX guest
>>>>> trustworthiness to other entities before provisioning secrets to the
>>>>> guest. The first step in the attestation process is TDREPORT
>>>>> generation, which involves getting the guest measurement data in the
>>>>> format of TDREPORT, which is further used to validate the authenticity
>>>>> of the TDX guest. TDREPORT by design is integrity-protected and can
>>>>> only be verified on the local machine.
>>>>>
>>>>> To support remote verification of the TDREPORT in a SGX-based
>>>>> attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
>>>>> (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
>>>>> only run outside of the TDX guest (i.e. in a host process or in a
>>>>> normal VM) and guest can use communication channels like vsock or
>>>>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
>>>>> TDX guest may not support these communication channels. To handle such
>>>>> cases, TDX defines a GetQuote hypercall which can be used by the guest
>>>>> to request the host VMM to communicate with the SGX QE. More details
>>>>> about GetQuote hypercall can be found in TDX Guest-Host Communication
>>>>> Interface (GHCI) for Intel TDX 1.0, section titled
>>>>> "TDG.VP.VMCALL<GetQuote>".
>>>>>
>>>>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
>>>>> Computing Guest platforms to get the measurement data via ConfigFS.
>>>>> Extend the TSM framework and add support to allow an attestation agent
>>>>> to get the TDX Quote data (included usage example below).
>>>>>
>>>>> report=/sys/kernel/config/tsm/report/report0
>>>>> mkdir $report
>>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>>> hexdump -C $report/outblob
>>>>> rmdir $report
>>>>>
>>>>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
>>>>> with TDREPORT data as input, which is further used by the VMM to copy
>>>>> the TD Quote result after successful Quote generation. To create the
>>>>> shared buffer, allocate a large enough memory and mark it shared using
>>>>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
>>>>> for GetQuote requests in the TDX TSM handler.
>>>>>
>>>>> Although this method reserves a fixed chunk of memory for GetQuote
>>>>> requests, such one time allocation can help avoid memory fragmentation
>>>>> related allocation failures later in the uptime of the guest.
>>>>>
>>>>> Since the Quote generation process is not time-critical or frequently
>>>>> used, the current version uses a polling model for Quote requests and
>>>>> it also does not support parallel GetQuote requests.
>>>>>
>>>>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
>>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>>>>> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
>>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>>>>
>>>> Hey Dan,
>>>>
>>>> I tried running your test commands on an SNP enabled guest. To build
>>>> the kernel I just checked out linus/master and applied your series. I
>>>> haven't done any debugging yet, so I will update with what I find.
>>>>
>>>> root@Ubuntu2004:~# hexdump -C $report/outblob
>>>> [ 219.871875] ------------[ cut here ]------------
>>>> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
>>>
>>> Ok, it does not like virtual address of one of the buffers, but my
>>> changes "should" not have affected that as get_ext_report() internally
>>> uses snp_dev->certs_data and snp_dev->response for bounce buffering the
>>> actual request / response memory. First test I want to try once I can
>>> get on an SNP system is compare this to the ioctl path just make sure
>>> that succeeds.
>>
>
> I think there may be an issue with CONFIG_DEBUG_SG. That was the
> warning we were getting in my above stack trace:
>
>> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
>
> This was for this line in enc_dec_message():
>
> sg_set_buf(&src[1], src_buf, hdr->msg_sz);
>
> I am not sure why in sg_set_buf() virt_addr_valid() returns false for
> the address given in the sev_report_new() which is from the variable
> 'ext_req' which is stack allocated?
>
> static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
> unsigned int buflen)
> {
> #ifdef CONFIG_DEBUG_SG
> BUG_ON(!virt_addr_valid(buf));
> #endif
> sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> }
>
> When I disable CONFIG_DEBUG_SG in my config. Your patch seems to work,
> well at least it doesn't crash the guest. I haven't checked if the
> report is valid yet.
>
Dan, do you think it is related to not allocating direct mapped memory (using
kvalloc)?
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-10-03 19:29 ` Kuppuswamy Sathyanarayanan
@ 2023-10-03 20:06 ` Peter Gonda
2023-10-04 0:54 ` Dan Williams
0 siblings, 1 reply; 29+ messages in thread
From: Peter Gonda @ 2023-10-03 20:06 UTC (permalink / raw)
To: Kuppuswamy Sathyanarayanan
Cc: Dan Williams, linux-coco, Erdem Aktas, peterz, linux-kernel, x86,
dave.hansen
On Tue, Oct 3, 2023 at 1:29 PM Kuppuswamy Sathyanarayanan
<sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
>
> Hi,
>
> On 10/3/2023 11:37 AM, Peter Gonda wrote:
> > On Fri, Sep 29, 2023 at 11:26 AM Peter Gonda <pgonda@google.com> wrote:
> >>
> >> On Thu, Sep 28, 2023 at 4:49 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >>>
> >>> Peter Gonda wrote:
> >>>> On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> >>>>>
> >>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>>>
> >>>>> In TDX guest, the attestation process is used to verify the TDX guest
> >>>>> trustworthiness to other entities before provisioning secrets to the
> >>>>> guest. The first step in the attestation process is TDREPORT
> >>>>> generation, which involves getting the guest measurement data in the
> >>>>> format of TDREPORT, which is further used to validate the authenticity
> >>>>> of the TDX guest. TDREPORT by design is integrity-protected and can
> >>>>> only be verified on the local machine.
> >>>>>
> >>>>> To support remote verification of the TDREPORT in a SGX-based
> >>>>> attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> >>>>> (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> >>>>> only run outside of the TDX guest (i.e. in a host process or in a
> >>>>> normal VM) and guest can use communication channels like vsock or
> >>>>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> >>>>> TDX guest may not support these communication channels. To handle such
> >>>>> cases, TDX defines a GetQuote hypercall which can be used by the guest
> >>>>> to request the host VMM to communicate with the SGX QE. More details
> >>>>> about GetQuote hypercall can be found in TDX Guest-Host Communication
> >>>>> Interface (GHCI) for Intel TDX 1.0, section titled
> >>>>> "TDG.VP.VMCALL<GetQuote>".
> >>>>>
> >>>>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> >>>>> Computing Guest platforms to get the measurement data via ConfigFS.
> >>>>> Extend the TSM framework and add support to allow an attestation agent
> >>>>> to get the TDX Quote data (included usage example below).
> >>>>>
> >>>>> report=/sys/kernel/config/tsm/report/report0
> >>>>> mkdir $report
> >>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >>>>> hexdump -C $report/outblob
> >>>>> rmdir $report
> >>>>>
> >>>>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> >>>>> with TDREPORT data as input, which is further used by the VMM to copy
> >>>>> the TD Quote result after successful Quote generation. To create the
> >>>>> shared buffer, allocate a large enough memory and mark it shared using
> >>>>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> >>>>> for GetQuote requests in the TDX TSM handler.
> >>>>>
> >>>>> Although this method reserves a fixed chunk of memory for GetQuote
> >>>>> requests, such one time allocation can help avoid memory fragmentation
> >>>>> related allocation failures later in the uptime of the guest.
> >>>>>
> >>>>> Since the Quote generation process is not time-critical or frequently
> >>>>> used, the current version uses a polling model for Quote requests and
> >>>>> it also does not support parallel GetQuote requests.
> >>>>>
> >>>>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> >>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> >>>>> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> >>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> >>>>
> >>>> Hey Dan,
> >>>>
> >>>> I tried running your test commands on an SNP enabled guest. To build
> >>>> the kernel I just checked out linus/master and applied your series. I
> >>>> haven't done any debugging yet, so I will update with what I find.
> >>>>
> >>>> root@Ubuntu2004:~# hexdump -C $report/outblob
> >>>> [ 219.871875] ------------[ cut here ]------------
> >>>> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> >>>
> >>> Ok, it does not like virtual address of one of the buffers, but my
> >>> changes "should" not have affected that as get_ext_report() internally
> >>> uses snp_dev->certs_data and snp_dev->response for bounce buffering the
> >>> actual request / response memory. First test I want to try once I can
> >>> get on an SNP system is compare this to the ioctl path just make sure
> >>> that succeeds.
> >>
> >
> > I think there may be an issue with CONFIG_DEBUG_SG. That was the
> > warning we were getting in my above stack trace:
> >
> >> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> >
> > This was for this line in enc_dec_message():
> >
> > sg_set_buf(&src[1], src_buf, hdr->msg_sz);
> >
> > I am not sure why in sg_set_buf() virt_addr_valid() returns false for
> > the address given in the sev_report_new() which is from the variable
> > 'ext_req' which is stack allocated?
> >
> > static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
> > unsigned int buflen)
> > {
> > #ifdef CONFIG_DEBUG_SG
> > BUG_ON(!virt_addr_valid(buf));
> > #endif
> > sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> > }
> >
> > When I disable CONFIG_DEBUG_SG in my config. Your patch seems to work,
> > well at least it doesn't crash the guest. I haven't checked if the
> > report is valid yet.
> >
>
> Dan, do you think it is related to not allocating direct mapped memory (using
> kvalloc)?
But I think the issue is the stack allocated variable 'ext_req' here:
sev_report_new()
+ void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ guard(mutex)(&snp_cmd_mutex);
+ certs_address = buf + report_size;
+ struct snp_ext_report_req ext_req = {
+ .data = { .vmpl = desc->privlevel },
+ .certs_address = (__u64)certs_address,
+ .certs_len = ext_size,
+ };
+ memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
>
> --
> Sathyanarayanan Kuppuswamy
> Linux Kernel Developer
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-10-03 20:06 ` Peter Gonda
@ 2023-10-04 0:54 ` Dan Williams
2023-10-10 19:36 ` Dan Williams
0 siblings, 1 reply; 29+ messages in thread
From: Dan Williams @ 2023-10-04 0:54 UTC (permalink / raw)
To: Peter Gonda, Kuppuswamy Sathyanarayanan
Cc: Dan Williams, linux-coco, Erdem Aktas, peterz, linux-kernel, x86,
dave.hansen
Peter Gonda wrote:
> On Tue, Oct 3, 2023 at 1:29 PM Kuppuswamy Sathyanarayanan
> <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
> >
> > Hi,
> >
> > On 10/3/2023 11:37 AM, Peter Gonda wrote:
> > > On Fri, Sep 29, 2023 at 11:26 AM Peter Gonda <pgonda@google.com> wrote:
> > >>
> > >> On Thu, Sep 28, 2023 at 4:49 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > >>>
> > >>> Peter Gonda wrote:
> > >>>> On Mon, Sep 25, 2023 at 10:17 PM Dan Williams <dan.j.williams@intel.com> wrote:
> > >>>>>
> > >>>>> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > >>>>>
> > >>>>> In TDX guest, the attestation process is used to verify the TDX guest
> > >>>>> trustworthiness to other entities before provisioning secrets to the
> > >>>>> guest. The first step in the attestation process is TDREPORT
> > >>>>> generation, which involves getting the guest measurement data in the
> > >>>>> format of TDREPORT, which is further used to validate the authenticity
> > >>>>> of the TDX guest. TDREPORT by design is integrity-protected and can
> > >>>>> only be verified on the local machine.
> > >>>>>
> > >>>>> To support remote verification of the TDREPORT in a SGX-based
> > >>>>> attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> > >>>>> (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> > >>>>> only run outside of the TDX guest (i.e. in a host process or in a
> > >>>>> normal VM) and guest can use communication channels like vsock or
> > >>>>> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> > >>>>> TDX guest may not support these communication channels. To handle such
> > >>>>> cases, TDX defines a GetQuote hypercall which can be used by the guest
> > >>>>> to request the host VMM to communicate with the SGX QE. More details
> > >>>>> about GetQuote hypercall can be found in TDX Guest-Host Communication
> > >>>>> Interface (GHCI) for Intel TDX 1.0, section titled
> > >>>>> "TDG.VP.VMCALL<GetQuote>".
> > >>>>>
> > >>>>> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> > >>>>> Computing Guest platforms to get the measurement data via ConfigFS.
> > >>>>> Extend the TSM framework and add support to allow an attestation agent
> > >>>>> to get the TDX Quote data (included usage example below).
> > >>>>>
> > >>>>> report=/sys/kernel/config/tsm/report/report0
> > >>>>> mkdir $report
> > >>>>> dd if=/dev/urandom bs=64 count=1 > $report/inblob
> > >>>>> hexdump -C $report/outblob
> > >>>>> rmdir $report
> > >>>>>
> > >>>>> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> > >>>>> with TDREPORT data as input, which is further used by the VMM to copy
> > >>>>> the TD Quote result after successful Quote generation. To create the
> > >>>>> shared buffer, allocate a large enough memory and mark it shared using
> > >>>>> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> > >>>>> for GetQuote requests in the TDX TSM handler.
> > >>>>>
> > >>>>> Although this method reserves a fixed chunk of memory for GetQuote
> > >>>>> requests, such one time allocation can help avoid memory fragmentation
> > >>>>> related allocation failures later in the uptime of the guest.
> > >>>>>
> > >>>>> Since the Quote generation process is not time-critical or frequently
> > >>>>> used, the current version uses a polling model for Quote requests and
> > >>>>> it also does not support parallel GetQuote requests.
> > >>>>>
> > >>>>> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> > >>>>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> > >>>>> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> > >>>>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > >>>>
> > >>>> Hey Dan,
> > >>>>
> > >>>> I tried running your test commands on an SNP enabled guest. To build
> > >>>> the kernel I just checked out linus/master and applied your series. I
> > >>>> haven't done any debugging yet, so I will update with what I find.
> > >>>>
> > >>>> root@Ubuntu2004:~# hexdump -C $report/outblob
> > >>>> [ 219.871875] ------------[ cut here ]------------
> > >>>> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> > >>>
> > >>> Ok, it does not like virtual address of one of the buffers, but my
> > >>> changes "should" not have affected that as get_ext_report() internally
> > >>> uses snp_dev->certs_data and snp_dev->response for bounce buffering the
> > >>> actual request / response memory. First test I want to try once I can
> > >>> get on an SNP system is compare this to the ioctl path just make sure
> > >>> that succeeds.
> > >>
> > >
> > > I think there may be an issue with CONFIG_DEBUG_SG. That was the
> > > warning we were getting in my above stack trace:
> > >
> > >> [ 219.876642] kernel BUG at include/linux/scatterlist.h:187!
> > >
> > > This was for this line in enc_dec_message():
> > >
> > > sg_set_buf(&src[1], src_buf, hdr->msg_sz);
> > >
> > > I am not sure why in sg_set_buf() virt_addr_valid() returns false for
> > > the address given in the sev_report_new() which is from the variable
> > > 'ext_req' which is stack allocated?
> > >
> > > static inline void sg_set_buf(struct scatterlist *sg, const void *buf,
> > > unsigned int buflen)
> > > {
> > > #ifdef CONFIG_DEBUG_SG
> > > BUG_ON(!virt_addr_valid(buf));
> > > #endif
> > > sg_set_page(sg, virt_to_page(buf), buflen, offset_in_page(buf));
> > > }
> > >
> > > When I disable CONFIG_DEBUG_SG in my config. Your patch seems to work,
> > > well at least it doesn't crash the guest. I haven't checked if the
> > > report is valid yet.
> > >
> >
> > Dan, do you think it is related to not allocating direct mapped memory (using
> > kvalloc)?
>
> But I think the issue is the stack allocated variable 'ext_req' here:
>
> sev_report_new()
> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + guard(mutex)(&snp_cmd_mutex);
> + certs_address = buf + report_size;
> + struct snp_ext_report_req ext_req = {
> + .data = { .vmpl = desc->privlevel },
> + .certs_address = (__u64)certs_address,
> + .certs_len = ext_size,
> + };
> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
If the failure is coming from:
sg_set_buf(&src[1], src_buf, hdr->msg_sz);
...then that is always coming from the stack as get_ext_report()
internally copies either from the user ioctl() address or the kernel
stack into the local stack copy in both cases:
get_ext_report(...)
...
struct snp_ext_report_req req;
...
if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
return -EFAULT;
...
ret = handle_guest_request(..., &req.data, ...);
...where that "&req.data" always becomes the @src_buf argument to
enc_dec_message(). So while I do understand why sg_set_buf() is
complaining, I don't understand why it is not *always* complaining,
regardless of configfs-tsm or ioctl() with CONFIG_DEBUG_SG=y builds.
I will be able to dig deeper once I can test on hardware, but I am
thinking that the entire scheme to pass the source buffer on the kernel
stack is broken and is only happening to work because there are no
crypto-accelerators attached that require that the virtual addresses be
virt_addr_valid() for a later dma_map_sg() event.
...or my eyes are overlooking how the ioctl() path is succeeding.
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT
2023-09-26 4:17 ` [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
@ 2023-10-04 8:22 ` Dan Carpenter
0 siblings, 0 replies; 29+ messages in thread
From: Dan Carpenter @ 2023-10-04 8:22 UTC (permalink / raw)
To: oe-kbuild, Dan Williams, linux-coco
Cc: lkp, oe-kbuild-all, Borislav Petkov, Tom Lendacky, Dionna Glaze,
Brijesh Singh, Jeremi Piotrowski, peterz, linux-kernel, x86,
dave.hansen
Hi Dan,
kernel test robot noticed the following build warnings:
url: https://github.com/intel-lab-lkp/linux/commits/Dan-Williams/virt-coco-Add-a-coco-Makefile-and-coco-Kconfig/20230926-121843
base: 6465e260f48790807eef06b583b38ca9789b6072
patch link: https://lore.kernel.org/r/169570184829.596431.15991881056638719011.stgit%40dwillia2-xfh.jf.intel.com
patch subject: [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT
config: x86_64-randconfig-161-20231002 (https://download.01.org/0day-ci/archive/20231003/202310030341.zaOu0ew0-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce: (https://download.01.org/0day-ci/archive/20231003/202310030341.zaOu0ew0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202310030341.zaOu0ew0-lkp@intel.com/
smatch warnings:
drivers/virt/coco/sev-guest/sev-guest.c:853 sev_report_new() error: uninitialized symbol 'certs_size'.
vim +/certs_size +853 drivers/virt/coco/sev-guest/sev-guest.c
80013405d5b7c6 Dan Williams 2023-09-25 778 static int sev_report_new(struct tsm_report *report, void *data)
80013405d5b7c6 Dan Williams 2023-09-25 779 {
80013405d5b7c6 Dan Williams 2023-09-25 780 static const struct snp_msg_cert_entry zero_ent = { 0 };
80013405d5b7c6 Dan Williams 2023-09-25 781 struct tsm_desc *desc = &report->desc;
80013405d5b7c6 Dan Williams 2023-09-25 782 struct snp_guest_dev *snp_dev = data;
80013405d5b7c6 Dan Williams 2023-09-25 783 struct snp_msg_report_resp_hdr hdr;
80013405d5b7c6 Dan Williams 2023-09-25 784 const int report_size = SZ_4K;
80013405d5b7c6 Dan Williams 2023-09-25 785 const int ext_size = SZ_16K;
80013405d5b7c6 Dan Williams 2023-09-25 786 int ret, size = report_size + ext_size;
80013405d5b7c6 Dan Williams 2023-09-25 787 int certs_size, cert_count, i, offset;
80013405d5b7c6 Dan Williams 2023-09-25 788 u8 *certs_address;
80013405d5b7c6 Dan Williams 2023-09-25 789
80013405d5b7c6 Dan Williams 2023-09-25 790 if (desc->inblob_len != 64)
80013405d5b7c6 Dan Williams 2023-09-25 791 return -EINVAL;
80013405d5b7c6 Dan Williams 2023-09-25 792
80013405d5b7c6 Dan Williams 2023-09-25 793 void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
80013405d5b7c6 Dan Williams 2023-09-25 794 if (!buf)
80013405d5b7c6 Dan Williams 2023-09-25 795 return -ENOMEM;
80013405d5b7c6 Dan Williams 2023-09-25 796
80013405d5b7c6 Dan Williams 2023-09-25 797 guard(mutex)(&snp_cmd_mutex);
Hoho. Need guard stuff and no warnings generated. Perhaps I have added
all the new unlock functions. :)
80013405d5b7c6 Dan Williams 2023-09-25 798 certs_address = buf + report_size;
80013405d5b7c6 Dan Williams 2023-09-25 799 struct snp_ext_report_req ext_req = {
80013405d5b7c6 Dan Williams 2023-09-25 800 .data = { .vmpl = desc->privlevel },
80013405d5b7c6 Dan Williams 2023-09-25 801 .certs_address = (__u64)certs_address,
80013405d5b7c6 Dan Williams 2023-09-25 802 .certs_len = ext_size,
80013405d5b7c6 Dan Williams 2023-09-25 803 };
80013405d5b7c6 Dan Williams 2023-09-25 804 memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
80013405d5b7c6 Dan Williams 2023-09-25 805
80013405d5b7c6 Dan Williams 2023-09-25 806 struct snp_guest_request_ioctl input = {
80013405d5b7c6 Dan Williams 2023-09-25 807 .msg_version = 1,
80013405d5b7c6 Dan Williams 2023-09-25 808 .req_data = (__u64)&ext_req,
80013405d5b7c6 Dan Williams 2023-09-25 809 .resp_data = (__u64)buf,
80013405d5b7c6 Dan Williams 2023-09-25 810 };
80013405d5b7c6 Dan Williams 2023-09-25 811 struct snp_req_resp io = {
80013405d5b7c6 Dan Williams 2023-09-25 812 .req_data = KERNEL_SOCKPTR(&ext_req),
80013405d5b7c6 Dan Williams 2023-09-25 813 .resp_data = KERNEL_SOCKPTR(buf),
80013405d5b7c6 Dan Williams 2023-09-25 814 };
80013405d5b7c6 Dan Williams 2023-09-25 815
80013405d5b7c6 Dan Williams 2023-09-25 816 ret = get_ext_report(snp_dev, &input, &io);
80013405d5b7c6 Dan Williams 2023-09-25 817
80013405d5b7c6 Dan Williams 2023-09-25 818 if (ret)
80013405d5b7c6 Dan Williams 2023-09-25 819 return ret;
80013405d5b7c6 Dan Williams 2023-09-25 820
80013405d5b7c6 Dan Williams 2023-09-25 821 memcpy(&hdr, buf, sizeof(hdr));
80013405d5b7c6 Dan Williams 2023-09-25 822 if (hdr.status == SNP_REPORT_INVALID_PARAM)
80013405d5b7c6 Dan Williams 2023-09-25 823 return -EINVAL;
80013405d5b7c6 Dan Williams 2023-09-25 824 if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
80013405d5b7c6 Dan Williams 2023-09-25 825 return -EINVAL;
80013405d5b7c6 Dan Williams 2023-09-25 826 if (hdr.status)
80013405d5b7c6 Dan Williams 2023-09-25 827 return -ENXIO;
80013405d5b7c6 Dan Williams 2023-09-25 828 if ((hdr.report_size + sizeof(hdr)) > report_size)
80013405d5b7c6 Dan Williams 2023-09-25 829 return -ENOMEM;
80013405d5b7c6 Dan Williams 2023-09-25 830
80013405d5b7c6 Dan Williams 2023-09-25 831 void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
80013405d5b7c6 Dan Williams 2023-09-25 832 if (!rbuf)
80013405d5b7c6 Dan Williams 2023-09-25 833 return -ENOMEM;
80013405d5b7c6 Dan Williams 2023-09-25 834
80013405d5b7c6 Dan Williams 2023-09-25 835 memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
80013405d5b7c6 Dan Williams 2023-09-25 836 report->outblob = no_free_ptr(rbuf);
80013405d5b7c6 Dan Williams 2023-09-25 837 report->outblob_len = hdr.report_size;
80013405d5b7c6 Dan Williams 2023-09-25 838
80013405d5b7c6 Dan Williams 2023-09-25 839 for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
80013405d5b7c6 Dan Williams 2023-09-25 840 struct snp_msg_cert_entry *certs = buf + report_size;
80013405d5b7c6 Dan Williams 2023-09-25 841
80013405d5b7c6 Dan Williams 2023-09-25 842 if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
80013405d5b7c6 Dan Williams 2023-09-25 843 break;
80013405d5b7c6 Dan Williams 2023-09-25 844 certs_size += certs[i].length;
certs_size needs to be initialized to zero.
80013405d5b7c6 Dan Williams 2023-09-25 845 }
80013405d5b7c6 Dan Williams 2023-09-25 846 cert_count = i;
80013405d5b7c6 Dan Williams 2023-09-25 847
80013405d5b7c6 Dan Williams 2023-09-25 848 /* No certs to report */
80013405d5b7c6 Dan Williams 2023-09-25 849 if (cert_count == 0)
80013405d5b7c6 Dan Williams 2023-09-25 850 return 0;
80013405d5b7c6 Dan Williams 2023-09-25 851
80013405d5b7c6 Dan Williams 2023-09-25 852 /* sanity check that the entire certs table with metadata fits */
80013405d5b7c6 Dan Williams 2023-09-25 @853 if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
80013405d5b7c6 Dan Williams 2023-09-25 854 return -ENXIO;
80013405d5b7c6 Dan Williams 2023-09-25 855
80013405d5b7c6 Dan Williams 2023-09-25 856 void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
80013405d5b7c6 Dan Williams 2023-09-25 857 if (!cbuf)
80013405d5b7c6 Dan Williams 2023-09-25 858 return -ENOMEM;
80013405d5b7c6 Dan Williams 2023-09-25 859
80013405d5b7c6 Dan Williams 2023-09-25 860 /* Concatenate returned certs */
80013405d5b7c6 Dan Williams 2023-09-25 861 for (i = 0, offset = 0; i < cert_count; i++) {
80013405d5b7c6 Dan Williams 2023-09-25 862 struct snp_msg_cert_entry *certs = buf + report_size;
80013405d5b7c6 Dan Williams 2023-09-25 863
80013405d5b7c6 Dan Williams 2023-09-25 864 memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
80013405d5b7c6 Dan Williams 2023-09-25 865 offset += certs[i].length;
80013405d5b7c6 Dan Williams 2023-09-25 866 }
80013405d5b7c6 Dan Williams 2023-09-25 867
80013405d5b7c6 Dan Williams 2023-09-25 868 report->certs = no_free_ptr(cbuf);
80013405d5b7c6 Dan Williams 2023-09-25 869 report->certs_len = certs_size;
80013405d5b7c6 Dan Williams 2023-09-25 870
80013405d5b7c6 Dan Williams 2023-09-25 871 return 0;
80013405d5b7c6 Dan Williams 2023-09-25 872 }
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-10-04 0:54 ` Dan Williams
@ 2023-10-10 19:36 ` Dan Williams
0 siblings, 0 replies; 29+ messages in thread
From: Dan Williams @ 2023-10-10 19:36 UTC (permalink / raw)
To: Dan Williams, Peter Gonda, Kuppuswamy Sathyanarayanan
Cc: Dan Williams, linux-coco, Erdem Aktas, peterz, linux-kernel, x86,
dave.hansen
Dan Williams wrote:
> Peter Gonda wrote:
> > On Tue, Oct 3, 2023 at 1:29 PM Kuppuswamy Sathyanarayanan
> > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote:
[..]
> > > Dan, do you think it is related to not allocating direct mapped memory (using
> > > kvalloc)?
> >
> > But I think the issue is the stack allocated variable 'ext_req' here:
> >
> > sev_report_new()
> > + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + guard(mutex)(&snp_cmd_mutex);
> > + certs_address = buf + report_size;
> > + struct snp_ext_report_req ext_req = {
> > + .data = { .vmpl = desc->privlevel },
> > + .certs_address = (__u64)certs_address,
> > + .certs_len = ext_size,
> > + };
> > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
>
> If the failure is coming from:
>
> sg_set_buf(&src[1], src_buf, hdr->msg_sz);
>
> ...then that is always coming from the stack as get_ext_report()
> internally copies either from the user ioctl() address or the kernel
> stack into the local stack copy in both cases:
>
> get_ext_report(...)
> ...
> struct snp_ext_report_req req;
> ...
> if (copy_from_sockptr(&req, io->req_data, sizeof(req)))
> return -EFAULT;
> ...
> ret = handle_guest_request(..., &req.data, ...);
>
> ...where that "&req.data" always becomes the @src_buf argument to
> enc_dec_message(). So while I do understand why sg_set_buf() is
> complaining, I don't understand why it is not *always* complaining,
> regardless of configfs-tsm or ioctl() with CONFIG_DEBUG_SG=y builds.
>
> I will be able to dig deeper once I can test on hardware, but I am
> thinking that the entire scheme to pass the source buffer on the kernel
> stack is broken and is only happening to work because there are no
> crypto-accelerators attached that require that the virtual addresses be
> virt_addr_valid() for a later dma_map_sg() event.
>
> ...or my eyes are overlooking how the ioctl() path is succeeding.
Confirmed, I can make DEBUG_SG equally unhappy via the ioctl() path.
Note I changed the BUG_ON() to WARN_ON() in this kernel to keep the
system alive over reproductions:
WARNING: CPU: 0 PID: 1175 at include/linux/scatterlist.h:187 enc_dec_message+0x518/0x5b0 [sev_guest]
[..]
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
RIP: 0010:enc_dec_message+0x518/0x5b0 [sev_guest]
Call Trace:
<TASK>
[..]
handle_guest_request+0x135/0x520 [sev_guest]
get_ext_report+0x1ec/0x3e0 [sev_guest]
snp_guest_ioctl+0x157/0x200 [sev_guest]
So the required fix here will address both cases.
I will note that on this instance no certificate data is being returned,
so I can't test that path, but at least the report retrieval will be
tested for the next posting of this series.
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2023-10-10 19:36 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-26 4:16 [PATCH v4 0/6] configfs-tsm: Attestation Report ABI Dan Williams
2023-09-26 4:17 ` [PATCH v4 1/6] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
2023-09-26 4:17 ` [PATCH v4 2/6] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
2023-09-26 18:49 ` Kuppuswamy Sathyanarayanan
2023-09-26 18:59 ` Dan Williams
2023-09-27 0:43 ` Kuppuswamy Sathyanarayanan
2023-09-27 3:17 ` Dan Williams
2023-09-27 8:04 ` Thomas Fossati
2023-09-27 8:21 ` Dan Williams
2023-09-27 8:25 ` Thomas Fossati
2023-09-27 14:38 ` Peter Gonda
2023-09-27 19:05 ` Thomas Fossati
2023-09-27 8:43 ` Thomas Fossati
2023-09-27 2:10 ` Kuppuswamy Sathyanarayanan
2023-09-26 4:17 ` [PATCH v4 3/6] virt: sevguest: Prep for kernel internal {get, get_ext}_report() Dan Williams
2023-09-26 18:51 ` Kuppuswamy Sathyanarayanan
2023-09-26 4:17 ` [PATCH v4 4/6] mm/slab: Add __free() support for kvfree Dan Williams
2023-09-26 4:17 ` [PATCH v4 5/6] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT Dan Williams
2023-10-04 8:22 ` Dan Carpenter
2023-09-26 4:17 ` [PATCH v4 6/6] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
2023-09-27 16:14 ` Peter Gonda
2023-09-27 16:53 ` Dan Williams
2023-09-28 22:49 ` Dan Williams
2023-09-29 17:26 ` Peter Gonda
2023-10-03 18:37 ` Peter Gonda
2023-10-03 19:29 ` Kuppuswamy Sathyanarayanan
2023-10-03 20:06 ` Peter Gonda
2023-10-04 0:54 ` Dan Williams
2023-10-10 19:36 ` Dan Williams
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).