Hi Simon,
There's a v3 instead. If you have time, take a look at that, since it changes the logic On Tue, 28 Jul 2026 at 21:41, Simon Glass <[email protected]> wrote: > > Hi Shahriyar, > > On 2026-07-24T10:34:14, shj <[email protected]> wrote: > > tpm: bounds-check the device-controlled response length > > > > 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 without checking it: > > > > tpm2_get_capability() copies response_len - 15 bytes into the caller's > > buffer. A reply shorter than 15 bytes underflows the subtraction into a > > huge memcpy; a reply longer than the buffer (sized for prop_count > > properties) overruns it. > > > > tpm1_load_key2_oiap() and tpm1_get_pub_key_oiap() pass response_length > > - 41 to verify_response_auth(), which a reply shorter than 41 bytes > > underflows. > > > > 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. > > > > [...] > > > > lib/tpm-v1.c | 6 ++++++ > > lib/tpm-v2.c | 8 ++++++++ > > 2 files changed, 14 insertions(+) > > > diff --git a/lib/tpm-v2.c b/lib/tpm-v2.c > > @@ -519,6 +519,14 @@ u32 tpm2_get_capability(struct udevice *dev, u32 > > capability, u32 property, > > + if (capability == TPM2_CAP_TPM_PROPERTIES && > > + response_len - properties_off > > > + sizeof(u32) + prop_count * sizeof(struct tpms_tagged_property)) > > + return TPM_LIB_ERROR; > > + > > memcpy(buf, &response[properties_off], response_len - properties_off); > > This bounds the copy against a size derived from prop_count, not against > the real size of buf, which the function is never told. It is safe today > only because every caller happens to pass a buffer of at least > sizeof(u32) + prop_count * sizeof(struct tpms_tagged_property) bytes. The > kerneldoc does not spell that out - it describes @prop_count only as > 'Size of output buffer', which reads as a byte count rather than a > property count: > > @prop_count Size of output buffer > > So the memcpy() safety rests on an undocumented contract. Since you are > hardening this path, please can you pass the caller's actual buffer > length and bound the copy against that, and document what @prop_count > means? That also sidesteps what the bound should be for capabilities > other than TPM2_CAP_TPM_PROPERTIES, where the copy is still unbounded. > What do you think? > > Regards, > Simon
