fTPM: firmware TPM running in TEE
Subsystems (5)
v7 cover fTPM: firmware TPM running in TEE
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
Signature · 1 line
2.20.1
PATCH v7 1/2 fTPM: firmware TPM running in TEE
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
--- 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
drivers/char/tpm/Kconfig +5 -0
@@ -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
drivers/char/tpm/Makefile +1 -0
@@ -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
drivers/char/tpm/tpm_ftpm_tee.c new file +356 -0
356 lines not on this page · raw message
drivers/char/tpm/tpm_ftpm_tee.h new file +40 -0
@@ -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__ */
Signature · 1 line
2.20.1
Quoted 5 lines · 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/
Quoted 4 lines
.
As well as reference code for the firmware available here:
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM
Quoted 4 lines
Signed-off-by Thirupathaiah Annapureddy <[email protected]>Signed-off-by Sasha Levin <[email protected]>
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?
Quoted 40 lines
--- 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.hdrivers/char/tpm/Kconfig +5
-0@@ -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_TPMdrivers/char/tpm/Makefile +1
-0@@ -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.odrivers/char/tpm/tpm_ftpm_tee.c new file +0
-0
drivers/char/tpm/tpm_ftpm_tee.c line 3
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0@@ -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?
drivers/char/tpm/tpm_ftpm_tee.c line 7
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* +* Implements a firmware TPM as described here: +*
https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
drivers/char/tpm/tpm_ftpm_tee.c line 27
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* +* A reference implementation is available here: +*
https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +4
-0+*/ + +#include <linux/acpi.h> +#include <linux/of.h>
drivers/char/tpm/tpm_ftpm_tee.c line 23
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+#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.
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+ +/* +* 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: +*
drivers/char/tpm/tpm_ftpm_tee.c line 32
Quoted 2 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+*/ +
Probably should delete this empty line.
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 7 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+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.
drivers/char/tpm/tpm_ftpm_tee.c line 42
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* @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.
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +4
-0+* +* Return: +* In case of success the number of bytes received. +* On failure, -errno.
drivers/char/tpm/tpm_ftpm_tee.c line 56
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+*/ +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
drivers/char/tpm/tpm_ftpm_tee.c line 69
Quoted 13 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +13
-0+ __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.
drivers/char/tpm/tpm_ftpm_tee.c line 72
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* @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.
Quoted 9 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +9
-0+* +* 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;
drivers/char/tpm/tpm_ftpm_tee.c
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+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
Quoted 19 lines
TA\n",
drivers/char/tpm/tpm_ftpm_tee.c new file +18
-0+ __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 ,
drivers/char/tpm/tpm_ftpm_tee.c line 119
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ .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
drivers/char/tpm/tpm_ftpm_tee.c line 125
Quoted 6 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +6
-0+ __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.
drivers/char/tpm/tpm_ftpm_tee.c line 137
Quoted 12 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +12
-0+ + 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?
drivers/char/tpm/tpm_ftpm_tee.c line 139
Quoted 2 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+if ((rc <0 ) || (transceive_args.ret !=0 )) { + dev_err(&chip->dev, "%s:SUBMIT_COMMAND invoke error: 0x%x\n",
The white space char again...
Quoted 118 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +65
-0+ __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)
drivers/char/tpm/tpm_ftpm_tee.c new file +52
-0+{ +/* +* 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",
drivers/char/tpm/tpm_ftpm_tee.c line 266
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ __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.
drivers/char/tpm/tpm_ftpm_tee.c line 268
Quoted 2 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+if (IS_ERR (pvt_data->shm)) { +dev_err (dev,"%s:tee_shm_alloc failed\n" , __func__);
The white space character.
drivers/char/tpm/tpm_ftpm_tee.c line 276
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +8
-0+ 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.
drivers/char/tpm/tpm_ftpm_tee.c line 287
Quoted 11 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +11
-0+ 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.
Quoted 14 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +14
-0+ __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);
drivers/char/tpm/tpm_ftpm_tee.c line 311
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ +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.
Quoted 33 lines
+ */
drivers/char/tpm/tpm_ftpm_tee.c new file +32
-0+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 = {
drivers/char/tpm/tpm_ftpm_tee.c line 354
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ .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 <[email protected]>" );
I'm also wondering why we put MODULE_AUTHOR() to new modules...
Quoted 17 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+MODULE_DESCRIPTION ("TPM Driver for fTPM TA in TEE" ); +MODULE_LICENSE ("GPL v2" );drivers/char/tpm/tpm_ftpm_tee.h new file +9
-0@@ -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>
drivers/char/tpm/tpm_ftpm_tee.h line 19
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +10
-0+#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".
Quoted 11 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +11
-0+ +/** +* 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. +*/
drivers/char/tpm/tpm_ftpm_tee.h line 40
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +10
-0+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
Quoted 11 lines · Jarkko Sakkinen
Quoted 5 lines · 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/
Quoted 4 lines
.
As well as reference code for the firmware available here:
Quoted 8 lines
https://github.com/Microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM
Quoted 4 lines
Signed-off-by Thirupathaiah Annapureddy <[email protected]>Signed-off-by Sasha Levin <[email protected]>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.
Quoted 2 lines
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.
Quoted 40 lines
Quoted 40 lines
--- 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.hdrivers/char/tpm/Kconfig +5
-0@@ -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_TPMdrivers/char/tpm/Makefile +1
-0@@ -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.odrivers/char/tpm/tpm_ftpm_tee.c new file +0
-0@@ -0,0 +1,356 @@
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c line 3
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+// 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.
Quoted 19 lines
drivers/char/tpm/tpm_ftpm_tee.c line 7
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* +* Implements a firmware TPM as described here: +* https://www.microsoft.com/en-us/research/publication/ftpm-software-implementation-tpm-chip/
drivers/char/tpm/tpm_ftpm_tee.c line 27
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* +* A reference implementation is available here: +* https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM
Quoted 9 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +9
-0+*/ + +#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>
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c line 23
Quoted 5 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +5
-0+ +#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.
Quoted 14 lines
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+ +/* +* 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: +* drivers/char/tpm/tpm_ftpm_tee.c line 32
Quoted 2 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+*/ +Probably should delete this empty line.
Quoted 9 lines
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 7 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+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.
Quoted 5 lines
drivers/char/tpm/tpm_ftpm_tee.c line 42
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* @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.
Quoted 99 lines
Quoted 4 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +4
-0+* +* Return: +* In case of success the number of bytes received. +* On failure, -errno. drivers/char/tpm/tpm_ftpm_tee.c line 56
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+*/ +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
drivers/char/tpm/tpm_ftpm_tee.c line 69
Quoted 13 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +13
-0+ __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.
drivers/char/tpm/tpm_ftpm_tee.c line 72
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +3
-0+* @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.
Quoted 9 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +9
-0+* +* 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;drivers/char/tpm/tpm_ftpm_tee.c
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+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
Quoted 19 lines
TA\n",
drivers/char/tpm/tpm_ftpm_tee.c new file +18
-0+ __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 ,drivers/char/tpm/tpm_ftpm_tee.c line 119
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ .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
drivers/char/tpm/tpm_ftpm_tee.c line 125
Quoted 6 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +6
-0+ __func__); +return PTR_ERR (temp_buf); + } +memset (temp_buf,0 , (MAX_COMMAND_SIZE +MAX_RESPONSE_SIZE )); + +memcpy (temp_buf, buf, len);How about:
Quoted 6 lines
} 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.
Quoted 152 lines
drivers/char/tpm/tpm_ftpm_tee.c line 137
Quoted 12 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +12
-0+ + 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?
drivers/char/tpm/tpm_ftpm_tee.c line 139
Quoted 2 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+if ((rc <0 ) || (transceive_args.ret !=0 )) { + dev_err(&chip->dev, "%s:SUBMIT_COMMAND invoke error: 0x%x\n",The white space char again...
Quoted 118 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +65
-0+ __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)
drivers/char/tpm/tpm_ftpm_tee.c new file +52
-0+{ +/* +* 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",drivers/char/tpm/tpm_ftpm_tee.c line 266
Quoted 10 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +10
-0+ __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):
Quoted 5 lines
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).
Quoted 49 lines
drivers/char/tpm/tpm_ftpm_tee.c line 268
Quoted 3 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+if (IS_ERR (pvt_data->shm)) { +dev_err (dev,"%s:tee_shm_alloc failed\n" , __func__);The white space character.
drivers/char/tpm/tpm_ftpm_tee.c line 276
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +8
-0+ 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.
drivers/char/tpm/tpm_ftpm_tee.c line 287
Quoted 11 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +11
-0+ 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.
Quoted 18 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +18
-0+ __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; +} +
Quoted 8 lines
Quoted 6 lines
+/**
+ *ftpm_tee_remove- remove the TPM device
+ * @pdev: theplatform_devicedescription.
+ *
+ * Return:
+ * 0 in case of success."0 always" ? Left me puzzling with questions in that form.
Technically it's true :) (fixed).
Quoted 37 lines
Quoted 37 lines
+ */
drivers/char/tpm/tpm_ftpm_tee.c new file +36
-0+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,
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.c line 354
Quoted 6 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +6
-0+ .remove = ftpm_tee_remove, +}; + +module_platform_driver (ftpm_tee_driver); + +MODULE_AUTHOR ("Thirupathaiah Annapureddy <[email protected]>" );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.
Quoted 21 lines
Quoted 21 lines
drivers/char/tpm/tpm_ftpm_tee.c new file +2
-0+MODULE_DESCRIPTION ("TPM Driver for fTPM TA in TEE" ); +MODULE_LICENSE ("GPL v2" );drivers/char/tpm/tpm_ftpm_tee.h new file +13
-0@@ -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 */
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.h line 19
Quoted 6 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +6
-0+#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 4096Two whitespace chars after "#define".
Fixed.
Quoted 15 lines
Quoted 15 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +15
-0+ +/** +* 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;
Quoted 8 lines
drivers/char/tpm/tpm_ftpm_tee.h line 40
Quoted 6 lines
drivers/char/tpm/tpm_ftpm_tee.h new file +6
-0+u8 resp_buf[MAX_RESPONSE_SIZE ]; +struct tee_context *ctx; + struct tee_shm *shm; +}; + +#endif /* __TPM_FTPM_TEE_H__ */ /Jarkko
--
Thanks,
Sasha
Quoted 6 lines · Sasha Levin
Quoted 2 lines
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) :-)
Quoted 8 lines
Quoted 2 lines
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
Quoted 8 lines · Jarkko Sakkinen
Quoted 6 lines · Sasha Levin
Quoted 2 lines
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.
Quoted 5 lines
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,
Quoted 23 lines · Sasha Levin
Quoted 6 lines
Quoted 2 lines
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) :-)
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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/tpm_ftpm_optee.c
[2] https://github.com/jbech-linaro/manifest/blob/ftpm/README.md
Thanks
/Ilias
Quoted 6 lines
I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me.
/Jarkko
Quoted 2 lines · Ilias Apalodimas
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,
Quoted 6 lines · Ilias Apalodimas
Quoted 2 lines
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
Quoted 7 lines
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.
Quoted 39 lines
-----Original Message-----
From: Ilias Apalodimas <[email protected]>
Sent: Tuesday, July 2, 2019 7:21 AM
To: Jarkko Sakkinen <[email protected]>
Cc: Sasha Levin <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; Microsoft Linux Kernel List <linux- [email protected]>; Thirupathaiah Annapureddy <[email protected]>; Bryan Kelly (CSI) <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEEHi,
Quoted 2 lines · Ilias Apalodimas
Quoted 1 line
is really useful. I don't have hardware to test this at the moment, but
once i
Quoted 4 lines
Quoted 1 line
get it, i'll give it a spin.
Thank you for responding, really appreciate it.
No worries
Quoted 7 lines
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
Quoted 4 lines
- 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.
Quoted 13 lines
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 layerAm i missing anything?
Thanks
/Ilias
Ilias Apalodimas
Tested-byHi Thirupathaiah,
Quoted 3 lines
First of all, Thanks a lot for trying to test the driver.
np
[...]
Quoted 14 lines
Quoted 10 lines
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 6bQuoted 4 lines
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008
Quoted 8 lines
This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
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
Quoted 5 lines
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 error0xffff0008as TEE is unable to find fTPM TA.
Maybe i did something wrong and never noticed it wasn't built as an earlyTA
Quoted 4 lines
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)
drivers/tee/optee/core.c +3 -1
@@ -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:
Hi Thirupathaiah,
(+Joakim)
Quoted 24 lines · Ilias Apalodimas
Hi Thirupathaiah,
Quoted 3 lines
First of all, Thanks a lot for trying to test the driver.
np
[...]
Quoted 16 lines
Quoted 10 lines
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 6bQuoted 4 lines
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.
Quoted 8 lines
Quoted 6 lines
Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
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
Quoted 40 lines · Ilias Apalodimas
Hi Thirupathaiah,
(+Joakim)
Quoted 24 lines · Ilias Apalodimas
Hi Thirupathaiah,
Quoted 3 lines
First of all, Thanks a lot for trying to test the driver.
np
[...]
Quoted 16 lines
Quoted 10 lines
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 6bQuoted 4 lines
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Quoted 8 lines
Quoted 6 lines
Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
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
Quoted 11 lines
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 toTEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?Quoted 1 line
Quoted 1 line
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
Quoted 2 lines
Thanks
/Ilias
Quoted 63 lines · Sumit Garg
Quoted 40 lines · Ilias Apalodimas
Hi Thirupathaiah,
(+Joakim)
Quoted 24 lines · Ilias Apalodimas
Hi Thirupathaiah,
Quoted 3 lines
First of all, Thanks a lot for trying to test the driver.
np
[...]
Quoted 16 lines
Quoted 10 lines
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 6bQuoted 4 lines
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Quoted 8 lines
Quoted 6 lines
Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
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
Quoted 11 lines
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 toTEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?Quoted 1 line
Quoted 1 line
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:
Quoted 8 lines
// 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.
Signature · 2 lines
Regards, Joakim
Hi Ilias,
Quoted 54 lines
-----Original Message-----
From: Ilias Apalodimas <[email protected]>
Sent: Wednesday, July 3, 2019 1:12 AM
To: Thirupathaiah Annapureddy <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>; Sasha Levin <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]; Microsoft Linux Kernel List <linux- [email protected]>; Bryan Kelly (CSI) <[email protected]>; tee- [email protected]; [email protected]; [email protected]; Joakim Bech <[email protected]>
Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEEHi Thirupathaiah,
(+Joakim)
Quoted 27 lines · Ilias Apalodimas
Hi Thirupathaiah,
Quoted 3 lines
First of all, Thanks a lot for trying to test the driver.
np
[...]
Quoted 19 lines
Quoted 10 lines
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 6bQuoted 4 lines
- Built-in
# dmesg | grep optee ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff0008This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
Quoted 2 lines
Quoted 2 lines
My guess is that you are trying to load fTPM TA through user mode OP-TEE
supplicant.
Quoted 3 lines
Quoted 1 line
Can you confirm?
I tried both
Quoted 8 lines
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 toTEE_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,
[...]
Quoted 36 lines
Quoted 27 lines
Quoted 14 lines
Quoted 14 lines
Quoted 14 lines
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=ffff0008Quoted 5 lines
Quoted 5 lines
This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
Is it expected to be loaded during run-time with the help of user mode OP-TEE supplicant?
Quoted 2 lines
Quoted 2 lines
My guess is that you are trying to load fTPM TA through user mode OP-TEE
supplicant.
Quoted 3 lines
Quoted 1 line
Can you confirm?
I tried both
Quoted 8 lines
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 toTEE_ERROR_TARGET_DEAD) Since you tested it on real hardware i guess you tried both module/built-in. Which TEE version are you using?
Quoted 4 lines
I am glad that the first issue (
TEE_ERROR_ITEM_NOT_FOUND) is resolved after stitching fTPM TA as anEARLY_TA.Regarding
TEE_ERROR_TARGET_DEADerror, may I know which HW platform you are using to test?
QEMU, on armv7
Quoted 9 lines
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.
Quoted 4 lines
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.
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.
Quoted 3 lines
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
Quoted 80 lines
-----Original Message-----
From: Ilias Apalodimas <[email protected]>
Sent: Thursday, July 4, 2019 11:11 AM
To: Thirupathaiah Annapureddy <[email protected]>
Cc: Jarkko Sakkinen <[email protected]>; Sasha Levin <[email protected]>; [email protected]; [email protected]; [email protected]; linux- [email protected]; [email protected]; linux- [email protected]; Microsoft Linux Kernel List <linux- [email protected]>; Bryan Kelly (CSI) <[email protected]>; tee- [email protected]; [email protected]; [email protected]; Joakim Bech <[email protected]>
Subject: Re: [PATCH v7 1/2] fTPM: firmware TPM running in TEEHi Thirupathaiah,
[...]Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
I managed to do some quick testing in QEMU.
Everything works fine when i build this as a module (using IBM's TPM2.0
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
TSS)
- As module
# insmod /lib/modules/5.2.0-
rc1/kernel/drivers/char/tpm/tpm_ftpm_tee.koQuoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
# 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_sessionfailed,
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 1 line
err=ffff0008
This (
0xffff0008) translates toTEE_ERROR_ITEM_NOT_FOUND.Where is fTPM TA located in the your test setup?
Is it stitched into TEE binary as anEARLY_TAor
Is it expected to be loaded during run-time with the help of user modeOP-
Quoted 3 lines
Quoted 3 lines
TEE supplicant?
Quoted 2 lines
Quoted 2 lines
My guess is that you are trying to load fTPM TA through user mode OP-
TEE
Quoted 8 lines
Quoted 8 lines
supplicant.
Quoted 3 lines
Quoted 1 line
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 :)
Quoted 7 lines
Quoted 5 lines
Built as an earlyTA my error now is:
ftpm-tee firmware:optee: ftpm_tee_probe:tee_client_open_session failed, err=ffff3024 (translates toTEE_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 afterstitching
Quoted 3 lines
fTPM TA as an
EARLY_TA.Regarding
TEE_ERROR_TARGET_DEADerror, may I know which HW platform you areusing to test?
QEMU, on armv7
Quoted 6 lines
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 areusing
Quoted 1 line
built-in fTPM Linux driver, you can run into this issue as TA will try to
initialize
Quoted 4 lines
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
Quoted 7 lines
issue at all
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
If you let me know the HW platform details, I am happy to work with you to
enable/integrate
Quoted 2 lines
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.
[...]
Quoted 11 lines
Quoted 4 lines
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.
Quoted 5 lines
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
Quoted 10 lines
Quoted 9 lines
Quoted 3 lines
If you let me know the HW platform details, I am happy to work with you to
enable/integrate
Quoted 2 lines
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.
Quoted 1 line
Is it one of the 96boards?
stm32mp157c-dk2 is one of our targets.
Quoted 5 lines
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,
Quoted 24 lines · Jarkko Sakkinen
Quoted 6 lines · Sasha Levin
Quoted 2 lines
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) :-)
Quoted 8 lines
Quoted 2 lines
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.
Quoted 3 lines
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
Quoted 6 lines
I can also give an explanation in my next PR along the lines what you are saying. This would definitely work for me.
/Jarkko
Quoted 14 lines · Jarkko Sakkinen
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 7 lines · Sasha Levin
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+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.
Quoted 3 lines
+ * @chip: the
tpm_chipdescription as specified indriver/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
Quoted 12 lines · Sasha Levin
Quoted 11 lines · Jarkko Sakkinen
drivers/char/tpm/tpm_ftpm_tee.c line 39
Quoted 7 lines · Sasha Levin
drivers/char/tpm/tpm_ftpm_tee.c new file +7
-0+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.
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines
+ * @chip: the
tpm_chipdescription as specified indriver/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 particulartpm_crb.cwhich 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
PATCH v7 2/2 fTPM: add documentation for ftpm driver
In reply to Sasha LevinThis patch adds basic documentation to describe the new fTPM driver.
--- 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
Documentation/security/tpm/index.rst +1 -0
@@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree:: + tpm_ftpm_tee tpm_vtpm_proxy
Documentation/security/tpm/tpm_ftpm_tee.rst new file +31 -0
@@ -0,0 +1,31 @@ +============================================= +Firmware TPM Driver +============================================= + +| Authors: +| Thirupathaiah Annapureddy <[email protected]> +| Sasha Levin <[email protected]> + +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.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.
Signature · 1 line
2.20.1
Re: [PATCH v7 2/2] fTPM: add documentation for ftpm driver
Quoted 3 lines · Sasha Levin
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by Sasha Levin <[email protected]>
Thanks.
Quoted 54 lines
--- 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.rstDocumentation/security/tpm/index.rst +1
-0@@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree:: + tpm_ftpm_tee tpm_vtpm_proxyDocumentation/security/tpm/tpm_ftpm_tee.rst new file +31
-0@@ -0,0 +1,31 @@ +============================================= +Firmware TPM Driver +============================================= + +| Authors: +| Thirupathaiah Annapureddy <[email protected]> +| Sasha Levin <[email protected]> + +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.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.
Signature · 1 line
~Randy
Re: [PATCH v7 2/2] fTPM: add documentation for ftpm driver
In reply to Sasha LevinQuoted 27 lines · Sasha Levin
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by Sasha Levin <[email protected]>--- 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.rstDocumentation/security/tpm/index.rst +1
-0@@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree:: + tpm_ftpm_tee tpm_vtpm_proxyDocumentation/security/tpm/tpm_ftpm_tee.rst new file +0
-0
Documentation/security/tpm/tpm_ftpm_tee.rst line 7
Quoted 7 lines
Documentation/security/tpm/tpm_ftpm_tee.rst new file +7
-0@@ -0,0 +1,31 @@ +============================================= +Firmware TPM Driver +============================================= + +| Authors: +| Thirupathaiah Annapureddy <[email protected]> +| Sasha Levin <[email protected]>
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
Re: [PATCH v7 2/2] fTPM: add documentation for ftpm driver
Quoted 32 lines · Jarkko Sakkinen
Quoted 31 lines · Sasha Levin
This patch adds basic documentation to describe the new fTPM driver.
Signed-off-by Sasha Levin <[email protected]>--- 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.rstDocumentation/security/tpm/index.rst +1
-0@@ -4,4 +4,5 @@ Trusted Platform Module documentation .. toctree:: + tpm_ftpm_tee tpm_vtpm_proxyDocumentation/security/tpm/tpm_ftpm_tee.rst new file +4
-0@@ -0,0 +1,31 @@ +============================================= +Firmware TPM Driver +============================================= +
Quoted 8 lines
Documentation/security/tpm/tpm_ftpm_tee.rst line 7
Quoted 3 lines
Documentation/security/tpm/tpm_ftpm_tee.rst new file +3
-0+| Authors: +| Thirupathaiah Annapureddy <[email protected]> +| Sasha Levin <[email protected]>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