[RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[James Bottomley]

This is a sketch for how a fully enlightened vTPM driver would work. 
The idea is that the SVSM responds on function 8 to vTPM requests, so
we use that to send down a buffer which is modified on return (the
buffer must be big enough, so the agreed protocol is it should be a
page in length, which is larger than any possible TPM command or
response).  The protocol used is the MSSIM one which is self describing
in terms of length, so there's no need to transmit sizes (it also
leaves room for expansion to localities and cancellation, which is
useful in the light of discussions).  A NULL in place of the buffer is
a probe and the SVSM call simply returns SVSM_SUCCESS without doing
anything.  This can be used to probe for vTPM support because any other
return would indicate no vTPM present.

Hopefully IBM will publish the new svsm-vtpm repo shortly, but we're
still working with the old CRB based one at the moment, so it may take
some time.

The three following patches are for two different repos.  Patch 1 will
apply to any upstream Linux kernel, Patch 2 requires the non-upstream
sev-snp repo and patch 3 is against the non upstream sev-snp edk repo.

James

[RFC 1/3] tpm: add generic platform device

[James Bottomley]

From: James Bottomley <James.Bottomley@HansenPartnership.com>

This is primarily designed to support an enlightened driver for the
AMD svsm based vTPM, but it could be used by any platform which
communicates with a TPM device.  The platform must fill in struct
tpm_platform_ops as the platform_data and set the device name to "tpm"
to have the binding by name work correctly.  The sole sendrecv
function is designed to do a single buffer request/response conforming
to the MSSIM protocol.  For the svsm vTPM case, this protocol is
transmitted directly to the SVSM, but it could be massaged for other
function type platform interfaces.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 drivers/char/tpm/Kconfig        |   7 ++
 drivers/char/tpm/Makefile       |   1 +
 drivers/char/tpm/tpm_platform.c | 138 ++++++++++++++++++++++++++++++++
 include/linux/tpm_platform.h    | 107 +++++++++++++++++++++++++
 4 files changed, 253 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_platform.c
 create mode 100644 include/linux/tpm_platform.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 4a5516406c22..c00ecec1d710 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -198,5 +198,12 @@ config TCG_FTPM_TEE
 	help
 	  This driver proxies for firmware TPM running in TEE.
 
+config TCG_PLATFORM
+	tristate "Platform TPM Device"
+	help
+	  This driver requires a platform implementation to provide
the
+	  TPM function.  It will not bind if the implementation is not
+	  present.
+
 source "drivers/char/tpm/st33zp24/Kconfig"
 endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 66d39ea6bd10..d87512a64ffd 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -41,3 +41,4 @@ obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
 obj-$(CONFIG_TCG_CRB) += tpm_crb.o
 obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
 obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
