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

[PATCH v2 0/4] 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 can lead to calling memblock_reserve on a range
with an effectively negative length.

Additionally, there are silently ignored error conditions may
result in undefined behavior.  Address these.

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

Gregory Price (4):
  tpm: fix signed/unsigned bug when checking event logs
  tpm: do not ignore memblock_reserve return value
  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 |  9 ++++++---
 drivers/firmware/efi/tpm.c         | 26 ++++++++++++++++----------
 include/linux/tpm_eventlog.h       |  2 +-
 3 files changed, 23 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v2 1/4] 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

Re: [PATCH v2 1/4] tpm: fix signed/unsigned bug when checking event logs

[Ilias Apalodimas]

On Sat, 14 Sept 2024 at 02:20, Gregory Price <gourry@gourry.net> wrote:
>
> 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;

nit, perhaps u32 is better here

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

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v2 2/4] 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 | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 9c3613e6af15..b0cc2cc11d7e 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -61,7 +61,12 @@ 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);
+		ret = -ENOMEM;
+		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 v2 2/4] tpm: do not ignore memblock_reserve return value

[Ard Biesheuvel]

(cc Dave)

On Sat, 14 Sept 2024 at 15:26, 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 | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 9c3613e6af15..b0cc2cc11d7e 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -61,7 +61,12 @@ 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);
> +               ret = -ENOMEM;
> +               goto out;
> +       }
>

Given the discussion in the other thread, I wonder if this should be
efi_mem_reserve() instead - might as well fix that too.

Dave?

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

[Usama Arif]

On 16/09/2024 09:29, Ard Biesheuvel wrote:
> (cc Dave)
> 
> On Sat, 14 Sept 2024 at 15:26, 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 | 7 ++++++-
>>  1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
>> index 9c3613e6af15..b0cc2cc11d7e 100644
>> --- a/drivers/firmware/efi/tpm.c
>> +++ b/drivers/firmware/efi/tpm.c
>> @@ -61,7 +61,12 @@ 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);
>> +               ret = -ENOMEM;
>> +               goto out;
>> +       }
>>
> 
> Given the discussion in the other thread, I wonder if this should be
> efi_mem_reserve() instead - might as well fix that too.
> 
> Dave?

I dont believe efi_mem_reserve is needed after your patch in
https://lore.kernel.org/all/20240912155159.1951792-2-ardb+git@google.com/

which will cover both kexec_load and kexec_file_load cases.

Thanks,
Usama

[PATCH v2 3/4] 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 df3182f2e63a..f194e43f00ad 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 b0cc2cc11d7e..cdd431027065 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 v2 4/4] 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,
and break early because all subsequent calls will also fail.

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

diff --git a/drivers/firmware/efi/libstub/tpm.c b/drivers/firmware/efi/libstub/tpm.c
index f194e43f00ad..8e04aaf428d0 100644
--- a/drivers/firmware/efi/libstub/tpm.c
+++ b/drivers/firmware/efi/libstub/tpm.c
@@ -124,6 +124,9 @@ 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 calc fails this is a malformed log */
+			if (!event_size)
+				break;
 			final_events_size += event_size;
 			i--;
 		}
-- 
2.43.0

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

[Ard Biesheuvel]

On Sat, 14 Sept 2024 at 15:26, Gregory Price <gourry@gourry.net> wrote:
>
> 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 can lead to calling memblock_reserve on a range
> with an effectively negative length.
>
> Additionally, there are silently ignored error conditions may
> result in undefined behavior.  Address these.
>
> Signed-off-by: Gregory Price <gourry@gourry.net>
>
> Gregory Price (4):
>   tpm: fix signed/unsigned bug when checking event logs
>   tpm: do not ignore memblock_reserve return value
>   tpm: fix unsigned/signed mismatch errors related to
>     __calc_tpm2_event_size
>   libstub,tpm: do not ignore failure case when reading final event log
>

Now queued up - thanks.

>  drivers/firmware/efi/libstub/tpm.c |  9 ++++++---
>  drivers/firmware/efi/tpm.c         | 26 ++++++++++++++++----------
>  include/linux/tpm_eventlog.h       |  2 +-
>  3 files changed, 23 insertions(+), 14 deletions(-)
>
> --
> 2.43.0
>