diff mbox series

tpm: efi: Don't create binary_bios_measurements file for an empty log

Message ID 20201028154102.9595-1-tyhicks@linux.microsoft.com (mailing list archive)
State New, archived
Headers show
Series tpm: efi: Don't create binary_bios_measurements file for an empty log | expand

Commit Message

Tyler Hicks Oct. 28, 2020, 3:41 p.m. UTC 
Mimic the pre-existing ACPI and Device Tree event log behavior by not
creating the binary_bios_measurements file when the EFI TPM event log is
empty.

This fixes the following NULL pointer dereference that can occur when
reading /sys/kernel/security/tpm0/binary_bios_measurements after the
kernel received an empty event log from the firmware:

 BUG: kernel NULL pointer dereference, address: 000000000000002c
 #PF: supervisor read access in kernel mode
 #PF: error_code(0x0000) - not-present page
 PGD 0 P4D 0
 Oops: 0000 [#1] SMP PTI
 CPU: 2 PID: 3932 Comm: fwupdtpmevlog Not tainted 5.9.0-00003-g629990edad62 #17
 Hardware name: LENOVO 20LCS03L00/20LCS03L00, BIOS N27ET38W (1.24 ) 11/28/2019
 RIP: 0010:tpm2_bios_measurements_start+0x3a/0x550
 Code: 54 53 48 83 ec 68 48 8b 57 70 48 8b 1e 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 48 8b 82 c0 06 00 00 48 8b 8a c8 06 00 00 <44> 8b 60 1c 48 89 4d a0 4c 89 e2 49 83 c4 20 48 83 fb 00 75 2a 49
 RSP: 0018:ffffa9c901203db0 EFLAGS: 00010246
 RAX: 0000000000000010 RBX: 0000000000000000 RCX: 0000000000000010
 RDX: ffff8ba1eb99c000 RSI: ffff8ba1e4ce8280 RDI: ffff8ba1e4ce8258
 RBP: ffffa9c901203e40 R08: ffffa9c901203dd8 R09: ffff8ba1ec443300
 R10: ffffa9c901203e50 R11: 0000000000000000 R12: ffff8ba1e4ce8280
 R13: ffffa9c901203ef0 R14: ffffa9c901203ef0 R15: ffff8ba1e4ce8258
 FS:  00007f6595460880(0000) GS:ffff8ba1ef880000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 000000000000002c CR3: 00000007d8d18003 CR4: 00000000003706e0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  ? __kmalloc_node+0x113/0x320
  ? kvmalloc_node+0x31/0x80
  seq_read+0x94/0x420
  vfs_read+0xa7/0x190
  ksys_read+0xa7/0xe0
  __x64_sys_read+0x1a/0x20
  do_syscall_64+0x37/0x80
  entry_SYSCALL_64_after_hwframe+0x44/0xa9

In this situation, the bios_event_log pointer in the tpm_bios_log struct
was not NULL but was equal to the ZERO_SIZE_PTR (0x10) value. This was
due to the following kmemdup() in tpm_read_log_efi():

int tpm_read_log_efi(struct tpm_chip *chip)
{
...
	/* malloc EventLog space */
	log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
	if (!log->bios_event_log) {
		ret = -ENOMEM;
		goto out;
	}
...
}

When log_size is zero, due to an empty event log from firmware,
ZERO_SIZE_PTR is returned from kmemdup(). Upon a read of the
binary_bios_measurements file, the tpm2_bios_measurements_start()
function does not perform a ZERO_OR_NULL_PTR() check on the
bios_event_log pointer before dereferencing it.

Rather than add a ZERO_OR_NULL_PTR() check in functions that make use of
the bios_event_log pointer, simply avoid creating the
binary_bios_measurements_file as is done in other event log retrieval
backends.

Explicitly ignore all of the events in the final event log when the main
event log is empty. The list of events in the final event log cannot be
accurately parsed without referring to the first event in the main event
log (the event log header) so the final event log is useless in such a
situation.

Fixes: 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
Link: https://lore.kernel.org/linux-integrity/E1FDCCCB-CA51-4AEE-AC83-9CDE995EAE52@canonical.com/
Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Reported-by: Kenneth R. Crudup <kenny@panix.com>
Reported-by: Mimi Zohar <zohar@linux.ibm.com>
Cc: Thiébaud Weksteen <tweek@google.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
---
 drivers/char/tpm/eventlog/efi.c | 5 +++++
 1 file changed, 5 insertions(+)


base-commit: ed8780e3f2ecc82645342d070c6b4e530532e680

Comments

Tyler Hicks Oct. 28, 2020, 5:39 p.m. UTC | #1 
On 2020-10-28 11:30:11, Tyler Hicks wrote:
> So, we need help from Kai, Kenneth, or Mimi to verify my assumption that
> their firmware is providing an empty main event log and a populated
> final event log.

Hi Kai, Kenneth, and Mimi - could one or two of you please follow these
steps:

1) Apply the proposed patch in the grandparent of this email so that
   your kernel doesn't crash