+obj-$(CONFIG_TCG_PLATFORM) += tpm_platform.o
diff --git a/drivers/char/tpm/tpm_platform.c
b/drivers/char/tpm/tpm_platform.c
new file mode 100644
index 000000000000..46d60fd23a0d
--- /dev/null
+++ b/drivers/char/tpm/tpm_platform.c
@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Platform based TPM emulator
+ *
+ * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
+ *
+ * Designed to handle a simple function request/response single buffer
+ * TPM or vTPM rooted in the platform.  This device driver uses the
+ * MSSIM protocol from the Microsoft reference implementation
+ *
+ * https://github.com/microsoft/ms-tpm-20-ref
+ *
+ * to communicate between the driver and the platform.  This is rich
+ * enough to allow platform operations like cancellation The platform
+ * should not act on platform commands like power on/off and reset
+ * which can disrupt the TPM guarantees.
+ *
+ * This driver is designed to be single threaded (one call in to the
+ * platform TPM at any one time).  The threading guarantees are
+ * provided by the chip mutex.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/tpm_platform.h>
+
+#include "tpm.h"
+
+static struct tpm_platform_ops *pops;
+
+static u8 *buffer;
+/*
+ * FIXME: before implementing locality we need to agree what it means
+ * to the platform
+ */
+static u8 locality;
+
+static int tpm_platform_send(struct tpm_chip *chip, u8 *buf, size_t
len)
+{
+	int ret;
+	struct tpm_send_cmd_req *req = (struct tpm_send_cmd_req
*)buffer;
+
+	if (len > TPM_PLATFORM_MAX_BUFFER - sizeof(*req))
+		return -EINVAL;
+	req->cmd = TPM_SEND_COMMAND;
+	req->locality = locality;
+	req->inbuf_size = len;
+	memcpy(req->inbuf, buf, len);
+
+	ret = pops->sendrcv(buffer);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int tpm_platform_recv(struct tpm_chip *chip, u8 *buf, size_t
len)
+{
+	struct tpm_resp *resp = (struct tpm_resp *)buffer;
+
+	if (resp->size < 0)
+		return resp->size;
+
+	if (len < resp->size)
+		return -E2BIG;
+
+	memcpy(buf, buffer + sizeof(*resp), resp->size);
+
+	return resp->size;
+}
+
+static struct tpm_class_ops tpm_chip_ops = {
+	.flags = TPM_OPS_AUTO_STARTUP,
+	.send = tpm_platform_send,
+	.recv = tpm_platform_recv,
+};
+
+static struct platform_driver tpm_platform_driver = {
+	.driver = {
+		.name = "tpm",
+	},
+};
+
+static int __init tpm_platform_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct tpm_chip *chip;
+	int err;
+
+	if (!dev->platform_data)
+		return -ENODEV;
+
+	/*
+	 * in theory platform matching should mean this is always
+	 * true, but just in case anyone tries force binding
+	 */
+	if(strcmp(pdev->name, tpm_platform_driver.driver.name) != 0)
+		return -ENODEV;
+
+	if (!buffer)
+		buffer = kmalloc(TPM_PLATFORM_MAX_BUFFER, GFP_KERNEL);
+
+	if (!buffer)
+		return -ENOMEM;
+
+	pops = dev->platform_data;
+
+	chip = tpmm_chip_alloc(dev, &tpm_chip_ops);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	/*
+	 * Setting TPM_CHIP_FLAG_IRQ guarantees that ->recv will be
+	 * called straight after ->send and means we don't need to
+	 * implement any other chip ops.
+	 */
+	chip->flags |= TPM_CHIP_FLAG_IRQ;
+	err = tpm2_probe(chip);
+	if (err)
+		return err;
+
+	err = tpm_chip_register(chip);
+	if (err)
+		return err;
+
+	dev_info(dev, "TPM %s platform device\n",
+		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2");
+
+	return 0;
+}
+
+module_platform_driver_probe(tpm_platform_driver, tpm_platform_probe);
+
+MODULE_AUTHOR("James Bottomley
<James.Bottomley@HansenPartnership.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Platform TPM Driver");
+MODULE_ALIAS("platform:tpm");
diff --git a/include/linux/tpm_platform.h
b/include/linux/tpm_platform.h
new file mode 100644
index 000000000000..a90089affe0b
--- /dev/null
+++ b/include/linux/tpm_platform.h
@@ -0,0 +1,107 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
+ *
+ * Interface specification for platforms wishing to activate the
+ * platform tpm device.  The device must be a platform device created
+ * with the name "tpm" and it must populate platform_data with struct
+ * tpm_platform_ops
+ */
+
+/*
+ * The current MSSIM TPM commands we support.  The complete list is
+ * in the TcpTpmProtocol header:
+ *
+ *
https://github.com/microsoft/ms-tpm-20-ref/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
+ */
+
+#define TPM_SEND_COMMAND		8
+#define TPM_SIGNAL_CANCEL_ON		9
+#define TPM_SIGNAL_CANCEL_OFF		10
+/*
+ * Any platform specific commands should be placed here and should
start
+ * at 0x8000 to avoid clashes with the MSSIM protocol.  They should
follow
+ * the same self describing buffer format below
+ */
+
+#define TPM_PLATFORM_MAX_BUFFER		4096 /* max req/resp
buffer size */
+
+/**
+ * struct tpm_platform_ops - the share platform operations
+ *
+ * @sendrecv:	Send a TPM command using the MSSIM protocol.
+ *
+ * The MSSIM protocol is designed for a network, so the buffers are
+ * self describing.  The minimum buffer size is sizeof(u32).  Every
+ * MSSIM command defines its own transport buffer and the command is
+ * sent in the first u32 array.  The only modification we make is that
+ * the MSSIM uses network order and we use the endianness of the
+ * architecture.  The response to every command (in the same buffer)
+ * is a u32 size preceded array.  Most of the MSSIM commands simply
+ * return zero here because they have no defined response.
+ *
+ * The only command with a defined request/response size is
TPM_SEND_COMMAND
+ * The definition is in the structures below
+ */
+struct tpm_platform_ops {
+	int (*sendrcv)(u8 *buffer);
+};
+
+/**
+ * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND
+ *
+ * @cmd:	The command (must be TPM_SEND_COMMAND)
+ * @locality:	The locality
+ * @inbuf_size:	The size of the input buffer following
+ * @inbuf:	A buffer of size inbuf_size
+ *
+ * Note that @inbuf_size must be large enough to hold the response so
+ * represents the maximum buffer size, not the size of the specific
+ * TPM command.
+ */
+struct tpm_send_cmd_req {
+	u32	cmd;
+	u8	locality;
+	u32	inbuf_size;
+	u8	inbuf[];
+} __packed;
+
+/**
+ * struct tpm_req - generic request header for single word command
+ *
+ * @cmd:	The command to send
+ */
+struct tpm_req {
+	u32	cmd;
+} __packed;
+
+/**
+ * struct tpm_resp - generic response header
+ *
+ * @size:	The response size (zero if nothing follows)
+ *
+ * Note: most MSSIM commands simply return zero here with no
indication
+ * of success or failure.
+ */
+
+struct tpm_resp {
+	s32	size;
+} __packed;
+
+
+
+
+
+/*
+ *
+ *  uint32_t TPM_SEND_COMMAND, uint8_t Locality, uint32_t
InBufferSize,
+ *    uint8_t[InBufferSize] InBuffer
+ *
+ * Where the InBufferSize must be big enough to hold the response
+ * because it represents the maximum allowable response size.  So if
+ * the first u32 is TPM_SEND_COMMAND, the minimum buffer size is
+ * 9. The response is
+ *
+ * uint32_t OutBufferSize, uint8_t[OutBufferSize] OutBuffer
+ * 
+ */
-- 
2.35.3

[RFC 2/2] x86/sev: add a SVSM vTPM platform device

[James Bottomley]

From: James Bottomley <James.Bottomley@HansenPartnership.com>

If the SNP boot has a SVSM, probe for the vTPM device by sending a
call to SVSM function 8 with no arguments.  If this returns
successfully, the vTPM is present in the SVSM (If the SVSM doesn't
have a vTPM, this call should return SVSM_ERR_UNSUPPORTED_CALLID).

If a vTPM is found, register a platform device as "platform:tpm" so it
can be attached to the tpm_platform.c driver.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 arch/x86/kernel/sev.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index 0297077f7602..276568b1f01a 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -21,6 +21,7 @@
 #include <linux/cpumask.h>
 #include <linux/efi.h>
 #include <linux/platform_device.h>
+#include <linux/tpm_platform.h>
 #include <linux/io.h>
 
 #include <asm/cpu_entry_area.h>
@@ -2426,6 +2427,11 @@ static struct platform_device guest_req_device = {
 	.id		= -1,
 };
 
+static struct platform_device tpm_device = {
+	.name		= "tpm",
+	.id		= -1,
+};
+
 static u64 get_secrets_page(void)
 {
 	u64 pa_data = boot_params.cc_blob_address;
@@ -2450,10 +2456,19 @@ static u64 get_secrets_page(void)
 	return info.secrets_phys;
 }
 
+static int tpm_send_buffer(u8 *buffer)
+{
+	struct svsm_caa *caa;
+
+	caa = this_cpu_read(svsm_caa);
+	return __svsm_msr_protocol(caa, 8, __pa(buffer), 0, 0, 0);
+}
+
 static int __init snp_init_platform_device(void)
 {
 	struct snp_guest_platform_data data;
 	u64 gpa;
+	struct svsm_caa *caa = this_cpu_read(svsm_caa);
 
 	if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP))
 		return -ENODEV;
@@ -2470,6 +2485,24 @@ static int __init snp_init_platform_device(void)
 		return -ENODEV;
 
 	pr_info("SNP guest platform device initialized.\n");
+
+	/* 
+	 * The VTPM device is available only if we have a SVSM and it
+	 * probes correctly (probe is to send a call with no arguments
+	 * to function 8 and see it comes back as OK)
+	 */
+	if (IS_ENABLED(CONFIG_TCG_PLATFORM) && svsm_vmpl &&
+	    __svsm_msr_protocol(caa, 8, 0, 0, 0, 0) == 0) {
+		struct tpm_platform_ops pops = {
+			.sendrcv = tpm_send_buffer,
+		};
+
+		if (platform_device_add_data(&tpm_device, &pops, sizeof(pops)))
+			return -ENODEV;
+		if (platform_device_register(&tpm_device))
+			return -ENODEV;
+		pr_info("SNP SVSM VTPM platform device initialized\n");
+	}
 	return 0;
 }
 device_initcall(snp_init_platform_device);
