[PATCH v3 0/2] ftpm: a firmware based TPM driver

[PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sasha Levin]

From: "Sasha Levin (Microsoft)" <sashal@kernel.org>

Changes since v2:

 - Drop the devicetree bindings patch (we don't add any new ones).
 - More code cleanups based on Jason Gunthorpe's review.

Sasha Levin (2):
  ftpm: firmware TPM running in TEE
  ftpm: add documentation for ftpm driver

 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             | 366 ++++++++++++++++++++
 drivers/char/tpm/tpm_ftpm_tee.h             |  47 +++
 6 files changed, 451 insertions(+)
 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 v3 1/2] 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 | 366 ++++++++++++++++++++++++++++++++
 drivers/char/tpm/tpm_ftpm_tee.h |  47 ++++
 4 files changed, 419 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..f33cdfeb5376
--- /dev/null
+++ b/drivers/char/tpm/tpm_ftpm_tee.c
@@ -0,0 +1,366 @@
+// 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"
+
+/* 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);
+
+/*
+ * 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);
+
+	/* Release the chip */
+	tpm_chip_unregister(pvt_data->chip);
+
+	/* frees chip */
+	put_device(&pvt_data->chip->dev);
+
+	/* 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 */
+
+	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..9de513e72dbb
--- /dev/null
+++ b/drivers/char/tpm/tpm_ftpm_tee.h
@@ -0,0 +1,47 @@
+// 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
+ */
+
+#endif /* __TPM_FTPM_TEE_H__ */
-- 
2.19.1

Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE

[Jarkko Sakkinen]

On Mon, Apr 15, 2019 at 11:56:35AM -0400, Sasha Levin wrote:
> 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

The commit message should include at least a brief description what TEE
is.

> 
> 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 | 366 ++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_ftpm_tee.h |  47 ++++
>  4 files changed, 419 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..f33cdfeb5376
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) Microsoft Corporation
> + */

There should be at least some description what kind of implementation
this is and something about TEE.

> +
> +#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"
> +
> +/* 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);

Just wondering why prefixes are here in different order in the comment
and code.

> +
> +/*
> + * 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
> + */

This documentation block should be removed. It could be in all drivers
and thus this documentation belongs outside of specific HW drivers.

> +
> +/**
> + * 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.
> + */

This should be modified to follow kdoc [1]. It is inconsistent with the
other kdoc comments we have. Now it is all wrong and contains redundant
information. It should be something like

/**
 * ftpm_tee_tpm_op_recv() - retrieve a response from fTPM
 * @chip:	TPM chip associated with the fTPM
 * @buf:	buffer for the received data
 * @count:	number of bytes to read
 *
 * Copy the response from fTPM's internal buffer to the buffer provided
 * by the caller.
 *
 * Return:
 *   0 on success,
 *   -errno on error
 */

> +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.
> + */

This should be modified to follow kdoc [1]. It is inconsistent with the
other kdoc comments we have.


> +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);
> +
> +	/* Release the chip */
> +	tpm_chip_unregister(pvt_data->chip);
> +
> +	/* frees chip */
> +	put_device(&pvt_data->chip->dev);
> +
> +	/* 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 */
> +
> +	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..9de513e72dbb
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.h
> @@ -0,0 +1,47 @@
> +// 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
> + */

This comment should be removed.

> +
> +#endif /* __TPM_FTPM_TEE_H__ */
> -- 
> 2.19.1
> 

[1] https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

/Jarkko

Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE

[Sasha Levin]

On Wed, May 15, 2019 at 11:12:50AM +0300, Jarkko Sakkinen wrote:
>On Mon, Apr 15, 2019 at 11:56:35AM -0400, Sasha Levin wrote:
>> 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
>
>The commit message should include at least a brief description what TEE
>is.

The whole TEE subsystem is already well documented in our kernel tree
(https://www.kernel.org/doc/Documentation/tee.txt) and beyond. I can add
a reference to the doc here, but I'd rather not add a bunch of TEE
related comments as you suggest later in your review.

The same way a PCI device driver doesn't describe what PCI is in it's
code, we shouldn't be doing the same for TEE here.

>> +
>> +#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"
>> +
>> +/* 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);
>
>Just wondering why prefixes are here in different order in the comment
>and code.

No prefixes, this is a completely randomly generated UUID.

I'll address the rest of your comments in the next ver.

--
Thanks,
Sasha

Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE

[Jarkko Sakkinen]

On Fri, May 17, 2019 at 09:22:26AM -0400, Sasha Levin wrote:
> The whole TEE subsystem is already well documented in our kernel tree
> (https://www.kernel.org/doc/Documentation/tee.txt) and beyond. I can add
> a reference to the doc here, but I'd rather not add a bunch of TEE
> related comments as you suggest later in your review.
> 
> The same way a PCI device driver doesn't describe what PCI is in it's
> code, we shouldn't be doing the same for TEE here.

Thanks for pointing out the documentation! That is sufficient.

/Jarkko

Re: [PATCH v3 1/2] ftpm: firmware TPM running in TEE

[Igor Opaniuk]

Hi Sasha,

Just my two cents here after a quick look:

On Mon, Apr 15, 2019 at 6:58 PM Sasha Levin <sashal@kernel.org> wrote:
>
> 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 | 366 ++++++++++++++++++++++++++++++++
>  drivers/char/tpm/tpm_ftpm_tee.h |  47 ++++
>  4 files changed, 419 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

depends on OPTEE also.

> +       ---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..f33cdfeb5376
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -0,0 +1,366 @@
> +// 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>
minor: alphabetical order (simplify detecting duplicate include files etc)

> +
> +#include "tpm.h"
> +#include "tpm_ftpm_tee.h"
> +
> +#define DRIVER_NAME "ftpm-tee"
> +
Please add a comment about TA UUID (what is it etc.), and where it is
defined in the ms-tpm-ta repository.

> +/* 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);
> +
> +/*
> + * 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 */
> +}
Check my patch 4f062dc1b75 ("tee: add cancellation support to client
interface"), with this API you can easily implement cancellation using
tee_client_cancel_req(...).
Within MS fwTPM TA implementation it also can be implemented by
checking the cancellation flag calling GP API
TEE_GetCancellationFlag() (AFAIK this should be done in
_plat__IsCanceled(void) wrapper function).

> +
> +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
> +        */
This can be done implicitly by providing tee_ioctl_version_data struct
with proper values as the last param when the context is opened
(tee_client_open_context() invocation).
Something like this:

struct tee_ioctl_version_data vers = {
      .impl_id = TEE_OPTEE_CAP_TZ,
      .impl_caps = TEE_IMPL_ID_OPTEE,
      .gen_caps = TEE_GEN_CAP_GP,
};

And then:
tee_client_open_context(NULL, ftpm_tee_match, NULL, &vers);

> +       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);
Check my comments above about tee_ioctl_version_data struct.

