[v7,1/2] fTPM: firmware TPM running in TEE

Message ID	20190625201341.15865-2-sashal@kernel.org (mailing list archive)
State	New, archived
Headers	show Return-Path: <linux-integrity-owner@kernel.org> From: Sasha Levin <sashal@kernel.org> To: peterhuewe@gmx.de, jarkko.sakkinen@linux.intel.com, jgg@ziepe.ca Cc: corbet@lwn.net, linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org, linux-integrity@vger.kernel.org, linux-kernel@microsoft.com, thiruan@microsoft.com, bryankel@microsoft.com, tee-dev@lists.linaro.org, ilias.apalodimas@linaro.org, sumit.garg@linaro.org, rdunlap@infradead.org, Sasha Levin <sashal@kernel.org> Subject: [PATCH v7 1/2] fTPM: firmware TPM running in TEE Date: Tue, 25 Jun 2019 16:13:40 -0400 Message-Id: <20190625201341.15865-2-sashal@kernel.org> In-Reply-To: <20190625201341.15865-1-sashal@kernel.org> References: <20190625201341.15865-1-sashal@kernel.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-integrity-owner@vger.kernel.org Precedence: bulk
Series	fTPM: firmware TPM running in TEE \| expand [v7,0/2] fTPM: firmware TPM running in TEE [v7,1/2] fTPM: firmware TPM running in TEE [v7,2/2] fTPM: add documentation for ftpm driver

Sasha Levin June 25, 2019, 8:13 p.m. UTC

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

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

Jarkko Sakkinen June 26, 2019, 11:31 p.m. UTC | #1

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-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
> 
> 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-implementation-tpm-chip/
> + *
> + * A reference implementation is available here:
> + * 
> 
https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM
> + */
> +
> +#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-FirmwareTPM/optee_ta/fTPM/include/fTPM.h#L42
> + */
> +

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

Sasha Levin June 26, 2019, 11:56 p.m. UTC | #2

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-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
>>
>> 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-implementation-tpm-chip/
>> + *
>> + * A reference implementation is available here:
>> + *
>>
>https://github.com/microsoft/ms-tpm-20-ref/tree/master/Samples/ARM32-FirmwareTPM/optee_ta/fTPM
>> + */
>> +
>> +#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-FirmwareTPM/optee_ta/fTPM/include/fTPM.h#L42
>> + */
>> +
>
>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 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.

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

Jarkko Sakkinen June 27, 2019, 1:17 p.m. UTC | #3

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

Jarkko Sakkinen June 27, 2019, 1:19 p.m. UTC | #4

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

Ilias Apalodimas June 27, 2019, 1:30 p.m. UTC | #5

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/tpm_ftpm_optee.c
[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
>

Jarkko Sakkinen June 27, 2019, 4:32 p.m. UTC | #6

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

Sumit Garg June 28, 2019, 5:50 a.m. UTC | #7

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
>

Sasha Levin June 29, 2019, 3:01 p.m. UTC | #8

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

Ilias Apalodimas July 2, 2019, 2:21 p.m. UTC | #9

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

Thirupathaiah Annapureddy July 2, 2019, 4:54 p.m. UTC | #10

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

Ilias Apalodimas July 3, 2019, 6:58 a.m. UTC | #11

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>

Ilias Apalodimas July 3, 2019, 8:12 a.m. UTC | #12

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

Sumit Garg July 3, 2019, 10:03 a.m. UTC | #13

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

Joakim Bech July 3, 2019, 2:16 p.m. UTC | #14

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.

Thirupathaiah Annapureddy July 4, 2019, 6:28 a.m. UTC | #15

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

Jarkko Sakkinen July 4, 2019, 9:20 a.m. UTC | #16

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

Ilias Apalodimas July 4, 2019, 6:11 p.m. UTC | #17

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. 
> 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. 
> 

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

Thirupathaiah Annapureddy July 5, 2019, 2:40 a.m. UTC | #18

> -----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.
> > 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.
> >
> 
> 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

Ilias Apalodimas July 10, 2019, 12:13 p.m. UTC | #19

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

[v7,1/2] fTPM: firmware TPM running in TEE

Commit Message

Comments

Patch