Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 12:51 PM Matthew Garrett wrote:
> Yes, it looks like this is just broken. Can you try with the attached patch?
Actually, please try this one.
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index 2ccaa6661aaf..db0fdaa9c666 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index dccc97e6135c..662410710ac0 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -158,7 +158,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -176,9 +175,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -199,9 +198,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -219,9 +218,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
if (do_mapping) {
early_memunmap(mapping, mapping_size);
mapping_size = marker - marker_start;
- mapping = early_memremap((unsigned long)marker_start,
+ event = early_memremap((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -243,11 +242,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- early_memunmap(marker_start, mapping_size);
+ early_memunmap(mapping, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = early_memremap((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = early_memremap((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -257,8 +256,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
+ event_field->event_size;
size = marker - marker_start;
- if ((event->event_type == 0) && (event_field->event_size == 0))
- size = 0;
out:
if (do_mapping)
early_memunmap(mapping, mapping_size);
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
On Tue, Apr 30, 2019 at 6:07 AM Bartosz Szczepanek wrote:
>
> I may be a little late with this comment, but I've just tested these
> patches on aarch64 platform (from the top of jjs/master) and got
> kernel panic ("Unable to handle kernel read", full log at the end of
> mail). I think there's problem with below call to
> tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
> passed as (void *) and never remapped:
Yes, it looks like this is just broken. Can you try with the attached patch?
diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
index fe48150f06d1..9711bd34f8ae 100644
--- a/drivers/firmware/efi/tpm.c
+++ b/drivers/firmware/efi/tpm.c
@@ -28,6 +28,7 @@ static int tpm2_calc_event_log_size(void *data, int count, void *size_info)
if (event_size == 0)
return -1;
size += event_size;
+ count--;
}
return size;
@@ -41,6 +42,7 @@ int __init efi_tpm_eventlog_init(void)
struct linux_efi_tpm_eventlog *log_tbl;
struct efi_tcg2_final_events_table *final_tbl;
unsigned int tbl_size;
+ int ret = 0;
if (efi.tpm_log == EFI_INVALID_TABLE_ADDR) {
/*
@@ -60,10 +62,9 @@ int __init efi_tpm_eventlog_init(void)
tbl_size = sizeof(*log_tbl) + log_tbl->size;
memblock_reserve(efi.tpm_log, tbl_size);
- early_memunmap(log_tbl, sizeof(*log_tbl));
if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR)
- return 0;
+ goto out;
final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
@@ -71,17 +72,20 @@ int __init efi_tpm_eventlog_init(void)
pr_err("Failed to map TPM Final Event Log table @ 0x%lx\n",
efi.tpm_final_log);
efi.tpm_final_log = EFI_INVALID_TABLE_ADDR;
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
tbl_size = tpm2_calc_event_log_size(final_tbl->events,
final_tbl->nr_events,
- (void *)efi.tpm_log);
+ log_tbl->log);
memblock_reserve((unsigned long)final_tbl,
tbl_size + sizeof(*final_tbl));
early_memunmap(final_tbl, sizeof(*final_tbl));
efi_tpm_final_log_size = tbl_size;
- return 0;
+out:
+ early_memunmap(log_tbl, sizeof(*log_tbl));
+ return ret;
}
diff --git a/include/linux/tpm_eventlog.h b/include/linux/tpm_eventlog.h
index 0ca27bc053af..9cfbb14f54e6 100644
--- a/include/linux/tpm_eventlog.h
+++ b/include/linux/tpm_eventlog.h
@@ -161,7 +161,6 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
{
struct tcg_efi_specid_event_head *efispecid;
struct tcg_event_field *event_field;
- void *mapping = NULL;
int mapping_size;
void *marker;
void *marker_start;
@@ -179,9 +178,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the event header */
if (do_mapping) {
mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -200,11 +199,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest's algorithm identifier */
if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size = marker - marker_start + halg_size;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -220,11 +219,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
/* Map the digest content itself */
if (do_mapping) {
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size = marker - marker_start;
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
- mapping_size);
- if (!mapping) {
+ event = TPM_MEMREMAP((unsigned long)marker_start,
+ mapping_size);
+ if (!event) {
size = 0;
goto out;
}
@@ -246,11 +245,11 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
* we don't need to map it
*/
if (do_mapping) {
- TPM_MEMUNMAP(marker_start, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
mapping_size += sizeof(event_field->event_size);
- mapping = TPM_MEMREMAP((unsigned long)marker_start,
+ event = TPM_MEMREMAP((unsigned long)marker_start,
mapping_size);
- if (!mapping) {
+ if (!event) {
size = 0;
goto out;
}
@@ -260,11 +259,9 @@ static inline int __calc_tpm2_event_size(struct tcg_pcr_event2_head *event,
+ event_field->event_size;
size = marker - marker_start;
- if ((event->event_type == 0) && (event_field->event_size == 0))
- size = 0;
out:
if (do_mapping)
- TPM_MEMUNMAP(mapping, mapping_size);
+ TPM_MEMUNMAP(event, mapping_size);
return size;
}
Re: [PATCH V5 2/4] tpm: Reserve the TPM final events table
I may be a little late with this comment, but I've just tested these
patches on aarch64 platform (from the top of jjs/master) and got
kernel panic ("Unable to handle kernel read", full log at the end of
mail). I think there's problem with below call to
tpm2_calc_event_log_size(), where physical address of efi.tpm_log is
passed as (void *) and never remapped:
> + tbl_size = tpm2_calc_event_log_size(final_tbl->events,
> + final_tbl->nr_events,
> + (void *)efi.tpm_log);
This is later used to get efispecid:
> efispecid = (struct tcg_efi_specid_event_head *)event_header->event;
It seems event_header is not mapped during dereference. This is
somewhat expected, because it comes from different, already unmapped
memory region (region of initial TPM log) than "event" itself (which
comes from TPM final log).
Also, value passed as size_info shouldn't be pointing to
linux_efi_tpm_eventlog with its size and version fields, but to the
first event (header event) within. I tried with log_tbl->log and it
worked fine (I omitted unmapping part). On the other hand, with bare
log_tbl it still fails. Not sure how does it even work on other
platforms.
One more thing that's not clear for me – shouldn't the value returned
from early_memremap be used for further accesses? Throughout
__calc_tpm2_event_size() "mapping" is only checked for being zero.
When it is, you're still unmapping it – is it correct?
> + while (count > 0) {
> + header = data + size;
> + event_size = __calc_tpm2_event_size(header, size_info, true);
> + if (event_size == 0)
> + return -1;
> + size += event_size;
> + }
Loop condition here is always true, by the way.
One information about my setup – I'm working with below local diff to
enable operation on ARM:
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -194,6 +194,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t
> *sys_table,
>
> /* Ask the firmware to clear memory on unclean shutdown */
>efi_enable_reset_attack_mitigation(sys_table);
> + efi_retrieve_tpm2_eventlog(sys_table);
Full log of kernel panic follows.
EFI stub: Booting Linux Kernel...
EFI stub: EFI_RNG_PROTOCOL unavailable, no randomness supplied
EFI stub: Using DTB from configuration table
EFI stub: Exiting boot services and installing virtual address map...
[0.00] Booting Linux on physical CPU 0x00 [0x420f5162]
[0.00] Linux version 5.1.0-rc2+ (root@localhost.localdomain)
(gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC)) #69 SMP Fri Apr
26 03:20:57 EDT 2019
[0.00] earlycon: pl11 at MMIO 0x00040202 (options '115200n8')
[0.00] printk: bootconsole [pl11] enabled
[0.00] efi: Getting EFI parameters from FDT:
[0.00] efi: EFI v2.60 by Cavium Inc.
TX2-FW-Release-7.2-build_08-0-g14f8c5bf8a Apr 15 2019 18:51:41
[0.00] efi: TPMFinalLog=0xed5f SMBIOS=0xfad9 SMBIOS
3.0=0xed53 ACPI 2.0=0xeda9 ESRT=0xfafdb218
MEMATTR=0xf8489018 TPMEventLog=0xedaa9018 MEMRESERVE=0xedaa8018
[0.00] Unable to handle kernel read from unreadable memory at
virtual address edaa9050
[0.00] Mem abort info:
[0.00] ESR = 0x9604
[0.00] Exception class = DABT (current EL), IL = 32 bits
[0.00] SET = 0, FnV = 0
[0.00] EA = 0, S1PTW = 0
[0.00] Data abort info:
[0.00] ISV = 0, ISS = 0x0004
[0.00] CM = 0, WnR = 0
[0.00] [edaa9050] user address but active_mm is swapper
[0.00] Internal error: Oops: 9604 [#1] SMP
[0.00] Modules linked in:
[0.00] CPU: 0 PID: 0 Comm: swapper Not tainted 5.1.0-rc2+ #69
[0.00] pstate: 60400089 (nZCv daIf +PAN -UAO)
[0.00] pc : efi_tpm_eventlog_init+0xfc/0x26c
[0.00] lr : efi_tpm_eventlog_init+0xf4/0x26c
[0.00] sp : 11533ce0
[0.00] x29: 11533ce0 x28: edaa8018
[0.00] x27: 7dfffe6fa010 x26: 0023
[0.00] x25: 7dfffe6fa000 x24: edaa9038
[0.00] x23: x22: 7dfffe6fa010
[0.00] x21: 1153d000 x20: 7dfffe6fa018
[0.00] x19: 11542500 x18:
[0.00] x17: 0435 x16:
[0.00] x15: 1153d708 x14: 6576454d50542020
[0.00] x13: 3831303938343866 x12: 78303d525454414d
[0.00] x11: 454d202038313262 x10: 6466616678303d54
[0.00] x9 : 1153ef58 x8 : 0200
[0.00] x7 : 0a30 x6 : 110d2a18
[0.00] x5 : 013a x4 : 04c5
[0.00] x3 : 11714000 x2 : 0002
[0.00] x1 : 7dfffe6fa000 x0 : 7dfffe73a010
[0.00] Process
[PATCH 4.19 083/100] x86/fpu: Dont export __kernel_fpu_{begin,end}()
From: Sebastian Andrzej Siewior
commit 12209993e98c5fa1855c467f22a24e3d5b8be205 upstream.
There is one user of __kernel_fpu_begin() and before invoking it,
it invokes preempt_disable(). So it could invoke kernel_fpu_begin()
right away. The 32bit version of arch_efi_call_virt_setup() and
arch_efi_call_virt_teardown() does this already.
The comment above *kernel_fpu*() claims that before invoking
__kernel_fpu_begin() preemption should be disabled and that KVM is a
good example of doing it. Well, KVM doesn't do that since commit
f775b13eedee2 ("x86,kvm: move qemu/guest FPU switching out to vcpu_run")
so it is not an example anymore.
With EFI gone as the last user of __kernel_fpu_{begin|end}(), both can
be made static and not exported anymore.
Signed-off-by: Sebastian Andrzej Siewior
Signed-off-by: Borislav Petkov
Reviewed-by: Rik van Riel
Cc: "H. Peter Anvin"
Cc: "Jason A. Donenfeld"
Cc: Andy Lutomirski
Cc: Ard Biesheuvel
Cc: Dave Hansen
Cc: Ingo Molnar
Cc: Nicolai Stange
Cc: Paolo Bonzini
Cc: Radim Krčmář
Cc: Thomas Gleixner
Cc: kvm ML
Cc: linux-efi
Cc: x86-ml
Link: https://lkml.kernel.org/r/20181129150210.2k4mawt37ow6c...@linutronix.de
Signed-off-by: Greg Kroah-Hartman
---
arch/x86/include/asm/efi.h |6 ++
arch/x86/include/asm/fpu/api.h | 15 +--
arch/x86/kernel/fpu/core.c |6 ++
3 files changed, 9 insertions(+), 18 deletions(-)
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -82,8 +82,7 @@ struct efi_scratch {
#define arch_efi_call_virt_setup() \
({ \
efi_sync_low_kernel_mappings(); \
- preempt_disable(); \
- __kernel_fpu_begin(); \
+ kernel_fpu_begin(); \
firmware_restrict_branch_speculation_start(); \
\
if (!efi_enabled(EFI_OLD_MEMMAP)) \
@@ -99,8 +98,7 @@ struct efi_scratch {
efi_switch_mm(efi_scratch.prev_mm); \
\
firmware_restrict_branch_speculation_end(); \
- __kernel_fpu_end(); \
- preempt_enable(); \
+ kernel_fpu_end(); \
})
extern void __iomem *__init efi_ioremap(unsigned long addr, unsigned long size,
--- a/arch/x86/include/asm/fpu/api.h
+++ b/arch/x86/include/asm/fpu/api.h
@@ -12,17 +12,12 @@
#define _ASM_X86_FPU_API_H
/*
- * Careful: __kernel_fpu_begin/end() must be called with preempt disabled
- * and they don't touch the preempt state on their own.
- * If you enable preemption after __kernel_fpu_begin(), preempt notifier
- * should call the __kernel_fpu_end() to prevent the kernel/user FPU
- * state from getting corrupted. KVM for example uses this model.
- *
- * All other cases use kernel_fpu_begin/end() which disable preemption
- * during kernel FPU usage.
+ * Use kernel_fpu_begin/end() if you intend to use FPU in kernel context. It
+ * disables preemption so be careful if you intend to use it for long periods
+ * of time.
+ * If you intend to use the FPU in softirq you need to check first with
+ * irq_fpu_usable() if it is possible.
*/
-extern void __kernel_fpu_begin(void);
-extern void __kernel_fpu_end(void);
extern void kernel_fpu_begin(void);
extern void kernel_fpu_end(void);
extern bool irq_fpu_usable(void);
--- a/arch/x86/kernel/fpu/core.c
+++ b/arch/x86/kernel/fpu/core.c
@@ -93,7 +93,7 @@ bool irq_fpu_usable(void)
}
EXPORT_SYMBOL(irq_fpu_usable);
-void __kernel_fpu_begin(void)
+static void __kernel_fpu_begin(void)
{
struct fpu *fpu = >thread.fpu;
@@ -111,9 +111,8 @@ void __kernel_fpu_begin(void)
__cpu_invalidate_fpregs_state();
}
}
-EXPORT_SYMBOL(__kernel_fpu_begin);
-void __kernel_fpu_end(void)
+static void __kernel_fpu_end(void)
{
struct fpu *fpu = >thread.fpu;
@@ -122,7 +121,6 @@ void __kernel_fpu_end(void)
kernel_fpu_enable();
}
-EXPORT_SYMBOL(__kernel_fpu_end);
void kernel_fpu_begin(void)
{