-- 
2.35.3

[RFC 3/3] edk2: Add SVSM based vTPM

[James Bottomley]

From: James Bottomley <James.Bottomley@HansenPartnership.com>

Add this as a new device in TPM2DeviceLibDTpm.  The SVSM vTPM has no
physical presence interface, so handle detecing this device before
that check.  The detection is done by sending all zeros to the SVSM
for the VTPM function (8) and seeing if success is returned.

Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 .../Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf   |  3 +
 .../Tpm2DeviceLibDTpmStandaloneMm.inf         |  3 +
 .../Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf |  3 +
 SecurityPkg/Include/Library/Tpm2DeviceLib.h   |  1 +
 .../Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.h     | 33 +++++++
 UefiCpuPkg/Include/Library/VmgExitLib.h       |  6 ++
 OvmfPkg/Library/VmgExitLib/VmgExitSvsm.c      | 36 +++++++
 .../Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c       | 14 +++
 .../Library/Tpm2DeviceLibDTpm/Tpm2Svsm.c      | 96 +++++++++++++++++++
 .../Library/VmgExitLibNull/VmgExitLibNull.c   | 23 +++++
 10 files changed, 218 insertions(+)
 create mode 100644 SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Svsm.c

diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
index be3a0053ccce..2cd5216a0d42 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.inf
@@ -33,6 +33,7 @@ [Defines]
 
 [Sources]
   Tpm2Tis.c
+  Tpm2Svsm.c
   Tpm2Ptp.c
   Tpm2DeviceLibDTpm.c
   Tpm2DeviceLibDTpmBase.c
@@ -41,6 +42,7 @@ [Sources]
 [Packages]
   MdePkg/MdePkg.dec
   SecurityPkg/SecurityPkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -49,6 +51,7 @@ [LibraryClasses]
   TimerLib
   DebugLib
   PcdLib
+  VmgExitLib
 
 [Pcd]
   gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress            ## CONSUMES
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpmStandaloneMm.inf b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpmStandaloneMm.inf
index 18c08ad8bdcc..99b140be1c54 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpmStandaloneMm.inf
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpmStandaloneMm.inf
@@ -33,6 +33,7 @@ [Defines]
 
 [Sources]
   Tpm2Tis.c
+  Tpm2Svsm.c
   Tpm2Ptp.c
   Tpm2DeviceLibDTpm.c
   Tpm2DeviceLibDTpmStandaloneMm.c
@@ -41,6 +42,7 @@ [Sources]
 [Packages]
   MdePkg/MdePkg.dec
   SecurityPkg/SecurityPkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -49,6 +51,7 @@ [LibraryClasses]
   TimerLib
   DebugLib
   PcdLib
+  VmgExitLib
 
 [Pcd]
   gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress            ## CONSUMES
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
index 31113d93ee41..7c41ab0a8531 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2InstanceLibDTpm.inf
@@ -29,6 +29,7 @@ [Defines]
 
 [Sources]
   Tpm2Tis.c
+  Tpm2Svsm.c
   Tpm2Ptp.c
   Tpm2InstanceLibDTpm.c
   Tpm2DeviceLibDTpmBase.c
@@ -37,6 +38,7 @@ [Sources]
 [Packages]
   MdePkg/MdePkg.dec
   SecurityPkg/SecurityPkg.dec
+  UefiCpuPkg/UefiCpuPkg.dec
 
 [LibraryClasses]
   BaseLib
@@ -45,6 +47,7 @@ [LibraryClasses]
   TimerLib
   DebugLib
   PcdLib
+  VmgExitLib
 
 [Pcd]
   gEfiSecurityPkgTokenSpaceGuid.PcdTpmBaseAddress          ## CONSUMES
diff --git a/SecurityPkg/Include/Library/Tpm2DeviceLib.h b/SecurityPkg/Include/Library/Tpm2DeviceLib.h
index 783bfa533394..b45ea0d07eb4 100644
--- a/SecurityPkg/Include/Library/Tpm2DeviceLib.h
+++ b/SecurityPkg/Include/Library/Tpm2DeviceLib.h
@@ -18,6 +18,7 @@ typedef enum {
   Tpm2PtpInterfaceTis,
   Tpm2PtpInterfaceFifo,
   Tpm2PtpInterfaceCrb,
+  Tpm2PtpInterfaceSvsm,
   Tpm2PtpInterfaceMax,
 } TPM2_PTP_INTERFACE_TYPE;
 
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.h b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.h
index d703f15a2fbb..8af67e31a757 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.h
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2DeviceLibDTpm.h
@@ -64,4 +64,37 @@ InternalTpm2DeviceLibDTpmCommonConstructor (
   VOID
   );
 
+/**
+  Check if an SVSM based TPM is present
+
+  @retval TRUE    SVSM TPM is present
+  @retval FALSE   no SVSM TPM is present
+**/
+BOOLEAN
+Tpm2IsSvsm (
+  VOID
+  );
+
+/**
+  Send a command to TPM for execution and return response data.
+
+  @param[in]      BufferIn      Buffer for command data.
+  @param[in]      SizeIn        Size of command data.
+  @param[in, out] BufferOut     Buffer for response data.
+  @param[in, out] SizeOut       Size of response data.
+
+  @retval EFI_SUCCESS           Operation completed successfully.
+  @retval EFI_BUFFER_TOO_SMALL  Response data buffer is too small.
+  @retval EFI_DEVICE_ERROR      Unexpected device behavior.
+  @retval EFI_UNSUPPORTED       Unsupported TPM version
+
+**/
+EFI_STATUS
+Tpm2SvsmTpmCommand (
+  IN     UINT8                 *BufferIn,
+  IN     UINT32                SizeIn,
+  IN OUT UINT8                 *BufferOut,
+  IN OUT UINT32                *SizeOut
+  );
+
 #endif // _TPM2_DEVICE_LIB_DTPM_H_
diff --git a/UefiCpuPkg/Include/Library/VmgExitLib.h b/UefiCpuPkg/Include/Library/VmgExitLib.h
index f32938e8f262..4b65bcba780c 100644
--- a/UefiCpuPkg/Include/Library/VmgExitLib.h
+++ b/UefiCpuPkg/Include/Library/VmgExitLib.h
@@ -242,4 +242,10 @@ VmgExitVmsaRmpAdjust (
   IN BOOLEAN           SetVmsa
   );
 
+BOOLEAN
+EFIAPI
+VmgExitVTPM (
+  IN OUT UINT8             *Buffer
+  );
+
 #endif
