[PATCH v2 0/3] Fix bugs related to TPM2 event log

[PATCH v2 0/3] Fix bugs related to TPM2 event log

[Stefan Berger]

This series of patches fixes a couple of issues related to TPM2
event logs, such as the disappearance of the TPM2 log on QEMU machines
running with UEFI (my fault) and a kernel fault due to an integer under-
flow when reading the TPM 2 log multiple times.

Regards,
   Stefan

v1->v2:
 - Revised patches 1 & 2

Stefan Berger (3):
  tpm: efi: Use local variable for calculating final log size
  tpm: acpi: Check eventlog signature before using it
  tpm: vtpm_proxy: Avoid reading host log when using a virtual device

 drivers/char/tpm/eventlog/acpi.c   | 33 +++++++++++++++++++++++++++++-
 drivers/char/tpm/eventlog/common.c |  3 +++
 drivers/char/tpm/eventlog/efi.c    | 29 ++++++++++++++++++--------
 3 files changed, 56 insertions(+), 9 deletions(-)

-- 
2.29.2

[PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Stefan Berger]

When tpm_read_log_efi is called multiple times, which happens when
one loads and unloads a TPM2 driver multiple times, then the global
variable efi_tpm_final_log_size will at some point become a negative
number due to the subtraction of final_events_preboot_size occurring
each time. Use a local variable to avoid this integer underflow.

The following issue is now resolved:

Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
Mar  8 15:35:12 hibinst kernel: Call Trace:
Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/efi.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 35229e5143ca..e6cb9d525e30 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -17,6 +17,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 {
 
 	struct efi_tcg2_final_events_table *final_tbl = NULL;
+	int final_events_log_size = efi_tpm_final_log_size;
 	struct linux_efi_tpm_eventlog *log_tbl;
 	struct tpm_bios_log *log;
 	u32 log_size;
@@ -66,12 +67,12 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 	ret = tpm_log_version;
 
 	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
-	    efi_tpm_final_log_size == 0 ||
+	    final_events_log_size == 0 ||
 	    tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
 		goto out;
 
 	final_tbl = memremap(efi.tpm_final_log,
-			     sizeof(*final_tbl) + efi_tpm_final_log_size,
+			     sizeof(*final_tbl) + final_events_log_size,
 			     MEMREMAP_WB);
 	if (!final_tbl) {
 		pr_err("Could not map UEFI TPM final log\n");
@@ -80,10 +81,18 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 		goto out;
 	}
 
-	efi_tpm_final_log_size -= log_tbl->final_events_preboot_size;
+	/*
+	 * The 'final events log' size excludes the 'final events preboot log'
+	 * at its beginning.
+	 */
+	final_events_log_size -= log_tbl->final_events_preboot_size;
 
+	/*
+	 * Allocate memory for the 'combined log' where we will append the
+	 * 'final events log' to.
+	 */
 	tmp = krealloc(log->bios_event_log,
-		       log_size + efi_tpm_final_log_size,
+		       log_size + final_events_log_size,
 		       GFP_KERNEL);
 	if (!tmp) {
 		kfree(log->bios_event_log);
@@ -94,15 +103,19 @@ int tpm_read_log_efi(struct tpm_chip *chip)
 	log->bios_event_log = tmp;
 
 	/*
-	 * Copy any of the final events log that didn't also end up in the
-	 * main log. Events can be logged in both if events are generated
+	 * Append any of the 'final events log' that didn't also end up in the
+	 * 'main log'. Events can be logged in both if events are generated
 	 * between GetEventLog() and ExitBootServices().
 	 */
 	memcpy((void *)log->bios_event_log + log_size,
 	       final_tbl->events + log_tbl->final_events_preboot_size,
-	       efi_tpm_final_log_size);
+	       final_events_log_size);
+	/*
+	 * The size of the 'combined log' is the size of the 'main log' plus
+	 * the size of the 'final events log'.
+	 */
 	log->bios_event_log_end = log->bios_event_log +
-		log_size + efi_tpm_final_log_size;
+		log_size + final_events_log_size;
 
 out:
 	memunmap(final_tbl);
-- 
2.29.2

[PATCH v2 2/3] tpm: acpi: Check eventlog signature before using it

[Stefan Berger]

Check the eventlog signature before using it. This avoids using an
empty log, as may be the case when QEMU created the ACPI tables,
rather than probing the EFI log next. This resolves an issue where
the EFI log was empty since an empty ACPI log was used.

Fixes: 85467f63a05c ("tpm: Add support for event log pointer found in TPM2 ACPI table")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/acpi.c | 33 +++++++++++++++++++++++++++++++-
 1 file changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
index 3633ed70f48f..1b18ce5ebab1 100644
--- a/drivers/char/tpm/eventlog/acpi.c
+++ b/drivers/char/tpm/eventlog/acpi.c
@@ -41,6 +41,27 @@ struct acpi_tcpa {
 	};
 };
 
