[PATCH v6 0/7] configfs-tsm: Attestation Report ABI

[PATCH v6 0/7] configfs-tsm: Attestation Report ABI

[Dan Williams]

Changes since v5 [1]:
- Dump the raw cert_table via a new @auxblob attribute rather than
  concatenate the certificate data. (Dionna and Tom)
- Fix usage of guard(), drop erroneous copy-pasted mutex_unlock().
  (Tom)

[1]: http://lore.kernel.org/r/169700203032.779347.11603484721811916604.stgit@dwillia2-xfh.jf.intel.com

---

Merge notes: I am looking for Dave or Boris to pick this up, I believe
all outstanding comments have been resolved and this has now been
smoke-tested on AMD and Intel platforms (both v5 and v6).

---

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 (6):
      virt: sevguest: Fix passing a stack buffer as a scatterlist target
      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_ext_report()
      mm/slab: Add __free() support for kvfree
      virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

Kuppuswamy Sathyanarayanan (1):
      virt: tdx-guest: Add Quote generation support using TSM_REPORTS


 Documentation/ABI/testing/configfs-tsm  |   82 ++++++
 MAINTAINERS                             |    8 +
 arch/x86/coco/tdx/tdx.c                 |   21 ++
 arch/x86/include/asm/shared/tdx.h       |    1 
 arch/x86/include/asm/tdx.h              |    2 
 drivers/virt/Kconfig                    |    6 
 drivers/virt/Makefile                   |    4 
 drivers/virt/coco/Kconfig               |   14 +
 drivers/virt/coco/Makefile              |    8 +
 drivers/virt/coco/sev-guest/Kconfig     |    1 
 drivers/virt/coco/sev-guest/sev-guest.c |  212 ++++++++++++++--
 drivers/virt/coco/tdx-guest/Kconfig     |    1 
 drivers/virt/coco/tdx-guest/tdx-guest.c |  229 +++++++++++++++++
 drivers/virt/coco/tsm.c                 |  423 +++++++++++++++++++++++++++++++
 include/linux/slab.h                    |    2 
 include/linux/tsm.h                     |   68 +++++
 16 files changed, 1046 insertions(+), 36 deletions(-)
 create mode 100644 Documentation/ABI/testing/configfs-tsm
 create mode 100644 drivers/virt/coco/Kconfig
 create mode 100644 drivers/virt/coco/Makefile
 create mode 100644 drivers/virt/coco/tsm.c
 create mode 100644 include/linux/tsm.h

base-commit: 6465e260f48790807eef06b583b38ca9789b6072

[PATCH v6 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target

[Dan Williams]

CONFIG_DEBUG_SG highlights that get_{report,ext_report,derived_key)()}
are passing stack buffers as the @req_buf argument to
handle_guest_request(), generating a Call Trace of the following form:

    WARNING: CPU: 0 PID: 1175 at include/linux/scatterlist.h:187 enc_dec_message+0x518/0x5b0 [sev_guest]
    [..]
    Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 07/26/2023
    RIP: 0010:enc_dec_message+0x518/0x5b0 [sev_guest]
    Call Trace:
     <TASK>
    [..]
     handle_guest_request+0x135/0x520 [sev_guest]
     get_ext_report+0x1ec/0x3e0 [sev_guest]
     snp_guest_ioctl+0x157/0x200 [sev_guest]

Note that the above Call Trace was with the DEBUG_SG BUG_ON()s converted
to WARN_ON()s.

This is benign as long as there are no hardware crypto accelerators
loaded for the aead cipher, and no subsequent dma_map_sg() is performed
on the scatterlist. However, sev-guest can not assume the presence of
an aead accelerator nor can it assume that CONFIG_DEBUG_SG is disabled.

Resolve this bug by allocating virt_addr_valid() memory, similar to the
other buffers am @snp_dev instance carries, to marshal requests from
user buffers to kernel buffers.

Reported-by: Peter Gonda <pgonda@google.com>
Closes: http://lore.kernel.org/r/CAMkAt6r2VPPMZ__SQfJse8qWsUyYW3AgYbOUVM0S_Vtk=KvkxQ@mail.gmail.com
Fixes: fce96cf04430 ("virt: Add SEV-SNP guest driver")
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c |   45 +++++++++++++++++--------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 97dbe715e96a..5bee58ef5f1e 100644
--- a/drivers/virt/coco/sev-guest/sev-guest.c
+++ b/drivers/virt/coco/sev-guest/sev-guest.c
@@ -57,6 +57,11 @@ struct snp_guest_dev {
 
 	struct snp_secrets_page_layout *layout;
 	struct snp_req_data input;
+	union {
+		struct snp_report_req report;
+		struct snp_derived_key_req derived_key;
+		struct snp_ext_report_req ext_report;
+	} req;
 	u32 *os_area_msg_seqno;
 	u8 *vmpck;
 };
@@ -473,8 +478,8 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
 {
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
+	struct snp_report_req *req = &snp_dev->req.report;
 	struct snp_report_resp *resp;
-	struct snp_report_req req;
 	int rc, resp_len;
 
 	lockdep_assert_held(&snp_cmd_mutex);
@@ -482,7 +487,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
 		return -EFAULT;
 
 	/*
@@ -496,7 +501,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 		return -ENOMEM;
 
 	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
-				  SNP_MSG_REPORT_REQ, &req, sizeof(req), resp->data,
+				  SNP_MSG_REPORT_REQ, req, sizeof(*req), resp->data,
 				  resp_len);
 	if (rc)
 		goto e_free;
@@ -511,9 +516,9 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io
 
 static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
 {
+	struct snp_derived_key_req *req = &snp_dev->req.derived_key;
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	struct snp_derived_key_resp resp = {0};
-	struct snp_derived_key_req req;
 	int rc, resp_len;
 	/* Response data is 64 bytes and max authsize for GCM is 16 bytes. */
 	u8 buf[64 + 16];
@@ -532,11 +537,11 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 	if (sizeof(buf) < resp_len)
 		return -ENOMEM;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
 		return -EFAULT;
 
 	rc = handle_guest_request(snp_dev, SVM_VMGEXIT_GUEST_REQUEST, arg,
-				  SNP_MSG_KEY_REQ, &req, sizeof(req), buf, resp_len);
+				  SNP_MSG_KEY_REQ, req, sizeof(*req), buf, resp_len);
 	if (rc)
 		return rc;
 
@@ -552,8 +557,8 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque
 
 static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg)
 {
+	struct snp_ext_report_req *req = &snp_dev->req.ext_report;
 	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;
 
@@ -562,18 +567,18 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	if (!arg->req_data || !arg->resp_data)
 		return -EINVAL;
 
-	if (copy_from_user(&req, (void __user *)arg->req_data, sizeof(req)))
+	if (copy_from_user(req, (void __user *)arg->req_data, sizeof(*req)))
 		return -EFAULT;
 
 	/* userspace does not want certificate data */
-	if (!req.certs_len || !req.certs_address)
+	if (!req->certs_len || !req->certs_address)
 		goto cmd;
 
-	if (req.certs_len > SEV_FW_BLOB_MAX_SIZE ||
-	    !IS_ALIGNED(req.certs_len, PAGE_SIZE))
+	if (req->certs_len > SEV_FW_BLOB_MAX_SIZE ||
+	    !IS_ALIGNED(req->certs_len, PAGE_SIZE))
 		return -EINVAL;
 
-	if (!access_ok((const void __user *)req.certs_address, req.certs_len))
+	if (!access_ok((const void __user *)req->certs_address, req->certs_len))
 		return -EFAULT;
 
 	/*
@@ -582,8 +587,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 	 * the host. If host does not supply any certs in it, then copy
 	 * zeros to indicate that certificate data was not provided.
 	 */
-	memset(snp_dev->certs_data, 0, req.certs_len);
-	npages = req.certs_len >> PAGE_SHIFT;
+	memset(snp_dev->certs_data, 0, req->certs_len);
+	npages = req->certs_len >> PAGE_SHIFT;
 cmd:
 	/*
 	 * The intermediate response buffer is used while decrypting the
@@ -597,14 +602,14 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 
 	snp_dev->input.data_npages = npages;
 	ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg,
-				   SNP_MSG_REPORT_REQ, &req.data,
-				   sizeof(req.data), resp->data, resp_len);
+				   SNP_MSG_REPORT_REQ, &req->data,
+				   sizeof(req->data), resp->data, resp_len);
 
 	/* If certs length is invalid then copy the returned length */
 	if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) {
-		req.certs_len = snp_dev->input.data_npages << PAGE_SHIFT;
+		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_user((void __user *)arg->req_data, req, sizeof(*req)))
 			ret = -EFAULT;
 	}
 
@@ -612,8 +617,8 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques
 		goto e_free;
 
 	if (npages &&
-	    copy_to_user((void __user *)req.certs_address, snp_dev->certs_data,
-			 req.certs_len)) {
+	    copy_to_user((void __user *)req->certs_address, snp_dev->certs_data,
+			 req->certs_len)) {
 		ret = -EFAULT;
 		goto e_free;
 	}

[PATCH v6 2/7] 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.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/Kconfig       |    6 +-----
 drivers/virt/Makefile      |    4 +---
 drivers/virt/coco/Kconfig  |    9 +++++++++
 drivers/virt/coco/Makefile |    7 +++++++
 4 files changed, 18 insertions(+), 8 deletions(-)
 create mode 100644 drivers/virt/coco/Kconfig
 create mode 100644 drivers/virt/coco/Makefile

diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index f79ab13a5c28..40129b6f0eca 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -48,10 +48,6 @@ source "drivers/virt/nitro_enclaves/Kconfig"
 
 source "drivers/virt/acrn/Kconfig"
 
-source "drivers/virt/coco/efi_secret/Kconfig"
-
-source "drivers/virt/coco/sev-guest/Kconfig"
-
-source "drivers/virt/coco/tdx-guest/Kconfig"
+source "drivers/virt/coco/Kconfig"
 
 endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index e9aa6fc96fab..f29901bd7820 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -9,6 +9,4 @@ obj-y				+= vboxguest/
 
 obj-$(CONFIG_NITRO_ENCLAVES)	+= nitro_enclaves/
 obj-$(CONFIG_ACRN_HSM)		+= acrn/
-obj-$(CONFIG_EFI_SECRET)	+= coco/efi_secret/
-obj-$(CONFIG_SEV_GUEST)		+= coco/sev-guest/
-obj-$(CONFIG_INTEL_TDX_GUEST)	+= coco/tdx-guest/
+obj-y				+= coco/
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
new file mode 100644
index 000000000000..fc5c64f04c4a
--- /dev/null
+++ b/drivers/virt/coco/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+source "drivers/virt/coco/efi_secret/Kconfig"
+
+source "drivers/virt/coco/sev-guest/Kconfig"
+
+source "drivers/virt/coco/tdx-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
new file mode 100644
index 000000000000..55302ef719ad
--- /dev/null
+++ b/drivers/virt/coco/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
+obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
+obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/

[PATCH v6 3/7] 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>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/ABI/testing/configfs-tsm |   82 ++++++
 MAINTAINERS                            |    8 +
 drivers/virt/coco/Kconfig              |    5 
 drivers/virt/coco/Makefile             |    1 
 drivers/virt/coco/tsm.c                |  423 ++++++++++++++++++++++++++++++++
 include/linux/tsm.h                    |   68 +++++
 6 files changed, 587 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..64eb4d937e48
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-tsm
@@ -0,0 +1,82 @@
+What:		/sys/kernel/config/tsm/report/$name/inblob
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(WO) Up to 64 bytes of user specified binary data. For replay
+		protection this should include a nonce, but the kernel does not
+		place any restrictions on the content.
+
+What:		/sys/kernel/config/tsm/report/$name/outblob
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) Binary attestation report generated from @inblob and other
+		options The format of the report is implementation specific
+		where the implementation is conveyed via the @provider
+		attribute.
+
+What:		/sys/kernel/config/tsm/report/$name/auxblob
+Date:		October, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) Optional supplemental data that a TSM may emit, visibility
+		of this attribute depends on TSM, and may be empty if no
+		auxiliary data is available.
+
+		When @provider is "sev-guest" this file contains the
+		"cert_table" from SEV-ES Guest-Hypervisor Communication Block
+		Standardization v2.03 Section 4.1.8.1 MSG_REPORT_REQ.
+		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf
+
+What:		/sys/kernel/config/tsm/report/$name/provider
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) A name for the format-specification of @outblob like
+		"sev-guest" [1]  or "tdx-guest" [2] in the near term, or a
+		common standard format in the future.
+
+		[1]: SEV Secure Nested Paging Firmware ABI Specification
+		Revision 1.55 Table 22
+		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
+
+		[2]: Intel® Trust Domain Extensions Data Center Attestation
+		Primitives : Quote Generation Library and Quote Verification
+		Library Revision 0.8 Appendix 4,5
+		https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
+
+What:		/sys/kernel/config/tsm/report/$name/generation
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) The value in this attribute increments each time @inblob or
+		any option is written. Userspace can detect conflicts by
+		checking generation before writing to any attribute and making
+		sure the number of writes matches expectations after reading
+		@outblob, or it can prevent conflicts by creating a report
+		instance per requesting context.
+
+What:		/sys/kernel/config/tsm/report/$name/privlevel
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(WO) Attribute is visible if a TSM implementation provider
+		supports the concept of attestation reports for TVMs running at
+		different privilege levels, like SEV-SNP "VMPL", specify the
+		privilege level via this attribute.  The minimum acceptable
+		value is conveyed via @privlevel_floor and the maximum
+		acceptable value is TSM_PRIVLEVEL_MAX (3).
+
+What:		/sys/kernel/config/tsm/report/$name/privlevel_floor
+Date:		September, 2023
+KernelVersion:	v6.7
+Contact:	linux-coco@lists.linux.dev
+Description:
+		(RO) Indicates the minimum permissible value that can be written
+		to @privlevel.
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..8acbeb029ba1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21889,6 +21889,14 @@ W:	https://github.com/srcres258/linux-doc
 T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
 F:	Documentation/translations/zh_TW/
 
+TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
+M:	Dan Williams <dan.j.williams@intel.com>
+L:	linux-coco@lists.linux.dev
+S:	Maintained
+F:	Documentation/ABI/testing/configfs-tsm
+F:	drivers/virt/coco/tsm.c
+F:	include/linux/tsm.h
+
 TTY LAYER AND SERIAL DRIVERS
 M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
 M:	Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index fc5c64f04c4a..87d142c1f932 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -2,6 +2,11 @@
 #
 # Confidential computing related collateral
 #
+
+config TSM_REPORTS
+	select CONFIGFS_FS
+	tristate
+
 source "drivers/virt/coco/efi_secret/Kconfig"
 
 source "drivers/virt/coco/sev-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index 55302ef719ad..18c1aba5edb7 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -2,6 +2,7 @@
 #
 # Confidential computing related collateral
 #
+obj-$(CONFIG_TSM_REPORTS)	+= tsm.o
 obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
 obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
 obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
