[PATCH v2 0/3] ftpm: a firmware based TPM driver

[PATCH v2 0/3] ftpm: a firmware based TPM driver

[Sasha Levin]

Changes since v1:

 - Code nits (remove flags, debug statements, close up leaks, etc)
 - Add docs under Documentation/

Sasha Levin (3):
  ftpm: dt-binding: add dts documentation for fTPM driver
  ftpm: firmware TPM running in TEE
  ftpm: add documentation for ftpm driver

 .../bindings/security/tpm/tpm_ftpm_tee.txt    |  13 +
 .../devicetree/bindings/vendor-prefixes.txt   |   1 +
 Documentation/security/tpm/index.rst          |   1 +
 Documentation/security/tpm/tpm_ftpm_tee.rst   |  31 ++
 drivers/char/tpm/Kconfig                      |   5 +
 drivers/char/tpm/Makefile                     |   1 +
 drivers/char/tpm/tpm_ftpm_tee.c               | 346 ++++++++++++++++++
 drivers/char/tpm/tpm_ftpm_tee.h               |  52 +++
 8 files changed, 450 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
 create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
 create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
 create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h

-- 
2.19.1

[PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Sasha Levin]

The parameters are similar to the ones used by IBM's vTPM and the
various I2C tpm drivers.

Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
 .../devicetree/bindings/vendor-prefixes.txt         |  1 +
 2 files changed, 14 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt

diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
new file mode 100644
index 000000000000..20fca67a56c4
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
@@ -0,0 +1,13 @@
+Required properties:
+- compatible: should be "microsoft,ftpm"
+- linux,sml-base: 64-bit base address of the reserved memory allocated
+		  for the firmware event log
+- linux,sml-size: size of the memory allocated for the firmware event log
+
+Example:
+
+tpm@0 {
+	compatible = "microsoft,ftpm";
+	linux,sml-base = <0x0 0xC0000000>;
+	linux,sml-size = <0x10000>;
+};
diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt
index 8162b0eb4b50..902798bcb9a5 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.txt
+++ b/Documentation/devicetree/bindings/vendor-prefixes.txt
@@ -249,6 +249,7 @@ micrel	Micrel Inc.
 microchip	Microchip Technology Inc.
 microcrystal	Micro Crystal AG
 micron	Micron Technology Inc.
+microsoft	Microsoft Corporation
 mikroe		MikroElektronika d.o.o.
 minix	MINIX Technology Ltd.
 miramems	MiraMEMS Sensing Technology Co., Ltd.
-- 
2.19.1

[PATCH v2 2/3] ftpm: firmware TPM running in TEE

[Sasha Levin]

This patch adds support for a software-only implementation of a TPM
running in TEE.

There is extensive documentation of the design here:
https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/ .

As well as reference code for the firmware available here:
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM

Signed-off-by: Thirupathaiah Annapureddy <thiruan@microsoft.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/char/tpm/Kconfig        |   5 +
 drivers/char/tpm/Makefile       |   1 +
 drivers/char/tpm/tpm_ftpm_tee.c | 346 ++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_ftpm_tee.h |  52 +++++
 4 files changed, 404 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_ftpm_tee.c
 create mode 100644 drivers/char/tpm/tpm_ftpm_tee.h

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index 536e55d3919f..5638726641eb 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -164,6 +164,11 @@ config TCG_VTPM_PROXY
 	  /dev/vtpmX and a server-side file descriptor on which the vTPM
 	  can receive commands.
 
+config TCG_FTPM_TEE
+	tristate "TEE based fTPM Interface"
+	depends on TEE
+	---help---
+	  This driver proxies for fTPM running in TEE
 
 source "drivers/char/tpm/st33zp24/Kconfig"
 endif # TCG_TPM
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index a01c4cab902a..c354cdff9c62 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -33,3 +33,4 @@ obj-$(CONFIG_TCG_TIS_ST33ZP24) += st33zp24/
 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
diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
new file mode 100644
index 000000000000..673f9a8b8603
--- /dev/null
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -0,0 +1,346 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Microsoft Corporation
+ */
+
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/acpi.h>
+#include <linux/platform_device.h>
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+#include <linux/tpm.h>
+
+#include "tpm.h"
+#include "tpm_ftpm_tee.h"
+
+#define DRIVER_NAME "ftpm-tee"
+
+/*
+ * Note: ftpm_tee_tpm_op_recv and ftpm_tee_tpm_op_send are called from the
+ * same routine tpm_try_transmit in tpm-interface.c. These calls are protected
+ * by chip->tpm_mutex => There is no need for protecting any data shared
+ * between these routines ex: struct ftpm_tee_private
+ */
+
+/**
+ * ftpm_tee_tpm_op_recv retrieve fTPM response.
+ * @param: chip, the tpm_chip description as specified in driver/char/tpm/tpm.h.
+ * @param: buf,	the buffer to store data.
+ * @param: count, the number of bytes to read.
+ * @return: In case of success the number of bytes received.
+ *	    In other case, a < 0 value describing the issue.
+ */
+static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
+	size_t len;
+
+	len = pvt_data->resp_len;
+	if (count < len) {
+		dev_err(&chip->dev,
+			"%s:Invalid size in recv: count=%zd, resp_len=%zd\n",
+			__func__, count, len);
+		return -EIO;
+	}
+
+	memcpy(buf, pvt_data->resp_buf, len);
+	pvt_data->resp_len = 0;
+
+	return len;
+}
+
+/**
+ * ftpm_tee_tpm_op_send send TPM commands through the TEE shared memory.
+ *
+ * @param: chip, the tpm_chip description as specified in driver/char/tpm/tpm.h
+ * @param: buf,	the buffer to send.
+ * @param: len, the number of bytes to send.
+ * @return: In case of success, returns 0.
+ *	    In other case, a < 0 value describing the issue.
+ */
+static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
+{
+	struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
+	size_t resp_len;
+	int rc;
+	u8 *temp_buf;
+	struct tpm_header *resp_header;
+	struct tee_ioctl_invoke_arg transceive_args;
+	struct tee_param command_params[4];
+	struct tee_shm *shm = pvt_data->shm;
+
+	if (len > MAX_COMMAND_SIZE) {
+		dev_err(&chip->dev,
+			"%s:len=%zd exceeds MAX_COMMAND_SIZE supported by fTPM TA\n",
+			__func__, len);
+		return -EIO;
+	}
+
+	memset(&transceive_args, 0, sizeof(transceive_args));
+	memset(command_params, 0, sizeof(command_params));
+	pvt_data->resp_len = 0;
+
+	/* Invoke FTPM_OPTEE_TA_SUBMIT_COMMAND function of fTPM TA */
+	transceive_args = (struct tee_ioctl_invoke_arg) {
+		.func = FTPM_OPTEE_TA_SUBMIT_COMMAND,
+		.session = pvt_data->session,
+		.num_params = 4,
+	};
+
+	/* Fill FTPM_OPTEE_TA_SUBMIT_COMMAND parameters */
+	command_params[0] = (struct tee_param) {
+		.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
+		.u.memref = {
+			.shm = shm,
+			.size = len,
+			.shm_offs = 0,
+		},
+	};
+
+	temp_buf = tee_shm_get_va(shm, 0);
+	if (IS_ERR(temp_buf)) {
+		dev_err(&chip->dev, "%s:tee_shm_get_va failed for transmit\n",
+			__func__);
+		return PTR_ERR(temp_buf);
+	}
+	memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE));
+
+	memcpy(temp_buf, buf, len);
+
+	command_params[1] = (struct tee_param) {
+		.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
+		.u.memref = {
+			.shm = shm,
+			.size = MAX_RESPONSE_SIZE,
+			.shm_offs = MAX_COMMAND_SIZE,
+		},
+	};
+
+	rc = tee_client_invoke_func(pvt_data->ctx, &transceive_args,
+					command_params);
+	if ((rc < 0) || (transceive_args.ret != 0)) {
+		dev_err(&chip->dev, "%s:SUBMIT_COMMAND invoke error: 0x%x\n",
+			__func__, transceive_args.ret);
+		return (rc < 0) ? rc : transceive_args.ret;
+	}
+
+	temp_buf = tee_shm_get_va(shm, command_params[1].u.memref.shm_offs);
+	if (IS_ERR(temp_buf)) {
+		dev_err(&chip->dev, "%s:tee_shm_get_va failed for receive\n",
+			__func__);
+		return PTR_ERR(temp_buf);
+	}
+
+	resp_header = (struct tpm_header *)temp_buf;
+	resp_len = be32_to_cpu(resp_header->length);
+
+	/* sanity check resp_len */
+	if (resp_len < TPM_HEADER_SIZE) {
+		dev_err(&chip->dev, "%s:tpm response header too small\n",
+			__func__);
+		return -EIO;
+	}
+	if (resp_len > MAX_RESPONSE_SIZE) {
+		dev_err(&chip->dev,
+			"%s:resp_len=%zd exceeds MAX_RESPONSE_SIZE\n",
+			__func__, resp_len);
+		return -EIO;
+	}
+
+	/* sanity checks look good, cache the response */
+	memcpy(pvt_data->resp_buf, temp_buf, resp_len);
+	pvt_data->resp_len = resp_len;
+
+	return 0;
+}
+
+static void ftpm_tee_tpm_op_cancel(struct tpm_chip *chip)
+{
+	/* not supported */
+}
+
+static u8 ftpm_tee_tpm_op_status(struct tpm_chip *chip)
+{
+	return 0;
+}
+
+static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
+{
+	return 0;
+}
+
+static const struct tpm_class_ops ftpm_tee_tpm_ops = {
+	.flags = TPM_OPS_AUTO_STARTUP,
+	.recv = ftpm_tee_tpm_op_recv,
+	.send = ftpm_tee_tpm_op_send,
+	.cancel = ftpm_tee_tpm_op_cancel,
+	.status = ftpm_tee_tpm_op_status,
+	.req_complete_mask = 0,
+	.req_complete_val = 0,
+	.req_canceled = ftpm_tee_tpm_req_canceled,
+};
+
+/*
+ * Check whether this driver supports the fTPM TA in the TEE instance
+ * represented by the params (ver/data) to this function.
+ */
+static int ftpm_tee_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	/*
+	 * Currently this driver only support GP Complaint OPTEE based fTPM TA
+	 */
+	if ((ver->impl_id == TEE_IMPL_ID_OPTEE) &&
+		(ver->gen_caps & TEE_GEN_CAP_GP))
+		return 1;
+	else
+		return 0;
+}
+
+/*
+ * Undo what has been done in ftpm_tee_probe
+ */
+static void ftpm_tee_deinit(struct ftpm_tee_private *pvt_data)
+{
+	/* Release the chip */
+	tpm_chip_unregister(pvt_data->chip);
+
+	/* frees chip */
+	if (pvt_data->chip)
+		put_device(&pvt_data->chip->dev);
+
+	if (pvt_data->ctx) {
+		/* Free the shared memory pool */
+		tee_shm_free(pvt_data->shm);
+
+		/* close the existing session with fTPM TA*/
+		tee_client_close_session(pvt_data->ctx, pvt_data->session);
+
+		/* close the context with TEE driver */
+		tee_client_close_context(pvt_data->ctx);
+	}
+
+	/* memory allocated with devm_kzalloc() is freed automatically */
+}
+
+/**
+ * ftpm_tee_probe initialize the fTPM
+ * @param: pdev, the platform_device description.
+ * @return: 0 in case of success.
+ *	 or a negative value describing the error.
+ */
+static int ftpm_tee_probe(struct platform_device *pdev)
+{
+	int rc;
+	struct tpm_chip *chip;
+	struct device *dev = &pdev->dev;
+	struct ftpm_tee_private *pvt_data = NULL;
+	struct tee_ioctl_open_session_arg sess_arg;
+
+	pvt_data = devm_kzalloc(dev, sizeof(struct ftpm_tee_private),
+				GFP_KERNEL);
+	if (!pvt_data)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, pvt_data);
+
+	/* Open context with TEE driver */
+	pvt_data->ctx = tee_client_open_context(NULL, ftpm_tee_match, NULL,
+						NULL);
+	if (IS_ERR(pvt_data->ctx)) {
+		dev_err(dev, "%s:tee_client_open_context failed\n", __func__);
+		return -EPROBE_DEFER;
+	}
+
+	/* Open a session with fTPM TA */
+	memset(&sess_arg, 0, sizeof(sess_arg));
+	memcpy(sess_arg.uuid, ftpm_ta_uuid.b, TEE_IOCTL_UUID_LEN);
+	sess_arg.clnt_login = TEE_IOCTL_LOGIN_PUBLIC;
+	sess_arg.num_params = 0;
+
+	rc = tee_client_open_session(pvt_data->ctx, &sess_arg, NULL);
+	if ((rc < 0) || (sess_arg.ret != 0)) {
+		dev_err(dev, "%s:tee_client_open_session failed, err=%x\n",
+			__func__, sess_arg.ret);
+		rc = -EINVAL;
+		goto out_tee_session;
+	}
+	pvt_data->session = sess_arg.session;
+
+	/* Allocate dynamic shared memory with fTPM TA */
+	pvt_data->shm = tee_shm_alloc(pvt_data->ctx,
+				(MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE),
+				TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
+	if (IS_ERR(pvt_data->shm)) {
+		dev_err(dev, "%s:tee_shm_alloc failed\n", __func__);
+		rc = -ENOMEM;
+		goto out_shm_alloc;
+	}
+
+	/* Allocate new struct tpm_chip instance */
+	chip = tpm_chip_alloc(dev, &ftpm_tee_tpm_ops);
+	if (IS_ERR(chip)) {
+		dev_err(dev, "%s:tpm_chip_alloc failed\n", __func__);
+		rc = PTR_ERR(chip);
+		goto out_chip_alloc;
+	}
+
+	pvt_data->chip = chip;
+	pvt_data->chip->flags |= TPM_CHIP_FLAG_TPM2;
+
+	/* Create a character device for the fTPM */
+	rc = tpm_chip_register(pvt_data->chip);
+	if (rc) {
+		dev_err(dev, "%s:tpm_chip_register failed with rc=%d\n",
+			__func__, rc);
+		goto out_chip;
+	}
+
+	return 0;
+
+out_chip:
+	put_device(&pvt_data->chip->dev);
+out_chip_alloc:
+	tee_shm_free(pvt_data->shm);
+out_shm_alloc:
+	tee_client_close_session(pvt_data->ctx, pvt_data->session);
+out_tee_session:
+	tee_client_close_context(pvt_data->ctx);
+
+	return rc;
+}
+
+/**
+ * ftpm_tee_remove remove the TPM device
+ * @param: pdev, the platform_device description.
+ * @return: 0 in case of success.
+ */
+static int ftpm_tee_remove(struct platform_device *pdev)
+{
+	struct ftpm_tee_private *pvt_data = dev_get_drvdata(&pdev->dev);
+
+	ftpm_tee_deinit(pvt_data);
+
+	return 0;
+}
+
+static const struct of_device_id of_ftpm_tee_ids[] = {
+	{ .compatible = "microsoft,ftpm" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, of_ftpm_tee_ids);
+
+static struct platform_driver ftpm_tee_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = of_match_ptr(of_ftpm_tee_ids),
+	},
+	.probe = ftpm_tee_probe,
+	.remove = ftpm_tee_remove,
+};
+
+module_platform_driver(ftpm_tee_driver);
+
+MODULE_AUTHOR("Thirupathaiah Annapureddy <thiruan@microsoft.com>");
+MODULE_DESCRIPTION("TPM Driver for fTPM TA in TEE");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/char/tpm/tpm_ftpm_tee.h b/drivers/char/tpm/tpm_ftpm_tee.h
new file mode 100644
index 000000000000..c1dd416d27c9
--- /dev/null
+++ b/drivers/char/tpm/tpm_ftpm_tee.h
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) Microsoft Corporation
+ */
+
+#ifndef __TPM_FTPM_TEE_H__
+#define __TPM_FTPM_TEE_H__
+
+#include <linux/tee_drv.h>
+#include <linux/uuid.h>
+#include <linux/tpm.h>
+
+/* The TAFs ID implemented in this TA */
+#define FTPM_OPTEE_TA_SUBMIT_COMMAND  (0)
+#define FTPM_OPTEE_TA_EMULATE_PPI     (1)
+
+/* max. buffer size supported by fTPM  */
+#define  MAX_COMMAND_SIZE       4096
+#define  MAX_RESPONSE_SIZE      4096
+
+/**
+ * struct ftpm_tee_private - fTPM's private data
+ * @chip:     struct tpm_chip instance registered with tpm framework.
+ * @state:    internal state
+ * @session:  fTPM TA session identifier.
+ * @resp_len: cached response buffer length.
+ * @resp_buf: cached response buffer.
+ * @ctx:      TEE context handler.
+ * @shm:      Memory pool shared with fTPM TA in TEE.
+ */
+struct ftpm_tee_private {
+	struct tpm_chip *chip;
+	u32 session;
+	size_t resp_len;
+	u8 resp_buf[MAX_RESPONSE_SIZE];
+	struct tee_context *ctx;
+	struct tee_shm *shm;
+};
+
+/*
+ * Note: ftpm_tee_tpm_op_recv and ftpm_tee_tpm_op_send are called from the
+ * same routine tpm_try_transmit in tpm-interface.c. These calls are protected
+ * by chip->tpm_mutex => There is no need for protecting any data shared
+ * between these routines ex: struct ftpm_tee_private
+ */
+
+/* TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896 */
+static const uuid_t ftpm_ta_uuid =
+	UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
+		  0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
+
+#endif /* __TPM_FTPM_TEE_H__ */
-- 
2.19.1