diff --git a/OvmfPkg/Library/VmgExitLib/VmgExitSvsm.c b/OvmfPkg/Library/VmgExitLib/VmgExitSvsm.c
index bbf8e5591a92..f3b169c4cba3 100644
--- a/OvmfPkg/Library/VmgExitLib/VmgExitSvsm.c
+++ b/OvmfPkg/Library/VmgExitLib/VmgExitSvsm.c
@@ -476,3 +476,39 @@ VmgExitVmsaRmpAdjust (
   return VmgExitSvsmPresent () ? SvsmVmsaRmpAdjust (Vmsa, ApicId, SetVmsa)
                                : BaseVmsaRmpAdjust (Vmsa, ApicId, SetVmsa);
 }
+
+
+/**
+  Tries to perform a SVSM call to the vTPM.
+
+  The buffer should contain a marshalled TPM command and will return a
+  marshalled TPM response.  Sending down a NULL buffer will do nothing but
+  can be used to probe for the existence of the SVSM vTPM (will return
+  non zero if no vTPM is present)
+
+  @param Buffer               A pointer to the input command and output response
+
+  @retval TRUE                The Command is processed (or the vTPM is present)
+  @retval FALSE               The vTPM is not present
+
+**/
+BOOLEAN
+EFIAPI
+VmgExitVTPM (
+  IN OUT UINT8             *Buffer
+  )
+{
+  SVSM_CAA                 *Caa;
+  SVSM_FUNCTION            Function;
+  UINTN                    Ret;
+
+  if (!VmgExitSvsmPresent ())
+    return FALSE;
+
+  Caa = SvsmGetCaa ();
+  Function.Protocol = 0;
+  Function.CallId = 8;
+
+  Ret = SvsmMsrProtocol (Caa, Function.Uint64, (UINT64)Buffer, 0, 0);
+  return (Ret == 0) ? TRUE : FALSE;
+}
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
index 1d99beaa101f..19e4ddf3e09c 100644
--- a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Ptp.c
@@ -422,6 +422,11 @@ Tpm2GetPtpInterface (
   PTP_CRB_INTERFACE_IDENTIFIER   InterfaceId;
   PTP_FIFO_INTERFACE_CAPABILITY  InterfaceCapability;
 
+  if (Tpm2IsSvsm ()) {
+    DEBUG((DEBUG_INFO, "Found SVSM TPM\n"));
+    return Tpm2PtpInterfaceSvsm;
+  }
+
   if (!Tpm2IsPtpPresence (Register)) {
     return Tpm2PtpInterfaceMax;
   }
@@ -595,6 +600,13 @@ DTpm2SubmitCommand (
                OutputParameterBlock,
                OutputParameterBlockSize
                );
+    case Tpm2PtpInterfaceSvsm:
+      return Tpm2SvsmTpmCommand (
+	       InputParameterBlock,
+               InputParameterBlockSize,
+               OutputParameterBlock,
+               OutputParameterBlockSize
+               );
     default:
       return EFI_NOT_FOUND;
   }
@@ -622,6 +634,8 @@ DTpm2RequestUseTpm (
     case Tpm2PtpInterfaceFifo:
     case Tpm2PtpInterfaceTis:
       return TisPcRequestUseTpm ((TIS_PC_REGISTERS_PTR)(UINTN)PcdGet64 (PcdTpmBaseAddress));
+    case Tpm2PtpInterfaceSvsm:
+      return EFI_SUCCESS;
     default:
       return EFI_NOT_FOUND;
   }
