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
