From: Jarkko Sakkinen <jarkko@kernel.org>
To: Stefan Berger <stefanb@linux.ibm.com>
Cc: peterhuewe@gmx.de, jgg@ziepe.ca, linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size
Date: Thu, 11 Mar 2021 01:14:32 +0200 [thread overview]
Message-ID: <YElS2By8CXCOWody@kernel.org> (raw)
In-Reply-To: <20210310221916.356716-2-stefanb@linux.ibm.com>
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
next prev parent reply other threads:[~2021-03-10 23:15 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-10 22:19 [PATCH v2 0/3] Fix bugs related to TPM2 event log Stefan Berger
2021-03-10 22:19 ` [PATCH v2 1/3] tpm: efi: Use local variable for calculating final log size Stefan Berger
2021-03-10 23:14 ` Jarkko Sakkinen [this message]
2021-03-10 23:21 ` Jarkko Sakkinen
2021-03-10 23:24 ` Jarkko Sakkinen
2021-03-11 14:02 ` Stefan Berger
2021-03-12 16:54 ` Jarkko Sakkinen
2021-03-10 22:19 ` [PATCH v2 2/3] tpm: acpi: Check eventlog signature before using it Stefan Berger
2021-03-10 23:13 ` Jarkko Sakkinen
2021-03-10 22:19 ` [PATCH v2 3/3] tpm: vtpm_proxy: Avoid reading host log when using a virtual device Stefan Berger
2021-03-10 23:04 ` Jarkko Sakkinen
2021-03-10 23:04 ` [PATCH v2 0/3] Fix bugs related to TPM2 event log Jarkko Sakkinen
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=YElS2By8CXCOWody@kernel.org \
--to=jarkko@kernel.org \
--cc=jgg@ziepe.ca \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=peterhuewe@gmx.de \
--cc=stefanb@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.