On Thu, Jun 04, 2026 at 10:53:47AM +0800, [email protected] wrote:
> From: Zongyao Chen <[email protected]>
>
> The TPM2 firmware event log buffer is a half-open range:
> [bios_event_log, bios_event_log_end). An entry ending exactly at
> bios_event_log_end is still inside the buffer; only an entry extending
> past that address is malformed.
>
> The TPM2 seq_file iterator did not handle this boundary consistently.
> The TCG_EfiSpecIdEvent header had to satisfy "addr + size < limit".
> Later events were rejected when "addr + size >= limit". Firmware that
> packs the final measurement tightly at the end of the log can therefore
> lose that measurement. If it is the first measurement after the spec ID
> header, binary_bios_measurements shows only the header.
>
> This has been observed on bare-metal systems whose UEFI enables the SM3
> PCR bank, but the bug is not SM3-specific. Any tightly packed TPM2 log
> whose final event ends at bios_event_log_end can hit it.
>
> Accept entries that end exactly at the log boundary by rejecting only
> "addr + size > limit". An accepted boundary entry has its last byte at
> limit - 1, so this does not allow reading past the buffer. Keep
> zero-length entries rejected.
>
> Also treat addr >= limit as EOF in tpm2_bios_measurements_start().
> After seq_file restarts from a later position, start() can scan past a
> valid final entry and leave addr equal to bios_event_log_end. That
> address is the end marker, not another event header.
>
> Leave the "marker >= limit" check in tpm2_bios_measurements_next()
> unchanged. There, marker is already the start of the next event, so
> "marker == limit" means EOF.
This is the most unclear bug description I've read for a long
time. Please explain what's the problem in simple teerms and
how this solves this. Mixing up pseudo-code and text does not
help.
>
> Fixes: 4d23cc323cdb ("tpm: add securityfs support for TPM 2.0 firmware event
> log")
> Signed-off-by: Zongyao Chen <[email protected]>
> ---
> drivers/char/tpm/eventlog/tpm2.c | 25 ++++++++++++++++---------
> 1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/char/tpm/eventlog/tpm2.c
> b/drivers/char/tpm/eventlog/tpm2.c
> index 37a05800980c..6b65d872e43a 100644
> --- a/drivers/char/tpm/eventlog/tpm2.c
> +++ b/drivers/char/tpm/eventlog/tpm2.c
> @@ -54,31 +54,38 @@ static void *tpm2_bios_measurements_start(struct seq_file
> *m, loff_t *pos)
> size = struct_size(event_header, event, event_header->event_size);
>
> if (*pos == 0) {
> - if (addr + size < limit) {
> - if ((event_header->event_type == 0) &&
> - (event_header->event_size == 0))
> - return NULL;
> - return SEQ_START_TOKEN;
> - }
> + if (addr + size > limit)
> + return NULL;
> + if (event_header->event_type == 0 &&
> + event_header->event_size == 0)
> + return NULL;
> + return SEQ_START_TOKEN;
This looks unnecessary turnover. Please rethink. We should be minizing
the diff for bug fixes, not the other way around.
> }
>
> if (*pos > 0) {
> addr += size;
> + if (addr >= limit)
> + return NULL;
> event = addr;
> size = calc_tpm2_event_size(event, event_header);
> - if ((addr + size >= limit) || (size == 0))
> + if ((addr + size > limit) || size == 0)
> return NULL;
> }
>
> for (i = 0; i < (*pos - 1); i++) {
> + if (addr >= limit)
> + return NULL;
> event = addr;
> size = calc_tpm2_event_size(event, event_header);
>
> - if ((addr + size >= limit) || (size == 0))
> + if ((addr + size > limit) || size == 0)
> return NULL;
> addr += size;
> }
>
> + if (addr >= limit)
> + return NULL;
> +
> return addr;
> }
>
> @@ -115,7 +122,7 @@ static void *tpm2_bios_measurements_next(struct seq_file
> *m, void *v,
> event = v;
>
> event_size = calc_tpm2_event_size(event, event_header);
> - if (((v + event_size) >= limit) || (event_size == 0))
> + if (((v + event_size) > limit) || event_size == 0)
> return NULL;
>
> return v;
> --
> 2.47.3
>
BR, Jarkko