> +       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);
Why not to keep stuff in two different shm buffers instead (one for
cmd buffer, another for response)?
It will definitely simplify the implementation from both fwtpm driver
and TA sides.

> +       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);
> +
> +       /* Release the chip */
> +       tpm_chip_unregister(pvt_data->chip);
> +
> +       /* frees chip */
> +       put_device(&pvt_data->chip->dev);
> +
> +       /* 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 */
> +
> +       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..9de513e72dbb
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_ftpm_tee.h
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
... header files, instead, use traditional (/* */) comments for
reasons related to tooling [1]
/* 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>
minor: alphabetical order of includes?

> +
> +/* 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
> + */
> +
> +#endif /* __TPM_FTPM_TEE_H__ */
> --
> 2.19.1
>

And also a question, a while ago I implemented a similar fTPM driver
in U-boot and managed to get it working with MS TPM2.0 TA (integrated
into OP-TEE blob as Early TA) ,and eMMC RPMB was used under the hood
as a secure storage.
This also needed some minor changes in the TA.

Let me know if you're planning to introduce/or generally interested in
having a similar driver in U-boot mainline (I think it's better to
have a driver synced with the one in the Linux mainline, just in a
sake of avoiding possible code divergence), if there are no such plans
- I'll send on my own patch-series based on this driver ASAP when it's
accepted.

[1] https://lwn.net/Articles/739183/

Thanks!
-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk

mailto: igor.opaniuk@gmail.com
skype: igor.opanyuk
+380 (93) 836 40 67
http://ua.linkedin.com/in/iopaniuk

[PATCH v3 2/2] 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>
Signed-off-by: Sasha Levin (Microsoft) <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..29c2f8b5ed10
--- /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 v3 2/2] ftpm: add documentation for ftpm driver

[Enrico Weigelt, metux IT consult]

On 15.04.19 17:56, Sasha Levin wrote:

Hi,

> +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.
Is that TPM already used in production or yet an PoC ?
IOW: can the protocol be changed ?

If so, I'd prefer using 9P for that. This already proven well, not just
for grid computing (where it originally came from), but also in things
like virtio, etc.

In general, many of the hardware/chip interfaces out there basically
deal with either either passing around some data packets or streams,
or reading/setting some attributes. But everybody seems to do that part
in his own special way - that takes up a big share of the driver
development resources and final code - and that needs to be repeated
for each OS. In many, many cases a standard protocol like 9P could
already provide this - if folks would just use it :p

Therefore, I'm really a strong supporter of the idea of using 9P
for this.

In your case, you could design the highlevel TPM interface like with
a tcp stream / socket or a synthetic filesystem, and for the lowlevel
part just like kvm does w/ virtio.

In case you have no experience w/ 9P+friends, feel free to ask,
I'll to my best to explain it :)


--mtx

-- 
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

Re: [PATCH v3 2/2] ftpm: add documentation for ftpm driver

[Sasha Levin]

On Wed, Apr 17, 2019 at 02:23:13PM +0200, Enrico Weigelt, metux IT consult wrote:
>On 15.04.19 17:56, Sasha Levin wrote:
>
>Hi,
>
>> +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.
>Is that TPM already used in production or yet an PoC ?
>IOW: can the protocol be changed ?

Sadly no, this is based on something that exists for a few years already
and we're trying to make Linux run on it.

--
Thanks,
Sasha

Re: [PATCH v3 2/2] ftpm: add documentation for ftpm driver

[Jarkko Sakkinen]

On Mon, Apr 15, 2019 at 11:56:36AM -0400, Sasha Levin wrote:
> This patch adds basic documentation to describe the new fTPM driver.
> 
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> Signed-off-by: Sasha Levin (Microsoft) <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..29c2f8b5ed10
> --- /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
> 

Actually this would a better place at least with some words to describe
what is TEE. I'm, for example, confused whether there is only single TEE
in existence always used with TZ or is this some MS specific TEE.

Otherwise, looks legit.

/Jarkko

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sasha Levin]