diff --git a/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Svsm.c b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Svsm.c
new file mode 100644
index 000000000000..ce0f162b9e01
--- /dev/null
+++ b/SecurityPkg/Library/Tpm2DeviceLibDTpm/Tpm2Svsm.c
@@ -0,0 +1,96 @@
+/** @file
+  SVSM TPM communication
+
+Copyright (c) 2023 James.Bottomley@HansenPartnership.com
+
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/BaseMemoryLib.h>
+#include <Library/DebugLib.h>
+#include <Library/Tpm2DeviceLib.h>
+#include <Library/PcdLib.h>
+#include <Library/VmgExitLib.h>
+
+#include "Tpm2DeviceLibDTpm.h"
+
+#pragma pack(1)
+typedef struct Tpm2SendCmdReq {
+	UINT32	Cmd;
+	UINT8	Locality;
+	UINT32	BufSize;
+	UINT8	Buf[];
+} Tpm2SendCmdReq;
+typedef struct Tpm2SendCmdResp {
+	INT32	Size;
+	UINT8	Buf[];
+} Tpm2SendCmdResp;
+#pragma pack()
+
+/* These defines come from the TPM platform interface */
+#define TPM_SEND_COMMAND		8
+#define TPM_PLATFORM_MAX_BUFFER		4096 /* max req/resp buffer size */
+
+STATIC UINT8 Tpm2SvsmBuffer[TPM_PLATFORM_MAX_BUFFER];
+
+/**
+  Send a command to TPM for execution and return response data.
+
+  @param[in]      BufferIn      Buffer for command data.
+  @param[in]      SizeIn        Size of command data.
+  @param[in, out] BufferOut     Buffer for response data.
+  @param[in, out] SizeOut       Size of response data.
+
+  @retval EFI_SUCCESS           Operation completed successfully.
+  @retval EFI_BUFFER_TOO_SMALL  Response data buffer is too small.
+  @retval EFI_DEVICE_ERROR      Unexpected device behavior.
+  @retval EFI_UNSUPPORTED       Unsupported TPM version
+
+**/
+EFI_STATUS
+Tpm2SvsmTpmCommand (
+  IN     UINT8                 *BufferIn,
+  IN     UINT32                SizeIn,
+  IN OUT UINT8                 *BufferOut,
+  IN OUT UINT32                *SizeOut
+  )
+{
+  Tpm2SendCmdReq  *req  = (Tpm2SendCmdReq *)Tpm2SvsmBuffer;
+  Tpm2SendCmdResp *resp = (Tpm2SendCmdResp *)Tpm2SvsmBuffer;
+
+  if (SizeIn > sizeof (Tpm2SvsmBuffer) - sizeof (*req)) {
+    return EFI_BUFFER_TOO_SMALL;
+  }
+
+  req->Cmd = TPM_SEND_COMMAND;
+  req->Locality = 0;
+  req->BufSize = SizeIn;
+  CopyMem (req->Buf, BufferIn, SizeIn);
+  if (!VmgExitVTPM (Tpm2SvsmBuffer)) {
+    return EFI_DEVICE_ERROR;
+  }
+
+  *SizeOut = resp->Size;
+  CopyMem (BufferOut, resp->Buf, *SizeOut);
+
+  return EFI_SUCCESS;
+}
+
+/**
+  Check if an SVSM based TPM is present
+
+  @retval TRUE    SVSM TPM is present
+  @retval FALSE   no SVSM TPM is present
+**/
+BOOLEAN
+Tpm2IsSvsm (
+  VOID
+  )
+{
+  BOOLEAN ret;
+  ret = VmgExitVTPM(NULL);
+  DEBUG((DEBUG_INFO, "Tpm2IsSvsm return %d\n", ret));
+  return ret;
+}
diff --git a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
index 02d72adab2b2..fd2b332980d9 100644
--- a/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
+++ b/UefiCpuPkg/Library/VmgExitLibNull/VmgExitLibNull.c
@@ -246,3 +246,26 @@ VmgExitVmsaRmpAdjust (
 {
   return EFI_UNSUPPORTED;
 }
+
+/**
+  Tries to perform a SVSM call to the vTPM.
+
+  The buffer should contain a marshalled TPM command and will return a
+  marshalled TPM response.  Sending down a NULL buffer will do nothing but
+  can be used to probe for the existence of the SVSM vTPM (will return
+  non zero if no vTPM is present)
+
+  @param Buffer               A pointer to the input command and output response
+
+  @retval TRUE                The Command is processed (or the vTPM is present)
+  @retval FALSE               The vTPM is not present
+
+**/
+BOOLEAN
+EFIAPI
+VmgExitVTPM (
+  IN OUT UINT8             *Buffer
+  )
+{
+  return FALSE;
+}
-- 
2.35.3

Re: [RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[Tom Lendacky]

On 1/3/23 15:01, James Bottomley wrote:
> This is a sketch for how a fully enlightened vTPM driver would work.
> The idea is that the SVSM responds on function 8 to vTPM requests, so
> we use that to send down a buffer which is modified on return (the
> buffer must be big enough, so the agreed protocol is it should be a
> page in length, which is larger than any possible TPM command or
> response).  The protocol used is the MSSIM one which is self describing
> in terms of length, so there's no need to transmit sizes (it also
> leaves room for expansion to localities and cancellation, which is
> useful in the light of discussions).  A NULL in place of the buffer is
> a probe and the SVSM call simply returns SVSM_SUCCESS without doing
> anything.  This can be used to probe for vTPM support because any other
> return would indicate no vTPM present.

As I write the vTPM section of the SVSM specification, I am making it a 
separate protocol number (2), not part of the core protocol. You can use 
the core protocol Query Protocol call to determine the presence of the 
vTPM protocol instead of sending a NULL buffer.

Hopefully I'll have something out very soon. I'll gear it towards the 
driver implementation that you are proposing.

How should we indicate when cancellation or locality support is available? 
Should there just be a protocol call ID that returns a bitmap of supported 
command values (e.g.: bit 8 would be set for TPM_SEND_COMMAND) and a 
bitmap of supported features?

Thanks,
Tom

> 
> Hopefully IBM will publish the new svsm-vtpm repo shortly, but we're
> still working with the old CRB based one at the moment, so it may take
> some time.
> 
> The three following patches are for two different repos.  Patch 1 will
> apply to any upstream Linux kernel, Patch 2 requires the non-upstream
> sev-snp repo and patch 3 is against the non upstream sev-snp edk repo.
> 
> James
> 
> 

Re: [RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[James Bottomley]

On Wed, 2023-01-04 at 16:44 -0600, Tom Lendacky wrote:
> How should we indicate when cancellation or locality support is
> available? Should there just be a protocol call ID that returns a
> bitmap of supported command values (e.g.: bit 8 would be set for
> TPM_SEND_COMMAND) and a bitmap of supported features?

I'm not sure anyone will ever care about cancellation.  There are only
two cancellable points in the entire MS TPM implementation; one is in
CryptRsa to abort the prime search and the other is in
CryptEccCommitCompute.  On a full power CPU system you're unlikely ever
to be able to signal a cancellation in time for it to be meaningfully
useful.

Discovery for all the commands bar locality is simply they return -
EINVAL if you try to invoke them in the SVSM when they're not
implemented (that's an extension to the MSSIM protocol).

Locality can't simply be added, it needs someone to come up with the
actual locality properties and for the SVSM to police them.  Presumably
discovery would be part of that exercise.

James

Re: [RFC 1/3] tpm: add generic platform device

[Dov Murik]

Hi James,

On 03/01/2023 23:02, James Bottomley wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> This is primarily designed to support an enlightened driver for the
> AMD svsm based vTPM, but it could be used by any platform which
> communicates with a TPM device.  The platform must fill in struct
> tpm_platform_ops as the platform_data and set the device name to "tpm"
> to have the binding by name work correctly.  The sole sendrecv
> function is designed to do a single buffer request/response conforming
> to the MSSIM protocol.  For the svsm vTPM case, this protocol is
> transmitted directly to the SVSM, but it could be massaged for other
> function type platform interfaces.
> 
> Signed-off-by: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  drivers/char/tpm/Kconfig        |   7 ++
>  drivers/char/tpm/Makefile       |   1 +
>  drivers/char/tpm/tpm_platform.c | 138 ++++++++++++++++++++++++++++++++
>  include/linux/tpm_platform.h    | 107 +++++++++++++++++++++++++
>  4 files changed, 253 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm_platform.c
>  create mode 100644 include/linux/tpm_platform.h
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index 4a5516406c22..c00ecec1d710 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -198,5 +198,12 @@ config TCG_FTPM_TEE
>  	help
>  	  This driver proxies for firmware TPM running in TEE.
>  
> +config TCG_PLATFORM
> +	tristate "Platform TPM Device"
> +	help
> +	  This driver requires a platform implementation to provide
> the
> +	  TPM function.  It will not bind if the implementation is not
> +	  present.
> +
>  source "drivers/char/tpm/st33zp24/Kconfig"
>  endif # TCG_TPM
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 66d39ea6bd10..d87512a64ffd 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_TCG_XEN) += xen-tpmfront.o
>  obj-$(CONFIG_TCG_CRB) += tpm_crb.o
>  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
>  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> +obj-$(CONFIG_TCG_PLATFORM) += tpm_platform.o
> diff --git a/drivers/char/tpm/tpm_platform.c
> b/drivers/char/tpm/tpm_platform.c
> new file mode 100644
> index 000000000000..46d60fd23a0d
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_platform.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Platform based TPM emulator
> + *
> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
> + *
> + * Designed to handle a simple function request/response single buffer
> + * TPM or vTPM rooted in the platform.  This device driver uses the
> + * MSSIM protocol from the Microsoft reference implementation
> + *
> + * https://github.com/microsoft/ms-tpm-20-ref
> + *
> + * to communicate between the driver and the platform.  This is rich
> + * enough to allow platform operations like cancellation The platform
> + * should not act on platform commands like power on/off and reset
> + * which can disrupt the TPM guarantees.
> + *
> + * This driver is designed to be single threaded (one call in to the
> + * platform TPM at any one time).  The threading guarantees are
> + * provided by the chip mutex.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/tpm_platform.h>
> +
> +#include "tpm.h"
> +
> +static struct tpm_platform_ops *pops;
> +
> +static u8 *buffer;
> +/*
> + * FIXME: before implementing locality we need to agree what it means
> + * to the platform
> + */
> +static u8 locality;
> +
> +static int tpm_platform_send(struct tpm_chip *chip, u8 *buf, size_t
> len)
> +{
> +	int ret;
> +	struct tpm_send_cmd_req *req = (struct tpm_send_cmd_req
> *)buffer;
> +
> +	if (len > TPM_PLATFORM_MAX_BUFFER - sizeof(*req))
> +		return -EINVAL;
> +	req->cmd = TPM_SEND_COMMAND;
> +	req->locality = locality;
> +	req->inbuf_size = len;

In include/linux/tpm_platform.h (below) the comment says:

+ * Note that @inbuf_size must be large enough to hold the response so
+ * represents the maximum buffer size, not the size of the specific
+ * TPM command.

but here we don't set req->inbuf_size to the maximum size (which is
TPM_PLATFORM_MAX_BUFFER - sizeof(*req) ). Is that OK?


> +	memcpy(req->inbuf, buf, len);
> +
> +	ret = pops->sendrcv(buffer);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tpm_platform_recv(struct tpm_chip *chip, u8 *buf, size_t
> len)
> +{
> +	struct tpm_resp *resp = (struct tpm_resp *)buffer;
> +
> +	if (resp->size < 0)
> +		return resp->size;
> +
> +	if (len < resp->size)
> +		return -E2BIG;


I suggest another check before the memcpy:

if (resp->size > TPM_PLATFORM_MAX_BUFFER - sizeof(*resp))
	return -EINVAL;  // Invalid response from the platform TPM


> +
> +	memcpy(buf, buffer + sizeof(*resp), resp->size);
> +
> +	return resp->size;

Is there a possibility of a double recv?
Should we set resp->size = 0 (or -1) before returning? (while still
returning the original value)


> +}
> +
> +static struct tpm_class_ops tpm_chip_ops = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
> +	.send = tpm_platform_send,
> +	.recv = tpm_platform_recv,
> +};
> +
> +static struct platform_driver tpm_platform_driver = {
> +	.driver = {
> +		.name = "tpm",
> +	},
> +};
> +
> +static int __init tpm_platform_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct tpm_chip *chip;
> +	int err;
> +
> +	if (!dev->platform_data)
> +		return -ENODEV;
> +
> +	/*
> +	 * in theory platform matching should mean this is always
> +	 * true, but just in case anyone tries force binding
> +	 */
> +	if(strcmp(pdev->name, tpm_platform_driver.driver.name) != 0)
> +		return -ENODEV;
> +
> +	if (!buffer)
> +		buffer = kmalloc(TPM_PLATFORM_MAX_BUFFER, GFP_KERNEL);
> +
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	pops = dev->platform_data;
> +
> +	chip = tpmm_chip_alloc(dev, &tpm_chip_ops);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	/*
> +	 * Setting TPM_CHIP_FLAG_IRQ guarantees that ->recv will be
> +	 * called straight after ->send and means we don't need to
> +	 * implement any other chip ops.
> +	 */
> +	chip->flags |= TPM_CHIP_FLAG_IRQ;
> +	err = tpm2_probe(chip);
> +	if (err)
> +		return err;
> +
> +	err = tpm_chip_register(chip);
> +	if (err)
> +		return err;
> +
> +	dev_info(dev, "TPM %s platform device\n",
> +		 (chip->flags & TPM_CHIP_FLAG_TPM2) ? "2.0" : "1.2");
> +
> +	return 0;
> +}
> +
> +module_platform_driver_probe(tpm_platform_driver, tpm_platform_probe);
> +
> +MODULE_AUTHOR("James Bottomley
> <James.Bottomley@HansenPartnership.com>");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Platform TPM Driver");
> +MODULE_ALIAS("platform:tpm");
> diff --git a/include/linux/tpm_platform.h
> b/include/linux/tpm_platform.h
> new file mode 100644
> index 000000000000..a90089affe0b
> --- /dev/null
> +++ b/include/linux/tpm_platform.h
> @@ -0,0 +1,107 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * Copyright (C) 2023 James.Bottomley@HansenPartnership.com
> + *
> + * Interface specification for platforms wishing to activate the
> + * platform tpm device.  The device must be a platform device created
> + * with the name "tpm" and it must populate platform_data with struct
> + * tpm_platform_ops
> + */
> +
> +/*
> + * The current MSSIM TPM commands we support.  The complete list is
> + * in the TcpTpmProtocol header:
> + *
> + *
> https://github.com/microsoft/ms-tpm-20-ref/blob/main/TPMCmd/Simulator/include/TpmTcpProtocol.h
> + */
> +
> +#define TPM_SEND_COMMAND		8
> +#define TPM_SIGNAL_CANCEL_ON		9
> +#define TPM_SIGNAL_CANCEL_OFF		10
> +/*
> + * Any platform specific commands should be placed here and should
> start
> + * at 0x8000 to avoid clashes with the MSSIM protocol.  They should
> follow
> + * the same self describing buffer format below
> + */
> +
> +#define TPM_PLATFORM_MAX_BUFFER		4096 /* max req/resp
> buffer size */
> +
> +/**
> + * struct tpm_platform_ops - the share platform operations
> + *
> + * @sendrecv:	Send a TPM command using the MSSIM protocol.
> + *
> + * The MSSIM protocol is designed for a network, so the buffers are
> + * self describing.  The minimum buffer size is sizeof(u32).  Every
> + * MSSIM command defines its own transport buffer and the command is
> + * sent in the first u32 array.  The only modification we make is that
> + * the MSSIM uses network order and we use the endianness of the
> + * architecture.  The response to every command (in the same buffer)
> + * is a u32 size preceded array.  Most of the MSSIM commands simply
> + * return zero here because they have no defined response.
> + *
> + * The only command with a defined request/response size is
> TPM_SEND_COMMAND
> + * The definition is in the structures below
> + */
> +struct tpm_platform_ops {
> +	int (*sendrcv)(u8 *buffer);

nit: In the commit message and struct description you
use "sendrecv" but the member here is called "sendrcv".

-Dov

> +};
> +
> +/**
> + * struct tpm_send_cmd_req - Structure for a TPM_SEND_COMMAND
> + *
> + * @cmd:	The command (must be TPM_SEND_COMMAND)
> + * @locality:	The locality
> + * @inbuf_size:	The size of the input buffer following
> + * @inbuf:	A buffer of size inbuf_size
> + *
> + * Note that @inbuf_size must be large enough to hold the response so
> + * represents the maximum buffer size, not the size of the specific
> + * TPM command.
> + */
> +struct tpm_send_cmd_req {
> +	u32	cmd;
> +	u8	locality;
> +	u32	inbuf_size;
> +	u8	inbuf[];
> +} __packed;
> +
> +/**
> + * struct tpm_req - generic request header for single word command
> + *
> + * @cmd:	The command to send
> + */
> +struct tpm_req {
> +	u32	cmd;
> +} __packed;
> +
> +/**
> + * struct tpm_resp - generic response header
> + *
> + * @size:	The response size (zero if nothing follows)
> + *
> + * Note: most MSSIM commands simply return zero here with no
> indication
> + * of success or failure.
> + */
> +
> +struct tpm_resp {
> +	s32	size;
> +} __packed;
> +
> +
> +
> +
> +
> +/*
> + *
> + *  uint32_t TPM_SEND_COMMAND, uint8_t Locality, uint32_t
> InBufferSize,
> + *    uint8_t[InBufferSize] InBuffer
> + *
> + * Where the InBufferSize must be big enough to hold the response
> + * because it represents the maximum allowable response size.  So if
> + * the first u32 is TPM_SEND_COMMAND, the minimum buffer size is
> + * 9. The response is
> + *
> + * uint32_t OutBufferSize, uint8_t[OutBufferSize] OutBuffer
> + * 
> + */