new file mode 100644
index 000000000000..0200a86f1efe
--- /dev/null
+++ b/drivers/virt/coco/tsm.c
@@ -0,0 +1,423 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/tsm.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/rwsem.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/cleanup.h>
+#include <linux/configfs.h>
+
+static struct tsm_provider {
+	const struct tsm_ops *ops;
+	const struct config_item_type *type;
+	void *data;
+} provider;
+static DECLARE_RWSEM(tsm_rwsem);
+
+/**
+ * DOC: Trusted Security Module (TSM) Attestation Report Interface
+ *
+ * The TSM report interface is a common provider of blobs that facilitate
+ * attestation of a TVM (confidential computing guest) by an attestation
+ * service. A TSM report combines a user-defined blob (likely a public-key with
+ * a nonce for a key-exchange protocol) with a signed attestation report. That
+ * combined blob is then used to obtain secrets provided by an agent that can
+ * validate the attestation report. The expectation is that this interface is
+ * invoked infrequently, however configfs allows for multiple agents to
+ * own their own report generation instances to generate reports as
+ * often as needed.
+ *
+ * The attestation report format is TSM provider specific, when / if a standard
+ * materializes that can be published instead of the vendor layout. Until then
+ * the 'provider' attribute indicates the format of 'outblob', and optionally
+ * 'auxblob'.
+ */
+
+struct tsm_report_state {
+	struct tsm_report report;
+	unsigned long write_generation;
+	unsigned long read_generation;
+	struct config_item cfg;
+};
+
+enum tsm_data_select {
+	TSM_REPORT,
+	TSM_CERTS,
+};
+
+static struct tsm_report *to_tsm_report(struct config_item *cfg)
+{
+	struct tsm_report_state *state =
+		container_of(cfg, struct tsm_report_state, cfg);
+
+	return &state->report;
+}
+
+static struct tsm_report_state *to_state(struct tsm_report *report)
+{
+	return container_of(report, struct tsm_report_state, report);
+}
+
+static int try_advance_write_generation(struct tsm_report *report)
+{
+	struct tsm_report_state *state = to_state(report);
+
+	lockdep_assert_held_write(&tsm_rwsem);
+
+	/*
+	 * Malicious or broken userspace has written enough times for
+	 * read_generation == write_generation by modular arithmetic without an
+	 * interim read. Stop accepting updates until the current report
+	 * configuration is read.
+	 */
+	if (state->write_generation == state->read_generation - 1)
+		return -EBUSY;
+	state->write_generation++;
+	return 0;
+}
+
+static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
+					  const char *buf, size_t len)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	unsigned int val;
+	int rc;
+
+	rc = kstrtouint(buf, 0, &val);
+	if (rc)
+		return rc;
+
+	/*
+	 * The valid privilege levels that a TSM might accept, if it accepts a
+	 * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
+	 * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
+	 * than 0.
+	 */
+	if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
+		return -EINVAL;
+
+	guard(rwsem_write)(&tsm_rwsem);
+	rc = try_advance_write_generation(report);
+	if (rc)
+		return rc;
+	report->desc.privlevel = val;
+
+	return len;
+}
+CONFIGFS_ATTR_WO(tsm_report_, privlevel);
+
+static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
+					       char *buf)
+{
+	guard(rwsem_read)(&tsm_rwsem);
+	return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
+}
+CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
+
+static ssize_t tsm_report_inblob_write(struct config_item *cfg,
+				       const void *buf, size_t count)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	int rc;
+
+	guard(rwsem_write)(&tsm_rwsem);
+	rc = try_advance_write_generation(report);
+	if (rc)
+		return rc;
+
+	report->desc.inblob_len = count;
+	memcpy(report->desc.inblob, buf, count);
+	return count;
+}
+CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
+
+static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	struct tsm_report_state *state = to_state(report);
+
+	guard(rwsem_read)(&tsm_rwsem);
+	return sysfs_emit(buf, "%lu\n", state->write_generation);
+}
+CONFIGFS_ATTR_RO(tsm_report_, generation);
+
+static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
+{
+	guard(rwsem_read)(&tsm_rwsem);
+	return sysfs_emit(buf, "%s\n", provider.ops->name);
+}
+CONFIGFS_ATTR_RO(tsm_report_, provider);
+
+static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
+			     enum tsm_data_select select)
+{
+	loff_t offset = 0;
+	ssize_t len;
+	u8 *out;
+
+	if (select == TSM_REPORT) {
+		out = report->outblob;
+		len = report->outblob_len;
+	} else {
+		out = report->auxblob;
+		len = report->auxblob_len;
+	}
+
+	/*
+	 * Recall that a NULL @buf is configfs requesting the size of
+	 * the buffer.
+	 */
+	if (!buf)
+		return len;
+	return memory_read_from_buffer(buf, count, &offset, out, len);
+}
+
+static ssize_t read_cached_report(struct tsm_report *report, void *buf,
+				  size_t count, enum tsm_data_select select)
+{
+	struct tsm_report_state *state = to_state(report);
+
+	guard(rwsem_read)(&tsm_rwsem);
+	if (!report->desc.inblob_len)
+		return -EINVAL;
+
+	/*
+	 * A given TSM backend always fills in ->outblob regardless of
+	 * whether the report includes an auxblob or not.
+	 */
+	if (!report->outblob ||
+	    state->read_generation != state->write_generation)
+		return -EWOULDBLOCK;
+
+	return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
+			       size_t count, enum tsm_data_select select)
+{
+	struct tsm_report_state *state = to_state(report);
+	const struct tsm_ops *ops;
+	ssize_t rc;
+
+	/* try to read from the existing report if present and valid... */
+	rc = read_cached_report(report, buf, count, select);
+	if (rc >= 0 || rc != -EWOULDBLOCK)
+		return rc;
+
+	/* slow path, report may need to be regenerated... */
+	guard(rwsem_write)(&tsm_rwsem);
+	ops = provider.ops;
+	if (!report->desc.inblob_len)
+		return -EINVAL;
+
+	/* did another thread already generate this report? */
+	if (report->outblob &&
+	    state->read_generation == state->write_generation)
+		goto out;
+
+	kvfree(report->outblob);
+	kvfree(report->auxblob);
+	report->outblob = NULL;
+	report->auxblob = NULL;
+	rc = ops->report_new(report, provider.data);
+	if (rc < 0)
+		return rc;
+	state->read_generation = state->write_generation;
+out:
+	return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
+				       size_t count)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+
+	return tsm_report_read(report, buf, count, TSM_REPORT);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
+
+static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
+				       size_t count)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+
+	return tsm_report_read(report, buf, count, TSM_CERTS);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, 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,
+};
+
+#define TSM_DEFAULT_BIN_ATTRS() \
+	&tsm_report_attr_inblob, \
+	&tsm_report_attr_outblob
+
+static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
+	TSM_DEFAULT_BIN_ATTRS(),
+	NULL,
+};
+
+static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
+	TSM_DEFAULT_BIN_ATTRS(),
+	&tsm_report_attr_auxblob,
+	NULL,
+};
+
+static struct configfs_attribute *tsm_report_extra_attrs[] = {
+	TSM_DEFAULT_ATTRS(),
+	&tsm_report_attr_privlevel,
+	&tsm_report_attr_privlevel_floor,
+	NULL,
+};
+
+static void tsm_report_item_release(struct config_item *cfg)
+{
+	struct tsm_report *report = to_tsm_report(cfg);
+	struct tsm_report_state *state = to_state(report);
+
+	kvfree(report->auxblob);
+	kvfree(report->outblob);
+	kfree(state);
+}
+
+static struct configfs_item_operations tsm_report_item_ops = {
+	.release = tsm_report_item_release,
+};
+
+const struct config_item_type tsm_report_default_type = {
+	.ct_owner = THIS_MODULE,
+	.ct_bin_attrs = tsm_report_bin_attrs,
+	.ct_attrs = tsm_report_attrs,
+	.ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_default_type);
+
+const struct config_item_type tsm_report_ext_type = {
+	.ct_owner = THIS_MODULE,
+	.ct_bin_attrs = tsm_report_bin_extra_attrs,
+	.ct_attrs = tsm_report_extra_attrs,
+	.ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_ext_type);
+
+static struct config_item *tsm_report_make_item(struct config_group *group,
+						const char *name)
+{
+	struct tsm_report_state *state;
+
+	guard(rwsem_read)(&tsm_rwsem);
+	if (!provider.ops)
+		return ERR_PTR(-ENXIO);
+
+	state = kzalloc(sizeof(*state), GFP_KERNEL);
+	if (!state)
+		return ERR_PTR(-ENOMEM);
+
+	config_item_init_type_name(&state->cfg, name, provider.type);
+	return &state->cfg;
+}
+
+static struct configfs_group_operations tsm_report_group_ops = {
+	.make_item = tsm_report_make_item,
+};
+
+static const struct config_item_type tsm_reports_type = {
+	.ct_owner = THIS_MODULE,
+	.ct_group_ops = &tsm_report_group_ops,
+};
+
+static const struct config_item_type tsm_root_group_type = {
+	.ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem tsm_configfs = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "tsm",
+			.ci_type = &tsm_root_group_type,
+		},
+	},
+	.su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
+};
+
+static struct config_group *tsm_report_group;
+
+int tsm_register(const struct tsm_ops *ops, void *priv,
+		 const struct config_item_type *type)
+{
+	const struct tsm_ops *conflict;
+
+	if (!type)
+		type = &tsm_report_default_type;
+	if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
+		return -EINVAL;
+
+	guard(rwsem_write)(&tsm_rwsem);
+	conflict = provider.ops;
+	if (conflict) {
+		pr_err("\"%s\" ops already registered\n", conflict->name);
+		return -EBUSY;
+	}
+
+	provider.ops = ops;
+	provider.data = priv;
+	provider.type = type;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_register);
+
+int tsm_unregister(const struct tsm_ops *ops)
+{
+	guard(rwsem_write)(&tsm_rwsem);
+	if (ops != provider.ops)
+		return -EBUSY;
+	provider.ops = NULL;
+	provider.data = NULL;
+	provider.type = NULL;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_unregister);
+
+static int __init tsm_init(void)
+{
+	struct config_group *root = &tsm_configfs.su_group;
+	struct config_group *tsm;
+	int rc;
+
+	config_group_init(root);
+	rc = configfs_register_subsystem(&tsm_configfs);
+	if (rc)
+		return rc;
+
+	tsm = configfs_register_default_group(root, "report",
+					      &tsm_reports_type);
+	if (IS_ERR(tsm)) {
+		configfs_unregister_subsystem(&tsm_configfs);
+		return PTR_ERR(tsm);
+	}
+	tsm_report_group = tsm;
+
+	return 0;
+}
+module_init(tsm_init);
+
+static void __exit tsm_exit(void)
+{
+	configfs_unregister_default_group(tsm_report_group);
+	configfs_unregister_subsystem(&tsm_configfs);
+}
+module_exit(tsm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
new file mode 100644
index 000000000000..5fadc382064d
--- /dev/null
+++ b/include/linux/tsm.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __TSM_H
+#define __TSM_H
+
+#include <linux/sizes.h>
+#include <linux/types.h>
+#include <linux/device.h>
+
+#define TSM_INBLOB_MAX 64
+#define TSM_OUTBLOB_MAX SZ_32K
+
+/*
+ * Privilege level is a nested permission concept to allow confidential
+ * guests to partition address space, 4-levels are supported.
+ */
+#define TSM_PRIVLEVEL_MAX 3
+
+/**
+ * struct tsm_desc - option descriptor for generating tsm report blobs
+ * @privlevel: optional privilege level to associate with @outblob
+ * @inblob_len: sizeof @inblob
+ * @inblob: arbitrary input data
+ */
+struct tsm_desc {
+	unsigned int privlevel;
+	size_t inblob_len;
+	u8 inblob[TSM_INBLOB_MAX];
+};
+
+/**
+ * struct tsm_report - track state of report generation relative to options
+ * @desc: input parameters to @report_new()
+ * @outblob_len: sizeof(@outblob)
+ * @outblob: generated evidence to provider to the attestation agent
+ * @auxblob_len: sizeof(@auxblob)
+ * @auxblob: (optional) auxiliary data to the report (e.g. certificate data)
+ */
+struct tsm_report {
+	struct tsm_desc desc;
+	size_t outblob_len;
+	u8 *outblob;
+	size_t auxblob_len;
+	u8 *auxblob;
+};
+
+/**
+ * struct tsm_ops - attributes and operations for tsm instances
+ * @name: tsm id reflected in /sys/kernel/config/tsm/report/$report/provider
+ * @privlevel_floor: convey base privlevel for nested scenarios
+ * @report_new: Populate @report with the report blob and auxblob
+ * (optional), return 0 on successful population, or -errno otherwise
+ *
+ * Implementation specific ops, only one is expected to be registered at
+ * a time i.e. only one of "sev-guest", "tdx-guest", etc.
+ */
+struct tsm_ops {
+	const char *name;
+	const int privlevel_floor;
+	int (*report_new)(struct tsm_report *report, void *data);
+};
+
+extern const struct config_item_type tsm_report_ext_type;
+extern const struct config_item_type tsm_report_default_type;
+
+int tsm_register(const struct tsm_ops *ops, void *priv,
+		 const struct config_item_type *type);
+int tsm_unregister(const struct tsm_ops *ops);
+#endif /* __TSM_H */

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dionna Amalie Glaze]

> +What:          /sys/kernel/config/tsm/report/$name/privlevel
> +Date:          September, 2023
> +KernelVersion: v6.7
> +Contact:       linux-coco@lists.linux.dev
> +Description:
> +               (WO) Attribute is visible if a TSM implementation provider
> +               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).
> +

I'm unaware of another CC technology that has different privilege
levels at which to request an attestation, but I'd be much happier to
see another example here.

I take it my feedback about VLEK vs VCEK selection is going to be left
for a future patch series? I can drop it if we can agree another WO
attribute for that won't be met with a lot of barriers, but I think we
may be generalizing a single data point with the privlevel and key
selection attributes.

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

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Dionna Amalie Glaze wrote:
> > +What:          /sys/kernel/config/tsm/report/$name/privlevel
> > +Date:          September, 2023
> > +KernelVersion: v6.7
> > +Contact:       linux-coco@lists.linux.dev
> > +Description:
> > +               (WO) Attribute is visible if a TSM implementation provider
> > +               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).
> > +
> 
> I'm unaware of another CC technology that has different privilege
> levels at which to request an attestation, but I'd be much happier to
> see another example here.

I am also unaware of such a CC definition, but the concept seems
generally powerful that I feel (speaking only for myself) it may see
adoption. If that happens it would need to be compatible with this ABI
definition.  In the meantime this @privlevel attribute only shows up
when @provider is "sev-guest" to not clutter other implementations.

However, if another CC platform never adopts this concept then this will
be another casualty of the early stage vendor divergence of these
interfaces.  The fact that these files are called "blob" and the
Documentation points to per @provider specifications is also evidence of
that. I hope over time the @*blob files become deprecated in favor of
common files with standard formats, but that too remains to be seen.

> I take it my feedback about VLEK vs VCEK selection is going to be left
> for a future patch series? I can drop it if we can agree another WO
> attribute for that won't be met with a lot of barriers, but I think we
> may be generalizing a single data point with the privlevel and key
> selection attributes.

Apologies, I should have addressed key selection in the cover letter. In
general, yes, I think it can be handled with a follow-on patchset, and
from your description it is not clear to me that this needs to be
promoted to configfs-tsm ABI. When you say:

> This is more of a customer-wide policy or machine pool policy.

It makes me think it is amenable to be solved via other means that do
not immediately implicate configfs-tsm ABI. For example KEY_SEL==0 is
defined as:

"If VLEK is installed, sign with VLEK. Otherwise, sign with VCEK."

...so theoretically the machine pool policy could be determined by which
certificate is installed. The customer-wide policy could perhaps be a
sev-guest driver compile-time decision, it need not be something that
each invocation of report generation interface needs to be worried
about.

As for:

> ...if we can agree another WO attribute for that won't be met with a
> lot of barriers

I think peak-level barrier was when I suggested that tdx-guest not
follow the precedent of new per-vendor ioctl() ABI. If we can overcome
that then the relative angst for a KEY_SEL discussion is low.

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Alexey Kardashevskiy]

On 13/10/23 13:14, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its launch state, sign it and
> submit it to a verifying party. Upon successful attestation that
> verifies the integrity of the TVM additional secrets may be deployed.
> The concept is common across TSMs, but the implementations are
> unfortunately vendor specific. While the industry grapples with a common
> definition of this attestation format [1], Linux need not make this
> problem worse by defining a new ABI per TSM that wants to perform a
> similar operation. The current momentum has been to invent new ioctl-ABI
> per TSM per function which at best is an abdication of the kernel's
> responsibility to make common infrastructure concepts share common ABI.
> 
> The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
> not more, is to define a configfs interface to retrieve the TSM-specific
> blob.
> 
>      report=/sys/kernel/config/tsm/report/report0
>      mkdir $report
>      dd if=binary_userdata_plus_nonce > $report/inblob
>      hexdump $report/outblob
> 
> This approach later allows for the standardization of the attestation
> blob format without needing to invent a new ABI. Once standardization
> happens the standard format can be emitted by $report/outblob and
> indicated by $report/provider, or a new attribute like
> "$report/tcg_coco_report" can emit the standard format alongside the
> vendor format.
> 
> Review of previous iterations of this interface identified that there is
> a need to scale report generation for multiple container environments
> [2]. Configfs enables a model where each container can bind mount one or
> more report generation item instances. Still, within a container only a
> single thread can be manipulating a given configuration instance at a
> time. A 'generation' count is provided to detect conflicts between
> multiple threads racing to configure a report instance.
> 
> The SEV-SNP concepts of "extended reports" and "privilege levels" are
> optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> time. The expectation is that those concepts are generic enough that
> they may be adopted by other TSM implementations. In other words,
> configfs-tsm aims to address a superset of TSM specific functionality
> with a common ABI where attributes may appear, or not appear, based on
> the set of concepts the implementation supports.
> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
>   Documentation/ABI/testing/configfs-tsm |   82 ++++++
>   MAINTAINERS                            |    8 +
>   drivers/virt/coco/Kconfig              |    5
>   drivers/virt/coco/Makefile             |    1
>   drivers/virt/coco/tsm.c                |  423 ++++++++++++++++++++++++++++++++
>   include/linux/tsm.h                    |   68 +++++
>   6 files changed, 587 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..64eb4d937e48
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-tsm
> @@ -0,0 +1,82 @@
> +What:		/sys/kernel/config/tsm/report/$name/inblob
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(WO) Up to 64 bytes of user specified binary data. For replay
> +		protection this should include a nonce, but the kernel does not
> +		place any restrictions on the content.
> +
> +What:		/sys/kernel/config/tsm/report/$name/outblob
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Binary attestation report generated from @inblob and other
> +		options The format of the report is implementation specific
> +		where the implementation is conveyed via the @provider
> +		attribute.
> +
> +What:		/sys/kernel/config/tsm/report/$name/auxblob
> +Date:		October, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Optional supplemental data that a TSM may emit, visibility
> +		of this attribute depends on TSM, and may be empty if no
> +		auxiliary data is available.
> +
> +		When @provider is "sev-guest" this file contains the
> +		"cert_table" from SEV-ES Guest-Hypervisor Communication Block
> +		Standardization v2.03 Section 4.1.8.1 MSG_REPORT_REQ.
> +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56421.pdf
> +
> +What:		/sys/kernel/config/tsm/report/$name/provider
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) A name for the format-specification of @outblob like
> +		"sev-guest" [1]  or "tdx-guest" [2] in the near term, or a
> +		common standard format in the future.

Nit: /sys/kernel/config/tsm/report/report0/provider contains 
"sev_guest", i.e. "_", not "-".

> +
> +		[1]: SEV Secure Nested Paging Firmware ABI Specification
> +		Revision 1.55 Table 22
> +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
> +
> +		[2]: Intel® Trust Domain Extensions Data Center Attestation
> +		Primitives : Quote Generation Library and Quote Verification
> +		Library Revision 0.8 Appendix 4,5
> +		https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
> +
> +What:		/sys/kernel/config/tsm/report/$name/generation
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) The value in this attribute increments each time @inblob or
> +		any option is written. Userspace can detect conflicts by
> +		checking generation before writing to any attribute and making
> +		sure the number of writes matches expectations after reading
> +		@outblob, or it can prevent conflicts by creating a report
> +		instance per requesting context.
> +
> +What:		/sys/kernel/config/tsm/report/$name/privlevel
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(WO) Attribute is visible if a TSM implementation provider
> +		supports the concept of attestation reports for TVMs running at
> +		different privilege levels, like SEV-SNP "VMPL", specify the
> +		privilege level via this attribute.  The minimum acceptable
> +		value is conveyed via @privlevel_floor and the maximum
> +		acceptable value is TSM_PRIVLEVEL_MAX (3).
> +
> +What:		/sys/kernel/config/tsm/report/$name/privlevel_floor
> +Date:		September, 2023
> +KernelVersion:	v6.7
> +Contact:	linux-coco@lists.linux.dev
> +Description:
> +		(RO) Indicates the minimum permissible value that can be written
> +		to @privlevel.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..8acbeb029ba1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21889,6 +21889,14 @@ W:	https://github.com/srcres258/linux-doc
>   T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
>   F:	Documentation/translations/zh_TW/
>   
> +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
> +M:	Dan Williams <dan.j.williams@intel.com>
> +L:	linux-coco@lists.linux.dev
> +S:	Maintained
> +F:	Documentation/ABI/testing/configfs-tsm
> +F:	drivers/virt/coco/tsm.c
> +F:	include/linux/tsm.h
> +
>   TTY LAYER AND SERIAL DRIVERS
>   M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>   M:	Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index fc5c64f04c4a..87d142c1f932 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -2,6 +2,11 @@
>   #
>   # Confidential computing related collateral
>   #
> +
> +config TSM_REPORTS
> +	select CONFIGFS_FS
> +	tristate
> +
>   source "drivers/virt/coco/efi_secret/Kconfig"
>   
>   source "drivers/virt/coco/sev-guest/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index 55302ef719ad..18c1aba5edb7 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -2,6 +2,7 @@
>   #
>   # Confidential computing related collateral
>   #
> +obj-$(CONFIG_TSM_REPORTS)	+= tsm.o
>   obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
>   obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
>   obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> new file mode 100644
> index 000000000000..0200a86f1efe
> --- /dev/null
> +++ b/drivers/virt/coco/tsm.c
> @@ -0,0 +1,423 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/rwsem.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/configfs.h>
> +
> +static struct tsm_provider {
> +	const struct tsm_ops *ops;
> +	const struct config_item_type *type;
> +	void *data;
> +} provider;
> +static DECLARE_RWSEM(tsm_rwsem);
> +
> +/**
> + * DOC: Trusted Security Module (TSM) Attestation Report Interface
> + *
> + * The TSM report interface is a common provider of blobs that facilitate
> + * attestation of a TVM (confidential computing guest) by an attestation
> + * service. A TSM report combines a user-defined blob (likely a public-key with
> + * a nonce for a key-exchange protocol) with a signed attestation report. That
> + * combined blob is then used to obtain secrets provided by an agent that can
> + * validate the attestation report. The expectation is that this interface is
> + * invoked infrequently, however configfs allows for multiple agents to
> + * own their own report generation instances to generate reports as
> + * often as needed.
> + *
> + * The attestation report format is TSM provider specific, when / if a standard
> + * materializes that can be published instead of the vendor layout. Until then
> + * the 'provider' attribute indicates the format of 'outblob', and optionally
> + * 'auxblob'.
> + */
> +
> +struct tsm_report_state {
> +	struct tsm_report report;
> +	unsigned long write_generation;
> +	unsigned long read_generation;
> +	struct config_item cfg;
> +};
> +
> +enum tsm_data_select {
> +	TSM_REPORT,
> +	TSM_CERTS,
> +};
> +
> +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> +{
> +	struct tsm_report_state *state =
> +		container_of(cfg, struct tsm_report_state, cfg);

This be one line of 88 chars (less than allowed 100).

(I'll comment once on this, feel free to ignore :) )

> +
> +	return &state->report;
> +}
> +
> +static struct tsm_report_state *to_state(struct tsm_report *report)
> +{
> +	return container_of(report, struct tsm_report_state, report);
> +}
> +
> +static int try_advance_write_generation(struct tsm_report *report)
> +{
> +	struct tsm_report_state *state = to_state(report);
> +
> +	lockdep_assert_held_write(&tsm_rwsem);
> +
> +	/*
> +	 * Malicious or broken userspace has written enough times for
> +	 * read_generation == write_generation by modular arithmetic without an
> +	 * interim read. Stop accepting updates until the current report
> +	 * configuration is read.
> +	 */
> +	if (state->write_generation == state->read_generation - 1)
> +		return -EBUSY;
> +	state->write_generation++;
> +	return 0;
> +}
> +
> +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> +					  const char *buf, size_t len)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	unsigned int val;
> +	int rc;
> +
> +	rc = kstrtouint(buf, 0, &val);
> +	if (rc)
> +		return rc;
> +
> +	/*
> +	 * The valid privilege levels that a TSM might accept, if it accepts a
> +	 * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
> +	 * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
> +	 * than 0.
> +	 */

Sounds like privlevel_floor should be "unsigned int" rather than "int".


> +	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);

%d  or  change the type.