2) Revert commit 7f3d176f5f7e ("tpm: Require that all digests are
   present in TCG_PCR_EVENT2 structures") so that
   __calc_tpm2_event_size() goes back to being less picky and may treat
   a final log event as a valid event log header
3) Add some debugging warnings in efi_tpm_eventlog_init() to check for
   an empty main event log and a populated final event log, as shown
   below
4) Boot the resulting kernel build, look for the warnings, and report
   back

diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index c1955d320fec..c4d2dbd5ed42 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -78,6 +78,9 @@ int __init efi_tpm_eventlog_init(void)
 		goto out;
 	}
 
+	WARN(!log_tbl->size && final_tbl->nr_events,
+	     "nr_events = %llu\n", final_tbl->nr_events);
+
 	tbl_size = 0;
 	if (final_tbl->nr_events != 0) {
 		void *events = (void *)efi.tpm_final_log
@@ -95,6 +98,8 @@ int __init efi_tpm_eventlog_init(void)
 		goto out_calc;
 	}
 
+	WARN(!log_tbl->size && tbl_size, "tbl_size = %d\n", tbl_size);
+
 	memblock_reserve((unsigned long)final_tbl,
 			 tbl_size + sizeof(*final_tbl));
 	efi_tpm_final_log_size = tbl_size;

For your convenience, I've created a branch with these changes on top of
v5.9:

 https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=tpm/bin-bios-measurements-debug

Thank you!

