[PATCH 0/6] libstub,tpm: fix small bugs and improve error reporting

[PATCH 0/6] libstub,tpm: fix small bugs and improve error reporting

[Gregory Price]

The efi/tpm code has a number of small signed/unsigned bugs and
inaccuracies are prone to cause further bugs in a difficult to
debug manner.  For example, there is a signed/unsigned mismatch
in efi/tpm.c that leads to a memblock_reserve on a range with
an effectively negative length.

Additionally, there are silently ignored error conditions that are
better explicitly reported.

Finally, there exists some bad interaction between tpm and kexec
that causes the log version and the log size to become corrupted.
The log size cannot be reasonably sanity checked, as the value is
a u32 and there is no defined max-size per the spec - however the
version can at least be sanity checked. This reports the error and
avoids calling memblock_reserve with clearly corrupted arguments.

Signed-off-by: Gregory Price <gourry@gourry.net>

Gregory Price (6):
  tpm: fix signed/unsigned bug when checking event logs
  tpm: do not ignore memblock_reserve return value
  libstub,tpm: provide indication of failure when getting event log
  tpm: sanity check the log version before using it
  tpm: fix unsigned/signed mismatch errors related to
    __calc_tpm2_event_size
  libstub,tpm: do not ignore failure case when reading final event log

 drivers/firmware/efi/libstub/tpm.c | 14 ++++++++----
 drivers/firmware/efi/tpm.c         | 34 +++++++++++++++++++++---------
 include/linux/tpm_eventlog.h       |  2 +-
 3 files changed, 35 insertions(+), 15 deletions(-)

-- 
2.43.0

[PATCH 1/6] tpm: fix signed/unsigned bug when checking event logs

[Gregory Price]

A prior bugfix that fixes a signed/unsigned error causes
another signed unsigned error.

A situation where log_tbl->size is invalid can cause the
size passed to memblock_reserve to become negative.

log_size from the main event log is an unsigned int, and
the code reduces to the following

u64 value = (int)unsigned_value;

This results in sign extension, and the value sent to
memblock_reserve becomes effectively negative.

Fixes: be59d57f9806 ("efi/tpm: Fix sanity check of unsigned tbl_size being less than zero")
Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/tpm.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index e8d69bd548f3..9c3613e6af15 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -40,7 +40,8 @@ int __init efi_tpm_eventlog_init(void)
 {
 	struct linux_efi_tpm_eventlog *log_tbl;
 	struct efi_tcg2_final_events_table *final_tbl;
-	int tbl_size;
+	unsigned int tbl_size;
+	int final_tbl_size;
 	int ret = 0;
 
 	if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
@@ -80,26 +81,26 @@ int __init efi_tpm_eventlog_init(void)
 		goto out;
 	}
 
-	tbl_size = 0;
+	final_tbl_size = 0;
 	if (final_tbl->nr_events != 0) {
 		void *events = (void *)efi.tpm_final_log
 				+ sizeof(final_tbl->version)
 				+ sizeof(final_tbl->nr_events);
 
-		tbl_size = tpm2_calc_event_log_size(events,
-						    final_tbl->nr_events,
-						    log_tbl->log);
+		final_tbl_size = tpm2_calc_event_log_size(events,
+							  final_tbl->nr_events,
+							  log_tbl->log);
 	}
 
-	if (tbl_size < 0) {
+	if (final_tbl_size < 0) {
 		pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n");
 		ret = -EINVAL;
 		goto out_calc;
 	}
 
 	memblock_reserve(efi.tpm_final_log,
-			 tbl_size + sizeof(*final_tbl));
-	efi_tpm_final_log_size = tbl_size;
+			 final_tbl_size + sizeof(*final_tbl));
+	efi_tpm_final_log_size = final_tbl_size;
 
 out_calc:
 	early_memunmap(final_tbl, sizeof(*final_tbl));
-- 
2.43.0

[PATCH 2/6] tpm: do not ignore memblock_reserve return value

[Gregory Price]

tpm code currently ignores a relevant failure case silently.
Add an error to make this failure non-silent.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/tpm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 9c3613e6af15..6e03eed0dc6f 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -61,7 +61,11 @@ int __init efi_tpm_eventlog_init(void)
 	}
 
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
-	memblock_reserve(efi.tpm_log, tbl_size);
+	if (memblock_reserve(efi.tpm_log, tbl_size)) {
+		pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
+		       efi.tpm_log, tbl_size);
+		goto out;
+	}
 
 	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
 		pr_info("TPM Final Events table not present\n");