+/* Check that the given log is indeed a TPM2 log. */
+static bool tpm_is_tpm2_log(void *bios_event_log, u64 len)
+{
+	struct tcg_efi_specid_event_head *efispecid;
+	struct tcg_pcr_event *event_header;
+	int n;
+
+	if (len < sizeof(*event_header))
+		return false;
+	len -= sizeof(*event_header);
+	event_header = bios_event_log;
+
+	if (len < sizeof(*efispecid))
+		return false;
+	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
+
+	n = memcmp(efispecid->signature, TCG_SPECID_SIG,
+		   sizeof(TCG_SPECID_SIG));
+	return n == 0;
+}
+
 /* read binary bios log */
 int tpm_read_log_acpi(struct tpm_chip *chip)
 {
@@ -52,6 +73,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
 	struct acpi_table_tpm2 *tbl;
 	struct acpi_tpm2_phy *tpm2_phy;
 	int format;
+	int ret;
 
 	log = &chip->log;
 
@@ -112,6 +134,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
 
 	log->bios_event_log_end = log->bios_event_log + len;
 
+	ret = -EIO;
 	virt = acpi_os_map_iomem(start, len);
 	if (!virt)
 		goto err;
@@ -119,11 +142,19 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
 	memcpy_fromio(log->bios_event_log, virt, len);
 
 	acpi_os_unmap_iomem(virt, len);
+
+	if (chip->flags & TPM_CHIP_FLAG_TPM2 &&
+	    !tpm_is_tpm2_log(log->bios_event_log, len)) {
+		/* try EFI log next */
+		ret = -ENODEV;
+		goto err;
+	}
+
 	return format;
 
 err:
 	kfree(log->bios_event_log);
 	log->bios_event_log = NULL;
-	return -EIO;
+	return ret;
 
 }
-- 
2.29.2

[PATCH v2 3/3] tpm: vtpm_proxy: Avoid reading host log when using a virtual device

[Stefan Berger]

Avoid allocating memory and reading the host log when a virtual device
is used since this log is of no use to that driver. A virtual
device can be identified through the flag TPM_CHIP_FLAG_VIRTUAL, which
is only set for the tpm_vtpm_proxy driver.

Fixes: 6f99612e2500 ("tpm: Proxy driver for supporting multiple emulated TPMs")
Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
---
 drivers/char/tpm/eventlog/common.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
index 7460f230bae4..8512ec76d526 100644
--- a/drivers/char/tpm/eventlog/common.c
+++ b/drivers/char/tpm/eventlog/common.c
@@ -107,6 +107,9 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
 	int log_version;
 	int rc = 0;
 
+	if (chip->flags & TPM_CHIP_FLAG_VIRTUAL)
+		return;
+
 	rc = tpm_read_log(chip);
 	if (rc < 0)
 		return;
-- 
2.29.2

Re: [PATCH v2 0/3] Fix bugs related to TPM2 event log

[Jarkko Sakkinen]

