From: Jarkko Sakkinen <jarkko@kernel.org>
To: Tyler Hicks <tyhicks@linux.microsoft.com>
Cc: "Peter Huewe" <peterhuewe@gmx.de>,
"Jason Gunthorpe" <jgg@ziepe.ca>,
linux-integrity@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-efi@vger.kernel.org,
"Kai-Heng Feng" <kai.heng.feng@canonical.com>,
"Kenneth R . Crudup" <kenny@panix.com>,
"Mimi Zohar" <zohar@linux.ibm.com>,
"Thiébaud Weksteen" <tweek@google.com>,
"Ard Biesheuvel" <ardb@kernel.org>
Subject: Re: [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log
Date: Fri, 30 Oct 2020 06:46:26 +0200 [thread overview]
Message-ID: <20201030044626.GA36779@kernel.org> (raw)
In-Reply-To: <20201028154102.9595-1-tyhicks@linux.microsoft.com>
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
prev parent reply other threads:[~2020-10-30 4:46 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-28 12:31 [Regression] "tpm: Require that all digests are present in TCG_PCR_EVENT2 structures" causes null pointer dereference Kai-Heng Feng
2020-09-28 14:06 ` Jarkko Sakkinen
2020-09-28 14:16 ` Kai-Heng Feng
[not found] ` <20200928155215.GA92669@linux.intel.com>
2020-09-28 16:15 ` Ard Biesheuvel
2020-09-28 16:26 ` Ard Biesheuvel
2020-09-28 17:12 ` Jarkko Sakkinen
2020-09-28 18:05 ` Kenneth R. Crudup
2020-09-29 17:52 ` Mimi Zohar
2020-09-30 2:20 ` Jarkko Sakkinen
2020-10-08 9:09 ` Kai-Heng Feng
2020-10-09 16:06 ` Jarkko Sakkinen
2020-10-13 14:31 ` Tyler Hicks
2020-10-13 14:33 ` Jarkko Sakkinen
2020-10-20 21:07 ` Mimi Zohar
2020-10-21 5:48 ` Tyler Hicks
2020-10-26 5:49 ` Kai-Heng Feng
2020-10-27 5:08 ` Tyler Hicks
2020-10-28 15:41 ` [PATCH] tpm: efi: Don't create binary_bios_measurements file for an empty log Tyler Hicks
2020-10-28 16:30 ` Tyler Hicks
2020-10-28 17:39 ` Tyler Hicks
2020-10-30 6:41 ` Kai-Heng Feng
2020-11-04 2:12 ` Kenneth R. Crudup
2020-10-30 4:46 ` Jarkko Sakkinen [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201030044626.GA36779@kernel.org \
--to=jarkko@kernel.org \
--cc=ardb@kernel.org \
--cc=jgg@ziepe.ca \
--cc=kai.heng.feng@canonical.com \
--cc=kenny@panix.com \
--cc=linux-efi@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=peterhuewe@gmx.de \
--cc=tweek@google.com \
--cc=tyhicks@linux.microsoft.com \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).