Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Guo Zihao]

Hi Jarkko,

No bus interposer is involved: the threat here is firmware shipping a
self-contradictory final events table (preboot size larger than the
table itself), not tampering on the wire. So the quantitative
question - does this actually happen - does have data.

This area of efi.c has already needed fixes for real firmware tables
four times:

- 05c8c1ff81ed ("efi/tpm: Don't traverse an event log with no
  events"): some machines return an unpopulated template table whose
  contents are just garbage.

- b4f1874c6216 ("tpm: check event log version before reading final
  events"): several Dell models froze at boot because the firmware
  event log and final events table did not match each other.
  Fixes: includes 166a2809d65b.

- 80a6c216b16d ("tpm/eventlog: Don't abort tpm_read_log on faulty
  ACPI address"): a Framework laptop with an INSYDE BIOS shipped an
  un-mappable TPM2 ACPI table.

- 48cff270b037 ("tpm: efi: Use local variable for calculating final
  log size"): this one is the closest. The same subtraction went
  negative and hit __memcpy with a wrapped size, with a real oops in
  the commit message. It carries Fixes: 166a2809d65b and went to
  stable.

So the corrupted-table input and the negative-size memcpy path have
both been observed in the wild, just not through this exact
combination yet.

I'm fine either way on the tag. If you still consider this one
hardening, I'll send a v3 without Fixes - the change itself is not
affected by that.

Thanks.

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Jarkko Sakkinen]

On Tue, Nov 10, 2026 at 11:08:29AM +0800, Guo Zihao wrote:
> Hi Jarkko,
> 
> No bus interposer is involved: the threat here is firmware shipping a
> self-contradictory final events table (preboot size larger than the
> table itself), not tampering on the wire. So the quantitative
> question - does this actually happen - does have data.
> 
> This area of efi.c has already needed fixes for real firmware tables
> four times:
> 
> - 05c8c1ff81ed ("efi/tpm: Don't traverse an event log with no
>   events"): some machines return an unpopulated template table whose
>   contents are just garbage.
> 
> - b4f1874c6216 ("tpm: check event log version before reading final
>   events"): several Dell models froze at boot because the firmware
>   event log and final events table did not match each other.
>   Fixes: includes 166a2809d65b.
> 
> - 80a6c216b16d ("tpm/eventlog: Don't abort tpm_read_log on faulty
>   ACPI address"): a Framework laptop with an INSYDE BIOS shipped an
>   un-mappable TPM2 ACPI table.
> 
> - 48cff270b037 ("tpm: efi: Use local variable for calculating final
>   log size"): this one is the closest. The same subtraction went
>   negative and hit __memcpy with a wrapped size, with a real oops in
>   the commit message. It carries Fixes: 166a2809d65b and went to
>   stable.
> 
> So the corrupted-table input and the negative-size memcpy path have
> both been observed in the wild, just not through this exact
> combination yet.
> 
> I'm fine either way on the tag. If you still consider this one
> hardening, I'll send a v3 without Fixes - the change itself is not
> affected by that.
> 
> Thanks.

OK, I think you are making a fair argument.

This is not just pure hardening given the various possibilities for
pure software (firmware) mishaps.

Maybe v3 with a reminder of why this crossed the border?

That said, I think I agree with you now.

Br, Jarkko

[PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Guo Zihao]

The preboot portion recorded in the main event log is subtracted from
the size of the final events table taken from the firmware:

	/*
	 * The 'final events log' size excludes the 'final events preboot log'
	 * at its beginning.
	 */
	final_events_log_size -= log_tbl->final_events_preboot_size;

Both sizes come from EFI configuration tables and are not validated
against each other. A final events table claiming a preboot portion
larger than the table itself makes final_events_log_size go negative:
the subsequent devm_krealloc() can then end up with a wrapped, too
small allocation, and the memcpy() that appends the final events log
uses the negative value as its length, running far past the buffer.

The boot path has no way to fix up such a table, so fail the final
events log instead of appending from it.

Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event log in the TCG2 log")
Cc: stable@vger.kernel.org
Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
---
 drivers/char/tpm/eventlog/efi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 4e9d7c2bf..0a8006f3e 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -82,6 +82,9 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 		goto out;
 	}
 
+	if (log_tbl->final_events_preboot_size > final_events_log_size)
+		goto out;
+
 	/*
 	 * The 'final events log' size excludes the 'final events preboot log'
 	 * at its beginning.
-- 
2.50.1

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Ard Biesheuvel]

On Fri, 6 Nov 2026, at 02:49, Guo Zihao wrote:
> The preboot portion recorded in the main event log is subtracted from
> the size of the final events table taken from the firmware:
>
> 	/*
> 	 * The 'final events log' size excludes the 'final events preboot log'
> 	 * at its beginning.
> 	 */
> 	final_events_log_size -= log_tbl->final_events_preboot_size;
>
> Both sizes come from EFI configuration tables and are not validated
> against each other. A final events table claiming a preboot portion
> larger than the table itself makes final_events_log_size go negative:
> the subsequent devm_krealloc() can then end up with a wrapped, too
> small allocation, and the memcpy() that appends the final events log
> uses the negative value as its length, running far past the buffer.
>
> The boot path has no way to fix up such a table, so fail the final
> events log instead of appending from it.
>
> Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event 
> log in the TCG2 log")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>
> ---
>  drivers/char/tpm/eventlog/efi.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 4e9d7c2bf..0a8006f3e 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -82,6 +82,9 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  		goto out;
>  	}
> 
> +	if (log_tbl->final_events_preboot_size > final_events_log_size)
> +		goto out;
> +

Should we add a pr_warn() here?

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Jarkko Sakkinen]

On Fri, Nov 06, 2026 at 09:49:56AM +0800, Guo Zihao wrote:
> The preboot portion recorded in the main event log is subtracted from
> the size of the final events table taken from the firmware:
> 
> 	/*
> 	 * The 'final events log' size excludes the 'final events preboot log'
> 	 * at its beginning.
> 	 */
> 	final_events_log_size -= log_tbl->final_events_preboot_size;
> 
> Both sizes come from EFI configuration tables and are not validated
> against each other. A final events table claiming a preboot portion
> larger than the table itself makes final_events_log_size go negative:
> the subsequent devm_krealloc() can then end up with a wrapped, too
> small allocation, and the memcpy() that appends the final events log
> uses the negative value as its length, running far past the buffer.
> 
> The boot path has no way to fix up such a table, so fail the final
> events log instead of appending from it.
> 
> Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event log in the TCG2 log")
> Cc: stable@vger.kernel.org
> Signed-off-by: Guo Zihao <guozh23@xiaopeng.com>

Ditto.

For all four address validation vs bug fix intersection. I did not
check throughly enough to be exactly sure of all four.

Br, Jarkko

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Guo Zihao]

On Fri, Nov 06, 2026 at 03:24:39PM +0000, Jarkko Sakkinen wrote:
> Ditto.
>
> For all four address validation vs bug fix intersection. I did not
> check throughly enough to be exactly sure of all four.

For this one I'd like to argue the bug fix case, since the data being
validated is not something the TPM reports over a TCG-defined
protocol: both sizes come from EFI configuration tables written by
firmware, and nothing in the TCG spec constrains what firmware puts
there.

The firmware data is already treated as untrusted in the same
function: it bails out on an empty log (log_tbl->size == 0), on a
missing final log address, and on a wrong log format version. The
unchecked subtraction is the odd one out.

The consequence is also plain memory corruption rather than a
graceful rejection: final_events_log_size goes negative, the
following devm_krealloc() gets a wrapped size, and the memcpy()
appending the final events log runs with a negative length.

And the out-of-bounds write was introduced by 166a2809d65b ("tpm:
Don't duplicate events from the final event log in the TCG2 log"):
before that commit nothing subtracted the preboot size, so a
malformed table was harmless. The Fixes: tag refers to that
regression, not to the malformed table itself.

Ard also had a look at v2 and only asked for a pr_warn() on the
rejection path, which is in.

So I'd like to keep the Fixes: tag here, but I'm happy to drop it if
you disagree - your tree, your call.

Thanks.

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Jarkko Sakkinen]

On Mon, Nov 09, 2026 at 09:49:34AM +0800, Guo Zihao wrote:
> On Fri, Nov 06, 2026 at 03:24:39PM +0000, Jarkko Sakkinen wrote:
> > Ditto.
> >
> > For all four address validation vs bug fix intersection. I did not
> > check throughly enough to be exactly sure of all four.
> 
> For this one I'd like to argue the bug fix case, since the data being
> validated is not something the TPM reports over a TCG-defined
> protocol: both sizes come from EFI configuration tables written by
> firmware, and nothing in the TCG spec constrains what firmware puts
> there.
> 
> The firmware data is already treated as untrusted in the same
> function: it bails out on an empty log (log_tbl->size == 0), on a
> missing final log address, and on a wrong log format version. The
> unchecked subtraction is the odd one out.
> 
> The consequence is also plain memory corruption rather than a
> graceful rejection: final_events_log_size goes negative, the
> following devm_krealloc() gets a wrapped size, and the memcpy()
> appending the final events log runs with a negative length.
> 
> And the out-of-bounds write was introduced by 166a2809d65b ("tpm:
> Don't duplicate events from the final event log in the TCG2 log"):
> before that commit nothing subtracted the preboot size, so a
> malformed table was harmless. The Fixes: tag refers to that
> regression, not to the malformed table itself.
> 
> Ard also had a look at v2 and only asked for a pr_warn() on the
> rejection path, which is in.
> 
> So I'd like to keep the Fixes: tag here, but I'm happy to drop it if
> you disagree - your tree, your call.
> 
> Thanks.

Let's take quantitative approach instead.

OK, so does this actually happen? I'm weighting this based on that
mainly. Usually bus corruption is not very common when it comes to
TPMs because they are usually either soldered or in the firmware.

Is there something I'm missing out? I don't think bus interposer
type of security scenario is common enough to use it as a rationale.

Br, Jarkko

Re: [PATCH] tpm: efi: reject a final events table with an oversized preboot log

[Jarkko Sakkinen]

On Mon, Nov 09, 2026 at 04:43:23PM +0200, Jarkko Sakkinen wrote:
> On Mon, Nov 09, 2026 at 09:49:34AM +0800, Guo Zihao wrote:
> > On Fri, Nov 06, 2026 at 03:24:39PM +0000, Jarkko Sakkinen wrote:
> > > Ditto.
> > >
> > > For all four address validation vs bug fix intersection. I did not
> > > check throughly enough to be exactly sure of all four.
> > 
> > For this one I'd like to argue the bug fix case, since the data being
> > validated is not something the TPM reports over a TCG-defined
> > protocol: both sizes come from EFI configuration tables written by
> > firmware, and nothing in the TCG spec constrains what firmware puts
> > there.
> > 
> > The firmware data is already treated as untrusted in the same
> > function: it bails out on an empty log (log_tbl->size == 0), on a
> > missing final log address, and on a wrong log format version. The
> > unchecked subtraction is the odd one out.
> > 
> > The consequence is also plain memory corruption rather than a
> > graceful rejection: final_events_log_size goes negative, the
> > following devm_krealloc() gets a wrapped size, and the memcpy()
> > appending the final events log runs with a negative length.
> > 
> > And the out-of-bounds write was introduced by 166a2809d65b ("tpm:
> > Don't duplicate events from the final event log in the TCG2 log"):
> > before that commit nothing subtracted the preboot size, so a
> > malformed table was harmless. The Fixes: tag refers to that
> > regression, not to the malformed table itself.
> > 
> > Ard also had a look at v2 and only asked for a pr_warn() on the
> > rejection path, which is in.
> > 
> > So I'd like to keep the Fixes: tag here, but I'm happy to drop it if
> > you disagree - your tree, your call.
> > 
> > Thanks.
> 
> Let's take quantitative approach instead.
> 
> OK, so does this actually happen? I'm weighting this based on that
> mainly. Usually bus corruption is not very common when it comes to
> TPMs because they are usually either soldered or in the firmware.
> 
> Is there something I'm missing out? I don't think bus interposer
> type of security scenario is common enough to use it as a rationale.

I do think that the change is for better. I'm disregarding its value
by saying this :-)

There's so many bug incoming that we need to be just somewhat rigid on
what we consider as a bug fix per se and what not.

Br, Jarkko