-- 
2.43.0

Re: [PATCH 2/6] tpm: do not ignore memblock_reserve return value

[Ilias Apalodimas]

Hi Gregory,

On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
>
> tpm code currently ignores a relevant failure case silently.
> Add an error to make this failure non-silent.
>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/firmware/efi/tpm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 9c3613e6af15..6e03eed0dc6f 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -61,7 +61,11 @@ int __init efi_tpm_eventlog_init(void)
>         }
>
>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> -       memblock_reserve(efi.tpm_log, tbl_size);
> +       if (memblock_reserve(efi.tpm_log, tbl_size)) {
> +               pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> +                      efi.tpm_log, tbl_size);
> +               goto out;
> +       }

ret is going to be 0 here. I haven't followed the rest of the code and
where this function is used, but you probably need to return -ENOMEM

Thanks
/Ilias
>
>         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
>                 pr_info("TPM Final Events table not present\n");
> --
> 2.43.0
>

Re: [PATCH 2/6] tpm: do not ignore memblock_reserve return value

[Gregory Price]

On Fri, Sep 13, 2024 at 10:02:32AM +0300, Ilias Apalodimas wrote:
> Hi Gregory,
> 
> On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> >
> > tpm code currently ignores a relevant failure case silently.
> > Add an error to make this failure non-silent.
> >
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/firmware/efi/tpm.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > index 9c3613e6af15..6e03eed0dc6f 100644
> > --- a/drivers/firmware/efi/tpm.c
> > +++ b/drivers/firmware/efi/tpm.c
> > @@ -61,7 +61,11 @@ int __init efi_tpm_eventlog_init(void)
> >         }
> >
> >         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> > -       memblock_reserve(efi.tpm_log, tbl_size);
> > +       if (memblock_reserve(efi.tpm_log, tbl_size)) {
> > +               pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> > +                      efi.tpm_log, tbl_size);
> > +               goto out;
> > +       }
> 
> ret is going to be 0 here. I haven't followed the rest of the code and
> where this function is used, but you probably need to return -ENOMEM
> 

good catch, will send v2

> Thanks
> /Ilias
> >
> >         if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
> >                 pr_info("TPM Final Events table not present\n");
> > --
> > 2.43.0
> >

[PATCH 3/6] libstub,tpm: provide indication of failure when getting event log

[Gregory Price]

If get_event_log fails, at least provide an indicator of this failure
to assist debugging later failures that attempt to interact with it.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/libstub/tpm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index df3182f2e63a..192914e04e0f 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -185,8 +185,10 @@ void efi_retrieve_eventlog(void)
 			get_efi_config_table(EFI_CC_FINAL_EVENTS_TABLE_GUID);
 	}
 
-	if (status != EFI_SUCCESS || !log_location)
+	if (status != EFI_SUCCESS || !log_location) {
+		efi_err("TPM unable to provide Event Log\n");
 		return;
+	}
 
 	efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
 				   truncated, final_events_table);
-- 
2.43.0

Re: [PATCH 3/6] libstub,tpm: provide indication of failure when getting event log

[Ilias Apalodimas]

Hi Gregory,

On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
>
> If get_event_log fails, at least provide an indicator of this failure
> to assist debugging later failures that attempt to interact with it.
>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/firmware/efi/libstub/tpm.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> index df3182f2e63a..192914e04e0f 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -185,8 +185,10 @@ void efi_retrieve_eventlog(void)
>                         get_efi_config_table(EFI_CC_FINAL_EVENTS_TABLE_GUID);
>         }
>
> -       if (status != EFI_SUCCESS || !log_location)
> +       if (status != EFI_SUCCESS || !log_location) {
> +               efi_err("TPM unable to provide Event Log\n");

s/provide/retrieve/ and yes the print is going to be useful.  Do you
know if the EventLog is mandatory. Reading at the spec GetEventlog
only has 2 return values, which implies you can't return "Not
supported", but it's not explicitly stated anywhere

Thanks
/Ilias
>                 return;
> +       }
>
>         efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
>                                    truncated, final_events_table);
> --
> 2.43.0
>

Re: [PATCH 3/6] libstub,tpm: provide indication of failure when getting event log

[Gregory Price]