On Wed, Mar 10, 2021 at 05:19:13PM -0500, Stefan Berger wrote:
> This series of patches fixes a couple of issues related to TPM2
> event logs, such as the disappearance of the TPM2 log on QEMU machines
> running with UEFI (my fault) and a kernel fault due to an integer under-
> flow when reading the TPM 2 log multiple times.
> 
> Regards,
>    Stefan
> 
> v1->v2:
>  - Revised patches 1 & 2
> 
> Stefan Berger (3):
>   tpm: efi: Use local variable for calculating final log size
>   tpm: acpi: Check eventlog signature before using it
>   tpm: vtpm_proxy: Avoid reading host log when using a virtual device
> 
>  drivers/char/tpm/eventlog/acpi.c   | 33 +++++++++++++++++++++++++++++-
>  drivers/char/tpm/eventlog/common.c |  3 +++
>  drivers/char/tpm/eventlog/efi.c    | 29 ++++++++++++++++++--------
>  3 files changed, 56 insertions(+), 9 deletions(-)
> 
> -- 
> 2.29.2
> 
> 

b4 seems like remarkably useful tool:

$ b4 am 20210310221916.356716-1-stefanb@linux.ibm.com
Looking up https://lore.kernel.org/r/20210310221916.356716-1-stefanb%40linux.ibm.com
Grabbing thread from lore.kernel.org/linux-integrity
Analyzing 4 messages in the thread
---
Writing ./v2_20210310_stefanb_fix_bugs_related_to_tpm2_event_log.mbx
  ✓ [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size
  ✓ [PATCH v2 2/3] tpm: acpi: Check eventlog signature before using it
  ✓ [PATCH v2 3/3] tpm: vtpm_proxy: Avoid reading host log when using a virtual device
  ---
  ✓ Attestation-by: DKIM/ibm.com (From: stefanb@linux.ibm.com)
---
Total patches: 3
---
Cover: ./v2_20210310_stefanb_fix_bugs_related_to_tpm2_event_log.cover
 Link: https://lore.kernel.org/r/20210310221916.356716-1-stefanb@linux.ibm.com
 Base: not found (applies clean to current tree)
       git am ./v2_20210310_stefanb_fix_bugs_related_to_tpm2_event_log.mbx

$ git am -3 v2_20210310_stefanb_fix_bugs_related_to_tpm2_event_log.mbx
Applying: tpm: efi: Use local variable for calculating final log size
Applying: tpm: acpi: Check eventlog signature before using it
Applying: tpm: vtpm_proxy: Avoid reading host log when using a virtual device

I also did run checkpatch.pl, no errors.

/Jarkko

Re: [PATCH v2 3/3] tpm: vtpm_proxy: Avoid reading host log when using a virtual device

[Jarkko Sakkinen]

On Wed, Mar 10, 2021 at 05:19:16PM -0500, Stefan Berger wrote:
> Avoid allocating memory and reading the host log when a virtual device
> is used since this log is of no use to that driver. A virtual
> device can be identified through the flag TPM_CHIP_FLAG_VIRTUAL, which
> is only set for the tpm_vtpm_proxy driver.
> 
> Fixes: 6f99612e2500 ("tpm: Proxy driver for supporting multiple emulated TPMs")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/eventlog/common.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c
> index 7460f230bae4..8512ec76d526 100644
> --- a/drivers/char/tpm/eventlog/common.c
> +++ b/drivers/char/tpm/eventlog/common.c
> @@ -107,6 +107,9 @@ void tpm_bios_log_setup(struct tpm_chip *chip)
>  	int log_version;
>  	int rc = 0;
>  
> +	if (chip->flags & TPM_CHIP_FLAG_VIRTUAL)
> +		return;
> +
>  	rc = tpm_read_log(chip);
>  	if (rc < 0)
>  		return;
> -- 
> 2.29.2
> 
> 


Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>


/Jarkko

Re: [PATCH v2 2/3] tpm: acpi: Check eventlog signature before using it

[Jarkko Sakkinen]

On Wed, Mar 10, 2021 at 05:19:15PM -0500, Stefan Berger wrote:
> Check the eventlog signature before using it. This avoids using an
> empty log, as may be the case when QEMU created the ACPI tables,
> rather than probing the EFI log next. This resolves an issue where
> the EFI log was empty since an empty ACPI log was used.
> 
> Fixes: 85467f63a05c ("tpm: Add support for event log pointer found in TPM2 ACPI table")
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/eventlog/acpi.c | 33 +++++++++++++++++++++++++++++++-
>  1 file changed, 32 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c
> index 3633ed70f48f..1b18ce5ebab1 100644
> --- a/drivers/char/tpm/eventlog/acpi.c
> +++ b/drivers/char/tpm/eventlog/acpi.c
> @@ -41,6 +41,27 @@ struct acpi_tcpa {
>  	};
>  };
>  
> +/* Check that the given log is indeed a TPM2 log. */
> +static bool tpm_is_tpm2_log(void *bios_event_log, u64 len)
> +{
> +	struct tcg_efi_specid_event_head *efispecid;
> +	struct tcg_pcr_event *event_header;
> +	int n;
> +
> +	if (len < sizeof(*event_header))
> +		return false;
> +	len -= sizeof(*event_header);
> +	event_header = bios_event_log;
> +
> +	if (len < sizeof(*efispecid))
> +		return false;
> +	efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
> +
> +	n = memcmp(efispecid->signature, TCG_SPECID_SIG,
> +		   sizeof(TCG_SPECID_SIG));
> +	return n == 0;
> +}
> +
>  /* read binary bios log */
>  int tpm_read_log_acpi(struct tpm_chip *chip)
>  {
> @@ -52,6 +73,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>  	struct acpi_table_tpm2 *tbl;
>  	struct acpi_tpm2_phy *tpm2_phy;
>  	int format;
> +	int ret;
>  
>  	log = &chip->log;
>  
> @@ -112,6 +134,7 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>  
>  	log->bios_event_log_end = log->bios_event_log + len;
>  
> +	ret = -EIO;
>  	virt = acpi_os_map_iomem(start, len);
>  	if (!virt)
>  		goto err;
> @@ -119,11 +142,19 @@ int tpm_read_log_acpi(struct tpm_chip *chip)
>  	memcpy_fromio(log->bios_event_log, virt, len);
>  
>  	acpi_os_unmap_iomem(virt, len);
> +
> +	if (chip->flags & TPM_CHIP_FLAG_TPM2 &&
> +	    !tpm_is_tpm2_log(log->bios_event_log, len)) {
> +		/* try EFI log next */
> +		ret = -ENODEV;
> +		goto err;
> +	}
> +
>  	return format;
>  
>  err:
>  	kfree(log->bios_event_log);
>  	log->bios_event_log = NULL;
> -	return -EIO;
> +	return ret;
>  
>  }
> -- 
> 2.29.2
> 
> 

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

