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

[Jens Wiklander]

On Thu, Mar 13, 2025 at 10:13 AM Sumit Garg <sumit.garg@xxxxxxxxxx> wrote:
>
> + 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@xxxxxxxxxx>
> > ---
> > 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!

Tested-by: Jens Wiklander <jens.wiklander@xxxxxxxxxx>

Cheers,
Jens

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