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

[James Bottomley]

On Thu, 2024-12-12 at 16:30 +0100, Stefano Garzarella wrote:
> On Thu, Dec 12, 2024 at 09:35:46AM -0500, James Bottomley wrote:
> > On Thu, 2024-12-12 at 10:51 +0100, Stefano Garzarella wrote:
> > > On Tue, Dec 10, 2024 at 03:34:21PM +0100, Stefano Garzarella
> > > wrote:
> > [...]
> > > > +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;
> > > 
> > > While reviewing Oliver's work for the driver in edk2[1], I
> > > noticed that there wasn't this check and asked to add it, but
> > > talking to him and looking in the code/spec, we realized that
> > > it's strange that tpm_resp.size field is signed.
> > > 
> > >  From SVSM spec it looks like it can't be negative:
> > > 
> > >      Table 17: TPM_SEND_COMMAND Response Structure
> > > 
> > >      Byte     Size        Meaning
> > >      Offset   (Bytes)
> > >      0x000    4           Response size (in bytes)
> > >      0x004    Variable    Variable Response
> > > 
> > > And also Coconut SVSM remap it to the `responseSize` of the TCG
> > > TPM implementation which is unsigned:
> > > 
> > >      LIB_EXPORT void _plat__RunCommand(
> > >          uint32_t        requestSize,   // IN: command buffer size
> > >          unsigned char*  request,       // IN: command buffer
> > >          uint32_t*       responseSize,  // IN/OUT: response buffer
> > > size
> > >          unsigned char** response       // IN/OUT: response buffer
> > >      )
> > > 
> > > @James, @Claudio, @Tom, should we use u32 for tpm_resp.size?
> > 
> > The original idea was to allow the protocol to return an error
> > (like out of memory or something) before the command ever got to
> > the TPM rather than having to wrap it up in a TPM error.  However,
> > that's done in the actual return from the SVSM call, which the
> > sendrecv routine checks, so I agree this can be removed and a u32
> > done for the length.
> 
> Thanks for the details!
> I'll fix it in v2 and put a comment also in the edk2 PR.
> 
> > Dov did recommend we should check the returned length against the
> > maximum allowable:
> > 
> > https://lore.kernel.org/linux-coco/f7d0bd07-ba1b-894e-5e39-15fb1817bc8b@xxxxxxxxxxxxx/
> 
> I added in this version the check he suggested:
> 
>         if (resp->size > TPM_PLATFORM_MAX_BUFFER - sizeof(*resp))
>                 return -EINVAL;  // Invalid response from the
> platform TPM
> 
> Were you referring to that?

Yes, the theory being that we're required to provide a buffer of this
length for the response, but if someone can inject a bogus response
they could induce us to copy beyond the end of the buffer we provided.

Regards,

James