/Jarkko

Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Jarkko Sakkinen]

On Wed, Mar 10, 2021 at 05:19:14PM -0500, Stefan Berger wrote:
> When tpm_read_log_efi is called multiple times, which happens when
> one loads and unloads a TPM2 driver multiple times, then the global
> variable efi_tpm_final_log_size will at some point become a negative
> number due to the subtraction of final_events_preboot_size occurring
> each time. Use a local variable to avoid this integer underflow.
> 
> The following issue is now resolved:
> 
> Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
> Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
> Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
> Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
> Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
> Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
> Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
> Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
> Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
> Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
> Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
> Mar  8 15:35:12 hibinst kernel: Call Trace:
> Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
> Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
> Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
> Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
> Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
> Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
> Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  drivers/char/tpm/eventlog/efi.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 35229e5143ca..e6cb9d525e30 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -17,6 +17,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  {
>  
>  	struct efi_tcg2_final_events_table *final_tbl = NULL;
> +	int final_events_log_size = efi_tpm_final_log_size;
>  	struct linux_efi_tpm_eventlog *log_tbl;
>  	struct tpm_bios_log *log;
>  	u32 log_size;
> @@ -66,12 +67,12 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	ret = tpm_log_version;
>  
>  	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
> -	    efi_tpm_final_log_size == 0 ||
> +	    final_events_log_size == 0 ||
>  	    tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>  		goto out;
>  
>  	final_tbl = memremap(efi.tpm_final_log,
> -			     sizeof(*final_tbl) + efi_tpm_final_log_size,
> +			     sizeof(*final_tbl) + final_events_log_size,
>  			     MEMREMAP_WB);
>  	if (!final_tbl) {
>  		pr_err("Could not map UEFI TPM final log\n");
> @@ -80,10 +81,18 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  		goto out;
>  	}
>  
> -	efi_tpm_final_log_size -= log_tbl->final_events_preboot_size;
> +	/*
> +	 * The 'final events log' size excludes the 'final events preboot log'
> +	 * at its beginning.
> +	 */
> +	final_events_log_size -= log_tbl->final_events_preboot_size;
>  
> +	/*
> +	 * Allocate memory for the 'combined log' where we will append the
> +	 * 'final events log' to.
> +	 */
>  	tmp = krealloc(log->bios_event_log,
> -		       log_size + efi_tpm_final_log_size,
> +		       log_size + final_events_log_size,
>  		       GFP_KERNEL);
>  	if (!tmp) {
>  		kfree(log->bios_event_log);
> @@ -94,15 +103,19 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	log->bios_event_log = tmp;
>  
>  	/*
> -	 * Copy any of the final events log that didn't also end up in the
> -	 * main log. Events can be logged in both if events are generated
> +	 * Append any of the 'final events log' that didn't also end up in the
> +	 * 'main log'. Events can be logged in both if events are generated
>  	 * between GetEventLog() and ExitBootServices().
>  	 */
>  	memcpy((void *)log->bios_event_log + log_size,
>  	       final_tbl->events + log_tbl->final_events_preboot_size,
> -	       efi_tpm_final_log_size);
> +	       final_events_log_size);
> +	/*
> +	 * The size of the 'combined log' is the size of the 'main log' plus
> +	 * the size of the 'final events log'.
> +	 */
>  	log->bios_event_log_end = log->bios_event_log +
> -		log_size + efi_tpm_final_log_size;
> +		log_size + final_events_log_size;
>  
>  out:
>  	memunmap(final_tbl);
> -- 
> 2.29.2
> 
> 

Hey, thanks a lot for that documentation!

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

I applied these to my master, planning to squeeze in 5.12 (if Linus accepts
them).

/Jarkko

Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Jarkko Sakkinen]

