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

Stefano Garzarella
2024-12-12T15:30:17Z

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@linux.ibm.com/

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?

Thanks,
Stefano