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?
Thanks,
Stefano
