Re: [PATCH 1/1] tpm: ignore duplicate PCR banks in tpm2_get_pcr_allocation

[Yuqi Xu <xuyuqiabc@gmail.com>]

Thank you for the review. We checked all three items against the tree this
patch was based on. Two of them describe real pre-existing validation gaps
in tpm2_get_pcr_allocation(); the third does not apply to that function.
None of them is introduced or altered by this patch, which only makes
allocated_banks[] unique by hash algorithm.

> 1) Stack out-of-bounds read in `tpm2_get_pcr_allocation()` when parsing
> TPM response due to trusting `size_of_select`.

Confirmed, and it is pre-existing. struct tpm2_pcr_selection declares
pcr_select as a fixed u8[3] (include/linux/tpm_command.h:419-423), while
size_of_select is a u8 taken verbatim from the TPM response
(drivers/char/tpm/tpm2-cmd.c:567). memchr_inv() is then called with that
attacker-controlled length over the 3-byte stack array (tpm2-cmd.c:570), so
size_of_select up to 255 makes it read up to 252 bytes past pcr_select. The
value is only used as a boolean, so this is an out-of-bounds read rather
than a direct information leak, but it is still undefined behaviour and
KASAN would flag it with a hostile TPM. It was introduced by bcfff8384f6c
("tpm: dynamically allocate the allocated_banks array"), long before this
patch.

It is a different root cause from ignoring duplicate hash algorithms,
so it is outside the scope of this series.

> 2) Heap out-of-bounds read in `tpm2_get_pcr_allocation()` due to
> insufficient buffer length validation before copying the TPM response
> payload.

The "insufficient validation" part is correct, but we do not think it is
reachable as a heap out-of-bounds read. The guard before the copy only
proves that marker + offsetof(size_of_select) is inside the response, i.e.
3 bytes (tpm2-cmd.c:560-565), while memcpy() then copies
sizeof(pcr_selection) = 6 bytes (tpm2-cmd.c:567). memcpy() can go past
end only because of that 3-byte guard versus the 6-byte copy, so the
source read exceeds the logical end by at most 3 bytes.

After a successful tpm_try_transmit(), len equals header->length
(tpm-interface.c:202), and tpm_transmit_cmd() stores that value in
buf->length. rsp_len taken from the header is therefore the same length,
not a weaker bound than buf->length.

The loop is capped at nr_possible_banks <= TPM2_MAX_PCR_BANKS = 8
(tpm2-cmd.c:548), with marker advancing by at most 3 + 255 per iteration.
The last memcpy source is therefore at offset 1825; after the loop marker
is at most 2083, still well inside the TPM_BUFSIZE (4096) kzalloc buffer,
so the read never leaves the allocation. The over-read is limited to the
area between the logical response end and the end of the same buffer.

This guard also predates the patch (1db15344f874).

> 3) Function mixes goto-based error handling with `__free()` scope-based
> cleanup, violating subsystem guidelines.

We could not find such a mix in tpm2_get_pcr_allocation(). The function has
no goto: it uses only break and return (tpm2-cmd.c:534, 544, 551, 597),
with __free(kfree) on buf as the only cleanup construct.

To summarize: item 1 is a real pre-existing out-of-bounds read. For item 2
the insufficient guard is real, but a heap out-of-bounds read is not
reachable. Item 3 does not apply to this function. This series is
intentionally limited to ignoring duplicate hash algorithms so that
nr_allocated_banks, and therefore groups_cnt, always equals the number of
distinct algorithms the TPM reports.

Best regards,
Yuqi Xu