On Fri, Sep 13, 2024 at 09:59:03AM +0300, Ilias Apalodimas wrote:
> Hi Gregory,
> 
> On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> >
> > If get_event_log fails, at least provide an indicator of this failure
> > to assist debugging later failures that attempt to interact with it.
> >
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/firmware/efi/libstub/tpm.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > index df3182f2e63a..192914e04e0f 100644
> > --- a/drivers/firmware/efi/libstub/tpm.c
> > +++ b/drivers/firmware/efi/libstub/tpm.c
> > @@ -185,8 +185,10 @@ void efi_retrieve_eventlog(void)
> >                         get_efi_config_table(EFI_CC_FINAL_EVENTS_TABLE_GUID);
> >         }
> >
> > -       if (status != EFI_SUCCESS || !log_location)
> > +       if (status != EFI_SUCCESS || !log_location) {
> > +               efi_err("TPM unable to provide Event Log\n");
> 
> s/provide/retrieve/ and yes the print is going to be useful.  Do you
> know if the EventLog is mandatory. Reading at the spec GetEventlog
> only has 2 return values, which implies you can't return "Not
> supported", but it's not explicitly stated anywhere
> 

I believe it is mandatory from my reading of the spec - but the
"Final Event Log" was only added in 2.0.  We report an error when
2.0 is reported but the final event log is not supported, so i figure
we should probably report when the event log fails as well.

> Thanks
> /Ilias
> >                 return;
> > +       }
> >
> >         efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> >                                    truncated, final_events_table);
> > --
> > 2.43.0
> >

Re: [PATCH 3/6] libstub,tpm: provide indication of failure when getting event log

[Ilias Apalodimas]

On Fri, 13 Sept 2024 at 15:58, Gregory Price <gourry@gourry.net> wrote:
>
> On Fri, Sep 13, 2024 at 09:59:03AM +0300, Ilias Apalodimas wrote:
> > Hi Gregory,
> >
> > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> > >
> > > If get_event_log fails, at least provide an indicator of this failure
> > > to assist debugging later failures that attempt to interact with it.
> > >
> > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > ---
> > >  drivers/firmware/efi/libstub/tpm.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > > index df3182f2e63a..192914e04e0f 100644
> > > --- a/drivers/firmware/efi/libstub/tpm.c
> > > +++ b/drivers/firmware/efi/libstub/tpm.c
> > > @@ -185,8 +185,10 @@ void efi_retrieve_eventlog(void)
> > >                         get_efi_config_table(EFI_CC_FINAL_EVENTS_TABLE_GUID);
> > >         }
> > >
> > > -       if (status != EFI_SUCCESS || !log_location)
> > > +       if (status != EFI_SUCCESS || !log_location) {
> > > +               efi_err("TPM unable to provide Event Log\n");
> >
> > s/provide/retrieve/ and yes the print is going to be useful.  Do you
> > know if the EventLog is mandatory. Reading at the spec GetEventlog
> > only has 2 return values, which implies you can't return "Not
> > supported", but it's not explicitly stated anywhere
> >
>
> I believe it is mandatory from my reading of the spec - but the
> "Final Event Log" was only added in 2.0.  We report an error when
> 2.0 is reported but the final event log is not supported, so i figure
> we should probably report when the event log fails as well.

Yea I am fine with that, I was just wondering if we should do _err or
_warn. I am fine with the error

/Ilias
>
> > Thanks
> > /Ilias
> > >                 return;
> > > +       }
> > >
> > >         efi_retrieve_tcg2_eventlog(version, log_location, log_last_entry,
> > >                                    truncated, final_events_table);
> > > --
> > > 2.43.0
> > >

Re: [PATCH 3/6] libstub,tpm: provide indication of failure when getting event log

[Gregory Price]

