[PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[Dan Williams]

Note that I am sending this during the merge window due to the high
level of interest. My current expectation, barring major review
concerns, is that this intercepts Linux-next soon after v6.6-rc1 for a
v6.7 merge. Given the switch to configfs I did not carry forward
Reviewed-by's from v2.

Changes since v2 [1]:
- Switch from sysfs to configfs to scale the interface for containers
  (Jeremi)
- Fix locking in outblob_read() to avoid racing freeing and generation
  of ->outblob (Jeremi)
- Add missing mutex to sev_report_new() (Jeremi)
- Fix incorrect usage of no_free_ptr(), switch to return_ptr() (Peter)
- Drop hex input parsing (configfs bin attributes are not seekable which
  eliminates the concern) (Greg)
- Note why DEFINE_FREE() for kvfree() includes a NULL check (Greg)
- Document the permissible values of privlevel in the ABI documentation
  (Greg)
- Bump column limit to 100 for sev-guest changes, since that's existing
  code, for tsm.c use the .clang_format default. (Tom)
- Drop report buffer size to 4K (Tom)
- Fix uninitialized variable @rc in register_tsm() (Tom)
- Fix collision detection confusion, always increment write_generation
  on successful write regardless if old data is being re-written (Tom)
- Switch to sockptr_t for sharing {get,get_ext}_report() between the
  ioctl and configfs paths (Andy)

[1]: http://lore.kernel.org/r/169199898909.1782217.10899362240465838600.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


 Documentation/ABI/testing/configfs-tsm  |   68 +++++
 MAINTAINERS                             |    8 +
 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 |  133 +++++++++--
 drivers/virt/coco/tdx-guest/Kconfig     |    1
 drivers/virt/coco/tsm.c                 |  391 +++++++++++++++++++++++++++++++
 include/linux/slab.h                    |    2
 include/linux/tsm.h                     |   54 ++++
 12 files changed, 665 insertions(+), 25 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: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c

[PATCH v3 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig

[Dan Williams]

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.

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/

Re: [PATCH v3 1/5] virt: coco: Add a coco/Makefile and coco/Kconfig

[Kuppuswamy Sathyanarayanan]

On 8/30/2023 12:33 PM, Dan Williams wrote:
> 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.
> 
> 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/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/
> 
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

[PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

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>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/configfs-tsm |   68 ++++++
 MAINTAINERS                            |    8 +
 drivers/virt/coco/Kconfig              |    5 
 drivers/virt/coco/Makefile             |    1 
 drivers/virt/coco/tdx-guest/Kconfig    |    1 
 drivers/virt/coco/tsm.c                |  391 ++++++++++++++++++++++++++++++++
 include/linux/tsm.h                    |   54 ++++
 7 files changed, 528 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..0f137039643b
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-tsm
@@ -0,0 +1,68 @@
+What:		/sys/kernel/config/tsm/report/$name/inblob
+Date:		August, 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:		August, 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
+		(modulo options like @format and @privlevel) where the
+		implementation is conveyed via the @provider attribute.
+
+What:		/sys/kernel/config/tsm/report/$name/provider
+Date:		August, 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:		August, 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:		August, 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:		August, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) Indicates the minimum permissible value that can be written
+		to @privlevel.
+
+What:		/sys/kernel/config/tsm/report/$name/format
+Date:		August, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(WO) If a TSM implementation supports the concept of attestation
+		reports with "extended" contents, like SEV-SNP extended reports
+		with certificate chains, specify "extended" vs "default" via
+		this attribute.
diff --git a/MAINTAINERS b/MAINTAINERS
index 4cc6bf79fdd8..996122ab62ab 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21671,6 +21671,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/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/tsm.c b/drivers/virt/coco/tsm.c
new file mode 100644
index 000000000000..da19257797d7
--- /dev/null
+++ b/drivers/virt/coco/tsm.c
@@ -0,0 +1,391 @@
+// 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, likely only once at TVM boot time.
+ *
+ * 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'.
+ */
+
+/**
+ * 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;
+	unsigned long write_generation;
+	unsigned long read_generation;
+	struct config_item cfg;
+};
+
+static struct tsm_report *to_tsm_report(struct config_item *cfg)
+{
+	return container_of(cfg, struct tsm_report, cfg);
+}
+
+static int try_advance_write_generation(struct tsm_report *report)
+{
+	lockdep_assert_held_write(&tsm_rwsem);
+
+	/*
+	 * malicious or broken userspace is attempting to wrap read_generation,
+	 * stop accepting updates until current report configuration is read.
+	 */
+	if (report->write_generation == report->read_generation - 1)
+		return -EBUSY;
+	report->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_format_store(struct config_item *cfg, const char *buf,
+				       size_t len)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	enum tsm_format format;
+	int rc;
+
+	if (sysfs_streq(buf, "default"))
+		format = TSM_FORMAT_DEFAULT;
+	else if (sysfs_streq(buf, "extended"))
+		format = TSM_FORMAT_EXTENDED;
+	else
+		return -EINVAL;
+
+	guard(rwsem_write)(&tsm_rwsem);
+	rc = try_advance_write_generation(report);
+	if (rc)
+		return rc;
+	report->desc.outblob_format = format;
+
+	return len;
+}
+CONFIGFS_ATTR_WO(tsm_report_, format);
+
+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);
+
+	guard(rwsem_read)(&tsm_rwsem);
+	return sysfs_emit(buf, "%lu\n", report->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_cached_report(struct tsm_report *report, void *buf, size_t count)
+{
+	loff_t offset = 0;
+
+	guard(rwsem_read)(&tsm_rwsem);
+	if (!report->desc.inblob_len)
+		return -EINVAL;
+
+	if (!report->outblob ||
+	    report->read_generation != report->write_generation)
+		return -EWOULDBLOCK;
+
+	if (!buf)
+		return report->outblob_len;
+	return memory_read_from_buffer(buf, count, &offset, report->outblob,
+				       report->outblob_len);
+}
+
+static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
+				       size_t count)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	const struct tsm_ops *ops;
+	size_t outblob_len;
+	loff_t offset = 0;
+	u8 *outblob;
+	ssize_t rc;
+
+	/* try to read from the existing report if present and valid... */
+	rc = read_cached_report(report, buf, count);
+	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 &&
+	    report->read_generation == report->write_generation)
+		goto out;
+
+	kvfree(report->outblob);
+	report->outblob = NULL;
+	outblob = ops->report_new(&report->desc, provider.data, &outblob_len);
+	if (IS_ERR(outblob))
+		return PTR_ERR(outblob);
+	report->outblob_len = outblob_len;
+	report->outblob = outblob;
+	report->read_generation = report->write_generation;
+
+out:
+	if (!buf)
+		return report->outblob_len;
+	return memory_read_from_buffer(buf, count, &offset, report->outblob,
+				       report->outblob_len);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, 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,
+	NULL,
+};
+
+static struct configfs_attribute *tsm_report_extra_attrs[] = {
+	TSM_DEFAULT_ATTRS(),
+	&tsm_report_attr_format,
+	&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);
+
+	kvfree(report->outblob);
+	kfree(report);
+}
+
+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 *report;
+
+	guard(rwsem_read)(&tsm_rwsem);
+	if (!provider.ops)
+		return ERR_PTR(-ENXIO);
+
+	report = kzalloc(sizeof(*report), GFP_KERNEL);
+	if (!report)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&report->cfg, name, provider.type);
+	return &report->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 register_tsm(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(register_tsm);
+
+int unregister_tsm(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(unregister_tsm);
+
+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..4b110b69a330
--- /dev/null
+++ b/include/linux/tsm.h
@@ -0,0 +1,54 @@
+/* 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
+
+enum tsm_format {
+	TSM_FORMAT_DEFAULT,
+	TSM_FORMAT_EXTENDED,
+};
+
+/**
+ * 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
+ * @outblob_format: for TSMs with an "extended" format
+ */
+struct tsm_desc {
+	unsigned int privlevel;
+	size_t inblob_len;
+	u8 inblob[TSM_INBLOB_MAX];
+	enum tsm_format outblob_format;
+};
+
+/*
+ * 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;
+	u8 *(*report_new)(const struct tsm_desc *desc, void *data,
+			  size_t *outblob_len);
+};
+
+extern const struct config_item_type tsm_report_ext_type;
+extern const struct config_item_type tsm_report_default_type;
+
+int register_tsm(const struct tsm_ops *ops, void *priv,
+		 const struct config_item_type *type);
+int unregister_tsm(const struct tsm_ops *ops);
+#endif /* __TSM_H */

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Greg Kroah-Hartman]

On Wed, Aug 30, 2023 at 12:33:24PM -0700, 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>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/configfs-tsm |   68 ++++++

Nice, I like the use of configfs here.

One very tiny naming nit, feel free to ignore if you don't want to
change it:

> +int register_tsm(const struct tsm_ops *ops, void *priv,
> +		 const struct config_item_type *type);
> +int unregister_tsm(const struct tsm_ops *ops);

Usually it's "noun_verb" for stuff that you export to the global
namespace these days.

So perhaps tsm_register() and tsm_unregister()?

Either way, it's your call:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Greg Kroah-Hartman wrote:
[..]
> > +int register_tsm(const struct tsm_ops *ops, void *priv,
> > +		 const struct config_item_type *type);
> > +int unregister_tsm(const struct tsm_ops *ops);
> 
> Usually it's "noun_verb" for stuff that you export to the global
> namespace these days.
> 
> So perhaps tsm_register() and tsm_unregister()?
> 
> Either way, it's your call:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Sure, noun_verb works for me, will make that change. Thanks Greg.

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Kuppuswamy Sathyanarayanan]