Re: [RFC 1/3] tpm: add generic platform device

[James Bottomley]

On Thu, 2023-01-05 at 10:08 +0200, Dov Murik wrote:
[...]
> > +static int tpm_platform_send(struct tpm_chip *chip, u8 *buf,
> > size_t
> > len)
> > +{
> > +       int ret;
> > +       struct tpm_send_cmd_req *req = (struct tpm_send_cmd_req
> > *)buffer;
> > +
> > +       if (len > TPM_PLATFORM_MAX_BUFFER - sizeof(*req))
> > +               return -EINVAL;
> > +       req->cmd = TPM_SEND_COMMAND;
> > +       req->locality = locality;
> > +       req->inbuf_size = len;
> 
> In include/linux/tpm_platform.h (below) the comment says:
> 
> + * Note that @inbuf_size must be large enough to hold the response
> so
> + * represents the maximum buffer size, not the size of the specific
> + * TPM command.
> 
> but here we don't set req->inbuf_size to the maximum size (which is
> TPM_PLATFORM_MAX_BUFFER - sizeof(*req) ). Is that OK?

I'll fix the comment.  Unfortunately the ms TPM has a check in the
ExecuteCommand() routine that checks the command and request size are
equal.  I had originally thought to use this field to communicate max
size, but it's not really any problem to make it a define.



