Re: [PATCH] tpm: eventlog: tpm2: allow event log entries ending at the log boundary

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, Jun 04, 2026 at 10:53:47AM +0800, ZongYao.Chen@linux.alibaba.com wrote:
> From: Zongyao Chen <ZongYao.Chen@linux.alibaba.com>
> 
> 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 <ZongYao.Chen@linux.alibaba.com>
> ---
>  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