> +}
> +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
> +
> +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> +				       const void *buf, size_t count)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	int rc;
> +
> +	guard(rwsem_write)(&tsm_rwsem);
> +	rc = try_advance_write_generation(report);
> +	if (rc)
> +		return rc;
> +
> +	report->desc.inblob_len = count;
> +	memcpy(report->desc.inblob, buf, count);
> +	return count;
> +}
> +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
> +
> +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	struct tsm_report_state *state = to_state(report);
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	return sysfs_emit(buf, "%lu\n", state->write_generation);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, generation);
> +
> +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
> +{
> +	guard(rwsem_read)(&tsm_rwsem);
> +	return sysfs_emit(buf, "%s\n", provider.ops->name);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, provider);
> +
> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> +			     enum tsm_data_select select)
> +{
> +	loff_t offset = 0;
> +	ssize_t len;
> +	u8 *out;
> +
> +	if (select == TSM_REPORT) {
> +		out = report->outblob;
> +		len = report->outblob_len;
> +	} else {
> +		out = report->auxblob;
> +		len = report->auxblob_len;
> +	}
> +
> +	/*
> +	 * Recall that a NULL @buf is configfs requesting the size of
> +	 * the buffer.
> +	 */

The comment can be one line (or even dropped as it is configfs api).

> +	if (!buf)
> +		return len;
> +	return memory_read_from_buffer(buf, count, &offset, out, len);
> +}
> +
> +static ssize_t read_cached_report(struct tsm_report *report, void *buf,
> +				  size_t count, enum tsm_data_select select)
> +{
> +	struct tsm_report_state *state = to_state(report);
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	if (!report->desc.inblob_len)
> +		return -EINVAL;
> +
> +	/*
> +	 * A given TSM backend always fills in ->outblob regardless of
> +	 * whether the report includes an auxblob or not.
> +	 */
> +	if (!report->outblob ||
> +	    state->read_generation != state->write_generation)
> +		return -EWOULDBLOCK;
> +
> +	return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> +			       size_t count, enum tsm_data_select select)
> +{
> +	struct tsm_report_state *state = to_state(report);
> +	const struct tsm_ops *ops;
> +	ssize_t rc;
> +
> +	/* try to read from the existing report if present and valid... */
> +	rc = read_cached_report(report, buf, count, select);
> +	if (rc >= 0 || rc != -EWOULDBLOCK)
> +		return rc;
> +
> +	/* slow path, report may need to be regenerated... */
> +	guard(rwsem_write)(&tsm_rwsem);
> +	ops = provider.ops;
> +	if (!report->desc.inblob_len)
> +		return -EINVAL;
> +
> +	/* did another thread already generate this report? */
> +	if (report->outblob &&
> +	    state->read_generation == state->write_generation)
> +		goto out;
> +	kvfree(report->outblob);
> +	kvfree(report->auxblob);
> +	report->outblob = NULL;
> +	report->auxblob = NULL;
> +	rc = ops->report_new(report, provider.data);


Drop @ops and use "provider.ops" here?

> +	if (rc < 0)
> +		return rc;
> +	state->read_generation = state->write_generation;
> +out:
> +	return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> +				       size_t count)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +
> +	return tsm_report_read(report, buf, count, TSM_REPORT);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
> +
> +static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
> +				       size_t count)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +
> +	return tsm_report_read(report, buf, count, TSM_CERTS);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
> +
> +#define TSM_DEFAULT_ATTRS() \

imho this one and TSM_DEFAULT_BIN_ATTRS are not really helping with 
readability or a size :)

> +	&tsm_report_attr_generation, \
> +	&tsm_report_attr_provider
> +
> +static struct configfs_attribute *tsm_report_attrs[] = {
> +	TSM_DEFAULT_ATTRS(),
> +	NULL,
> +};
> +
> +#define TSM_DEFAULT_BIN_ATTRS() \
> +	&tsm_report_attr_inblob, \
> +	&tsm_report_attr_outblob
> +
> +static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
> +	TSM_DEFAULT_BIN_ATTRS(),
> +	NULL,
> +};
> +
> +static struct configfs_bin_attribute *tsm_report_bin_extra_attrs[] = {
> +	TSM_DEFAULT_BIN_ATTRS(),
> +	&tsm_report_attr_auxblob,
> +	NULL,
> +};
> +
> +static struct configfs_attribute *tsm_report_extra_attrs[] = {
> +	TSM_DEFAULT_ATTRS(),
> +	&tsm_report_attr_privlevel,
> +	&tsm_report_attr_privlevel_floor,
> +	NULL,
> +};
> +
> +static void tsm_report_item_release(struct config_item *cfg)
> +{
> +	struct tsm_report *report = to_tsm_report(cfg);
> +	struct tsm_report_state *state = to_state(report);
> +
> +	kvfree(report->auxblob);
> +	kvfree(report->outblob);
> +	kfree(state);
> +}
> +
> +static struct configfs_item_operations tsm_report_item_ops = {
> +	.release = tsm_report_item_release,
> +};
> +
> +const struct config_item_type tsm_report_default_type = {
> +	.ct_owner = THIS_MODULE,
> +	.ct_bin_attrs = tsm_report_bin_attrs,
> +	.ct_attrs = tsm_report_attrs,
> +	.ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_default_type);
> +
> +const struct config_item_type tsm_report_ext_type = {
> +	.ct_owner = THIS_MODULE,
> +	.ct_bin_attrs = tsm_report_bin_extra_attrs,
> +	.ct_attrs = tsm_report_extra_attrs,
> +	.ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_ext_type);
> +
> +static struct config_item *tsm_report_make_item(struct config_group *group,
> +						const char *name)
> +{
> +	struct tsm_report_state *state;
> +
> +	guard(rwsem_read)(&tsm_rwsem);
> +	if (!provider.ops)
> +		return ERR_PTR(-ENXIO);
> +
> +	state = kzalloc(sizeof(*state), GFP_KERNEL);
> +	if (!state)
> +		return ERR_PTR(-ENOMEM);
> +
> +	config_item_init_type_name(&state->cfg, name, provider.type);
> +	return &state->cfg;
> +}
> +
> +static struct configfs_group_operations tsm_report_group_ops = {
> +	.make_item = tsm_report_make_item,
> +};
> +
> +static const struct config_item_type tsm_reports_type = {
> +	.ct_owner = THIS_MODULE,
> +	.ct_group_ops = &tsm_report_group_ops,
> +};
> +
> +static const struct config_item_type tsm_root_group_type = {
> +	.ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem tsm_configfs = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_namebuf = "tsm",
> +			.ci_type = &tsm_root_group_type,
> +		},
> +	},
> +	.su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
> +};
> +
> +static struct config_group *tsm_report_group;

A little confusing thing is that this guy is neither near the beginning 
of the file (with other statics, this could even be a member of 
tsm_provider) nor the code which sets/clears it - tsm_init/tsm_unregister.


> +
> +int tsm_register(const struct tsm_ops *ops, void *priv,
> +		 const struct config_item_type *type)
> +{
> +	const struct tsm_ops *conflict;
> +
> +	if (!type)
> +		type = &tsm_report_default_type;
> +	if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
> +		return -EINVAL;
> +
> +	guard(rwsem_write)(&tsm_rwsem);
> +	conflict = provider.ops;

Is a new local variable really needed here?

> +	if (conflict) {
> +		pr_err("\"%s\" ops already registered\n", conflict->name);
> +		return -EBUSY;
> +	}
> +
> +	provider.ops = ops;
> +	provider.data = priv;
> +	provider.type = type;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +int tsm_unregister(const struct tsm_ops *ops)
> +{
> +	guard(rwsem_write)(&tsm_rwsem);
> +	if (ops != provider.ops)
> +		return -EBUSY;
> +	provider.ops = NULL;
> +	provider.data = NULL;
> +	provider.type = NULL;
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
> +
> +static int __init tsm_init(void)
> +{
> +	struct config_group *root = &tsm_configfs.su_group;
> +	struct config_group *tsm;
> +	int rc;
> +
> +	config_group_init(root);
> +	rc = configfs_register_subsystem(&tsm_configfs);
> +	if (rc)
> +		return rc;
> +
> +	tsm = configfs_register_default_group(root, "report",
> +					      &tsm_reports_type);
> +	if (IS_ERR(tsm)) {
> +		configfs_unregister_subsystem(&tsm_configfs);
> +		return PTR_ERR(tsm);
> +	}
> +	tsm_report_group = tsm;
> +
> +	return 0;
> +}
> +module_init(tsm_init);
> +
> +static void __exit tsm_exit(void)
> +{
> +	configfs_unregister_default_group(tsm_report_group);
> +	configfs_unregister_subsystem(&tsm_configfs);
> +}
> +module_exit(tsm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> new file mode 100644
> index 000000000000..5fadc382064d
> --- /dev/null
> +++ b/include/linux/tsm.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __TSM_H
> +#define __TSM_H
> +
> +#include <linux/sizes.h>
> +#include <linux/types.h>
> +#include <linux/device.h>

device.h is not needed.

> +
> +#define TSM_INBLOB_MAX 64
> +#define TSM_OUTBLOB_MAX SZ_32K
> +
> +/*
> + * Privilege level is a nested permission concept to allow confidential
> + * guests to partition address space, 4-levels are supported.
> + */
> +#define TSM_PRIVLEVEL_MAX 3
> +
> +/**
> + * struct tsm_desc - option descriptor for generating tsm report blobs
> + * @privlevel: optional privilege level to associate with @outblob
> + * @inblob_len: sizeof @inblob
> + * @inblob: arbitrary input data
> + */
> +struct tsm_desc {
> +	unsigned int privlevel;
> +	size_t inblob_len;
> +	u8 inblob[TSM_INBLOB_MAX];
> +};
> +
> +/**
> + * struct tsm_report - track state of report generation relative to options
> + * @desc: input parameters to @report_new()
> + * @outblob_len: sizeof(@outblob)
> + * @outblob: generated evidence to provider to the attestation agent
> + * @auxblob_len: sizeof(@auxblob)
> + * @auxblob: (optional) auxiliary data to the report (e.g. certificate data)
> + */
> +struct tsm_report {
> +	struct tsm_desc desc;
> +	size_t outblob_len;
> +	u8 *outblob;
> +	size_t auxblob_len;
> +	u8 *auxblob;
> +};
> +
> +/**
> + * struct tsm_ops - attributes and operations for tsm instances
> + * @name: tsm id reflected in /sys/kernel/config/tsm/report/$report/provider
> + * @privlevel_floor: convey base privlevel for nested scenarios
> + * @report_new: Populate @report with the report blob and auxblob
> + * (optional), return 0 on successful population, or -errno otherwise
> + *
> + * Implementation specific ops, only one is expected to be registered at
> + * a time i.e. only one of "sev-guest", "tdx-guest", etc.
> + */
> +struct tsm_ops {
> +	const char *name;
> +	const int privlevel_floor;
> +	int (*report_new)(struct tsm_report *report, void *data);
> +};
> +
> +extern const struct config_item_type tsm_report_ext_type;
> +extern const struct config_item_type tsm_report_default_type;
> +
> +int tsm_register(const struct tsm_ops *ops, void *priv,
> +		 const struct config_item_type *type);
> +int tsm_unregister(const struct tsm_ops *ops);
> +#endif /* __TSM_H */
> 

-- 
Alexey

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Alexey Kardashevskiy wrote:
[..]
> > +What:		/sys/kernel/config/tsm/report/$name/provider
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) A name for the format-specification of @outblob like
> > +		"sev-guest" [1]  or "tdx-guest" [2] in the near term, or a
> > +		common standard format in the future.
> 
> Nit: /sys/kernel/config/tsm/report/report0/provider contains 
> "sev_guest", i.e. "_", not "-".

Yes, will fix either with a follow-on or a respin if more feedback
arrives.

> 
> > +
> > +		[1]: SEV Secure Nested Paging Firmware ABI Specification
> > +		Revision 1.55 Table 22
> > +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
> > +
> > +		[2]: Intel® Trust Domain Extensions Data Center Attestation
> > +		Primitives : Quote Generation Library and Quote Verification
> > +		Library Revision 0.8 Appendix 4,5
> > +		https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/generation
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) The value in this attribute increments each time @inblob or
> > +		any option is written. Userspace can detect conflicts by
> > +		checking generation before writing to any attribute and making
> > +		sure the number of writes matches expectations after reading
> > +		@outblob, or it can prevent conflicts by creating a report
> > +		instance per requesting context.
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/privlevel
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(WO) Attribute is visible if a TSM implementation provider
> > +		supports the concept of attestation reports for TVMs running at
> > +		different privilege levels, like SEV-SNP "VMPL", specify the
> > +		privilege level via this attribute.  The minimum acceptable
> > +		value is conveyed via @privlevel_floor and the maximum
> > +		acceptable value is TSM_PRIVLEVEL_MAX (3).
> > +
> > +What:		/sys/kernel/config/tsm/report/$name/privlevel_floor
> > +Date:		September, 2023
> > +KernelVersion:	v6.7
> > +Contact:	linux-coco@lists.linux.dev
> > +Description:
> > +		(RO) Indicates the minimum permissible value that can be written
> > +		to @privlevel.
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index b19995690904..8acbeb029ba1 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -21889,6 +21889,14 @@ W:	https://github.com/srcres258/linux-doc
> >   T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
> >   F:	Documentation/translations/zh_TW/
> >   
> > +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
> > +M:	Dan Williams <dan.j.williams@intel.com>
> > +L:	linux-coco@lists.linux.dev
> > +S:	Maintained
> > +F:	Documentation/ABI/testing/configfs-tsm
> > +F:	drivers/virt/coco/tsm.c
> > +F:	include/linux/tsm.h
> > +
> >   TTY LAYER AND SERIAL DRIVERS
> >   M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >   M:	Jiri Slaby <jirislaby@kernel.org>
> > diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> > index fc5c64f04c4a..87d142c1f932 100644
> > --- a/drivers/virt/coco/Kconfig
> > +++ b/drivers/virt/coco/Kconfig
> > @@ -2,6 +2,11 @@
> >   #
> >   # Confidential computing related collateral
> >   #
> > +
> > +config TSM_REPORTS
> > +	select CONFIGFS_FS
> > +	tristate
> > +
> >   source "drivers/virt/coco/efi_secret/Kconfig"
> >   
> >   source "drivers/virt/coco/sev-guest/Kconfig"
> > diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> > index 55302ef719ad..18c1aba5edb7 100644
> > --- a/drivers/virt/coco/Makefile
> > +++ b/drivers/virt/coco/Makefile
> > @@ -2,6 +2,7 @@
> >   #
> >   # Confidential computing related collateral
> >   #
> > +obj-$(CONFIG_TSM_REPORTS)	+= tsm.o
> >   obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
> >   obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
> >   obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
> > diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> > new file mode 100644
> > index 000000000000..0200a86f1efe
> > --- /dev/null
> > +++ b/drivers/virt/coco/tsm.c
> > @@ -0,0 +1,423 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/tsm.h>
> > +#include <linux/err.h>
> > +#include <linux/slab.h>
> > +#include <linux/rwsem.h>
> > +#include <linux/string.h>
> > +#include <linux/module.h>
> > +#include <linux/cleanup.h>
> > +#include <linux/configfs.h>
> > +
> > +static struct tsm_provider {
> > +	const struct tsm_ops *ops;
> > +	const struct config_item_type *type;
> > +	void *data;
> > +} provider;
> > +static DECLARE_RWSEM(tsm_rwsem);
> > +
> > +/**
> > + * DOC: Trusted Security Module (TSM) Attestation Report Interface
> > + *
> > + * The TSM report interface is a common provider of blobs that facilitate
> > + * attestation of a TVM (confidential computing guest) by an attestation
> > + * service. A TSM report combines a user-defined blob (likely a public-key with
> > + * a nonce for a key-exchange protocol) with a signed attestation report. That
> > + * combined blob is then used to obtain secrets provided by an agent that can
> > + * validate the attestation report. The expectation is that this interface is
> > + * invoked infrequently, however configfs allows for multiple agents to
> > + * own their own report generation instances to generate reports as
> > + * often as needed.
> > + *
> > + * The attestation report format is TSM provider specific, when / if a standard
> > + * materializes that can be published instead of the vendor layout. Until then
> > + * the 'provider' attribute indicates the format of 'outblob', and optionally
> > + * 'auxblob'.
> > + */
> > +
> > +struct tsm_report_state {
> > +	struct tsm_report report;
> > +	unsigned long write_generation;
> > +	unsigned long read_generation;
> > +	struct config_item cfg;
> > +};
> > +
> > +enum tsm_data_select {
> > +	TSM_REPORT,
> > +	TSM_CERTS,
> > +};
> > +
> > +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> > +{
> > +	struct tsm_report_state *state =
> > +		container_of(cfg, struct tsm_report_state, cfg);
> 
> This be one line of 88 chars (less than allowed 100).
> 
> (I'll comment once on this, feel free to ignore :) )

Unless and until the kernel's .clang-format template is updated to 100
you will find my patches wrapped to its ColumnLimit setting (80). I did
manually fixup my sev_guest changes to 100 since there was precedent in
that file, everything else I just let .clang-format do its thing.

> 
> > +
> > +	return &state->report;
> > +}
> > +
> > +static struct tsm_report_state *to_state(struct tsm_report *report)
> > +{
> > +	return container_of(report, struct tsm_report_state, report);
> > +}
> > +
> > +static int try_advance_write_generation(struct tsm_report *report)
> > +{
> > +	struct tsm_report_state *state = to_state(report);
> > +
> > +	lockdep_assert_held_write(&tsm_rwsem);
> > +
> > +	/*
> > +	 * Malicious or broken userspace has written enough times for
> > +	 * read_generation == write_generation by modular arithmetic without an
> > +	 * interim read. Stop accepting updates until the current report
> > +	 * configuration is read.
> > +	 */
> > +	if (state->write_generation == state->read_generation - 1)
> > +		return -EBUSY;
> > +	state->write_generation++;
> > +	return 0;
> > +}
> > +
> > +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> > +					  const char *buf, size_t len)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	unsigned int val;
> > +	int rc;
> > +
> > +	rc = kstrtouint(buf, 0, &val);
> > +	if (rc)
> > +		return rc;
> > +
> > +	/*
> > +	 * The valid privilege levels that a TSM might accept, if it accepts a
> > +	 * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
> > +	 * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
> > +	 * than 0.
> > +	 */
> 
> Sounds like privlevel_floor should be "unsigned int" rather than "int".

Sure.

> > +	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);
> 
> %d  or  change the type.

Ok.

> 
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
> > +
> > +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> > +				       const void *buf, size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	int rc;
> > +
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	rc = try_advance_write_generation(report);
> > +	if (rc)
> > +		return rc;
> > +
> > +	report->desc.inblob_len = count;
> > +	memcpy(report->desc.inblob, buf, count);
> > +	return count;
> > +}
> > +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
> > +
> > +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +	struct tsm_report_state *state = to_state(report);
> > +
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	return sysfs_emit(buf, "%lu\n", state->write_generation);
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, generation);
> > +
> > +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
> > +{
> > +	guard(rwsem_read)(&tsm_rwsem);
> > +	return sysfs_emit(buf, "%s\n", provider.ops->name);
> > +}
> > +CONFIGFS_ATTR_RO(tsm_report_, provider);
> > +
> > +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> > +			     enum tsm_data_select select)
> > +{
> > +	loff_t offset = 0;
> > +	ssize_t len;
> > +	u8 *out;
> > +
> > +	if (select == TSM_REPORT) {
> > +		out = report->outblob;
> > +		len = report->outblob_len;
> > +	} else {
> > +		out = report->auxblob;
> > +		len = report->auxblob_len;
> > +	}
> > +
> > +	/*
> > +	 * Recall that a NULL @buf is configfs requesting the size of
> > +	 * the buffer.
> > +	 */
> 
> The comment can be one line (or even dropped as it is configfs api).

I got a comment from a reviewer that did not understand why @buf is
allowed to be NULL. It's an oddity compared to sysfs binary attributes,
so I don't mind the comment.

[..]
> > +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> > +			       size_t count, enum tsm_data_select select)
> > +{
> > +	struct tsm_report_state *state = to_state(report);
> > +	const struct tsm_ops *ops;
> > +	ssize_t rc;
> > +
> > +	/* try to read from the existing report if present and valid... */
> > +	rc = read_cached_report(report, buf, count, select);
> > +	if (rc >= 0 || rc != -EWOULDBLOCK)
> > +		return rc;
> > +
> > +	/* slow path, report may need to be regenerated... */
> > +	guard(rwsem_write)(&tsm_rwsem);
> > +	ops = provider.ops;
> > +	if (!report->desc.inblob_len)
> > +		return -EINVAL;
> > +
> > +	/* did another thread already generate this report? */
> > +	if (report->outblob &&
> > +	    state->read_generation == state->write_generation)
> > +		goto out;
> > +	kvfree(report->outblob);
> > +	kvfree(report->auxblob);
> > +	report->outblob = NULL;
> > +	report->auxblob = NULL;
> > +	rc = ops->report_new(report, provider.data);
> 
> 
> Drop @ops and use "provider.ops" here?

Shrug, ok.

> 
> > +	if (rc < 0)
> > +		return rc;
> > +	state->read_generation = state->write_generation;
> > +out:
> > +	return __read_report(report, buf, count, select);
> > +}
> > +
> > +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> > +				       size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +
> > +	return tsm_report_read(report, buf, count, TSM_REPORT);
> > +}
> > +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
> > +
> > +static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
> > +				       size_t count)
> > +{
> > +	struct tsm_report *report = to_tsm_report(cfg);
> > +
> > +	return tsm_report_read(report, buf, count, TSM_CERTS);
> > +}
> > +CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
> > +
> > +#define TSM_DEFAULT_ATTRS() \
> 
> imho this one and TSM_DEFAULT_BIN_ATTRS are not really helping with 
> readability or a size :)