On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
>From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
>
>Changes since v2:
>
> - Drop the devicetree bindings patch (we don't add any new ones).
> - More code cleanups based on Jason Gunthorpe's review.
>
>Sasha Levin (2):
>  ftpm: firmware TPM running in TEE
>  ftpm: add documentation for ftpm driver

Ping? Does anyone have any objections to this?

--
Thanks,
Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

+ TEE ML

Hi Sasha,

Firstly apologies for my comments here as I recently joined
linux-integrity ML so I don't have other patches in my inbox. Also, it
would be nice if you could cc TEE ML in future patches, so that people
are aware of such interesting use-cases and may provide some feedback.

On Tue, 7 May 2019 at 23:10, Sasha Levin <sashal@kernel.org> wrote:
>
> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> >From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> >
> >Changes since v2:
> >
> > - Drop the devicetree bindings patch (we don't add any new ones).
> > - More code cleanups based on Jason Gunthorpe's review.
> >
> >Sasha Levin (2):
> >  ftpm: firmware TPM running in TEE
> >  ftpm: add documentation for ftpm driver
>
> Ping? Does anyone have any objections to this?
>

From [PATCH v3 1/2] ftpm: firmware TPM running in TEE:

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

Here this fTPM driver (seems to communicate with OP-TEE based TA)
should register on TEE bus [1] rather than platform bus as its actual
dependency is on TEE driver rather than using deferred probe to meet
its dependency. Have a look at OP-TEE based RNG driver here [2].

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0fc1db9d105915021260eb241661b8e96f5c0f1a
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5fe8b1cc6a03c46b3061e808256d39dcebd0d0f0

-Sumit

> --
> Thanks,
> Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Daniel Thompson]

On Wed, May 08, 2019 at 10:11:54AM +0530, Sumit Garg wrote:
> + TEE ML
> 
> Hi Sasha,
> 
> Firstly apologies for my comments here as I recently joined
> linux-integrity ML so I don't have other patches in my inbox. Also, it
> would be nice if you could cc TEE ML in future patches, so that people
> are aware of such interesting use-cases and may provide some feedback.

If this kind is desire exists then shouldn't it be captured in
MAINTAINERS?


Daniel.

> 
> On Tue, 7 May 2019 at 23:10, Sasha Levin <sashal@kernel.org> wrote:
> >
> > On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > >From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > >
> > >Changes since v2:
> > >
> > > - Drop the devicetree bindings patch (we don't add any new ones).
> > > - More code cleanups based on Jason Gunthorpe's review.
> > >
> > >Sasha Levin (2):
> > >  ftpm: firmware TPM running in TEE
> > >  ftpm: add documentation for ftpm driver
> >
> > Ping? Does anyone have any objections to this?
> >
> 
> From [PATCH v3 1/2] ftpm: firmware TPM running in TEE:
> 
> > +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);
> 
> Here this fTPM driver (seems to communicate with OP-TEE based TA)
> should register on TEE bus [1] rather than platform bus as its actual
> dependency is on TEE driver rather than using deferred probe to meet
> its dependency. Have a look at OP-TEE based RNG driver here [2].
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0fc1db9d105915021260eb241661b8e96f5c0f1a
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5fe8b1cc6a03c46b3061e808256d39dcebd0d0f0
> 
> -Sumit
> 
> > --
> > Thanks,
> > Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

On Wed, 8 May 2019 at 13:32, Daniel Thompson <daniel.thompson@linaro.org> wrote:
>
> On Wed, May 08, 2019 at 10:11:54AM +0530, Sumit Garg wrote:
> > + TEE ML
> >
> > Hi Sasha,
> >
> > Firstly apologies for my comments here as I recently joined
> > linux-integrity ML so I don't have other patches in my inbox. Also, it
> > would be nice if you could cc TEE ML in future patches, so that people
> > are aware of such interesting use-cases and may provide some feedback.
>
> If this kind is desire exists then shouldn't it be captured in
> MAINTAINERS?
>

Makes sense, will send a patch to capture it in MAINTAINERS file.

-Sumit

>
> Daniel.
>
> >
> > On Tue, 7 May 2019 at 23:10, Sasha Levin <sashal@kernel.org> wrote:
> > >
> > > On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > >From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > >
> > > >Changes since v2:
> > > >
> > > > - Drop the devicetree bindings patch (we don't add any new ones).
> > > > - More code cleanups based on Jason Gunthorpe's review.
> > > >
> > > >Sasha Levin (2):
> > > >  ftpm: firmware TPM running in TEE
> > > >  ftpm: add documentation for ftpm driver
> > >
> > > Ping? Does anyone have any objections to this?
> > >
> >
> > From [PATCH v3 1/2] ftpm: firmware TPM running in TEE:
> >
> > > +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);
> >
> > Here this fTPM driver (seems to communicate with OP-TEE based TA)
> > should register on TEE bus [1] rather than platform bus as its actual
> > dependency is on TEE driver rather than using deferred probe to meet
> > its dependency. Have a look at OP-TEE based RNG driver here [2].
> >
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0fc1db9d105915021260eb241661b8e96f5c0f1a
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5fe8b1cc6a03c46b3061e808256d39dcebd0d0f0
> >
> > -Sumit
> >
> > > --
> > > Thanks,
> > > Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Jarkko Sakkinen]

On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > 
> > Changes since v2:
> > 
> > - Drop the devicetree bindings patch (we don't add any new ones).
> > - More code cleanups based on Jason Gunthorpe's review.
> > 
> > Sasha Levin (2):
> >  ftpm: firmware TPM running in TEE
> >  ftpm: add documentation for ftpm driver
> 
> Ping? Does anyone have any objections to this?

Sorry I've been on vacation week before last week and last week
I was extremely busy because I had been on vacation. This in
my TODO list. Will look into it tomorrow in detail.

Apologies for the delay with this!

/Jarkko

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sasha Levin]