On Fri, Sep 13, 2024 at 04:10:08PM +0300, Ilias Apalodimas wrote:
> On Fri, 13 Sept 2024 at 15:58, Gregory Price <gourry@gourry.net> wrote:
> >
> > > > -       if (status != EFI_SUCCESS || !log_location)
> > > > +       if (status != EFI_SUCCESS || !log_location) {
> > > > +               efi_err("TPM unable to provide Event Log\n");
> > >
> > > s/provide/retrieve/ and yes the print is going to be useful.  Do you
> > > know if the EventLog is mandatory. Reading at the spec GetEventlog
> > > only has 2 return values, which implies you can't return "Not
> > > supported", but it's not explicitly stated anywhere
> > >
> >
> > I believe it is mandatory from my reading of the spec - but the
> > "Final Event Log" was only added in 2.0.  We report an error when
> > 2.0 is reported but the final event log is not supported, so i figure
> > we should probably report when the event log fails as well.
> 
> Yea I am fine with that, I was just wondering if we should do _err or
> _warn. I am fine with the error
> 
> /Ilias

Per Ard's notes on patch 6 i'm going to drop this.  These prints apparently
don't actually end up anywhere.

~Gregory

[PATCH 4/6] tpm: sanity check the log version before using it

[Gregory Price]

If the log version is not sane (0 or >2), don't attempt to use
the rest of the log values for anything to avoid potential corruption.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/tpm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 6e03eed0dc6f..9a080887a3e0 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
 		return -ENOMEM;
 	}
 
+	if (!log_tbl->version ||
+	    log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
+		pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
+		       log_tbl->version);
+		early_memunmap(log_tbl, sizeof(*log_tbl));
+		efi.tpm_log = EFI_INVALID_TABLE_ADDR;
+		return -EINVAL;
+	}
+
 	tbl_size = sizeof(*log_tbl) + log_tbl->size;
 	if (memblock_reserve(efi.tpm_log, tbl_size)) {
 		pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
-- 
2.43.0

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Ilias Apalodimas]

Hi Gregory,

On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
>
> If the log version is not sane (0 or >2), don't attempt to use
> the rest of the log values for anything to avoid potential corruption.
>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/firmware/efi/tpm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 6e03eed0dc6f..9a080887a3e0 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
>                 return -ENOMEM;
>         }
>
> +       if (!log_tbl->version ||
> +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> +                      log_tbl->version);
> +               early_memunmap(log_tbl, sizeof(*log_tbl));
> +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> +               return -EINVAL;

I don't think we need this check at all. Did you actually see this happening?
efi_retrieve_eventlog() that runs during the efistub tries to retrieve
the log and the EFI protocol itself explicitly says that the firmware
*must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
2.0 format. If the firmware does something wrong, we should report the
FW BUG in that function, instead of installing the config tables Linux
uses internally to handover the log and catching it late.

Thanks
/Ilias



> +       }
> +
>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
>         if (memblock_reserve(efi.tpm_log, tbl_size)) {
>                 pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> --
> 2.43.0
>

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Gregory Price]

On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote:
> Hi Gregory,
> 
> On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> >
> > If the log version is not sane (0 or >2), don't attempt to use
> > the rest of the log values for anything to avoid potential corruption.
> >
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/firmware/efi/tpm.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > index 6e03eed0dc6f..9a080887a3e0 100644
> > --- a/drivers/firmware/efi/tpm.c
> > +++ b/drivers/firmware/efi/tpm.c
> > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
> >                 return -ENOMEM;
> >         }
> >
> > +       if (!log_tbl->version ||
> > +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> > +                      log_tbl->version);
> > +               early_memunmap(log_tbl, sizeof(*log_tbl));
> > +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > +               return -EINVAL;
> 
> I don't think we need this check at all. Did you actually see this happening?
> efi_retrieve_eventlog() that runs during the efistub tries to retrieve
> the log and the EFI protocol itself explicitly says that the firmware
> *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
> 2.0 format. If the firmware does something wrong, we should report the
> FW BUG in that function, instead of installing the config tables Linux
> uses internally to handover the log and catching it late.
> 
> Thanks
> /Ilias
>

We saw this happen and discovered it was a disagreement between EFI/OS/kexec
causing the table to be overwritten during kexec.  We've since found a fix for
that.  So the result was that it appeared the firmware was doing something
wrong. The sanity check at least allowed us to boot without immediately
crashing - because the tables don't get reinstalled, they get re-used
(at least that's by best understanding of the whole interaction).

If the check seems superfluous, i can drop it.
 
> 
> 
> > +       }
> > +
> >         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> >         if (memblock_reserve(efi.tpm_log, tbl_size)) {
> >                 pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> > --
> > 2.43.0
> >

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Ilias Apalodimas]