[PATCH v2 3/3] ftpm: add documentation for ftpm driver

[Sasha Levin]

This patch adds basic documentation to describe the new fTPM driver.

Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 Documentation/security/tpm/index.rst        |  1 +
 Documentation/security/tpm/tpm_ftpm_tee.rst | 31 +++++++++++++++++++++
 2 files changed, 32 insertions(+)
 create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst

diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst
index af77a7bbb070..15783668644f 100644
--- a/Documentation/security/tpm/index.rst
+++ b/Documentation/security/tpm/index.rst
@@ -4,4 +4,5 @@ Trusted Platform Module documentation
 
 .. toctree::
 
+   tpm_ftpm_tee
    tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst b/Documentation/security/tpm/tpm_ftpm_tee.rst
new file mode 100644
index 000000000000..3f19b12d9c57
--- /dev/null
+++ b/Documentation/security/tpm/tpm_ftpm_tee.rst
@@ -0,0 +1,31 @@
+=============================================
+Firmware TPM Driver
+=============================================
+
+| Authors:
+| Thirupathaiah Annapureddy <thiruan@microsoft.com>
+| Sasha Levin <sashal@kernel.org>
+
+This document describes the firmware Trusted Platform Module (fTPM)
+device driver.
+
+Introduction
+============
+
+This driver is a shim for a firmware implemented in ARM's TrustZone
+environment. The driver allows programs to interact with the TPM in the same
+way the would interact with a hardware TPM.
+
+Design
+======
+
+The driver acts as a thin layer that passes commands to and from a TPM
+implemented in firmware. The driver itself doesn't contain much logic and is
+used more like a dumb pipe between firmware and kernel/userspace.
+
+The firmware itself is based on the following paper:
+https://www.microsoft.com/en-us/research/wp-content/uploads/2017/06/ftpm1.pdf
+
+When the driver is loaded it will expose ``/dev/tpmX`` character devices to
+userspace which will enable userspace to communicate with the firmware tpm
+through this device.
-- 
2.19.1

