Re: [PATCH v4 1/2] tpm: bounds-check the device-reported response length

Hi Shahriyar,


On Wed, 29 Jul 2026 at 21:25, Shahriyar Jalayeri
<[email protected]> wrote:
>
> The length of a TPM reply is set by the device: tpm_sendrecv_command()
> stores the number of bytes received, caps it only at the command buffer
> size, and reports success whenever the reply's return code is 0. Callers
> then use that length unchecked. The clearest damage is in the parsers that
> compute response_length - <header> and index the result (the TPM1 OIAP
> helpers and tpm2_get_capability): a reply shorter than the header
> underflows the subtraction into a huge memcpy.
>
> Rather than guard each caller, check the length once at the choke point.
> tpm_sendrecv_command() gains a min_response_len argument and rejects a
> reply shorter than the caller says it needs, before the caller parses it;
> every command that reads a response now declares its minimum.
>
> tpm2_get_capability() additionally copies response_len - 15 bytes into the
> caller's buffer, but was never told how big that buffer is, so a reply
> longer than it overruns it. Give the function a buf_size argument and
> reject a reply that would not fit, so the copy is bounded for every
> capability rather than for the properties query alone.
>
> A TPM sits on a discrete SPI/I2C/LPC bus that is physically accessible, so
> a reply this malformed is reachable by a bus interposer or a faulty part;
> on the TPM2 path this parsing runs during measured boot.
>

[...]

>  }
>
>  u32 tpm1_end_oiap(struct udevice *dev)
> @@ -695,7 +702,8 @@ u32 tpm1_oiap(struct udevice *dev, u32 *auth_handle)
>         if (oiap_session.valid)
>                 tpm1_terminate_auth_session(dev, oiap_session.handle);
>
> -       err = tpm_sendrecv_command(dev, command, response, &response_length);
> +       /* tag(2) + size(4) + rc(4) + authHandle(4) + nonceEven(20) = 34 */
> +       err = tpm_sendrecv_command(dev, command, response, &response_length, 34);
>         if (err)
>                 return err;
>         if (unpack_byte_string(response, response_length, "ds",
> @@ -750,7 +758,8 @@ u32 tpm1_load_key2_oiap(struct udevice *dev, u32 parent_handle, const void *key,
>                                   parent_key_usage_auth);
>         if (err)
>                 return err;
> -       err = tpm_sendrecv_command(dev, request, response, &response_length);
> +       /* nonceEven(20) + continueAuthSession(1) + resAuth(20) = 41 */
> +       err = tpm_sendrecv_command(dev, request, response, &response_length, 41);

The load doesn't seem to factor in the header len which the commit
message mentions. The code later on does "response_length -
TPM_RESPONSE_AUTH_LENGTH", so it should cause any issues, but we
should keep the same policy everywhere.
Do you know if loading the key includes the header in the response?

[...]


Cheers
/Ilias