HI,

On 8/30/2023 12:33 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>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  Documentation/ABI/testing/configfs-tsm |   68 ++++++
>  MAINTAINERS                            |    8 +
>  drivers/virt/coco/Kconfig              |    5 
>  drivers/virt/coco/Makefile             |    1 
>  drivers/virt/coco/tdx-guest/Kconfig    |    1 
>  drivers/virt/coco/tsm.c                |  391 ++++++++++++++++++++++++++++++++
>  include/linux/tsm.h                    |   54 ++++
>  7 files changed, 528 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..0f137039643b
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-tsm
> @@ -0,0 +1,68 @@
> +What:		/sys/kernel/config/tsm/report/$name/inblob
> +Date:		August, 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:		August, 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
> +		(modulo options like @format and @privlevel) where the
> +		implementation is conveyed via the @provider attribute.
> +
> +What:		/sys/kernel/config/tsm/report/$name/provider
> +Date:		August, 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:		August, 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:		August, 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:		August, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Indicates the minimum permissible value that can be written
> +		to @privlevel.
> +
> +What:		/sys/kernel/config/tsm/report/$name/format
> +Date:		August, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(WO) If a TSM implementation supports the concept of attestation
> +		reports with "extended" contents, like SEV-SNP extended reports
> +		with certificate chains, specify "extended" vs "default" via
> +		this attribute.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4cc6bf79fdd8..996122ab62ab 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21671,6 +21671,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/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

I think it is more appropriate to let TDX support patch add it. Agree?

