* [PATCH v5 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 5:27 ` [PATCH v5 2/7] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
` (6 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Peter Gonda, Borislav Petkov, Tom Lendacky, Dionna Glaze,
Brijesh Singh, Jeremi Piotrowski, peterz,
sathyanarayanan.kuppuswamy, dave.hansen, bp
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>
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;
}
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 2/7] virt: coco: Add a coco/Makefile and coco/Kconfig
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
2023-10-11 5:27 ` [PATCH v5 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 5:27 ` [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
` (5 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, peterz, sathyanarayanan.kuppuswamy,
dave.hansen, bp
In preparation for adding another coco build target, relieve
drivers/virt/Makefile of the responsibility to track new compilation
unit additions to drivers/virt/coco/, and do the same for
drivers/virt/Kconfig.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
drivers/virt/Kconfig | 6 +-----
drivers/virt/Makefile | 4 +---
drivers/virt/coco/Kconfig | 9 +++++++++
drivers/virt/coco/Makefile | 7 +++++++
4 files changed, 18 insertions(+), 8 deletions(-)
create mode 100644 drivers/virt/coco/Kconfig
create mode 100644 drivers/virt/coco/Makefile
diff --git a/drivers/virt/Kconfig b/drivers/virt/Kconfig
index f79ab13a5c28..40129b6f0eca 100644
--- a/drivers/virt/Kconfig
+++ b/drivers/virt/Kconfig
@@ -48,10 +48,6 @@ source "drivers/virt/nitro_enclaves/Kconfig"
source "drivers/virt/acrn/Kconfig"
-source "drivers/virt/coco/efi_secret/Kconfig"
-
-source "drivers/virt/coco/sev-guest/Kconfig"
-
-source "drivers/virt/coco/tdx-guest/Kconfig"
+source "drivers/virt/coco/Kconfig"
endif
diff --git a/drivers/virt/Makefile b/drivers/virt/Makefile
index e9aa6fc96fab..f29901bd7820 100644
--- a/drivers/virt/Makefile
+++ b/drivers/virt/Makefile
@@ -9,6 +9,4 @@ obj-y += vboxguest/
obj-$(CONFIG_NITRO_ENCLAVES) += nitro_enclaves/
obj-$(CONFIG_ACRN_HSM) += acrn/
-obj-$(CONFIG_EFI_SECRET) += coco/efi_secret/
-obj-$(CONFIG_SEV_GUEST) += coco/sev-guest/
-obj-$(CONFIG_INTEL_TDX_GUEST) += coco/tdx-guest/
+obj-y += coco/
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
new file mode 100644
index 000000000000..fc5c64f04c4a
--- /dev/null
+++ b/drivers/virt/coco/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+source "drivers/virt/coco/efi_secret/Kconfig"
+
+source "drivers/virt/coco/sev-guest/Kconfig"
+
+source "drivers/virt/coco/tdx-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
new file mode 100644
index 000000000000..55302ef719ad
--- /dev/null
+++ b/drivers/virt/coco/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0-only
+#
+# Confidential computing related collateral
+#
+obj-$(CONFIG_EFI_SECRET) += efi_secret/
+obj-$(CONFIG_SEV_GUEST) += sev-guest/
+obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
2023-10-11 5:27 ` [PATCH v5 1/7] virt: sevguest: Fix passing a stack buffer as a scatterlist target Dan Williams
2023-10-11 5:27 ` [PATCH v5 2/7] virt: coco: Add a coco/Makefile and coco/Kconfig Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 6:29 ` Kuppuswamy Sathyanarayanan
2023-10-11 5:27 ` [PATCH v5 4/7] virt: sevguest: Prep for kernel internal get_ext_report() Dan Williams
` (4 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, Dionna Amalie Glaze, James Bottomley,
Peter Gonda, Greg Kroah-Hartman, Samuel Ortiz, Greg Kroah-Hartman,
Thomas Gleixner, peterz, sathyanarayanan.kuppuswamy, dave.hansen,
bp
One of the common operations of a TSM (Trusted Security Module) is to
provide a way for a TVM (confidential computing guest execution
environment) to take a measurement of its launch state, sign it and
submit it to a verifying party. Upon successful attestation that
verifies the integrity of the TVM additional secrets may be deployed.
The concept is common across TSMs, but the implementations are
unfortunately vendor specific. While the industry grapples with a common
definition of this attestation format [1], Linux need not make this
problem worse by defining a new ABI per TSM that wants to perform a
similar operation. The current momentum has been to invent new ioctl-ABI
per TSM per function which at best is an abdication of the kernel's
responsibility to make common infrastructure concepts share common ABI.
The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
not more, is to define a configfs interface to retrieve the TSM-specific
blob.
report=/sys/kernel/config/tsm/report/report0
mkdir $report
dd if=binary_userdata_plus_nonce > $report/inblob
hexdump $report/outblob
This approach later allows for the standardization of the attestation
blob format without needing to invent a new ABI. Once standardization
happens the standard format can be emitted by $report/outblob and
indicated by $report/provider, or a new attribute like
"$report/tcg_coco_report" can emit the standard format alongside the
vendor format.
Review of previous iterations of this interface identified that there is
a need to scale report generation for multiple container environments
[2]. Configfs enables a model where each container can bind mount one or
more report generation item instances. Still, within a container only a
single thread can be manipulating a given configuration instance at a
time. A 'generation' count is provided to detect conflicts between
multiple threads racing to configure a report instance.
The SEV-SNP concepts of "extended reports" and "privilege levels" are
optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
time. The expectation is that those concepts are generic enough that
they may be adopted by other TSM implementations. In other words,
configfs-tsm aims to address a superset of TSM specific functionality
with a common ABI where attributes may appear, or not appear, based on
the set of concepts the implementation supports.
Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Peter Gonda <pgonda@google.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Samuel Ortiz <sameo@rivosinc.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Documentation/ABI/testing/configfs-tsm | 76 ++++++
MAINTAINERS | 8 +
drivers/virt/coco/Kconfig | 5
drivers/virt/coco/Makefile | 1
drivers/virt/coco/tsm.c | 416 ++++++++++++++++++++++++++++++++
include/linux/tsm.h | 68 +++++
6 files changed, 574 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..8c6987ece462
--- /dev/null
+++ b/Documentation/ABI/testing/configfs-tsm
@@ -0,0 +1,76 @@
+What: /sys/kernel/config/tsm/report/$name/inblob
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (WO) Up to 64 bytes of user specified binary data. For replay
+ protection this should include a nonce, but the kernel does not
+ place any restrictions on the content.
+
+What: /sys/kernel/config/tsm/report/$name/outblob
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Binary attestation report generated from @inblob and other
+ options The format of the report is implementation specific
+ where the implementation is conveyed via the @provider
+ attribute.
+
+What: /sys/kernel/config/tsm/report/$name/certs
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Zero or more certificates in concatenated PEM format. Refer
+ to implementation specific documentation on which certificates
+ might be returned.
+
+What: /sys/kernel/config/tsm/report/$name/provider
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) A name for the format-specification of @outblob like
+ "sev-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) If a TSM implementation supports the concept of attestation
+ reports for TVMs running at different privilege levels, like
+ SEV-SNP "VMPL", specify the privilege level via this attribute.
+ The minimum acceptable value is conveyed via @privlevel_floor
+ and the maximum acceptable value is TSM_PRIVLEVEL_MAX (3).
+
+What: /sys/kernel/config/tsm/report/$name/privlevel_floor
+Date: September, 2023
+KernelVersion: v6.7
+Contact: linux-coco@lists.linux.dev
+Description:
+ (RO) Indicates the minimum permissible value that can be written
+ to @privlevel.
diff --git a/MAINTAINERS b/MAINTAINERS
index b19995690904..8acbeb029ba1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21889,6 +21889,14 @@ W: https://github.com/srcres258/linux-doc
T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
F: Documentation/translations/zh_TW/
+TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
+M: Dan Williams <dan.j.williams@intel.com>
+L: linux-coco@lists.linux.dev
+S: Maintained
+F: Documentation/ABI/testing/configfs-tsm
+F: drivers/virt/coco/tsm.c
+F: include/linux/tsm.h
+
TTY LAYER AND SERIAL DRIVERS
M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
M: Jiri Slaby <jirislaby@kernel.org>
diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
index fc5c64f04c4a..87d142c1f932 100644
--- a/drivers/virt/coco/Kconfig
+++ b/drivers/virt/coco/Kconfig
@@ -2,6 +2,11 @@
#
# Confidential computing related collateral
#
+
+config TSM_REPORTS
+ select CONFIGFS_FS
+ tristate
+
source "drivers/virt/coco/efi_secret/Kconfig"
source "drivers/virt/coco/sev-guest/Kconfig"
diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
index 55302ef719ad..18c1aba5edb7 100644
--- a/drivers/virt/coco/Makefile
+++ b/drivers/virt/coco/Makefile
@@ -2,6 +2,7 @@
#
# Confidential computing related collateral
#
+obj-$(CONFIG_TSM_REPORTS) += tsm.o
obj-$(CONFIG_EFI_SECRET) += efi_secret/
obj-$(CONFIG_SEV_GUEST) += sev-guest/
obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
new file mode 100644
index 000000000000..054fec924bea
--- /dev/null
+++ b/drivers/virt/coco/tsm.c
@@ -0,0 +1,416 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/tsm.h>
+#include <linux/err.h>
+#include <linux/slab.h>
+#include <linux/rwsem.h>
+#include <linux/string.h>
+#include <linux/module.h>
+#include <linux/cleanup.h>
+#include <linux/configfs.h>
+
+static struct tsm_provider {
+ const struct tsm_ops *ops;
+ const struct config_item_type *type;
+ void *data;
+} provider;
+static DECLARE_RWSEM(tsm_rwsem);
+
+/**
+ * DOC: Trusted Security Module (TSM) Attestation Report Interface
+ *
+ * The TSM report interface is a common provider of blobs that facilitate
+ * attestation of a TVM (confidential computing guest) by an attestation
+ * service. A TSM report combines a user-defined blob (likely a public-key with
+ * a nonce for a key-exchange protocol) with a signed attestation report. That
+ * combined blob is then used to obtain secrets provided by an agent that can
+ * validate the attestation report. The expectation is that this interface is
+ * invoked infrequently, however configfs allows for multiple agents to
+ * own their own report generation instances to generate reports as
+ * often as needed.
+ *
+ * The attestation report format is TSM provider specific, when / if a standard
+ * materializes that can be published instead of the vendor layout. Until then
+ * the 'provider' attribute indicates the format of 'outblob'. However,
+ * the common "return a list of certs" capability across multiple TSM
+ * implementations is returned in a unified @certs attribute.
+ */
+
+struct tsm_report_state {
+ struct tsm_report report;
+ unsigned long write_generation;
+ unsigned long read_generation;
+ struct config_item cfg;
+};
+
+enum tsm_data_select {
+ TSM_REPORT,
+ TSM_CERTS,
+};
+
+static struct tsm_report *to_tsm_report(struct config_item *cfg)
+{
+ struct tsm_report_state *state =
+ container_of(cfg, struct tsm_report_state, cfg);
+
+ return &state->report;
+}
+
+static struct tsm_report_state *to_state(struct tsm_report *report)
+{
+ return container_of(report, struct tsm_report_state, report);
+}
+
+static int try_advance_write_generation(struct tsm_report *report)
+{
+ struct tsm_report_state *state = to_state(report);
+
+ lockdep_assert_held_write(&tsm_rwsem);
+
+ /*
+ * Malicious or broken userspace has written enough times for
+ * read_generation == write_generation by modular arithmetic without an
+ * interim read. Stop accepting updates until the current report
+ * configuration is read.
+ */
+ if (state->write_generation == state->read_generation - 1)
+ return -EBUSY;
+ state->write_generation++;
+ return 0;
+}
+
+static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
+ const char *buf, size_t len)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ unsigned int val;
+ int rc;
+
+ rc = kstrtouint(buf, 0, &val);
+ if (rc)
+ return rc;
+
+ /*
+ * The valid privilege levels that a TSM might accept, if it accepts a
+ * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
+ * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
+ * than 0.
+ */
+ if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
+ return -EINVAL;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ rc = try_advance_write_generation(report);
+ if (rc)
+ return rc;
+ report->desc.privlevel = val;
+
+ return len;
+}
+CONFIGFS_ATTR_WO(tsm_report_, privlevel);
+
+static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
+ char *buf)
+{
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
+}
+CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
+
+static ssize_t tsm_report_inblob_write(struct config_item *cfg,
+ const void *buf, size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ int rc;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ rc = try_advance_write_generation(report);
+ if (rc)
+ return rc;
+
+ report->desc.inblob_len = count;
+ memcpy(report->desc.inblob, buf, count);
+ return count;
+}
+CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
+
+static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ struct tsm_report_state *state = to_state(report);
+
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%lu\n", state->write_generation);
+}
+CONFIGFS_ATTR_RO(tsm_report_, generation);
+
+static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
+{
+ guard(rwsem_read)(&tsm_rwsem);
+ return sysfs_emit(buf, "%s\n", provider.ops->name);
+}
+CONFIGFS_ATTR_RO(tsm_report_, provider);
+
+static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
+ enum tsm_data_select select)
+{
+ loff_t offset = 0;
+ ssize_t len;
+ u8 *out;
+
+ if (select == TSM_REPORT) {
+ out = report->outblob;
+ len = report->outblob_len;
+ } else {
+ out = report->certs;
+ len = report->certs_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 certs or not.
+ */
+ if (!report->outblob ||
+ state->read_generation != state->write_generation)
+ return -EWOULDBLOCK;
+
+ return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
+ size_t count, enum tsm_data_select select)
+{
+ struct tsm_report_state *state = to_state(report);
+ const struct tsm_ops *ops;
+ ssize_t rc;
+
+ /* try to read from the existing report if present and valid... */
+ rc = read_cached_report(report, buf, count, select);
+ if (rc >= 0 || rc != -EWOULDBLOCK)
+ return rc;
+
+ /* slow path, report may need to be regenerated... */
+ guard(rwsem_write)(&tsm_rwsem);
+ ops = provider.ops;
+ if (!report->desc.inblob_len)
+ return -EINVAL;
+
+ /* did another thread already generate this report? */
+ if (report->outblob &&
+ state->read_generation == state->write_generation)
+ goto out;
+
+ kvfree(report->outblob);
+ kvfree(report->certs);
+ report->outblob = NULL;
+ report->certs = NULL;
+ rc = ops->report_new(report, provider.data);
+ if (rc < 0)
+ return rc;
+ state->read_generation = state->write_generation;
+out:
+ return __read_report(report, buf, count, select);
+}
+
+static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
+ size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+
+ return tsm_report_read(report, buf, count, TSM_REPORT);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
+
+static ssize_t tsm_report_certs_read(struct config_item *cfg, void *buf,
+ size_t count)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+
+ return tsm_report_read(report, buf, count, TSM_CERTS);
+}
+CONFIGFS_BIN_ATTR_RO(tsm_report_, certs, NULL, TSM_OUTBLOB_MAX);
+
+#define TSM_DEFAULT_ATTRS() \
+ &tsm_report_attr_generation, \
+ &tsm_report_attr_provider
+
+static struct configfs_attribute *tsm_report_attrs[] = {
+ TSM_DEFAULT_ATTRS(),
+ NULL,
+};
+
+static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
+ &tsm_report_attr_inblob,
+ &tsm_report_attr_outblob,
+ &tsm_report_attr_certs,
+ NULL,
+};
+
+static struct configfs_attribute *tsm_report_extra_attrs[] = {
+ TSM_DEFAULT_ATTRS(),
+ &tsm_report_attr_privlevel,
+ &tsm_report_attr_privlevel_floor,
+ NULL,
+};
+
+static void tsm_report_item_release(struct config_item *cfg)
+{
+ struct tsm_report *report = to_tsm_report(cfg);
+ struct tsm_report_state *state = to_state(report);
+
+ kvfree(report->certs);
+ kvfree(report->outblob);
+ kfree(state);
+}
+
+static struct configfs_item_operations tsm_report_item_ops = {
+ .release = tsm_report_item_release,
+};
+
+const struct config_item_type tsm_report_default_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_bin_attrs = tsm_report_bin_attrs,
+ .ct_attrs = tsm_report_attrs,
+ .ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_default_type);
+
+const struct config_item_type tsm_report_ext_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_bin_attrs = tsm_report_bin_attrs,
+ .ct_attrs = tsm_report_extra_attrs,
+ .ct_item_ops = &tsm_report_item_ops,
+};
+EXPORT_SYMBOL_GPL(tsm_report_ext_type);
+
+static struct config_item *tsm_report_make_item(struct config_group *group,
+ const char *name)
+{
+ struct tsm_report_state *state;
+
+ guard(rwsem_read)(&tsm_rwsem);
+ if (!provider.ops)
+ return ERR_PTR(-ENXIO);
+
+ state = kzalloc(sizeof(*state), GFP_KERNEL);
+ if (!state)
+ return ERR_PTR(-ENOMEM);
+
+ config_item_init_type_name(&state->cfg, name, provider.type);
+ return &state->cfg;
+}
+
+static struct configfs_group_operations tsm_report_group_ops = {
+ .make_item = tsm_report_make_item,
+};
+
+static const struct config_item_type tsm_reports_type = {
+ .ct_owner = THIS_MODULE,
+ .ct_group_ops = &tsm_report_group_ops,
+};
+
+static const struct config_item_type tsm_root_group_type = {
+ .ct_owner = THIS_MODULE,
+};
+
+static struct configfs_subsystem tsm_configfs = {
+ .su_group = {
+ .cg_item = {
+ .ci_namebuf = "tsm",
+ .ci_type = &tsm_root_group_type,
+ },
+ },
+ .su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
+};
+
+static struct config_group *tsm_report_group;
+
+int tsm_register(const struct tsm_ops *ops, void *priv,
+ const struct config_item_type *type)
+{
+ const struct tsm_ops *conflict;
+
+ if (!type)
+ type = &tsm_report_default_type;
+ if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
+ return -EINVAL;
+
+ guard(rwsem_write)(&tsm_rwsem);
+ conflict = provider.ops;
+ if (conflict) {
+ pr_err("\"%s\" ops already registered\n", conflict->name);
+ return -EBUSY;
+ }
+
+ provider.ops = ops;
+ provider.data = priv;
+ provider.type = type;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_register);
+
+int tsm_unregister(const struct tsm_ops *ops)
+{
+ guard(rwsem_write)(&tsm_rwsem);
+ if (ops != provider.ops)
+ return -EBUSY;
+ provider.ops = NULL;
+ provider.data = NULL;
+ provider.type = NULL;
+ return 0;
+}
+EXPORT_SYMBOL_GPL(tsm_unregister);
+
+static int __init tsm_init(void)
+{
+ struct config_group *root = &tsm_configfs.su_group;
+ struct config_group *tsm;
+ int rc;
+
+ config_group_init(root);
+ rc = configfs_register_subsystem(&tsm_configfs);
+ if (rc)
+ return rc;
+
+ tsm = configfs_register_default_group(root, "report",
+ &tsm_reports_type);
+ if (IS_ERR(tsm)) {
+ configfs_unregister_subsystem(&tsm_configfs);
+ return PTR_ERR(tsm);
+ }
+ tsm_report_group = tsm;
+
+ return 0;
+}
+module_init(tsm_init);
+
+static void __exit tsm_exit(void)
+{
+ configfs_unregister_default_group(tsm_report_group);
+ configfs_unregister_subsystem(&tsm_configfs);
+}
+module_exit(tsm_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
diff --git a/include/linux/tsm.h b/include/linux/tsm.h
new file mode 100644
index 000000000000..d6c0752502e0
--- /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
+ * @certs_len: sizeof(@certs)
+ * @certs: concatenated list of PEM formatted certificates
+ */
+struct tsm_report {
+ struct tsm_desc desc;
+ size_t outblob_len;
+ u8 *outblob;
+ size_t certs_len;
+ u8 *certs;
+};
+
+/**
+ * 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 certs
+ * (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 */
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports
2023-10-11 5:27 ` [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
@ 2023-10-11 6:29 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 19+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-10-11 6:29 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Dionna Amalie Glaze, James Bottomley, Peter Gonda,
Greg Kroah-Hartman, Samuel Ortiz, Thomas Gleixner, peterz,
dave.hansen, bp
On 10/10/2023 10:27 PM, Dan Williams wrote:
> One of the common operations of a TSM (Trusted Security Module) is to
> provide a way for a TVM (confidential computing guest execution
> environment) to take a measurement of its launch state, sign it and
> submit it to a verifying party. Upon successful attestation that
> verifies the integrity of the TVM additional secrets may be deployed.
> The concept is common across TSMs, but the implementations are
> unfortunately vendor specific. While the industry grapples with a common
> definition of this attestation format [1], Linux need not make this
> problem worse by defining a new ABI per TSM that wants to perform a
> similar operation. The current momentum has been to invent new ioctl-ABI
> per TSM per function which at best is an abdication of the kernel's
> responsibility to make common infrastructure concepts share common ABI.
>
> The proposal, targeted to conceptually work with TDX, SEV-SNP, COVE if
> not more, is to define a configfs interface to retrieve the TSM-specific
> blob.
>
> report=/sys/kernel/config/tsm/report/report0
> mkdir $report
> dd if=binary_userdata_plus_nonce > $report/inblob
> hexdump $report/outblob
>
> This approach later allows for the standardization of the attestation
> blob format without needing to invent a new ABI. Once standardization
> happens the standard format can be emitted by $report/outblob and
> indicated by $report/provider, or a new attribute like
> "$report/tcg_coco_report" can emit the standard format alongside the
> vendor format.
>
> Review of previous iterations of this interface identified that there is
> a need to scale report generation for multiple container environments
> [2]. Configfs enables a model where each container can bind mount one or
> more report generation item instances. Still, within a container only a
> single thread can be manipulating a given configuration instance at a
> time. A 'generation' count is provided to detect conflicts between
> multiple threads racing to configure a report instance.
>
> The SEV-SNP concepts of "extended reports" and "privilege levels" are
> optionally enabled by selecting 'tsm_report_ext_type' at register_tsm()
> time. The expectation is that those concepts are generic enough that
> they may be adopted by other TSM implementations. In other words,
> configfs-tsm aims to address a superset of TSM specific functionality
> with a common ABI where attributes may appear, or not appear, based on
> the set of concepts the implementation supports.
>
> Link: http://lore.kernel.org/r/64961c3baf8ce_142af829436@dwillia2-xfh.jf.intel.com.notmuch [1]
> Link: http://lore.kernel.org/r/57f3a05e-8fcd-4656-beea-56bb8365ae64@linux.microsoft.com [2]
> Cc: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Cc: Dionna Amalie Glaze <dionnaglaze@google.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Peter Gonda <pgonda@google.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Samuel Ortiz <sameo@rivosinc.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Thomas Gleixner <tglx@linutronix.de>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> Documentation/ABI/testing/configfs-tsm | 76 ++++++
> MAINTAINERS | 8 +
> drivers/virt/coco/Kconfig | 5
> drivers/virt/coco/Makefile | 1
> drivers/virt/coco/tsm.c | 416 ++++++++++++++++++++++++++++++++
> include/linux/tsm.h | 68 +++++
> 6 files changed, 574 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..8c6987ece462
> --- /dev/null
> +++ b/Documentation/ABI/testing/configfs-tsm
> @@ -0,0 +1,76 @@
> +What: /sys/kernel/config/tsm/report/$name/inblob
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (WO) Up to 64 bytes of user specified binary data. For replay
> + protection this should include a nonce, but the kernel does not
> + place any restrictions on the content.
> +
> +What: /sys/kernel/config/tsm/report/$name/outblob
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Binary attestation report generated from @inblob and other
> + options The format of the report is implementation specific
> + where the implementation is conveyed via the @provider
> + attribute.
> +
> +What: /sys/kernel/config/tsm/report/$name/certs
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Zero or more certificates in concatenated PEM format. Refer
> + to implementation specific documentation on which certificates
> + might be returned.
> +
> +What: /sys/kernel/config/tsm/report/$name/provider
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) A name for the format-specification of @outblob like
> + "sev-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) If a TSM implementation supports the concept of attestation
> + reports for TVMs running at different privilege levels, like
> + SEV-SNP "VMPL", specify the privilege level via this attribute.
> + The minimum acceptable value is conveyed via @privlevel_floor
> + and the maximum acceptable value is TSM_PRIVLEVEL_MAX (3).
> +
> +What: /sys/kernel/config/tsm/report/$name/privlevel_floor
> +Date: September, 2023
> +KernelVersion: v6.7
> +Contact: linux-coco@lists.linux.dev
> +Description:
> + (RO) Indicates the minimum permissible value that can be written
> + to @privlevel.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index b19995690904..8acbeb029ba1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21889,6 +21889,14 @@ W: https://github.com/srcres258/linux-doc
> T: git git://github.com/srcres258/linux-doc.git doc-zh-tw
> F: Documentation/translations/zh_TW/
>
> +TRUSTED SECURITY MODULE (TSM) ATTESTATION REPORTS
> +M: Dan Williams <dan.j.williams@intel.com>
> +L: linux-coco@lists.linux.dev
> +S: Maintained
> +F: Documentation/ABI/testing/configfs-tsm
> +F: drivers/virt/coco/tsm.c
> +F: include/linux/tsm.h
> +
> TTY LAYER AND SERIAL DRIVERS
> M: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> M: Jiri Slaby <jirislaby@kernel.org>
> diff --git a/drivers/virt/coco/Kconfig b/drivers/virt/coco/Kconfig
> index fc5c64f04c4a..87d142c1f932 100644
> --- a/drivers/virt/coco/Kconfig
> +++ b/drivers/virt/coco/Kconfig
> @@ -2,6 +2,11 @@
> #
> # Confidential computing related collateral
> #
> +
> +config TSM_REPORTS
> + select CONFIGFS_FS
> + tristate
> +
> source "drivers/virt/coco/efi_secret/Kconfig"
>
> source "drivers/virt/coco/sev-guest/Kconfig"
> diff --git a/drivers/virt/coco/Makefile b/drivers/virt/coco/Makefile
> index 55302ef719ad..18c1aba5edb7 100644
> --- a/drivers/virt/coco/Makefile
> +++ b/drivers/virt/coco/Makefile
> @@ -2,6 +2,7 @@
> #
> # Confidential computing related collateral
> #
> +obj-$(CONFIG_TSM_REPORTS) += tsm.o
> obj-$(CONFIG_EFI_SECRET) += efi_secret/
> obj-$(CONFIG_SEV_GUEST) += sev-guest/
> obj-$(CONFIG_INTEL_TDX_GUEST) += tdx-guest/
> diff --git a/drivers/virt/coco/tsm.c b/drivers/virt/coco/tsm.c
> new file mode 100644
> index 000000000000..054fec924bea
> --- /dev/null
> +++ b/drivers/virt/coco/tsm.c
> @@ -0,0 +1,416 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright(c) 2023 Intel Corporation. All rights reserved. */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/tsm.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +#include <linux/rwsem.h>
> +#include <linux/string.h>
> +#include <linux/module.h>
> +#include <linux/cleanup.h>
> +#include <linux/configfs.h>
> +
> +static struct tsm_provider {
> + const struct tsm_ops *ops;
> + const struct config_item_type *type;
> + void *data;
> +} provider;
> +static DECLARE_RWSEM(tsm_rwsem);
> +
> +/**
> + * DOC: Trusted Security Module (TSM) Attestation Report Interface
> + *
> + * The TSM report interface is a common provider of blobs that facilitate
> + * attestation of a TVM (confidential computing guest) by an attestation
> + * service. A TSM report combines a user-defined blob (likely a public-key with
> + * a nonce for a key-exchange protocol) with a signed attestation report. That
> + * combined blob is then used to obtain secrets provided by an agent that can
> + * validate the attestation report. The expectation is that this interface is
> + * invoked infrequently, however configfs allows for multiple agents to
> + * own their own report generation instances to generate reports as
> + * often as needed.
> + *
> + * The attestation report format is TSM provider specific, when / if a standard
> + * materializes that can be published instead of the vendor layout. Until then
> + * the 'provider' attribute indicates the format of 'outblob'. However,
> + * the common "return a list of certs" capability across multiple TSM
> + * implementations is returned in a unified @certs attribute.
> + */
> +
> +struct tsm_report_state {
> + struct tsm_report report;
> + unsigned long write_generation;
> + unsigned long read_generation;
> + struct config_item cfg;
> +};
> +
> +enum tsm_data_select {
> + TSM_REPORT,
> + TSM_CERTS,
> +};
> +
> +static struct tsm_report *to_tsm_report(struct config_item *cfg)
> +{
> + struct tsm_report_state *state =
> + container_of(cfg, struct tsm_report_state, cfg);
> +
> + return &state->report;
> +}
> +
> +static struct tsm_report_state *to_state(struct tsm_report *report)
> +{
> + return container_of(report, struct tsm_report_state, report);
> +}
> +
> +static int try_advance_write_generation(struct tsm_report *report)
> +{
> + struct tsm_report_state *state = to_state(report);
> +
> + lockdep_assert_held_write(&tsm_rwsem);
> +
> + /*
> + * Malicious or broken userspace has written enough times for
> + * read_generation == write_generation by modular arithmetic without an
> + * interim read. Stop accepting updates until the current report
> + * configuration is read.
> + */
> + if (state->write_generation == state->read_generation - 1)
> + return -EBUSY;
> + state->write_generation++;
> + return 0;
> +}
> +
> +static ssize_t tsm_report_privlevel_store(struct config_item *cfg,
> + const char *buf, size_t len)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + unsigned int val;
> + int rc;
> +
> + rc = kstrtouint(buf, 0, &val);
> + if (rc)
> + return rc;
> +
> + /*
> + * The valid privilege levels that a TSM might accept, if it accepts a
> + * privilege level setting at all, are a max of TSM_PRIVLEVEL_MAX (see
> + * SEV-SNP GHCB) and a minimum of a TSM selected floor value no less
> + * than 0.
> + */
> + if (provider.ops->privlevel_floor > val || val > TSM_PRIVLEVEL_MAX)
> + return -EINVAL;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + rc = try_advance_write_generation(report);
> + if (rc)
> + return rc;
> + report->desc.privlevel = val;
> +
> + return len;
> +}
> +CONFIGFS_ATTR_WO(tsm_report_, privlevel);
> +
> +static ssize_t tsm_report_privlevel_floor_show(struct config_item *cfg,
> + char *buf)
> +{
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%u\n", provider.ops->privlevel_floor);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, privlevel_floor);
> +
> +static ssize_t tsm_report_inblob_write(struct config_item *cfg,
> + const void *buf, size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + int rc;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + rc = try_advance_write_generation(report);
> + if (rc)
> + return rc;
> +
> + report->desc.inblob_len = count;
> + memcpy(report->desc.inblob, buf, count);
> + return count;
> +}
> +CONFIGFS_BIN_ATTR_WO(tsm_report_, inblob, NULL, TSM_INBLOB_MAX);
> +
> +static ssize_t tsm_report_generation_show(struct config_item *cfg, char *buf)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + struct tsm_report_state *state = to_state(report);
> +
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%lu\n", state->write_generation);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, generation);
> +
> +static ssize_t tsm_report_provider_show(struct config_item *cfg, char *buf)
> +{
> + guard(rwsem_read)(&tsm_rwsem);
> + return sysfs_emit(buf, "%s\n", provider.ops->name);
> +}
> +CONFIGFS_ATTR_RO(tsm_report_, provider);
> +
> +static ssize_t __read_report(struct tsm_report *report, void *buf, size_t count,
> + enum tsm_data_select select)
> +{
> + loff_t offset = 0;
> + ssize_t len;
> + u8 *out;
> +
> + if (select == TSM_REPORT) {
> + out = report->outblob;
> + len = report->outblob_len;
> + } else {
> + out = report->certs;
> + len = report->certs_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 certs or not.
> + */
> + if (!report->outblob ||
> + state->read_generation != state->write_generation)
> + return -EWOULDBLOCK;
> +
> + return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_read(struct tsm_report *report, void *buf,
> + size_t count, enum tsm_data_select select)
> +{
> + struct tsm_report_state *state = to_state(report);
> + const struct tsm_ops *ops;
> + ssize_t rc;
> +
> + /* try to read from the existing report if present and valid... */
> + rc = read_cached_report(report, buf, count, select);
> + if (rc >= 0 || rc != -EWOULDBLOCK)
> + return rc;
> +
> + /* slow path, report may need to be regenerated... */
> + guard(rwsem_write)(&tsm_rwsem);
> + ops = provider.ops;
> + if (!report->desc.inblob_len)
> + return -EINVAL;
> +
> + /* did another thread already generate this report? */
> + if (report->outblob &&
> + state->read_generation == state->write_generation)
> + goto out;
> +
> + kvfree(report->outblob);
> + kvfree(report->certs);
> + report->outblob = NULL;
> + report->certs = NULL;
> + rc = ops->report_new(report, provider.data);
> + if (rc < 0)
> + return rc;
> + state->read_generation = state->write_generation;
> +out:
> + return __read_report(report, buf, count, select);
> +}
> +
> +static ssize_t tsm_report_outblob_read(struct config_item *cfg, void *buf,
> + size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> +
> + return tsm_report_read(report, buf, count, TSM_REPORT);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, outblob, NULL, TSM_OUTBLOB_MAX);
> +
> +static ssize_t tsm_report_certs_read(struct config_item *cfg, void *buf,
> + size_t count)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> +
> + return tsm_report_read(report, buf, count, TSM_CERTS);
> +}
> +CONFIGFS_BIN_ATTR_RO(tsm_report_, certs, NULL, TSM_OUTBLOB_MAX);
> +
> +#define TSM_DEFAULT_ATTRS() \
> + &tsm_report_attr_generation, \
> + &tsm_report_attr_provider
> +
> +static struct configfs_attribute *tsm_report_attrs[] = {
> + TSM_DEFAULT_ATTRS(),
> + NULL,
> +};
> +
> +static struct configfs_bin_attribute *tsm_report_bin_attrs[] = {
> + &tsm_report_attr_inblob,
> + &tsm_report_attr_outblob,
> + &tsm_report_attr_certs,
> + NULL,
> +};
> +
> +static struct configfs_attribute *tsm_report_extra_attrs[] = {
> + TSM_DEFAULT_ATTRS(),
> + &tsm_report_attr_privlevel,
> + &tsm_report_attr_privlevel_floor,
> + NULL,
> +};
> +
> +static void tsm_report_item_release(struct config_item *cfg)
> +{
> + struct tsm_report *report = to_tsm_report(cfg);
> + struct tsm_report_state *state = to_state(report);
> +
> + kvfree(report->certs);
> + kvfree(report->outblob);
> + kfree(state);
> +}
> +
> +static struct configfs_item_operations tsm_report_item_ops = {
> + .release = tsm_report_item_release,
> +};
> +
> +const struct config_item_type tsm_report_default_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_bin_attrs = tsm_report_bin_attrs,
> + .ct_attrs = tsm_report_attrs,
> + .ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_default_type);
> +
> +const struct config_item_type tsm_report_ext_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_bin_attrs = tsm_report_bin_attrs,
> + .ct_attrs = tsm_report_extra_attrs,
> + .ct_item_ops = &tsm_report_item_ops,
> +};
> +EXPORT_SYMBOL_GPL(tsm_report_ext_type);
> +
> +static struct config_item *tsm_report_make_item(struct config_group *group,
> + const char *name)
> +{
> + struct tsm_report_state *state;
> +
> + guard(rwsem_read)(&tsm_rwsem);
> + if (!provider.ops)
> + return ERR_PTR(-ENXIO);
> +
> + state = kzalloc(sizeof(*state), GFP_KERNEL);
> + if (!state)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&state->cfg, name, provider.type);
> + return &state->cfg;
> +}
> +
> +static struct configfs_group_operations tsm_report_group_ops = {
> + .make_item = tsm_report_make_item,
> +};
> +
> +static const struct config_item_type tsm_reports_type = {
> + .ct_owner = THIS_MODULE,
> + .ct_group_ops = &tsm_report_group_ops,
> +};
> +
> +static const struct config_item_type tsm_root_group_type = {
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem tsm_configfs = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "tsm",
> + .ci_type = &tsm_root_group_type,
> + },
> + },
> + .su_mutex = __MUTEX_INITIALIZER(tsm_configfs.su_mutex),
> +};
> +
> +static struct config_group *tsm_report_group;
> +
> +int tsm_register(const struct tsm_ops *ops, void *priv,
> + const struct config_item_type *type)
> +{
> + const struct tsm_ops *conflict;
> +
> + if (!type)
> + type = &tsm_report_default_type;
> + if (!(type == &tsm_report_default_type || type == &tsm_report_ext_type))
> + return -EINVAL;
> +
> + guard(rwsem_write)(&tsm_rwsem);
> + conflict = provider.ops;
> + if (conflict) {
> + pr_err("\"%s\" ops already registered\n", conflict->name);
> + return -EBUSY;
> + }
> +
> + provider.ops = ops;
> + provider.data = priv;
> + provider.type = type;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_register);
> +
> +int tsm_unregister(const struct tsm_ops *ops)
> +{
> + guard(rwsem_write)(&tsm_rwsem);
> + if (ops != provider.ops)
> + return -EBUSY;
> + provider.ops = NULL;
> + provider.data = NULL;
> + provider.type = NULL;
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(tsm_unregister);
> +
> +static int __init tsm_init(void)
> +{
> + struct config_group *root = &tsm_configfs.su_group;
> + struct config_group *tsm;
> + int rc;
> +
> + config_group_init(root);
> + rc = configfs_register_subsystem(&tsm_configfs);
> + if (rc)
> + return rc;
> +
> + tsm = configfs_register_default_group(root, "report",
> + &tsm_reports_type);
> + if (IS_ERR(tsm)) {
> + configfs_unregister_subsystem(&tsm_configfs);
> + return PTR_ERR(tsm);
> + }
> + tsm_report_group = tsm;
> +
> + return 0;
> +}
> +module_init(tsm_init);
> +
> +static void __exit tsm_exit(void)
> +{
> + configfs_unregister_default_group(tsm_report_group);
> + configfs_unregister_subsystem(&tsm_configfs);
> +}
> +module_exit(tsm_exit);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Provide Trusted Security Module attestation reports via configfs");
> diff --git a/include/linux/tsm.h b/include/linux/tsm.h
> new file mode 100644
> index 000000000000..d6c0752502e0
> --- /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
> + * @certs_len: sizeof(@certs)
> + * @certs: concatenated list of PEM formatted certificates
> + */
> +struct tsm_report {
> + struct tsm_desc desc;
> + size_t outblob_len;
> + u8 *outblob;
> + size_t certs_len;
> + u8 *certs;
> +};
> +
> +/**
> + * 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 certs
> + * (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 */
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 4/7] virt: sevguest: Prep for kernel internal get_ext_report()
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
` (2 preceding siblings ...)
2023-10-11 5:27 ` [PATCH v5 3/7] configfs-tsm: Introduce a shared ABI for attestation reports Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 5:27 ` [PATCH v5 5/7] mm/slab: Add __free() support for kvfree Dan Williams
` (3 subsequent siblings)
7 siblings, 0 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh,
Kuppuswamy Sathyanarayanan, peterz, sathyanarayanan.kuppuswamy,
dave.hansen, bp
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>
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;
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v5 5/7] mm/slab: Add __free() support for kvfree
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
` (3 preceding siblings ...)
2023-10-11 5:27 ` [PATCH v5 4/7] virt: sevguest: Prep for kernel internal get_ext_report() Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 6:31 ` Kuppuswamy Sathyanarayanan
2023-10-11 5:27 ` [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
` (2 subsequent siblings)
7 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Andrew Morton, Peter Zijlstra, Greg Kroah-Hartman, Pankaj Gupta,
Greg Kroah-Hartman, sathyanarayanan.kuppuswamy, dave.hansen, bp
Allow for the declaration of variables that trigger kvfree() when they
go out of scope. The check for NULL and call to kvfree() can be elided
by the compiler in most cases, otherwise without the NULL check an
unnecessary call to kvfree() may be emitted. Peter proposed a comment
for this detail [1].
Link: http://lore.kernel.org/r/20230816103102.GF980931@hirez.programming.kicks-ass.net [1]
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
include/linux/slab.h | 2 ++
1 file changed, 2 insertions(+)
diff --git a/include/linux/slab.h b/include/linux/slab.h
index 8228d1276a2f..df4c2d45bb86 100644
--- a/include/linux/slab.h
+++ b/include/linux/slab.h
@@ -763,6 +763,8 @@ static inline __alloc_size(1, 2) void *kvcalloc(size_t n, size_t size, gfp_t fla
extern void *kvrealloc(const void *p, size_t oldsize, size_t newsize, gfp_t flags)
__realloc_size(3);
extern void kvfree(const void *addr);
+DEFINE_FREE(kvfree, void *, if (_T) kvfree(_T))
+
extern void kvfree_sensitive(const void *addr, size_t len);
unsigned int kmem_cache_size(struct kmem_cache *s);
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 5/7] mm/slab: Add __free() support for kvfree
2023-10-11 5:27 ` [PATCH v5 5/7] mm/slab: Add __free() support for kvfree Dan Williams
@ 2023-10-11 6:31 ` Kuppuswamy Sathyanarayanan
0 siblings, 0 replies; 19+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-10-11 6:31 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Andrew Morton, Peter Zijlstra, Greg Kroah-Hartman, Pankaj Gupta,
dave.hansen, bp
On 10/10/2023 10:27 PM, Dan Williams wrote:
> Allow for the declaration of variables that trigger kvfree() when they
> go out of scope. The check for NULL and call to kvfree() can be elided
> by the compiler in most cases, otherwise without the NULL check an
> unnecessary call to kvfree() may be emitted. Peter proposed a comment
> for this detail [1].
>
> Link: http://lore.kernel.org/r/20230816103102.GF980931@hirez.programming.kicks-ass.net [1]
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Acked-by: Pankaj Gupta <pankaj.gupta@amd.com>
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
Looks good to me.
Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.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);
>
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
` (4 preceding siblings ...)
2023-10-11 5:27 ` [PATCH v5 5/7] mm/slab: Add __free() support for kvfree Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 16:13 ` Dionna Amalie Glaze
2023-10-11 19:24 ` Tom Lendacky
2023-10-11 5:27 ` [PATCH v5 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
2023-10-11 6:44 ` [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Kuppuswamy Sathyanarayanan
7 siblings, 2 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Borislav Petkov, Tom Lendacky, Dionna Glaze, Brijesh Singh,
Jeremi Piotrowski, peterz, sathyanarayanan.kuppuswamy,
dave.hansen, bp
The sevguest driver was a first mover in the confidential computing
space. As a first mover that afforded some leeway to build the driver
without concern for common infrastructure.
Now that sevguest is no longer a singleton [1] the common operation of
building and transmitting attestation report blobs can / should be made
common. In this model the so called "TSM-provider" implementations can
share a common envelope ABI even if the contents of that envelope remain
vendor-specific. When / if the industry agrees on an attestation record
format, that definition can also fit in the same ABI. In the meantime
the kernel's maintenance burden is reduced and collaboration on the
commons is increased.
Convert sevguest to use CONFIG_TSM_REPORTS to retrieve the data that
the SNP_GET_EXT_REPORT ioctl produces. An example flow follows for
retrieving the report blob via the TSM interface utility,
assuming no nonce and VMPL==2:
report=/sys/kernel/config/tsm/report/report0
mkdir $report
echo 2 > $report/privlevel
dd if=/dev/urandom bs=64 count=1 > $report/inblob
hexdump -C $report/outblob
cat $report/certs
rmdir $report
Given that the platform implementation is free to return empty
certificate data if none is available it lets configfs-tsm be simplified
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>
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 | 139 +++++++++++++++++++++++++++++++
2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
+ int certs_size = 0, cert_count, i, offset;
+ struct tsm_desc *desc = &report->desc;
+ struct snp_guest_dev *snp_dev = data;
+ struct snp_msg_report_resp_hdr hdr;
+ const int report_size = SZ_4K;
+ const int ext_size = SZ_16K;
+ int ret, size = report_size + ext_size;
+ u8 *certs_address;
+
+ if (desc->inblob_len != 64)
+ return -EINVAL;
+
+ void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ guard(mutex)(&snp_cmd_mutex);
+
+ /* Check if the VMPCK is not empty */
+ if (is_vmpck_empty(snp_dev)) {
+ dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
+ mutex_unlock(&snp_cmd_mutex);
+ return -ENOTTY;
+ }
+
+ certs_address = buf + report_size;
+ struct snp_ext_report_req ext_req = {
+ .data = { .vmpl = desc->privlevel },
+ .certs_address = (__u64)certs_address,
+ .certs_len = ext_size,
+ };
+ memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
+
+ struct snp_guest_request_ioctl input = {
+ .msg_version = 1,
+ .req_data = (__u64)&ext_req,
+ .resp_data = (__u64)buf,
+ .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;
+
+ for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
+ struct snp_msg_cert_entry *certs = buf + report_size;
+
+ if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
+ break;
+ certs_size += certs[i].length;
+ }
+ cert_count = i;
+
+ /* No certs to report */
+ if (cert_count == 0)
+ return 0;
+
+ /* sanity check that the entire certs table with metadata fits */
+ if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
+ return -ENXIO;
+
+ void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
+ if (!cbuf)
+ return -ENOMEM;
+
+ /* Concatenate returned certs */
+ for (i = 0, offset = 0; i < cert_count; i++) {
+ struct snp_msg_cert_entry *certs = buf + report_size;
+
+ memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
+ offset += certs[i].length;
+ }
+
+ report->certs = no_free_ptr(cbuf);
+ report->certs_len = certs_size;
+
+ return 0;
+}
+
+static const struct tsm_ops sev_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .report_new = sev_report_new,
+};
+
+static void unregister_sev_tsm(void *data)
+{
+ tsm_unregister(&sev_tsm_ops);
+}
+
static int __init sev_guest_probe(struct platform_device *pdev)
{
struct snp_secrets_page_layout *layout;
@@ -841,6 +972,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
snp_dev->input.resp_gpa = __pa(snp_dev->response);
snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
+ ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
+ if (ret)
+ goto e_free_cert_data;
+
+ ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
+ if (ret)
+ goto e_free_cert_data;
+
ret = misc_register(misc);
if (ret)
goto e_free_cert_data;
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 5:27 ` [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
@ 2023-10-11 16:13 ` Dionna Amalie Glaze
2023-10-11 20:41 ` Dan Williams
2023-10-11 19:24 ` Tom Lendacky
1 sibling, 1 reply; 19+ messages in thread
From: Dionna Amalie Glaze @ 2023-10-11 16:13 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Borislav Petkov, Tom Lendacky, Brijesh Singh,
Jeremi Piotrowski, peterz, sathyanarayanan.kuppuswamy,
dave.hansen
On Tue, Oct 10, 2023 at 10:27 PM Dan Williams <dan.j.williams@intel.com> 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
> cat $report/certs
> rmdir $report
>
> Given that the platform implementation is free to return empty
> certificate data if none is available it lets configfs-tsm be simplified
> 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>
> 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 | 139 +++++++++++++++++++++++++++++++
> 2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
> + int certs_size = 0, cert_count, i, offset;
> + struct tsm_desc *desc = &report->desc;
> + struct snp_guest_dev *snp_dev = data;
> + struct snp_msg_report_resp_hdr hdr;
> + const int report_size = SZ_4K;
> + const int ext_size = SZ_16K;
> + int ret, size = report_size + ext_size;
> + u8 *certs_address;
> +
> + if (desc->inblob_len != 64)
> + return -EINVAL;
> +
> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + guard(mutex)(&snp_cmd_mutex);
> +
> + /* Check if the VMPCK is not empty */
> + if (is_vmpck_empty(snp_dev)) {
> + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> + mutex_unlock(&snp_cmd_mutex);
> + return -ENOTTY;
> + }
> +
> + certs_address = buf + report_size;
> + struct snp_ext_report_req ext_req = {
> + .data = { .vmpl = desc->privlevel },
> + .certs_address = (__u64)certs_address,
> + .certs_len = ext_size,
> + };
> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> +
> + struct snp_guest_request_ioctl input = {
> + .msg_version = 1,
> + .req_data = (__u64)&ext_req,
> + .resp_data = (__u64)buf,
> + .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;
> +
> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
> + break;
> + certs_size += certs[i].length;
> + }
> + cert_count = i;
> +
> + /* No certs to report */
> + if (cert_count == 0)
> + return 0;
> +
> + /* sanity check that the entire certs table with metadata fits */
> + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
> + return -ENXIO;
> +
> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> + if (!cbuf)
> + return -ENOMEM;
> +
> + /* Concatenate returned certs */
> + for (i = 0, offset = 0; i < cert_count; i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> + offset += certs[i].length;
> + }
This concatenation isn't going to work since you lose the identity of
the certificates. The GUIDs of the certificate table matter, and the
concatenation isn't necessarily easily parsed out.
Consider our use case of providing not just the ARK, ASK, and VCEK
certificates, but also a non-x.509 document that is a signed golden
measurement of the firmware. We may also want to provide the VLEK so
the endorsement key selection attribute of the MSG_REPORT_REQ can be
utilized to swap between VCEK and VLEK. I don't believe your patch
here allows for that selection either.
By the GHCB specification, the host is allowed to document their own
GUIDs and provide their data to the guest in this data blob.
I think probably what's better is for the configfs to just create
GUID-named files with the contents of the entries, but I don't think
just inblob of length 64 is going to handle the VLEK/VCEK selection.
> +
> + report->certs = no_free_ptr(cbuf);
> + report->certs_len = certs_size;
> +
> + return 0;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> + .name = KBUILD_MODNAME,
> + .report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> + tsm_unregister(&sev_tsm_ops);
> +}
> +
> static int __init sev_guest_probe(struct platform_device *pdev)
> {
> struct snp_secrets_page_layout *layout;
> @@ -841,6 +972,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;
>
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 16:13 ` Dionna Amalie Glaze
@ 2023-10-11 20:41 ` Dan Williams
2023-10-11 21:06 ` Dionna Amalie Glaze
0 siblings, 1 reply; 19+ messages in thread
From: Dan Williams @ 2023-10-11 20:41 UTC (permalink / raw)
To: Dionna Amalie Glaze, Dan Williams
Cc: linux-coco, Borislav Petkov, Tom Lendacky, Brijesh Singh,
Jeremi Piotrowski, peterz, sathyanarayanan.kuppuswamy,
dave.hansen
Dionna Amalie Glaze wrote:
[..]
> > + /* Concatenate returned certs */
> > + for (i = 0, offset = 0; i < cert_count; i++) {
> > + struct snp_msg_cert_entry *certs = buf + report_size;
> > +
> > + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> > + offset += certs[i].length;
> > + }
>
> This concatenation isn't going to work
I will note that v4 had this as well, so the timing here is not great,
but the feedback is still important because it affects an ABI decision
and "ABI is hard".
> since you lose the identity of
> the certificates. The GUIDs of the certificate table matter, and the
> concatenation isn't necessarily easily parsed out.
I notice "isn't necessarily easily parsed out", leaves some wiggle room.
The rationale for the concatenate proposal came from the observation
that conveying certificates is a common capability between SNP reports
and TDX quotes. In the TDX quote the certificate payload is a:
"Concatenated PCK Cert Chain (PEM formatted)"
...so the thought was unify on that common denominator of "concatenated
certificates". Is that an oversimplification?
> Consider our use case of providing not just the ARK, ASK, and VCEK
> certificates, but also a non-x.509 document that is a signed golden
> measurement of the firmware. We may also want to provide the VLEK so
> the endorsement key selection attribute of the MSG_REPORT_REQ can be
> utilized to swap between VCEK and VLEK. I don't believe your patch
> here allows for that selection either.
To date the kernel's snp_report_req definition is:
struct snp_report_req {
/* user data that should be included in the report */
__u8 user_data[64];
/* The vmpl level to be included in the report */
__u32 vmpl;
/* Must be zero filled */
__u8 rsvd[28];
};
...so the design was only considering what was called out in the existing
ioctl ABI.
When would the key-select field of MSG_REPORT_REQ be non-zero, and would
that be a per call decision, or is that selection a fleet wide policy?
> By the GHCB specification, the host is allowed to document their own
> GUIDs and provide their data to the guest in this data blob.
What does the caller do with these certificates, or are they only
conveyed to the verifier?
> I think probably what's better is for the configfs to just create
> GUID-named files with the contents of the entries, but I don't think
> just inblob of length 64 is going to handle the VLEK/VCEK selection.
That's a possibility. Would the kernel need to invent GUID to represent
the TDX quote PCK cert chain, would the kernel need to un-concatenate
that blob into separate files?
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 20:41 ` Dan Williams
@ 2023-10-11 21:06 ` Dionna Amalie Glaze
0 siblings, 0 replies; 19+ messages in thread
From: Dionna Amalie Glaze @ 2023-10-11 21:06 UTC (permalink / raw)
To: Dan Williams
Cc: linux-coco, Borislav Petkov, Tom Lendacky, Brijesh Singh,
Jeremi Piotrowski, peterz, sathyanarayanan.kuppuswamy,
dave.hansen
On Wed, Oct 11, 2023 at 1:42 PM Dan Williams <dan.j.williams@intel.com> wrote:
>
> Dionna Amalie Glaze wrote:
> [..]
> > > + /* Concatenate returned certs */
> > > + for (i = 0, offset = 0; i < cert_count; i++) {
> > > + struct snp_msg_cert_entry *certs = buf + report_size;
> > > +
> > > + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> > > + offset += certs[i].length;
> > > + }
> >
> > This concatenation isn't going to work
>
> I will note that v4 had this as well, so the timing here is not great,
> but the feedback is still important because it affects an ABI decision
> and "ABI is hard".
Yes, thanks for taking the feedback.
>
> > since you lose the identity of
> > the certificates. The GUIDs of the certificate table matter, and the
> > concatenation isn't necessarily easily parsed out.
>
> I notice "isn't necessarily easily parsed out", leaves some wiggle room.
>
Yes, DER certificates can be concatenated and parsed without
additional metadata,
but I'm less sure about other formats. We could be looking at COSE or
binary-serialized
protocol buffers. It's an entirely avoidable complexity.
If you want to keep PEM, then you'd need to interpret the GUID table
and do the base64
encoding yourself
> The rationale for the concatenate proposal came from the observation
> that conveying certificates is a common capability between SNP reports
> and TDX quotes. In the TDX quote the certificate payload is a:
>
> "Concatenated PCK Cert Chain (PEM formatted)"
>
> ...so the thought was unify on that common denominator of "concatenated
> certificates". Is that an oversimplification?
>
It is. We'll need to get the quoting enclave to additionally provide
an integrity manifest of the firmware.
The TDX team hasn't implemented this functionality yet. Intel told us
this is the pathway we need to take.
> > Consider our use case of providing not just the ARK, ASK, and VCEK
> > certificates, but also a non-x.509 document that is a signed golden
> > measurement of the firmware. We may also want to provide the VLEK so
> > the endorsement key selection attribute of the MSG_REPORT_REQ can be
> > utilized to swap between VCEK and VLEK. I don't believe your patch
> > here allows for that selection either.
>
> To date the kernel's snp_report_req definition is:
>
> struct snp_report_req {
> /* user data that should be included in the report */
> __u8 user_data[64];
>
> /* The vmpl level to be included in the report */
> __u32 vmpl;
>
> /* Must be zero filled */
> __u8 rsvd[28];
> };
>
> ...so the design was only considering what was called out in the existing
> ioctl ABI.
>
> When would the key-select field of MSG_REPORT_REQ be non-zero, and would
> that be a per call decision, or is that selection a fleet wide policy?
>
This is more of a customer-wide policy or machine pool policy.
We have some folks that want VCEK endorsement and we have other folks
that want VLEK, and sole ownership vs fungibility for that choice is I
think is not where this ABI decision should come from.
> > By the GHCB specification, the host is allowed to document their own
> > GUIDs and provide their data to the guest in this data blob.
>
> What does the caller do with these certificates, or are they only
> conveyed to the verifier?
>
The caller could do anything it wants. They can be forwarded to a
verification service, yes, but they may also be analyzed locally and
added to a database for manual review provided some criteria are met.
Say we have a user that is skeptical of any new firmware and rejects
any document that the VM is not preconfigured to accept. This is a
kind of verifier, yes, but it's not RATS.
> > I think probably what's better is for the configfs to just create
> > GUID-named files with the contents of the entries, but I don't think
> > just inblob of length 64 is going to handle the VLEK/VCEK selection.
>
> That's a possibility. Would the kernel need to invent GUID to represent
> the TDX quote PCK cert chain, would the kernel need to un-concatenate
> that blob into separate files?
I don't think that Intel plans on utilizing this "certs" outblob since
it includes all collateral with the quote.
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 5:27 ` [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
2023-10-11 16:13 ` Dionna Amalie Glaze
@ 2023-10-11 19:24 ` Tom Lendacky
2023-10-11 21:30 ` Dan Williams
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lendacky @ 2023-10-11 19:24 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, Jeremi Piotrowski,
peterz, sathyanarayanan.kuppuswamy, dave.hansen
On 10/11/23 00:27, 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
> cat $report/certs
> rmdir $report
>
> Given that the platform implementation is free to return empty
> certificate data if none is available it lets configfs-tsm be simplified
> 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>
> 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 | 139 +++++++++++++++++++++++++++++++
> 2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
> + int certs_size = 0, cert_count, i, offset;
> + struct tsm_desc *desc = &report->desc;
> + struct snp_guest_dev *snp_dev = data;
> + struct snp_msg_report_resp_hdr hdr;
> + const int report_size = SZ_4K;
> + const int ext_size = SZ_16K;
> + int ret, size = report_size + ext_size;
> + u8 *certs_address;
> +
> + if (desc->inblob_len != 64)
> + return -EINVAL;
> +
> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> + if (!buf)
> + return -ENOMEM;
> +
> + guard(mutex)(&snp_cmd_mutex);
> +
> + /* Check if the VMPCK is not empty */
> + if (is_vmpck_empty(snp_dev)) {
> + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> + mutex_unlock(&snp_cmd_mutex);
Is this needed given the guard above?
> + return -ENOTTY;
> + }
> +
> + certs_address = buf + report_size;
> + struct snp_ext_report_req ext_req = {
> + .data = { .vmpl = desc->privlevel },
> + .certs_address = (__u64)certs_address,
> + .certs_len = ext_size,
> + };
> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> +
> + struct snp_guest_request_ioctl input = {
> + .msg_version = 1,
> + .req_data = (__u64)&ext_req,
> + .resp_data = (__u64)buf,
> + .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;
> +
> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
> + break;
> + certs_size += certs[i].length;
> + }
> + cert_count = i;
> +
> + /* No certs to report */
> + if (cert_count == 0)
> + return 0;
> +
> + /* sanity check that the entire certs table with metadata fits */
> + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
> + return -ENXIO;
> +
> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> + if (!cbuf)
> + return -ENOMEM;
> +
> + /* Concatenate returned certs */
> + for (i = 0, offset = 0; i < cert_count; i++) {
> + struct snp_msg_cert_entry *certs = buf + report_size;
> +
> + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> + offset += certs[i].length;
> + }
I agree with Dionna here, you must keep the GUID<-->Cert relationship
here. I think you can just copy the full returned cert buffer into the
destination buffer. Then it would look just like what the ioctl() returns
and make it easier for userspace programs to switch to the new mechanism.
Thanks,
Tom
> +
> + report->certs = no_free_ptr(cbuf);
> + report->certs_len = certs_size;
> +
> + return 0;
> +}
> +
> +static const struct tsm_ops sev_tsm_ops = {
> + .name = KBUILD_MODNAME,
> + .report_new = sev_report_new,
> +};
> +
> +static void unregister_sev_tsm(void *data)
> +{
> + tsm_unregister(&sev_tsm_ops);
> +}
> +
> static int __init sev_guest_probe(struct platform_device *pdev)
> {
> struct snp_secrets_page_layout *layout;
> @@ -841,6 +972,14 @@ static int __init sev_guest_probe(struct platform_device *pdev)
> snp_dev->input.resp_gpa = __pa(snp_dev->response);
> snp_dev->input.data_gpa = __pa(snp_dev->certs_data);
>
> + ret = tsm_register(&sev_tsm_ops, snp_dev, &tsm_report_ext_type);
> + if (ret)
> + goto e_free_cert_data;
> +
> + ret = devm_add_action_or_reset(&pdev->dev, unregister_sev_tsm, NULL);
> + if (ret)
> + goto e_free_cert_data;
> +
> ret = misc_register(misc);
> if (ret)
> goto e_free_cert_data;
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 19:24 ` Tom Lendacky
@ 2023-10-11 21:30 ` Dan Williams
2023-10-11 22:21 ` Dionna Amalie Glaze
2023-10-11 22:24 ` Tom Lendacky
0 siblings, 2 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 21:30 UTC (permalink / raw)
To: Tom Lendacky, Dan Williams, linux-coco
Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, Jeremi Piotrowski,
peterz, sathyanarayanan.kuppuswamy, dave.hansen
Tom Lendacky wrote:
> On 10/11/23 00:27, 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
> > cat $report/certs
> > rmdir $report
> >
> > Given that the platform implementation is free to return empty
> > certificate data if none is available it lets configfs-tsm be simplified
> > 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>
> > 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 | 139 +++++++++++++++++++++++++++++++
> > 2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
> > + int certs_size = 0, cert_count, i, offset;
> > + struct tsm_desc *desc = &report->desc;
> > + struct snp_guest_dev *snp_dev = data;
> > + struct snp_msg_report_resp_hdr hdr;
> > + const int report_size = SZ_4K;
> > + const int ext_size = SZ_16K;
> > + int ret, size = report_size + ext_size;
> > + u8 *certs_address;
> > +
> > + if (desc->inblob_len != 64)
> > + return -EINVAL;
> > +
> > + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + guard(mutex)(&snp_cmd_mutex);
> > +
> > + /* Check if the VMPCK is not empty */
> > + if (is_vmpck_empty(snp_dev)) {
> > + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> > + mutex_unlock(&snp_cmd_mutex);
>
> Is this needed given the guard above?
>
> > + return -ENOTTY;
> > + }
> > +
> > + certs_address = buf + report_size;
> > + struct snp_ext_report_req ext_req = {
> > + .data = { .vmpl = desc->privlevel },
> > + .certs_address = (__u64)certs_address,
> > + .certs_len = ext_size,
> > + };
> > + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> > +
> > + struct snp_guest_request_ioctl input = {
> > + .msg_version = 1,
> > + .req_data = (__u64)&ext_req,
> > + .resp_data = (__u64)buf,
> > + .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;
> > +
> > + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> > + struct snp_msg_cert_entry *certs = buf + report_size;
> > +
> > + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
> > + break;
> > + certs_size += certs[i].length;
> > + }
> > + cert_count = i;
> > +
> > + /* No certs to report */
> > + if (cert_count == 0)
> > + return 0;
> > +
> > + /* sanity check that the entire certs table with metadata fits */
> > + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
> > + return -ENXIO;
> > +
> > + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> > + if (!cbuf)
> > + return -ENOMEM;
> > +
> > + /* Concatenate returned certs */
> > + for (i = 0, offset = 0; i < cert_count; i++) {
> > + struct snp_msg_cert_entry *certs = buf + report_size;
> > +
> > + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> > + offset += certs[i].length;
> > + }
>
> I agree with Dionna here, you must keep the GUID<-->Cert relationship
> here. I think you can just copy the full returned cert buffer into the
> destination buffer. Then it would look just like what the ioctl() returns
> and make it easier for userspace programs to switch to the new mechanism.
This reverses the feedback from Jeremi where he asked for a separate
"certs" file.
Hmm, perhaps this should be an optional @auxblob attribute where a
backend can publish supplemental data to the report. The issue from a
common ABI perspective is that the SNP report format is independent of
the conveyed certificates and the TDX quote format includes a reference
to the "certs" from the "reportblob". In the SNP case that certs
reference is conveyed in the ioctl envelope which does not exist in the
configfs-tsm case.
So the proposal is @auxblob is documented as supplemental data to the
report, and then when @provider indicates "sev-guest" the format of
@auxblob is defined by 'struct cert_table' in GHCB 4.1.8.1
MSG_REPORT_REQ.
In the @provider == "tdx-guest" case the @auxblob attribute is hidden,
or empty.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 21:30 ` Dan Williams
@ 2023-10-11 22:21 ` Dionna Amalie Glaze
2023-10-11 22:24 ` Tom Lendacky
1 sibling, 0 replies; 19+ messages in thread
From: Dionna Amalie Glaze @ 2023-10-11 22:21 UTC (permalink / raw)
To: Dan Williams
Cc: Tom Lendacky, linux-coco, Borislav Petkov, Brijesh Singh,
Jeremi Piotrowski, peterz, sathyanarayanan.kuppuswamy,
dave.hansen, Erdem Aktas
> >
> > I agree with Dionna here, you must keep the GUID<-->Cert relationship
> > here. I think you can just copy the full returned cert buffer into the
> > destination buffer. Then it would look just like what the ioctl() returns
> > and make it easier for userspace programs to switch to the new mechanism.
>
> This reverses the feedback from Jeremi where he asked for a separate
> "certs" file.
>
> Hmm, perhaps this should be an optional @auxblob attribute where a
> backend can publish supplemental data to the report. The issue from a
> common ABI perspective is that the SNP report format is independent of
> the conveyed certificates and the TDX quote format includes a reference
> to the "certs" from the "reportblob". In the SNP case that certs
> reference is conveyed in the ioctl envelope which does not exist in the
> configfs-tsm case.
>
> So the proposal is @auxblob is documented as supplemental data to the
> report, and then when @provider indicates "sev-guest" the format of
> @auxblob is defined by 'struct cert_table' in GHCB 4.1.8.1
> MSG_REPORT_REQ.
>
> In the @provider == "tdx-guest" case the @auxblob attribute is hidden,
> or empty.
auxblob might need to get added anyways, because the quoting enclave's
embedding of the PCK certificate isn't through PEM but a decomposition
into big endian integer pairs for the ECDSA curve.
This makes extension for a platform maintainer to provide cached RIMs
for things like trusted I/O devices and vendored firmware much more
difficult.
I would need Intel to comment more directly on
https://github.com/intel/SGXDataCenterAttestationPrimitives/issues/336
--
-Dionna Glaze, PhD (she/her)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 21:30 ` Dan Williams
2023-10-11 22:21 ` Dionna Amalie Glaze
@ 2023-10-11 22:24 ` Tom Lendacky
2023-10-12 0:38 ` Dan Williams
1 sibling, 1 reply; 19+ messages in thread
From: Tom Lendacky @ 2023-10-11 22:24 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, Jeremi Piotrowski,
peterz, sathyanarayanan.kuppuswamy, dave.hansen
On 10/11/23 16:30, Dan Williams wrote:
> Tom Lendacky wrote:
>> On 10/11/23 00:27, 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
>>> cat $report/certs
>>> rmdir $report
>>>
>>> Given that the platform implementation is free to return empty
>>> certificate data if none is available it lets configfs-tsm be simplified
>>> 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>
>>> 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 | 139 +++++++++++++++++++++++++++++++
>>> 2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
>>> + int certs_size = 0, cert_count, i, offset;
>>> + struct tsm_desc *desc = &report->desc;
>>> + struct snp_guest_dev *snp_dev = data;
>>> + struct snp_msg_report_resp_hdr hdr;
>>> + const int report_size = SZ_4K;
>>> + const int ext_size = SZ_16K;
>>> + int ret, size = report_size + ext_size;
>>> + u8 *certs_address;
>>> +
>>> + if (desc->inblob_len != 64)
>>> + return -EINVAL;
>>> +
>>> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
>>> + if (!buf)
>>> + return -ENOMEM;
>>> +
>>> + guard(mutex)(&snp_cmd_mutex);
>>> +
>>> + /* Check if the VMPCK is not empty */
>>> + if (is_vmpck_empty(snp_dev)) {
>>> + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
>>> + mutex_unlock(&snp_cmd_mutex);
>>
>> Is this needed given the guard above?
>>
>>> + return -ENOTTY;
>>> + }
>>> +
>>> + certs_address = buf + report_size;
>>> + struct snp_ext_report_req ext_req = {
>>> + .data = { .vmpl = desc->privlevel },
>>> + .certs_address = (__u64)certs_address,
>>> + .certs_len = ext_size,
>>> + };
>>> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
>>> +
>>> + struct snp_guest_request_ioctl input = {
>>> + .msg_version = 1,
>>> + .req_data = (__u64)&ext_req,
>>> + .resp_data = (__u64)buf,
>>> + .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;
>>> +
>>> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
>>> + struct snp_msg_cert_entry *certs = buf + report_size;
>>> +
>>> + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
>>> + break;
>>> + certs_size += certs[i].length;
>>> + }
>>> + cert_count = i;
>>> +
>>> + /* No certs to report */
>>> + if (cert_count == 0)
>>> + return 0;
>>> +
>>> + /* sanity check that the entire certs table with metadata fits */
>>> + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
>>> + return -ENXIO;
>>> +
>>> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
>>> + if (!cbuf)
>>> + return -ENOMEM;
>>> +
>>> + /* Concatenate returned certs */
>>> + for (i = 0, offset = 0; i < cert_count; i++) {
>>> + struct snp_msg_cert_entry *certs = buf + report_size;
>>> +
>>> + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
>>> + offset += certs[i].length;
>>> + }
>>
>> I agree with Dionna here, you must keep the GUID<-->Cert relationship
>> here. I think you can just copy the full returned cert buffer into the
>> destination buffer. Then it would look just like what the ioctl() returns
>> and make it easier for userspace programs to switch to the new mechanism.
>
> This reverses the feedback from Jeremi where he asked for a separate
> "certs" file.
I'm not sure I follow, you would still have a separate certs file. It
would contain the GUID table followed by the certs data and would be
separate from the attestation report. You would be replacing the
"Concatenate returned certs" loop with a straight memcpy. If you want to
truncate the size to the actual data size, then you could go to the last
entry in the GUID table and use the offset and length to arrive at the
final size needed to be copied.
The current ioctl() has two requests, GET_REPORT and GET_EXT_REPORT. In
the case of the latter, a separate buffer is used to hold the returned
certs data (see struct snp_ext_report_req) and this would allow the data
from the certs file to be treated exactly the same as if returned via the
ioctl().
>
> Hmm, perhaps this should be an optional @auxblob attribute where a
> backend can publish supplemental data to the report. The issue from a
> common ABI perspective is that the SNP report format is independent of
> the conveyed certificates and the TDX quote format includes a reference
> to the "certs" from the "reportblob". In the SNP case that certs
> reference is conveyed in the ioctl envelope which does not exist in the
> configfs-tsm case.
>
> So the proposal is @auxblob is documented as supplemental data to the
> report, and then when @provider indicates "sev-guest" the format of
> @auxblob is defined by 'struct cert_table' in GHCB 4.1.8.1
> MSG_REPORT_REQ.
This will work, too. It allows the userspace to read and parse the data
just as if it had been returned via the ioctl(). And calling it auxblob
gives you the freedom to indicate that it is provider defined vs. certs
which might imply a certain format.
Thanks,
Tom
>
> In the @provider == "tdx-guest" case the @auxblob attribute is hidden,
> or empty.
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT
2023-10-11 22:24 ` Tom Lendacky
@ 2023-10-12 0:38 ` Dan Williams
0 siblings, 0 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-12 0:38 UTC (permalink / raw)
To: Tom Lendacky, Dan Williams, linux-coco
Cc: Borislav Petkov, Dionna Glaze, Brijesh Singh, Jeremi Piotrowski,
peterz, sathyanarayanan.kuppuswamy, dave.hansen
Tom Lendacky wrote:
> On 10/11/23 16:30, Dan Williams wrote:
> > Tom Lendacky wrote:
> >> On 10/11/23 00:27, 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
> >>> cat $report/certs
> >>> rmdir $report
> >>>
> >>> Given that the platform implementation is free to return empty
> >>> certificate data if none is available it lets configfs-tsm be simplified
> >>> 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>
> >>> 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 | 139 +++++++++++++++++++++++++++++++
> >>> 2 files changed, 140 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..7fdc5a774eab 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,135 @@ 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 };
> >>> + int certs_size = 0, cert_count, i, offset;
> >>> + struct tsm_desc *desc = &report->desc;
> >>> + struct snp_guest_dev *snp_dev = data;
> >>> + struct snp_msg_report_resp_hdr hdr;
> >>> + const int report_size = SZ_4K;
> >>> + const int ext_size = SZ_16K;
> >>> + int ret, size = report_size + ext_size;
> >>> + u8 *certs_address;
> >>> +
> >>> + if (desc->inblob_len != 64)
> >>> + return -EINVAL;
> >>> +
> >>> + void *buf __free(kvfree) = kvzalloc(size, GFP_KERNEL);
> >>> + if (!buf)
> >>> + return -ENOMEM;
> >>> +
> >>> + guard(mutex)(&snp_cmd_mutex);
> >>> +
> >>> + /* Check if the VMPCK is not empty */
> >>> + if (is_vmpck_empty(snp_dev)) {
> >>> + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
> >>> + mutex_unlock(&snp_cmd_mutex);
> >>
> >> Is this needed given the guard above?
> >>
> >>> + return -ENOTTY;
> >>> + }
> >>> +
> >>> + certs_address = buf + report_size;
> >>> + struct snp_ext_report_req ext_req = {
> >>> + .data = { .vmpl = desc->privlevel },
> >>> + .certs_address = (__u64)certs_address,
> >>> + .certs_len = ext_size,
> >>> + };
> >>> + memcpy(&ext_req.data.user_data, desc->inblob, desc->inblob_len);
> >>> +
> >>> + struct snp_guest_request_ioctl input = {
> >>> + .msg_version = 1,
> >>> + .req_data = (__u64)&ext_req,
> >>> + .resp_data = (__u64)buf,
> >>> + .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;
> >>> +
> >>> + for (i = 0; i < ext_size / sizeof(struct snp_msg_cert_entry); i++) {
> >>> + struct snp_msg_cert_entry *certs = buf + report_size;
> >>> +
> >>> + if (memcmp(&certs[i], &zero_ent, sizeof(zero_ent)) == 0)
> >>> + break;
> >>> + certs_size += certs[i].length;
> >>> + }
> >>> + cert_count = i;
> >>> +
> >>> + /* No certs to report */
> >>> + if (cert_count == 0)
> >>> + return 0;
> >>> +
> >>> + /* sanity check that the entire certs table with metadata fits */
> >>> + if ((cert_count + 1) * sizeof(zero_ent) + certs_size > ext_size)
> >>> + return -ENXIO;
> >>> +
> >>> + void *cbuf __free(kvfree) = kvzalloc(certs_size, GFP_KERNEL);
> >>> + if (!cbuf)
> >>> + return -ENOMEM;
> >>> +
> >>> + /* Concatenate returned certs */
> >>> + for (i = 0, offset = 0; i < cert_count; i++) {
> >>> + struct snp_msg_cert_entry *certs = buf + report_size;
> >>> +
> >>> + memcpy(cbuf + offset, certs_address + certs[i].offset, certs[i].length);
> >>> + offset += certs[i].length;
> >>> + }
> >>
> >> I agree with Dionna here, you must keep the GUID<-->Cert relationship
> >> here. I think you can just copy the full returned cert buffer into the
> >> destination buffer. Then it would look just like what the ioctl() returns
> >> and make it easier for userspace programs to switch to the new mechanism.
> >
> > This reverses the feedback from Jeremi where he asked for a separate
> > "certs" file.
>
> I'm not sure I follow, you would still have a separate certs file. It
> would contain the GUID table followed by the certs data and would be
> separate from the attestation report. You would be replacing the
> "Concatenate returned certs" loop with a straight memcpy. If you want to
> truncate the size to the actual data size, then you could go to the last
> entry in the GUID table and use the offset and length to arrive at the
> final size needed to be copied.
The issue is common ABI across vendors. TDX Quote does not have the
same "cert_table" scheme to convey this data. So that's why I arrived at
@auxblob to convey this "on the side" + "vendor format" data. I was
hoping that "concatenated PEM" was a point of commonality, but as Dionna
is pointing out these things are not quite the same.
> The current ioctl() has two requests, GET_REPORT and GET_EXT_REPORT. In
> the case of the latter, a separate buffer is used to hold the returned
> certs data (see struct snp_ext_report_req) and this would allow the data
> from the certs file to be treated exactly the same as if returned via the
> ioctl().
Right, that's what I would route to @auxblob.
> > Hmm, perhaps this should be an optional @auxblob attribute where a
> > backend can publish supplemental data to the report. The issue from a
> > common ABI perspective is that the SNP report format is independent of
> > the conveyed certificates and the TDX quote format includes a reference
> > to the "certs" from the "reportblob". In the SNP case that certs
> > reference is conveyed in the ioctl envelope which does not exist in the
> > configfs-tsm case.
> >
> > So the proposal is @auxblob is documented as supplemental data to the
> > report, and then when @provider indicates "sev-guest" the format of
> > @auxblob is defined by 'struct cert_table' in GHCB 4.1.8.1
> > MSG_REPORT_REQ.
>
> This will work, too. It allows the userspace to read and parse the data
> just as if it had been returned via the ioctl(). And calling it auxblob
> gives you the freedom to indicate that it is provider defined vs. certs
> which might imply a certain format.
Ok, sounds good.
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v5 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
` (5 preceding siblings ...)
2023-10-11 5:27 ` [PATCH v5 6/7] virt: sevguest: Add TSM_REPORTS support for SNP_GET_EXT_REPORT Dan Williams
@ 2023-10-11 5:27 ` Dan Williams
2023-10-11 6:44 ` [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Kuppuswamy Sathyanarayanan
7 siblings, 0 replies; 19+ messages in thread
From: Dan Williams @ 2023-10-11 5:27 UTC (permalink / raw)
To: linux-coco
Cc: Kuppuswamy Sathyanarayanan, Erdem Aktas, peterz,
sathyanarayanan.kuppuswamy, dave.hansen, bp
From: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
In TDX guest, the attestation process is used to verify the TDX guest
trustworthiness to other entities before provisioning secrets to the
guest. The first step in the attestation process is TDREPORT
generation, which involves getting the guest measurement data in the
format of TDREPORT, which is further used to validate the authenticity
of the TDX guest. TDREPORT by design is integrity-protected and can
only be verified on the local machine.
To support remote verification of the TDREPORT in a SGX-based
attestation, the TDREPORT needs to be sent to the SGX Quoting Enclave
(QE) to convert it to a remotely verifiable Quote. SGX QE by design can
only run outside of the TDX guest (i.e. in a host process or in a
normal VM) and guest can use communication channels like vsock or
TCP/IP to send the TDREPORT to the QE. But for security concerns, the
TDX guest may not support these communication channels. To handle such
cases, TDX defines a GetQuote hypercall which can be used by the guest
to request the host VMM to communicate with the SGX QE. More details
about GetQuote hypercall can be found in TDX Guest-Host Communication
Interface (GHCI) for Intel TDX 1.0, section titled
"TDG.VP.VMCALL<GetQuote>".
Trusted Security Module (TSM) [1] exposes a common ABI for Confidential
Computing Guest platforms to get the measurement data via ConfigFS.
Extend the TSM framework and add support to allow an attestation agent
to get the TDX Quote data (included usage example below).
report=/sys/kernel/config/tsm/report/report0
mkdir $report
dd if=/dev/urandom bs=64 count=1 > $report/inblob
hexdump -C $report/outblob
rmdir $report
GetQuote TDVMCALL requires TD guest pass a 4K aligned shared buffer
with TDREPORT data as input, which is further used by the VMM to copy
the TD Quote result after successful Quote generation. To create the
shared buffer, allocate a large enough memory and mark it shared using
set_memory_decrypted() in tdx_guest_init(). This buffer will be re-used
for GetQuote requests in the TDX TSM handler.
Although this method reserves a fixed chunk of memory for GetQuote
requests, such one time allocation can help avoid memory fragmentation
related allocation failures later in the uptime of the guest.
Since the Quote generation process is not time-critical or frequently
used, the current version uses a polling model for Quote requests and
it also does not support parallel GetQuote requests.
Link: https://lore.kernel.org/lkml/169342399185.3934343.3035845348326944519.stgit@dwillia2-xfh.jf.intel.com/ [1]
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
Reviewed-by: Erdem Aktas <erdemaktas@google.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
arch/x86/coco/tdx/tdx.c | 21 +++
arch/x86/include/asm/shared/tdx.h | 1
arch/x86/include/asm/tdx.h | 2
drivers/virt/coco/tdx-guest/Kconfig | 1
drivers/virt/coco/tdx-guest/tdx-guest.c | 229 +++++++++++++++++++++++++++++++
5 files changed, 253 insertions(+), 1 deletion(-)
diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 1d6b863c42b0..752867b1d11b 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -104,6 +104,27 @@ int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport)
}
EXPORT_SYMBOL_GPL(tdx_mcall_get_report0);
+/**
+ * tdx_hcall_get_quote() - Wrapper to request TD Quote using GetQuote
+ * hypercall.
+ * @buf: Address of the directly mapped shared kernel buffer which
+ * contains TDREPORT. The same buffer will be used by VMM to
+ * store the generated TD Quote output.
+ * @size: size of the tdquote buffer (4KB-aligned).
+ *
+ * Refer to section titled "TDG.VP.VMCALL<GetQuote>" in the TDX GHCI
+ * v1.0 specification for more information on GetQuote hypercall.
+ * It is used in the TDX guest driver module to get the TD Quote.
+ *
+ * Return 0 on success or error code on failure.
+ */
+u64 tdx_hcall_get_quote(u8 *buf, size_t size)
+{
+ /* Since buf is a shared memory, set the shared (decrypted) bits */
+ return _tdx_hypercall(TDVMCALL_GET_QUOTE, cc_mkdec(virt_to_phys(buf)), size, 0, 0);
+}
+EXPORT_SYMBOL_GPL(tdx_hcall_get_quote);
+
static void __noreturn tdx_panic(const char *msg)
{
struct tdx_hypercall_args args = {
diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
index 7513b3bb69b7..9eab19950f39 100644
--- a/arch/x86/include/asm/shared/tdx.h
+++ b/arch/x86/include/asm/shared/tdx.h
@@ -22,6 +22,7 @@
/* TDX hypercall Leaf IDs */
#define TDVMCALL_MAP_GPA 0x10001
+#define TDVMCALL_GET_QUOTE 0x10002
#define TDVMCALL_REPORT_FATAL_ERROR 0x10003
#ifndef __ASSEMBLY__
diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
index 603e6d1e9d4a..ebd1cda4875f 100644
--- a/arch/x86/include/asm/tdx.h
+++ b/arch/x86/include/asm/tdx.h
@@ -52,6 +52,8 @@ bool tdx_early_handle_ve(struct pt_regs *regs);
int tdx_mcall_get_report0(u8 *reportdata, u8 *tdreport);
+u64 tdx_hcall_get_quote(u8 *buf, size_t size);
+
#else
static inline void tdx_early_init(void) { };
diff --git a/drivers/virt/coco/tdx-guest/Kconfig b/drivers/virt/coco/tdx-guest/Kconfig
index 14246fc2fb02..22dd59e19431 100644
--- a/drivers/virt/coco/tdx-guest/Kconfig
+++ b/drivers/virt/coco/tdx-guest/Kconfig
@@ -1,6 +1,7 @@
config TDX_GUEST_DRIVER
tristate "TDX Guest driver"
depends on INTEL_TDX_GUEST
+ select TSM_REPORTS
help
The driver provides userspace interface to communicate with
the TDX module to request the TDX guest details like attestation
diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 5e44a0fa69bd..1253bf76b570 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -12,12 +12,60 @@
#include <linux/mod_devicetable.h>
#include <linux/string.h>
#include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/io.h>
+#include <linux/delay.h>
+#include <linux/tsm.h>
+#include <linux/sizes.h>
#include <uapi/linux/tdx-guest.h>
#include <asm/cpu_device_id.h>
#include <asm/tdx.h>
+/*
+ * Intel's SGX QE implementation generally uses Quote size less
+ * than 8K (2K Quote data + ~5K of certificate blob).
+ */
+#define GET_QUOTE_BUF_SIZE SZ_8K
+
+#define GET_QUOTE_CMD_VER 1
+
+/* TDX GetQuote status codes */
+#define GET_QUOTE_SUCCESS 0
+#define GET_QUOTE_IN_FLIGHT 0xffffffffffffffff
+
+/* struct tdx_quote_buf: Format of Quote request buffer.
+ * @version: Quote format version, filled by TD.
+ * @status: Status code of Quote request, filled by VMM.
+ * @in_len: Length of TDREPORT, filled by TD.
+ * @out_len: Length of Quote data, filled by VMM.
+ * @data: Quote data on output or TDREPORT on input.
+ *
+ * More details of Quote request buffer can be found in TDX
+ * Guest-Host Communication Interface (GHCI) for Intel TDX 1.0,
+ * section titled "TDG.VP.VMCALL<GetQuote>"
+ */
+struct tdx_quote_buf {
+ u64 version;
+ u64 status;
+ u32 in_len;
+ u32 out_len;
+ u8 data[];
+};
+
+/* Quote data buffer */
+static void *quote_data;
+
+/* Lock to streamline quote requests */
+static DEFINE_MUTEX(quote_lock);
+
+/*
+ * GetQuote request timeout in seconds. Expect that 30 seconds
+ * is enough time for QE to respond to any Quote requests.
+ */
+static u32 getquote_timeout = 30;
+
static long tdx_get_report0(struct tdx_report_req __user *req)
{
u8 *reportdata, *tdreport;
@@ -53,6 +101,154 @@ static long tdx_get_report0(struct tdx_report_req __user *req)
return ret;
}
+static void free_quote_buf(void *buf)
+{
+ size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+ unsigned int count = len >> PAGE_SHIFT;
+
+ if (set_memory_encrypted((unsigned long)buf, count)) {
+ pr_err("Failed to restore encryption mask for Quote buffer, leak it\n");
+ return;
+ }
+
+ free_pages_exact(buf, len);
+}
+
+static void *alloc_quote_buf(void)
+{
+ size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
+ unsigned int count = len >> PAGE_SHIFT;
+ void *addr;
+
+ addr = alloc_pages_exact(len, GFP_KERNEL | __GFP_ZERO);
+ if (!addr)
+ return NULL;
+
+ if (set_memory_decrypted((unsigned long)addr, count)) {
+ free_pages_exact(addr, len);
+ return NULL;
+ }
+
+ return addr;
+}
+
+/*
+ * wait_for_quote_completion() - Wait for Quote request completion
+ * @quote_buf: Address of Quote buffer.
+ * @timeout: Timeout in seconds to wait for the Quote generation.
+ *
+ * As per TDX GHCI v1.0 specification, sec titled "TDG.VP.VMCALL<GetQuote>",
+ * the status field in the Quote buffer will be set to GET_QUOTE_IN_FLIGHT
+ * while VMM processes the GetQuote request, and will change it to success
+ * or error code after processing is complete. So wait till the status
+ * changes from GET_QUOTE_IN_FLIGHT or the request being timed out.
+ */
+static int wait_for_quote_completion(struct tdx_quote_buf *quote_buf, u32 timeout)
+{
+ int i = 0;
+
+ /*
+ * Quote requests usually take a few seconds to complete, so waking up
+ * once per second to recheck the status is fine for this use case.
+ */
+ while (quote_buf->status == GET_QUOTE_IN_FLIGHT && i++ < timeout) {
+ if (msleep_interruptible(MSEC_PER_SEC))
+ return -EINTR;
+ }
+
+ return (i == timeout) ? -ETIMEDOUT : 0;
+}
+
+static int tdx_report_new(struct tsm_report *report, void *data)
+{
+ u8 *buf, *reportdata = NULL, *tdreport = NULL;
+ struct tdx_quote_buf *quote_buf = quote_data;
+ struct tsm_desc *desc = &report->desc;
+ int ret;
+ u64 err;
+
+ /* TODO: switch to guard(mutex_intr) */
+ if (mutex_lock_interruptible("e_lock))
+ return -EINTR;
+
+ /*
+ * If the previous request is timedout or interrupted, and the
+ * Quote buf status is still in GET_QUOTE_IN_FLIGHT (owned by
+ * VMM), don't permit any new request.
+ */
+ if (quote_buf->status == GET_QUOTE_IN_FLIGHT) {
+ ret = -EBUSY;
+ goto done;
+ }
+
+ if (desc->inblob_len != TDX_REPORTDATA_LEN) {
+ ret = -EINVAL;
+ goto done;
+ }
+
+ reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
+ if (!reportdata) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
+ if (!tdreport) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ memcpy(reportdata, desc->inblob, desc->inblob_len);
+
+ /* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
+ ret = tdx_mcall_get_report0(reportdata, tdreport);
+ if (ret) {
+ pr_err("GetReport call failed\n");
+ goto done;
+ }
+
+ memset(quote_data, 0, GET_QUOTE_BUF_SIZE);
+
+ /* Update Quote buffer header */
+ quote_buf->version = GET_QUOTE_CMD_VER;
+ quote_buf->in_len = TDX_REPORT_LEN;
+
+ memcpy(quote_buf->data, tdreport, TDX_REPORT_LEN);
+
+ err = tdx_hcall_get_quote(quote_data, GET_QUOTE_BUF_SIZE);
+ if (err) {
+ pr_err("GetQuote hypercall failed, status:%llx\n", err);
+ ret = -EIO;
+ goto done;
+ }
+
+ ret = wait_for_quote_completion(quote_buf, getquote_timeout);
+ if (ret) {
+ pr_err("GetQuote request timedout\n");
+ goto done;
+ }
+
+ buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
+ if (!buf) {
+ ret = -ENOMEM;
+ goto done;
+ }
+
+ report->outblob = buf;
+ report->outblob_len = quote_buf->out_len;
+
+ /*
+ * TODO: parse the PEM-formatted cert chain out of the quote buffer when
+ * provided
+ */
+done:
+ mutex_unlock("e_lock);
+ kfree(reportdata);
+ kfree(tdreport);
+
+ return ret;
+}
+
static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -82,17 +278,48 @@ static const struct x86_cpu_id tdx_guest_ids[] = {
};
MODULE_DEVICE_TABLE(x86cpu, tdx_guest_ids);
+static const struct tsm_ops tdx_tsm_ops = {
+ .name = KBUILD_MODNAME,
+ .report_new = tdx_report_new,
+};
+
static int __init tdx_guest_init(void)
{
+ int ret;
+
if (!x86_match_cpu(tdx_guest_ids))
return -ENODEV;
- return misc_register(&tdx_misc_dev);
+ ret = misc_register(&tdx_misc_dev);
+ if (ret)
+ return ret;
+
+ quote_data = alloc_quote_buf();
+ if (!quote_data) {
+ pr_err("Failed to allocate Quote buffer\n");
+ ret = -ENOMEM;
+ goto free_misc;
+ }
+
+ ret = tsm_register(&tdx_tsm_ops, NULL, NULL);
+ if (ret)
+ goto free_quote;
+
+ return 0;
+
+free_quote:
+ free_quote_buf(quote_data);
+free_misc:
+ misc_deregister(&tdx_misc_dev);
+
+ return ret;
}
module_init(tdx_guest_init);
static void __exit tdx_guest_exit(void)
{
+ tsm_unregister(&tdx_tsm_ops);
+ free_quote_buf(quote_data);
misc_deregister(&tdx_misc_dev);
}
module_exit(tdx_guest_exit);
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v5 0/7] configfs-tsm: Attestation Report ABI
2023-10-11 5:27 [PATCH v5 0/7] configfs-tsm: Attestation Report ABI Dan Williams
` (6 preceding siblings ...)
2023-10-11 5:27 ` [PATCH v5 7/7] virt: tdx-guest: Add Quote generation support using TSM_REPORTS Dan Williams
@ 2023-10-11 6:44 ` Kuppuswamy Sathyanarayanan
7 siblings, 0 replies; 19+ messages in thread
From: Kuppuswamy Sathyanarayanan @ 2023-10-11 6:44 UTC (permalink / raw)
To: Dan Williams, linux-coco
Cc: Brijesh Singh, Erdem Aktas, Peter Zijlstra, Tom Lendacky,
Peter Gonda, Borislav Petkov, Dionna Amalie Glaze,
Jeremi Piotrowski, Thomas Gleixner, Samuel Ortiz, Pankaj Gupta,
Greg Kroah-Hartman, Andrew Morton, James Bottomley, dave.hansen
On 10/10/2023 10:27 PM, Dan Williams wrote:
> Changes since v4 [1]:
> - Fix a stack buffer vs scatterlist bug in sev-guest (Peter)
> - Test on AMD hardware, thanks Peter for the help!
> - Fix size of @len in __read_report() (Sathya)
> - Clarify the NULL @buf case in __read_report() (Sathya)
> - Fix kdoc for 'struct tsm_report' (Sathya)
> - Add kdoc for 'struct tsm_ops' (Sathya)
> - Initialize @certs_size to zero in sev_report_new() (Dan, smatch)
> - Add links to documentation for the attestation report formats
> - Drop conversion of sev-guest get_report(), just use get_ext_report()
> exclusively
> - Add is_vmpck_empty() and exitinfo2 init in set_report_new() similar to
> the ioctl() path
>
> [1]: http://lore.kernel.org/r/169570181657.596431.6178773442587231200.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.
>
> ---
>
> 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
>
> ---
Works fine in TDX environment.
Tested-by: Kuppuswamy Sathyanarayanan <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 | 76 ++++++
> 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 | 218 ++++++++++++++--
> drivers/virt/coco/tdx-guest/Kconfig | 1
> drivers/virt/coco/tdx-guest/tdx-guest.c | 229 +++++++++++++++++
> drivers/virt/coco/tsm.c | 416 +++++++++++++++++++++++++++++++
> include/linux/slab.h | 2
> include/linux/tsm.h | 68 +++++
> 16 files changed, 1039 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
--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer
^ permalink raw reply [flat|nested] 19+ messages in thread