| Message ID | 20241221113318.562138-1-jarkko@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | tpm: Map the ACPI provided event log | expand |
On Sat, 21 Dec 2024 at 12:33, 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. > > 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> > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> This is a very intrusive fix - care to provide some more context on why all these changes are needed? > --- > drivers/char/tpm/eventlog/acpi.c | 30 +++++-------------- > drivers/char/tpm/eventlog/common.c | 33 ++++++++++++-------- > drivers/char/tpm/eventlog/common.h | 6 ++++ > drivers/char/tpm/eventlog/tpm1.c | 38 +++++++++++++++-------- > drivers/char/tpm/eventlog/tpm2.c | 48 +++++++++++++++++++----------- > include/linux/tpm.h | 1 + > 6 files changed, 91 insertions(+), 65 deletions(-) > > diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c > index 69533d0bfb51..8d8db66ce876 100644 > --- a/drivers/char/tpm/eventlog/acpi.c > +++ b/drivers/char/tpm/eventlog/acpi.c > @@ -22,8 +22,6 @@ > #include <linux/slab.h> > #include <linux/acpi.h> > #include <linux/tpm_eventlog.h> > - > -#include "../tpm.h" > #include "common.h" > > struct acpi_tcpa { > @@ -70,14 +68,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 +130,27 @@ 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)) { > + acpi_os_unmap_iomem(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..d934ef8c8b7f 100644 > --- a/drivers/char/tpm/eventlog/common.c > +++ b/drivers/char/tpm/eventlog/common.c > @@ -25,11 +25,12 @@ > static int tpm_bios_measurements_open(struct inode *inode, > struct file *file) > { > - 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; > + struct seq_file *seq; > + int ret; > > inode_lock(inode); > if (!inode->i_private) { > @@ -42,27 +43,35 @@ static int tpm_bios_measurements_open(struct inode *inode, > get_device(&chip->dev); > inode_unlock(inode); > > - /* now register seq file */ > - err = seq_open(file, seqops); > - if (!err) { > - seq = file->private_data; > - seq->private = chip; > - } else { > + priv = kzalloc(sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + priv->chip = chip; > + > + ret = seq_open(file, seqops); > + if (ret) { > + kfree(priv); > put_device(&chip->dev); > + } else { > + seq = file->private_data; > + seq->private = priv; > } > > - return err; > + return ret; > } > > 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..ad89a0daf585 100644 > --- a/drivers/char/tpm/eventlog/common.h > +++ b/drivers/char/tpm/eventlog/common.h > @@ -7,6 +7,12 @@ 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; > +}; > + > #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..6141a580e99c 100644 > --- a/drivers/char/tpm/eventlog/tpm1.c > +++ b/drivers/char/tpm/eventlog/tpm1.c > @@ -22,11 +22,8 @@ > #include <linux/module.h> > #include <linux/slab.h> > #include <linux/tpm_eventlog.h> > - > -#include "../tpm.h" > #include "common.h" > > - > static const char* tcpa_event_type_strings[] = { > "PREBOOT", > "POST CODE", > @@ -70,20 +67,32 @@ 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_measurements *priv = m->private; > + struct tpm_chip *chip = priv->chip; > struct tpm_bios_log *log = &chip->log; > - void *addr = log->bios_event_log; > - void *limit = log->bios_event_log_end; > struct tcpa_event *event; > u32 converted_event_size; > u32 converted_event_type; > + size_t log_size; > + void *addr; > + > + log_size = log->bios_event_log_end - log->bios_event_log; > + > + priv->start = !(chip->flags & TPM_CHIP_FLAG_ACPI_LOG) ? > + log->bios_event_log : > + acpi_os_map_iomem((unsigned long)log->bios_event_log, log_size); > + if (!priv->start) > + return NULL; > + priv->end = priv->start + log_size; > + > + 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 +102,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) > @@ -108,10 +117,8 @@ static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos) > static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, > loff_t *pos) > { > + struct tpm_measurements *priv = m->private; > 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; > u32 converted_event_size; > u32 converted_event_type; > > @@ -121,7 +128,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 +137,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 +145,11 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, > > static void tpm1_bios_measurements_stop(struct seq_file *m, void *v) > { > + 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); > } > > 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..79e090dd751a 100644 > --- a/drivers/char/tpm/eventlog/tpm2.c > +++ b/drivers/char/tpm/eventlog/tpm2.c > @@ -12,14 +12,13 @@ > * content. > */ > > +#include "linux/tpm.h" > #include <linux/seq_file.h> > #include <linux/fs.h> > #include <linux/security.h> > #include <linux/module.h> > #include <linux/slab.h> > #include <linux/tpm_eventlog.h> > - > -#include "../tpm.h" > #include "common.h" > > /* > @@ -41,20 +40,31 @@ 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_measurements *priv = m->private; > + struct tpm_chip *chip = priv->chip; > struct tpm_bios_log *log = &chip->log; > - void *addr = log->bios_event_log; > - void *limit = log->bios_event_log_end; > struct tcg_pcr_event *event_header; > struct tcg_pcr_event2_head *event; > - size_t size; > + size_t size, log_size; > + void *addr; > int i; > > + log_size = log->bios_event_log_end - log->bios_event_log; > + > + priv->start = !(chip->flags & TPM_CHIP_FLAG_ACPI_LOG) ? > + log->bios_event_log : > + acpi_os_map_iomem((unsigned long)log->bios_event_log, log_size); > + if (!priv->start) > + return NULL; > + > + priv->end = priv->start + log_size; > + > + 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 +76,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 +84,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; > } > @@ -85,16 +95,14 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) > static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, > loff_t *pos) > { > + struct tpm_measurements *priv = m->private; > 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; > 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 +117,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 +131,17 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, > > static void tpm2_bios_measurements_stop(struct seq_file *m, void *v) > { > + 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); > } > > 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 Sat, 2024-12-21 at 17:04 +0100, Ard Biesheuvel wrote: > On Sat, 21 Dec 2024 at 12:33, 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. > > > > 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> > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > This is a very intrusive fix - care to provide some more context on > why all these changes are needed? Since the bug reports never found an actual log over a few tens of kilobytes this is caused by the BIOS implementation allocating a huge buffer that is mostly unused. There are two other possibilities for fixing this, which were both part of the original suggestions. One would be to work out the size of the log and then allocate an exact size. This would require implementing tpm1 and tpm2 parsers for log size. However, since we can never go over KMALLOC_MAX_SIZE without an error even with this calculated size, the simplest straight line fix would be to cap the copy at KMALLOC_MAX_SIZE if it's over. That would be a simple one liner. Regards, James
On Sat Dec 21, 2024 at 7:16 PM EET, James Bottomley wrote: > On Sat, 2024-12-21 at 17:04 +0100, Ard Biesheuvel wrote: > > On Sat, 21 Dec 2024 at 12:33, 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. > > > > > > 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> > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > This is a very intrusive fix - care to provide some more context on > > why all these changes are needed? > > Since the bug reports never found an actual log over a few tens of > kilobytes this is caused by the BIOS implementation allocating a huge > buffer that is mostly unused. > > There are two other possibilities for fixing this, which were both part > of the original suggestions. One would be to work out the size of the > log and then allocate an exact size. This would require implementing > tpm1 and tpm2 parsers for log size. However, since we can never go > over KMALLOC_MAX_SIZE without an error even with this calculated size, > the simplest straight line fix would be to cap the copy at > KMALLOC_MAX_SIZE if it's over. That would be a simple one liner. All I'm saying is this. I've got bunch of complains of this from mainly SUSE, and now I'm here with a response to that feedback. So I don't care. You decide. I'm 100% sure that the fix that Stefan proposed is not a sustainable path in long-term, so I guess this was more like more long-term but intrusive fix. Ya, and also please test the changes, especially anything that can reach of OF eventlogs would be welcome feedback. BR, Jarkko
On Sat Dec 21, 2024 at 10:11 PM EET, Jarkko Sakkinen wrote: > On Sat Dec 21, 2024 at 7:16 PM EET, James Bottomley wrote: > > On Sat, 2024-12-21 at 17:04 +0100, Ard Biesheuvel wrote: > > > On Sat, 21 Dec 2024 at 12:33, 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. > > > > > > > > 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> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > > This is a very intrusive fix - care to provide some more context on > > > why all these changes are needed? > > > > Since the bug reports never found an actual log over a few tens of > > kilobytes this is caused by the BIOS implementation allocating a huge > > buffer that is mostly unused. > > > > There are two other possibilities for fixing this, which were both part > > of the original suggestions. One would be to work out the size of the > > log and then allocate an exact size. This would require implementing > > tpm1 and tpm2 parsers for log size. However, since we can never go > > over KMALLOC_MAX_SIZE without an error even with this calculated size, > > the simplest straight line fix would be to cap the copy at > > KMALLOC_MAX_SIZE if it's over. That would be a simple one liner. > > All I'm saying is this. > > I've got bunch of complains of this from mainly SUSE, and now I'm > here with a response to that feedback. So I don't care. You decide. > > I'm 100% sure that the fix that Stefan proposed is not a sustainable > path in long-term, so I guess this was more like more long-term but > intrusive fix. > > Ya, and also please test the changes, especially anything that can > reach of OF eventlogs would be welcome feedback. Note that I precision cut that transcript because it includes all infos of the debatable hardware. BR, Jarkko
On Sat, 2024-12-21 at 22:11 +0200, Jarkko Sakkinen wrote: > On Sat Dec 21, 2024 at 7:16 PM EET, James Bottomley wrote: > > On Sat, 2024-12-21 at 17:04 +0100, Ard Biesheuvel wrote: > > > On Sat, 21 Dec 2024 at 12:33, 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. > > > > > > > > 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> > > > > Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> > > > > > > This is a very intrusive fix - care to provide some more context > > > on > > > why all these changes are needed? > > > > Since the bug reports never found an actual log over a few tens of > > kilobytes this is caused by the BIOS implementation allocating a > > huge > > buffer that is mostly unused. > > > > There are two other possibilities for fixing this, which were both > > part > > of the original suggestions. One would be to work out the size of > > the > > log and then allocate an exact size. This would require > > implementing > > tpm1 and tpm2 parsers for log size. However, since we can never go > > over KMALLOC_MAX_SIZE without an error even with this calculated > > size, > > the simplest straight line fix would be to cap the copy at > > KMALLOC_MAX_SIZE if it's over. That would be a simple one liner. > > All I'm saying is this. > > I've got bunch of complains of this from mainly SUSE, and now I'm > here with a response to that feedback. So I don't care. You decide. > > I'm 100% sure that the fix that Stefan proposed is not a sustainable > path in long-term, so I guess this was more like more long-term but > intrusive fix. If event logs grow to greater than KMALLOC_MAX_SIZE then absolutely it makes sense to map them instead of copying them. But we'd have to do that for all event log locators: ACPI, EFI and OF, because event log size should be independent of the mechanism used to locate it. So, even as a long term fix (assuming we think there's a possibility of logs expanding by 50x), this patch doesn't do the right thing because it only maps ACPI logs. If you're determined to do the mapping approach for all logs, I don't see why we shouldn't keep the log permanently mapped which would really simplify the patch set. The main reason why ACPI memory is mapped and unmapped is because it's usually I/O regions of devices for which we have to use the device mapping primitives. However, for machine main memory, which is where we know the log is, the mapping eventually boils down to a nop. Regards, James > Ya, and also please test the changes, especially anything that can > reach of OF eventlogs would be welcome feedback. > > BR, Jarkko >
On Sun Dec 22, 2024 at 5:00 PM EET, James Bottomley wrote: > If event logs grow to greater than KMALLOC_MAX_SIZE then absolutely it > makes sense to map them instead of copying them. But we'd have to do > that for all event log locators: ACPI, EFI and OF, because event log > size should be independent of the mechanism used to locate it. So, > even as a long term fix (assuming we think there's a possibility of > logs expanding by 50x), this patch doesn't do the right thing because > it only maps ACPI logs. Because we have a test target only on ACPI where this happens fix should still fix only ACPI. It's not hard to reiterate this but precursory iteration is a bad idea. BR, Jarkko
On Sun Dec 22, 2024 at 5:23 PM EET, Jarkko Sakkinen wrote: > On Sun Dec 22, 2024 at 5:00 PM EET, James Bottomley wrote: > > If event logs grow to greater than KMALLOC_MAX_SIZE then absolutely it > > makes sense to map them instead of copying them. But we'd have to do > > that for all event log locators: ACPI, EFI and OF, because event log > > size should be independent of the mechanism used to locate it. So, > > even as a long term fix (assuming we think there's a possibility of > > logs expanding by 50x), this patch doesn't do the right thing because > > it only maps ACPI logs. > > Because we have a test target only on ACPI where this happens fix > should still fix only ACPI. It's not hard to reiterate this but > precursory iteration is a bad idea. Also, "event log size should be independent of the mechanism used to locate it" is a sentence that is sky high too abstract to say much. I don't know what it means to be frank. BR, Jarkko
On Sun, 2024-12-22 at 17:33 +0200, Jarkko Sakkinen wrote: > On Sun Dec 22, 2024 at 5:23 PM EET, Jarkko Sakkinen wrote: > > On Sun Dec 22, 2024 at 5:00 PM EET, James Bottomley wrote: > > > If event logs grow to greater than KMALLOC_MAX_SIZE then > > > absolutely it makes sense to map them instead of copying them. > > > But we'd have to do that for all event log locators: ACPI, EFI > > > and OF, because event log size should be independent of the > > > mechanism used to locate it. So, even as a long term fix > > > (assuming we think there's a possibility of logs expanding by > > > 50x), this patch doesn't do the right thing because it only maps > > > ACPI logs. > > > > Because we have a test target only on ACPI where this happens fix > > should still fix only ACPI. It's not hard to reiterate this but > > precursory iteration is a bad idea. > > Also, "event log size should be independent of the mechanism used to > locate it" is a sentence that is sky high too abstract to say much. > > I don't know what it means to be frank. event log size means the number of bytes from the beginning to the end of the event log. Since the event log is created by the pre-boot environment, there is a convention for how to communicate this information from pre-boot to the kernel; this is the mechanism used to locate it. We decode three mechanisms: an ACPI table, an EFI table and an OF entry. The pre-boot environment generating the event log is supposed to conform to the TCG standards for what events it contains; none of the entries depends on the mechanism used to locate the log, which is why the size also can't depend on the mechanis. There are many optional events, but even if the pre-boot took a maximalist approach the most it could contain is a couple of hundred entries. The variable entries are mostly small but several types can contain device paths or certificates, but even if you allow a 10k size for each entry, that's still at most 2MB. So I think if a pre-boot declared log area goes over KMALLOC_MAX_SIZE (4MB on x86) it's safe to truncate the area because the log will never fill all of it. The corollary is that if we ever did find an actual log over 4MB, then the EFI and OF mechanisms used to locate it would also fail in the kmalloc, which is why KMALLOC_MAX_SIZE is the correct cap for the declared size. Regards, James
On Sun Dec 22, 2024 at 7:41 PM EET, James Bottomley wrote: > On Sun, 2024-12-22 at 17:33 +0200, Jarkko Sakkinen wrote: > > On Sun Dec 22, 2024 at 5:23 PM EET, Jarkko Sakkinen wrote: > > > On Sun Dec 22, 2024 at 5:00 PM EET, James Bottomley wrote: > > > > If event logs grow to greater than KMALLOC_MAX_SIZE then > > > > absolutely it makes sense to map them instead of copying them. > > > > But we'd have to do that for all event log locators: ACPI, EFI > > > > and OF, because event log size should be independent of the > > > > mechanism used to locate it. So, even as a long term fix > > > > (assuming we think there's a possibility of logs expanding by > > > > 50x), this patch doesn't do the right thing because it only maps > > > > ACPI logs. > > > > > > Because we have a test target only on ACPI where this happens fix > > > should still fix only ACPI. It's not hard to reiterate this but > > > precursory iteration is a bad idea. > > > > Also, "event log size should be independent of the mechanism used to > > locate it" is a sentence that is sky high too abstract to say much. > > > > I don't know what it means to be frank. > > event log size means the number of bytes from the beginning to the end > of the event log. Since the event log is created by the pre-boot > environment, there is a convention for how to communicate this > information from pre-boot to the kernel; this is the mechanism used to > locate it. We decode three mechanisms: an ACPI table, an EFI table and > an OF entry. > > The pre-boot environment generating the event log is supposed to > conform to the TCG standards for what events it contains; none of the > entries depends on the mechanism used to locate the log, which is why > the size also can't depend on the mechanis. There are many optional > events, but even if the pre-boot took a maximalist approach the most it > could contain is a couple of hundred entries. The variable entries are > mostly small but several types can contain device paths or > certificates, but even if you allow a 10k size for each entry, that's > still at most 2MB. So I think if a pre-boot declared log area goes > over KMALLOC_MAX_SIZE (4MB on x86) it's safe to truncate the area > because the log will never fill all of it. > > The corollary is that if we ever did find an actual log over 4MB, then > the EFI and OF mechanisms used to locate it would also fail in the > kmalloc, which is why KMALLOC_MAX_SIZE is the correct cap for the > declared size. Have you verified this with the failing system? > > Regards, > > James BR, Jarkko
diff --git a/drivers/char/tpm/eventlog/acpi.c b/drivers/char/tpm/eventlog/acpi.c index 69533d0bfb51..8d8db66ce876 100644 --- a/drivers/char/tpm/eventlog/acpi.c +++ b/drivers/char/tpm/eventlog/acpi.c @@ -22,8 +22,6 @@ #include <linux/slab.h> #include <linux/acpi.h> #include <linux/tpm_eventlog.h> - -#include "../tpm.h" #include "common.h" struct acpi_tcpa { @@ -70,14 +68,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 +130,27 @@ 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)) { + acpi_os_unmap_iomem(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..d934ef8c8b7f 100644 --- a/drivers/char/tpm/eventlog/common.c +++ b/drivers/char/tpm/eventlog/common.c @@ -25,11 +25,12 @@ static int tpm_bios_measurements_open(struct inode *inode, struct file *file) { - 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; + struct seq_file *seq; + int ret; inode_lock(inode); if (!inode->i_private) { @@ -42,27 +43,35 @@ static int tpm_bios_measurements_open(struct inode *inode, get_device(&chip->dev); inode_unlock(inode); - /* now register seq file */ - err = seq_open(file, seqops); - if (!err) { - seq = file->private_data; - seq->private = chip; - } else { + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + priv->chip = chip; + + ret = seq_open(file, seqops); + if (ret) { + kfree(priv); put_device(&chip->dev); + } else { + seq = file->private_data; + seq->private = priv; } - return err; + return ret; } 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..ad89a0daf585 100644 --- a/drivers/char/tpm/eventlog/common.h +++ b/drivers/char/tpm/eventlog/common.h @@ -7,6 +7,12 @@ 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; +}; + #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..6141a580e99c 100644 --- a/drivers/char/tpm/eventlog/tpm1.c +++ b/drivers/char/tpm/eventlog/tpm1.c @@ -22,11 +22,8 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/tpm_eventlog.h> - -#include "../tpm.h" #include "common.h" - static const char* tcpa_event_type_strings[] = { "PREBOOT", "POST CODE", @@ -70,20 +67,32 @@ 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_measurements *priv = m->private; + struct tpm_chip *chip = priv->chip; struct tpm_bios_log *log = &chip->log; - void *addr = log->bios_event_log; - void *limit = log->bios_event_log_end; struct tcpa_event *event; u32 converted_event_size; u32 converted_event_type; + size_t log_size; + void *addr; + + log_size = log->bios_event_log_end - log->bios_event_log; + + priv->start = !(chip->flags & TPM_CHIP_FLAG_ACPI_LOG) ? + log->bios_event_log : + acpi_os_map_iomem((unsigned long)log->bios_event_log, log_size); + if (!priv->start) + return NULL; + priv->end = priv->start + log_size; + + 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 +102,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) @@ -108,10 +117,8 @@ static void *tpm1_bios_measurements_start(struct seq_file *m, loff_t *pos) static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, loff_t *pos) { + struct tpm_measurements *priv = m->private; 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; u32 converted_event_size; u32 converted_event_type; @@ -121,7 +128,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 +137,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 +145,11 @@ static void *tpm1_bios_measurements_next(struct seq_file *m, void *v, static void tpm1_bios_measurements_stop(struct seq_file *m, void *v) { + 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); } 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..79e090dd751a 100644 --- a/drivers/char/tpm/eventlog/tpm2.c +++ b/drivers/char/tpm/eventlog/tpm2.c @@ -12,14 +12,13 @@ * content. */ +#include "linux/tpm.h" #include <linux/seq_file.h> #include <linux/fs.h> #include <linux/security.h> #include <linux/module.h> #include <linux/slab.h> #include <linux/tpm_eventlog.h> - -#include "../tpm.h" #include "common.h" /* @@ -41,20 +40,31 @@ 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_measurements *priv = m->private; + struct tpm_chip *chip = priv->chip; struct tpm_bios_log *log = &chip->log; - void *addr = log->bios_event_log; - void *limit = log->bios_event_log_end; struct tcg_pcr_event *event_header; struct tcg_pcr_event2_head *event; - size_t size; + size_t size, log_size; + void *addr; int i; + log_size = log->bios_event_log_end - log->bios_event_log; + + priv->start = !(chip->flags & TPM_CHIP_FLAG_ACPI_LOG) ? + log->bios_event_log : + acpi_os_map_iomem((unsigned long)log->bios_event_log, log_size); + if (!priv->start) + return NULL; + + priv->end = priv->start + log_size; + + 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 +76,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 +84,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; } @@ -85,16 +95,14 @@ static void *tpm2_bios_measurements_start(struct seq_file *m, loff_t *pos) static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, loff_t *pos) { + struct tpm_measurements *priv = m->private; 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; 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 +117,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 +131,17 @@ static void *tpm2_bios_measurements_next(struct seq_file *m, void *v, static void tpm2_bios_measurements_stop(struct seq_file *m, void *v) { + 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); } 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)
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. 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> Signed-off-by: Jarkko Sakkinen <jarkko@kernel.org> --- drivers/char/tpm/eventlog/acpi.c | 30 +++++-------------- drivers/char/tpm/eventlog/common.c | 33 ++++++++++++-------- drivers/char/tpm/eventlog/common.h | 6 ++++ drivers/char/tpm/eventlog/tpm1.c | 38 +++++++++++++++-------- drivers/char/tpm/eventlog/tpm2.c | 48 +++++++++++++++++++----------- include/linux/tpm.h | 1 + 6 files changed, 91 insertions(+), 65 deletions(-)