On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote:
>
> On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote:
> > Hi Gregory,
> >
> > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> > >
> > > If the log version is not sane (0 or >2), don't attempt to use
> > > the rest of the log values for anything to avoid potential corruption.
> > >
> > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > ---
> > >  drivers/firmware/efi/tpm.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > > index 6e03eed0dc6f..9a080887a3e0 100644
> > > --- a/drivers/firmware/efi/tpm.c
> > > +++ b/drivers/firmware/efi/tpm.c
> > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
> > >                 return -ENOMEM;
> > >         }
> > >
> > > +       if (!log_tbl->version ||
> > > +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > > +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> > > +                      log_tbl->version);
> > > +               early_memunmap(log_tbl, sizeof(*log_tbl));
> > > +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > > +               return -EINVAL;
> >
> > I don't think we need this check at all. Did you actually see this happening?
> > efi_retrieve_eventlog() that runs during the efistub tries to retrieve
> > the log and the EFI protocol itself explicitly says that the firmware
> > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
> > 2.0 format. If the firmware does something wrong, we should report the
> > FW BUG in that function, instead of installing the config tables Linux
> > uses internally to handover the log and catching it late.
> >
> > Thanks
> > /Ilias
> >
>
> We saw this happen and discovered it was a disagreement between EFI/OS/kexec
> causing the table to be overwritten during kexec.  We've since found a fix for
> that.  So the result was that it appeared the firmware was doing something
> wrong. The sanity check at least allowed us to boot without immediately
> crashing - because the tables don't get reinstalled, they get re-used
> (at least that's by best understanding of the whole interaction).
>
> If the check seems superfluous, i can drop it.

Ok, that explains why it wasn't caught earlier at least. I would
prefer dropping it tbh, but I am going to defer to Ard for that.

If we agree that this needs to go in btw, I think you should refactor
it a bit. That function already defines an out: label, which unmaps
memory. So you can rewrite the above as

If(....) {
    ret = -EINVAL;
    efi.tpm_log = EFI_INVALID_TABLE_ADDR;
   goto out;
}

Cheers
/Ilias

>
> >
> >
> > > +       }
> > > +
> > >         tbl_size = sizeof(*log_tbl) + log_tbl->size;
> > >         if (memblock_reserve(efi.tpm_log, tbl_size)) {
> > >                 pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> > > --
> > > 2.43.0
> > >

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Ard Biesheuvel]

On Fri, 13 Sept 2024 at 15:39, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote:
> >
> > On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote:
> > > Hi Gregory,
> > >
> > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> > > >
> > > > If the log version is not sane (0 or >2), don't attempt to use
> > > > the rest of the log values for anything to avoid potential corruption.
> > > >
> > > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > > ---
> > > >  drivers/firmware/efi/tpm.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > > > index 6e03eed0dc6f..9a080887a3e0 100644
> > > > --- a/drivers/firmware/efi/tpm.c
> > > > +++ b/drivers/firmware/efi/tpm.c
> > > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
> > > >                 return -ENOMEM;
> > > >         }
> > > >
> > > > +       if (!log_tbl->version ||
> > > > +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > > > +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> > > > +                      log_tbl->version);
> > > > +               early_memunmap(log_tbl, sizeof(*log_tbl));
> > > > +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > > > +               return -EINVAL;
> > >
> > > I don't think we need this check at all. Did you actually see this happening?
> > > efi_retrieve_eventlog() that runs during the efistub tries to retrieve
> > > the log and the EFI protocol itself explicitly says that the firmware
> > > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
> > > 2.0 format. If the firmware does something wrong, we should report the
> > > FW BUG in that function, instead of installing the config tables Linux
> > > uses internally to handover the log and catching it late.
> > >
> > > Thanks
> > > /Ilias
> > >
> >
> > We saw this happen and discovered it was a disagreement between EFI/OS/kexec
> > causing the table to be overwritten during kexec.  We've since found a fix for
> > that.  So the result was that it appeared the firmware was doing something
> > wrong. The sanity check at least allowed us to boot without immediately
> > crashing - because the tables don't get reinstalled, they get re-used
> > (at least that's by best understanding of the whole interaction).
> >
> > If the check seems superfluous, i can drop it.
>
> Ok, that explains why it wasn't caught earlier at least. I would
> prefer dropping it tbh, but I am going to defer to Ard for that.
>
> If we agree that this needs to go in btw, I think you should refactor
> it a bit. That function already defines an out: label, which unmaps
> memory. So you can rewrite the above as
>
> If(....) {
>     ret = -EINVAL;
>     efi.tpm_log = EFI_INVALID_TABLE_ADDR;
>    goto out;
> }
>

Validating a table that was created by the EFI stub seems redundant.
If the version check needs to be tightened, please do so in
efi_retrieve_tcg2_eventlog() (in the stub).

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Ard Biesheuvel]