Re: [PATCH v2 2/3] ftpm: firmware TPM running in TEE

[Jason Gunthorpe]

On Tue, Apr 09, 2019 at 02:49:57PM -0400, Sasha Levin wrote:

> +/*
> + * Undo what has been done in ftpm_tee_probe
> + */
> +static void ftpm_tee_deinit(struct ftpm_tee_private *pvt_data)
> +{
> +	/* Release the chip */
> +	tpm_chip_unregister(pvt_data->chip);
> +
> +	/* frees chip */
> +	if (pvt_data->chip)
> +		put_device(&pvt_data->chip->dev);
> +
> +	if (pvt_data->ctx) {
> +		/* Free the shared memory pool */
> +		tee_shm_free(pvt_data->shm);
> +
> +		/* close the existing session with fTPM TA*/
> +		tee_client_close_session(pvt_data->ctx, pvt_data->session);
> +
> +		/* close the context with TEE driver */
> +		tee_client_close_context(pvt_data->ctx);
> +	}

None of these if's are necessary, remove is only called if probe
succeeds. Would also make more sense to put this code into remove
instead of having it call one function..

> diff --git a/drivers/char/tpm/tpm_ftpm_tee.h b/drivers/char/tpm/tpm_ftpm_tee.h
> new file mode 100644
> index 000000000000..c1dd416d27c9
> +++ b/drivers/char/tpm/tpm_ftpm_tee.h
> @@ -0,0 +1,52 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation
> + */
> +
> +#ifndef __TPM_FTPM_TEE_H__
> +#define __TPM_FTPM_TEE_H__
> +
> +#include <linux/tee_drv.h>
> +#include <linux/uuid.h>
> +#include <linux/tpm.h>
> +
> +/* The TAFs ID implemented in this TA */
> +#define FTPM_OPTEE_TA_SUBMIT_COMMAND  (0)
> +#define FTPM_OPTEE_TA_EMULATE_PPI     (1)
> +
> +/* max. buffer size supported by fTPM  */
> +#define  MAX_COMMAND_SIZE       4096
> +#define  MAX_RESPONSE_SIZE      4096
> +
> +/**
> + * struct ftpm_tee_private - fTPM's private data
> + * @chip:     struct tpm_chip instance registered with tpm framework.
> + * @state:    internal state
> + * @session:  fTPM TA session identifier.
> + * @resp_len: cached response buffer length.
> + * @resp_buf: cached response buffer.
> + * @ctx:      TEE context handler.
> + * @shm:      Memory pool shared with fTPM TA in TEE.
> + */
> +struct ftpm_tee_private {
> +	struct tpm_chip *chip;
> +	u32 session;
> +	size_t resp_len;
> +	u8 resp_buf[MAX_RESPONSE_SIZE];
> +	struct tee_context *ctx;
> +	struct tee_shm *shm;
> +};
> +
> +/*
> + * Note: ftpm_tee_tpm_op_recv and ftpm_tee_tpm_op_send are called from the
> + * same routine tpm_try_transmit in tpm-interface.c. These calls are protected
> + * by chip->tpm_mutex => There is no need for protecting any data shared
> + * between these routines ex: struct ftpm_tee_private
> + */
> +
> +/* TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896 */
> +static const uuid_t ftpm_ta_uuid =
> +	UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
> +		  0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);