It is meant to do neither, it is only here to make it clear that
tsm_report_bin_extra_attrs[] is a super-set of tsm_report_bin_attrs[].
What I really want is sysfs-style group syntax, but configfs does not
have that same declaration capability.

[..]
> 
> A little confusing thing is that this guy is neither near the beginning 
> of the file (with other statics, this could even be a member of 
> tsm_provider) nor the code which sets/clears it - tsm_init/tsm_unregister.

@tsm_report_group is only used in tsm_{init,exit}(). I'll move its
declartion closer to tsm_init() in a follow-on.

[..]
> > diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> > new file mode 100644
> > index 000000000000..5fadc382064d
> > --- /dev/null
> > +++ b/include/linux/tsm.h
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef __TSM_H
> > +#define __TSM_H
> > +
> > +#include <linux/sizes.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> 
> device.h is not needed.

Yes, an earlier version of this interface referenced devices.

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Alexey Kardashevskiy]

On 17/10/23 13:19, Dan Williams wrote:
> Alexey Kardashevskiy wrote:
> [..]
>>> +What:		/sys/kernel/config/tsm/report/$name/provider
>>> +Date:		September, 2023
>>> +KernelVersion:	v6.7
>>> +Contact:	linux-coco@lists.linux.dev
>>> +Description:
>>> +		(RO) A name for the format-specification of @outblob like
>>> +		"sev-guest" [1]  or "tdx-guest" [2] in the near term, or a
>>> +		common standard format in the future.
>>
>> Nit: /sys/kernel/config/tsm/report/report0/provider contains
>> "sev_guest", i.e. "_", not "-".
> 
> Yes, will fix either with a follow-on or a respin if more feedback
> arrives.
> 
>>
>>> +
>>> +		[1]: SEV Secure Nested Paging Firmware ABI Specification
>>> +		Revision 1.55 Table 22
>>> +		https://www.amd.com/content/dam/amd/en/documents/epyc-technical-docs/specifications/56860.pdf
>>> +
>>> +		[2]: Intel® Trust Domain Extensions Data Center Attestation
>>> +		Primitives : Quote Generation Library and Quote Verification
>>> +		Library Revision 0.8 Appendix 4,5
>>> +		https://download.01.org/intel-sgx/latest/dcap-latest/linux/docs/Intel_TDX_DCAP_Quoting_Library_API.pdf
>>> +
>>> +What:		/sys/kernel/config/tsm/report/$name/generation
>>> +Date:		September, 2023
>>> +KernelVersion:	v6.7
>>> +Contact:	linux-coco@lists.linux.dev
>>> +Description:
>>> +		(RO) The value in this attribute increments each time @inblob or
>>> +		any option is written. Userspace can detect conflicts by
>>> +		checking generation before writing to any attribute and making
>>> +		sure the number of writes matches expectations after reading
>>> +		@outblob, or it can prevent conflicts by creating a report
>>> +		instance per requesting context.
>>> +
>>> +What:		/sys/kernel/config/tsm/report/$name/privlevel
>>> +Date:		September, 2023
>>> +KernelVersion:	v6.7
>>> +Contact:	linux-coco@lists.linux.dev
>>> +Description:
>>> +		(WO) Attribute is visible if a TSM implementation provider
>>> +		supports the concept of attestation reports for TVMs running at
>>> +		different privilege levels, like SEV-SNP "VMPL", specify the
>>> +		privilege level via this attribute.  The minimum acceptable
>>> +		value is conveyed via @privlevel_floor and the maximum
>>> +		acceptable value is TSM_PRIVLEVEL_MAX (3).
>>> +
>>> +What:		/sys/kernel/config/tsm/report/$name/privlevel_floor
>>> +Date:		September, 2023
>>> +KernelVersion:	v6.7
>>> +Contact:	linux-coco@lists.linux.dev
>>> +Description:
>>> +		(RO) Indicates the minimum permissible value that can be written
>>> +		to @privlevel.
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index b19995690904..8acbeb029ba1 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -21889,6 +21889,14 @@ W:	https://github.com/srcres258/linux-doc
>>>    T:	git git://github.com/srcres258/linux-doc.git doc-zh-tw
>>>    F:	Documentation/translations/zh_TW/
>>>    
>>> +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
>>> +M:	Dan Williams <dan.j.williams@intel.com>
>>> +L:	linux-coco@lists.linux.dev
>>> +S:	Maintained
>>> +F:	Documentation/ABI/testing/configfs-tsm
>>> +F:	drivers/virt/coco/tsm.c
>>> +F:	include/linux/tsm.h
>>> +
>>>    TTY LAYER AND SERIAL DRIVERS
>>>    M:	Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>>    M:	Jiri Slaby <jirislaby@kernel.org>
>>> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
>>> index fc5c64f04c4a..87d142c1f932 100644
>>> --- a/drivers/virt/coco/Kconfig
>>> +++ b/drivers/virt/coco/Kconfig
>>> @@ -2,6 +2,11 @@
>>>    #
>>>    # Confidential computing related collateral
>>>    #
>>> +
>>> +config TSM_REPORTS
>>> +	select CONFIGFS_FS
>>> +	tristate
>>> +
>>>    source "drivers/virt/coco/efi_secret/Kconfig"
>>>    
>>>    source "drivers/virt/coco/sev-guest/Kconfig"
>>> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
>>> index 55302ef719ad..18c1aba5edb7 100644
>>> --- a/drivers/virt/coco/Makefile
>>> +++ b/drivers/virt/coco/Makefile
>>> @@ -2,6 +2,7 @@
>>>    #
>>>    # Confidential computing related collateral
>>>    #
>>> +obj-$(CONFIG_TSM_REPORTS)	+= tsm.o
>>>    obj-$(CONFIG_EFI_SECRET)	+= efi_secret/
>>>    obj-$(CONFIG_SEV_GUEST)		+= sev-guest/
>>>    obj-$(CONFIG_INTEL_TDX_GUEST)	+= tdx-guest/
>>> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
>>> new file mode 100644
>>> index 000000000000..0200a86f1efe
>>> --- /dev/null
>>> +++ b/drivers/virt/coco/tsm.c
>>> @@ -0,0 +1,423 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
>>> +
>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>>> +
>>> +#include <linux/tsm.h>
>>> +#include <linux/err.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/rwsem.h>
>>> +#include <linux/string.h>
>>> +#include <linux/module.h>
>>> +#include <linux/cleanup.h>
>>> +#include <linux/configfs.h>
>>> +
>>> +static struct tsm_provider {
>>> +	const struct tsm_ops *ops;
>>> +	const struct config_item_type *type;
>>> +	void *data;
>>> +} provider;
>>> +static DECLARE_RWSEM(tsm_rwsem);
>>> +
>>> +/**
>>> + * DOC: Trusted Security Module (TSM) Attestation Report Interface
>>> + *
>>> + * The TSM report interface is a common provider of blobs that facilitate
>>> + * attestation of a TVM (confidential computing guest) by an attestation
>>> + * service. A TSM report combines a user-defined blob (likely a public-key with
>>> + * a nonce for a key-exchange protocol) with a signed attestation report. That
>>> + * combined blob is then used to obtain secrets provided by an agent that can
>>> + * validate the attestation report. The expectation is that this interface is
>>> + * invoked infrequently, however configfs allows for multiple agents to
>>> + * own their own report generation instances to generate reports as
>>> + * often as needed.
>>> + *
>>> + * The attestation report format is TSM provider specific, when / if a standard
>>> + * materializes that can be published instead of the vendor layout. Until then
>>> + * the 'provider' attribute indicates the format of 'outblob', and optionally
>>> + * 'auxblob'.
>>> + */
>>> +
>>> +struct tsm_report_state {
>>> +	struct tsm_report report;
>>> +	unsigned long write_generation;
>>> +	unsigned long read_generation;
>>> +	struct config_item cfg;
>>> +};
>>> +
>>> +enum tsm_data_select {
>>> +	TSM_REPORT,
>>> +	TSM_CERTS,
>>> +};
>>> +
>>> +static struct tsm_report *to_tsm_report(struct config_item *cfg)
>>> +{
>>> +	struct tsm_report_state *state =
>>> +		container_of(cfg, struct tsm_report_state, cfg);
>>
>> This be one line of 88 chars (less than allowed 100).
>>
>> (I'll comment once on this, feel free to ignore :) )
> 
> Unless and until the kernel's .clang-format template is updated to 100
> you will find my patches wrapped to its ColumnLimit setting (80). I did
> manually fixup my sev_guest changes to 100 since there was precedent in
> that file, everything else I just let .clang-format do its thing.
> 
>>
>>> +
>>> +	return &state->report;
>>> +}
>>> +
>>> +static struct tsm_report_state *to_state(struct tsm_report *report)
>>> +{
>>> +	return container_of(report, struct tsm_report_state, report);
>>> +}
>>> +
>>> +static int try_advance_write_generation(struct tsm_report *report)
>>> +{
>>> +	struct tsm_report_state *state = to_state(report);
>>> +
>>> +	lockdep_assert_held_write(&tsm_rwsem);
>>> +
>>> +	/*
>>> +	 * Malicious or broken userspace has written enough times for
>>> +	 * read_generation == write_generation by modular arithmetic without an
>>> +	 * interim read. Stop accepting updates until the current report
>>> +	 * configuration is read.
>>> +	 */
>>> +	if (state->write_generation == state->read_generation - 1)
>>> +		return -EBUSY;
>>> +	state->write_generation++;
>>> +	return 0;
>>> +}
>>> +
>>> +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
>>> +					  const char *buf, size_t len)
>>> +{
>>> +	struct tsm_report *report = to_tsm_report(cfg);
>>> +	unsigned int val;
>>> +	int rc;
>>> +
>>> +	rc = kstrtouint(buf, 0, &val);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	/*
>>> +	 * The valid privilege levels that a TSM might accept, if it accepts a
>>> +	 * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
>>> +	 * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
>>> +	 * than 0.
>>> +	 */
>>
>> Sounds like privlevel_floor should be "unsigned int" rather than "int".
> 
> Sure.
> 
>>> +	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);
>>
>> %d  or  change the type.
> 
> Ok.
> 
>>
>>> +}
>>> +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
>>> +
>>> +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
>>> +				       const void *buf, size_t count)
>>> +{
>>> +	struct tsm_report *report = to_tsm_report(cfg);
>>> +	int rc;
>>> +
>>> +	guard(rwsem_write)(&tsm_rwsem);
>>> +	rc = try_advance_write_generation(report);
>>> +	if (rc)
>>> +		return rc;
>>> +
>>> +	report->desc.inblob_len = count;
>>> +	memcpy(report->desc.inblob, buf, count);
>>> +	return count;
>>> +}
>>> +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
>>> +
>>> +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
>>> +{
>>> +	struct tsm_report *report = to_tsm_report(cfg);
>>> +	struct tsm_report_state *state = to_state(report);
>>> +
>>> +	guard(rwsem_read)(&tsm_rwsem);
>>> +	return sysfs_emit(buf, "%lu\n", state->write_generation);
>>> +}
>>> +CONFIGFS_ATTR_RO(tsm_report_, generation);
>>> +
>>> +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
>>> +{
>>> +	guard(rwsem_read)(&tsm_rwsem);
>>> +	return sysfs_emit(buf, "%s\n", provider.ops->name);
>>> +}
>>> +CONFIGFS_ATTR_RO(tsm_report_, provider);
>>> +
>>> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
>>> +			     enum tsm_data_select select)
>>> +{
>>> +	loff_t offset = 0;
>>> +	ssize_t len;
>>> +	u8 *out;
>>> +
>>> +	if (select == TSM_REPORT) {
>>> +		out = report->outblob;
>>> +		len = report->outblob_len;
>>> +	} else {
>>> +		out = report->auxblob;
>>> +		len = report->auxblob_len;
>>> +	}
>>> +
>>> +	/*
>>> +	 * Recall that a NULL @buf is configfs requesting the size of
>>> +	 * the buffer.
>>> +	 */
>>
>> The comment can be one line (or even dropped as it is configfs api).
> 
> I got a comment from a reviewer that did not understand why @buf is
> allowed to be NULL. It's an oddity compared to sysfs binary attributes,
> so I don't mind the comment.

I did not understand why memcpy() and not copy_from_user(), for example, 
had to grep. Commenting on the configfs api outside of it imho is a bit 
too much but ok.


> [..]
>>> +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
>>> +			       size_t count, enum tsm_data_select select)
>>> +{
>>> +	struct tsm_report_state *state = to_state(report);
>>> +	const struct tsm_ops *ops;
>>> +	ssize_t rc;
>>> +
>>> +	/* try to read from the existing report if present and valid... */
>>> +	rc = read_cached_report(report, buf, count, select);
>>> +	if (rc >= 0 || rc != -EWOULDBLOCK)
>>> +		return rc;
>>> +
>>> +	/* slow path, report may need to be regenerated... */
>>> +	guard(rwsem_write)(&tsm_rwsem);
>>> +	ops = provider.ops;
>>> +	if (!report->desc.inblob_len)
>>> +		return -EINVAL;
>>> +
>>> +	/* did another thread already generate this report? */
>>> +	if (report->outblob &&
>>> +	    state->read_generation == state->write_generation)
>>> +		goto out;
>>> +	kvfree(report->outblob);
>>> +	kvfree(report->auxblob);
>>> +	report->outblob = NULL;
>>> +	report->auxblob = NULL;
>>> +	rc = ops->report_new(report, provider.data);
>>
>>
>> Drop @ops and use "provider.ops" here?
> 
> Shrug, ok.

I asked as at first I thought it is stored right after rwsem_write in 
@ops for a reason but then it was not even checked for NULL so it could 
be initialized where declared.


> 
>>
>>> +	if (rc < 0)
>>> +		return rc;
>>> +	state->read_generation = state->write_generation;
>>> +out:
>>> +	return __read_report(report, buf, count, select);
>>> +}
>>> +
>>> +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
>>> +				       size_t count)
>>> +{
>>> +	struct tsm_report *report = to_tsm_report(cfg);
>>> +
>>> +	return tsm_report_read(report, buf, count, TSM_REPORT);
>>> +}
>>> +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
>>> +
>>> +static ssize_t tsm_report_auxblob_read(struct config_item *cfg, void *buf,
>>> +				       size_t count)
>>> +{
>>> +	struct tsm_report *report = to_tsm_report(cfg);
>>> +
>>> +	return tsm_report_read(report, buf, count, TSM_CERTS);
>>> +}
>>> +CONFIGFS_BIN_ATTR_RO(tsm_report_, auxblob, NULL, TSM_OUTBLOB_MAX);
>>> +
>>> +#define TSM_DEFAULT_ATTRS() \
>>
>> imho this one and TSM_DEFAULT_BIN_ATTRS are not really helping with
>> readability or a size :)
> 
> It is meant to do neither, it is only here to make it clear that
> tsm_report_bin_extra_attrs[] is a super-set of tsm_report_bin_attrs[].
> What I really want is sysfs-style group syntax, but configfs does not
> have that same declaration capability.

The order is mixed now:

TSM_DEFAULT_ATTRS
tsm_report_attrs
TSM_DEFAULT_BIN_ATTRS
tsm_report_bin_attrs
tsm_report_bin_extra_attrs (*)
tsm_report_extra_attrs  (*)
tsm_report_item_release
tsm_report_item_ops
tsm_report_default_type
tsm_report_ext_type (*)

Grouping (*) together would make the superset thing clearer imho.

tsm_report_ distract too as they could be just tsm_ (as those in capital 
letters). Either _ext_ or _extra_. I am not insisting though, just 
whining :) Thanks,


> [..]
>>
>> A little confusing thing is that this guy is neither near the beginning
>> of the file (with other statics, this could even be a member of
>> tsm_provider) nor the code which sets/clears it - tsm_init/tsm_unregister.
> 
> @tsm_report_group is only used in tsm_{init,exit}(). I'll move its
> declartion closer to tsm_init() in a follow-on.
> 
> [..]
>>> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
>>> new file mode 100644
>>> index 000000000000..5fadc382064d
>>> --- /dev/null
>>> +++ b/include/linux/tsm.h
>>> @@ -0,0 +1,68 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +#ifndef __TSM_H
>>> +#define __TSM_H
>>> +
>>> +#include <linux/sizes.h>
>>> +#include <linux/types.h>
>>> +#include <linux/device.h>
>>
>> device.h is not needed.
> 
> Yes, an earlier version of this interface referenced devices.

-- 
Alexey

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Alexey Kardashevskiy wrote:
[..]
> > It is meant to do neither, it is only here to make it clear that
> > tsm_report_bin_extra_attrs[] is a super-set of tsm_report_bin_attrs[].
> > What I really want is sysfs-style group syntax, but configfs does not
> > have that same declaration capability.
> 
> The order is mixed now:
> 
> TSM_DEFAULT_ATTRS
> tsm_report_attrs
> TSM_DEFAULT_BIN_ATTRS
> tsm_report_bin_attrs
> tsm_report_bin_extra_attrs (*)
> tsm_report_extra_attrs  (*)
> tsm_report_item_release
> tsm_report_item_ops
> tsm_report_default_type
> tsm_report_ext_type (*)
> 
> Grouping (*) together would make the superset thing clearer imho.

I can do that.

> tsm_report_ distract too as they could be just tsm_ (as those in capital 
> letters). Either _ext_ or _extra_. I am not insisting though, just 
> whining :) Thanks,

The "tsm_report_" is deliberate as these files are all associated with
the /sys/kernel/config/tsm/report/ directory. If there was ever a
/sys/kernel/config/tsm/$other_thing a new prefix would be needed.

Re: [PATCH v6 3/7] configfs-tsm: Introduce a shared ABI for attestation reports

[Dan Williams]

Alexey Kardashevskiy wrote:
[..]
> >>> +			       size_t count, enum tsm_data_select select)
> >>> +{
> >>> +	struct tsm_report_state *state = to_state(report);
> >>> +	const struct tsm_ops *ops;
> >>> +	ssize_t rc;
> >>> +
> >>> +	/* try to read from the existing report if present and valid... */
> >>> +	rc = read_cached_report(report, buf, count, select);
> >>> +	if (rc >= 0 || rc != -EWOULDBLOCK)
> >>> +		return rc;
> >>> +
> >>> +	/* slow path, report may need to be regenerated... */
> >>> +	guard(rwsem_write)(&tsm_rwsem);
> >>> +	ops = provider.ops;
> >>> +	if (!report->desc.inblob_len)
> >>> +		return -EINVAL;
> >>> +
> >>> +	/* did another thread already generate this report? */
> >>> +	if (report->outblob &&
> >>> +	    state->read_generation == state->write_generation)
> >>> +		goto out;
> >>> +	kvfree(report->outblob);
> >>> +	kvfree(report->auxblob);
> >>> +	report->outblob = NULL;
> >>> +	report->auxblob = NULL;
> >>> +	rc = ops->report_new(report, provider.data);
> >>
> >>
> >> Drop @ops and use "provider.ops" here?
> > 
> > Shrug, ok.
> 
> I asked as at first I thought it is stored right after rwsem_write in 
> @ops for a reason but then it was not even checked for NULL so it could 
> be initialized where declared.

I went back to declaring an @ops variable because this sequence needs to
validate that the ops are non-NULL under the lock, and typing
provider.ops several times is a bit grating.

        /* slow path, report may need to be regenerated... */
        guard(rwsem_write)(&tsm_rwsem);
        ops = provider.ops;
        if (!ops)
                return -ENOTTY;

[PATCH v6 4/7] virt: sevguest: Prep for kernel internal 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>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/virt/coco/sev-guest/sev-guest.c |   44 +++++++++++++++++++++++--------
 1 file changed, 32 insertions(+), 12 deletions(-)

diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c
index 5bee58ef5f1e..e5f8f115f4af 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>
 
@@ -475,6 +476,11 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,
 	return 0;
 }
 