On Fri, 13 Sept 2024 at 15:44, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 13 Sept 2024 at 15:39, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > On Fri, 13 Sept 2024 at 15:57, Gregory Price <gourry@gourry.net> wrote:
> > >
> > > On Fri, Sep 13, 2024 at 09:40:30AM +0300, Ilias Apalodimas wrote:
> > > > Hi Gregory,
> > > >
> > > > On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@gourry.net> wrote:
> > > > >
> > > > > If the log version is not sane (0 or >2), don't attempt to use
> > > > > the rest of the log values for anything to avoid potential corruption.
> > > > >
> > > > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > > > ---
> > > > >  drivers/firmware/efi/tpm.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> > > > > index 6e03eed0dc6f..9a080887a3e0 100644
> > > > > --- a/drivers/firmware/efi/tpm.c
> > > > > +++ b/drivers/firmware/efi/tpm.c
> > > > > @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
> > > > >                 return -ENOMEM;
> > > > >         }
> > > > >
> > > > > +       if (!log_tbl->version ||
> > > > > +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > > > > +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> > > > > +                      log_tbl->version);
> > > > > +               early_memunmap(log_tbl, sizeof(*log_tbl));
> > > > > +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > > > > +               return -EINVAL;
> > > >
> > > > I don't think we need this check at all. Did you actually see this happening?
> > > > efi_retrieve_eventlog() that runs during the efistub tries to retrieve
> > > > the log and the EFI protocol itself explicitly says that the firmware
> > > > *must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
> > > > 2.0 format. If the firmware does something wrong, we should report the
> > > > FW BUG in that function, instead of installing the config tables Linux
> > > > uses internally to handover the log and catching it late.
> > > >
> > > > Thanks
> > > > /Ilias
> > > >
> > >
> > > We saw this happen and discovered it was a disagreement between EFI/OS/kexec
> > > causing the table to be overwritten during kexec.  We've since found a fix for
> > > that.  So the result was that it appeared the firmware was doing something
> > > wrong. The sanity check at least allowed us to boot without immediately
> > > crashing - because the tables don't get reinstalled, they get re-used
> > > (at least that's by best understanding of the whole interaction).
> > >
> > > If the check seems superfluous, i can drop it.
> >
> > Ok, that explains why it wasn't caught earlier at least. I would
> > prefer dropping it tbh, but I am going to defer to Ard for that.
> >
> > If we agree that this needs to go in btw, I think you should refactor
> > it a bit. That function already defines an out: label, which unmaps
> > memory. So you can rewrite the above as
> >
> > If(....) {
> >     ret = -EINVAL;
> >     efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> >    goto out;
> > }
> >
>
> Validating a table that was created by the EFI stub seems redundant.
> If the version check needs to be tightened, please do so in
> efi_retrieve_tcg2_eventlog() (in the stub).

... and actually, this version is set by the EFI stub based on which
flavor of the TCG protocols it found.

So i don't think we need this check to begin with.

If we need to detect corruption of these tables, I'd prefer to add a
checksum or something like that. But I don't think we should bother.

Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Gregory Price]

On Fri, Sep 13, 2024 at 03:47:03PM +0200, Ard Biesheuvel wrote:
> > > If we agree that this needs to go in btw, I think you should refactor
> > > it a bit. That function already defines an out: label, which unmaps
> > > memory. So you can rewrite the above as
> > >
> > > If(....) {
> > >     ret = -EINVAL;
> > >     efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> > >    goto out;
> > > }
> > >
> >
> > Validating a table that was created by the EFI stub seems redundant.
> > If the version check needs to be tightened, please do so in
> > efi_retrieve_tcg2_eventlog() (in the stub).
> 
> ... and actually, this version is set by the EFI stub based on which
> flavor of the TCG protocols it found.
> 
> So i don't think we need this check to begin with.
> 
> If we need to detect corruption of these tables, I'd prefer to add a
> checksum or something like that. But I don't think we should bother.

Will drop, east enough.  Will send v2 later today.

~Gregory

[PATCH 5/6] tpm: fix unsigned/signed mismatch errors related to __calc_tpm2_event_size

[Gregory Price]