>  	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/tsm.c b/drivers/virt/coco/tsm.c
> new file mode 100644
> index 000000000000..da19257797d7
> --- /dev/null
> +++ b/drivers/virt/coco/tsm.c
> @@ -0,0 +1,391 @@
> +// 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, likely only once at TVM boot time.
> + *
> + * 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'.
> + */
> +
> +/**
> + * 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;
> +	unsigned long write_generation;
> +	unsigned long read_generation;
> +	struct config_item cfg;
> +};
> +
> +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> +{
> +	return container_of(cfg, struct tsm_report, cfg);
> +}
> +
> +static int try_advance_write_generation(struct tsm_report *report)
> +{
> +	lockdep_assert_held_write(&tsm_rwsem);
> +
> +	/*
> +	 * malicious or broken userspace is attempting to wrap read_generation,
> +	 * stop accepting updates until current report configuration is read.
> +	 */
> +	if (report->write_generation == report->read_generation - 1)
> +		return -EBUSY;
> +	report->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_format_store(struct config_item *cfg, const char *buf,
> +				       size_t len)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	enum tsm_format format;
> +	int rc;
> +
> +	if (sysfs_streq(buf, "default"))
> +		format = TSM_FORMAT_DEFAULT;
> +	else if (sysfs_streq(buf, "extended"))
> +		format = TSM_FORMAT_EXTENDED;
> +	else
> +		return -EINVAL;
> +
> +	guard(rwsem_write)(&tsm_rwsem);
> +	rc = try_advance_write_generation(report);
> +	if (rc)
> +		return rc;
> +	report->desc.outblob_format = format;
> +
> +	return len;
> +}
> +CONFIGFS_ATTR_WO(tsm_report_, format);
> +
> +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);
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	return sysfs_emit(buf, "%lu\n", report->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_cached_report(struct tsm_report *report, void *buf, size_t count)
> +{
> +	loff_t offset = 0;
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	if (!report->desc.inblob_len)
> +		return -EINVAL;
> +
> +	if (!report->outblob ||
> +	    report->read_generation != report->write_generation)
> +		return -EWOULDBLOCK;
> +
> +	if (!buf)
> +		return report->outblob_len;
> +	return memory_read_from_buffer(buf, count, &offset, report->outblob,
> +				       report->outblob_len);
> +}
> +
> +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> +				       size_t count)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	const struct tsm_ops *ops;
> +	size_t outblob_len;
> +	loff_t offset = 0;
> +	u8 *outblob;
> +	ssize_t rc;
> +
> +	/* try to read from the existing report if present and valid... */
> +	rc = read_cached_report(report, buf, count);
> +	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 &&
> +	    report->read_generation == report->write_generation)
> +		goto out;
> +
> +	kvfree(report->outblob);
> +	report->outblob = NULL;
> +	outblob = ops->report_new(&report->desc, provider.data, &outblob_len);
> +	if (IS_ERR(outblob))
> +		return PTR_ERR(outblob);
> +	report->outblob_len = outblob_len;
> +	report->outblob = outblob;
> +	report->read_generation = report->write_generation;
> +
> +out:
> +	if (!buf)
> +		return report->outblob_len;
> +	return memory_read_from_buffer(buf, count, &offset, report->outblob,
> +				       report->outblob_len);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, 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,
> +	NULL,
> +};
> +
> +static struct configfs_attribute *tsm_report_extra_attrs[] = {
> +	TSM_DEFAULT_ATTRS(),
> +	&tsm_report_attr_format,
> +	&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);
> +
> +	kvfree(report->outblob);
> +	kfree(report);
> +}
> +
> +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 *report;
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	if (!provider.ops)
> +		return ERR_PTR(-ENXIO);
> +
> +	report = kzalloc(sizeof(*report), GFP_KERNEL);
> +	if (!report)
> +		return ERR_PTR(-ENOMEM);
> +
> +	config_item_init_type_name(&report->cfg, name, provider.type);
> +	return &report->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 register_tsm(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(register_tsm);
> +
> +int unregister_tsm(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(unregister_tsm);
> +
> +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..4b110b69a330
> --- /dev/null
> +++ b/include/linux/tsm.h
> @@ -0,0 +1,54 @@
> +/* 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
> +
> +enum tsm_format {
> +	TSM_FORMAT_DEFAULT,
> +	TSM_FORMAT_EXTENDED,
> +};
> +
> +/**
> + * 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
> + * @outblob_format: for TSMs with an "extended" format
> + */
> +struct tsm_desc {
> +	unsigned int privlevel;
> +	size_t inblob_len;
> +	u8 inblob[TSM_INBLOB_MAX];
> +	enum tsm_format outblob_format;
> +};
> +
> +/*
> + * 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;
> +	u8 *(*report_new)(const struct tsm_desc *desc, void *data,
> +			  size_t *outblob_len);
> +};
> +
> +extern const struct config_item_type tsm_report_ext_type;
> +extern const struct config_item_type tsm_report_default_type;
> +
> +int register_tsm(const struct tsm_ops *ops, void *priv,
> +		 const struct config_item_type *type);
> +int unregister_tsm(const struct tsm_ops *ops);
> +#endif /* __TSM_H */
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Kuppuswamy Sathyanarayanan wrote:
[..]
> > 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
> 
> I think it is more appropriate to let TDX support patch add it. Agree?

Oh definitely, I think this was a leftover from some local debug.

I have fixed that up along with Greg's feedback and pushed an updated
set here:

https://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux.git/log/?h=for-6.7/coco

...will keep that up to date as review feedback arrives.

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dionna Amalie Glaze]

This is clean and approachable. Thanks for your implementation.

> +static int try_advance_write_generation(struct tsm_report *report)
> +{
> +       lockdep_assert_held_write(&tsm_rwsem);
> +
> +       /*
> +        * malicious or broken userspace is attempting to wrap read_generation,
> +        * stop accepting updates until current report configuration is read.
> +        */
> +       if (report->write_generation == report->read_generation - 1)
> +               return -EBUSY;
> +       report->write_generation++;
> +       return 0;
> +}
> +

This took me a while to wrap my head around.
The property we want is that when we read outblob, it is the result of
the attribute changes since the last read. If we write to an attribute
2^64 times, we could get write_generation to wrap around to equal
read_generation without actually reading outblob. So we could end up
given a badly cached result when we read outblob? Is that what this is
preventing?

I think I would word this to say,

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

I think that at least in the SEV-SNP case, we can double-check from
userspace that the report has values that we expected to configure the
get_report with, so this isn't really an issue. I'm not sure what the
use is of configuration that doesn't lead to observable (and
checkable) differences, but I suppose this check doesn't hurt.

-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Dionna Amalie Glaze wrote:
> This is clean and approachable. Thanks for your implementation.
> 
> > +static int try_advance_write_generation(struct tsm_report *report)
> > +{
> > +       lockdep_assert_held_write(&tsm_rwsem);
> > +
> > +       /*
> > +        * malicious or broken userspace is attempting to wrap read_generation,
> > +        * stop accepting updates until current report configuration is read.
> > +        */
> > +       if (report->write_generation == report->read_generation - 1)
> > +               return -EBUSY;
> > +       report->write_generation++;
> > +       return 0;
> > +}
> > +
> 
> This took me a while to wrap my head around.
> The property we want is that when we read outblob, it is the result of
> the attribute changes since the last read. If we write to an attribute
> 2^64 times, we could get write_generation to wrap around to equal
> read_generation without actually reading outblob. So we could end up
> given a badly cached result when we read outblob? Is that what this is
> preventing?

Correct. The criticism of kernfs based interfaces like sysfs and
configfs is that there is no facility to atomically modify a set of
attributes at once. The expectated mitigation is simply that userspace
is well behaved. For example, 2 invocations of fdisk can confuse each
other, so userspace is expected to run them serially and the kernel
otherwise lets userspace do silly things.

If a tool has any concern that it has exclusive ownership of the
interface this 'generation' attribute allows for a flow like:

gen=$(cat $report/generation)
dd if=userdata > $report/inblob
cat $report/outblob > report
gen2=$(cat $report/generation)

...and if $gen2 is not $((gen + 1)) then tooling can detect that the
"userspace is well behaved" expectation was violated.

Now configs is slightly better in this regard because objects can be
atomically created. So if 2 threads happen to pick the same name for the
report object then 'mkdir' will only succeed for one while the other
gets an EEXIST error. So for containers each can get their own
submission interface without worrying about other containers.

> I think I would word this to say,
> 
> "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."

That works for me.

> I think that at least in the SEV-SNP case, we can double-check from
> userspace that the report has values that we expected to configure the
> get_report with, so this isn't really an issue. I'm not sure what the
> use is of configuration that doesn't lead to observable (and
> checkable) differences, but I suppose this check doesn't hurt.