On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
>On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
>> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
>> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
>> >
>> > Changes since v2:
>> >
>> > - Drop the devicetree bindings patch (we don't add any new ones).
>> > - More code cleanups based on Jason Gunthorpe's review.
>> >
>> > Sasha Levin (2):
>> >  ftpm: firmware TPM running in TEE
>> >  ftpm: add documentation for ftpm driver
>>
>> Ping? Does anyone have any objections to this?
>
>Sorry I've been on vacation week before last week and last week
>I was extremely busy because I had been on vacation. This in
>my TODO list. Will look into it tomorrow in detail.
>
>Apologies for the delay with this!

Hi Jarkko,

If there aren't any big objections to this, can we get it merged in?
We'll be happy to address any comments that come up.

--
Thanks,
Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
>
> On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> >> >
> >> > Changes since v2:
> >> >
> >> > - Drop the devicetree bindings patch (we don't add any new ones).
> >> > - More code cleanups based on Jason Gunthorpe's review.
> >> >
> >> > Sasha Levin (2):
> >> >  ftpm: firmware TPM running in TEE
> >> >  ftpm: add documentation for ftpm driver
> >>
> >> Ping? Does anyone have any objections to this?
> >
> >Sorry I've been on vacation week before last week and last week
> >I was extremely busy because I had been on vacation. This in
> >my TODO list. Will look into it tomorrow in detail.
> >
> >Apologies for the delay with this!
>
> Hi Jarkko,
>
> If there aren't any big objections to this, can we get it merged in?
> We'll be happy to address any comments that come up.

I guess you have missed or ignored this comment [1]. Please address it.

[1] https://lkml.org/lkml/2019/5/8/11

-Sumit

>
> --
> Thanks,
> Sasha

RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Thirupathaiah Annapureddy]

> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Tuesday, May 14, 2019 7:02 PM
> To: Sasha Levin <sashal@kernel.org>
> Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de;
> jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> 
> On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> >
> > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > >> >
> > >> > Changes since v2:
> > >> >
> > >> > - Drop the devicetree bindings patch (we don't add any new ones).
> > >> > - More code cleanups based on Jason Gunthorpe's review.
> > >> >
> > >> > Sasha Levin (2):
> > >> >  ftpm: firmware TPM running in TEE
> > >> >  ftpm: add documentation for ftpm driver
> > >>
> > >> Ping? Does anyone have any objections to this?
> > >
> > >Sorry I've been on vacation week before last week and last week
> > >I was extremely busy because I had been on vacation. This in
> > >my TODO list. Will look into it tomorrow in detail.
> > >
> > >Apologies for the delay with this!
> >
> > Hi Jarkko,
> >
> > If there aren't any big objections to this, can we get it merged in?
> > We'll be happy to address any comments that come up.
> 
> I guess you have missed or ignored this comment [1]. Please address it.
> 
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> 
> -Sumit

Thanks for reviewing and adding comments.

We tried to use TEE bus framework you suggested for fTPM enumeration.
We were not able to pass the TCG Logs collected by the boot loaders.

Currently there are 3 ways to pass TCG Logs based on the code 
in drivers/char/tpm/eventlog:

1. ACPI Table
2. EFI Table
3. OF Device node properties

Our ARM system is booting using U-boot and Device Tree. 
So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
We needed to use OF device node properties to pass TCG2 Logs.
TEE bus enumeration framework does not work for our use case due to the above.

Is it possible to add flexibility in TEE bus enumeration framework to support 
platform specific properties through OF nodes or ACPI?

> 
> >
> > --
> > Thanks,
> > Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
<thiruan@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Tuesday, May 14, 2019 7:02 PM
> > To: Sasha Levin <sashal@kernel.org>
> > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de;
> > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> >
> > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > >
> > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > >> >
> > > >> > Changes since v2:
> > > >> >
> > > >> > - Drop the devicetree bindings patch (we don't add any new ones).
> > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > >> >
> > > >> > Sasha Levin (2):
> > > >> >  ftpm: firmware TPM running in TEE
> > > >> >  ftpm: add documentation for ftpm driver
> > > >>
> > > >> Ping? Does anyone have any objections to this?
> > > >
> > > >Sorry I've been on vacation week before last week and last week
> > > >I was extremely busy because I had been on vacation. This in
> > > >my TODO list. Will look into it tomorrow in detail.
> > > >
> > > >Apologies for the delay with this!
> > >
> > > Hi Jarkko,
> > >
> > > If there aren't any big objections to this, can we get it merged in?
> > > We'll be happy to address any comments that come up.
> >
> > I guess you have missed or ignored this comment [1]. Please address it.
> >
> > [1]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> >
> > -Sumit
>
> Thanks for reviewing and adding comments.
>
> We tried to use TEE bus framework you suggested for fTPM enumeration.
> We were not able to pass the TCG Logs collected by the boot loaders.
>
> Currently there are 3 ways to pass TCG Logs based on the code
> in drivers/char/tpm/eventlog:
>
> 1. ACPI Table
> 2. EFI Table
> 3. OF Device node properties
>
> Our ARM system is booting using U-boot and Device Tree.
> So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> We needed to use OF device node properties to pass TCG2 Logs.
> TEE bus enumeration framework does not work for our use case due to the above.

Firstly let me clarify that this framework is intended to communicate
with TEE based services/devices rather than boot loader. And in this
case fTPM being a TEE based service, so this framework should be used.

>
> Is it possible to add flexibility in TEE bus enumeration framework to support
> platform specific properties through OF nodes or ACPI?
>

As you mentioned above, TCG logs are collected by boot loader. So it
should find a way to pass them to Linux.

How about if boot loader register these TCG logs with fTPM TA which
could be fetched during fTPM driver probe or new api like
tpm_read_log_tee()? This is something similar to what I used in
optee-rng [1] driver to fetch RNG properties.

[1] https://github.com/torvalds/linux/blob/master/drivers/char/hw_random/optee-rng.c#L176

-Sumit

> >
> > >
> > > --
> > > Thanks,
> > > Sasha

RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Thirupathaiah Annapureddy]

> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Thursday, May 16, 2019 12:06 AM
> To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> <bryankel@microsoft.com>
> Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> 
> On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> <thiruan@microsoft.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > To: Sasha Levin <sashal@kernel.org>
> > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> peterhuewe@gmx.de;
> > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> > >
> > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > > >
> > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > >> >
> > > > >> > Changes since v2:
> > > > >> >
> > > > >> > - Drop the devicetree bindings patch (we don't add any new
> ones).
> > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > >> >
> > > > >> > Sasha Levin (2):
> > > > >> >  ftpm: firmware TPM running in TEE
> > > > >> >  ftpm: add documentation for ftpm driver
> > > > >>
> > > > >> Ping? Does anyone have any objections to this?
> > > > >
> > > > >Sorry I've been on vacation week before last week and last week
> > > > >I was extremely busy because I had been on vacation. This in
> > > > >my TODO list. Will look into it tomorrow in detail.
> > > > >
> > > > >Apologies for the delay with this!
> > > >
> > > > Hi Jarkko,
> > > >
> > > > If there aren't any big objections to this, can we get it merged in?
> > > > We'll be happy to address any comments that come up.
> > >
> > > I guess you have missed or ignored this comment [1]. Please address it.
> > >
> > > [1]
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > >
> 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > >
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> > > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> > >
> > > -Sumit
> >
> > Thanks for reviewing and adding comments.
> >
> > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > We were not able to pass the TCG Logs collected by the boot loaders.
> >
> > Currently there are 3 ways to pass TCG Logs based on the code
> > in drivers/char/tpm/eventlog:
> >
> > 1. ACPI Table
> > 2. EFI Table
> > 3. OF Device node properties
> >
> > Our ARM system is booting using U-boot and Device Tree.
> > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > We needed to use OF device node properties to pass TCG2 Logs.
> > TEE bus enumeration framework does not work for our use case due to the
> above.
> 
> Firstly let me clarify that this framework is intended to communicate
> with TEE based services/devices rather than boot loader. And in this
> case fTPM being a TEE based service, so this framework should be used.
> 
It does not work for our use case. We gave enough justification so far. 
TEE bus enumeration needs to be flexible to support our use case and 
more future use cases. 

> >
> > Is it possible to add flexibility in TEE bus enumeration framework to
> support
> > platform specific properties through OF nodes or ACPI?
> >
> 
> As you mentioned above, TCG logs are collected by boot loader. So it
> should find a way to pass them to Linux.
> 
> How about if boot loader register these TCG logs with fTPM TA which
> could be fetched during fTPM driver probe or new api like
> tpm_read_log_tee()? 

And then how does fTPM driver pass TCG Logs to the TPM framework? 
It requires changes to the upstream TPM framework to ask the driver
explicitly for the TCG Logs. 

Note that this also requires changes to the fTPM TA that has been existing for few years.

Is it not possible to add flexibility in TEE bus enumeration framework to
support platform specific properties through OF nodes or ACPI?

Devices enumerated by buses such as i2c can read platform specific properties.
With this flexibility added, more future use cases through TEE bus.


> This is something similar to what I used in
> optee-rng [1] driver to fetch RNG properties.
> 
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> rng.c%23L176&amp;data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> 3006&amp;sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&amp;reserved=
> 0
> 
> -Sumit
> 
> > >
> > > >
> > > > --
> > > > Thanks,
> > > > Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

+ Rob

On Fri, 17 May 2019 at 00:54, Thirupathaiah Annapureddy
<thiruan@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Thursday, May 16, 2019 12:06 AM
> > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > <bryankel@microsoft.com>
> > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> >
> > On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> > <thiruan@microsoft.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > > To: Sasha Levin <sashal@kernel.org>
> > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> > peterhuewe@gmx.de;
> > > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > > kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> > > >
> > > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org> wrote:
> > > > >
> > > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > > >> >
> > > > > >> > Changes since v2:
> > > > > >> >
> > > > > >> > - Drop the devicetree bindings patch (we don't add any new
> > ones).
> > > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > > >> >
> > > > > >> > Sasha Levin (2):
> > > > > >> >  ftpm: firmware TPM running in TEE
> > > > > >> >  ftpm: add documentation for ftpm driver
> > > > > >>
> > > > > >> Ping? Does anyone have any objections to this?
> > > > > >
> > > > > >Sorry I've been on vacation week before last week and last week
> > > > > >I was extremely busy because I had been on vacation. This in
> > > > > >my TODO list. Will look into it tomorrow in detail.
> > > > > >
> > > > > >Apologies for the delay with this!
> > > > >
> > > > > Hi Jarkko,
> > > > >
> > > > > If there aren't any big objections to this, can we get it merged in?
> > > > > We'll be happy to address any comments that come up.
> > > >
> > > > I guess you have missed or ignored this comment [1]. Please address it.
> > > >
> > > > [1]
> > > >
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > > >
> > 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > > >
> > 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> > > > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> > > >
> > > > -Sumit
> > >
> > > Thanks for reviewing and adding comments.
> > >
> > > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > > We were not able to pass the TCG Logs collected by the boot loaders.
> > >
> > > Currently there are 3 ways to pass TCG Logs based on the code
> > > in drivers/char/tpm/eventlog:
> > >
> > > 1. ACPI Table
> > > 2. EFI Table
> > > 3. OF Device node properties
> > >
> > > Our ARM system is booting using U-boot and Device Tree.
> > > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > > We needed to use OF device node properties to pass TCG2 Logs.
> > > TEE bus enumeration framework does not work for our use case due to the
> > above.
> >
> > Firstly let me clarify that this framework is intended to communicate
> > with TEE based services/devices rather than boot loader. And in this
> > case fTPM being a TEE based service, so this framework should be used.
> >
> It does not work for our use case. We gave enough justification so far.
> TEE bus enumeration needs to be flexible to support our use case and
> more future use cases.
>

TEE based services are virtual devices which could be very well be
aware about the platform and device driver could easily query these
devices for platform specific properties. In case of firmware TPM as a
TEE based service, it could easily store the event logs generated
during PCR extend operations which could be fetched at runtime. But a
real TPM device doesn't possess that storage capability leading to
software managing these event logs.

> > >
> > > Is it possible to add flexibility in TEE bus enumeration framework to
> > support
> > > platform specific properties through OF nodes or ACPI?
> > >
> >
> > As you mentioned above, TCG logs are collected by boot loader. So it
> > should find a way to pass them to Linux.
> >
> > How about if boot loader register these TCG logs with fTPM TA which
> > could be fetched during fTPM driver probe or new api like
> > tpm_read_log_tee()?
>
> And then how does fTPM driver pass TCG Logs to the TPM framework?
> It requires changes to the upstream TPM framework to ask the driver
> explicitly for the TCG Logs.

My suggestion was to add 4th way to pass TCG logs as follows:

--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -95,6 +95,10 @@ static int tpm_read_log(struct tpm_chip *chip)
        if (rc != -ENODEV)
                return rc;

+       rc = tpm_read_log_tee(chip);
+       if (rc != -ENODEV)
+               return rc;
+
        return tpm_read_log_of(chip);
 }
--- /dev/null
+++ b/drivers/char/tpm/eventlog/tee.c
@@ -0,0 +1,43 @@
<snip>
+int tpm_read_log_tee(struct tpm_chip *chip)
+{
+       struct tpm_bios_log *log;
+       struct ftpm_tee_private *pvt_data;
+
+       log = &chip->log;
+       if (!strcmp(chip->dev.bus->name, tee_bus_type.name))
+               pvt_data = dev_get_drvdata(chip->dev.parent);
+       else
+               return -ENODEV;
+
+       // Here you could simply do an invoke command to fetch the TCG logs.
+
+       if (chip->flags & TPM_CHIP_FLAG_TPM2)
+               return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
+       return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
+}

>
> Note that this also requires changes to the fTPM TA that has been existing for few years.
>

That's not a sane reason to avoid implementing generic stuff. As there
could be other fTPM implementations and specific DT nodes for each
which might not be maintainable. BTW, in current implementation also I
don't find DT bindings corresponding to DT node used in this
patch-set.

> Is it not possible to add flexibility in TEE bus enumeration framework to
> support platform specific properties through OF nodes or ACPI?
>

TEE bus framework was designed specifically to avoid OF nodes or ACPI
properties. As devices could be intelligent enough to be queried for
required properties.

> Devices enumerated by buses such as i2c can read platform specific properties.

i2c devices are real hardware which could be platform agnostic, so you
need to have platform specific properties.

-Sumit

> With this flexibility added, more future use cases through TEE bus.
>
>
> > This is something similar to what I used in
> > optee-rng [1] driver to fetch RNG properties.
> >
> > [1]
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> > m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> > rng.c%23L176&amp;data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> > 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> > 3006&amp;sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&amp;reserved=
> > 0
> >
> > -Sumit
> >
> > > >
> > > > >
> > > > > --
> > > > > Thanks,
> > > > > Sasha

RE: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Thirupathaiah Annapureddy]