+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_guest_crypto *crypto = snp_dev->crypto;
@@ -555,22 +561,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_ext_report_req *req = &snp_dev->req.ext_report;
 	struct snp_guest_crypto *crypto = snp_dev->crypto;
 	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;
 
@@ -578,8 +587,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
@@ -609,21 +623,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:
@@ -636,6 +648,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)))
@@ -664,7 +677,14 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long
 		ret = get_derived_key(snp_dev, &input);
 		break;
 	case SNP_GET_EXT_REPORT:
-		ret = get_ext_report(snp_dev, &input);
+		/*
+		 * As get_ext_report() may be called from the ioctl() path and a
+		 * kernel internal path (configfs-tsm), decorate the passed
+		 * buffers as user pointers.
+		 */
+		io.req_data = USER_SOCKPTR((void __user *)input.req_data);
+		io.resp_data = USER_SOCKPTR((void __user *)input.resp_data);
+		ret = get_ext_report(snp_dev, &input, &io);
 		break;
 	default:
 		break;

[PATCH v6 5/7] 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>
Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/slab.h |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8228d1276a2f..df4c2d45bb86 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -763,6 +763,8 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla
 extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
 		      __realloc_size(3);
 extern void kvfree(const void *addr);
+DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
+
 extern void kvfree_sensitive(const void *addr, size_t len);
 
 unsigned int kmem_cache_size(struct kmem_cache *s);

[PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_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 data that
the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
retrieving the report blob via the TSM interface utility,
assuming no nonce and VMPL==2:

    report=/sys/kernel/config/tsm/report/report0
    mkdir $report
    echo 2 > $report/privlevel
    dd if=/dev/urandom bs=64 count=1 > $report/inblob
    hexdump -C $report/outblob # SNP report
    hexdump -C $report/auxblob # cert_table
    rmdir $report

Given that the platform implementation is free to return empty
certificate data if none is available it lets configfs-tsm be simplified
as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
SNP_GET_REPORT alone.

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.

Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Cc: Borislav Petkov <bp@alien8.de>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Dionna Glaze <dionnaglaze@google.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 +++++++++++++++++++++++++++++++
 2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
 
@@ -768,6 +770,129 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
 	return key;
 }
 
+struct snp_msg_report_resp_hdr {
+	u32 status;
+	u32 report_size;
+	u8 rsvd[24];
+};
+#define SNP_REPORT_INVALID_PARAM 0x16
+#define SNP_REPORT_INVALID_KEY_SEL 0x27
+
+struct snp_msg_cert_entry {
+	unsigned char guid[16];
+	u32 offset;
+	u32 length;
+};
+
+static int sev_report_new(struct tsm_report *report, void *data)
+{
+	static const struct snp_msg_cert_entry zero_ent = { 0 };
+	struct snp_msg_cert_entry *cert_table;
+	struct tsm_desc *desc = &report->desc;
+	struct snp_guest_dev *snp_dev = data;
+	struct snp_msg_report_resp_hdr hdr;
+	const int report_size = SZ_4K;
+	const int ext_size = SEV_FW_BLOB_MAX_SIZE;
+	int ret, size = report_size + ext_size;
+	u32 certs_size, i;
+
+	if (desc->inblob_len != 64)
+		return -EINVAL;
+
+	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
+	guard(mutex)(&snp_cmd_mutex);
+
+	/* Check if the VMPCK is not empty */
+	if (is_vmpck_empty(snp_dev)) {
+		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
+		return -ENOTTY;
+	}
+
+	cert_table = buf + report_size;
+	struct snp_ext_report_req ext_req = {
+		.data = { .vmpl = desc->privlevel },
+		.certs_address = (__u64)cert_table,
+		.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,
+		.exitinfo2 = 0xff,
+	};
+	struct snp_req_resp io = {
+		.req_data = KERNEL_SOCKPTR(&ext_req),
+		.resp_data = KERNEL_SOCKPTR(buf),
+	};
+
+	ret = get_ext_report(snp_dev, &input, &io);
+
+	if (ret)
+		return ret;
+
+	memcpy(&hdr, buf, sizeof(hdr));
+	if (hdr.status == SNP_REPORT_INVALID_PARAM)
+		return -EINVAL;
+	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
+		return -EINVAL;
+	if (hdr.status)
+		return -ENXIO;
+	if ((hdr.report_size + sizeof(hdr)) > report_size)
+		return -ENOMEM;
+
+	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
+	if (!rbuf)
+		return -ENOMEM;
+
+	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
+	report->outblob = no_free_ptr(rbuf);
+	report->outblob_len = hdr.report_size;
+
+	certs_size = 0;
+	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
+		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
+			break;
+		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
+	}
+
+	/* No certs to report */
+	if (!certs_size)
+		return 0;
+
+	/*
+	 * cert_table reports more data than fits in ext_size the
+	 * userspace cert_table walker can decide what happens next,
+	 * truncate the output
+	 */
+	if (certs_size > ext_size)
+		certs_size = ext_size;
+
+	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
+	if (!cbuf)
+		return -ENOMEM;
+
+	memcpy(cbuf, cert_table, certs_size);
+	report->auxblob = no_free_ptr(cbuf);
+	report->auxblob_len = certs_size;
+
+	return 0;
+}
+
+static const struct tsm_ops sev_tsm_ops = {
+	.name = KBUILD_MODNAME,
+	.report_new = sev_report_new,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+	tsm_unregister(&sev_tsm_ops);
+}
+
 static int __init sev_guest_probe(struct platform_device *pdev)
 {
 	struct snp_secrets_page_layout *layout;
@@ -841,6 +966,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
 	snp_dev->input.resp_gpa = __pa(snp_dev->response);
 	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
 
+	ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
+	if (ret)
+		goto e_free_cert_data;
+
+	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
+	if (ret)
+		goto e_free_cert_data;
+
 	ret =  misc_register(misc);
 	if (ret)
 		goto e_free_cert_data;

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Tom Lendacky]

On 10/12/23 21:14, 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 data that
> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
> retrieving the report blob via the TSM interface utility,
> assuming no nonce and VMPL==2:
> 
>      report=/sys/kernel/config/tsm/report/report0
>      mkdir $report
>      echo 2 > $report/privlevel
>      dd if=/dev/urandom bs=64 count=1 > $report/inblob
>      hexdump -C $report/outblob # SNP report
>      hexdump -C $report/auxblob # cert_table
>      rmdir $report
> 
> Given that the platform implementation is free to return empty
> certificate data if none is available it lets configfs-tsm be simplified
> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
> SNP_GET_REPORT alone.
> 
> 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.

Everything looks good. As for deprecating the ioctl(), you will probably 
only be able to deprecate the certificate related portions of it. If we 
ever have a common key derivation interface, then the complete ioctl() 
could be deprecated.

Thanks,
Tom

> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 +++++++++++++++++++++++++++++++
>   2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
>   
> @@ -768,6 +770,129 @@ static u8 *get_vmpck(int id, struct snp_secrets_page_layout *layout, u32 **seqno
>   	return key;
>   }
>   
> +struct snp_msg_report_resp_hdr {
> +	u32 status;
> +	u32 report_size;
> +	u8 rsvd[24];
> +};
> +#define SNP_REPORT_INVALID_PARAM 0x16
> +#define SNP_REPORT_INVALID_KEY_SEL 0x27
> +
> +struct snp_msg_cert_entry {
> +	unsigned char guid[16];
> +	u32 offset;
> +	u32 length;
> +};
> +
> +static int sev_report_new(struct tsm_report *report, void *data)
> +{
> +	static const struct snp_msg_cert_entry zero_ent = { 0 };
> +	struct snp_msg_cert_entry *cert_table;
> +	struct tsm_desc *desc = &report->desc;
> +	struct snp_guest_dev *snp_dev = data;
> +	struct snp_msg_report_resp_hdr hdr;
> +	const int report_size = SZ_4K;
> +	const int ext_size = SEV_FW_BLOB_MAX_SIZE;
> +	int ret, size = report_size + ext_size;
> +	u32 certs_size, i;
> +
> +	if (desc->inblob_len != 64)
> +		return -EINVAL;
> +
> +	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	guard(mutex)(&snp_cmd_mutex);
> +
> +	/* Check if the VMPCK is not empty */
> +	if (is_vmpck_empty(snp_dev)) {
> +		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> +		return -ENOTTY;
> +	}
> +
> +	cert_table = buf + report_size;
> +	struct snp_ext_report_req ext_req = {
> +		.data = { .vmpl = desc->privlevel },
> +		.certs_address = (__u64)cert_table,
> +		.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,
> +		.exitinfo2 = 0xff,
> +	};
> +	struct snp_req_resp io = {
> +		.req_data = KERNEL_SOCKPTR(&ext_req),
> +		.resp_data = KERNEL_SOCKPTR(buf),
> +	};
> +
> +	ret = get_ext_report(snp_dev, &input, &io);
> +
> +	if (ret)
> +		return ret;
> +
> +	memcpy(&hdr, buf, sizeof(hdr));
> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
> +		return -EINVAL;
> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> +		return -EINVAL;
> +	if (hdr.status)
> +		return -ENXIO;
> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
> +		return -ENOMEM;
> +
> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> +	if (!rbuf)
> +		return -ENOMEM;
> +
> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> +	report->outblob = no_free_ptr(rbuf);
> +	report->outblob_len = hdr.report_size;
> +
> +	certs_size = 0;
> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
> +			break;
> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
> +	}
> +
> +	/* No certs to report */
> +	if (!certs_size)
> +		return 0;
> +
> +	/*
> +	 * cert_table reports more data than fits in ext_size the
> +	 * userspace cert_table walker can decide what happens next,
> +	 * truncate the output
> +	 */
> +	if (certs_size > ext_size)
> +		certs_size = ext_size;
> +
> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> +	if (!cbuf)
> +		return -ENOMEM;
> +
> +	memcpy(cbuf, cert_table, certs_size);
> +	report->auxblob = no_free_ptr(cbuf);
> +	report->auxblob_len = certs_size;
> +
> +	return 0;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> +	tsm_unregister(&sev_tsm_ops);
> +}
> +
>   static int __init sev_guest_probe(struct platform_device *pdev)
>   {
>   	struct snp_secrets_page_layout *layout;
> @@ -841,6 +966,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	snp_dev->input.resp_gpa = __pa(snp_dev->response);
>   	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>   
> +	ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
> +	if (ret)
> +		goto e_free_cert_data;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> +	if (ret)
> +		goto e_free_cert_data;
> +
>   	ret =  misc_register(misc);
>   	if (ret)
>   		goto e_free_cert_data;
> 

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dan Williams]

Tom Lendacky wrote:
> On 10/12/23 21:14, 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 data that
> > the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
> > retrieving the report blob via the TSM interface utility,
> > assuming no nonce and VMPL==2:
> > 
> >      report=/sys/kernel/config/tsm/report/report0
> >      mkdir $report
> >      echo 2 > $report/privlevel
> >      dd if=/dev/urandom bs=64 count=1 > $report/inblob
> >      hexdump -C $report/outblob # SNP report
> >      hexdump -C $report/auxblob # cert_table
> >      rmdir $report
> > 
> > Given that the platform implementation is free to return empty
> > certificate data if none is available it lets configfs-tsm be simplified
> > as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
> > SNP_GET_REPORT alone.
> > 
> > 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.
> 
> Everything looks good.

I appreciate the help here, thank you.

> As for deprecating the ioctl(), you will probably only be able to
> deprecate the certificate related portions of it. If we ever have a
> common key derivation interface, then the complete ioctl() could be
> deprecated.

Yes, lets cross that bridge when/if another implementation develops a
derived-key retrieval facility. It feels like the unified ABI approach
there wants to be wrapped by trusted-keys. I recall James had thoughts
about that relative to vTPM, and whether vTPM supersedes this facility.

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy]

On 13/10/23 13:14, 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 data that
> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
> retrieving the report blob via the TSM interface utility,
> assuming no nonce and VMPL==2:
> 
>      report=/sys/kernel/config/tsm/report/report0
>      mkdir $report
>      echo 2 > $report/privlevel
>      dd if=/dev/urandom bs=64 count=1 > $report/inblob

Is not this one a "nonce"?

>      hexdump -C $report/outblob # SNP report
>      hexdump -C $report/auxblob # cert_table
>      rmdir $report
> 
> Given that the platform implementation is free to return empty
> certificate data if none is available it lets configfs-tsm be simplified
> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
> SNP_GET_REPORT alone.
> 
> 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.
> 
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Dionna Glaze <dionnaglaze@google.com>
> Cc: Brijesh Singh <brijesh.singh@amd.com>
> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 +++++++++++++++++++++++++++++++
>   2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
>   
> @@ -768,6 +770,129 @@ 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

There is one already - SEV_RET_INVALID_PARAM, defined in "Secure 
Encrypted Virtualization API".

> +#define SNP_REPORT_INVALID_KEY_SEL 0x27

This one needs to be defined in include/uapi/linux/psp-sev.h's sev_ret_code.

> +
> +struct snp_msg_cert_entry {
> +	unsigned char guid[16];
> +	u32 offset;
> +	u32 length;
> +};
> +
> +static int sev_report_new(struct tsm_report *report, void *data)
> +{
> +	static const struct snp_msg_cert_entry zero_ent = { 0 };
> +	struct snp_msg_cert_entry *cert_table;
> +	struct tsm_desc *desc = &report->desc;
> +	struct snp_guest_dev *snp_dev = data;
> +	struct snp_msg_report_resp_hdr hdr;
> +	const int report_size = SZ_4K;
> +	const int ext_size = SEV_FW_BLOB_MAX_SIZE;

These two are size_t. Or u32. "int" is just weird :)

> +	int ret, size = report_size + ext_size;
> +	u32 certs_size, i;

@certs_size is size_t (as it is copied to ->auxblob_len in the end), and 
@size is size_t as well.

@i is just "unsigned", can be declared right in the "for" below?

> +
> +	if (desc->inblob_len != 64)

64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
May be even BUILD_BUG_ON(TSM_INBLOB_MAX != sizeof(ext_req.data.user_data)) ?

> +		return -EINVAL;
> +
> +	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);

I did not realize declaring variables in a middle of a scope is allowed 
now :)
Since you are doing this, move zero_ent below. Or, better, use 
guid_is_null().

> +	if (!buf)
> +		return -ENOMEM;
> +
> +	guard(mutex)(&snp_cmd_mutex);
> +
> +	/* Check if the VMPCK is not empty */
> +	if (is_vmpck_empty(snp_dev)) {
> +		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> +		return -ENOTTY;
> +	}
> +
> +	cert_table = buf + report_size;
> +	struct snp_ext_report_req ext_req = {
> +		.data = { .vmpl = desc->privlevel },
> +		.certs_address = (__u64)cert_table,
> +		.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,
> +		.exitinfo2 = 0xff,

Not sure we need this line with 0xff.

The GHCB spec says the hypervisor sets it, not the guest. And I could 
not figure out why exactly snp_guest_ioctl() does "input.exitinfo2 = 
0xff", my best guest it is to catch GHCB not being called before copying 
memory to user.

> +	};
> +	struct snp_req_resp io = {
> +		.req_data = KERNEL_SOCKPTR(&ext_req),
> +		.resp_data = KERNEL_SOCKPTR(buf),
> +	};
> +
> +	ret = get_ext_report(snp_dev, &input, &io);
> +

Unnecessary empty line.

> +	if (ret)
> +		return ret;
> +
> +	memcpy(&hdr, buf, sizeof(hdr));
> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
> +		return -EINVAL;
> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> +		return -EINVAL;
> +	if (hdr.status)
> +		return -ENXIO;
> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
> +		return -ENOMEM;
> +
> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> +	if (!rbuf)
> +		return -ENOMEM;
> +
> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> +	report->outblob = no_free_ptr(rbuf);
> +	report->outblob_len = hdr.report_size;
> +
> +	certs_size = 0;
> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
> +			break;
> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
> +	}
> +
> +	/* No certs to report */
> +	if (!certs_size)

Nit: WARN_ON_ONCE(i) here?

> +		return 0;
> +
> +	/*
> +	 * cert_table reports more data than fits in ext_size the
> +	 * userspace cert_table walker can decide what happens next,
> +	 * truncate the output
> +	 */
> +	if (certs_size > ext_size)
> +		certs_size = ext_size;

This sounds more like the HV provided a broken table with offset(s) 
ouside of the certs buffer. The HV is expected instead return 
SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the 
guest to retry.

> +
> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> +	if (!cbuf)
> +		return -ENOMEM;

In a such (unlikely) event the function returns an error but does not 
free report->outblob which is going to leak if consequent call succeded. 
This new no_free_ptr business is confusing at times :(


> +
> +	memcpy(cbuf, cert_table, certs_size);
> +	report->auxblob = no_free_ptr(cbuf);
> +	report->auxblob_len = certs_size;


Aaaand, it works, so:

Tested-by: Alexey Kardashevskiy <aik@amd.com>

Thanks,

> +
> +	return 0;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> +	.name = KBUILD_MODNAME,
> +	.report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> +	tsm_unregister(&sev_tsm_ops);
> +}
> +
>   static int __init sev_guest_probe(struct platform_device *pdev)
>   {
>   	struct snp_secrets_page_layout *layout;
> @@ -841,6 +966,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
>   	snp_dev->input.resp_gpa = __pa(snp_dev->response);
>   	snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>   
> +	ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
> +	if (ret)
> +		goto e_free_cert_data;
> +
> +	ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> +	if (ret)
> +		goto e_free_cert_data;
> +
>   	ret =  misc_register(misc);
>   	if (ret)
>   		goto e_free_cert_data;
> 

-- 
Alexey

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dionna Amalie Glaze]

> > +
> > +     struct snp_guest_request_ioctl input = {
> > +             .msg_version = 1,
> > +             .req_data = (__u64)&ext_req,
> > +             .resp_data = (__u64)buf,
> > +             .exitinfo2 = 0xff,
>
> Not sure we need this line with 0xff.
>

The exitinfo2 value had an uninitialized memory bug, where random data
would get returned to user space. I think this is carrying forward the
initial value that sev-guest currently uses.

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

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Peter Gonda]

On Mon, Oct 16, 2023 at 9:39 AM Dionna Amalie Glaze
<dionnaglaze@google.com> wrote:
>
> > > +
> > > +     struct snp_guest_request_ioctl input = {
> > > +             .msg_version = 1,
> > > +             .req_data = (__u64)&ext_req,
> > > +             .resp_data = (__u64)buf,
> > > +             .exitinfo2 = 0xff,
> >
> > Not sure we need this line with 0xff.
> >
>
> The exitinfo2 value had an uninitialized memory bug, where random data
> would get returned to user space. I think this is carrying forward the
> initial value that sev-guest currently uses.

I think during the initial review I asked for exitinfo2 to be set to
some known value initially. That way userspace can tell if the request
failed before the ASP was involved.
>
> --
> -Dionna Glaze, PhD (she/her)
>

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy]

On 17/10/23 02:42, Peter Gonda wrote:
> On Mon, Oct 16, 2023 at 9:39 AM Dionna Amalie Glaze
> <dionnaglaze@google.com> wrote:
>>
>>>> +
>>>> +     struct snp_guest_request_ioctl input = {
>>>> +             .msg_version = 1,
>>>> +             .req_data = (__u64)&ext_req,
>>>> +             .resp_data = (__u64)buf,
>>>> +             .exitinfo2 = 0xff,
>>>
>>> Not sure we need this line with 0xff.
>>>
>>
>> The exitinfo2 value had an uninitialized memory bug, where random data
>> would get returned to user space. I think this is carrying forward the
>> initial value that sev-guest currently uses.
> 
> I think during the initial review I asked for exitinfo2 to be set to
> some known value initially.
> That way userspace can tell if the request
> failed before the ASP was involved.

ASP or HV? SEV-SNP ABI or GHCB? It is possible to make a GHCB call and 
have KVM write something in exit_info_2 without calling the ASP. The 
guest should really look into the encrypted response buffer to know what 
really happened.

Anyway, looks like "rio->exitinfo2 = SEV_RET_NO_FW_CALL" in 
snp_issue_guest_request() is supposed to handle this. 0xff in 
snp_guest_ioctl() seems to be for something else. May be 
s/0xff/SEV_RET_NO_FW_CALL/. And I do not really see why the userspace 
could not write this 0xff to @input itself before doing ioctl().

The efb339a83368ab25de log is ambiguous btw -
"The PSP can return  ... in circumstances where the PSP has not actually 
been called" - if something is not called - it cannot possibly return :) 
Thanks,


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

-- 
Alexey

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dan Williams]