Tyler
Jarkko Sakkinen Oct. 30, 2020, 4:46 a.m. UTC | #2 
On Wed, Oct 28, 2020 at 10:41:02AM -0500, Tyler Hicks wrote:
> Mimic the pre-existing ACPI and Device Tree event log behavior by not
> creating the binary_bios_measurements file when the EFI TPM event log is
> empty.
> 
> This fixes the following NULL pointer dereference that can occur when
> reading /sys/kernel/security/tpm0/binary_bios_measurements after the
> kernel received an empty event log from the firmware:
> 
>  BUG: kernel NULL pointer dereference, address: 000000000000002c
>  #PF: supervisor read access in kernel mode
>  #PF: error_code(0x0000) - not-present page
>  PGD 0 P4D 0
>  Oops: 0000 [#1] SMP PTI
>  CPU: 2 PID: 3932 Comm: fwupdtpmevlog Not tainted 5.9.0-00003-g629990edad62 #17
>  Hardware name: LENOVO 20LCS03L00/20LCS03L00, BIOS N27ET38W (1.24 ) 11/28/2019
>  RIP: 0010:tpm2_bios_measurements_start+0x3a/0x550
>  Code: 54 53 48 83 ec 68 48 8b 57 70 48 8b 1e 65 48 8b 04 25 28 00 00 00 48 89 45 d0 31 c0 48 8b 82 c0 06 00 00 48 8b 8a c8 06 00 00 <44> 8b 60 1c 48 89 4d a0 4c 89 e2 49 83 c4 20 48 83 fb 00 75 2a 49
>  RSP: 0018:ffffa9c901203db0 EFLAGS: 00010246
>  RAX: 0000000000000010 RBX: 0000000000000000 RCX: 0000000000000010
>  RDX: ffff8ba1eb99c000 RSI: ffff8ba1e4ce8280 RDI: ffff8ba1e4ce8258
>  RBP: ffffa9c901203e40 R08: ffffa9c901203dd8 R09: ffff8ba1ec443300
>  R10: ffffa9c901203e50 R11: 0000000000000000 R12: ffff8ba1e4ce8280
>  R13: ffffa9c901203ef0 R14: ffffa9c901203ef0 R15: ffff8ba1e4ce8258
>  FS:  00007f6595460880(0000) GS:ffff8ba1ef880000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 000000000000002c CR3: 00000007d8d18003 CR4: 00000000003706e0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   ? __kmalloc_node+0x113/0x320
>   ? kvmalloc_node+0x31/0x80
>   seq_read+0x94/0x420
>   vfs_read+0xa7/0x190
>   ksys_read+0xa7/0xe0
>   __x64_sys_read+0x1a/0x20
>   do_syscall_64+0x37/0x80
>   entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> In this situation, the bios_event_log pointer in the tpm_bios_log struct
> was not NULL but was equal to the ZERO_SIZE_PTR (0x10) value. This was
> due to the following kmemdup() in tpm_read_log_efi():
> 
> int tpm_read_log_efi(struct tpm_chip *chip)
> {
> ...
> 	/* malloc EventLog space */
> 	log->bios_event_log = kmemdup(log_tbl->log, log_size, GFP_KERNEL);
> 	if (!log->bios_event_log) {
> 		ret = -ENOMEM;
> 		goto out;
> 	}
> ...
> }
> 
> When log_size is zero, due to an empty event log from firmware,
> ZERO_SIZE_PTR is returned from kmemdup(). Upon a read of the
> binary_bios_measurements file, the tpm2_bios_measurements_start()
> function does not perform a ZERO_OR_NULL_PTR() check on the
> bios_event_log pointer before dereferencing it.
> 
> Rather than add a ZERO_OR_NULL_PTR() check in functions that make use of
> the bios_event_log pointer, simply avoid creating the
> binary_bios_measurements_file as is done in other event log retrieval
> backends.
> 
> Explicitly ignore all of the events in the final event log when the main
> event log is empty. The list of events in the final event log cannot be
> accurately parsed without referring to the first event in the main event
> log (the event log header) so the final event log is useless in such a
> situation.
> 
> Fixes: 58cc1e4faf10 ("tpm: parse TPM event logs based on EFI table")
> Link: https://lore.kernel.org/linux-integrity/E1FDCCCB-CA51-4AEE-AC83-9CDE995EAE52@canonical.com/
> Reported-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> Reported-by: Kenneth R. Crudup <kenny@panix.com>
> Reported-by: Mimi Zohar <zohar@linux.ibm.com>
> Cc: Thiébaud Weksteen <tweek@google.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Signed-off-by: Tyler Hicks <tyhicks@linux.microsoft.com>
> ---

Applied, thanks.

>  drivers/char/tpm/eventlog/efi.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
> index 6bb023de17f1..35229e5143ca 100644
> --- a/drivers/char/tpm/eventlog/efi.c
> +++ b/drivers/char/tpm/eventlog/efi.c
> @@ -41,6 +41,11 @@ int tpm_read_log_efi(struct tpm_chip *chip)
>  	log_size = log_tbl->size;
>  	memunmap(log_tbl);
>  
> +	if (!log_size) {
> +		pr_warn("UEFI TPM log area empty\n");
> +		return -EIO;
> +	}
> +
>  	log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl) + log_size,
>  			   MEMREMAP_WB);
>  	if (!log_tbl) {
> 
> base-commit: ed8780e3f2ecc82645342d070c6b4e530532e680
> -- 
> 2.25.1
> 
> 

/Jarkko
Kai-Heng Feng Oct. 30, 2020, 6:41 a.m. UTC | #3 
> On Oct 29, 2020, at 01:39, Tyler Hicks <tyhicks@linux.microsoft.com> wrote:
> 
> On 2020-10-28 11:30:11, Tyler Hicks wrote:
>> So, we need help from Kai, Kenneth, or Mimi to verify my assumption that
>> their firmware is providing an empty main event log and a populated
>> final event log.
> 
> Hi Kai, Kenneth, and Mimi - could one or two of you please follow these
> steps:
> 
> 1) Apply the proposed patch in the grandparent of this email so that
>   your kernel doesn't crash
> 2) Revert commit 7f3d176f5f7e ("tpm: Require that all digests are
>   present in TCG_PCR_EVENT2 structures") so that
>   __calc_tpm2_event_size() goes back to being less picky and may treat
>   a final log event as a valid event log header
> 3) Add some debugging warnings in efi_tpm_eventlog_init() to check for
>   an empty main event log and a populated final event log, as shown
>   below
> 4) Boot the resulting kernel build, look for the warnings, and report
>   back
> 
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index c1955d320fec..c4d2dbd5ed42 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -78,6 +78,9 @@ int __init efi_tpm_eventlog_init(void)
> 		goto out;
> 	}
> 
> +	WARN(!log_tbl->size && final_tbl->nr_events,
> +	     "nr_events = %llu\n", final_tbl->nr_events);
> +
> 	tbl_size = 0;
> 	if (final_tbl->nr_events != 0) {
> 		void *events = (void *)efi.tpm_final_log
> @@ -95,6 +98,8 @@ int __init efi_tpm_eventlog_init(void)
> 		goto out_calc;
> 	}
> 
> +	WARN(!log_tbl->size && tbl_size, "tbl_size = %d\n", tbl_size);
> +
> 	memblock_reserve((unsigned long)final_tbl,
> 			 tbl_size + sizeof(*final_tbl));
> 	efi_tpm_final_log_size = tbl_size;
> 
> For your convenience, I've created a branch with these changes on top of
> v5.9:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/tyhicks/linux.git/log/?h=tpm/bin-bios-measurements-debug

