On Thu, Dec 12, 2024 at 10:41:40AM -0500, James Bottomley wrote:
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.
I see, but we alread check that `len < resp->size` in
tpm_platform_recv(), so on second glance, for the current
implementation, maybe it's a duplicate check.
This because in tpm_platform_send() we return an error if `len >
TPM_PLATFORM_MAX_BUFFER - sizeof(*req)` and here, in
tpm_platform_recv(), we already return an error if `len < resp->size`.
IIUC buf/len are the same for send() and recv(), so the `resp->size >
TPM_PLATFORM_MAX_BUFFER - sizeof(*resp)` case would already be covered,
right?
Anyway this code will change a bit in v2 if we implement the send_recv()
op for tpm_class_ops, so I'll be sure to take care of this case.
Thanks,
Stefano