__calc_tpm2_event_size returns 0 or a positive length, but return values
are often interpreted as ints.  Convert everything over to u32 to avoid
signed/unsigned logic errors.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/libstub/tpm.c | 6 +++---
 drivers/firmware/efi/tpm.c         | 2 +-
 include/linux/tpm_eventlog.h       | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 192914e04e0f..4f9f0e049a7a 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -57,7 +57,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
 	struct linux_efi_tpm_eventlog *log_tbl = NULL;
 	unsigned long first_entry_addr, last_entry_addr;
 	size_t log_size, last_entry_size;
-	int final_events_size = 0;
+	u32 final_events_size = 0;
 
 	first_entry_addr = (unsigned long) log_location;
 
@@ -110,9 +110,9 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
 	 */
 	if (final_events_table && final_events_table->nr_events) {
 		struct tcg_pcr_event2_head *header;
-		int offset;
+		u32 offset;
 		void *data;
-		int event_size;
+		u32 event_size;
 		int i = final_events_table->nr_events;
 
 		data = (void *)final_events_table;
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 9a080887a3e0..7673cf8e53d6 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -19,7 +19,7 @@ EXPORT_SYMBOL(efi_tpm_final_log_size);
 static int __init tpm2_calc_event_log_size(void *data, int count, void *size_info)
 {
 	struct tcg_pcr_event2_head *header;
-	int event_size, size = 0;
+	u32 event_size, size = 0;
 
 	while (count > 0) {
 		header = data + size;
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 7d68a5cc5881..891368e82558 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -157,7 +157,7 @@ struct tcg_algorithm_info {
  * Return: size of the event on success, 0 on failure
  */
 
-static __always_inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
+static __always_inline u32 __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
 					 struct tcg_pcr_event *event_header,
 					 bool do_mapping)
 {
-- 
2.43.0

[PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

[Gregory Price]

Current code fails to check for an error case when reading events from
final event log to calculate offsets.  Check the error case, report the
error, and break early because all subsequent calls will also fail.

Signed-off-by: Gregory Price <gourry@gourry.net>
---
 drivers/firmware/efi/libstub/tpm.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index 4f9f0e049a7a..c71b0d3e66d2 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
 			event_size = __calc_tpm2_event_size(header,
 						   (void *)(long)log_location,
 						   false);
+			if (!event_size) {
+				efi_err("Invalid TPM Final Event Log Entry\n");
+				break;
+			}
 			final_events_size += event_size;
 			i--;
 		}
-- 
2.43.0

Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

[Ard Biesheuvel]

On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote:
>
> Current code fails to check for an error case when reading events from
> final event log to calculate offsets.  Check the error case, report the
> error, and break early because all subsequent calls will also fail.
>
> Signed-off-by: Gregory Price <gourry@gourry.net>
> ---
>  drivers/firmware/efi/libstub/tpm.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> index 4f9f0e049a7a..c71b0d3e66d2 100644
> --- a/drivers/firmware/efi/libstub/tpm.c
> +++ b/drivers/firmware/efi/libstub/tpm.c
> @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
>                         event_size = __calc_tpm2_event_size(header,
>                                                    (void *)(long)log_location,
>                                                    false);
> +                       if (!event_size) {
> +                               efi_err("Invalid TPM Final Event Log Entry\n");
> +                               break;
> +                       }

I don't object to this in principle, the only problem is that these
log prints are not recorded anywhere: they are printed to the EFI boot
console by the EFI stub, which may not even be visible, and is
definitely not captured by the kernel logging routines.

Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

[Gregory Price]

On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote:
> On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote:
> >
> > Current code fails to check for an error case when reading events from
> > final event log to calculate offsets.  Check the error case, report the
> > error, and break early because all subsequent calls will also fail.
> >
> > Signed-off-by: Gregory Price <gourry@gourry.net>
> > ---
> >  drivers/firmware/efi/libstub/tpm.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > index 4f9f0e049a7a..c71b0d3e66d2 100644
> > --- a/drivers/firmware/efi/libstub/tpm.c
> > +++ b/drivers/firmware/efi/libstub/tpm.c
> > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
> >                         event_size = __calc_tpm2_event_size(header,
> >                                                    (void *)(long)log_location,
> >                                                    false);
> > +                       if (!event_size) {
> > +                               efi_err("Invalid TPM Final Event Log Entry\n");
> > +                               break;
> > +                       }
> 
> I don't object to this in principle, the only problem is that these
> log prints are not recorded anywhere: they are printed to the EFI boot
> console by the EFI stub, which may not even be visible, and is
> definitely not captured by the kernel logging routines.

Could simply drop the err and break if you think it's just going to get
lost anyway.  Not sure there's a good way to generate a signal at this point.

~Gregory

Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

[Ard Biesheuvel]

On Fri, 13 Sept 2024 at 17:30, Gregory Price <gourry@gourry.net> wrote:
>
> On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote:
> > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote:
> > >
> > > Current code fails to check for an error case when reading events from
> > > final event log to calculate offsets.  Check the error case, report the
> > > error, and break early because all subsequent calls will also fail.
> > >
> > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > ---
> > >  drivers/firmware/efi/libstub/tpm.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > > index 4f9f0e049a7a..c71b0d3e66d2 100644
> > > --- a/drivers/firmware/efi/libstub/tpm.c
> > > +++ b/drivers/firmware/efi/libstub/tpm.c
> > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
> > >                         event_size = __calc_tpm2_event_size(header,
> > >                                                    (void *)(long)log_location,
> > >                                                    false);
> > > +                       if (!event_size) {
> > > +                               efi_err("Invalid TPM Final Event Log Entry\n");
> > > +                               break;
> > > +                       }
> >
> > I don't object to this in principle, the only problem is that these
> > log prints are not recorded anywhere: they are printed to the EFI boot
> > console by the EFI stub, which may not even be visible, and is
> > definitely not captured by the kernel logging routines.
>
> Could simply drop the err and break if you think it's just going to get
> lost anyway.  Not sure there's a good way to generate a signal at this point.
>

Yeah. For the record, I absolutely detest the kludgy code there, how
we parse the map, parse and unmap the event header for every entry in
the log.

So while I highly appreciate the effort you are putting in to polish
this code, I wonder if it wouldn't be better to code this up properly
instead.

Re: [PATCH 6/6] libstub,tpm: do not ignore failure case when reading final event log

[Gregory Price]

On Fri, Sep 13, 2024 at 05:59:17PM +0200, Ard Biesheuvel wrote:
> On Fri, 13 Sept 2024 at 17:30, Gregory Price <gourry@gourry.net> wrote:
> >
> > On Fri, Sep 13, 2024 at 05:25:27PM +0200, Ard Biesheuvel wrote:
> > > On Fri, 6 Sept 2024 at 22:28, Gregory Price <gourry@gourry.net> wrote:
> > > >
> > > > Current code fails to check for an error case when reading events from
> > > > final event log to calculate offsets.  Check the error case, report the
> > > > error, and break early because all subsequent calls will also fail.
> > > >
> > > > Signed-off-by: Gregory Price <gourry@gourry.net>
> > > > ---
> > > >  drivers/firmware/efi/libstub/tpm.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
> > > > index 4f9f0e049a7a..c71b0d3e66d2 100644
> > > > --- a/drivers/firmware/efi/libstub/tpm.c
> > > > +++ b/drivers/firmware/efi/libstub/tpm.c
> > > > @@ -124,6 +124,10 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
> > > >                         event_size = __calc_tpm2_event_size(header,
> > > >                                                    (void *)(long)log_location,
> > > >                                                    false);
> > > > +                       if (!event_size) {
> > > > +                               efi_err("Invalid TPM Final Event Log Entry\n");
> > > > +                               break;
> > > > +                       }
> > >
> > > I don't object to this in principle, the only problem is that these
> > > log prints are not recorded anywhere: they are printed to the EFI boot
> > > console by the EFI stub, which may not even be visible, and is
> > > definitely not captured by the kernel logging routines.
> >
> > Could simply drop the err and break if you think it's just going to get
> > lost anyway.  Not sure there's a good way to generate a signal at this point.
> >
> 
> Yeah. For the record, I absolutely detest the kludgy code there, how
> we parse the map, parse and unmap the event header for every entry in
> the log.
> 
> So while I highly appreciate the effort you are putting in to polish
> this code, I wonder if it wouldn't be better to code this up properly
> instead.

Mostly I was both amused and triggered by a patch trying to fix a
signed/unsigned bug introducing another signed/unsigned bug lol.

Then I found more and felt this injustice cannot stand x_x

I'll at least push another version of these fixed up and i'll just
drop this print. Probably improving this code isn't worth it unless
it's fundamentally broken (which it does not *appear* to be, i spent
a day auditing it against the spec, and to my eye it looks largely ok).

~Gregory