Don't put static variables in header files

Jason

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Rob Herring]

On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
>
> The parameters are similar to the ones used by IBM's vTPM and the
> various I2C tpm drivers.

Bindings describe h/w (or firmware interfaces in this case), not drivers.

>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
>  2 files changed, 14 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> new file mode 100644
> index 000000000000..20fca67a56c4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> @@ -0,0 +1,13 @@
> +Required properties:
> +- compatible: should be "microsoft,ftpm"
> +- linux,sml-base: 64-bit base address of the reserved memory allocated
> +                 for the firmware event log
> +- linux,sml-size: size of the memory allocated for the firmware event log

Firmware is defining linux specific properties? What if I want to run
BSD? We should use 'reg' here instead.

What memory is used here? This should be under /reserved-memory if it
is part of "main" memory.

Really, I'd prefer to not see this in DT at all. Make the firmware
discoverable. Why repeat the mistakes of non-discoverable h/w in s/w
interfaces? OP-Tee at least has defined a mechanism to enumerate TEE
functions IIRC.

Rob

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Sasha Levin]

On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
>On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> The parameters are similar to the ones used by IBM's vTPM and the
>> various I2C tpm drivers.
>
>Bindings describe h/w (or firmware interfaces in this case), not drivers.
>
>>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
>>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
>>  2 files changed, 14 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>>
>> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>> new file mode 100644
>> index 000000000000..20fca67a56c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>> @@ -0,0 +1,13 @@
>> +Required properties:
>> +- compatible: should be "microsoft,ftpm"
>> +- linux,sml-base: 64-bit base address of the reserved memory allocated
>> +                 for the firmware event log
>> +- linux,sml-size: size of the memory allocated for the firmware event log
>
>Firmware is defining linux specific properties? What if I want to run
>BSD? We should use 'reg' here instead.