> 
> 
> > +       memcpy(req->inbuf, buf, len);
> > +
> > +       ret = pops->sendrcv(buffer);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +static int tpm_platform_recv(struct tpm_chip *chip, u8 *buf,
> > size_t
> > len)
> > +{
> > +       struct tpm_resp *resp = (struct tpm_resp *)buffer;
> > +
> > +       if (resp->size < 0)
> > +               return resp->size;
> > +
> > +       if (len < resp->size)
> > +               return -E2BIG;
> 
> 
> I suggest another check before the memcpy:
> 
> if (resp->size > TPM_PLATFORM_MAX_BUFFER - sizeof(*resp))
>         return -EINVAL;  // Invalid response from the platform TPM

Yes, I already added that because of your matrix comments.

> 
> 
> > +
> > +       memcpy(buf, buffer + sizeof(*resp), resp->size);
> > +
> > +       return resp->size;
> 
> Is there a possibility of a double recv?
> Should we set resp->size = 0 (or -1) before returning? (while still
> returning the original value)

Not according to the way ->recv() is used, but even if there were, it
should return the previous command until there is a new one.

[...]
> > +/**
> > + * struct tpm_platform_ops - the share platform operations
> > + *
> > + * @sendrecv:  Send a TPM command using the MSSIM protocol.
> > + *
> > + * The MSSIM protocol is designed for a network, so the buffers
> > are
> > + * self describing.  The minimum buffer size is sizeof(u32). 
> > Every
> > + * MSSIM command defines its own transport buffer and the command
> > is
> > + * sent in the first u32 array.  The only modification we make is
> > that
> > + * the MSSIM uses network order and we use the endianness of the
> > + * architecture.  The response to every command (in the same
> > buffer)
> > + * is a u32 size preceded array.  Most of the MSSIM commands
> > simply
> > + * return zero here because they have no defined response.
> > + *
> > + * The only command with a defined request/response size is
> > TPM_SEND_COMMAND
> > + * The definition is in the structures below
> > + */
> > +struct tpm_platform_ops {
> > +       int (*sendrcv)(u8 *buffer);
> 
> nit: In the commit message and struct description you
> use "sendrecv" but the member here is called "sendrcv".

OK, I'll; make the comments consistent (kernel docbook would complain
to the people who bother to run it).

James

Re: [RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[Stefano Garzarella]

Hi James,

On Tue, Jan 03, 2023 at 04:01:33PM -0500, James Bottomley wrote:
>This is a sketch for how a fully enlightened vTPM driver would work.
>The idea is that the SVSM responds on function 8 to vTPM requests, so
>we use that to send down a buffer which is modified on return (the
>buffer must be big enough, so the agreed protocol is it should be a
>page in length, which is larger than any possible TPM command or
>response).  The protocol used is the MSSIM one which is self describing
>in terms of length, so there's no need to transmit sizes (it also
>leaves room for expansion to localities and cancellation, which is
>useful in the light of discussions).  A NULL in place of the buffer is
>a probe and the SVSM call simply returns SVSM_SUCCESS without doing
>anything.  This can be used to probe for vTPM support because any other
>return would indicate no vTPM present.
>
>Hopefully IBM will publish the new svsm-vtpm repo shortly, but we're
>still working with the old CRB based one at the moment, so it may take
>some time.
>
>The three following patches are for two different repos.  Patch 1 will
>apply to any upstream Linux kernel, Patch 2 requires the non-upstream
>sev-snp repo and patch 3 is against the non upstream sev-snp edk repo.

Thanks for this series, I'd like to restart this work and Claudio 
pointed me here.

IIUC the last version we have is the one in Coconut [1] which is based 
on this series, but with some changes on top from Claudio to probe SVSM 
and check if TPM commands are supported or not.

I already rebased them for 6.11 here [2] and we will include them in the 
next coconut-svsm branch, but in the meantime I think nothing is 
blocking us to upstream them, so I can take care of Linux patches, while 
Oliver (in CC) is working on the EDK2 patch.

IIUC the only thing missing from the patches we have in Coconut are the 
fixes to Don's comments. Do you have a branch or patches with them 
already fixed?
Otherwise I can do it on top of the patches we have in Coconut.

Any other things to fix/improve that you can think of?

Let me know if it is okay with you that I will restart working on these 
patches.

Thanks,
Stefano

[1] https://github.com/coconut-svsm/linux/tree/svsm
[2] https://github.com/coconut-svsm/linux/pull/8

Re: [RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[James Bottomley]

On Wed, 2024-11-06 at 12:19 +0100, Stefano Garzarella wrote:
> Hi James,
> 
> On Tue, Jan 03, 2023 at 04:01:33PM -0500, James Bottomley wrote:
> > This is a sketch for how a fully enlightened vTPM driver would
> > work. The idea is that the SVSM responds on function 8 to vTPM
> > requests, so we use that to send down a buffer which is modified on
> > return (the buffer must be big enough, so the agreed protocol is it
> > should be a page in length, which is larger than any possible TPM
> > command or response).  The protocol used is the MSSIM one which is
> > self describing in terms of length, so there's no need to transmit
> > sizes (it also leaves room for expansion to localities and
> > cancellation, which is useful in the light of discussions).  A NULL
> > in place of the buffer is a probe and the SVSM call simply returns
> > SVSM_SUCCESS without doing anything.  This can be used to probe for
> > vTPM support because any other return would indicate no vTPM
> > present.
> > 
> > Hopefully IBM will publish the new svsm-vtpm repo shortly, but
> > we're still working with the old CRB based one at the moment, so it
> > may take some time.
> > 
> > The three following patches are for two different repos.  Patch 1
> > will apply to any upstream Linux kernel, Patch 2 requires the
> > non-upstream sev-snp repo and patch 3 is against the non upstream
> > sev-snp edk repo.
> 
> Thanks for this series, I'd like to restart this work and Claudio 
> pointed me here.
> 
> IIUC the last version we have is the one in Coconut [1] which is
> based on this series, but with some changes on top from Claudio to
> probe SVSM and check if TPM commands are supported or not.
> 
> I already rebased them for 6.11 here [2] and we will include them in
> the next coconut-svsm branch, but in the meantime I think nothing is 
> blocking us to upstream them, so I can take care of Linux patches,
> while Oliver (in CC) is working on the EDK2 patch.
> 
> IIUC the only thing missing from the patches we have in Coconut are
> the fixes to Don's comments. Do you have a branch or patches with
> them already fixed?

What are Don's comments? Did you mean Dov and this one:

https://lore.kernel.org/linux-coco/f7d0bd07-ba1b-894e-5e39-15fb1817bc8b@linux.ibm.com/

? Sure, but I thought Claudio had already addressed those? I can take a
look to see where this is, but I haven't been following closely.

> Otherwise I can do it on top of the patches we have in Coconut.
> 
> Any other things to fix/improve that you can think of?
> 
> Let me know if it is okay with you that I will restart working on
> these patches.

The original was pretty simple: a platform TPM with a single
send/receive and the implementation inside the SVSM call code. I think
the only real change is switching from the msr method of calling to the
ghcb one, which it looks like is already done (or at least indirected
to the proper snp wrapper routine that does it)?

Regards,

James

Re: [RFC 0/3] Enlightened vTPM support for SVSM on SEV-SNP

[Stefano Garzarella]

On Wed, Nov 06, 2024 at 09:54:15AM -0500, James Bottomley wrote:
>On Wed, 2024-11-06 at 12:19 +0100, Stefano Garzarella wrote:
>> Hi James,
>>
>> On Tue, Jan 03, 2023 at 04:01:33PM -0500, James Bottomley wrote:
>> > This is a sketch for how a fully enlightened vTPM driver would
>> > work. The idea is that the SVSM responds on function 8 to vTPM
>> > requests, so we use that to send down a buffer which is modified on
>> > return (the buffer must be big enough, so the agreed protocol is it
>> > should be a page in length, which is larger than any possible TPM
>> > command or response).  The protocol used is the MSSIM one which is
>> > self describing in terms of length, so there's no need to transmit
>> > sizes (it also leaves room for expansion to localities and
>> > cancellation, which is useful in the light of discussions).  A NULL
>> > in place of the buffer is a probe and the SVSM call simply returns
>> > SVSM_SUCCESS without doing anything.  This can be used to probe for
>> > vTPM support because any other return would indicate no vTPM
>> > present.
>> >
>> > Hopefully IBM will publish the new svsm-vtpm repo shortly, but
>> > we're still working with the old CRB based one at the moment, so it
>> > may take some time.
>> >
>> > The three following patches are for two different repos.  Patch 1
>> > will apply to any upstream Linux kernel, Patch 2 requires the
>> > non-upstream sev-snp repo and patch 3 is against the non upstream
>> > sev-snp edk repo.
>>
>> Thanks for this series, I'd like to restart this work and Claudio
>> pointed me here.
>>
>> IIUC the last version we have is the one in Coconut [1] which is
>> based on this series, but with some changes on top from Claudio to
>> probe SVSM and check if TPM commands are supported or not.
>>
>> I already rebased them for 6.11 here [2] and we will include them in
>> the next coconut-svsm branch, but in the meantime I think nothing is
>> blocking us to upstream them, so I can take care of Linux patches,
>> while Oliver (in CC) is working on the EDK2 patch.
>>
>> IIUC the only thing missing from the patches we have in Coconut are
>> the fixes to Don's comments. Do you have a branch or patches with
>> them already fixed?
>
>What are Don's comments? Did you mean Dov and this one:
>
>https://lore.kernel.org/linux-coco/f7d0bd07-ba1b-894e-5e39-15fb1817bc8b@linux.ibm.com/
>
>?

Ooops, yep I meant Dov's comments (exactly that thread), sorry about 
that!

>Sure, but I thought Claudio had already addressed those? I can take a
>look to see where this is, but I haven't been following closely.

Sure, I haven't seen those changes in the Coconut branch, but they seem 
straightforward such that I could make them if they got lost, no 
problem.

>
>> Otherwise I can do it on top of the patches we have in Coconut.
>>
>> Any other things to fix/improve that you can think of?
>>
>> Let me know if it is okay with you that I will restart working on
>> these patches.
>
>The original was pretty simple: a platform TPM with a single
>send/receive and the implementation inside the SVSM call code. I think
>the only real change is switching from the msr method of calling to the
>ghcb one, which it looks like is already done (or at least indirected
>to the proper snp wrapper routine that does it)?

Yeah, that part should be fixed in the Coconut version since we now call 
svsm_perform_call_protocol() that should handles ghcb/msr IIUC.

Thanks,
Stefano