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

[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?

[PATCH v2] 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. Warn before dropping it, so
a malformed firmware table leaves a trace instead of a silently
missing final events log.

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>
---

v2: Add a pr_warn() before dropping the final events log, as suggested
by Ard Biesheuvel, so a malformed firmware table leaves a trace.

 drivers/char/tpm/eventlog/efi.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 4e9d7c2bf32e..76860f9a25de 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -82,6 +82,13 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 		goto out;
 	}
 
+	if (log_tbl->final_events_preboot_size > final_events_log_size) {
+		pr_warn("UEFI TPM final events table has an oversized preboot log: %u > %d\n",
+			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

[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