This is based on already existing code that defines these names, see
tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .

These properties were described similarily by other interfaces (see
Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt or
Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt for example).

We could rename them all if you'd like, I was just trying to follow the
existing code.

>What memory is used here? This should be under /reserved-memory if it
>is part of "main" memory.

That's my understanding, yes.

>Really, I'd prefer to not see this in DT at all. Make the firmware
>discoverable. Why repeat the mistakes of non-discoverable h/w in s/w
>interfaces? OP-Tee at least has defined a mechanism to enumerate TEE
>functions IIRC.

Sadly the firmware already exists as-is on live hardware, there is a
paper describing it back from 2016 and we're stuck having to support
that.

--
Thanks,
Sasha

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Rob Herring]

On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote:
>
> On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
> >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
> >>
> >> The parameters are similar to the ones used by IBM's vTPM and the
> >> various I2C tpm drivers.
> >
> >Bindings describe h/w (or firmware interfaces in this case), not drivers.
> >
> >>
> >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> >> ---
> >>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
> >>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
> >>  2 files changed, 14 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> >> new file mode 100644
> >> index 000000000000..20fca67a56c4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> >> @@ -0,0 +1,13 @@
> >> +Required properties:
> >> +- compatible: should be "microsoft,ftpm"
> >> +- linux,sml-base: 64-bit base address of the reserved memory allocated
> >> +                 for the firmware event log
> >> +- linux,sml-size: size of the memory allocated for the firmware event log
> >
> >Firmware is defining linux specific properties? What if I want to run
> >BSD? We should use 'reg' here instead.
>
> This is based on already existing code that defines these names, see
> tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .

BTW, that probably needs updating to handle endianness correctly.

> These properties were described similarily by other interfaces (see
> Documentation/devicetree/bindings/security/tpm/ibmvtpm.txt or
> Documentation/devicetree/bindings/security/tpm/tpm-i2c.txt for example).
>
> We could rename them all if you'd like, I was just trying to follow the
> existing code.
>
> >What memory is used here? This should be under /reserved-memory if it
> >is part of "main" memory.
>
> That's my understanding, yes.
>
> >Really, I'd prefer to not see this in DT at all. Make the firmware
> >discoverable. Why repeat the mistakes of non-discoverable h/w in s/w
> >interfaces? OP-Tee at least has defined a mechanism to enumerate TEE
> >functions IIRC.
>
> Sadly the firmware already exists as-is on live hardware, there is a
> paper describing it back from 2016 and we're stuck having to support
> that.