Right, the tool can also just double check that all the
parameters are the expected value in the output report, but if you want
to trust the kernel output without necessarily trusting other tools to
not stomp on your config item instance then 'generation' helps.

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Thomas Gleixner]

On Thu, Aug 31 2023 at 15:13, Dan Williams wrote:
> Dionna Amalie Glaze wrote:
>> This is clean and approachable. Thanks for your implementation.
>> 
>> > +static int try_advance_write_generation(struct tsm_report *report)
>> > +{
>> > +       lockdep_assert_held_write(&tsm_rwsem);
>> > +
>> > +       /*
>> > +        * malicious or broken userspace is attempting to wrap read_generation,
>> > +        * stop accepting updates until current report configuration is read.
>> > +        */
>> > +       if (report->write_generation == report->read_generation - 1)
>> > +               return -EBUSY;
>> > +       report->write_generation++;
>> > +       return 0;
>> > +}
>> > +
>> 
>> This took me a while to wrap my head around.
>> The property we want is that when we read outblob, it is the result of
>> the attribute changes since the last read. If we write to an attribute
>> 2^64 times, we could get write_generation to wrap around to equal
>> read_generation without actually reading outblob. So we could end up
>> given a badly cached result when we read outblob? Is that what this is
>> preventing?
>
> Correct. The criticism of kernfs based interfaces like sysfs and
> configfs is that there is no facility to atomically modify a set of
> attributes at once. The expectated mitigation is simply that userspace
> is well behaved. For example, 2 invocations of fdisk can confuse each
> other, so userspace is expected to run them serially and the kernel
> otherwise lets userspace do silly things.
>
> If a tool has any concern that it has exclusive ownership of the
> interface this 'generation' attribute allows for a flow like:
>
> gen=$(cat $report/generation)
> dd if=userdata > $report/inblob
> cat $report/outblob > report
> gen2=$(cat $report/generation)
>
> ...and if $gen2 is not $((gen + 1)) then tooling can detect that the
> "userspace is well behaved" expectation was violated.
>
> Now configs is slightly better in this regard because objects can be
> atomically created. So if 2 threads happen to pick the same name for the
> report object then 'mkdir' will only succeed for one while the other
> gets an EEXIST error. So for containers each can get their own
> submission interface without worrying about other containers.
>
>> I think I would word this to say,
>> 
>> "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."
>
> That works for me.

That's a pretty theoretical problem. Under the assumption that one
syscall takes 50ns the wraparound on a 64bit variable will take ~29247
years to complete.

I think the important point is that the generation check there ensures
that the expected sequence takes place and can't be overwritten by
accident or malice, no?

Thanks,

        tglx

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Thomas Gleixner wrote:
> On Thu, Aug 31 2023 at 15:13, Dan Williams wrote:
> > Dionna Amalie Glaze wrote:
> >> This is clean and approachable. Thanks for your implementation.
> >> 
> >> > +static int try_advance_write_generation(struct tsm_report *report)
> >> > +{
> >> > +       lockdep_assert_held_write(&tsm_rwsem);
> >> > +
> >> > +       /*
> >> > +        * malicious or broken userspace is attempting to wrap read_generation,
> >> > +        * stop accepting updates until current report configuration is read.
> >> > +        */
> >> > +       if (report->write_generation == report->read_generation - 1)
> >> > +               return -EBUSY;
> >> > +       report->write_generation++;
> >> > +       return 0;
> >> > +}
> >> > +
> >> 
> >> This took me a while to wrap my head around.
> >> The property we want is that when we read outblob, it is the result of
> >> the attribute changes since the last read. If we write to an attribute
> >> 2^64 times, we could get write_generation to wrap around to equal
> >> read_generation without actually reading outblob. So we could end up
> >> given a badly cached result when we read outblob? Is that what this is
> >> preventing?
> >
> > Correct. The criticism of kernfs based interfaces like sysfs and
> > configfs is that there is no facility to atomically modify a set of
> > attributes at once. The expectated mitigation is simply that userspace
> > is well behaved. For example, 2 invocations of fdisk can confuse each
> > other, so userspace is expected to run them serially and the kernel
> > otherwise lets userspace do silly things.
> >
> > If a tool has any concern that it has exclusive ownership of the
> > interface this 'generation' attribute allows for a flow like:
> >
> > gen=$(cat $report/generation)
> > dd if=userdata > $report/inblob
> > cat $report/outblob > report
> > gen2=$(cat $report/generation)
> >
> > ...and if $gen2 is not $((gen + 1)) then tooling can detect that the
> > "userspace is well behaved" expectation was violated.
> >
> > Now configs is slightly better in this regard because objects can be
> > atomically created. So if 2 threads happen to pick the same name for the
> > report object then 'mkdir' will only succeed for one while the other
> > gets an EEXIST error. So for containers each can get their own
> > submission interface without worrying about other containers.
> >
> >> I think I would word this to say,
> >> 
> >> "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."
> >
> > That works for me.
> 
> That's a pretty theoretical problem. Under the assumption that one
> syscall takes 50ns the wraparound on a 64bit variable will take ~29247
> years to complete.
> 
> I think the important point is that the generation check there ensures
> that the expected sequence takes place and can't be overwritten by
> accident or malice, no?

Exactly. The "attack" / "bug" is hard to carry out, so this is more for
theoretical completeness than practical protection.

Re: [PATCH v3 2/5] configfs-tsm: Introduce a shared ABI for attestation reports

[Thomas Gleixner]

On Wed, Aug 30 2023 at 12:33, Dan Williams wrote:
>
> 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>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Nice!

Acked-by: Thomas Gleixner <tglx@linutronix.de>

[PATCH v3 3/5] virt: sevguest: Prep for kernel internal {get, get_ext}_report()

[Dan Williams]

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;

[PATCH v3 4/5] mm/slab: Add __free() support for kvfree

[Dan Williams]

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>
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 848c7c82ad5a..241025367943 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -746,6 +746,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);

Re: [PATCH v3 4/5] mm/slab: Add __free() support for kvfree

[Greg Kroah-Hartman]

On Wed, Aug 30, 2023 at 12:33:36PM -0700, Dan Williams wrote:
> 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>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>  include/linux/slab.h |    2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 4/5] mm/slab: Add __free() support for kvfree

[Gupta, Pankaj]

On 8/30/2023 9:33 PM, Dan Williams wrote:
> 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>
> 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 848c7c82ad5a..241025367943 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -746,6 +746,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);
> 
> 
Liked this patch and use in the patch5. Still cannot comment on the 
attestation part of things. So, for this patch:

Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>

[PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Dan Williams]

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 blobs that
the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow follows for
retrieving the SNP_GET_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
    rmdir $report

...while the SNP_GET_EXT_REPORT flow needs to additionally set the
format to "extended":

    report=/sys/kernel/config/tsm/report/report1
    mkdir $report
    echo extended > $report/format
    dd if=/dev/urandom bs=64 count=1 > $report/inblob
    hexdump -C $report/outblob
    rmdir $report

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>
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 |   83 +++++++++++++++++++++++++++++++
 2 files changed, 84 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..c7bbb8f372a3 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,79 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
 	return key;
 }
 
+static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
+{
+	struct snp_guest_dev *snp_dev = data;
+	const int report_size = SZ_4K;
+	const int ext_size = SZ_16K;
+	int ret, size;
+
+	if (desc->inblob_len != 64)
+		return ERR_PTR(-EINVAL);
+
+	if (desc->outblob_format == TSM_FORMAT_EXTENDED)
+		size = report_size + ext_size;
+	else
+		size = report_size;
+
+	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+
+	guard(mutex)(&snp_cmd_mutex);
+	if (desc->outblob_format == TSM_FORMAT_EXTENDED) {
+		struct snp_ext_report_req ext_req = {
+			.data = { .vmpl = desc->privlevel },
+			.certs_address = (__u64)buf + report_size,
+			.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);
+	} else {
+		struct snp_report_req req = {
+			.vmpl = desc->privlevel,
+		};
+		memcpy(&req.user_data, desc->inblob, desc->inblob_len);
+
+		struct snp_guest_request_ioctl input = {
+			.msg_version = 1,
+			.req_data = (__u64)&req,
+			.resp_data = (__u64)buf,
+		};
+		struct snp_req_resp io = {
+			.req_data = KERNEL_SOCKPTR(&req),
+			.resp_data = KERNEL_SOCKPTR(buf),
+		};
+
+		ret = get_report(snp_dev, &input, &io);
+	}
+
+	if (ret)
+		return ERR_PTR(ret);
+
+	*outblob_len = size;
+	return_ptr(buf);
+}
+
+static const struct tsm_ops sev_tsm_ops = {
+	.name = KBUILD_MODNAME,
+	.report_new = sev_report_new,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+	unregister_tsm(&sev_tsm_ops);
+}
+
 static int __init sev_guest_probe(struct platform_device *pdev)
 {
 	struct snp_secrets_page_layout *layout;
@@ -832,6 +907,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 = register_tsm(&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;

Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Jeremi Piotrowski]

On 8/30/2023 9:33 PM, Dan Williams wrote:
> 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 blobs that
> the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow follows for
> retrieving the SNP_GET_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
>     rmdir $report
> 
> ...while the SNP_GET_EXT_REPORT flow needs to additionally set the
> format to "extended":
> 
>     report=/sys/kernel/config/tsm/report/report1
>     mkdir $report
>     echo extended > $report/format
>     dd if=/dev/urandom bs=64 count=1 > $report/inblob
>     hexdump -C $report/outblob
>     rmdir $report
> 
> 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>
> 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 |   83 +++++++++++++++++++++++++++++++
>  2 files changed, 84 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..c7bbb8f372a3 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,79 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
>  	return key;
>  }
>  
> +static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> +{
> +	struct snp_guest_dev *snp_dev = data;
> +	const int report_size = SZ_4K;
> +	const int ext_size = SZ_16K;
> +	int ret, size;
> +
> +	if (desc->inblob_len != 64)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (desc->outblob_format == TSM_FORMAT_EXTENDED)
> +		size = report_size + ext_size;
> +	else
> +		size = report_size;
> +
> +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> +
> +	guard(mutex)(&snp_cmd_mutex);
> +	if (desc->outblob_format == TSM_FORMAT_EXTENDED) {
> +		struct snp_ext_report_req ext_req = {
> +			.data = { .vmpl = desc->privlevel },
> +			.certs_address = (__u64)buf + report_size,
> +			.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);
> +	} else {
> +		struct snp_report_req req = {
> +			.vmpl = desc->privlevel,
> +		};
> +		memcpy(&req.user_data, desc->inblob, desc->inblob_len);
> +
> +		struct snp_guest_request_ioctl input = {
> +			.msg_version = 1,
> +			.req_data = (__u64)&req,
> +			.resp_data = (__u64)buf,
> +		};
> +		struct snp_req_resp io = {
> +			.req_data = KERNEL_SOCKPTR(&req),
> +			.resp_data = KERNEL_SOCKPTR(buf),
> +		};
> +
> +		ret = get_report(snp_dev, &input, &io);
> +	}
> +
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	*outblob_len = size;
> +	return_ptr(buf);
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> +	unregister_tsm(&sev_tsm_ops);
> +}
> +
>  static int __init sev_guest_probe(struct platform_device *pdev)
>  {
>  	struct snp_secrets_page_layout *layout;
> @@ -832,6 +907,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 = register_tsm(&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;
> 

I tried this with the non-extended request and realized it's a bit awkward from
a uapi point of view. The returned outblob has a header prepended (table 23 in [1])
and is arbitrarily sized at 4096. It would be more natural to only return the report
field and the count bytes that the report actually has. I've attached a rough patch
below to give you an idea of what I mean.

The extended guest request is another topic, since userspace has to be aware of
where the kernel choses to put the extended data, and fixup all the offsets in the
table (section 4.1.8.1 in [2]). It would be better to return this data through a
separate file.

[1]: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
[2]: https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421-guest-hypervisor-communication-block-standardization.pdf

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index c7bbb8f372a3..e92a82d9c53f 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -761,9 +761,18 @@ 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
+
 static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
 {
 	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;
@@ -777,6 +786,8 @@ static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outbl
 		size = report_size;
 
 	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+	if (!buf)
+		return ERR_PTR(-ENOMEM);
 
 	guard(mutex)(&snp_cmd_mutex);
 	if (desc->outblob_format == TSM_FORMAT_EXTENDED) {
@@ -820,8 +831,24 @@ static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outbl
 	if (ret)
 		return ERR_PTR(ret);
 
-	*outblob_len = size;
-	return_ptr(buf);
+	memcpy(&hdr, buf, sizeof(hdr));
+	if (hdr.status == SNP_REPORT_INVALID_PARAM)
+		return ERR_PTR(-EINVAL);
+	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
+		return ERR_PTR(-ENOKEY);
+	if (hdr.status)
+		return ERR_PTR(-EPROTO);
+	if ((hdr.report_size + sizeof(hdr)) > size)
+		return ERR_PTR(-ENOSPC);
+
+	/* TODO: figure out how we want to handle extended report */
+	u8 *buf2 = kvzalloc(hdr.report_size, GFP_KERNEL);
+	if (!buf2)
+		return ERR_PTR(-ENOMEM);
+
+	memcpy(buf2, buf + sizeof(hdr), hdr.report_size);
+	*outblob_len = hdr.report_size;
+	return buf2;
 }
 
 static const struct tsm_ops sev_tsm_ops = {

Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Dan Williams]

Jeremi Piotrowski wrote:
> On 8/30/2023 9:33 PM, Dan Williams wrote:
> > 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 blobs that
> > the SNP_{GET,GET_EXT}_REPORT ioctls produce. An example flow follows for
> > retrieving the SNP_GET_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
> >     rmdir $report
> > 
> > ...while the SNP_GET_EXT_REPORT flow needs to additionally set the
> > format to "extended":
> > 
> >     report=/sys/kernel/config/tsm/report/report1
> >     mkdir $report
> >     echo extended > $report/format
> >     dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >     hexdump -C $report/outblob
> >     rmdir $report
> > 
> > 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>
> > 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 |   83 +++++++++++++++++++++++++++++++
> >  2 files changed, 84 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..c7bbb8f372a3 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,79 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
> >  	return key;
> >  }
> >  
> > +static u8 *sev_report_new(const struct tsm_desc *desc, void *data, size_t *outblob_len)
> > +{
> > +	struct snp_guest_dev *snp_dev = data;
> > +	const int report_size = SZ_4K;
> > +	const int ext_size = SZ_16K;
> > +	int ret, size;
> > +
> > +	if (desc->inblob_len != 64)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	if (desc->outblob_format == TSM_FORMAT_EXTENDED)
> > +		size = report_size + ext_size;
> > +	else
> > +		size = report_size;
> > +
> > +	u8 *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > +
> > +	guard(mutex)(&snp_cmd_mutex);
> > +	if (desc->outblob_format == TSM_FORMAT_EXTENDED) {
> > +		struct snp_ext_report_req ext_req = {
> > +			.data = { .vmpl = desc->privlevel },
> > +			.certs_address = (__u64)buf + report_size,
> > +			.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);
> > +	} else {
> > +		struct snp_report_req req = {
> > +			.vmpl = desc->privlevel,
> > +		};
> > +		memcpy(&req.user_data, desc->inblob, desc->inblob_len);
> > +
> > +		struct snp_guest_request_ioctl input = {
> > +			.msg_version = 1,
> > +			.req_data = (__u64)&req,
> > +			.resp_data = (__u64)buf,
> > +		};
> > +		struct snp_req_resp io = {
> > +			.req_data = KERNEL_SOCKPTR(&req),
> > +			.resp_data = KERNEL_SOCKPTR(buf),
> > +		};
> > +
> > +		ret = get_report(snp_dev, &input, &io);
> > +	}
> > +
> > +	if (ret)
> > +		return ERR_PTR(ret);
> > +
> > +	*outblob_len = size;
> > +	return_ptr(buf);
> > +}
> > +
> > +static const struct tsm_ops sev_tsm_ops = {
> > +	.name = KBUILD_MODNAME,
> > +	.report_new = sev_report_new,
> > +};
> > +
> > +static void unregister_sev_tsm(void *data)
> > +{
> > +	unregister_tsm(&sev_tsm_ops);
> > +}
> > +
> >  static int __init sev_guest_probe(struct platform_device *pdev)
> >  {
> >  	struct snp_secrets_page_layout *layout;
> > @@ -832,6 +907,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 = register_tsm(&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;
> > 
> 
> I tried this with the non-extended request

Thanks for testing!

> ...and realized it's a bit awkward from
> a uapi point of view. The returned outblob has a header prepended (table 23 in [1])
> and is arbitrarily sized at 4096. It would be more natural to only return the report
> field and the count bytes that the report actually has. I've attached a rough patch
> below to give you an idea of what I mean.

It makes sense, especially if that is what the legacy ioctl output
buffer is emitting.

> The extended guest request is another topic, since userspace has to be aware of
> where the kernel choses to put the extended data, and fixup all the offsets in the
> table (section 4.1.8.1 in [2]). It would be better to return this data through a
> separate file.

I notice that the TDX report also includes a certificate blob, so if
that is a common concept then yes, it makes sense to have a separate
file for that.

Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Huang, Kai]

On Fri, 2023-09-01 at 09:38 -0700, Dan Williams wrote:
> > The extended guest request is another topic, since userspace has to be aware of
> > where the kernel choses to put the extended data, and fixup all the offsets in the
> > table (section 4.1.8.1 in [2]). It would be better to return this data through a
> > separate file.
> 
> I notice that the TDX report also includes a certificate blob, so if
> that is a common concept then yes, it makes sense to have a separate
> file for that.

+ Sathy and Isaku.

It is a common concept from the perspective of "concept", because we need
certificates to verify the attestation blob anyway.  But in implementation,
unlike to SEV, TDX doesn't have a command to return certificates separately or
independently [1] -- they are embed to the Quote itself, or theoretically can be
fetched from Intel.  

More, for TDX (SGX based attestation) certificates blob itself isn't mandatory
to be part of the Quote.  Instead, TDX Quote can choose to include some more
basic platform identification which can in turn be used to get those
certificates from Intel's provisioning certificate service [2].

[1] I am not sure whether we can add one or already have one in the latest TDX
development.  Maybe Sathy or Isaku can help to confirm.

[2]: Table 9: QE Certification Data
https://download.01.org/intel-sgx/dcap-1.0.1/docs/Intel_SGX_ECDSA_QuoteGenReference_DCAP_API_Linux_1.0.1.pdf

Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Kuppuswamy Sathyanarayanan]

On 9/3/2023 7:14 PM, Huang, Kai wrote:
> On Fri, 2023-09-01 at 09:38 -0700, Dan Williams wrote:
>>> The extended guest request is another topic, since userspace has to be aware of
>>> where the kernel choses to put the extended data, and fixup all the offsets in the
>>> table (section 4.1.8.1 in [2]). It would be better to return this data through a
>>> separate file.
>>
>> I notice that the TDX report also includes a certificate blob, so if
>> that is a common concept then yes, it makes sense to have a separate
>> file for that.
> 
> + Sathy and Isaku.
> 
> It is a common concept from the perspective of "concept", because we need
> certificates to verify the attestation blob anyway.  But in implementation,
> unlike to SEV, TDX doesn't have a command to return certificates separately or
> independently [1] -- they are embed to the Quote itself, or theoretically can be
> fetched from Intel.  
> 
> More, for TDX (SGX based attestation) certificates blob itself isn't mandatory
> to be part of the Quote.  Instead, TDX Quote can choose to include some more
> basic platform identification which can in turn be used to get those
> certificates from Intel's provisioning certificate service [2].
> 
> [1] I am not sure whether we can add one or already have one in the latest TDX
> development.  Maybe Sathy or Isaku can help to confirm.
> 
> [2]: Table 9: QE Certification Data
> https://download.01.org/intel-sgx/dcap-1.0.1/docs/Intel_SGX_ECDSA_QuoteGenReference_DCAP_API_Linux_1.0.1.pdf

Yes. TDX does not have any special command to fetch the certificate blob
separately. Currently, it is fetched as part of Quote data. But, since the
certificate blob is fixed per boot (unlike Quote data), I think it makes
sense to add a separate command for it.


-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v3 5/5] virt: sevguest: Add TSM_REPORTS support for SNP_{GET, GET_EXT}_REPORT

[Huang, Kai]

On Sun, 2023-09-03 at 19:57 -0700, Kuppuswamy Sathyanarayanan wrote:
> 
> On 9/3/2023 7:14 PM, Huang, Kai wrote:
> > On Fri, 2023-09-01 at 09:38 -0700, Dan Williams wrote:
> > > > The extended guest request is another topic, since userspace has to be aware of
> > > > where the kernel choses to put the extended data, and fixup all the offsets in the
> > > > table (section 4.1.8.1 in [2]). It would be better to return this data through a
> > > > separate file.
> > > 
> > > I notice that the TDX report also includes a certificate blob, so if
> > > that is a common concept then yes, it makes sense to have a separate
> > > file for that.
> > 
> > + Sathy and Isaku.
> > 
> > It is a common concept from the perspective of "concept", because we need
> > certificates to verify the attestation blob anyway.  But in implementation,
> > unlike to SEV, TDX doesn't have a command to return certificates separately or
> > independently [1] -- they are embed to the Quote itself, or theoretically can be
> > fetched from Intel.  
> > 
> > More, for TDX (SGX based attestation) certificates blob itself isn't mandatory
> > to be part of the Quote.  Instead, TDX Quote can choose to include some more
> > basic platform identification which can in turn be used to get those
> > certificates from Intel's provisioning certificate service [2].
> > 
> > [1] I am not sure whether we can add one or already have one in the latest TDX
> > development.  Maybe Sathy or Isaku can help to confirm.
> > 
> > [2]: Table 9: QE Certification Data
> > https://download.01.org/intel-sgx/dcap-1.0.1/docs/Intel_SGX_ECDSA_QuoteGenReference_DCAP_API_Linux_1.0.1.pdf
> 
> Yes. TDX does not have any special command to fetch the certificate blob
> separately. Currently, it is fetched as part of Quote data. But, since the
> certificate blob is fixed per boot (unlike Quote data), I think it makes
> sense to add a separate command for it.
> 

I thought about this for a while, but I think we probably don't have enough
justification to do so.  Intel attestation userspace stack has already fully
adopted parsing Quote with the certificates blob, so I guess they just don't
have motivation to use the new interface.

However perhaps this shouldn't be a strong factor to impact whether kernel
should provide a separate file for certificates blob (or extended data in
general). If some vendor doesn't support such operation, I suppose we can just
return error when userspace accesses that file.

Re: [PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[Jeremi Piotrowski]

On 8/30/2023 9:33 PM, Dan Williams wrote:
> Note that I am sending this during the merge window due to the high
> level of interest. My current expectation, barring major review
> concerns, is that this intercepts Linux-next soon after v6.6-rc1 for a
> v6.7 merge. Given the switch to configfs I did not carry forward
> Reviewed-by's from v2.
> 
> Changes since v2 [1]:
> - Switch from sysfs to configfs to scale the interface for containers
>   (Jeremi)
> - Fix locking in outblob_read() to avoid racing freeing and generation
>   of ->outblob (Jeremi)
> - Add missing mutex to sev_report_new() (Jeremi)
> - Fix incorrect usage of no_free_ptr(), switch to return_ptr() (Peter)
> - Drop hex input parsing (configfs bin attributes are not seekable which
>   eliminates the concern) (Greg)
> - Note why DEFINE_FREE() for kvfree() includes a NULL check (Greg)
> - Document the permissible values of privlevel in the ABI documentation
>   (Greg)
> - Bump column limit to 100 for sev-guest changes, since that's existing
>   code, for tsm.c use the .clang_format default. (Tom)
> - Drop report buffer size to 4K (Tom)
> - Fix uninitialized variable @rc in register_tsm() (Tom)
> - Fix collision detection confusion, always increment write_generation
>   on successful write regardless if old data is being re-written (Tom)
> - Switch to sockptr_t for sharing {get,get_ext}_report() between the
>   ioctl and configfs paths (Andy)
> 
> [1]: http://lore.kernel.org/r/169199898909.1782217.10899362240465838600.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.

Besides the platform (cpu) attestation report, there are also attestation
reports from individual secure PCIe devices that we'd want to fetch. This
uses the SPDM protocol[1]. There is a CHALLENGE command which (too me)
roughly maps to an attestation request, but also separate interfaces to
fetch individual measurements and certificates (like the SNP extended
report interface allows).

If this is to become the one attestation interface then we'll need to
consider that. That will probably require adding a second level
directory: /sys/kernel/config/tsm/<device path>.

[1]: https://www.dmtf.org/sites/default/files/standards/documents/DSP2058_1.0.0_1.pdf

Re: [PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[Dan Williams]

[ Add Lukas since 'SPDM' was mentioned ]

Jeremi Piotrowski wrote:
[..]
> > 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.
> 
> Besides the platform (cpu) attestation report, there are also attestation
> reports from individual secure PCIe devices that we'd want to fetch. This
> uses the SPDM protocol[1]. There is a CHALLENGE command which (too me)
> roughly maps to an attestation request, but also separate interfaces to
> fetch individual measurements and certificates (like the SNP extended
> report interface allows).

Yes, but I am not yet convinced this configfs-tsm interface would get
involved there.

> 
> If this is to become the one attestation interface then we'll need to
> consider that. That will probably require adding a second level
> directory: /sys/kernel/config/tsm/<device path>.

The SPDM situation is different in my mind in that the kernel has an
interest in being able to attest a device itself. Think of cases like
power management where userspace is frozen, but the kernel needs to
validate the device in the resume flow.

For TVMs the kernel would validate devices and the verifying party would
validate the kernel as part of the guest measurement.

The main difficulty again here is evidence format differentiation. My
hope is that there is some standardization or otherwise a way to update
the kernel's verification logic for per-device evidence-formats.  Maybe
eBPF has a role to play in that story, but that's a converstation for a
different patch set.

> 
> [1]: https://www.dmtf.org/sites/default/files/standards/documents/DSP2058_1.0.0_1.pdf

Re: [PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[Samuel Ortiz]

On Fri, Sep 01, 2023 at 09:51:42AM -0700, Dan Williams wrote:
> [ Add Lukas since 'SPDM' was mentioned ]
> 
> Jeremi Piotrowski wrote:
> [..]
> > > 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.
> > 
> > Besides the platform (cpu) attestation report, there are also attestation
> > reports from individual secure PCIe devices that we'd want to fetch. This
> > uses the SPDM protocol[1]. There is a CHALLENGE command which (too me)
> > roughly maps to an attestation request, but also separate interfaces to
> > fetch individual measurements and certificates (like the SNP extended
> > report interface allows).
> 
> Yes, but I am not yet convinced this configfs-tsm interface would get
> involved there.

User space will want to get those devices attestation reports, and those
would be carried through the TSM. It would be nice to be able to use a
common ABI for this. A CPU/platform attestation report is not that
different from a device one.

> > 
> > If this is to become the one attestation interface then we'll need to
> > consider that. That will probably require adding a second level
> > directory: /sys/kernel/config/tsm/<device path>.
> 
> The SPDM situation is different in my mind in that the kernel has an
> interest in being able to attest a device itself. Think of cases like
> power management where userspace is frozen, but the kernel needs to
> validate the device in the resume flow.
> 
> For TVMs the kernel would validate devices

That means the TVM kernel would be provisioned with reference values and
policies that are likely to be tenant specific. The same TVM kernel,
running the same user space stack, getting the same PCIe device
attached, could either accept or reject such device, depending on the
tenant/workload owner policies and acceptable reference values. That
means each tenant would have to build its own guest images, and maintain
and update them with potentially each new device or new device stack it
wants to support.
Keeping most of the device attestation stack (similar to where the
platform attestation stack lives today) in user space seems more
flexible to me, and allows for tenant to use single guest images.

> and the verifying party would
> validate the kernel as part of the guest measurement.
> 
> The main difficulty again here is evidence format differentiation. My
> hope is that there is some standardization

FWIW there are IETF driven standardization efforts like e.g. EAT [1]
that go into the right directions imho. The latest CC implementations
(CCA, CoVE) follow those specs (EAT, CWT/JWT).

DMTF (driving the SPDM spec) defines also its own format, through its
measurement format spec. Device vendors may choose to implement that or
to e.g. add their EAT formatted attestation report in the reported
certificate chain. Realistically, we'll have to support all of those
flows.

> or otherwise a way to update
> the kernel's verification logic for per-device evidence-formats.  Maybe
> eBPF has a role to play in that story, but that's a converstation for a
> different patch set.

This conversation will hopefully include a user space architecture.
Potentially something we could talk about at the CC LPC microconference?

Cheers,
Samuel.

[1] https://datatracker.ietf.org/doc/draft-ietf-rats-eat/

Re: [PATCH v3 0/5] configfs-tsm: Attestation Report ABI

[Dan Williams]

Samuel Ortiz wrote:
> On Fri, Sep 01, 2023 at 09:51:42AM -0700, Dan Williams wrote:
> > [ Add Lukas since 'SPDM' was mentioned ]
> > 
> > Jeremi Piotrowski wrote:
> > [..]
> > > > 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.
> > > 
> > > Besides the platform (cpu) attestation report, there are also attestation
> > > reports from individual secure PCIe devices that we'd want to fetch. This
> > > uses the SPDM protocol[1]. There is a CHALLENGE command which (too me)
> > > roughly maps to an attestation request, but also separate interfaces to
> > > fetch individual measurements and certificates (like the SNP extended
> > > report interface allows).
> > 
> > Yes, but I am not yet convinced this configfs-tsm interface would get
> > involved there.
> 
> User space will want to get those devices attestation reports, and those
> would be carried through the TSM. It would be nice to be able to use a
> common ABI for this. A CPU/platform attestation report is not that
> different from a device one.
> 
> > > 
> > > If this is to become the one attestation interface then we'll need to
> > > consider that. That will probably require adding a second level
> > > directory: /sys/kernel/config/tsm/<device path>.
> > 
> > The SPDM situation is different in my mind in that the kernel has an
> > interest in being able to attest a device itself. Think of cases like
> > power management where userspace is frozen, but the kernel needs to
> > validate the device in the resume flow.
> > 
> > For TVMs the kernel would validate devices
> 
> That means the TVM kernel would be provisioned with reference values and
> policies that are likely to be tenant specific. The same TVM kernel,
> running the same user space stack, getting the same PCIe device
> attached, could either accept or reject such device, depending on the
> tenant/workload owner policies and acceptable reference values. That
> means each tenant would have to build its own guest images, and maintain
> and update them with potentially each new device or new device stack it
> wants to support.
> Keeping most of the device attestation stack (similar to where the
> platform attestation stack lives today) in user space seems more
> flexible to me, and allows for tenant to use single guest images.

I am not seeing how moving the verification mechanism into the kernel
makes this less flexible. For an analogy the kernel supports firewall
policy without requiring userspace network stacks.

Userspace still has a role play in dynamically updating device
validation policy, but the design is such that the kernel can be
independent of userspace in scenarios like power management where it is
awkward to round trip to userspace.

> 
> > and the verifying party would
> > validate the kernel as part of the guest measurement.
> > 
> > The main difficulty again here is evidence format differentiation. My
> > hope is that there is some standardization
> 
> FWIW there are IETF driven standardization efforts like e.g. EAT [1]
> that go into the right directions imho. The latest CC implementations
> (CCA, CoVE) follow those specs (EAT, CWT/JWT).
> 
> DMTF (driving the SPDM spec) defines also its own format, through its
> measurement format spec. Device vendors may choose to implement that or
> to e.g. add their EAT formatted attestation report in the reported
> certificate chain. Realistically, we'll have to support all of those
> flows.

Yes, but hopefully the differentiation here converges to a static or
slowly moving set of evidence types. If Linux makes it painful for new
evidence types to be invented that seems like a *good* thing from a
maintenance perspective.

> 
> > or otherwise a way to update
> > the kernel's verification logic for per-device evidence-formats.  Maybe
> > eBPF has a role to play in that story, but that's a converstation for a
> > different patch set.
> 
> This conversation will hopefully include a user space architecture.
> Potentially something we could talk about at the CC LPC microconference?

Definitely. I have been distracted from submitting a formal topic
proposal here, but I definitely think LPC is a chance to converge on a
direction here.