Alexey Kardashevskiy wrote:
> 
> On 17/10/23 02:42, Peter Gonda wrote:
> > On Mon, Oct 16, 2023 at 9:39 AM Dionna Amalie Glaze
> > <dionnaglaze@google.com> wrote:
> >>
> >>>> +
> >>>> +     struct snp_guest_request_ioctl input = {
> >>>> +             .msg_version = 1,
> >>>> +             .req_data = (__u64)&ext_req,
> >>>> +             .resp_data = (__u64)buf,
> >>>> +             .exitinfo2 = 0xff,
> >>>
> >>> Not sure we need this line with 0xff.
> >>>
> >>
> >> The exitinfo2 value had an uninitialized memory bug, where random data
> >> would get returned to user space. I think this is carrying forward the
> >> initial value that sev-guest currently uses.
> > 
> > I think during the initial review I asked for exitinfo2 to be set to
> > some known value initially.
> > That way userspace can tell if the request
> > failed before the ASP was involved.
> 
> ASP or HV? SEV-SNP ABI or GHCB? It is possible to make a GHCB call and 
> have KVM write something in exit_info_2 without calling the ASP. The 
> guest should really look into the encrypted response buffer to know what 
> really happened.
> 
> Anyway, looks like "rio->exitinfo2 = SEV_RET_NO_FW_CALL" in 
> snp_issue_guest_request() is supposed to handle this. 0xff in 
> snp_guest_ioctl() seems to be for something else. May be 
> s/0xff/SEV_RET_NO_FW_CALL/.

It's a 64-bit value and it is not clear that -1 is the expectation.

> And I do not really see why the userspace 
> could not write this 0xff to @input itself before doing ioctl().

If it needs to be initialized either the kernel needs to set it, or the
kernel needs to verify that userspace set it.

At this point I would invite you to fix this after this series since
there is a question about why it is there that is independent of this
series.

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dan Williams]

Alexey Kardashevskiy wrote:
> On 13/10/23 13:14, 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 data that
> > the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
> > retrieving the report blob via the TSM interface utility,
> > assuming no nonce and VMPL==2:
> > 
> >      report=/sys/kernel/config/tsm/report/report0
> >      mkdir $report
> >      echo 2 > $report/privlevel
> >      dd if=/dev/urandom bs=64 count=1 > $report/inblob
> 
> Is not this one a "nonce"?

No, a nonce would come from the verifier to be mixed with the 64-bytes
that the guest uses to establish a shared secret. In this case this is
just showing the mechanics of the ABI with those security best practices
set aside.

> 
> >      hexdump -C $report/outblob # SNP report
> >      hexdump -C $report/auxblob # cert_table
> >      rmdir $report
> > 
> > Given that the platform implementation is free to return empty
> > certificate data if none is available it lets configfs-tsm be simplified
> > as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
> > SNP_GET_REPORT alone.
> > 
> > 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.
> > 
> > Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> > Cc: Borislav Petkov <bp@alien8.de>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Dionna Glaze <dionnaglaze@google.com>
> > Cc: Brijesh Singh <brijesh.singh@amd.com>
> > Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
> > Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 +++++++++++++++++++++++++++++++
> >   2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
> >   
> > @@ -768,6 +770,129 @@ 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
> 
> There is one already - SEV_RET_INVALID_PARAM, defined in "Secure 
> Encrypted Virtualization API".

Ok, indeed I took this from Jeremi without checking for other
definitions.

> 
> > +#define SNP_REPORT_INVALID_KEY_SEL 0x27
> 
> This one needs to be defined in include/uapi/linux/psp-sev.h's sev_ret_code.

Ah, ok, will move.

> 
> > +
> > +struct snp_msg_cert_entry {
> > +	unsigned char guid[16];
> > +	u32 offset;
> > +	u32 length;
> > +};
> > +
> > +static int sev_report_new(struct tsm_report *report, void *data)
> > +{
> > +	static const struct snp_msg_cert_entry zero_ent = { 0 };
> > +	struct snp_msg_cert_entry *cert_table;
> > +	struct tsm_desc *desc = &report->desc;
> > +	struct snp_guest_dev *snp_dev = data;
> > +	struct snp_msg_report_resp_hdr hdr;
> > +	const int report_size = SZ_4K;
> > +	const int ext_size = SEV_FW_BLOB_MAX_SIZE;
> 
> These two are size_t. Or u32. "int" is just weird :)

Nothing is bigger than a few kb here, but ok.

> 
> > +	int ret, size = report_size + ext_size;
> > +	u32 certs_size, i;
> 
> @certs_size is size_t (as it is copied to ->auxblob_len in the end), and 
> @size is size_t as well.
> 
> @i is just "unsigned", can be declared right in the "for" below?

ok.

> 
> > +
> > +	if (desc->inblob_len != 64)
> 
> 64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
> May be even BUILD_BUG_ON(TSM_INBLOB_MAX != sizeof(ext_req.data.user_data)) ?
> 
> > +		return -EINVAL;
> > +
> > +	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> 
> I did not realize declaring variables in a middle of a scope is allowed 
> now :)

Yes, it's been allowed in loop declaration for a few kernels now and the
new __free() helper in cleanup.h will make this model more prominent. My
sense is that it is still not open season on mid-function declarations,
but __attribute__((__cleanup__())) usage needs it.

> Since you are doing this, move zero_ent below. Or, better, use 
> guid_is_null().

guid_is_null() is not techinically enough given the specification
mandates that the entire entry is zero.


> > +	if (!buf)
> > +		return -ENOMEM;
> > +
> > +	guard(mutex)(&snp_cmd_mutex);
> > +
> > +	/* Check if the VMPCK is not empty */
> > +	if (is_vmpck_empty(snp_dev)) {
> > +		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> > +		return -ENOTTY;
> > +	}
> > +
> > +	cert_table = buf + report_size;
> > +	struct snp_ext_report_req ext_req = {
> > +		.data = { .vmpl = desc->privlevel },
> > +		.certs_address = (__u64)cert_table,
> > +		.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,
> > +		.exitinfo2 = 0xff,
> 
> Not sure we need this line with 0xff.
> 
> The GHCB spec says the hypervisor sets it, not the guest. And I could 
> not figure out why exactly snp_guest_ioctl() does "input.exitinfo2 = 
> 0xff", my best guest it is to catch GHCB not being called before copying 
> memory to user.

It does mostly seem that way, but given this is plumbed deep into
handle_guest_request() I figure might as well keep common semantics.

> > +	};
> > +	struct snp_req_resp io = {
> > +		.req_data = KERNEL_SOCKPTR(&ext_req),
> > +		.resp_data = KERNEL_SOCKPTR(buf),
> > +	};
> > +
> > +	ret = get_ext_report(snp_dev, &input, &io);
> > +
> 
> Unnecessary empty line.

ok.

> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	memcpy(&hdr, buf, sizeof(hdr));
> > +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
> > +		return -EINVAL;
> > +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> > +		return -EINVAL;
> > +	if (hdr.status)
> > +		return -ENXIO;
> > +	if ((hdr.report_size + sizeof(hdr)) > report_size)
> > +		return -ENOMEM;
> > +
> > +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> > +	if (!rbuf)
> > +		return -ENOMEM;
> > +
> > +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> > +	report->outblob = no_free_ptr(rbuf);
> > +	report->outblob_len = hdr.report_size;
> > +
> > +	certs_size = 0;
> > +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> > +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
> > +			break;
> > +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
> > +	}
> > +
> > +	/* No certs to report */
> > +	if (!certs_size)
> 
> Nit: WARN_ON_ONCE(i) here?

Seems harsh for what could only be a firmware bug, panic_on_warn users
would not appreciate crashing the kernel over something recoverable like
this.

> 
> > +		return 0;
> > +
> > +	/*
> > +	 * cert_table reports more data than fits in ext_size the
> > +	 * userspace cert_table walker can decide what happens next,
> > +	 * truncate the output
> > +	 */
> > +	if (certs_size > ext_size)
> > +		certs_size = ext_size;
> 
> This sounds more like the HV provided a broken table with offset(s) 
> ouside of the certs buffer. The HV is expected instead return 
> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the 
> guest to retry.

The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
prepared to retry. Retry support would be a follow-on new capability.

> > +
> > +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> > +	if (!cbuf)
> > +		return -ENOMEM;
> 
> In a such (unlikely) event the function returns an error but does not 
> free report->outblob which is going to leak if consequent call succeded. 
> This new no_free_ptr business is confusing at times :(

If this fails it results in the attribute read failing and
read_generation does not advance. The next read attempt will free the
partially completed report and retry, or the driver gets unloaded and
the partially completed report is freed at that time.

> 
> 
> > +
> > +	memcpy(cbuf, cert_table, certs_size);
> > +	report->auxblob = no_free_ptr(cbuf);
> > +	report->auxblob_len = certs_size;
> 
> 
> Aaaand, it works, so:
> 
> Tested-by: Alexey Kardashevskiy <aik@amd.com>

Thanks!

Did you happen to test @auxblob population? I was not able to test that
path on the hardware I tried.

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy]

On 17/10/23 15:07, Dan Williams wrote:
> Alexey Kardashevskiy wrote:
>> On 13/10/23 13:14, 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 data that
>>> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
>>> retrieving the report blob via the TSM interface utility,
>>> assuming no nonce and VMPL==2:
>>>
>>>       report=/sys/kernel/config/tsm/report/report0
>>>       mkdir $report
>>>       echo 2 > $report/privlevel
>>>       dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>
>> Is not this one a "nonce"?
> 
> No, a nonce would come from the verifier to be mixed with the 64-bytes
> that the guest uses to establish a shared secret. In this case this is
> just showing the mechanics of the ABI with those security best practices
> set aside.
> 
>>
>>>       hexdump -C $report/outblob # SNP report
>>>       hexdump -C $report/auxblob # cert_table
>>>       rmdir $report
>>>
>>> Given that the platform implementation is free to return empty
>>> certificate data if none is available it lets configfs-tsm be simplified
>>> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
>>> SNP_GET_REPORT alone.
>>>
>>> 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.
>>>
>>> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
>>> Cc: Borislav Petkov <bp@alien8.de>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Cc: Dionna Glaze <dionnaglaze@google.com>
>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 +++++++++++++++++++++++++++++++
>>>    2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
>>>    
>>> @@ -768,6 +770,129 @@ 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
>>
>> There is one already - SEV_RET_INVALID_PARAM, defined in "Secure
>> Encrypted Virtualization API".
> 
> Ok, indeed I took this from Jeremi without checking for other
> definitions.
> 
>>
>>> +#define SNP_REPORT_INVALID_KEY_SEL 0x27
>>
>> This one needs to be defined in include/uapi/linux/psp-sev.h's sev_ret_code.
> 
> Ah, ok, will move.
> 
>>
>>> +
>>> +struct snp_msg_cert_entry {
>>> +	unsigned char guid[16];
>>> +	u32 offset;
>>> +	u32 length;
>>> +};
>>> +
>>> +static int sev_report_new(struct tsm_report *report, void *data)
>>> +{
>>> +	static const struct snp_msg_cert_entry zero_ent = { 0 };
>>> +	struct snp_msg_cert_entry *cert_table;
>>> +	struct tsm_desc *desc = &report->desc;
>>> +	struct snp_guest_dev *snp_dev = data;
>>> +	struct snp_msg_report_resp_hdr hdr;
>>> +	const int report_size = SZ_4K;
>>> +	const int ext_size = SEV_FW_BLOB_MAX_SIZE;
>>
>> These two are size_t. Or u32. "int" is just weird :)
> 
> Nothing is bigger than a few kb here, but ok.

If it all was just "unsinged", I would not bother commenting :)

> 
>>
>>> +	int ret, size = report_size + ext_size;
>>> +	u32 certs_size, i;
>>
>> @certs_size is size_t (as it is copied to ->auxblob_len in the end), and
>> @size is size_t as well.
>>
>> @i is just "unsigned", can be declared right in the "for" below?
> 
> ok.
> 
>>
>>> +
>>> +	if (desc->inblob_len != 64)
>>
>> 64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
>> May be even BUILD_BUG_ON(TSM_INBLOB_MAX != sizeof(ext_req.data.user_data)) ?
>>
>>> +		return -EINVAL;
>>> +
>>> +	void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
>>
>> I did not realize declaring variables in a middle of a scope is allowed
>> now :)
> 
> Yes, it's been allowed in loop declaration for a few kernels now and the
> new __free() helper in cleanup.h will make this model more prominent. My
> sense is that it is still not open season on mid-function declarations,
> but __attribute__((__cleanup__())) usage needs it.
> 
>> Since you are doing this, move zero_ent below. Or, better, use
>> guid_is_null().
> 
> guid_is_null() is not techinically enough given the specification
> mandates that the entire entry is zero.

Well technically it says that guid, offset and length must be zeroes. 
So, (guid_is_null() && !offset && !length) and no yet another static 
declaration of a bunch of zeroes far away. I even wonder if there is a 
helper to check if some memory is all zeroes :) Up to you.


>>> +	if (!buf)
>>> +		return -ENOMEM;
>>> +
>>> +	guard(mutex)(&snp_cmd_mutex);
>>> +
>>> +	/* Check if the VMPCK is not empty */
>>> +	if (is_vmpck_empty(snp_dev)) {
>>> +		dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
>>> +		return -ENOTTY;
>>> +	}
>>> +
>>> +	cert_table = buf + report_size;
>>> +	struct snp_ext_report_req ext_req = {
>>> +		.data = { .vmpl = desc->privlevel },
>>> +		.certs_address = (__u64)cert_table,
>>> +		.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,
>>> +		.exitinfo2 = 0xff,
>>
>> Not sure we need this line with 0xff.
>>
>> The GHCB spec says the hypervisor sets it, not the guest. And I could
>> not figure out why exactly snp_guest_ioctl() does "input.exitinfo2 =
>> 0xff", my best guest it is to catch GHCB not being called before copying
>> memory to user.
> 
> It does mostly seem that way, but given this is plumbed deep into
> handle_guest_request() I figure might as well keep common semantics.
> 
>>> +	};
>>> +	struct snp_req_resp io = {
>>> +		.req_data = KERNEL_SOCKPTR(&ext_req),
>>> +		.resp_data = KERNEL_SOCKPTR(buf),
>>> +	};
>>> +
>>> +	ret = get_ext_report(snp_dev, &input, &io);
>>> +
>>
>> Unnecessary empty line.
> 
> ok.
> 
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	memcpy(&hdr, buf, sizeof(hdr));
>>> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
>>> +		return -EINVAL;
>>> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
>>> +		return -EINVAL;
>>> +	if (hdr.status)
>>> +		return -ENXIO;
>>> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
>>> +		return -ENOMEM;
>>> +
>>> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
>>> +	if (!rbuf)
>>> +		return -ENOMEM;
>>> +
>>> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
>>> +	report->outblob = no_free_ptr(rbuf);
>>> +	report->outblob_len = hdr.report_size;
>>> +
>>> +	certs_size = 0;
>>> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
>>> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
>>> +			break;
>>> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
>>> +	}
>>> +
>>> +	/* No certs to report */
>>> +	if (!certs_size)
>>
>> Nit: WARN_ON_ONCE(i) here?
> 
> Seems harsh for what could only be a firmware bug, panic_on_warn users
> would not appreciate crashing the kernel over something recoverable like
> this.

This would a HV bug as certificates come from the KVM. And it is 
(slightly) more likely that the HV is trying to trigger buffer overrun 
in the guest.

>>
>>> +		return 0;
>>> +
>>> +	/*
>>> +	 * cert_table reports more data than fits in ext_size the
>>> +	 * userspace cert_table walker can decide what happens next,
>>> +	 * truncate the output
>>> +	 */
>>> +	if (certs_size > ext_size)
>>> +		certs_size = ext_size;
>>
>> This sounds more like the HV provided a broken table with offset(s)
>> ouside of the certs buffer. The HV is expected instead return
>> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
>> guest to retry.
> 
> The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
> prepared to retry. Retry support would be a follow-on new capability.

My point is that you should not get into the situation when this 
calculated certs_size is greater than ext_size. If this is the case 
because someone sent too many certificaties via /dev/sev or kvmfd on the 
host, the GHCB call won't return any certs and will ask for a retry instead.


>>> +
>>> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>> +	if (!cbuf)
>>> +		return -ENOMEM;
>>
>> In a such (unlikely) event the function returns an error but does not
>> free report->outblob which is going to leak if consequent call succeded.
>> This new no_free_ptr business is confusing at times :(
> 
> If this fails it results in the attribute read failing and
> read_generation does not advance. The next read attempt will free the
> partially completed report and retry,

Ah ok. In general, it just feels like every use of no_free_ptr() defeats 
the whole purpose of __free(xxx).

> or the driver gets unloaded and
> the partially completed report is freed at that time.
> 
>>
>>
>>> +
>>> +	memcpy(cbuf, cert_table, certs_size);
>>> +	report->auxblob = no_free_ptr(cbuf);
>>> +	report->auxblob_len = certs_size;
>>
>>
>> Aaaand, it works, so:
>>
>> Tested-by: Alexey Kardashevskiy <aik@amd.com>
> 
> Thanks!
> 
> Did you happen to test @auxblob population? I was not able to test that
> path on the hardware I tried.

Yup. I have a disgusting python script which feeds some dummy certs to 
/dev/sev on the host and checked they travel all the way to this 
configfs nodes.


#! /usr/bin/env python3

#define _IOC_SIZEBITS   13
#define _IOC_DIRBITS    3
#define _IOC_NONE       1U
#define _IOC_READ       2U
#define _IOC_WRITE      4U
#define _IOC(dir,type,nr,size) \
#        (((dir)  << _IOC_DIRSHIFT=30) | \
#         ((type) << _IOC_TYPESHIFT=8) | \
#         ((nr)   << _IOC_NRSHIFT=0) | \
#         ((size) << _IOC_SIZESHIFT=16))
#define _IOWR(type,nr,size) 
_IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))

import fcntl, array, sys, binascii, pprint, re, uuid, subprocess, os, 
struct, uuid

# This is:  cat ~/s/sev/sev-guest | ssh $1 python3 -
if len(sys.argv) > 1 and '-' not in sys.argv:
	remote_host = sys.argv[1]
	print("*** Running remotely on {}".format(remote_host))
	f = open(os.path.realpath(__file__), 'r')
	ssh = subprocess.Popen(["ssh", remote_host, "python3", "-"], stdin = f)
	ssh.communicate()
	sys.exit(0)


_IOC_READ = 2
_IOC_WRITE = 4

def _IOC(d, t, n, s): return (d << (16 + 13)) | (t << 8) | n | (s << 16)
def _IOWR(t, n, s): return _IOC(_IOC_READ | _IOC_WRITE, t, n, s)

SEV_IOC_TYPE = ord('S')
sev_issue_cmd_size = 16
SEV_ISSUE_CMD = _IOWR(SEV_IOC_TYPE, 0x0, sev_issue_cmd_size)

SNP_SET_EXT_CONFIG = 10

def sev_issue_cmd(fd, cmd, data):
	data_ptr = data.buffer_info()[0]
	arg = struct.pack("=LQL", cmd, data_ptr, 0)
	try:
		arg = fcntl.ioctl(fd, SEV_ISSUE_CMD, arg, True)
	except OSError as x:
		return x.errno, 0
	_, _, err = struct.unpack("=LQL", arg)
	return 0, err

# struct cert_table {
#	struct {
#		unsigned char guid[16];
#		uint32 offset;
#		uint32 length;
#	} cert_table_entry[];
# };
def cert_table_entry(cert_table):
	ret = array.array('B')
	hdr = array.array('B')
	offset = (16 + 4 + 4) * (len(cert_table) + 1)
	i = 0
	for i, c in enumerate(cert_table):
		cert = array.array('B', [ord('0') + i] * 32)
		ret += cert
		hdr += array.array('B', c.bytes)
		hdr += array.array('B', struct.pack("=LL", offset, len(cert)))
		offset += len(cert)

	return hdr + array.array('B', [0] * 24) + ret

