Changes in v6:
- Address comments from Randy Dunlap
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 | 356 ++++++++++++++++++++ drivers/char/tpm/tpm_ftpm_tee.h | 40 +++ 6 files changed, 434 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
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-implement... .
As well as reference code for the firmware available here: https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar...
Tested-by: Thirupathaiah Annapureddy thiruan@microsoft.com 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 | 356 ++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm_ftpm_tee.h | 40 ++++ 4 files changed, 402 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 88a3c06fc153..17bfbf9f572f 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 && OPTEE + ---help--- + This driver proxies for firmware TPM 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..0312c10767bd --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) Microsoft Corporation + * + * Implements a firmware TPM as described here: + * https://www.microsoft.com/en-us/research/publication/ftpm-software-implement... + * + * A reference implementation is available here: + * https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar... + */ + +#include <linux/acpi.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/tee_drv.h> +#include <linux/tpm.h> +#include <linux/uuid.h> + +#include "tpm.h" +#include "tpm_ftpm_tee.h" + +#define DRIVER_NAME "ftpm-tee" + +/* + * TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896 + * + * Randomly generated, and must correspond to the GUID on the TA side. + * Defined here in the reference implementation: + * https://github.com/microsoft/ms-tpm-20-ref/blob/master/Samples/ARM32-Firmwar... + */ + +static const uuid_t ftpm_ta_uuid = + UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4, + 0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96); + +/** + * ftpm_tee_tpm_op_recv - retrieve fTPM response. + * + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. + * @buf: the buffer to store data. + * @count: the number of bytes to read. + * + * Return: + * In case of success the number of bytes received. + * On failure, -errno. + */ +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. + * + * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h + * @buf: the buffer to send. + * @len: the number of bytes to send. + * + * Return: + * In case of success, returns 0. + * On failure, -errno + */ +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; +} + +/** + * ftpm_tee_probe - initialize the fTPM + * @pdev: the platform_device description. + * + * Return: + * On success, 0. On failure, -errno. + */ +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)) { + if (ERR_PTR(pvt_data->ctx) == -ENOENT) + return -EPROBE_DEFER; + dev_err(dev, "%s:tee_client_open_context failed\n", __func__); + return ERR_PTR(pvt_data->ctx); + } + + /* 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 + * @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..b09ee7be4545 --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.h @@ -0,0 +1,40 @@ +/* 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/tpm.h> +#include <linux/uuid.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; +}; + +#endif /* __TPM_FTPM_TEE_H__ */
On Tue, 2019-06-25 at 16:13 -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-implement...
.
As well as reference code for the firmware available here:
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar...
Tested-by: Thirupathaiah Annapureddy thiruan@microsoft.com Signed-off-by: Thirupathaiah Annapureddy thiruan@microsoft.com Signed-off-by: Sasha Levin sashal@kernel.org
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all process-wise if the original author of the patch is also the only tester of the patch?
drivers/char/tpm/Kconfig | 5 + drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm_ftpm_tee.c | 356 ++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm_ftpm_tee.h | 40 ++++ 4 files changed, 402 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 88a3c06fc153..17bfbf9f572f 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 && OPTEE
- ---help---
This driver proxies for firmware TPM 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..0312c10767bd --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) Microsoft Corporation
The statement does not contain the years. I'm also wondering in more generic sense that with Git, which in its inner structure contains all the metadata to deriving this type of information, is this more like a legacy thing or why should be put these statements to new files?
- Implements a firmware TPM as described here:
https://www.microsoft.com/en-us/research/publication/ftpm-software-implement...
- A reference implementation is available here:
https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar...
- */
+#include <linux/acpi.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/tee_drv.h> +#include <linux/tpm.h> +#include <linux/uuid.h>
+#include "tpm.h" +#include "tpm_ftpm_tee.h"
+#define DRIVER_NAME "ftpm-tee"
Should be open coded where it is used because this does not bring any practical value.
+/*
- TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896
- Randomly generated, and must correspond to the GUID on the TA side.
- Defined here in the reference implementation:
https://github.com/microsoft/ms-tpm-20-ref/blob/master/Samples/ARM32-Firmwar...
- */
Probably should delete this empty line.
+static const uuid_t ftpm_ta_uuid =
- UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);+/**
- ftpm_tee_tpm_op_recv - retrieve fTPM response.
Should not have an empty line here.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
- @buf: the buffer to store data.
- @count: the number of bytes to read.
Should be aligned with a tab character.
- Return:
- In case of success the number of bytes received.
- On failure, -errno.
- */
+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",
^ a single white space also here
__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.
Should not have an empty line here.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h
- @buf: the buffer to send.
- @len: the number of bytes to send.
Should be aligned with a tab character.
- Return:
- In case of success, returns 0.
- On failure, -errno
- */
+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
^ a single white space also here
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",
^ a single white space also here
__func__);return PTR_ERR(temp_buf);- }
- memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE));
- memcpy(temp_buf, buf, len);
How about:
}
memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE)); memcpy(temp_buf, buf, len);
Just looked a bit dirty how the stuff was grouped.
- 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);
Aligment is not right in the 2nd like?
- if ((rc < 0) || (transceive_args.ret != 0)) {
dev_err(&chip->dev, "%s:SUBMIT_COMMAND invoke error: 0x%x\n",
The white space char again...
__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;+}
+/**
- ftpm_tee_probe - initialize the fTPM
- @pdev: the platform_device description.
- Return:
- On success, 0. On failure, -errno.
- */
+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)) {
if (ERR_PTR(pvt_data->ctx) == -ENOENT)return -EPROBE_DEFER;dev_err(dev, "%s:tee_client_open_context failed\n", __func__);return ERR_PTR(pvt_data->ctx);- }
- /* 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);
The alignment goes a bit off also here. Seems to fit to 80 chars even when it is nicely aligned (had to test):
pvt_data->shm = tee_shm_alloc(pvt_data->ctx, MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
Also had one pair of redundant parentheses.
- if (IS_ERR(pvt_data->shm)) {
dev_err(dev, "%s:tee_shm_alloc failed\n", __func__);
The white space character.
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__);
The white space character.
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",
The white space character.
__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
- @pdev: the platform_device description.
- Return:
- 0 in case of success.
"0 always" ? Left me puzzling with questions in that form.
- */
+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");
I'm also wondering why we put MODULE_AUTHOR() to new modules...
+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..b09ee7be4545 --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.h @@ -0,0 +1,40 @@ +/* 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/tpm.h> +#include <linux/uuid.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
Two whitespace chars after "#define".
+/**
- 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;
+};
+#endif /* __TPM_FTPM_TEE_H__ */
/Jarkko
On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote:
On Tue, 2019-06-25 at 16:13 -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-implement...
.
As well as reference code for the firmware available here:
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar...
Tested-by: Thirupathaiah Annapureddy thiruan@microsoft.com Signed-off-by: Thirupathaiah Annapureddy thiruan@microsoft.com Signed-off-by: Sasha Levin sashal@kernel.org
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all
Honestly, I've just been massaging this patch more than "authoring" it. If you feel strongly about it feel free to add a Co-authored patch with my name, but in my mind this is just Thiru's work.
process-wise if the original author of the patch is also the only tester of the patch?
There's not much we can do about this... Linaro folks have tested this without the fTPM firmware, so at the very least it won't explode for everyone. If for some reason non-microsoft folks see issues then we can submit patches on top to fix this, we're not just throwing this at you and running away.
drivers/char/tpm/Kconfig | 5 + drivers/char/tpm/Makefile | 1 + drivers/char/tpm/tpm_ftpm_tee.c | 356 ++++++++++++++++++++++++++++++++ drivers/char/tpm/tpm_ftpm_tee.h | 40 ++++ 4 files changed, 402 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 88a3c06fc153..17bfbf9f572f 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 && OPTEE
- ---help---
This driver proxies for firmware TPM 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..0312c10767bd --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.c @@ -0,0 +1,356 @@ +// SPDX-License-Identifier: GPL-2.0 +/*
- Copyright (C) Microsoft Corporation
The statement does not contain the years. I'm also wondering in more generic sense that with Git, which in its inner structure contains all the metadata to deriving this type of information, is this more like a legacy thing or why should be put these statements to new files?
There is a significant amount of newly added 2019 copyright tags in the kernel, and this is just following suit.
- Implements a firmware TPM as described here:
https://www.microsoft.com/en-us/research/publication/ftpm-software-implement...
- A reference implementation is available here:
https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-Firmwar...
- */
+#include <linux/acpi.h> +#include <linux/of.h> +#include <linux/of_platform.h> +#include <linux/platform_device.h> +#include <linux/tee_drv.h> +#include <linux/tpm.h> +#include <linux/uuid.h>
+#include "tpm.h" +#include "tpm_ftpm_tee.h"
+#define DRIVER_NAME "ftpm-tee"
Should be open coded where it is used because this does not bring any practical value.
Good point, this was used more with debug statements that were sprinkled around, but not anymore.
+/*
- TA_FTPM_UUID: BC50D971-D4C9-42C4-82CB-343FB7F37896
- Randomly generated, and must correspond to the GUID on the TA side.
- Defined here in the reference implementation:
https://github.com/microsoft/ms-tpm-20-ref/blob/master/Samples/ARM32-Firmwar...
- */
Probably should delete this empty line.
+static const uuid_t ftpm_ta_uuid =
- UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);+/**
- ftpm_tee_tpm_op_recv - retrieve fTPM response.
Should not have an empty line here.
Really? The rest of the kdoc comments under drivers/char/tpm/ have an empty line after function name.
I've looked at tpm_crb.c which you authored for reference.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
- @buf: the buffer to store data.
- @count: the number of bytes to read.
Should be aligned with a tab character.
I'll address this throughout the comments in the file.
- Return:
- In case of success the number of bytes received.
- On failure, -errno.
- */
+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",^ a single white space also here
__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.
Should not have an empty line here.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h
- @buf: the buffer to send.
- @len: the number of bytes to send.
Should be aligned with a tab character.
- Return:
- In case of success, returns 0.
- On failure, -errno
- */
+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^ a single white space also hereTA\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",^ a single white space also here
__func__);return PTR_ERR(temp_buf);- }
- memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE));
- memcpy(temp_buf, buf, len);
How about:
}
memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE)); memcpy(temp_buf, buf, len);
Just looked a bit dirty how the stuff was grouped.
Makes sense.
- 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);Aligment is not right in the 2nd like?
- if ((rc < 0) || (transceive_args.ret != 0)) {
dev_err(&chip->dev, "%s:SUBMIT_COMMAND invoke error: 0x%x\n",The white space char again...
__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;+}
+/**
- ftpm_tee_probe - initialize the fTPM
- @pdev: the platform_device description.
- Return:
- On success, 0. On failure, -errno.
- */
+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)) {
if (ERR_PTR(pvt_data->ctx) == -ENOENT)return -EPROBE_DEFER;dev_err(dev, "%s:tee_client_open_context failed\n", __func__);return ERR_PTR(pvt_data->ctx);- }
- /* 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);The alignment goes a bit off also here. Seems to fit to 80 chars even when it is nicely aligned (had to test):
pvt_data->shm = tee_shm_alloc(pvt_data->ctx, MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE, TEE_SHM_MAPPED | TEE_SHM_DMA_BUF);
Also had one pair of redundant parentheses.
Just in case math breaks! (fixed).
- if (IS_ERR(pvt_data->shm)) {
dev_err(dev, "%s:tee_shm_alloc failed\n", __func__);The white space character.
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__);The white space character.
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",The white space character.
__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
- @pdev: the platform_device description.
- Return:
- 0 in case of success.
"0 always" ? Left me puzzling with questions in that form.
Technically it's true :) (fixed).
- */
+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");
I'm also wondering why we put MODULE_AUTHOR() to new modules...
It shows up in modinfo and is a nice way to credit the author IMHO.
+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..b09ee7be4545 --- /dev/null +++ b/drivers/char/tpm/tpm_ftpm_tee.h @@ -0,0 +1,40 @@ +/* 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/tpm.h> +#include <linux/uuid.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
Two whitespace chars after "#define".
Fixed.
+/**
- 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;
+};
+#endif /* __TPM_FTPM_TEE_H__ */
/Jarkko
-- Thanks, Sasha
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all
Honestly, I've just been massaging this patch more than "authoring" it. If you feel strongly about it feel free to add a Co-authored patch with my name, but in my mind this is just Thiru's work.
This is just my subjective view but writing code is easier than making it work in the mainline in 99% of cases. If this patch was doing something revolutional, lets say a new outstanding scheduling algorithm, then I would think otherwise. It is not. You without question deserve both credit and also the blame (if this breaks everything) :-)
process-wise if the original author of the patch is also the only tester of the patch?
There's not much we can do about this... Linaro folks have tested this without the fTPM firmware, so at the very least it won't explode for everyone. If for some reason non-microsoft folks see issues then we can submit patches on top to fix this, we're not just throwing this at you and running away.
So why any of those Linaro folks can't do it? I can add after tested-by tag parentheses something explaining that context of testing. It is reasonable given the circumstances.
I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me.
/Jarkko
On Thu, 2019-06-27 at 16:17 +0300, Jarkko Sakkinen wrote:
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all
Honestly, I've just been massaging this patch more than "authoring" it. If you feel strongly about it feel free to add a Co-authored patch with my name, but in my mind this is just Thiru's work.
This is just my subjective view but writing code is easier than making it work in the mainline in 99% of cases. If this patch was doing something revolutional, lets say a new outstanding scheduling algorithm, then I would think otherwise. It is not. You without question deserve both credit and also the blame (if this breaks everything) :-)
Not like I'm putting the pressure on this. You make the call with the tag. Put it if you wwant. I'm cool with either.
/Jarkko
Hi Jarkko,
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all
Honestly, I've just been massaging this patch more than "authoring" it. If you feel strongly about it feel free to add a Co-authored patch with my name, but in my mind this is just Thiru's work.
This is just my subjective view but writing code is easier than making it work in the mainline in 99% of cases. If this patch was doing something revolutional, lets say a new outstanding scheduling algorithm, then I would think otherwise. It is not. You without question deserve both credit and also the blame (if this breaks everything) :-)
process-wise if the original author of the patch is also the only tester of the patch?
There's not much we can do about this... Linaro folks have tested this without the fTPM firmware, so at the very least it won't explode for everyone. If for some reason non-microsoft folks see issues then we can submit patches on top to fix this, we're not just throwing this at you and running away.
So why any of those Linaro folks can't do it? I can add after tested-by tag parentheses something explaining that context of testing. It is reasonable given the circumstances.
There's 2 teams from Microsoft trying to do this [1]. We tested the previous implementation (which problems on probing as built-in). We had to change some stuff in the OP-TEE fTPM implementation [2] and test it in QEMU.
What i quickly did with this module was to replace the kernel of the previous build with the new one. Unfortunately i couldn't get it to work, but i don't know if it's the module or the changes in the fTPM OP-TEE part. Since you have tested it my guess is that it has something to do with the OP-TEE part. I don't have any objections in this going in. On the contrary i think the functionality is really useful. I don't have hardware to test this at the moment, but once i get it, i'll give it a spin.
The part i tested is that the probing works as expected when no fTPM implementation is running on secure world. Since it has been tested and doesn't break anything we can always fix corner, cases afterwards with more extensive testing
[1] https://github.com/ms-iot/linux/blob/ms-optee-ocalls-merge/drivers/char/tpm/... [2] https://github.com/jbech-linaro/manifest/blob/ftpm/README.md
Thanks /Ilias
I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me.
/Jarkko
On Thu, 2019-06-27 at 16:30 +0300, Ilias Apalodimas wrote:
is really useful. I don't have hardware to test this at the moment, but once i get it, i'll give it a spin.
Thank you for responding, really appreciate it.
Please note, however, that I already did my v5.3 PR so there is a lot of time to give it a spin. In all cases, we will find a way to put this to my v5.4 PR. I don't see any reason why not.
As soon as the cosmetic stuff is fixed that I remarked in v7 I'm ready to take this to my tree and after that soonish make it available on linux-next.
/Jarkko
Hi,
On Thu, 2019-06-27 at 16:30 +0300, Ilias Apalodimas wrote:
is really useful. I don't have hardware to test this at the moment, but once i get it, i'll give it a spin.
Thank you for responding, really appreciate it.
No worries
Please note, however, that I already did my v5.3 PR so there is a lot of time to give it a spin. In all cases, we will find a way to put this to my v5.4 PR. I don't see any reason why not.
As soon as the cosmetic stuff is fixed that I remarked in v7 I'm ready to take this to my tree and after that soonish make it available on linux-next.
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module # insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in # dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008 ftpm-tee: probe of firmware:optee failed with error -22 # getrandom -by 8 random: fast init done urandom_read: 2 callbacks suppressed random: getrandom: uninitialized urandom read (32 bytes read) TSS_Dev_Open: Error opening /dev/tpm0 getrandom: failed, rc 000b0008 TSS_RC_NO_CONNECTION - Failure connecting to lower layer
Am i missing anything?
Thanks /Ilias
Hi Ilias,
First of all, Thanks a lot for trying to test the driver.
-----Original Message----- From: Ilias Apalodimas ilias.apalodimas@linaro.org Sent: Tuesday, July 2, 2019 7:21 AM To: Jarkko Sakkinen jarkko.sakkinen@linux.intel.com Cc: Sasha Levin sashal@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca; corbet@lwn.net; 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; tee-dev@lists.linaro.org; sumit.garg@linaro.org; rdunlap@infradead.org Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
Hi,
On Thu, 2019-06-27 at 16:30 +0300, Ilias Apalodimas wrote:
is really useful. I don't have hardware to test this at the moment, but
once i
get it, i'll give it a spin.
Thank you for responding, really appreciate it.
No worries
Please note, however, that I already did my v5.3 PR so there is a lot of time to give it a spin. In all cases, we will find a way to put this to my v5.4 PR. I don't see any reason why not.
As soon as the cosmetic stuff is fixed that I remarked in v7 I'm ready to take this to my tree and after that soonish make it available on linux-next.
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE supplicant. Can you confirm? If that is the true, - In the case of driver built as a module (CONFIG_TCG_FTPM_TEE=m), this is works fine as user mode supplicant is ready. - In the built-in case (CONFIG_TCG_FTPM_TEE=y), This would result in the above error 0xffff0008 as TEE is unable to find fTPM TA.
The expectation is that fTPM TA is built as an EARLY_TA (in BL32) so that U-boot and Linux driver stacks work seamlessly without dependency on supplicant.
ftpm-tee: probe of firmware:optee failed with error -22 # getrandom -by 8 random: fast init done urandom_read: 2 callbacks suppressed random: getrandom: uninitialized urandom read (32 bytes read) TSS_Dev_Open: Error opening /dev/tpm0 getrandom: failed, rc 000b0008 TSS_RC_NO_CONNECTION - Failure connecting to lower layer
Am i missing anything?
Thanks /Ilias
Hi Thirupathaiah,
First of all, Thanks a lot for trying to test the driver.
np
[...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE supplicant. Can you confirm?
I tried both
If that is the true,
- In the case of driver built as a module (CONFIG_TCG_FTPM_TEE=m), this is works fine
as user mode supplicant is ready.
- In the built-in case (CONFIG_TCG_FTPM_TEE=y),
This would result in the above error 0xffff0008 as TEE is unable to find fTPM TA.
Maybe i did something wrong and never noticed it wasn't built as an earlyTA
The expectation is that fTPM TA is built as an EARLY_TA (in BL32) so that U-boot and Linux driver stacks work seamlessly without dependency on supplicant.
You can add my tested-by tag for the module. I'll go back to testing it as built-in at some point in real hardware and let you know if i have any issues.
If someone's is interested in the QEMU testing: 1. compile this https://github.com/jbech-linaro/manifest/blob/ftpm/README.md 2. replace the whole linux kernel on the root-dir with a latest version + fTPM char driver 3. Apply a hack on kernel and disable dynamic shm (Need for this depends on kernel + op-tee version)
diff --git a/drivers/tee/optee/core.c b/drivers/tee/optee/core.c index 1854a3db..7aea8a5 100644 --- a/drivers/tee/optee/core.c +++ b/drivers/tee/optee/core.c @@ -588,13 +588,15 @@ static struct optee *optee_probe(struct device_node *np) /* * Try to use dynamic shared memory if possible */ +#if 0 if (sec_caps & OPTEE_SMC_SEC_CAP_DYNAMIC_SHM) pool = optee_config_dyn_shm(); +#endif
/* * If dynamic shared memory is not available or failed - try static one */ - if (IS_ERR(pool) && (sec_caps & OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM)) + if (sec_caps & OPTEE_SMC_SEC_CAP_HAVE_RESERVED_SHM) pool = optee_config_shm_memremap(invoke_fn, &memremaped_shm);
if (IS_ERR(pool))
For the module part: Tested-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Hi Thirupathaiah,
(+Joakim)
On Wed, 3 Jul 2019 at 09:58, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
First of all, Thanks a lot for trying to test the driver.
np
[...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE supplicant. Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
Thanks /Ilias
On Wed, 3 Jul 2019 at 13:42, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
(+Joakim)
On Wed, 3 Jul 2019 at 09:58, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
First of all, Thanks a lot for trying to test the driver.
np
[...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE supplicant. Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
U-boot and Linux driver stacks work seamlessly without dependency on supplicant.
Is this true?
It looks like this fTPM driver can't work as a built-in driver. The reason seems to be secure storage access required by OP-TEE fTPM TA that is provided via OP-TEE supplicant that's not available during kernel boot.
Snippet from ms-tpm-20-ref/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c +145:
// If we fail to open fTPM storage we cannot continue. if (_plat__NVEnable(NULL) == 0) { TEE_Panic(TEE_ERROR_BAD_STATE); }
So it seems like this module will work as a loadable module only after OP-TEE supplicant is up.
-Sumit
Thanks /Ilias
On Wed, Jul 03, 2019 at 03:33:14PM +0530, Sumit Garg wrote:
On Wed, 3 Jul 2019 at 13:42, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
(+Joakim)
On Wed, 3 Jul 2019 at 09:58, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
First of all, Thanks a lot for trying to test the driver.
np
[...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE supplicant. Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
U-boot and Linux driver stacks work seamlessly without dependency on supplicant.
Is this true?
It looks like this fTPM driver can't work as a built-in driver. The reason seems to be secure storage access required by OP-TEE fTPM TA that is provided via OP-TEE supplicant that's not available during kernel boot.
Snippet from ms-tpm-20-ref/Samples/ARM32-FirmwareTPM/optee_ta/fTPM/fTPM.c +145:
// If we fail to open fTPM storage we cannot continue. if (_plat__NVEnable(NULL) == 0) { TEE_Panic(TEE_ERROR_BAD_STATE); }So it seems like this module will work as a loadable module only after OP-TEE supplicant is up.
This seems to be the same issues that I faced when trying to put together a setup for Linaro Connect discussions. When compiling the fTPM driver into the kernel (instead of a module) I saw mainly two issues.
1) fTPM driver seems to be probed before the TEE driver has been probed. I temporary solved that by doing a late_initcall.
2) With the late_initcall hack applied, the TEE side was called successfully (if the fTPM TA's is compiled as "early TAs", i.e., built into the TEE core iself), but as Sumit said, it got stock on secure storage operations, since tee-supplicant, the userspace process serving the TEE with storage access hasn't been started.
The first issue can(?)/should(?) be solved by some deferred probing mechanism.
Regarding the second issue, is there a must to access secure storage when Linux kernel is booting up? I suppose this is some kind of initialization of the "NV" (adding TPM measurements?), but I guess it should be possible to delay those calls to a later point, when tee-supplicant is up and running and the first call to the TEE is made.
Hi Ilias,
-----Original Message----- From: Ilias Apalodimas ilias.apalodimas@linaro.org Sent: Wednesday, July 3, 2019 1:12 AM To: Thirupathaiah Annapureddy thiruan@microsoft.com Cc: Jarkko Sakkinen jarkko.sakkinen@linux.intel.com; Sasha Levin sashal@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca; corbet@lwn.net; 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; tee- dev@lists.linaro.org; sumit.garg@linaro.org; rdunlap@infradead.org; Joakim Bech joakim.bech@linaro.org Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
Hi Thirupathaiah,
(+Joakim)
On Wed, 3 Jul 2019 at 09:58, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Thirupathaiah,
First of all, Thanks a lot for trying to test the driver.
np
[...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-
TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE
supplicant.
Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after stitching fTPM TA as an EARLY_TA.
Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are using to test? What is the preboot environment (UEFI or U-boot)? Where is the secure storage in that HW platform? I could think of two classes of secure storage. 1. UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the fTPM TA NV Storage, there should be no issue. If fTPM TA NV storage is not initialized in pre-boot environment and you are using built-in fTPM Linux driver, you can run into this issue as TA will try to initialize NV store and fail.
2. other storage devices like QSPI accessible to only secure mode after EBS/ReadyToBoot mile posts during boot. In this case, there should be no issue at all as there is no dependency on non-secure side services provided by supplicant.
If you let me know the HW platform details, I am happy to work with you to enable/integrate fTPM TA on that HW platform.
Best Regards, Thiru
Hi Thirupathaiah, [...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM 2.0 TSS)
- As module
# insmod /lib/modules/5.2.0-rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko # getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode OP-
TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-TEE
supplicant.
Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after stitching fTPM TA as an EARLY_TA.
Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are using to test?
QEMU, on armv7
What is the preboot environment (UEFI or U-boot)? Where is the secure storage in that HW platform? I could think of two classes of secure storage.
- UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the
fTPM TA NV Storage, there should be no issue. If fTPM TA NV storage is not initialized in pre-boot environment and you are using built-in fTPM Linux driver, you can run into this issue as TA will try to initialize NV store and fail.
- other storage devices like QSPI accessible to only secure mode after
EBS/ReadyToBoot mile posts during boot. In this case, there should be no issue at all as there is no dependency on non-secure side services provided by supplicant.
Please check the previous mail from Sumit. It explains exaclty what's going on. The tl;dr version is that the storage is up only when the supplicant is running.
If you let me know the HW platform details, I am happy to work with you to enable/integrate fTPM TA on that HW platform.
Thanks, The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot supplicant support will be there so i'll be able to test it.
Thanks /Ilias
-----Original Message----- From: Ilias Apalodimas ilias.apalodimas@linaro.org Sent: Thursday, July 4, 2019 11:11 AM To: Thirupathaiah Annapureddy thiruan@microsoft.com Cc: Jarkko Sakkinen jarkko.sakkinen@linux.intel.com; Sasha Levin sashal@kernel.org; peterhuewe@gmx.de; jgg@ziepe.ca; corbet@lwn.net; 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; tee- dev@lists.linaro.org; sumit.garg@linaro.org; rdunlap@infradead.org; Joakim Bech joakim.bech@linaro.org Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEE
Hi Thirupathaiah, [...]
I managed to do some quick testing in QEMU. Everything works fine when i build this as a module (using IBM's TPM
2.0
TSS)
- As module
# insmod /lib/modules/5.2.0-
rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.ko
# getrandom -by 8 randomBytes length 8 23 b9 3d c3 90 13 d9 6b
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session
failed,
err=ffff0008
This (0xffff0008) translates to TEE_ERROR_ITEM_NOT_FOUND.
Where is fTPM TA located in the your test setup? Is it stitched into TEE binary as an EARLY_TA or Is it expected to be loaded during run-time with the help of user mode
OP-
TEE supplicant?
My guess is that you are trying to load fTPM TA through user mode OP-
TEE
supplicant.
Can you confirm?
I tried both
Ok apparently there was a failure with my built-in binary which i didn't notice. I did a full rebuilt and checked the elf this time :)
Built as an earlyTA my error now is: ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates to TEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
I am glad that the first issue (TEE_ERROR_ITEM_NOT_FOUND) is resolved after
stitching
fTPM TA as an EARLY_TA.
Regarding TEE_ERROR_TARGET_DEAD error, may I know which HW platform you are
using to test?
QEMU, on armv7
What is the preboot environment (UEFI or U-boot)? Where is the secure storage in that HW platform? I could think of two classes of secure storage.
- UFS/eMMC RPMB : If Supplicant in U-boot/UEFI initializes the
fTPM TA NV Storage, there should be no issue. If fTPM TA NV storage is not initialized in pre-boot environment and you are
using
built-in fTPM Linux driver, you can run into this issue as TA will try to
initialize
NV store and fail.
- other storage devices like QSPI accessible to only secure mode after
EBS/ReadyToBoot mile posts during boot. In this case, there should be no
issue at all
as there is no dependency on non-secure side services provided by supplicant.
Please check the previous mail from Sumit. It explains exaclty what's going on. The tl;dr version is that the storage is up only when the supplicant is running.
I definitely know that OP-TEE can access storage only when the "user mode" supplicant is running :). But fTPM NV storage should have been initialized in in the preboot environment (UEFI/U-boot).
It would also be helpful to understand the overall use case/scenario (Measured boot?)you are trying to exercise with the fTPM.
I also want to emphasize that this discussion is turning into more of how fTPM gets integrated/enabled in a new HW platform. fTPM is hosted in github and you definitely bring any issues/feature requests there.
If you let me know the HW platform details, I am happy to work with you to
enable/integrate
fTPM TA on that HW platform.
Thanks, The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot supplicant support will be there so i'll be able to test it.
Can you give me the details of HW so that I can order one for myself? Is it one of the 96boards? The reason for the ask is that we have not upstreamd u-boot fTPM stack yet, although we have future plans for it.
--Thiru
Hi Thirupathaiah
Apologies for tha lte reply, i somehow misplaced this mail.
[...]
Please check the previous mail from Sumit. It explains exaclty what's going on. The tl;dr version is that the storage is up only when the supplicant is running.
I definitely know that OP-TEE can access storage only when the "user mode" supplicant is running :). But fTPM NV storage should have been initialized in in the preboot environment (UEFI/U-boot).
It would also be helpful to understand the overall use case/scenario (Measured boot?)you are trying to exercise with the fTPM.
In the future yesm measured boot/ For now it's more like like try running it in QEMU to demonstrate firmware TPM makes sense and has use cases.
I also want to emphasize that this discussion is turning into more of how fTPM gets integrated/enabled in a new HW platform. fTPM is hosted in github and you definitely bring any issues/feature requests there.
Ok
If you let me know the HW platform details, I am happy to work with you to
enable/integrate
fTPM TA on that HW platform.
Thanks, The hardware i am waiting for for has an eMMC RPMB. In theory the U-Boot supplicant support will be there so i'll be able to test it.
Can you give me the details of HW so that I can order one for myself?
It's QEMU for now. We plan on doing something similar in an ST disco board though.
Is it one of the 96boards?
stm32mp157c-dk2 is one of our targets.
The reason for the ask is that we have not upstreamd u-boot fTPM stack yet, although we have future plans for it.
--Thiru
Thanks /Ilias
Hi Jarkko and Sasha,
On Thu, 27 Jun 2019 at 18:47, Jarkko Sakkinen jarkko.sakkinen@linux.intel.com wrote:
On Wed, 2019-06-26 at 19:56 -0400, Sasha Levin wrote:
You've used so much on this so shouldn't this have that somewhat new co-developed-by tag? I'm also wondering can this work at all
Honestly, I've just been massaging this patch more than "authoring" it. If you feel strongly about it feel free to add a Co-authored patch with my name, but in my mind this is just Thiru's work.
This is just my subjective view but writing code is easier than making it work in the mainline in 99% of cases. If this patch was doing something revolutional, lets say a new outstanding scheduling algorithm, then I would think otherwise. It is not. You without question deserve both credit and also the blame (if this breaks everything) :-)
process-wise if the original author of the patch is also the only tester of the patch?
There's not much we can do about this... Linaro folks have tested this without the fTPM firmware, so at the very least it won't explode for everyone. If for some reason non-microsoft folks see issues then we can submit patches on top to fix this, we're not just throwing this at you and running away.
So why any of those Linaro folks can't do it? I can add after tested-by tag parentheses something explaining that context of testing. It is reasonable given the circumstances.
Simply because the hardware I have (Developerbox) doesn't provide enough flash space (as per current memory map) for this fTPM driver to be loaded as early TA along with OP-TEE binary. So I can't get any further point than sanity probe failure check for which I think a tested-by won't be appropriate.
-Sumit
I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me.
/Jarkko
On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote:
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
+static const uuid_t ftpm_ta_uuid =
- UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);+/**
- ftpm_tee_tpm_op_recv - retrieve fTPM response.
Should not have an empty line here.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
- @buf: the buffer to store data.
- @count: the number of bytes to read.
Jarkko, w.r.t your comment above, there is an empty line between the function name and variables in drivers/char/tpm, and in particular tpm_crb.c which you authored and I used as reference. Do you want us to diverge here?
-- Thanks, Sasha
On Sat, 2019-06-29 at 11:01 -0400, Sasha Levin wrote:
On Thu, Jun 27, 2019 at 02:31:35AM +0300, Jarkko Sakkinen wrote:
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
+static const uuid_t ftpm_ta_uuid =
- UUID_INIT(0xBC50D971, 0xD4C9, 0x42C4,
0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);+/**
- ftpm_tee_tpm_op_recv - retrieve fTPM response.
Should not have an empty line here.
- @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h.
- @buf: the buffer to store data.
- @count: the number of bytes to read.
Jarkko, w.r.t your comment above, there is an empty line between the function name and variables in drivers/char/tpm, and in particular tpm_crb.c which you authored and I used as reference. Do you want us to diverge here?
There is divergence and that was the first thing I've contributed to the TPM driver. I use this as the reference for formatting function descriptions these days:
https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
According to that the legit way to format would be:
* ftpm_tee_tpm_op_recv() - retrieve fTPM response. * @chip: the tpm_chip description as specified in driver/char/tpm/tpm.h. * @buf: the buffer to store data. * @count: the number of bytes to read.
Since it is both a callback to an interface defined elsewhere and a static function and it does not document anything useful, I would just remove this comment. I'd do it for all callbacks that are part of tpm_call_ops.
/Jarkko
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by: Sasha Levin sashal@kernel.org --- Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_ftpm_tee.rst | 31 +++++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index af77a7bbb070..15783668644f 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,4 +4,5 @@ Trusted Platform Module documentation
.. toctree::
+ tpm_ftpm_tee tpm_vtpm_proxy diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst b/Documentation/security/tpm/tpm_ftpm_tee.rst new file mode 100644 index 000000000000..48de0dcec0f6 --- /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 firmware implemented in ARM's TrustZone +environment. The driver allows programs to interact with the TPM in the same +way they 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.pd... + +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.
On 6/25/19 1:13 PM, Sasha Levin wrote:
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by: Sasha Levin sashal@kernel.org
Acked-by: Randy Dunlap rdunlap@infradead.org
Thanks.
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..48de0dcec0f6 --- /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 firmware implemented in ARM's TrustZone +environment. The driver allows programs to interact with the TPM in the same +way they 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.pd...
+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.
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by: Sasha Levin sashal@kernel.org
Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_ftpm_tee.rst | 31 +++++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index af77a7bbb070..15783668644f 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree::
- tpm_ftpm_tee tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst b/Documentation/security/tpm/tpm_ftpm_tee.rst new file mode 100644 index 000000000000..48de0dcec0f6 --- /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
The way I see it this thing should not exist here at all but the first patch should have co-developed-by in addition to signed-off-by from you. Otherwise looks good. The list of authors is againt something that Git does better than humans.
/Jarkko
On Thu, Jun 27, 2019 at 02:34:06AM +0300, Jarkko Sakkinen wrote:
On Tue, 2019-06-25 at 16:13 -0400, Sasha Levin wrote:
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by: Sasha Levin sashal@kernel.org
Documentation/security/tpm/index.rst | 1 + Documentation/security/tpm/tpm_ftpm_tee.rst | 31 +++++++++++++++++++++ 2 files changed, 32 insertions(+) create mode 100644 Documentation/security/tpm/tpm_ftpm_tee.rst
diff --git a/Documentation/security/tpm/index.rst b/Documentation/security/tpm/index.rst index af77a7bbb070..15783668644f 100644 --- a/Documentation/security/tpm/index.rst +++ b/Documentation/security/tpm/index.rst @@ -4,4 +4,5 @@ Trusted Platform Module documentation
.. toctree::
- tpm_ftpm_tee tpm_vtpm_proxy
diff --git a/Documentation/security/tpm/tpm_ftpm_tee.rst b/Documentation/security/tpm/tpm_ftpm_tee.rst new file mode 100644 index 000000000000..48de0dcec0f6 --- /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
The way I see it this thing should not exist here at all but the first patch should have co-developed-by in addition to signed-off-by from you. Otherwise looks good. The list of authors is againt something that Git does better than humans.
I've dropped this part, but left the co-developed tag out, I honestly don't think that I had significant contribution here.
-- Thanks, Sasha
-
Ilias Apalodimas -
Jarkko Sakkinen -
Joakim Bech -
Randy Dunlap -
Sasha Levin -
Sumit Garg -
Thirupathaiah Annapureddy