> -----Original Message-----
> From: Sumit Garg <sumit.garg@linaro.org>
> Sent: Thursday, May 16, 2019 11:57 PM
> To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> <bryankel@microsoft.com>; Rob Herring <robh+dt@kernel.org>
> Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> 
> + Rob
> 
> On Fri, 17 May 2019 at 00:54, Thirupathaiah Annapureddy
> <thiruan@microsoft.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Sumit Garg <sumit.garg@linaro.org>
> > > Sent: Thursday, May 16, 2019 12:06 AM
> > > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > > corbet@lwn.net; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>;
> > > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft
> Linux
> > > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > > <bryankel@microsoft.com>
> > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > >
> > > On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> > > <thiruan@microsoft.com> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > > > To: Sasha Levin <sashal@kernel.org>
> > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> > > peterhuewe@gmx.de;
> > > > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > > > kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> > > > >
> > > > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org>
> wrote:
> > > > > >
> > > > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > > > >> >
> > > > > > >> > Changes since v2:
> > > > > > >> >
> > > > > > >> > - Drop the devicetree bindings patch (we don't add any new
> > > ones).
> > > > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > > > >> >
> > > > > > >> > Sasha Levin (2):
> > > > > > >> >  ftpm: firmware TPM running in TEE
> > > > > > >> >  ftpm: add documentation for ftpm driver
> > > > > > >>
> > > > > > >> Ping? Does anyone have any objections to this?
> > > > > > >
> > > > > > >Sorry I've been on vacation week before last week and last week
> > > > > > >I was extremely busy because I had been on vacation. This in
> > > > > > >my TODO list. Will look into it tomorrow in detail.
> > > > > > >
> > > > > > >Apologies for the delay with this!
> > > > > >
> > > > > > Hi Jarkko,
> > > > > >
> > > > > > If there aren't any big objections to this, can we get it merged
> in?
> > > > > > We'll be happy to address any comments that come up.
> > > > >
> > > > > I guess you have missed or ignored this comment [1]. Please address
> it.
> > > > >
> > > > > [1]
> > > > >
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > > > >
> > >
> 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > > > >
> > >
> 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> > > > >
> ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> > > > >
> > > > > -Sumit
> > > >
> > > > Thanks for reviewing and adding comments.
> > > >
> > > > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > > > We were not able to pass the TCG Logs collected by the boot loaders.
> > > >
> > > > Currently there are 3 ways to pass TCG Logs based on the code
> > > > in drivers/char/tpm/eventlog:
> > > >
> > > > 1. ACPI Table
> > > > 2. EFI Table
> > > > 3. OF Device node properties
> > > >
> > > > Our ARM system is booting using U-boot and Device Tree.
> > > > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > > > We needed to use OF device node properties to pass TCG2 Logs.
> > > > TEE bus enumeration framework does not work for our use case due to
> the
> > > above.
> > >
> > > Firstly let me clarify that this framework is intended to communicate
> > > with TEE based services/devices rather than boot loader. And in this
> > > case fTPM being a TEE based service, so this framework should be used.
> > >
> > It does not work for our use case. We gave enough justification so far.
> > TEE bus enumeration needs to be flexible to support our use case and
> > more future use cases.
> >
> 
> TEE based services are virtual devices which could be very well be
> aware about the platform and device driver could easily query these
> devices for platform specific properties. In case of firmware TPM as a
> TEE based service, it could easily store the event logs generated
> during PCR extend operations which could be fetched at runtime. But a
> real TPM device doesn't possess that storage capability leading to
> software managing these event logs.
> 
> > > >
> > > > Is it possible to add flexibility in TEE bus enumeration framework to
> > > support
> > > > platform specific properties through OF nodes or ACPI?
> > > >
> > >
> > > As you mentioned above, TCG logs are collected by boot loader. So it
> > > should find a way to pass them to Linux.
> > >
> > > How about if boot loader register these TCG logs with fTPM TA which
> > > could be fetched during fTPM driver probe or new api like
> > > tpm_read_log_tee()?
> >
> > And then how does fTPM driver pass TCG Logs to the TPM framework?
> > It requires changes to the upstream TPM framework to ask the driver
> > explicitly for the TCG Logs.
> 
> My suggestion was to add 4th way to pass TCG logs as follows:
> 
> --- a/drivers/char/tpm/eventlog/common.c
> +++ b/drivers/char/tpm/eventlog/common.c
> @@ -95,6 +95,10 @@ static int tpm_read_log(struct tpm_chip *chip)
>         if (rc != -ENODEV)
>                 return rc;
> 
> +       rc = tpm_read_log_tee(chip);
> +       if (rc != -ENODEV)
> +               return rc;
> +
>         return tpm_read_log_of(chip);
>  }
> --- /dev/null
> +++ b/drivers/char/tpm/eventlog/tee.c
> @@ -0,0 +1,43 @@
> <snip>
> +int tpm_read_log_tee(struct tpm_chip *chip)
> +{
> +       struct tpm_bios_log *log;
> +       struct ftpm_tee_private *pvt_data;
> +
> +       log = &chip->log;
> +       if (!strcmp(chip->dev.bus->name, tee_bus_type.name))
> +               pvt_data = dev_get_drvdata(chip->dev.parent);
> +       else
> +               return -ENODEV;
> +
> +       // Here you could simply do an invoke command to fetch the TCG
> logs.
> +
> +       if (chip->flags & TPM_CHIP_FLAG_TPM2)
> +               return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> +       return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> +}

