The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
Cc: stable@vger.kernel.org # v2.6.16+ Fixes: 55a82ab3181b ("[PATCH] tpm: add bios measurement log") Reported-by: Andy Liang andy.liang@hpe.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219495 Suggested-by: Matthew Garrett mjg59@srcf.ucam.org Tested-by: Andy Liang andy.liang@hpe.com Signed-off-by: Jarkko Sakkinen jarkko@kernel.org --- v5: * Spotted this right after sending: remove extra acpi_os_unmap_iomem() call. v4: * Added tested-by from Andy Liang. v3: * Flag mapping code in tpm{1,2}.c with CONFIG_ACPI (nios2 compilation fix). v2: * There was some extra cruft (irrelevant diff), which is now wiped away. * Added missing tags (fixes, stable). --- drivers/char/tpm/eventlog/acpi.c | 27 ++++++--------------- drivers/char/tpm/eventlog/common.c | 25 +++++++++++++------- drivers/char/tpm/eventlog/common.h | 28 ++++++++++++++++++++++ drivers/char/tpm/eventlog/tpm1.c | 30 ++++++++++++++--------- drivers/char/tpm/eventlog/tpm2.c | 38 +++++++++++++++++------------- include/linux/tpm.h | 1 + 6 files changed, 94 insertions(+), 55 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c index 69533d0bfb51..fb84dd3f6106 100644 --- a/drivers/char/tpm/eventlog/acpi.c +++ b/drivers/char/tpm/eventlog/acpi.c @@ -70,14 +70,11 @@ int tpm_read_log_acpi(struct tpm_chip *chip) acpi_status status; void __iomem *virt; u64 len, start; - struct tpm_bios_log *log; struct acpi_table_tpm2 *tbl; struct acpi_tpm2_phy *tpm2_phy; int format; int ret;
- log = &chip->log; - /* Unfortuntely ACPI does not associate the event log with a specific * TPM, like PPI. Thus all ACPI TPMs will read the same log. */ @@ -135,36 +132,26 @@ int tpm_read_log_acpi(struct tpm_chip *chip) return -EIO; }
- /* malloc EventLog space */ - log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL); - if (!log->bios_event_log) - return -ENOMEM; - - log->bios_event_log_end = log->bios_event_log + len; - virt = acpi_os_map_iomem(start, len); if (!virt) { dev_warn(&chip->dev, "%s: Failed to map ACPI memory\n", __func__); /* try EFI log next */ - ret = -ENODEV; - goto err; + return -ENODEV; }
- 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)) { + if (chip->flags & TPM_CHIP_FLAG_TPM2 && !tpm_is_tpm2_log(virt, len)) { /* try EFI log next */ ret = -ENODEV; goto err; }
+ acpi_os_unmap_iomem(virt, len); + chip->flags |= TPM_CHIP_FLAG_ACPI_LOG; + chip->log.bios_event_log = (void *)start; + chip->log.bios_event_log_end = (void *)start + len; return format;
err: - devm_kfree(&chip->dev, log->bios_event_log); - log->bios_event_log = NULL; + acpi_os_unmap_iomem(virt, len); return ret; } diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c index 4c0bbba64ee5..44340ca6e2ac 100644 --- a/drivers/char/tpm/eventlog/common.c +++ b/drivers/char/tpm/eventlog/common.c @@ -27,6 +27,7 @@ static int tpm_bios_measurements_open(struct inode *inode, { int err; struct seq_file *seq; + struct tpm_measurements *priv; struct tpm_chip_seqops *chip_seqops; const struct seq_operations *seqops; struct tpm_chip *chip; @@ -42,13 +43,18 @@ static int tpm_bios_measurements_open(struct inode *inode, get_device(&chip->dev); inode_unlock(inode);
- /* now register seq file */ + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + priv->chip = chip; + err = seq_open(file, seqops); - if (!err) { - seq = file->private_data; - seq->private = chip; - } else { + if (err) { + kfree(priv); put_device(&chip->dev); + } else { + seq = file->private_data; + seq->private = priv; }
return err; @@ -58,11 +64,14 @@ static int tpm_bios_measurements_release(struct inode *inode, struct file *file) { struct seq_file *seq = file->private_data; - struct tpm_chip *chip = seq->private; + struct tpm_measurements *priv = seq->private; + int ret;
- put_device(&chip->dev); + put_device(&priv->chip->dev); + ret = seq_release(inode, file); + kfree(priv);
- return seq_release(inode, file); + return ret; }
static const struct file_operations tpm_bios_measurements_ops = { diff --git a/drivers/char/tpm/eventlog/common.h b/drivers/char/tpm/eventlog/common.h index 47ff8136ceb5..b98fd6d9a6e9 100644 --- a/drivers/char/tpm/eventlog/common.h +++ b/drivers/char/tpm/eventlog/common.h @@ -1,12 +1,40 @@ #ifndef __TPM_EVENTLOG_COMMON_H__ #define __TPM_EVENTLOG_COMMON_H__
+#include <linux/acpi.h> #include "../tpm.h"
extern const struct seq_operations tpm1_ascii_b_measurements_seqops; extern const struct seq_operations tpm1_binary_b_measurements_seqops; extern const struct seq_operations tpm2_binary_b_measurements_seqops;
+struct tpm_measurements { + struct tpm_chip *chip; + void *start; + void *end; +}; + +static inline bool tpm_measurements_map(struct tpm_measurements *measurements) +{ + struct tpm_chip *chip = measurements->chip; + struct tpm_bios_log *log = &chip->log; + size_t size; + + size = log->bios_event_log_end - log->bios_event_log; + measurements->start = log->bios_event_log; + +#ifdef CONFIG_ACPI + if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG) + measurements->start = acpi_os_map_iomem((unsigned long)log->bios_event_log, size); +#endif + + if (!measurements->start) + return false; + + measurements->end = measurements->start + size; + return true; +} + #if defined(CONFIG_ACPI) int tpm_read_log_acpi(struct tpm_chip *chip); #else diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c index 12ee42a31c71..aef6ee39423a 100644 --- a/drivers/char/tpm/eventlog/tpm1.c +++ b/drivers/char/tpm/eventlog/tpm1.c @@ -70,20 +70,23 @@ static const char* tcpa_pc_event_id_strings[] = { static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos) { loff_t i = 0; - struct tpm_chip *chip = m->private; - struct tpm_bios_log *log = &chip->log; - void *addr = log->bios_event_log; - void *limit = log->bios_event_log_end; + struct tpm_measurements *priv = m->private; struct tcpa_event *event; u32 converted_event_size; u32 converted_event_type; + void *addr; + + if (!tpm_measurements_map(priv)) + return NULL; + + addr = priv->start;
/* read over *pos measurements */ do { event = addr;
/* check if current entry is valid */ - if (addr + sizeof(struct tcpa_event) > limit) + if (addr + sizeof(struct tcpa_event) > priv->end) return NULL;
converted_event_size = @@ -93,7 +96,7 @@ static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos)
if (((converted_event_type == 0) && (converted_event_size == 0)) || ((addr + sizeof(struct tcpa_event) + converted_event_size) - > limit)) + > priv->end)) return NULL;
if (i++ == *pos) @@ -109,9 +112,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, loff_t *pos) { struct tcpa_event *event = v; - struct tpm_chip *chip = m->private; - struct tpm_bios_log *log = &chip->log; - void *limit = log->bios_event_log_end; + struct tpm_measurements *priv = m->private; u32 converted_event_size; u32 converted_event_type;
@@ -121,7 +122,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, v += sizeof(struct tcpa_event) + converted_event_size;
/* now check if current entry is valid */ - if ((v + sizeof(struct tcpa_event)) > limit) + if ((v + sizeof(struct tcpa_event)) > priv->end) return NULL;
event = v; @@ -130,7 +131,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, converted_event_type = do_endian_conversion(event->event_type);
if (((converted_event_type == 0) && (converted_event_size == 0)) || - ((v + sizeof(struct tcpa_event) + converted_event_size) > limit)) + ((v + sizeof(struct tcpa_event) + converted_event_size) > priv->end)) return NULL;
return v; @@ -138,6 +139,13 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
static void tpm1_bios_measurements_stop(struct seq_file *m, void *v) { +#ifdef CONFIG_ACPI + struct tpm_measurements *priv = m->private; + struct tpm_chip *chip = priv->chip; + + if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG) + acpi_os_unmap_iomem(priv->start, priv->end - priv->start); +#endif }
static int get_event_name(char *dest, struct tcpa_event *event, diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index 37a05800980c..6289d8893e46 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -41,20 +41,22 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) { - struct tpm_chip *chip = m->private; - struct tpm_bios_log *log = &chip->log; - void *addr = log->bios_event_log; - void *limit = log->bios_event_log_end; + struct tpm_measurements *priv = m->private; struct tcg_pcr_event *event_header; struct tcg_pcr_event2_head *event; size_t size; + void *addr; int i;
+ if (!tpm_measurements_map(priv)) + return NULL; + + addr = priv->start; event_header = addr; size = struct_size(event_header, event, event_header->event_size);
if (*pos == 0) { - if (addr + size < limit) { + if (addr + size < priv->end) { if ((event_header->event_type == 0) && (event_header->event_size == 0)) return NULL; @@ -66,7 +68,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) addr += size; event = addr; size = calc_tpm2_event_size(event, event_header); - if ((addr + size >= limit) || (size == 0)) + if ((addr + size >= priv->end) || !size) return NULL; }
@@ -74,7 +76,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) event = addr; size = calc_tpm2_event_size(event, event_header);
- if ((addr + size >= limit) || (size == 0)) + if ((addr + size >= priv->end) || !size) return NULL; addr += size; } @@ -87,14 +89,12 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, { struct tcg_pcr_event *event_header; struct tcg_pcr_event2_head *event; - struct tpm_chip *chip = m->private; - struct tpm_bios_log *log = &chip->log; - void *limit = log->bios_event_log_end; + struct tpm_measurements *priv = m->private; size_t event_size; void *marker;
(*pos)++; - event_header = log->bios_event_log; + event_header = priv->start;
if (v == SEQ_START_TOKEN) { event_size = struct_size(event_header, event, @@ -109,13 +109,13 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, }
marker = marker + event_size; - if (marker >= limit) + if (marker >= priv->end) return NULL; v = marker; event = v;
event_size = calc_tpm2_event_size(event, event_header); - if (((v + event_size) >= limit) || (event_size == 0)) + if (((v + event_size) >= priv->end) || !event_size) return NULL;
return v; @@ -123,13 +123,19 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
static void tpm2_bios_measurements_stop(struct seq_file *m, void *v) { +#ifdef CONFIG_ACPI + struct tpm_measurements *priv = m->private; + struct tpm_chip *chip = priv->chip; + + if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG) + acpi_os_unmap_iomem(priv->start, priv->end - priv->start); +#endif }
static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v) { - struct tpm_chip *chip = m->private; - struct tpm_bios_log *log = &chip->log; - struct tcg_pcr_event *event_header = log->bios_event_log; + struct tpm_measurements *priv = m->private; + struct tcg_pcr_event *event_header = priv->start; struct tcg_pcr_event2_head *event = v; void *temp_ptr; size_t size; diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 20a40ade8030..f3d12738b93b 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -348,6 +348,7 @@ enum tpm_chip_flags { TPM_CHIP_FLAG_SUSPENDED = BIT(8), TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9), TPM_CHIP_FLAG_DISABLE = BIT(10), + TPM_CHIP_FLAG_ACPI_LOG = BIT(11), };
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
On Tue Dec 24, 2024 at 6:03 AM EET, Jarkko Sakkinen wrote:
The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
Cc: stable@vger.kernel.org # v2.6.16+ Fixes: 55a82ab3181b ("[PATCH] tpm: add bios measurement log") Reported-by: Andy Liang andy.liang@hpe.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219495 Suggested-by: Matthew Garrett mjg59@srcf.ucam.org Tested-by: Andy Liang andy.liang@hpe.com Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
Doing some weird truncate here would be pointless even if it is "too large". It's HPE's problem, not ours. The onnly piece of code where the fix makes any mentionable changes is really acpi.c and I've tested that quite throughly already...
In some other version of the hardware the size was BTW 8 MiB (according to TPM2 table contents) and later on it changed to 16 MiB (according to transcript above i.e. RSI). That is weird but I don't think we should care.
BR, Jarkko
On Tue, 24 Dec 2024 at 05:03, Jarkko Sakkinen jarkko@kernel.org wrote:
The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
How can you be sure the memory contents will be preserved? Does it say anywhere in the TCG spec that this needs to use a memory type that is preserved by default?
Also, the fact that we're now at v5 kind of proves my point that this approach may be too complex for a simple bug fix. Why not switch to kvmalloc() for a backportable fix, and improve upon that for future kernels?
Cc: stable@vger.kernel.org # v2.6.16+ Fixes: 55a82ab3181b ("[PATCH] tpm: add bios measurement log") Reported-by: Andy Liang andy.liang@hpe.com Closes: https://bugzilla.kernel.org/show_bug.cgi?id=219495 Suggested-by: Matthew Garrett mjg59@srcf.ucam.org Tested-by: Andy Liang andy.liang@hpe.com Signed-off-by: Jarkko Sakkinen jarkko@kernel.org
v5:
- Spotted this right after sending: remove extra acpi_os_unmap_iomem() call.
v4:
- Added tested-by from Andy Liang.
v3:
- Flag mapping code in tpm{1,2}.c with CONFIG_ACPI (nios2 compilation fix).
v2:
- There was some extra cruft (irrelevant diff), which is now wiped away.
- Added missing tags (fixes, stable).
drivers/char/tpm/eventlog/acpi.c | 27 ++++++--------------- drivers/char/tpm/eventlog/common.c | 25 +++++++++++++------- drivers/char/tpm/eventlog/common.h | 28 ++++++++++++++++++++++ drivers/char/tpm/eventlog/tpm1.c | 30 ++++++++++++++--------- drivers/char/tpm/eventlog/tpm2.c | 38 +++++++++++++++++------------- include/linux/tpm.h | 1 + 6 files changed, 94 insertions(+), 55 deletions(-)
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c index 69533d0bfb51..fb84dd3f6106 100644 --- a/drivers/char/tpm/eventlog/acpi.c +++ b/drivers/char/tpm/eventlog/acpi.c @@ -70,14 +70,11 @@ int tpm_read_log_acpi(struct tpm_chip *chip) acpi_status status; void __iomem *virt; u64 len, start;
struct tpm_bios_log *log; struct acpi_table_tpm2 *tbl; struct acpi_tpm2_phy *tpm2_phy; int format; int ret;log = &chip->log;/* Unfortuntely ACPI does not associate the event log with a specific * TPM, like PPI. Thus all ACPI TPMs will read the same log. */@@ -135,36 +132,26 @@ int tpm_read_log_acpi(struct tpm_chip *chip) return -EIO; }
/* malloc EventLog space */log->bios_event_log = devm_kmalloc(&chip->dev, len, GFP_KERNEL);if (!log->bios_event_log)return -ENOMEM;log->bios_event_log_end = log->bios_event_log + len;virt = acpi_os_map_iomem(start, len); if (!virt) { dev_warn(&chip->dev, "%s: Failed to map ACPI memory\n", __func__); /* try EFI log next */ret = -ENODEV;goto err;
return -ENODEV; }
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)) {
if (chip->flags & TPM_CHIP_FLAG_TPM2 && !tpm_is_tpm2_log(virt, len)) { /* try EFI log next */ ret = -ENODEV; goto err; }acpi_os_unmap_iomem(virt, len);chip->flags |= TPM_CHIP_FLAG_ACPI_LOG;chip->log.bios_event_log = (void *)start;chip->log.bios_event_log_end = (void *)start + len; return format;err:
devm_kfree(&chip->dev, log->bios_event_log);log->bios_event_log = NULL;
acpi_os_unmap_iomem(virt, len); return ret;} diff --git a/drivers/char/tpm/eventlog/common.c b/drivers/char/tpm/eventlog/common.c index 4c0bbba64ee5..44340ca6e2ac 100644 --- a/drivers/char/tpm/eventlog/common.c +++ b/drivers/char/tpm/eventlog/common.c @@ -27,6 +27,7 @@ static int tpm_bios_measurements_open(struct inode *inode, { int err; struct seq_file *seq;
struct tpm_measurements *priv; struct tpm_chip_seqops *chip_seqops; const struct seq_operations *seqops; struct tpm_chip *chip;@@ -42,13 +43,18 @@ static int tpm_bios_measurements_open(struct inode *inode, get_device(&chip->dev); inode_unlock(inode);
/* now register seq file */
priv = kzalloc(sizeof(*priv), GFP_KERNEL);if (!priv)return -ENOMEM;priv->chip = chip;err = seq_open(file, seqops);
if (!err) {seq = file->private_data;seq->private = chip;} else {
if (err) {kfree(priv); put_device(&chip->dev);} else {seq = file->private_data;seq->private = priv; } return err;@@ -58,11 +64,14 @@ static int tpm_bios_measurements_release(struct inode *inode, struct file *file) { struct seq_file *seq = file->private_data;
struct tpm_chip *chip = seq->private;
struct tpm_measurements *priv = seq->private;int ret;
put_device(&chip->dev);
put_device(&priv->chip->dev);ret = seq_release(inode, file);kfree(priv);
return seq_release(inode, file);
return ret;}
static const struct file_operations tpm_bios_measurements_ops = { diff --git a/drivers/char/tpm/eventlog/common.h b/drivers/char/tpm/eventlog/common.h index 47ff8136ceb5..b98fd6d9a6e9 100644 --- a/drivers/char/tpm/eventlog/common.h +++ b/drivers/char/tpm/eventlog/common.h @@ -1,12 +1,40 @@ #ifndef __TPM_EVENTLOG_COMMON_H__ #define __TPM_EVENTLOG_COMMON_H__
+#include <linux/acpi.h> #include "../tpm.h"
extern const struct seq_operations tpm1_ascii_b_measurements_seqops; extern const struct seq_operations tpm1_binary_b_measurements_seqops; extern const struct seq_operations tpm2_binary_b_measurements_seqops;
+struct tpm_measurements {
struct tpm_chip *chip;void *start;void *end;+};
+static inline bool tpm_measurements_map(struct tpm_measurements *measurements) +{
struct tpm_chip *chip = measurements->chip;struct tpm_bios_log *log = &chip->log;size_t size;size = log->bios_event_log_end - log->bios_event_log;measurements->start = log->bios_event_log;+#ifdef CONFIG_ACPI
if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG)measurements->start = acpi_os_map_iomem((unsigned long)log->bios_event_log, size);+#endif
if (!measurements->start)return false;measurements->end = measurements->start + size;return true;+}
#if defined(CONFIG_ACPI) int tpm_read_log_acpi(struct tpm_chip *chip); #else diff --git a/drivers/char/tpm/eventlog/tpm1.c b/drivers/char/tpm/eventlog/tpm1.c index 12ee42a31c71..aef6ee39423a 100644 --- a/drivers/char/tpm/eventlog/tpm1.c +++ b/drivers/char/tpm/eventlog/tpm1.c @@ -70,20 +70,23 @@ static const char* tcpa_pc_event_id_strings[] = { static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos) { loff_t i = 0;
struct tpm_chip *chip = m->private;struct tpm_bios_log *log = &chip->log;void *addr = log->bios_event_log;void *limit = log->bios_event_log_end;
struct tpm_measurements *priv = m->private; struct tcpa_event *event; u32 converted_event_size; u32 converted_event_type;void *addr;if (!tpm_measurements_map(priv))return NULL;addr = priv->start; /* read over *pos measurements */ do { event = addr; /* check if current entry is valid */
if (addr + sizeof(struct tcpa_event) > limit)
if (addr + sizeof(struct tcpa_event) > priv->end) return NULL; converted_event_size =@@ -93,7 +96,7 @@ static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos)
if (((converted_event_type == 0) && (converted_event_size == 0)) || ((addr + sizeof(struct tcpa_event) + converted_event_size)
> limit))
> priv->end)) return NULL; if (i++ == *pos)@@ -109,9 +112,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, loff_t *pos) { struct tcpa_event *event = v;
struct tpm_chip *chip = m->private;struct tpm_bios_log *log = &chip->log;void *limit = log->bios_event_log_end;
struct tpm_measurements *priv = m->private; u32 converted_event_size; u32 converted_event_type;@@ -121,7 +122,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, v += sizeof(struct tcpa_event) + converted_event_size;
/* now check if current entry is valid */
if ((v + sizeof(struct tcpa_event)) > limit)
if ((v + sizeof(struct tcpa_event)) > priv->end) return NULL; event = v;@@ -130,7 +131,7 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, converted_event_type = do_endian_conversion(event->event_type);
if (((converted_event_type == 0) && (converted_event_size == 0)) ||
((v + sizeof(struct tcpa_event) + converted_event_size) > limit))
((v + sizeof(struct tcpa_event) + converted_event_size) > priv->end)) return NULL; return v;@@ -138,6 +139,13 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v,
static void tpm1_bios_measurements_stop(struct seq_file *m, void *v) { +#ifdef CONFIG_ACPI
struct tpm_measurements *priv = m->private;struct tpm_chip *chip = priv->chip;if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG)acpi_os_unmap_iomem(priv->start, priv->end - priv->start);+#endif }
static int get_event_name(char *dest, struct tcpa_event *event, diff --git a/drivers/char/tpm/eventlog/tpm2.c b/drivers/char/tpm/eventlog/tpm2.c index 37a05800980c..6289d8893e46 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -41,20 +41,22 @@ static size_t calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) {
struct tpm_chip *chip = m->private;struct tpm_bios_log *log = &chip->log;void *addr = log->bios_event_log;void *limit = log->bios_event_log_end;
struct tpm_measurements *priv = m->private; struct tcg_pcr_event *event_header; struct tcg_pcr_event2_head *event; size_t size;void *addr; int i;if (!tpm_measurements_map(priv))return NULL;addr = priv->start; event_header = addr; size = struct_size(event_header, event, event_header->event_size); if (*pos == 0) {
if (addr + size < limit) {
if (addr + size < priv->end) { if ((event_header->event_type == 0) && (event_header->event_size == 0)) return NULL;@@ -66,7 +68,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) addr += size; event = addr; size = calc_tpm2_event_size(event, event_header);
if ((addr + size >= limit) || (size == 0))
if ((addr + size >= priv->end) || !size) return NULL; }@@ -74,7 +76,7 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) event = addr; size = calc_tpm2_event_size(event, event_header);
if ((addr + size >= limit) || (size == 0))
if ((addr + size >= priv->end) || !size) return NULL; addr += size; }@@ -87,14 +89,12 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, { struct tcg_pcr_event *event_header; struct tcg_pcr_event2_head *event;
struct tpm_chip *chip = m->private;struct tpm_bios_log *log = &chip->log;void *limit = log->bios_event_log_end;
struct tpm_measurements *priv = m->private; size_t event_size; void *marker; (*pos)++;
event_header = log->bios_event_log;
event_header = priv->start; if (v == SEQ_START_TOKEN) { event_size = struct_size(event_header, event,@@ -109,13 +109,13 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, }
marker = marker + event_size;
if (marker >= limit)
if (marker >= priv->end) return NULL; v = marker; event = v; event_size = calc_tpm2_event_size(event, event_header);
if (((v + event_size) >= limit) || (event_size == 0))
if (((v + event_size) >= priv->end) || !event_size) return NULL; return v;@@ -123,13 +123,19 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v,
static void tpm2_bios_measurements_stop(struct seq_file *m, void *v) { +#ifdef CONFIG_ACPI
struct tpm_measurements *priv = m->private;struct tpm_chip *chip = priv->chip;if (chip->flags & TPM_CHIP_FLAG_ACPI_LOG)acpi_os_unmap_iomem(priv->start, priv->end - priv->start);+#endif }
static int tpm2_binary_bios_measurements_show(struct seq_file *m, void *v) {
struct tpm_chip *chip = m->private;struct tpm_bios_log *log = &chip->log;struct tcg_pcr_event *event_header = log->bios_event_log;
struct tpm_measurements *priv = m->private;struct tcg_pcr_event *event_header = priv->start; struct tcg_pcr_event2_head *event = v; void *temp_ptr; size_t size;diff --git a/include/linux/tpm.h b/include/linux/tpm.h index 20a40ade8030..f3d12738b93b 100644 --- a/include/linux/tpm.h +++ b/include/linux/tpm.h @@ -348,6 +348,7 @@ enum tpm_chip_flags { TPM_CHIP_FLAG_SUSPENDED = BIT(8), TPM_CHIP_FLAG_HWRNG_DISABLED = BIT(9), TPM_CHIP_FLAG_DISABLE = BIT(10),
TPM_CHIP_FLAG_ACPI_LOG = BIT(11),};
#define to_tpm_chip(d) container_of(d, struct tpm_chip, dev)
2.47.1
On Tue Dec 24, 2024 at 6:05 PM EET, Ard Biesheuvel wrote:
On Tue, 24 Dec 2024 at 05:03, Jarkko Sakkinen jarkko@kernel.org wrote:
The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
How can you be sure the memory contents will be preserved? Does it say anywhere in the TCG spec that this needs to use a memory type that is preserved by default?
TCG log calls the size as the minimum size for the log area but is not too accurate on details [1]. I don't actually know what "minimum" even means in this context as it is just a fixed size cut of the physical address space.
I don't think that can ever change. It would be oddballs if some dynamic change would make ACPI tables show incorrect information on memory ranges. Do you know any pre-existing example of such behavior (not sarcasm, just interested)?
Anyway considering this type of dynamics TCG spec is inaccurate.
Also, the fact that we're now at v5 kind of proves my point that this approach may be too complex for a simple bug fix. Why not switch to kvmalloc() for a backportable fix, and improve upon that for future kernels?
OK, I could possibly live with this. 16 MiB is not that much with current memory sizes so if everyone agrees this then it is fine and I'll change this patch as feature for my next PR. I just don't want to decide any abritrarily chosen truncate range. For me it just feels wasting memory for no reason, that's all.
Alternatively the code do pre-fetch iteration of what happens when you do "cat /sys/kernel/security/tpm0/binary_measurements" and then we would end up about 100 kB or similar figure with this hardware but that would require code I already did and few bits more for full implementation.
[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_ACPIGeneralSpec_v1p...
BR, Jarkko
On Wed, 25 Dec 2024 at 16:31, Jarkko Sakkinen jarkko@kernel.org wrote:
On Tue Dec 24, 2024 at 6:05 PM EET, Ard Biesheuvel wrote:
On Tue, 24 Dec 2024 at 05:03, Jarkko Sakkinen jarkko@kernel.org wrote:
The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
How can you be sure the memory contents will be preserved? Does it say anywhere in the TCG spec that this needs to use a memory type that is preserved by default?
TCG log calls the size as the minimum size for the log area but is not too accurate on details [1]. I don't actually know what "minimum" even means in this context as it is just a fixed size cut of the physical address space.
I don't think that can ever change. It would be oddballs if some dynamic change would make ACPI tables show incorrect information on memory ranges. Do you know any pre-existing example of such behavior (not sarcasm, just interested)?
Anyway considering this type of dynamics TCG spec is inaccurate.
Thanks for the context but that is not at all what I was asking.
This change assumes that the contents of the memory region described by the ACPI table will be reserved in some way, and not be released to the kernel for general allocation.
This is not always the case for firmware tables: EFI configuration tables need to be reserved explicitly unless the memory type is EfiRuntimeServicesData. For ACPI tables, the situation might be different but there is at least one example (BGRT) where the memory type typically used is not one that the kernel usually reserves by default.
So my question is whether there is anything in the TCG platform spec (or whichever spec describes this ACPI table) that guarantees that the region that the TCPA or TPM2 table points to is of a type that does not require an explicit reservation?
On Mon Jan 6, 2025 at 7:23 PM EET, Ard Biesheuvel wrote:
On Wed, 25 Dec 2024 at 16:31, Jarkko Sakkinen jarkko@kernel.org wrote:
On Tue Dec 24, 2024 at 6:05 PM EET, Ard Biesheuvel wrote:
On Tue, 24 Dec 2024 at 05:03, Jarkko Sakkinen jarkko@kernel.org wrote:
The following failure was reported:
[ 10.693310][ T1] tpm_tis STM0925:00: 2.0 TPM (device-id 0x3, rev-id 0) [ 10.848132][ T1] ------------[ cut here ]------------ [ 10.853559][ T1] WARNING: CPU: 59 PID: 1 at mm/page_alloc.c:4727 __alloc_pages_noprof+0x2ca/0x330 [ 10.862827][ T1] Modules linked in: [ 10.866671][ T1] CPU: 59 UID: 0 PID: 1 Comm: swapper/0 Not tainted 6.12.0-lp155.2.g52785e2-default #1 openSUSE Tumbleweed (unreleased) 588cd98293a7c9eba9013378d807364c088c9375 [ 10.882741][ T1] Hardware name: HPE ProLiant DL320 Gen12/ProLiant DL320 Gen12, BIOS 1.20 10/28/2024 [ 10.892170][ T1] RIP: 0010:__alloc_pages_noprof+0x2ca/0x330 [ 10.898103][ T1] Code: 24 08 e9 4a fe ff ff e8 34 36 fa ff e9 88 fe ff ff 83 fe 0a 0f 86 b3 fd ff ff 80 3d 01 e7 ce 01 00 75 09 c6 05 f8 e6 ce 01 01 <0f> 0b 45 31 ff e9 e5 fe ff ff f7 c2 00 00 08 00 75 42 89 d9 80 e1 [ 10.917750][ T1] RSP: 0000:ffffb7cf40077980 EFLAGS: 00010246 [ 10.923777][ T1] RAX: 0000000000000000 RBX: 0000000000040cc0 RCX: 0000000000000000 [ 10.931727][ T1] RDX: 0000000000000000 RSI: 000000000000000c RDI: 0000000000040cc0
Above shows that ACPI pointed a 16 MiB buffer for the log events because RSI maps to the 'order' parameter of __alloc_pages_noprof(). Address the bug by mapping the region when needed instead of copying.
How can you be sure the memory contents will be preserved? Does it say anywhere in the TCG spec that this needs to use a memory type that is preserved by default?
TCG log calls the size as the minimum size for the log area but is not too accurate on details [1]. I don't actually know what "minimum" even means in this context as it is just a fixed size cut of the physical address space.
I don't think that can ever change. It would be oddballs if some dynamic change would make ACPI tables show incorrect information on memory ranges. Do you know any pre-existing example of such behavior (not sarcasm, just interested)?
Anyway considering this type of dynamics TCG spec is inaccurate.
Thanks for the context but that is not at all what I was asking.
This change assumes that the contents of the memory region described by the ACPI table will be reserved in some way, and not be released to the kernel for general allocation.
This is not always the case for firmware tables: EFI configuration tables need to be reserved explicitly unless the memory type is EfiRuntimeServicesData. For ACPI tables, the situation might be different but there is at least one example (BGRT) where the memory type typically used is not one that the kernel usually reserves by default.
So my question is whether there is anything in the TCG platform spec (or whichever spec describes this ACPI table) that guarantees that the region that the TCPA or TPM2 table points to is of a type that does not require an explicit reservation?
I agree that we must assume that we cannot guarantee taht since it is open in the spec. I think I went over the top with this.
Let's go with the simpler devm_add_action_or_reset() fix.
BR, Jarkko
linux-stable-mirror@lists.linaro.org
-
Ard Biesheuvel -
Jarkko Sakkinen