Re: [RFC 1/3] tpm: add generic platform device

[James Bottomley <jejb@linux.ibm.com>]

On Thu, 2023-01-05 at 10:08 +0200, Dov Murik wrote:
[...]
> > +static int tpm_platform_send(struct tpm_chip *chip, u8 *buf,
> > size_t
> > len)
> > +{
> > +       int ret;
> > +       struct tpm_send_cmd_req *req = (struct tpm_send_cmd_req
> > *)buffer;
> > +
> > +       if (len > TPM_PLATFORM_MAX_BUFFER - sizeof(*req))
> > +               return -EINVAL;
> > +       req->cmd = TPM_SEND_COMMAND;
> > +       req->locality = locality;
> > +       req->inbuf_size = len;
> 
> In include/linux/tpm_platform.h (below) the comment says:
> 
> + * Note that @inbuf_size must be large enough to hold the response
> so
> + * represents the maximum buffer size, not the size of the specific
> + * TPM command.
> 
> but here we don't set req->inbuf_size to the maximum size (which is
> TPM_PLATFORM_MAX_BUFFER - sizeof(*req) ). Is that OK?

I'll fix the comment.  Unfortunately the ms TPM has a check in the
ExecuteCommand() routine that checks the command and request size are
equal.  I had originally thought to use this field to communicate max
size, but it's not really any problem to make it a define.



> 
> 
> > +       memcpy(req->inbuf, buf, len);
> > +
> > +       ret = pops->sendrcv(buffer);
> > +       if (ret)
> > +               return ret;
> > +
> > +       return 0;
> > +}
> > +
> > +static int tpm_platform_recv(struct tpm_chip *chip, u8 *buf,
> > size_t
> > len)
> > +{
> > +       struct tpm_resp *resp = (struct tpm_resp *)buffer;
> > +
> > +       if (resp->size < 0)
> > +               return resp->size;
> > +
> > +       if (len < resp->size)
> > +               return -E2BIG;
> 
> 
> I suggest another check before the memcpy:
> 
> if (resp->size > TPM_PLATFORM_MAX_BUFFER - sizeof(*resp))
>         return -EINVAL;  // Invalid response from the platform TPM

Yes, I already added that because of your matrix comments.

> 
> 
> > +
> > +       memcpy(buf, buffer + sizeof(*resp), resp->size);
> > +
> > +       return resp->size;
> 
> Is there a possibility of a double recv?
> Should we set resp->size = 0 (or -1) before returning? (while still
> returning the original value)

Not according to the way ->recv() is used, but even if there were, it
should return the previous command until there is a new one.

[...]
> > +/**
> > + * struct tpm_platform_ops - the share platform operations
> > + *
> > + * @sendrecv:  Send a TPM command using the MSSIM protocol.
> > + *
> > + * The MSSIM protocol is designed for a network, so the buffers
> > are
> > + * self describing.  The minimum buffer size is sizeof(u32). 
> > Every
> > + * MSSIM command defines its own transport buffer and the command
> > is
> > + * sent in the first u32 array.  The only modification we make is
> > that
> > + * the MSSIM uses network order and we use the endianness of the
> > + * architecture.  The response to every command (in the same
> > buffer)
> > + * is a u32 size preceded array.  Most of the MSSIM commands
> > simply
> > + * return zero here because they have no defined response.
> > + *
> > + * The only command with a defined request/response size is
> > TPM_SEND_COMMAND
> > + * The definition is in the structures below
> > + */
> > +struct tpm_platform_ops {
> > +       int (*sendrcv)(u8 *buffer);
> 
> nit: In the commit message and struct description you
> use "sendrecv" but the member here is called "sendrcv".

OK, I'll; make the comments consistent (kernel docbook would complain
to the people who bother to run it).

James