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.
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/
Regards,
James