But I do not see a need for this 4th method if TEE bus enumeration framework supports flexibility. 
The problem is with the TEE bus enumeration. 

> 
> >
> > Note that this also requires changes to the fTPM TA that has been
> existing for few years.
> >
> 
> That's not a sane reason to avoid implementing generic stuff. 

Exactly, TEE bus enumeration framework need to support generic stuff that is expected
of a bus driver. You are expecting everything else to be generic enough, but not TEE bus enumeration. 

> As there
> could be other fTPM implementations and specific DT nodes for each
> which might not be maintainable. BTW, in current implementation also I
> don't find DT bindings corresponding to DT node used in this
> patch-set.
> 
> > Is it not possible to add flexibility in TEE bus enumeration framework to
> > support platform specific properties through OF nodes or ACPI?
> >
> 
> TEE bus framework was designed specifically to avoid OF nodes or ACPI
> properties. As devices could be intelligent enough to be queried for
> required properties.

Is there an expectation that TEE bus enumerated devices to be intelligent?
If so, this is another limitation of TEE bus enumeration. 
Please fix these limitations to make TEE bus enumeration 
scalable and flexible for future use cases.

> 
> > Devices enumerated by buses such as i2c can read platform specific
> properties.
> 
> i2c devices are real hardware which could be platform agnostic, so you
> need to have platform specific properties.
> 
> -Sumit
> 
> > With this flexibility added, more future use cases through TEE bus.
> >
> >
> > > This is something similar to what I used in
> > > optee-rng [1] driver to fetch RNG properties.
> > >
> > > [1]
> > >
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> > >
> m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> > >
> rng.c%23L176&amp;data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> > >
> 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> > >
> 3006&amp;sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&amp;reserved=
> > > 0
> > >
> > > -Sumit
> > >
> > > > >
> > > > > >
> > > > > > --
> > > > > > Thanks,
> > > > > > Sasha

Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver

[Sumit Garg]

On Fri, 17 May 2019 at 22:53, Thirupathaiah Annapureddy
<thiruan@microsoft.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Sumit Garg <sumit.garg@linaro.org>
> > Sent: Thursday, May 16, 2019 11:57 PM
> > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > corbet@lwn.net; Linux Kernel Mailing List <linux-kernel@vger.kernel.org>;
> > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft Linux
> > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > <bryankel@microsoft.com>; Rob Herring <robh+dt@kernel.org>
> > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> >
> > + Rob
> >
> > On Fri, 17 May 2019 at 00:54, Thirupathaiah Annapureddy
> > <thiruan@microsoft.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > Sent: Thursday, May 16, 2019 12:06 AM
> > > > To: Thirupathaiah Annapureddy <thiruan@microsoft.com>
> > > > Cc: Sasha Levin <sashal@kernel.org>; Jarkko Sakkinen
> > > > <jarkko.sakkinen@linux.intel.com>; peterhuewe@gmx.de; jgg@ziepe.ca;
> > > > corbet@lwn.net; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>;
> > > > linux-doc@vger.kernel.org; linux-integrity@vger.kernel.org; Microsoft
> > Linux
> > > > Kernel List <linux-kernel@microsoft.com>; Bryan Kelly (CSI)
> > > > <bryankel@microsoft.com>
> > > > Subject: Re: [PATCH v3 0/2] ftpm: a firmware based TPM driver
> > > >
> > > > On Thu, 16 May 2019 at 06:30, Thirupathaiah Annapureddy
> > > > <thiruan@microsoft.com> wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Sumit Garg <sumit.garg@linaro.org>
> > > > > > Sent: Tuesday, May 14, 2019 7:02 PM
> > > > > > To: Sasha Levin <sashal@kernel.org>
> > > > > > Cc: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>;
> > > > peterhuewe@gmx.de;
> > > > > > jgg@ziepe.ca; corbet@lwn.net; Linux Kernel Mailing List <linux-
> > > > > > kernel@vger.kernel.org>; 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 v3 0/2] ftpm: a firmware based TPM driver
> > > > > >
> > > > > > On Wed, 15 May 2019 at 01:00, Sasha Levin <sashal@kernel.org>
> > wrote:
> > > > > > >
> > > > > > > On Wed, May 08, 2019 at 03:44:36PM +0300, Jarkko Sakkinen wrote:
> > > > > > > >On Tue, May 07, 2019 at 01:40:20PM -0400, Sasha Levin wrote:
> > > > > > > >> On Mon, Apr 15, 2019 at 11:56:34AM -0400, Sasha Levin wrote:
> > > > > > > >> > From: "Sasha Levin (Microsoft)" <sashal@kernel.org>
> > > > > > > >> >
> > > > > > > >> > Changes since v2:
> > > > > > > >> >
> > > > > > > >> > - Drop the devicetree bindings patch (we don't add any new
> > > > ones).
> > > > > > > >> > - More code cleanups based on Jason Gunthorpe's review.
> > > > > > > >> >
> > > > > > > >> > Sasha Levin (2):
> > > > > > > >> >  ftpm: firmware TPM running in TEE
> > > > > > > >> >  ftpm: add documentation for ftpm driver
> > > > > > > >>
> > > > > > > >> Ping? Does anyone have any objections to this?
> > > > > > > >
> > > > > > > >Sorry I've been on vacation week before last week and last week
> > > > > > > >I was extremely busy because I had been on vacation. This in
> > > > > > > >my TODO list. Will look into it tomorrow in detail.
> > > > > > > >
> > > > > > > >Apologies for the delay with this!
> > > > > > >
> > > > > > > Hi Jarkko,
> > > > > > >
> > > > > > > If there aren't any big objections to this, can we get it merged
> > in?
> > > > > > > We'll be happy to address any comments that come up.
> > > > > >
> > > > > > I guess you have missed or ignored this comment [1]. Please address
> > it.
> > > > > >
> > > > > > [1]
> > > > > >
> > > >
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Flkml.org%
> > > > > >
> > > >
> > 2Flkml%2F2019%2F5%2F8%2F11&amp;data=01%7C01%7Cthiruan%40microsoft.com%7Cf2a
> > > > > >
> > > >
> > 80c7b94434329eaee08d6d8d962b1%7C72f988bf86f141af91ab2d7cd011db47%7C1&amp;sd
> > > > > >
> > ata=hyJRc23NwEFLDuaIMkbSCGetd%2BObQWiAg%2BJtMMR6z9U%3D&amp;reserved=0
> > > > > >
> > > > > > -Sumit
> > > > >
> > > > > Thanks for reviewing and adding comments.
> > > > >
> > > > > We tried to use TEE bus framework you suggested for fTPM enumeration.
> > > > > We were not able to pass the TCG Logs collected by the boot loaders.
> > > > >
> > > > > Currently there are 3 ways to pass TCG Logs based on the code
> > > > > in drivers/char/tpm/eventlog:
> > > > >
> > > > > 1. ACPI Table
> > > > > 2. EFI Table
> > > > > 3. OF Device node properties
> > > > >
> > > > > Our ARM system is booting using U-boot and Device Tree.
> > > > > So ACPI/EFI table mechanism to pass TCG2 logs won't be applicable.
> > > > > We needed to use OF device node properties to pass TCG2 Logs.
> > > > > TEE bus enumeration framework does not work for our use case due to
> > the
> > > > above.
> > > >
> > > > Firstly let me clarify that this framework is intended to communicate
> > > > with TEE based services/devices rather than boot loader. And in this
> > > > case fTPM being a TEE based service, so this framework should be used.
> > > >
> > > It does not work for our use case. We gave enough justification so far.
> > > TEE bus enumeration needs to be flexible to support our use case and
> > > more future use cases.
> > >
> >
> > TEE based services are virtual devices which could be very well be
> > aware about the platform and device driver could easily query these
> > devices for platform specific properties. In case of firmware TPM as a
> > TEE based service, it could easily store the event logs generated
> > during PCR extend operations which could be fetched at runtime. But a
> > real TPM device doesn't possess that storage capability leading to
> > software managing these event logs.
> >
> > > > >
> > > > > Is it possible to add flexibility in TEE bus enumeration framework to
> > > > support
> > > > > platform specific properties through OF nodes or ACPI?
> > > > >
> > > >
> > > > As you mentioned above, TCG logs are collected by boot loader. So it
> > > > should find a way to pass them to Linux.
> > > >
> > > > How about if boot loader register these TCG logs with fTPM TA which
> > > > could be fetched during fTPM driver probe or new api like
> > > > tpm_read_log_tee()?
> > >
> > > And then how does fTPM driver pass TCG Logs to the TPM framework?
> > > It requires changes to the upstream TPM framework to ask the driver
> > > explicitly for the TCG Logs.
> >
> > My suggestion was to add 4th way to pass TCG logs as follows:
> >
> > --- a/drivers/char/tpm/eventlog/common.c
> > +++ b/drivers/char/tpm/eventlog/common.c
> > @@ -95,6 +95,10 @@ static int tpm_read_log(struct tpm_chip *chip)
> >         if (rc != -ENODEV)
> >                 return rc;
> >
> > +       rc = tpm_read_log_tee(chip);
> > +       if (rc != -ENODEV)
> > +               return rc;
> > +
> >         return tpm_read_log_of(chip);
> >  }
> > --- /dev/null
> > +++ b/drivers/char/tpm/eventlog/tee.c
> > @@ -0,0 +1,43 @@
> > <snip>
> > +int tpm_read_log_tee(struct tpm_chip *chip)
> > +{
> > +       struct tpm_bios_log *log;
> > +       struct ftpm_tee_private *pvt_data;
> > +
> > +       log = &chip->log;
> > +       if (!strcmp(chip->dev.bus->name, tee_bus_type.name))
> > +               pvt_data = dev_get_drvdata(chip->dev.parent);
> > +       else
> > +               return -ENODEV;
> > +
> > +       // Here you could simply do an invoke command to fetch the TCG
> > logs.
> > +
> > +       if (chip->flags & TPM_CHIP_FLAG_TPM2)
> > +               return EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;
> > +       return EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
> > +}
>
> But I do not see a need for this 4th method if TEE bus enumeration framework supports flexibility.