On Wed, Mar 10, 2021 at 05:19:14PM -0500, Stefan Berger wrote:
> When tpm_read_log_efi is called multiple times, which happens when
> one loads and unloads a TPM2 driver multiple times, then the global
> variable efi_tpm_final_log_size will at some point become a negative
> number due to the subtraction of final_events_preboot_size occurring
> each time. Use a local variable to avoid this integer underflow.
> 
> The following issue is now resolved:
> 
> Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
> Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
> Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
> Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
> Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
> Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
> Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
> Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
> Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
> Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
> Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
> Mar  8 15:35:12 hibinst kernel: Call Trace:
> Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
> Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
> Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
> Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
> Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
> Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
> Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>

Fixes tag for this one? 

/Jarkko

> ---
>  drivers/char/tpm/eventlog/efi.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 35229e5143ca..e6cb9d525e30 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -17,6 +17,7 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  {
>  
>  	struct efi_tcg2_final_events_table *final_tbl = NULL;
> +	int final_events_log_size = efi_tpm_final_log_size;
>  	struct linux_efi_tpm_eventlog *log_tbl;
>  	struct tpm_bios_log *log;
>  	u32 log_size;
> @@ -66,12 +67,12 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	ret = tpm_log_version;
>  
>  	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR ||
> -	    efi_tpm_final_log_size == 0 ||
> +	    final_events_log_size == 0 ||
>  	    tpm_log_version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2)
>  		goto out;
>  
>  	final_tbl = memremap(efi.tpm_final_log,
> -			     sizeof(*final_tbl) + efi_tpm_final_log_size,
> +			     sizeof(*final_tbl) + final_events_log_size,
>  			     MEMREMAP_WB);
>  	if (!final_tbl) {
>  		pr_err("Could not map UEFI TPM final log\n");
> @@ -80,10 +81,18 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  		goto out;
>  	}
>  
> -	efi_tpm_final_log_size -= log_tbl->final_events_preboot_size;
> +	/*
> +	 * The 'final events log' size excludes the 'final events preboot log'
> +	 * at its beginning.
> +	 */
> +	final_events_log_size -= log_tbl->final_events_preboot_size;
>  
> +	/*
> +	 * Allocate memory for the 'combined log' where we will append the
> +	 * 'final events log' to.
> +	 */
>  	tmp = krealloc(log->bios_event_log,
> -		       log_size + efi_tpm_final_log_size,
> +		       log_size + final_events_log_size,
>  		       GFP_KERNEL);
>  	if (!tmp) {
>  		kfree(log->bios_event_log);
> @@ -94,15 +103,19 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	log->bios_event_log = tmp;
>  
>  	/*
> -	 * Copy any of the final events log that didn't also end up in the
> -	 * main log. Events can be logged in both if events are generated
> +	 * Append any of the 'final events log' that didn't also end up in the
> +	 * 'main log'. Events can be logged in both if events are generated
>  	 * between GetEventLog() and ExitBootServices().
>  	 */
>  	memcpy((void *)log->bios_event_log + log_size,
>  	       final_tbl->events + log_tbl->final_events_preboot_size,
> -	       efi_tpm_final_log_size);
> +	       final_events_log_size);
> +	/*
> +	 * The size of the 'combined log' is the size of the 'main log' plus
> +	 * the size of the 'final events log'.
> +	 */
>  	log->bios_event_log_end = log->bios_event_log +
> -		log_size + efi_tpm_final_log_size;
> +		log_size + final_events_log_size;
>  
>  out:
>  	memunmap(final_tbl);
> -- 
> 2.29.2
> 
> 

Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Jarkko Sakkinen]