Dmesg attached.

Kai-Heng
> 
> Thank you!
> 
> Tyler
Kenneth Crudup Nov. 4, 2020, 2:12 a.m. UTC | #4 
On Wed, 28 Oct 2020, Tyler Hicks wrote:

> Hi Kai, Kenneth, and Mimi - could one or two of you please follow these
> steps:
...

OK, I built a kernel with the two patches applied, and with and without commit
7f3d176f5f7e, and when I run "sudo fwupdtpmevlog" (which normally causes a
panic() when 7f3d176f5f7e is in place) and the results are:

- With 7f3d176f5f7e reverted, no WARNs of any kind
- With 7f3d176f5f7e in place, same, I just get "UEFI TPM log area empty"

	-Kenny
diff mbox series

Patch

diff --git a/drivers/char/tpm/eventlog/efi.c b/drivers/char/tpm/eventlog/efi.c
index 6bb023de17f1..35229e5143ca 100644
--- a/drivers/char/tpm/eventlog/efi.c
+++ b/drivers/char/tpm/eventlog/efi.c
@@ -41,6 +41,11 @@  int tpm_read_log_efi(struct tpm_chip *chip)
 	log_size = log_tbl->size;
 	memunmap(log_tbl);
 
+	if (!log_size) {
+		pr_warn("UEFI TPM log area empty\n");
+		return -EIO;
+	}
+
 	log_tbl = memremap(efi.tpm_log, sizeof(*log_tbl) + log_size,
 			   MEMREMAP_WB);
 	if (!log_tbl) {