If there is a way to fetch event logs buffer at runtime from TPM
device then why should we rely on DT or ACPI stuff?

> The problem is with the TEE bus enumeration.
>

See my response below.

> >
> > >
> > > Note that this also requires changes to the fTPM TA that has been
> > existing for few years.
> > >
> >
> > That's not a sane reason to avoid implementing generic stuff.
>
> Exactly, TEE bus enumeration framework need to support generic stuff that is expected
> of a bus driver. You are expecting everything else to be generic enough, but not TEE bus enumeration.
>

I guess you are missing the basic TEE bus view. It is an
auto-discoverable bus where TAs as devices are detected automatically.
It is similar to USB, PCI etc. busses. But in case of Platform, I2C,
SPI etc. which are NOT auto-discoverable busses, devices are described
in the hardware description language (ACPI or DT).

Like in case of TPM, when acting as an I2C based device is represented
via a DT node (see section: "2.4 Device population" in
Documentation/devicetree/usage-model.txt). And device specific
properties are retrieved from corresponding device DT node. But in
case of TPM being a TEE based device, detected automatically, you
won't have a device DT node to describe its properties but rather
those properties need to derived from the device automatically.

So in summary, it doesn't seems possible to support TEE devices
detected automatically and have properties described in DT unless you
hard code DT node path in the driver to provide those properties which
doesn't looks like a generic solution.

> > As there
> > could be other fTPM implementations and specific DT nodes for each
> > which might not be maintainable. BTW, in current implementation also I
> > don't find DT bindings corresponding to DT node used in this
> > patch-set.
> >
> > > Is it not possible to add flexibility in TEE bus enumeration framework to
> > > support platform specific properties through OF nodes or ACPI?
> > >
> >
> > TEE bus framework was designed specifically to avoid OF nodes or ACPI
> > properties. As devices could be intelligent enough to be queried for
> > required properties.
>
> Is there an expectation that TEE bus enumerated devices to be intelligent?
> If so, this is another limitation of TEE bus enumeration.
> Please fix these limitations to make TEE bus enumeration
> scalable and flexible for future use cases.
>

It's not a limitation but rather a feature of TEE devices being
auto-discoverable rather being described via DT or ACPI.

-Sumit

> >
> > > Devices enumerated by buses such as i2c can read platform specific
> > properties.
> >
> > i2c devices are real hardware which could be platform agnostic, so you
> > need to have platform specific properties.
> >
> > -Sumit
> >
> > > With this flexibility added, more future use cases through TEE bus.
> > >
> > >
> > > > This is something similar to what I used in
> > > > optee-rng [1] driver to fetch RNG properties.
> > > >
> > > > [1]
> > > >
> > https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> > > >
> > m%2Ftorvalds%2Flinux%2Fblob%2Fmaster%2Fdrivers%2Fchar%2Fhw_random%2Foptee-
> > > >
> > rng.c%23L176&amp;data=02%7C01%7Cthiruan%40microsoft.com%7Cd37438eaf4f9483e4
> > > >
> > 0c708d6d9ccfe0c%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63693587179549
> > > >
> > 3006&amp;sdata=As9sC45Bl7sZdJKOq0sHz6GmXttMxS80Nn5yvN4vqng%3D&amp;reserved=
> > > > 0
> > > >
> > > > -Sumit
> > > >
> > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Thanks,
> > > > > > > Sasha