On Thu, Mar 11, 2021 at 01:21:47AM +0200, Jarkko Sakkinen wrote:
> On Wed, Mar 10, 2021 at 05:19:14PM -0500, Stefan Berger wrote:
> > When tpm_read_log_efi is called multiple times, which happens when
> > one loads and unloads a TPM2 driver multiple times, then the global
> > variable efi_tpm_final_log_size will at some point become a negative
> > number due to the subtraction of final_events_preboot_size occurring
> > each time. Use a local variable to avoid this integer underflow.
> > 
> > The following issue is now resolved:
> > 
> > Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> > Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
> > Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
> > Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
> > Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
> > Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
> > Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
> > Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
> > Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
> > Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
> > Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
> > Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
> > Mar  8 15:35:12 hibinst kernel: Call Trace:
> > Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
> > Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
> > Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
> > Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
> > Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
> > Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
> > Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370
> > 
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> 
> Fixes tag for this one? 

Or just sanity check, I think it is:

Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event log in the TCG2 log")

Also, I guess all of the patches ought to have stable cc, right?

/Jarkko

Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Stefan Berger]

On 3/10/21 6:24 PM, Jarkko Sakkinen wrote:
> On Thu, Mar 11, 2021 at 01:21:47AM +0200, Jarkko Sakkinen wrote:
>> On Wed, Mar 10, 2021 at 05:19:14PM -0500, Stefan Berger wrote:
>>> When tpm_read_log_efi is called multiple times, which happens when
>>> one loads and unloads a TPM2 driver multiple times, then the global
>>> variable efi_tpm_final_log_size will at some point become a negative
>>> number due to the subtraction of final_events_preboot_size occurring
>>> each time. Use a local variable to avoid this integer underflow.
>>>
>>> The following issue is now resolved:
>>>
>>> Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
>>> Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
>>> Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
>>> Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
>>> Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
>>> Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
>>> Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
>>> Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
>>> Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
>>> Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
>>> Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
>>> Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
>>> Mar  8 15:35:12 hibinst kernel: Call Trace:
>>> Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
>>> Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
>>> Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
>>> Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
>>> Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
>>> Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
>>> Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370
>>>
>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Fixes tag for this one?
> Or just sanity check, I think it is:
>
> Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event log in the TCG2 log")
Looks good.
>
> Also, I guess all of the patches ought to have stable cc, right?

Yes, please. I think the maintainer has to add this 'at some point'.

Since you queued them already I won't send a v3.


Thanks!

    Stefan

Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size

[Jarkko Sakkinen]