myuuids = [uuid.UUID("f50ec0b7-f960-400d-91f0-c42a6d44e3d0"),
		uuid.UUID("9f3d7b34-6c0d-11ee-bcd4-f8e4e3857730"),
		uuid.UUID("aabbccdd-eeff-0011-2233-4567890abcde")]
cert_table = cert_table_entry(myuuids)
cert_table += array.array('B', [0] * (4096 - len(cert_table)))

# sev_user_data_ext_snp_config
ext_config = array.array('B', struct.pack("=QQL", 0, 
cert_table.buffer_info()[0], cert_table.buffer_info()[1]))

sev = open("/dev/sev", "w")
ret, err = sev_issue_cmd(sev, SNP_SET_EXT_CONFIG, ext_config)
print("ret = {} err = {}".format(ret, err))

-- 
Alexey

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy]

On 17/10/23 16:35, Alexey Kardashevskiy wrote:
> 
> On 17/10/23 15:07, Dan Williams wrote:
>> Alexey Kardashevskiy wrote:
>>> On 13/10/23 13:14, 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 data that
>>>> the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
>>>> retrieving the report blob via the TSM interface utility,
>>>> assuming no nonce and VMPL==2:
>>>>
>>>>       report=/sys/kernel/config/tsm/report/report0
>>>>       mkdir $report
>>>>       echo 2 > $report/privlevel
>>>>       dd if=/dev/urandom bs=64 count=1 > $report/inblob
>>>
>>> Is not this one a "nonce"?
>>
>> No, a nonce would come from the verifier to be mixed with the 64-bytes
>> that the guest uses to establish a shared secret. In this case this is
>> just showing the mechanics of the ABI with those security best practices
>> set aside.
>>
>>>
>>>>       hexdump -C $report/outblob # SNP report
>>>>       hexdump -C $report/auxblob # cert_table
>>>>       rmdir $report
>>>>
>>>> Given that the platform implementation is free to return empty
>>>> certificate data if none is available it lets configfs-tsm be 
>>>> simplified
>>>> as it only needs to worry about wrapping SNP_GET_EXT_REPORT, and leave
>>>> SNP_GET_REPORT alone.
>>>>
>>>> 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.
>>>>
>>>> Link: 
>>>> http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
>>>> Cc: Borislav Petkov <bp@alien8.de>
>>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>>> Cc: Dionna Glaze <dionnaglaze@google.com>
>>>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>>>> Cc: Jeremi Piotrowski <jpiotrowski@linux.microsoft.com>
>>>> Tested-by: Kuppuswamy Sathyanarayanan 
>>>> <sathyanarayanan.kuppuswamy@linux.intel.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 |  133 
>>>> +++++++++++++++++++++++++++++++
>>>>    2 files changed, 134 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 e5f8f115f4af..f3ca083127af 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>
>>>> @@ -768,6 +770,129 @@ 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
>>>
>>> There is one already - SEV_RET_INVALID_PARAM, defined in "Secure
>>> Encrypted Virtualization API".
>>
>> Ok, indeed I took this from Jeremi without checking for other
>> definitions.
>>
>>>
>>>> +#define SNP_REPORT_INVALID_KEY_SEL 0x27
>>>
>>> This one needs to be defined in include/uapi/linux/psp-sev.h's 
>>> sev_ret_code.
>>
>> Ah, ok, will move.
>>
>>>
>>>> +
>>>> +struct snp_msg_cert_entry {
>>>> +    unsigned char guid[16];
>>>> +    u32 offset;
>>>> +    u32 length;
>>>> +};
>>>> +
>>>> +static int sev_report_new(struct tsm_report *report, void *data)
>>>> +{
>>>> +    static const struct snp_msg_cert_entry zero_ent = { 0 };
>>>> +    struct snp_msg_cert_entry *cert_table;
>>>> +    struct tsm_desc *desc = &report->desc;
>>>> +    struct snp_guest_dev *snp_dev = data;
>>>> +    struct snp_msg_report_resp_hdr hdr;
>>>> +    const int report_size = SZ_4K;
>>>> +    const int ext_size = SEV_FW_BLOB_MAX_SIZE;
>>>
>>> These two are size_t. Or u32. "int" is just weird :)
>>
>> Nothing is bigger than a few kb here, but ok.
> 
> If it all was just "unsinged", I would not bother commenting :)
> 
>>
>>>
>>>> +    int ret, size = report_size + ext_size;
>>>> +    u32 certs_size, i;
>>>
>>> @certs_size is size_t (as it is copied to ->auxblob_len in the end), and
>>> @size is size_t as well.
>>>
>>> @i is just "unsigned", can be declared right in the "for" below?
>>
>> ok.
>>
>>>
>>>> +
>>>> +    if (desc->inblob_len != 64)
>>>
>>> 64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
>>> May be even BUILD_BUG_ON(TSM_INBLOB_MAX != 
>>> sizeof(ext_req.data.user_data)) ?
>>>
>>>> +        return -EINVAL;
>>>> +
>>>> +    void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
>>>
>>> I did not realize declaring variables in a middle of a scope is allowed
>>> now :)
>>
>> Yes, it's been allowed in loop declaration for a few kernels now and the
>> new __free() helper in cleanup.h will make this model more prominent. My
>> sense is that it is still not open season on mid-function declarations,
>> but __attribute__((__cleanup__())) usage needs it.
>>
>>> Since you are doing this, move zero_ent below. Or, better, use
>>> guid_is_null().
>>
>> guid_is_null() is not techinically enough given the specification
>> mandates that the entire entry is zero.
> 
> Well technically it says that guid, offset and length must be zeroes. 
> So, (guid_is_null() && !offset && !length) and no yet another static 
> declaration of a bunch of zeroes far away. I even wonder if there is a 
> helper to check if some memory is all zeroes :) Up to you.
> 
> 
>>>> +    if (!buf)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    guard(mutex)(&snp_cmd_mutex);
>>>> +
>>>> +    /* Check if the VMPCK is not empty */
>>>> +    if (is_vmpck_empty(snp_dev)) {
>>>> +        dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
>>>> +        return -ENOTTY;
>>>> +    }
>>>> +
>>>> +    cert_table = buf + report_size;
>>>> +    struct snp_ext_report_req ext_req = {
>>>> +        .data = { .vmpl = desc->privlevel },
>>>> +        .certs_address = (__u64)cert_table,
>>>> +        .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,
>>>> +        .exitinfo2 = 0xff,
>>>
>>> Not sure we need this line with 0xff.
>>>
>>> The GHCB spec says the hypervisor sets it, not the guest. And I could
>>> not figure out why exactly snp_guest_ioctl() does "input.exitinfo2 =
>>> 0xff", my best guest it is to catch GHCB not being called before copying
>>> memory to user.
>>
>> It does mostly seem that way, but given this is plumbed deep into
>> handle_guest_request() I figure might as well keep common semantics.
>>
>>>> +    };
>>>> +    struct snp_req_resp io = {
>>>> +        .req_data = KERNEL_SOCKPTR(&ext_req),
>>>> +        .resp_data = KERNEL_SOCKPTR(buf),
>>>> +    };
>>>> +
>>>> +    ret = get_ext_report(snp_dev, &input, &io);
>>>> +
>>>
>>> Unnecessary empty line.
>>
>> ok.
>>
>>>
>>>> +    if (ret)
>>>> +        return ret;
>>>> +
>>>> +    memcpy(&hdr, buf, sizeof(hdr));
>>>> +    if (hdr.status == SNP_REPORT_INVALID_PARAM)
>>>> +        return -EINVAL;
>>>> +    if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
>>>> +        return -EINVAL;
>>>> +    if (hdr.status)
>>>> +        return -ENXIO;
>>>> +    if ((hdr.report_size + sizeof(hdr)) > report_size)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
>>>> +    if (!rbuf)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
>>>> +    report->outblob = no_free_ptr(rbuf);
>>>> +    report->outblob_len = hdr.report_size;
>>>> +
>>>> +    certs_size = 0;
>>>> +    for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); 
>>>> i++) {
>>>> +        if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
>>>> +            break;
>>>> +        certs_size = max(certs_size, cert_table[i].offset + 
>>>> cert_table[i].length);
>>>> +    }
>>>> +
>>>> +    /* No certs to report */
>>>> +    if (!certs_size)
>>>
>>> Nit: WARN_ON_ONCE(i) here?
>>
>> Seems harsh for what could only be a firmware bug, panic_on_warn users
>> would not appreciate crashing the kernel over something recoverable like
>> this.
> 
> This would a HV bug as certificates come from the KVM. And it is 
> (slightly) more likely that the HV is trying to trigger buffer overrun 
> in the guest.
> 
>>>
>>>> +        return 0;
>>>> +
>>>> +    /*
>>>> +     * cert_table reports more data than fits in ext_size the
>>>> +     * userspace cert_table walker can decide what happens next,
>>>> +     * truncate the output
>>>> +     */
>>>> +    if (certs_size > ext_size)
>>>> +        certs_size = ext_size;
>>>
>>> This sounds more like the HV provided a broken table with offset(s)
>>> ouside of the certs buffer. The HV is expected instead return
>>> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
>>> guest to retry.
>>
>> The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
>> prepared to retry. Retry support would be a follow-on new capability.
> 
> My point is that you should not get into the situation when this 
> calculated certs_size is greater than ext_size. If this is the case 
> because someone sent too many certificaties via /dev/sev or kvmfd on the 
> host, the GHCB call won't return any certs and will ask for a retry 
> instead.
> 
> 
>>>> +
>>>> +    void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>>> +    if (!cbuf)
>>>> +        return -ENOMEM;
>>>
>>> In a such (unlikely) event the function returns an error but does not
>>> free report->outblob which is going to leak if consequent call succeded.
>>> This new no_free_ptr business is confusing at times :(
>>
>> If this fails it results in the attribute read failing and
>> read_generation does not advance. The next read attempt will free the
>> partially completed report and retry,
> 
> Ah ok. In general, it just feels like every use of no_free_ptr() defeats 
> the whole purpose of __free(xxx).
> 
>> or the driver gets unloaded and
>> the partially completed report is freed at that time.
>>
>>>
>>>
>>>> +
>>>> +    memcpy(cbuf, cert_table, certs_size);
>>>> +    report->auxblob = no_free_ptr(cbuf);
>>>> +    report->auxblob_len = certs_size;
>>>
>>>
>>> Aaaand, it works, so:
>>>
>>> Tested-by: Alexey Kardashevskiy <aik@amd.com>
>>
>> Thanks!
>>
>> Did you happen to test @auxblob population? I was not able to test that
>> path on the hardware I tried.
> 
> Yup. I have a disgusting python script which feeds some dummy certs to 
> /dev/sev on the host and checked they travel all the way to this 
> configfs nodes.
> 
> 
> #! /usr/bin/env python3
> 
> #define _IOC_SIZEBITS   13
> #define _IOC_DIRBITS    3
> #define _IOC_NONE       1U
> #define _IOC_READ       2U
> #define _IOC_WRITE      4U
> #define _IOC(dir,type,nr,size) \
> #        (((dir)  << _IOC_DIRSHIFT=30) | \
> #         ((type) << _IOC_TYPESHIFT=8) | \
> #         ((nr)   << _IOC_NRSHIFT=0) | \
> #         ((size) << _IOC_SIZESHIFT=16))
> #define _IOWR(type,nr,size) 
> _IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHECK(size)))
> 
> import fcntl, array, sys, binascii, pprint, re, uuid, subprocess, os, 
> struct, uuid
> 
> # This is:  cat ~/s/sev/sev-guest | ssh $1 python3 -
> if len(sys.argv) > 1 and '-' not in sys.argv:
>      remote_host = sys.argv[1]
>      print("*** Running remotely on {}".format(remote_host))
>      f = open(os.path.realpath(__file__), 'r')
>      ssh = subprocess.Popen(["ssh", remote_host, "python3", "-"], stdin 
> = f)
>      ssh.communicate()
>      sys.exit(0)
> 
> 
> _IOC_READ = 2
> _IOC_WRITE = 4
> 
> def _IOC(d, t, n, s): return (d << (16 + 13)) | (t << 8) | n | (s << 16)
> def _IOWR(t, n, s): return _IOC(_IOC_READ | _IOC_WRITE, t, n, s)
> 
> SEV_IOC_TYPE = ord('S')
> sev_issue_cmd_size = 16
> SEV_ISSUE_CMD = _IOWR(SEV_IOC_TYPE, 0x0, sev_issue_cmd_size)
> 
> SNP_SET_EXT_CONFIG = 10
> 
> def sev_issue_cmd(fd, cmd, data):
>      data_ptr = data.buffer_info()[0]
>      arg = struct.pack("=LQL", cmd, data_ptr, 0)
>      try:
>          arg = fcntl.ioctl(fd, SEV_ISSUE_CMD, arg, True)
>      except OSError as x:
>          return x.errno, 0
>      _, _, err = struct.unpack("=LQL", arg)
>      return 0, err
> 
> # struct cert_table {
> #    struct {
> #        unsigned char guid[16];
> #        uint32 offset;
> #        uint32 length;
> #    } cert_table_entry[];
> # };
> def cert_table_entry(cert_table):
>      ret = array.array('B')
>      hdr = array.array('B')
>      offset = (16 + 4 + 4) * (len(cert_table) + 1)
>      i = 0
>      for i, c in enumerate(cert_table):
>          cert = array.array('B', [ord('0') + i] * 32)
>          ret += cert
>          hdr += array.array('B', c.bytes)
>          hdr += array.array('B', struct.pack("=LL", offset, len(cert)))
>          offset += len(cert)
> 
>      return hdr + array.array('B', [0] * 24) + ret
> 
> myuuids = [uuid.UUID("f50ec0b7-f960-400d-91f0-c42a6d44e3d0"),
>          uuid.UUID("9f3d7b34-6c0d-11ee-bcd4-f8e4e3857730"),
>          uuid.UUID("aabbccdd-eeff-0011-2233-4567890abcde")]
> cert_table = cert_table_entry(myuuids)
> cert_table += array.array('B', [0] * (4096 - len(cert_table)))
> 
> # sev_user_data_ext_snp_config
> ext_config = array.array('B', struct.pack("=QQL", 0, 
> cert_table.buffer_info()[0], cert_table.buffer_info()[1]))
> 
> sev = open("/dev/sev", "w")
> ret, err = sev_issue_cmd(sev, SNP_SET_EXT_CONFIG, ext_config)
> print("ret = {} err = {}".format(ret, err))
> 

Ah the script's result as it appears in the guest, just in case:


aik@aik-Standard-PC-i440FX-PIIX-1996:~$ hexdump -vC 
/sys/kernel/config/tsm/report/report0/auxblob
00000000  f5 0e c0 b7 f9 60 40 0d  91 f0 c4 2a 6d 44 e3 d0 
|.....`@....*mD..|
00000010  60 00 00 00 20 00 00 00  9f 3d 7b 34 6c 0d 11 ee  |`... 
....={4l...|
00000020  bc d4 f8 e4 e3 85 77 30  80 00 00 00 20 00 00 00 
|......w0.... ...|
00000030  aa bb cc dd ee ff 00 11  22 33 45 67 89 0a bc de 
|........"3Eg....|
00000040  a0 00 00 00 20 00 00 00  00 00 00 00 00 00 00 00  |.... 
...........|
00000050  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00 
|................|
00000060  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 
|0000000000000000|
00000070  30 30 30 30 30 30 30 30  30 30 30 30 30 30 30 30 
|0000000000000000|
00000080  31 31 31 31 31 31 31 31  31 31 31 31 31 31 31 31 
|1111111111111111|
00000090  31 31 31 31 31 31 31 31  31 31 31 31 31 31 31 31 
|1111111111111111|
000000a0  32 32 32 32 32 32 32 32  32 32 32 32 32 32 32 32 
|2222222222222222|
000000b0  32 32 32 32 32 32 32 32  32 32 32 32 32 32 32 32 
|2222222222222222|



-- 
Alexey

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dan Williams]

Alexey Kardashevskiy wrote:
[..]
> > Yes, it's been allowed in loop declaration for a few kernels now and the
> > new __free() helper in cleanup.h will make this model more prominent. My
> > sense is that it is still not open season on mid-function declarations,
> > but __attribute__((__cleanup__())) usage needs it.
> > 
> >> Since you are doing this, move zero_ent below. Or, better, use
> >> guid_is_null().
> > 
> > guid_is_null() is not techinically enough given the specification
> > mandates that the entire entry is zero.
> 
> Well technically it says that guid, offset and length must be zeroes. 
> So, (guid_is_null() && !offset && !length) and no yet another static 
> declaration of a bunch of zeroes far away. I even wonder if there is a 
> helper to check if some memory is all zeroes :) Up to you.

Ok, I switched to comparing each field to zero.

[..]
> >>> +	if (ret)
> >>> +		return ret;
> >>> +
> >>> +	memcpy(&hdr, buf, sizeof(hdr));
> >>> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
> >>> +		return -EINVAL;
> >>> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
> >>> +		return -EINVAL;
> >>> +	if (hdr.status)
> >>> +		return -ENXIO;
> >>> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
> >>> +	if (!rbuf)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
> >>> +	report->outblob = no_free_ptr(rbuf);
> >>> +	report->outblob_len = hdr.report_size;
> >>> +
> >>> +	certs_size = 0;
> >>> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> >>> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
> >>> +			break;
> >>> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
> >>> +	}
> >>> +
> >>> +	/* No certs to report */
> >>> +	if (!certs_size)
> >>
> >> Nit: WARN_ON_ONCE(i) here?
> > 
> > Seems harsh for what could only be a firmware bug, panic_on_warn users
> > would not appreciate crashing the kernel over something recoverable like
> > this.
> 
> This would a HV bug as certificates come from the KVM. And it is 
> (slightly) more likely that the HV is trying to trigger buffer overrun 
> in the guest.

Added a dev_warn_ratelimited(). WARN_ON_ONCE() is too heavy as I expect
panic_on_warn policy has more reason to be deployed in a confidential VM
than other places.

> 
> >>
> >>> +		return 0;
> >>> +
> >>> +	/*
> >>> +	 * cert_table reports more data than fits in ext_size the
> >>> +	 * userspace cert_table walker can decide what happens next,
> >>> +	 * truncate the output
> >>> +	 */
> >>> +	if (certs_size > ext_size)
> >>> +		certs_size = ext_size;
> >>
> >> This sounds more like the HV provided a broken table with offset(s)
> >> ouside of the certs buffer. The HV is expected instead return
> >> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
> >> guest to retry.
> > 
> > The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
> > prepared to retry. Retry support would be a follow-on new capability.
> 
> My point is that you should not get into the situation when this 
> calculated certs_size is greater than ext_size. If this is the case 
> because someone sent too many certificaties via /dev/sev or kvmfd on the 
> host, the GHCB call won't return any certs and will ask for a retry instead.

I understand, but given this needs to walk the entries anyway to size
the buffer correctly this sanity check is "free".

> >>> +
> >>> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> >>> +	if (!cbuf)
> >>> +		return -ENOMEM;
> >>
> >> In a such (unlikely) event the function returns an error but does not
> >> free report->outblob which is going to leak if consequent call succeded.
> >> This new no_free_ptr business is confusing at times :(
> > 
> > If this fails it results in the attribute read failing and
> > read_generation does not advance. The next read attempt will free the
> > partially completed report and retry,
> 
> Ah ok. In general, it just feels like every use of no_free_ptr() defeats 
> the whole purpose of __free(xxx).

no_free_ptr() is there to say "we correctly populated this buffer,
there are no more error returns in this function, transition the
responsibility of freeing this buffer to the object it was assigned".

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Alexey Kardashevskiy]