Does the firmware depend on this binding or the DT just gets
statically populated with the address/size the firmware hardcodes? I'd
prefer not to propagate the IBM binding that we were kind of stuck
with. I had similar comments on it...

Are there other parts of the TEE firmware that need to be described in DT?

Rob

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Jason Gunthorpe]

On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote:
> On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote:
> >
> > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
> > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
> > >>
> > >> The parameters are similar to the ones used by IBM's vTPM and the
> > >> various I2C tpm drivers.
> > >
> > >Bindings describe h/w (or firmware interfaces in this case), not drivers.
> > >
> > >>
> > >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> > >>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
> > >>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
> > >>  2 files changed, 14 insertions(+)
> > >>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > >>
> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > >> new file mode 100644
> > >> index 000000000000..20fca67a56c4
> > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > >> @@ -0,0 +1,13 @@
> > >> +Required properties:
> > >> +- compatible: should be "microsoft,ftpm"
> > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated
> > >> +                 for the firmware event log
> > >> +- linux,sml-size: size of the memory allocated for the firmware event log
> > >
> > >Firmware is defining linux specific properties? What if I want to run
> > >BSD? We should use 'reg' here instead.
> >
> > This is based on already existing code that defines these names, see
> > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .
> 
> BTW, that probably needs updating to handle endianness correctly.

IIRC this legacy IBM code has broken endianness in the firmware..

All that stuff in read_log_of, and the related DT stuff, is historical
IBM special case-ness and should not be copied into new things.

Jason

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Sasha Levin]

On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote:
>On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote:
>> On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote:
>> >
>> > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
>> > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
>> > >>
>> > >> The parameters are similar to the ones used by IBM's vTPM and the
>> > >> various I2C tpm drivers.
>> > >
>> > >Bindings describe h/w (or firmware interfaces in this case), not drivers.
>> > >
>> > >>
>> > >> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> > >>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
>> > >>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
>> > >>  2 files changed, 14 insertions(+)
>> > >>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>> > >>
>> > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>> > >> new file mode 100644
>> > >> index 000000000000..20fca67a56c4
>> > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
>> > >> @@ -0,0 +1,13 @@
>> > >> +Required properties:
>> > >> +- compatible: should be "microsoft,ftpm"
>> > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated
>> > >> +                 for the firmware event log
>> > >> +- linux,sml-size: size of the memory allocated for the firmware event log
>> > >
>> > >Firmware is defining linux specific properties? What if I want to run
>> > >BSD? We should use 'reg' here instead.
>> >
>> > This is based on already existing code that defines these names, see
>> > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .
>>
>> BTW, that probably needs updating to handle endianness correctly.
>
>IIRC this legacy IBM code has broken endianness in the firmware..
>
>All that stuff in read_log_of, and the related DT stuff, is historical
>IBM special case-ness and should not be copied into new things.

The fTPM driver does not use it on it's own, so I guess I can just drop
this patch then?

--
Thanks,
Sasha

Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Jason Gunthorpe]

On Wed, Apr 10, 2019 at 01:53:59PM -0400, Sasha Levin wrote:
> On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote:
> > On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote:
> > > On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
> > > > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org> wrote:
> > > > >>
> > > > >> The parameters are similar to the ones used by IBM's vTPM and the
> > > > >> various I2C tpm drivers.
> > > > >
> > > > >Bindings describe h/w (or firmware interfaces in this case), not drivers.
> > > > >
> > > > >>
> > > > >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> > > > >>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13 +++++++++++++
> > > > >>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
> > > > >>  2 files changed, 14 insertions(+)
> > > > >>  create mode 100644 Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > >>
> > > > >> diff --git a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > >> new file mode 100644
> > > > >> index 000000000000..20fca67a56c4
> > > > >> +++ b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > >> @@ -0,0 +1,13 @@
> > > > >> +Required properties:
> > > > >> +- compatible: should be "microsoft,ftpm"
> > > > >> +- linux,sml-base: 64-bit base address of the reserved memory allocated
> > > > >> +                 for the firmware event log
> > > > >> +- linux,sml-size: size of the memory allocated for the firmware event log
> > > > >
> > > > >Firmware is defining linux specific properties? What if I want to run
> > > > >BSD? We should use 'reg' here instead.
> > > >
> > > > This is based on already existing code that defines these names, see
> > > > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .
> > > 
> > > BTW, that probably needs updating to handle endianness correctly.
> > 
> > IIRC this legacy IBM code has broken endianness in the firmware..
> > 
> > All that stuff in read_log_of, and the related DT stuff, is historical
> > IBM special case-ness and should not be copied into new things.
> 
> The fTPM driver does not use it on it's own, so I guess I can just drop
> this patch then?