On Thu, Mar 11, 2021 at 09:02:38AM -0500, Stefan Berger wrote:
> 
> On 3/10/21 6:24 PM, Jarkko Sakkinen wrote:
> > On Thu, Mar 11, 2021 at 01:21:47AM +0200, Jarkko Sakkinen wrote:
> > > On Wed, Mar 10, 2021 at 05:19:14PM -0500, Stefan Berger wrote:
> > > > When tpm_read_log_efi is called multiple times, which happens when
> > > > one loads and unloads a TPM2 driver multiple times, then the global
> > > > variable efi_tpm_final_log_size will at some point become a negative
> > > > number due to the subtraction of final_events_preboot_size occurring
> > > > each time. Use a local variable to avoid this integer underflow.
> > > > 
> > > > The following issue is now resolved:
> > > > 
> > > > Mar  8 15:35:12 hibinst kernel: Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 0.0.0 02/06/2015
> > > > Mar  8 15:35:12 hibinst kernel: Workqueue: tpm-vtpm vtpm_proxy_work [tpm_vtpm_proxy]
> > > > Mar  8 15:35:12 hibinst kernel: RIP: 0010:__memcpy+0x12/0x20
> > > > Mar  8 15:35:12 hibinst kernel: Code: 00 b8 01 00 00 00 85 d2 74 0a c7 05 44 7b ef 00 0f 00 00 00 c3 cc cc cc 66 66 90 66 90 48 89 f8 48 89 d1 48 c1 e9 03 83 e2 07 <f3> 48 a5 89 d1 f3 a4 c3 66 0f 1f 44 00 00 48 89 f8 48 89 d1 f3 a4
> > > > Mar  8 15:35:12 hibinst kernel: RSP: 0018:ffff9ac4c0fcfde0 EFLAGS: 00010206
> > > > Mar  8 15:35:12 hibinst kernel: RAX: ffff88f878cefed5 RBX: ffff88f878ce9000 RCX: 1ffffffffffffe0f
> > > > Mar  8 15:35:12 hibinst kernel: RDX: 0000000000000003 RSI: ffff9ac4c003bff9 RDI: ffff88f878cf0e4d
> > > > Mar  8 15:35:12 hibinst kernel: RBP: ffff9ac4c003b000 R08: 0000000000001000 R09: 000000007e9d6073
> > > > Mar  8 15:35:12 hibinst kernel: R10: ffff9ac4c003b000 R11: ffff88f879ad3500 R12: 0000000000000ed5
> > > > Mar  8 15:35:12 hibinst kernel: R13: ffff88f878ce9760 R14: 0000000000000002 R15: ffff88f77de7f018
> > > > Mar  8 15:35:12 hibinst kernel: FS:  0000000000000000(0000) GS:ffff88f87bd00000(0000) knlGS:0000000000000000
> > > > Mar  8 15:35:12 hibinst kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > Mar  8 15:35:12 hibinst kernel: CR2: ffff9ac4c003c000 CR3: 00000001785a6004 CR4: 0000000000060ee0
> > > > Mar  8 15:35:12 hibinst kernel: Call Trace:
> > > > Mar  8 15:35:12 hibinst kernel: tpm_read_log_efi+0x152/0x1a7
> > > > Mar  8 15:35:12 hibinst kernel: tpm_bios_log_setup+0xc8/0x1c0
> > > > Mar  8 15:35:12 hibinst kernel: tpm_chip_register+0x8f/0x260
> > > > Mar  8 15:35:12 hibinst kernel: vtpm_proxy_work+0x16/0x60 [tpm_vtpm_proxy]
> > > > Mar  8 15:35:12 hibinst kernel: process_one_work+0x1b4/0x370
> > > > Mar  8 15:35:12 hibinst kernel: worker_thread+0x53/0x3e0
> > > > Mar  8 15:35:12 hibinst kernel: ? process_one_work+0x370/0x370
> > > > 
> > > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Fixes tag for this one?
> > Or just sanity check, I think it is:
> > 
> > Fixes: 166a2809d65b ("tpm: Don't duplicate events from the final event log in the TCG2 log")
> Looks good.
> > 
> > Also, I guess all of the patches ought to have stable cc, right?
> 
> Yes, please. I think the maintainer has to add this 'at some point'.
> 
> Since you queued them already I won't send a v3.
> 
> 
> Thanks!
> 
>    Stefan

OK, I'll add cc's before I send the PR, thanks.

/Jarkko