Re: [RFC PATCH 2/3] tpm/tpm_ftpm_tee: use send_recv() op

[Sumit Garg <sumit.garg@kernel.org>]

+ Jens

Hi Stefano,

On Tue, Mar 11, 2025 at 11:01:29AM +0100, Stefano Garzarella wrote:
> This driver does not support interrupts, and receiving the response is
> synchronous with sending the command.
> 
> It used an internal buffer to cache the response when .send() is called,
> and then return it when .recv() is called.
> 
> Let's simplify the driver by implementing the new send_recv() op, so that
> we can also remove the 4KB internal buffer used to cache the response.

Looks like a nice cleanup to me but it needs to be tested. Jens, can you
give this patch a try?

> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> Note: I don't know how to test this driver, so I just build it.
> If someone can test it, or tell me how to do, it will be great!

The fTPM is now maintained as part of OP-TEE project here [1]. The
instructions to test it on Qemu can be found here [2] as part of CI
pipeline.

[1] https://github.com/OP-TEE/optee_ftpm
[2] https://github.com/OP-TEE/optee_ftpm/blob/master/.github/workflows/ci.yml

-Sumit

> ---
>  drivers/char/tpm/tpm_ftpm_tee.h |  4 --
>  drivers/char/tpm/tpm_ftpm_tee.c | 86 ++++++++-------------------------
>  2 files changed, 21 insertions(+), 69 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.h b/drivers/char/tpm/tpm_ftpm_tee.h
> index f98daa7bf68c..72b2f5c41274 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.h
> +++ b/drivers/char/tpm/tpm_ftpm_tee.h
> @@ -23,16 +23,12 @@
>   * @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;
>  };
> diff --git a/drivers/char/tpm/tpm_ftpm_tee.c b/drivers/char/tpm/tpm_ftpm_tee.c
> index 139556b21cc6..f0393d843780 100644
> --- a/drivers/char/tpm/tpm_ftpm_tee.c
> +++ b/drivers/char/tpm/tpm_ftpm_tee.c
> @@ -31,45 +31,19 @@ static const uuid_t ftpm_ta_uuid =
>  		  0x82, 0xCB, 0x34, 0x3F, 0xB7, 0xF3, 0x78, 0x96);
>  
>  /**
> - * ftpm_tee_tpm_op_recv() - retrieve fTPM response.
> - * @chip:	the tpm_chip description as specified in driver/char/tpm/tpm.h.
> - * @buf:	the buffer to store data.
> - * @count:	the number of bytes to read.
> - *
> - * Return:
> - *	In case of success the number of bytes received.
> - *	On failure, -errno.
> - */
> -static int ftpm_tee_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> -{
> -	struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
> -	size_t len;
> -
> -	len = pvt_data->resp_len;
> -	if (count < len) {
> -		dev_err(&chip->dev,
> -			"%s: Invalid size in recv: count=%zd, resp_len=%zd\n",
> -			__func__, count, len);
> -		return -EIO;
> -	}
> -
> -	memcpy(buf, pvt_data->resp_buf, len);
> -	pvt_data->resp_len = 0;
> -
> -	return len;
> -}
> -
> -/**
> - * ftpm_tee_tpm_op_send() - send TPM commands through the TEE shared memory.
> + * ftpm_tee_tpm_op_send_recv() - send TPM commands through the TEE shared memory
> + * and retrieve the response.
>   * @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.
> + * @buf:	the buffer to send and to store the response.
> + * @buf_len:	the size of the buffer.
> + * @cmd_len:	the number of bytes to send.
>   *
>   * Return:
> - *	In case of success, returns 0.
> + *	In case of success, returns the number of bytes received.
>   *	On failure, -errno
>   */
> -static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
> +static int ftpm_tee_tpm_op_send_recv(struct tpm_chip *chip, u8 *buf,
> +				     size_t buf_len, size_t cmd_len)
>  {
>  	struct ftpm_tee_private *pvt_data = dev_get_drvdata(chip->dev.parent);
>  	size_t resp_len;
> @@ -80,16 +54,15 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	struct tee_param command_params[4];
>  	struct tee_shm *shm = pvt_data->shm;
>  
> -	if (len > MAX_COMMAND_SIZE) {
> +	if (cmd_len > MAX_COMMAND_SIZE) {
>  		dev_err(&chip->dev,
>  			"%s: len=%zd exceeds MAX_COMMAND_SIZE supported by fTPM TA\n",
> -			__func__, len);
> +			__func__, cmd_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) {
> @@ -103,7 +76,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  		.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INPUT,
>  		.u.memref = {
>  			.shm = shm,
> -			.size = len,
> +			.size = cmd_len,
>  			.shm_offs = 0,
>  		},
>  	};
> @@ -115,7 +88,7 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  		return PTR_ERR(temp_buf);
>  	}
>  	memset(temp_buf, 0, (MAX_COMMAND_SIZE + MAX_RESPONSE_SIZE));
> -	memcpy(temp_buf, buf, len);
> +	memcpy(temp_buf, buf, cmd_len);
>  
>  	command_params[1] = (struct tee_param) {
>  		.attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT,
> @@ -156,38 +129,21 @@ static int ftpm_tee_tpm_op_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  			__func__, resp_len);
>  		return -EIO;
>  	}
> +	if (resp_len > buf_len) {
> +		dev_err(&chip->dev,
> +			"%s: Invalid size in recv: buf_len=%zd, resp_len=%zd\n",
> +			__func__, buf_len, 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;
> -}
> +	memcpy(buf, temp_buf, resp_len);
>  
> -static bool ftpm_tee_tpm_req_canceled(struct tpm_chip *chip, u8 status)
> -{
> -	return false;
> +	return resp_len;
>  }
>  
>  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,
> +	.send_recv = ftpm_tee_tpm_op_send_recv,
>  };
>  
>  /*
> -- 
> 2.48.1
> 
>