If your ARM system boots via EFI then it should pass the firmware
event log through EFI mechanisms, IIRC.

Otherwise maybe you could make a case for using this, but only if the
old IBM stuff is perfectly emulated, bugs and all.

Do you even have an event log?

Jason

RE: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for fTPM driver

[Thirupathaiah Annapureddy]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, April 10, 2019 10:57 AM
> To: Sasha Levin <sashal@kernel.org>
> Cc: Rob Herring <robh+dt@kernel.org>; Peter Huewe <peterhuewe@gmx.de>;
> Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; Mark Rutland
> <mark.rutland@arm.com>; Jonathan Corbet <corbet@lwn.net>; Arnd Bergmann
> <arnd@arndb.de>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; linux-
> kernel@vger.kernel.org; Linux Doc Mailing List <linux-doc@vger.kernel.org>;
> linux-integrity@vger.kernel.org; Microsoft Linux Kernel List <linux-
> kernel@microsoft.com>; Thirupathaiah Annapureddy <thiruan@microsoft.com>;
> Bryan Kelly (CSI) <bryankel@microsoft.com>
> Subject: Re: [PATCH v2 1/3] ftpm: dt-binding: add dts documentation for
> fTPM driver
> 
> On Wed, Apr 10, 2019 at 01:53:59PM -0400, Sasha Levin wrote:
> > On Wed, Apr 10, 2019 at 02:03:16PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Apr 10, 2019 at 12:01:37PM -0500, Rob Herring wrote:
> > > > On Wed, Apr 10, 2019 at 11:19 AM Sasha Levin <sashal@kernel.org>
> wrote:
> > > > >
> > > > > On Tue, Apr 09, 2019 at 04:18:29PM -0500, Rob Herring wrote:
> > > > > >On Tue, Apr 9, 2019 at 1:50 PM Sasha Levin <sashal@kernel.org>
> wrote:
> > > > > >>
> > > > > >> The parameters are similar to the ones used by IBM's vTPM and
> the
> > > > > >> various I2C tpm drivers.
> > > > > >
> > > > > >Bindings describe h/w (or firmware interfaces in this case), not
> drivers.
> > > > > >
> > > > > >>
> > > > > >> Signed-off-by: Sasha Levin <sashal@kernel.org>
> > > > > >>  .../bindings/security/tpm/tpm_ftpm_tee.txt          | 13
> +++++++++++++
> > > > > >>  .../devicetree/bindings/vendor-prefixes.txt         |  1 +
> > > > > >>  2 files changed, 14 insertions(+)
> > > > > >>  create mode 100644
> Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > > >>
> > > > > >> diff --git
> a/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > > >> new file mode 100644
> > > > > >> index 000000000000..20fca67a56c4
> > > > > >> +++
> b/Documentation/devicetree/bindings/security/tpm/tpm_ftpm_tee.txt
> > > > > >> @@ -0,0 +1,13 @@
> > > > > >> +Required properties:
> > > > > >> +- compatible: should be "microsoft,ftpm"
> > > > > >> +- linux,sml-base: 64-bit base address of the reserved memory
> allocated
> > > > > >> +                 for the firmware event log
> > > > > >> +- linux,sml-size: size of the memory allocated for the firmware
> event log
> > > > > >
> > > > > >Firmware is defining linux specific properties? What if I want to
> run
> > > > > >BSD? We should use 'reg' here instead.
> > > > >
> > > > > This is based on already existing code that defines these names,
> see
> > > > > tpm_read_log_of() in drivers/char/tpm/eventlog/of.c .
> > > >
> > > > BTW, that probably needs updating to handle endianness correctly.
> > >
> > > IIRC this legacy IBM code has broken endianness in the firmware..
> > >
> > > All that stuff in read_log_of, and the related DT stuff, is historical
> > > IBM special case-ness and should not be copied into new things.
> >
> > The fTPM driver does not use it on it's own, so I guess I can just drop
> > this patch then?
> 
> If your ARM system boots via EFI then it should pass the firmware
> event log through EFI mechanisms, IIRC.
> 
> Otherwise maybe you could make a case for using this, but only if the
> old IBM stuff is perfectly emulated, bugs and all.
> 
> Do you even have an event log?
> 

Our ARM system is booting using U-boot and Device Tree. 
So the ACPI/EFI table mechanism to pass binary boot measurements won't be applicable. 

We have event logs working and we are able to successfully read them using 
/sys/kernel/security/tpm0/binary_bios_measurements and parse them

--Thiru

> Jason