On 19/10/23 15:43, Dan Williams wrote:
> Alexey Kardashevskiy wrote:
> [..]
>>> Yes, it's been allowed in loop declaration for a few kernels now and the
>>> new __free() helper in cleanup.h will make this model more prominent. My
>>> sense is that it is still not open season on mid-function declarations,
>>> but __attribute__((__cleanup__())) usage needs it.
>>>
>>>> Since you are doing this, move zero_ent below. Or, better, use
>>>> guid_is_null().
>>>
>>> guid_is_null() is not techinically enough given the specification
>>> mandates that the entire entry is zero.
>>
>> Well technically it says that guid, offset and length must be zeroes.
>> So, (guid_is_null() && !offset && !length) and no yet another static
>> declaration of a bunch of zeroes far away. I even wonder if there is a
>> helper to check if some memory is all zeroes :) Up to you.
> 
> Ok, I switched to comparing each field to zero.
> 
> [..]
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	memcpy(&hdr, buf, sizeof(hdr));
>>>>> +	if (hdr.status == SNP_REPORT_INVALID_PARAM)
>>>>> +		return -EINVAL;
>>>>> +	if (hdr.status == SNP_REPORT_INVALID_KEY_SEL)
>>>>> +		return -EINVAL;
>>>>> +	if (hdr.status)
>>>>> +		return -ENXIO;
>>>>> +	if ((hdr.report_size + sizeof(hdr)) > report_size)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	void *rbuf __free(kvfree) = kvzalloc(hdr.report_size, GFP_KERNEL);
>>>>> +	if (!rbuf)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	memcpy(rbuf, buf + sizeof(hdr), hdr.report_size);
>>>>> +	report->outblob = no_free_ptr(rbuf);
>>>>> +	report->outblob_len = hdr.report_size;
>>>>> +
>>>>> +	certs_size = 0;
>>>>> +	for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
>>>>> +		if (memcmp(&cert_table[i], &zero_ent, sizeof(zero_ent)) == 0)
>>>>> +			break;
>>>>> +		certs_size = max(certs_size, cert_table[i].offset + cert_table[i].length);
>>>>> +	}
>>>>> +
>>>>> +	/* No certs to report */
>>>>> +	if (!certs_size)
>>>>
>>>> Nit: WARN_ON_ONCE(i) here?
>>>
>>> Seems harsh for what could only be a firmware bug, panic_on_warn users
>>> would not appreciate crashing the kernel over something recoverable like
>>> this.
>>
>> This would a HV bug as certificates come from the KVM. And it is
>> (slightly) more likely that the HV is trying to trigger buffer overrun
>> in the guest.
> 
> Added a dev_warn_ratelimited(). WARN_ON_ONCE() is too heavy as I expect
> panic_on_warn policy has more reason to be deployed in a confidential VM
> than other places. >>>>
>>>>> +		return 0;
>>>>> +
>>>>> +	/*
>>>>> +	 * cert_table reports more data than fits in ext_size the
>>>>> +	 * userspace cert_table walker can decide what happens next,
>>>>> +	 * truncate the output
>>>>> +	 */
>>>>> +	if (certs_size > ext_size)
>>>>> +		certs_size = ext_size;
>>>>
>>>> This sounds more like the HV provided a broken table with offset(s)
>>>> ouside of the certs buffer. The HV is expected instead return
>>>> SW_EXITINFO2=0x0000000100000000 and RBX=requred_pages_number, and the
>>>> guest to retry.
>>>
>>> The existence of SEV_FW_BLOB_MAX_SIZE suggests the driver is not
>>> prepared to retry. Retry support would be a follow-on new capability.
>>
>> My point is that you should not get into the situation when this
>> calculated certs_size is greater than ext_size. If this is the case
>> because someone sent too many certificaties via /dev/sev or kvmfd on the
>> host, the GHCB call won't return any certs and will ask for a retry instead.
> 
> I understand, but given this needs to walk the entries anyway to size
> the buffer correctly this sanity check is "free".

I do not mind the check at all but this hides a potential bug or 
malicious misbehavior attempt. I'd think a CoCo VM is very paranoid 
about such things.


>>>>> +
>>>>> +	void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>>>> +	if (!cbuf)
>>>>> +		return -ENOMEM;
>>>>
>>>> In a such (unlikely) event the function returns an error but does not
>>>> free report->outblob which is going to leak if consequent call succeded.
>>>> This new no_free_ptr business is confusing at times :(
>>>
>>> If this fails it results in the attribute read failing and
>>> read_generation does not advance. The next read attempt will free the
>>> partially completed report and retry,
>>
>> Ah ok. In general, it just feels like every use of no_free_ptr() defeats
>> the whole purpose of __free(xxx).
> 
> no_free_ptr() is there to say "we correctly populated this buffer,
> there are no more error returns in this function, transition the
> responsibility of freeing this buffer to the object it was assigned".


That's what I thought but there is one error return between the first 
no_free_ptr() and "return 0". I agree there is no outblob leak after all 
but the blob hangs around for some time and all this __free() machinery 
is supposed to auto-clean everything right on the spot before returning 
-ENOMEM. Grouping all these no_free_ptr() in the end would make it 
cleaner... Again, I do not insist.

And btw thanks for doing this! I never liked those ioctls :)


-- 
Alexey

Re: [PATCH v6 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT

[Dan Williams]

Alexey Kardashevskiy wrote:
> > +
> > +	if (desc->inblob_len != 64)
> 
> 64 is either ext_req.data.user_data or TSM_INBLOB_MAX really.
> May be even BUILD_BUG_ON(TSM_INBLOB_MAX != sizeof(ext_req.data.user_data)) ?

It may be the case that other implementations want a larger
TSM_INBLOB_MAX. Now that something else cares about the size of the user
data I think a new SNP_REPORT_USER_DATA_SIZE definition is appropriate.

[PATCH v6 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS

[Dan Williams]

From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

In TDX guest, the attestation process is used to verify the TDX guest
trustworthiness to other entities before provisioning secrets to the
guest. The first step in the attestation process is TDREPORT
generation, which involves getting the guest measurement data in the
format of TDREPORT, which is further used to validate the authenticity
of the TDX guest. TDREPORT by design is integrity-protected and can
only be verified on the local machine.

To support remote verification of the TDREPORT in a SGX-based
attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
(QE) to convert it to a remotely verifiable Quote. SGX QE by design can
only run outside of the TDX guest (i.e. in a host process or in a
normal VM) and guest can use communication channels like vsock or
TCP/IP to send the TDREPORT to the QE. But for security concerns, the
TDX guest may not support these communication channels. To handle such
cases, TDX defines a GetQuote hypercall which can be used by the guest
to request the host VMM to communicate with the SGX QE. More details
about GetQuote hypercall can be found in TDX Guest-Host Communication
Interface (GHCI) for Intel TDX 1.0, section titled
"TDG.VP.VMCALL<GetQuote>".

Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
Computing Guest platforms to get the measurement data via ConfigFS.
Extend the TSM framework and add support to allow an attestation agent
to get the TDX Quote data (included usage example below).

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

GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer, allocate a large enough memory and mark it shared using
set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
for GetQuote requests in the TDX TSM handler.

Although this method reserves a fixed chunk of memory for GetQuote
requests, such one time allocation can help avoid memory fragmentation
related allocation failures later in the uptime of the guest.

Since the Quote generation process is not time-critical or frequently
used, the current version uses a polling model for Quote requests and
it also does not support parallel GetQuote requests.

Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 arch/x86/coco/tdx/tdx.c                 |   21 +++
 arch/x86/include/asm/shared/tdx.h       |    1 
 arch/x86/include/asm/tdx.h              |    2 
 drivers/virt/coco/tdx-guest/Kconfig     |    1 
 drivers/virt/coco/tdx-guest/tdx-guest.c |  229 +++++++++++++++++++++++++++++++
 5 files changed, 253 insertions(+), 1 deletion(-)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..752867b1d11b 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
 }
 EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
 
+/**
+ * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
+ *                         hypercall.
+ * @buf: Address of the directly mapped shared kernel buffer which
+ *       contains TDREPORT. The same buffer will be used by VMM to
+ *       store the generated TD Quote output.
+ * @size: size of the tdquote buffer (4KB-aligned).
+ *
+ * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
+ * v1.0 specification for more information on GetQuote hypercall.
+ * It is used in the TDX guest driver module to get the TD Quote.
+ *
+ * Return 0 on success or error code on failure.
+ */
+u64 tdx_hcall_get_quote(u8 *buf, size_t size)
+{
+	/* Since buf is a shared memory, set the shared (decrypted) bits */
+	return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
 static void __noreturn tdx_panic(const char *msg)
 {
 	struct tdx_hypercall_args args = {
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 7513b3bb69b7..9eab19950f39 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -22,6 +22,7 @@
 
 /* TDX hypercall Leaf IDs */
 #define TDVMCALL_MAP_GPA		0x10001
+#define TDVMCALL_GET_QUOTE		0x10002
 #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
 
 #ifndef __ASSEMBLY__
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 603e6d1e9d4a..ebd1cda4875f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
 
 int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
 
+u64 tdx_hcall_get_quote(u8 *buf, size_t size);
+
 #else
 
 static inline void tdx_early_init(void) { };
diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
index 14246fc2fb02..22dd59e19431 100644
--- a/drivers/virt/coco/tdx-guest/Kconfig
+++ b/drivers/virt/coco/tdx-guest/Kconfig
@@ -1,6 +1,7 @@
 config TDX_GUEST_DRIVER
 	tristate "TDX Guest driver"
 	depends on INTEL_TDX_GUEST
+	select TSM_REPORTS
 	help
 	  The driver provides userspace interface to communicate with
 	  the TDX module to request the TDX guest details like attestation
diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 5e44a0fa69bd..1253bf76b570 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -12,12 +12,60 @@
 #include <linux/mod_devicetable.h>
 #include <linux/string.h>
 #include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/tsm.h>
+#include <linux/sizes.h>
 
 #include <uapi/linux/tdx-guest.h>
 
 #include <asm/cpu_device_id.h>
 #include <asm/tdx.h>
 
+/*
+ * Intel's SGX QE implementation generally uses Quote size less
+ * than 8K (2K Quote data + ~5K of certificate blob).
+ */
+#define GET_QUOTE_BUF_SIZE		SZ_8K
+
+#define GET_QUOTE_CMD_VER		1
+
+/* TDX GetQuote status codes */
+#define GET_QUOTE_SUCCESS		0
+#define GET_QUOTE_IN_FLIGHT		0xffffffffffffffff
+
+/* struct tdx_quote_buf: Format of Quote request buffer.
+ * @version: Quote format version, filled by TD.
+ * @status: Status code of Quote request, filled by VMM.
+ * @in_len: Length of TDREPORT, filled by TD.
+ * @out_len: Length of Quote data, filled by VMM.
+ * @data: Quote data on output or TDREPORT on input.
+ *
+ * More details of Quote request buffer can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_buf {
+	u64 version;
+	u64 status;
+	u32 in_len;
+	u32 out_len;
+	u8 data[];
+};
+
+/* Quote data buffer */
+static void *quote_data;
+
+/* Lock to streamline quote requests */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * GetQuote request timeout in seconds. Expect that 30 seconds
+ * is enough time for QE to respond to any Quote requests.
+ */
+static u32 getquote_timeout = 30;
+
 static long tdx_get_report0(struct tdx_report_req __user *req)
 {
 	u8 *reportdata, *tdreport;
@@ -53,6 +101,154 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
 	return ret;
 }
 
+static void free_quote_buf(void *buf)
+{
+	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+	unsigned int count = len >> PAGE_SHIFT;
+
+	if (set_memory_encrypted((unsigned long)buf, count)) {
+		pr_err("Failed to restore encryption mask for Quote buffer, leak it\n");
+		return;
+	}
+
+	free_pages_exact(buf, len);
+}
+
+static void *alloc_quote_buf(void)
+{
+	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+	unsigned int count = len >> PAGE_SHIFT;
+	void *addr;
+
+	addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
+	if (!addr)
+		return NULL;
+
+	if (set_memory_decrypted((unsigned long)addr, count)) {
+		free_pages_exact(addr, len);
+		return NULL;
+	}
+
+	return addr;
+}
+
+/*
+ * wait_for_quote_completion() - Wait for Quote request completion
+ * @quote_buf: Address of Quote buffer.
+ * @timeout: Timeout in seconds to wait for the Quote generation.
+ *
+ * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
+ * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
+ * while VMM processes the GetQuote request, and will change it to success
+ * or error code after processing is complete. So wait till the status
+ * changes from GET_QUOTE_IN_FLIGHT or the request being timed out.
+ */
+static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
+{
+	int i = 0;
+
+	/*
+	 * Quote requests usually take a few seconds to complete, so waking up
+	 * once per second to recheck the status is fine for this use case.
+	 */
+	while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
+		if (msleep_interruptible(MSEC_PER_SEC))
+			return -EINTR;
+	}
+
+	return (i == timeout) ? -ETIMEDOUT : 0;
+}
+
+static int tdx_report_new(struct tsm_report *report, void *data)
+{
+	u8 *buf, *reportdata = NULL, *tdreport = NULL;
+	struct tdx_quote_buf *quote_buf = quote_data;
+	struct tsm_desc *desc = &report->desc;
+	int ret;
+	u64 err;
+
+	/* TODO: switch to guard(mutex_intr) */
+	if (mutex_lock_interruptible(&quote_lock))
+		return -EINTR;
+
+	/*
+	 * If the previous request is timedout or interrupted, and the
+	 * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
+	 * VMM), don't permit any new request.
+	 */
+	if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
+		ret = -EBUSY;
+		goto done;
+	}
+
+	if (desc->inblob_len != TDX_REPORTDATA_LEN) {
+		ret = -EINVAL;
+		goto done;
+	}
+
+	reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+	if (!reportdata) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
+	if (!tdreport) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	memcpy(reportdata, desc->inblob, desc->inblob_len);
+
+	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
+	ret = tdx_mcall_get_report0(reportdata, tdreport);
+	if (ret) {
+		pr_err("GetReport call failed\n");
+		goto done;
+	}
+
+	memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
+
+	/* Update Quote buffer header */
+	quote_buf->version = GET_QUOTE_CMD_VER;
+	quote_buf->in_len = TDX_REPORT_LEN;
+
+	memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
+
+	err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
+	if (err) {
+		pr_err("GetQuote hypercall failed, status:%llx\n", err);
+		ret = -EIO;
+		goto done;
+	}
+
+	ret = wait_for_quote_completion(quote_buf, getquote_timeout);
+	if (ret) {
+		pr_err("GetQuote request timedout\n");
+		goto done;
+	}
+
+	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
+	if (!buf) {
+		ret = -ENOMEM;
+		goto done;
+	}
+
+	report->outblob = buf;
+	report->outblob_len = quote_buf->out_len;
+
+	/*
+	 * TODO: parse the PEM-formatted cert chain out of the quote buffer when
+	 * provided
+	 */
+done:
+	mutex_unlock(&quote_lock);
+	kfree(reportdata);
+	kfree(tdreport);
+
+	return ret;
+}
+
 static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
 			    unsigned long arg)
 {
@@ -82,17 +278,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
 };
 MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
 
+static const struct tsm_ops tdx_tsm_ops = {
+	.name = KBUILD_MODNAME,
+	.report_new = tdx_report_new,
+};
+
 static int __init tdx_guest_init(void)
 {
+	int ret;
+
 	if (!x86_match_cpu(tdx_guest_ids))
 		return -ENODEV;
 
-	return misc_register(&tdx_misc_dev);
+	ret = misc_register(&tdx_misc_dev);
+	if (ret)
+		return ret;
+
+	quote_data = alloc_quote_buf();
+	if (!quote_data) {
+		pr_err("Failed to allocate Quote buffer\n");
+		ret = -ENOMEM;
+		goto free_misc;
+	}
+
+	ret = tsm_register(&tdx_tsm_ops, NULL, NULL);
+	if (ret)
+		goto free_quote;
+
+	return 0;
+
+free_quote:
+	free_quote_buf(quote_data);
+free_misc:
+	misc_deregister(&tdx_misc_dev);
+
+	return ret;
 }
 module_init(tdx_guest_init);
 
 static void __exit tdx_guest_exit(void)
 {
+	tsm_unregister(&tdx_tsm_ops);
+	free_quote_buf(quote_data);
 	misc_deregister(&tdx_misc_dev);
 }
 module_exit(tdx_guest_exit);

Re: [PATCH v6 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS

[Peter Gonda]

On Thu, Oct 12, 2023 at 8:14 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>
> In TDX guest, the attestation process is used to verify the TDX guest
> trustworthiness to other entities before provisioning secrets to the
> guest. The first step in the attestation process is TDREPORT
> generation, which involves getting the guest measurement data in the
> format of TDREPORT, which is further used to validate the authenticity
> of the TDX guest. TDREPORT by design is integrity-protected and can
> only be verified on the local machine.
>
> To support remote verification of the TDREPORT in a SGX-based
> attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
> (QE) to convert it to a remotely verifiable Quote. SGX QE by design can
> only run outside of the TDX guest (i.e. in a host process or in a
> normal VM) and guest can use communication channels like vsock or
> TCP/IP to send the TDREPORT to the QE. But for security concerns, the
> TDX guest may not support these communication channels. To handle such
> cases, TDX defines a GetQuote hypercall which can be used by the guest
> to request the host VMM to communicate with the SGX QE. More details
> about GetQuote hypercall can be found in TDX Guest-Host Communication
> Interface (GHCI) for Intel TDX 1.0, section titled
> "TDG.VP.VMCALL<GetQuote>".
>
> Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
> Computing Guest platforms to get the measurement data via ConfigFS.
> Extend the TSM framework and add support to allow an attestation agent
> to get the TDX Quote data (included usage example below).
>
>   report=/sys/kernel/config/tsm/report/report0
>   mkdir $report
>   dd if=/dev/urandom bs=64 count=1 > $report/inblob
>   hexdump -C $report/outblob
>   rmdir $report
>
> GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
> with TDREPORT data as input, which is further used by the VMM to copy
> the TD Quote result after successful Quote generation. To create the
> shared buffer, allocate a large enough memory and mark it shared using
> set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
> for GetQuote requests in the TDX TSM handler.
>
> Although this method reserves a fixed chunk of memory for GetQuote
> requests, such one time allocation can help avoid memory fragmentation
> related allocation failures later in the uptime of the guest.
>
> Since the Quote generation process is not time-critical or frequently
> used, the current version uses a polling model for Quote requests and
> it also does not support parallel GetQuote requests.
>
> Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Reviewed-by: Erdem Aktas <erdemaktas@google.com>
> Tested-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

I was able to test this on an SNP machine with VCEKs provisioned and
handed to KVM.

Tested-by: Peter Gonda <pgonda@google.com>

Re: [PATCH v6 0/7] configfs-tsm: Attestation Report ABI

[Tom Lendacky]

On 10/12/23 21:13, Dan Williams wrote:
> Changes since v5 [1]:
> - Dump the raw cert_table via a new @auxblob attribute rather than
>    concatenate the certificate data. (Dionna and Tom)
> - Fix usage of guard(), drop erroneous copy-pasted mutex_unlock().
>    (Tom)
> 
> [1]: http://lore.kernel.org/r/169700203032.779347.11603484721811916604.stgit@dwillia2-xfh.jf.intel.com
> 

For the series:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

> ---
> 
> Merge notes: I am looking for Dave or Boris to pick this up, I believe
> all outstanding comments have been resolved and this has now been
> smoke-tested on AMD and Intel platforms (both v5 and v6).
> 
> ---
> 
> 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 (6):
>        virt: sevguest: Fix passing a stack buffer as a scatterlist target
>        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_ext_report()
>        mm/slab: Add __free() support for kvfree
>        virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
> 
> Kuppuswamy Sathyanarayanan (1):
>        virt: tdx-guest: Add Quote generation support using TSM_REPORTS
> 
> 
>   Documentation/ABI/testing/configfs-tsm  |   82 ++++++
>   MAINTAINERS                             |    8 +
>   arch/x86/coco/tdx/tdx.c                 |   21 ++
>   arch/x86/include/asm/shared/tdx.h       |    1
>   arch/x86/include/asm/tdx.h              |    2
>   drivers/virt/Kconfig                    |    6
>   drivers/virt/Makefile                   |    4
>   drivers/virt/coco/Kconfig               |   14 +
>   drivers/virt/coco/Makefile              |    8 +
>   drivers/virt/coco/sev-guest/Kconfig     |    1
>   drivers/virt/coco/sev-guest/sev-guest.c |  212 ++++++++++++++--
>   drivers/virt/coco/tdx-guest/Kconfig     |    1
>   drivers/virt/coco/tdx-guest/tdx-guest.c |  229 +++++++++++++++++
>   drivers/virt/coco/tsm.c                 |  423 +++++++++++++++++++++++++++++++
>   include/linux/slab.h                    |    2
>   include/linux/tsm.h                     |   68 +++++
>   16 files changed, 1046 insertions(+), 36 deletions(-)
>   create mode 100644 Documentation/ABI/testing/configfs-tsm
>   create mode 100644 drivers/virt/coco/Kconfig
>   create mode 100644 drivers/virt/coco/Makefile
>   create mode 100644 drivers/virt/coco/tsm.c
>   create mode 100644 include/linux/tsm.h
> 
> base-commit: 6465e260f48790807eef06b583b38ca9789b6072