All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-06-15 12:37 ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-06-15 12:37 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: jarkko, stefanb, eajames, jgg, yangyingliang, linux-integrity,
	peterhuewe

There's code in prom_instantiate_sml() to do a "SML handover" (Stored
Measurement Log) from OF to Linux, before Linux shuts down Open
Firmware.

This involves creating a buffer to hold the SML, and creating two device
tree properties to record its base address and size. The kernel then
later reads those properties from the device tree to find the SML.

When the code was initially added in commit 4a727429abec ("PPC64: Add
support for instantiating SML from Open Firmware") the powerpc kernel
was always built big endian, so the properties were created big endian
by default.

However since then little endian support was added to powerpc, and now
the code lacks conversions to big endian when creating the properties.

This means on little endian kernels the device tree properties are
little endian, which is contrary to the device tree spec, and in
contrast to all other device tree properties.

To cope with that a workaround was added in tpm_read_log_of() to skip
the endian conversion if the properties were created via the SML
handover.

A better solution is to encode the properties as big endian as they
should be, and remove the workaround.

Typically changing the encoding of a property like this would present
problems for kexec. However the SML is not propagated across kexec, so
changing the encoding of the properties is a non-issue.

Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 arch/powerpc/kernel/prom_init.c |  8 ++++++--
 drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
 2 files changed, 10 insertions(+), 21 deletions(-)

v2: Add Stefan's reviewed-by.

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index d464ba412084..72fe306b6820 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
 	u32 entry = 0, size = 0, succ = 0;
 	u64 base;
 	__be32 val;
+	__be64 val64;
 
 	prom_debug("prom_instantiate_sml: start...\n");
 
@@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
 
 	reserve_mem(base, size);
 
+	val64 = cpu_to_be64(base);
 	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
+		     &val64, sizeof(val64));
+
+	val = cpu_to_be32(size);
 	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
+		     &val, sizeof(val));
 
 	prom_debug("sml base     = 0x%llx\n", base);
 	prom_debug("sml size     = 0x%x\n", size);
diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index 930fe43d5daf..0bc0cb6333c6 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
 int tpm_read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
-	const u32 *sizep;
-	const u64 *basep;
+	const __be32 *sizep;
+	const __be64 *basep;
 	struct tpm_bios_log *log;
 	u32 size;
 	u64 base;
@@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (sizep == NULL || basep == NULL)
 		return -EIO;
 
-	/*
-	 * For both vtpm/tpm, firmware has log addr and log size in big
-	 * endian format. But in case of vtpm, there is a method called
-	 * sml-handover which is run during kernel init even before
-	 * device tree is setup. This sml-handover function takes care
-	 * of endianness and writes to sml-base and sml-size in little
-	 * endian format. For this reason, vtpm doesn't need conversion
-	 * but physical tpm needs the conversion.
-	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
-	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
-		size = be32_to_cpup((__force __be32 *)sizep);
-		base = be64_to_cpup((__force __be64 *)basep);
-	} else {
-		size = *sizep;
-		base = *basep;
-	}
+	size = be32_to_cpup(sizep);
+	base = be64_to_cpup(basep);
 
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-06-15 12:37 ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-06-15 12:37 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: eajames, jgg, jarkko, yangyingliang, linux-integrity, peterhuewe,
	stefanb

There's code in prom_instantiate_sml() to do a "SML handover" (Stored
Measurement Log) from OF to Linux, before Linux shuts down Open
Firmware.

This involves creating a buffer to hold the SML, and creating two device
tree properties to record its base address and size. The kernel then
later reads those properties from the device tree to find the SML.

When the code was initially added in commit 4a727429abec ("PPC64: Add
support for instantiating SML from Open Firmware") the powerpc kernel
was always built big endian, so the properties were created big endian
by default.

However since then little endian support was added to powerpc, and now
the code lacks conversions to big endian when creating the properties.

This means on little endian kernels the device tree properties are
little endian, which is contrary to the device tree spec, and in
contrast to all other device tree properties.

To cope with that a workaround was added in tpm_read_log_of() to skip
the endian conversion if the properties were created via the SML
handover.

A better solution is to encode the properties as big endian as they
should be, and remove the workaround.

Typically changing the encoding of a property like this would present
problems for kexec. However the SML is not propagated across kexec, so
changing the encoding of the properties is a non-issue.

Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
---
 arch/powerpc/kernel/prom_init.c |  8 ++++++--
 drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
 2 files changed, 10 insertions(+), 21 deletions(-)

v2: Add Stefan's reviewed-by.

diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index d464ba412084..72fe306b6820 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
 	u32 entry = 0, size = 0, succ = 0;
 	u64 base;
 	__be32 val;
+	__be64 val64;
 
 	prom_debug("prom_instantiate_sml: start...\n");
 
@@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
 
 	reserve_mem(base, size);
 
+	val64 = cpu_to_be64(base);
 	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
-		     &base, sizeof(base));
+		     &val64, sizeof(val64));
+
+	val = cpu_to_be32(size);
 	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
-		     &size, sizeof(size));
+		     &val, sizeof(val));
 
 	prom_debug("sml base     = 0x%llx\n", base);
 	prom_debug("sml size     = 0x%x\n", size);
diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index 930fe43d5daf..0bc0cb6333c6 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
 int tpm_read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
-	const u32 *sizep;
-	const u64 *basep;
+	const __be32 *sizep;
+	const __be64 *basep;
 	struct tpm_bios_log *log;
 	u32 size;
 	u64 base;
@@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (sizep == NULL || basep == NULL)
 		return -EIO;
 
-	/*
-	 * For both vtpm/tpm, firmware has log addr and log size in big
-	 * endian format. But in case of vtpm, there is a method called
-	 * sml-handover which is run during kernel init even before
-	 * device tree is setup. This sml-handover function takes care
-	 * of endianness and writes to sml-base and sml-size in little
-	 * endian format. For this reason, vtpm doesn't need conversion
-	 * but physical tpm needs the conversion.
-	 */
-	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
-	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
-		size = be32_to_cpup((__force __be32 *)sizep);
-		base = be64_to_cpup((__force __be64 *)basep);
-	} else {
-		size = *sizep;
-		base = *basep;
-	}
+	size = be32_to_cpup(sizep);
+	base = be64_to_cpup(basep);
 
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 2/2] powerpc/tpm: Reserve SML log when kexec'ing with kexec_file_load()
  2023-06-15 12:37 ` Michael Ellerman
@ 2023-06-15 12:37   ` Michael Ellerman
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-06-15 12:37 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: jarkko, stefanb, eajames, jgg, yangyingliang, linux-integrity,
	peterhuewe

The TPM code in prom_init.c creates a small buffer of memory to store
the TPM's SML (Stored Measurement Log). It's communicated to Linux via
the linux,sml-base/size device tree properties of the TPM node.

When kexec'ing that buffer can be overwritten, or when kdump'ing it may
not be mapped by the second kernel. The latter can lead to a crash when
booting the second kernel such as:

  tpm_ibmvtpm 71000003: CRQ initialization completed
  BUG: Unable to handle kernel data access on read at 0xc00000002ffb0000
  Faulting instruction address: 0xc0000000200a70e0
  Oops: Kernel access of bad area, sig: 11 [#1]
  LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
  Modules linked in:
  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-00134-g9307ce092f5d #314
  Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-5b4c5a pSeries
  NIP:  c0000000200a70e0 LR: c0000000203dd5dc CTR: 0000000000000800
  REGS: c000000024543280 TRAP: 0300   Not tainted  (6.2.0-rc2-00134-g9307ce092f5d)
  MSR:  8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 24002280  XER: 00000006
  CFAR: c0000000200a70c8 DAR: c00000002ffb0000 DSISR: 40000000 IRQMASK: 0
  ...
  NIP memcpy_power7+0x400/0x7d0
  LR  kmemdup+0x5c/0x80
  Call Trace:
    memcpy_power7+0x274/0x7d0 (unreliable)
    kmemdup+0x5c/0x80
    tpm_read_log_of+0xe8/0x1b0
    tpm_bios_log_setup+0x60/0x210
    tpm_chip_register+0x134/0x320
    tpm_ibmvtpm_probe+0x520/0x7d0
    vio_bus_probe+0x9c/0x460
    really_probe+0x104/0x420
    __driver_probe_device+0xb0/0x170
    driver_probe_device+0x58/0x180
    __driver_attach+0xd8/0x250
    bus_for_each_dev+0xb4/0x140
    driver_attach+0x34/0x50
    bus_add_driver+0x1e8/0x2d0
    driver_register+0xb4/0x1c0
    __vio_register_driver+0x74/0x9c
    ibmvtpm_module_init+0x34/0x48
    do_one_initcall+0x80/0x320
    kernel_init_freeable+0x304/0x3ac
    kernel_init+0x30/0x1a0
    ret_from_kernel_thread+0x5c/0x64

To fix the crash, add the SML region to the usable memory areas for the
kdump kernel, so that the second kernel will map the region. To avoid
corruption of the region, add the region to the reserved memory areas,
so that the second kernel does not use the memory for something else.

Note that when loading a kdump kernel with the regular kexec_load()
syscall the SML may be overwritten by the kdump kernel, depending on
where the SML is in memory in relation to the crashkernel region. That
is a separate problem that is not solved by this patch.

Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
Reported-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/kexec_ranges.h |  1 +
 arch/powerpc/kexec/file_load_64.c       | 12 ++++++++++++
 arch/powerpc/kexec/ranges.c             | 20 ++++++++++++++++++++
 3 files changed, 33 insertions(+)

v2: Add fixes tag as suggested by Jarkko.
    Make change log clearer that this only fixes kexec_file_load().

diff --git a/arch/powerpc/include/asm/kexec_ranges.h b/arch/powerpc/include/asm/kexec_ranges.h
index f83866a19e87..cf6a97f9113d 100644
--- a/arch/powerpc/include/asm/kexec_ranges.h
+++ b/arch/powerpc/include/asm/kexec_ranges.h
@@ -21,5 +21,6 @@ int add_kernel_mem_range(struct crash_mem **mem_ranges);
 int add_rtas_mem_range(struct crash_mem **mem_ranges);
 int add_opal_mem_range(struct crash_mem **mem_ranges);
 int add_reserved_mem_ranges(struct crash_mem **mem_ranges);
+int add_sml_mem_range(struct crash_mem **mem_ranges);
 
 #endif /* _ASM_POWERPC_KEXEC_RANGES_H */
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 110d28bede2a..90c10a89fcbc 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -79,6 +79,10 @@ static int get_exclude_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_opal_mem_range(mem_ranges);
 	if (ret)
 		goto out;
@@ -122,6 +126,10 @@ static int get_usable_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_opal_mem_range(mem_ranges);
 	if (ret)
 		goto out;
@@ -225,6 +233,10 @@ static int get_reserved_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_tce_mem_ranges(mem_ranges);
 	if (ret)
 		goto out;
diff --git a/arch/powerpc/kexec/ranges.c b/arch/powerpc/kexec/ranges.c
index 5fc53a5fcfdf..8b01655ceb5e 100644
--- a/arch/powerpc/kexec/ranges.c
+++ b/arch/powerpc/kexec/ranges.c
@@ -350,6 +350,26 @@ int add_rtas_mem_range(struct crash_mem **mem_ranges)
 	return ret;
 }
 
+int add_sml_mem_range(struct crash_mem **mem_ranges)
+{
+	struct device_node *dn;
+	int ret = 0;
+	u64 base;
+	u32 size;
+
+	// Matches the device type in tpm_ibmvtpm.c
+	for_each_node_by_type(dn, "IBM,vtpm") {
+		if (of_property_read_u64(dn, "linux,sml-base", &base) == 0 &&
+		    of_property_read_u32(dn, "linux,sml-size", &size) == 0) {
+			ret = add_mem_range(mem_ranges, base, size);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
 /**
  * add_opal_mem_range - Adds OPAL region to the given memory ranges list.
  * @mem_ranges:         Range list to add the memory range to.
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v2 2/2] powerpc/tpm: Reserve SML log when kexec'ing with kexec_file_load()
@ 2023-06-15 12:37   ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-06-15 12:37 UTC (permalink / raw)
  To: linuxppc-dev
  Cc: eajames, jgg, jarkko, yangyingliang, linux-integrity, peterhuewe,
	stefanb

The TPM code in prom_init.c creates a small buffer of memory to store
the TPM's SML (Stored Measurement Log). It's communicated to Linux via
the linux,sml-base/size device tree properties of the TPM node.

When kexec'ing that buffer can be overwritten, or when kdump'ing it may
not be mapped by the second kernel. The latter can lead to a crash when
booting the second kernel such as:

  tpm_ibmvtpm 71000003: CRQ initialization completed
  BUG: Unable to handle kernel data access on read at 0xc00000002ffb0000
  Faulting instruction address: 0xc0000000200a70e0
  Oops: Kernel access of bad area, sig: 11 [#1]
  LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
  Modules linked in:
  CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-00134-g9307ce092f5d #314
  Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-5b4c5a pSeries
  NIP:  c0000000200a70e0 LR: c0000000203dd5dc CTR: 0000000000000800
  REGS: c000000024543280 TRAP: 0300   Not tainted  (6.2.0-rc2-00134-g9307ce092f5d)
  MSR:  8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 24002280  XER: 00000006
  CFAR: c0000000200a70c8 DAR: c00000002ffb0000 DSISR: 40000000 IRQMASK: 0
  ...
  NIP memcpy_power7+0x400/0x7d0
  LR  kmemdup+0x5c/0x80
  Call Trace:
    memcpy_power7+0x274/0x7d0 (unreliable)
    kmemdup+0x5c/0x80
    tpm_read_log_of+0xe8/0x1b0
    tpm_bios_log_setup+0x60/0x210
    tpm_chip_register+0x134/0x320
    tpm_ibmvtpm_probe+0x520/0x7d0
    vio_bus_probe+0x9c/0x460
    really_probe+0x104/0x420
    __driver_probe_device+0xb0/0x170
    driver_probe_device+0x58/0x180
    __driver_attach+0xd8/0x250
    bus_for_each_dev+0xb4/0x140
    driver_attach+0x34/0x50
    bus_add_driver+0x1e8/0x2d0
    driver_register+0xb4/0x1c0
    __vio_register_driver+0x74/0x9c
    ibmvtpm_module_init+0x34/0x48
    do_one_initcall+0x80/0x320
    kernel_init_freeable+0x304/0x3ac
    kernel_init+0x30/0x1a0
    ret_from_kernel_thread+0x5c/0x64

To fix the crash, add the SML region to the usable memory areas for the
kdump kernel, so that the second kernel will map the region. To avoid
corruption of the region, add the region to the reserved memory areas,
so that the second kernel does not use the memory for something else.

Note that when loading a kdump kernel with the regular kexec_load()
syscall the SML may be overwritten by the kdump kernel, depending on
where the SML is in memory in relation to the crashkernel region. That
is a separate problem that is not solved by this patch.

Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
Reported-by: Stefan Berger <stefanb@linux.ibm.com>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
---
 arch/powerpc/include/asm/kexec_ranges.h |  1 +
 arch/powerpc/kexec/file_load_64.c       | 12 ++++++++++++
 arch/powerpc/kexec/ranges.c             | 20 ++++++++++++++++++++
 3 files changed, 33 insertions(+)

v2: Add fixes tag as suggested by Jarkko.
    Make change log clearer that this only fixes kexec_file_load().

diff --git a/arch/powerpc/include/asm/kexec_ranges.h b/arch/powerpc/include/asm/kexec_ranges.h
index f83866a19e87..cf6a97f9113d 100644
--- a/arch/powerpc/include/asm/kexec_ranges.h
+++ b/arch/powerpc/include/asm/kexec_ranges.h
@@ -21,5 +21,6 @@ int add_kernel_mem_range(struct crash_mem **mem_ranges);
 int add_rtas_mem_range(struct crash_mem **mem_ranges);
 int add_opal_mem_range(struct crash_mem **mem_ranges);
 int add_reserved_mem_ranges(struct crash_mem **mem_ranges);
+int add_sml_mem_range(struct crash_mem **mem_ranges);
 
 #endif /* _ASM_POWERPC_KEXEC_RANGES_H */
diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c
index 110d28bede2a..90c10a89fcbc 100644
--- a/arch/powerpc/kexec/file_load_64.c
+++ b/arch/powerpc/kexec/file_load_64.c
@@ -79,6 +79,10 @@ static int get_exclude_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_opal_mem_range(mem_ranges);
 	if (ret)
 		goto out;
@@ -122,6 +126,10 @@ static int get_usable_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_opal_mem_range(mem_ranges);
 	if (ret)
 		goto out;
@@ -225,6 +233,10 @@ static int get_reserved_memory_ranges(struct crash_mem **mem_ranges)
 	if (ret)
 		goto out;
 
+	ret = add_sml_mem_range(mem_ranges);
+	if (ret)
+		goto out;
+
 	ret = add_tce_mem_ranges(mem_ranges);
 	if (ret)
 		goto out;
diff --git a/arch/powerpc/kexec/ranges.c b/arch/powerpc/kexec/ranges.c
index 5fc53a5fcfdf..8b01655ceb5e 100644
--- a/arch/powerpc/kexec/ranges.c
+++ b/arch/powerpc/kexec/ranges.c
@@ -350,6 +350,26 @@ int add_rtas_mem_range(struct crash_mem **mem_ranges)
 	return ret;
 }
 
+int add_sml_mem_range(struct crash_mem **mem_ranges)
+{
+	struct device_node *dn;
+	int ret = 0;
+	u64 base;
+	u32 size;
+
+	// Matches the device type in tpm_ibmvtpm.c
+	for_each_node_by_type(dn, "IBM,vtpm") {
+		if (of_property_read_u64(dn, "linux,sml-base", &base) == 0 &&
+		    of_property_read_u32(dn, "linux,sml-size", &size) == 0) {
+			ret = add_mem_range(mem_ranges, base, size);
+			if (ret)
+				break;
+		}
+	}
+
+	return ret;
+}
+
 /**
  * add_opal_mem_range - Adds OPAL region to the given memory ranges list.
  * @mem_ranges:         Range list to add the memory range to.
-- 
2.40.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/2] powerpc/tpm: Reserve SML log when kexec'ing with kexec_file_load()
  2023-06-15 12:37   ` Michael Ellerman
@ 2023-06-19  0:26     ` Stefan Berger
  -1 siblings, 0 replies; 18+ messages in thread
From: Stefan Berger @ 2023-06-19  0:26 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: jarkko, eajames, jgg, yangyingliang, linux-integrity, peterhuewe



On 6/15/23 08:37, Michael Ellerman wrote:
> The TPM code in prom_init.c creates a small buffer of memory to store
> the TPM's SML (Stored Measurement Log). It's communicated to Linux via
> the linux,sml-base/size device tree properties of the TPM node.
> 
> When kexec'ing that buffer can be overwritten, or when kdump'ing it may
> not be mapped by the second kernel. The latter can lead to a crash when
> booting the second kernel such as:
> 
>    tpm_ibmvtpm 71000003: CRQ initialization completed
>    BUG: Unable to handle kernel data access on read at 0xc00000002ffb0000
>    Faulting instruction address: 0xc0000000200a70e0
>    Oops: Kernel access of bad area, sig: 11 [#1]
>    LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>    Modules linked in:
>    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-00134-g9307ce092f5d #314
>    Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-5b4c5a pSeries
>    NIP:  c0000000200a70e0 LR: c0000000203dd5dc CTR: 0000000000000800
>    REGS: c000000024543280 TRAP: 0300   Not tainted  (6.2.0-rc2-00134-g9307ce092f5d)
>    MSR:  8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 24002280  XER: 00000006
>    CFAR: c0000000200a70c8 DAR: c00000002ffb0000 DSISR: 40000000 IRQMASK: 0
>    ...
>    NIP memcpy_power7+0x400/0x7d0
>    LR  kmemdup+0x5c/0x80
>    Call Trace:
>      memcpy_power7+0x274/0x7d0 (unreliable)
>      kmemdup+0x5c/0x80
>      tpm_read_log_of+0xe8/0x1b0
>      tpm_bios_log_setup+0x60/0x210
>      tpm_chip_register+0x134/0x320
>      tpm_ibmvtpm_probe+0x520/0x7d0
>      vio_bus_probe+0x9c/0x460
>      really_probe+0x104/0x420
>      __driver_probe_device+0xb0/0x170
>      driver_probe_device+0x58/0x180
>      __driver_attach+0xd8/0x250
>      bus_for_each_dev+0xb4/0x140
>      driver_attach+0x34/0x50
>      bus_add_driver+0x1e8/0x2d0
>      driver_register+0xb4/0x1c0
>      __vio_register_driver+0x74/0x9c
>      ibmvtpm_module_init+0x34/0x48
>      do_one_initcall+0x80/0x320
>      kernel_init_freeable+0x304/0x3ac
>      kernel_init+0x30/0x1a0
>      ret_from_kernel_thread+0x5c/0x64
> 
> To fix the crash, add the SML region to the usable memory areas for the
> kdump kernel, so that the second kernel will map the region. To avoid
> corruption of the region, add the region to the reserved memory areas,

To me the 2nd paragraph and the one below seem to say that in general it does NOT 'avoid corruption of the region.'


> so that the second kernel does not use the memory for something else.
> 
> Note that when loading a kdump kernel with the regular kexec_load()
> syscall the SML may be overwritten by the kdump kernel, depending on
> where the SML is in memory in relation to the crashkernel region. That
> is a separate problem that is not solved by this patch.
> 
> Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

I agree to the code:

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 2/2] powerpc/tpm: Reserve SML log when kexec'ing with kexec_file_load()
@ 2023-06-19  0:26     ` Stefan Berger
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Berger @ 2023-06-19  0:26 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: eajames, jgg, jarkko, yangyingliang, linux-integrity, peterhuewe



On 6/15/23 08:37, Michael Ellerman wrote:
> The TPM code in prom_init.c creates a small buffer of memory to store
> the TPM's SML (Stored Measurement Log). It's communicated to Linux via
> the linux,sml-base/size device tree properties of the TPM node.
> 
> When kexec'ing that buffer can be overwritten, or when kdump'ing it may
> not be mapped by the second kernel. The latter can lead to a crash when
> booting the second kernel such as:
> 
>    tpm_ibmvtpm 71000003: CRQ initialization completed
>    BUG: Unable to handle kernel data access on read at 0xc00000002ffb0000
>    Faulting instruction address: 0xc0000000200a70e0
>    Oops: Kernel access of bad area, sig: 11 [#1]
>    LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries
>    Modules linked in:
>    CPU: 0 PID: 1 Comm: swapper/0 Not tainted 6.2.0-rc2-00134-g9307ce092f5d #314
>    Hardware name: IBM pSeries (emulated by qemu) POWER9 (raw) 0x4e1200 0xf000005 of:SLOF,git-5b4c5a pSeries
>    NIP:  c0000000200a70e0 LR: c0000000203dd5dc CTR: 0000000000000800
>    REGS: c000000024543280 TRAP: 0300   Not tainted  (6.2.0-rc2-00134-g9307ce092f5d)
>    MSR:  8000000002009033 <SF,VEC,EE,ME,IR,DR,RI,LE>  CR: 24002280  XER: 00000006
>    CFAR: c0000000200a70c8 DAR: c00000002ffb0000 DSISR: 40000000 IRQMASK: 0
>    ...
>    NIP memcpy_power7+0x400/0x7d0
>    LR  kmemdup+0x5c/0x80
>    Call Trace:
>      memcpy_power7+0x274/0x7d0 (unreliable)
>      kmemdup+0x5c/0x80
>      tpm_read_log_of+0xe8/0x1b0
>      tpm_bios_log_setup+0x60/0x210
>      tpm_chip_register+0x134/0x320
>      tpm_ibmvtpm_probe+0x520/0x7d0
>      vio_bus_probe+0x9c/0x460
>      really_probe+0x104/0x420
>      __driver_probe_device+0xb0/0x170
>      driver_probe_device+0x58/0x180
>      __driver_attach+0xd8/0x250
>      bus_for_each_dev+0xb4/0x140
>      driver_attach+0x34/0x50
>      bus_add_driver+0x1e8/0x2d0
>      driver_register+0xb4/0x1c0
>      __vio_register_driver+0x74/0x9c
>      ibmvtpm_module_init+0x34/0x48
>      do_one_initcall+0x80/0x320
>      kernel_init_freeable+0x304/0x3ac
>      kernel_init+0x30/0x1a0
>      ret_from_kernel_thread+0x5c/0x64
> 
> To fix the crash, add the SML region to the usable memory areas for the
> kdump kernel, so that the second kernel will map the region. To avoid
> corruption of the region, add the region to the reserved memory areas,

To me the 2nd paragraph and the one below seem to say that in general it does NOT 'avoid corruption of the region.'


> so that the second kernel does not use the memory for something else.
> 
> Note that when loading a kdump kernel with the regular kexec_load()
> syscall the SML may be overwritten by the kdump kernel, depending on
> where the SML is in memory in relation to the crashkernel region. That
> is a separate problem that is not solved by this patch.
> 
> Fixes: a0458284f062 ("powerpc: Add support code for kexec_file_load()")
> Reported-by: Stefan Berger <stefanb@linux.ibm.com>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>

I agree to the code:

Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-06-15 12:37 ` Michael Ellerman
@ 2023-07-10 21:23   ` Jarkko Sakkinen
  -1 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-10 21:23 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: stefanb, eajames, jgg, yangyingliang, linux-integrity, peterhuewe

On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> Measurement Log) from OF to Linux, before Linux shuts down Open
> Firmware.
> 
> This involves creating a buffer to hold the SML, and creating two device
> tree properties to record its base address and size. The kernel then
> later reads those properties from the device tree to find the SML.
> 
> When the code was initially added in commit 4a727429abec ("PPC64: Add
> support for instantiating SML from Open Firmware") the powerpc kernel
> was always built big endian, so the properties were created big endian
> by default.
> 
> However since then little endian support was added to powerpc, and now
> the code lacks conversions to big endian when creating the properties.
> 
> This means on little endian kernels the device tree properties are
> little endian, which is contrary to the device tree spec, and in
> contrast to all other device tree properties.
> 
> To cope with that a workaround was added in tpm_read_log_of() to skip
> the endian conversion if the properties were created via the SML
> handover.
> 
> A better solution is to encode the properties as big endian as they
> should be, and remove the workaround.
> 
> Typically changing the encoding of a property like this would present
> problems for kexec. However the SML is not propagated across kexec, so
> changing the encoding of the properties is a non-issue.
> 
> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  arch/powerpc/kernel/prom_init.c |  8 ++++++--
>  drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>  2 files changed, 10 insertions(+), 21 deletions(-)

Split into two patches (producer and consumer).

BR, Jarkko

> 
> v2: Add Stefan's reviewed-by.
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index d464ba412084..72fe306b6820 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
>  	u32 entry = 0, size = 0, succ = 0;
>  	u64 base;
>  	__be32 val;
> +	__be64 val64;
>  
>  	prom_debug("prom_instantiate_sml: start...\n");
>  
> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>  
>  	reserve_mem(base, size);
>  
> +	val64 = cpu_to_be64(base);
>  	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> -		     &base, sizeof(base));
> +		     &val64, sizeof(val64));
> +
> +	val = cpu_to_be32(size);
>  	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> -		     &size, sizeof(size));
> +		     &val, sizeof(val));
>  
>  	prom_debug("sml base     = 0x%llx\n", base);
>  	prom_debug("sml size     = 0x%x\n", size);
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..0bc0cb6333c6 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
>  int tpm_read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
> -	const u32 *sizep;
> -	const u64 *basep;
> +	const __be32 *sizep;
> +	const __be64 *basep;
>  	struct tpm_bios_log *log;
>  	u32 size;
>  	u64 base;
> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	if (sizep == NULL || basep == NULL)
>  		return -EIO;
>  
> -	/*
> -	 * For both vtpm/tpm, firmware has log addr and log size in big
> -	 * endian format. But in case of vtpm, there is a method called
> -	 * sml-handover which is run during kernel init even before
> -	 * device tree is setup. This sml-handover function takes care
> -	 * of endianness and writes to sml-base and sml-size in little
> -	 * endian format. For this reason, vtpm doesn't need conversion
> -	 * but physical tpm needs the conversion.
> -	 */
> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> -	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> -		size = be32_to_cpup((__force __be32 *)sizep);
> -		base = be64_to_cpup((__force __be64 *)basep);
> -	} else {
> -		size = *sizep;
> -		base = *basep;
> -	}
> +	size = be32_to_cpup(sizep);
> +	base = be64_to_cpup(basep);
>  
>  	if (size == 0) {
>  		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-10 21:23   ` Jarkko Sakkinen
  0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-10 21:23 UTC (permalink / raw)
  To: Michael Ellerman, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe, stefanb

On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> Measurement Log) from OF to Linux, before Linux shuts down Open
> Firmware.
> 
> This involves creating a buffer to hold the SML, and creating two device
> tree properties to record its base address and size. The kernel then
> later reads those properties from the device tree to find the SML.
> 
> When the code was initially added in commit 4a727429abec ("PPC64: Add
> support for instantiating SML from Open Firmware") the powerpc kernel
> was always built big endian, so the properties were created big endian
> by default.
> 
> However since then little endian support was added to powerpc, and now
> the code lacks conversions to big endian when creating the properties.
> 
> This means on little endian kernels the device tree properties are
> little endian, which is contrary to the device tree spec, and in
> contrast to all other device tree properties.
> 
> To cope with that a workaround was added in tpm_read_log_of() to skip
> the endian conversion if the properties were created via the SML
> handover.
> 
> A better solution is to encode the properties as big endian as they
> should be, and remove the workaround.
> 
> Typically changing the encoding of a property like this would present
> problems for kexec. However the SML is not propagated across kexec, so
> changing the encoding of the properties is a non-issue.
> 
> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> ---
>  arch/powerpc/kernel/prom_init.c |  8 ++++++--
>  drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>  2 files changed, 10 insertions(+), 21 deletions(-)

Split into two patches (producer and consumer).

BR, Jarkko

> 
> v2: Add Stefan's reviewed-by.
> 
> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> index d464ba412084..72fe306b6820 100644
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
>  	u32 entry = 0, size = 0, succ = 0;
>  	u64 base;
>  	__be32 val;
> +	__be64 val64;
>  
>  	prom_debug("prom_instantiate_sml: start...\n");
>  
> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>  
>  	reserve_mem(base, size);
>  
> +	val64 = cpu_to_be64(base);
>  	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
> -		     &base, sizeof(base));
> +		     &val64, sizeof(val64));
> +
> +	val = cpu_to_be32(size);
>  	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
> -		     &size, sizeof(size));
> +		     &val, sizeof(val));
>  
>  	prom_debug("sml base     = 0x%llx\n", base);
>  	prom_debug("sml size     = 0x%x\n", size);
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index 930fe43d5daf..0bc0cb6333c6 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
>  int tpm_read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
> -	const u32 *sizep;
> -	const u64 *basep;
> +	const __be32 *sizep;
> +	const __be64 *basep;
>  	struct tpm_bios_log *log;
>  	u32 size;
>  	u64 base;
> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	if (sizep == NULL || basep == NULL)
>  		return -EIO;
>  
> -	/*
> -	 * For both vtpm/tpm, firmware has log addr and log size in big
> -	 * endian format. But in case of vtpm, there is a method called
> -	 * sml-handover which is run during kernel init even before
> -	 * device tree is setup. This sml-handover function takes care
> -	 * of endianness and writes to sml-base and sml-size in little
> -	 * endian format. For this reason, vtpm doesn't need conversion
> -	 * but physical tpm needs the conversion.
> -	 */
> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> -	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> -		size = be32_to_cpup((__force __be32 *)sizep);
> -		base = be64_to_cpup((__force __be64 *)basep);
> -	} else {
> -		size = *sizep;
> -		base = *basep;
> -	}
> +	size = be32_to_cpup(sizep);
> +	base = be64_to_cpup(basep);
>  
>  	if (size == 0) {
>  		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-07-10 21:23   ` Jarkko Sakkinen
@ 2023-07-11 12:47     ` Stefan Berger
  -1 siblings, 0 replies; 18+ messages in thread
From: Stefan Berger @ 2023-07-11 12:47 UTC (permalink / raw)
  To: Jarkko Sakkinen, Michael Ellerman, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe



On 7/10/23 17:23, Jarkko Sakkinen wrote:
> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>> Measurement Log) from OF to Linux, before Linux shuts down Open
>> Firmware.
>>
>> This involves creating a buffer to hold the SML, and creating two device
>> tree properties to record its base address and size. The kernel then
>> later reads those properties from the device tree to find the SML.
>>
>> When the code was initially added in commit 4a727429abec ("PPC64: Add
>> support for instantiating SML from Open Firmware") the powerpc kernel
>> was always built big endian, so the properties were created big endian
>> by default.
>>
>> However since then little endian support was added to powerpc, and now
>> the code lacks conversions to big endian when creating the properties.
>>
>> This means on little endian kernels the device tree properties are
>> little endian, which is contrary to the device tree spec, and in
>> contrast to all other device tree properties.
>>
>> To cope with that a workaround was added in tpm_read_log_of() to skip
>> the endian conversion if the properties were created via the SML
>> handover.
>>
>> A better solution is to encode the properties as big endian as they
>> should be, and remove the workaround.
>>
>> Typically changing the encoding of a property like this would present
>> problems for kexec. However the SML is not propagated across kexec, so
>> changing the encoding of the properties is a non-issue.
>>
>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>>   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>>   2 files changed, 10 insertions(+), 21 deletions(-)
> 
> Split into two patches (producer and consumer).

I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.

     Stefan
> 
> BR, Jarkko
> 
>>
>> v2: Add Stefan's reviewed-by.
>>
>> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
>> index d464ba412084..72fe306b6820 100644
>> --- a/arch/powerpc/kernel/prom_init.c
>> +++ b/arch/powerpc/kernel/prom_init.c
>> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
>>   	u32 entry = 0, size = 0, succ = 0;
>>   	u64 base;
>>   	__be32 val;
>> +	__be64 val64;
>>   
>>   	prom_debug("prom_instantiate_sml: start...\n");
>>   
>> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>>   
>>   	reserve_mem(base, size);
>>   
>> +	val64 = cpu_to_be64(base);
>>   	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
>> -		     &base, sizeof(base));
>> +		     &val64, sizeof(val64));
>> +
>> +	val = cpu_to_be32(size);
>>   	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
>> -		     &size, sizeof(size));
>> +		     &val, sizeof(val));
>>   
>>   	prom_debug("sml base     = 0x%llx\n", base);
>>   	prom_debug("sml size     = 0x%x\n", size);
>> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
>> index 930fe43d5daf..0bc0cb6333c6 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
>>   int tpm_read_log_of(struct tpm_chip *chip)
>>   {
>>   	struct device_node *np;
>> -	const u32 *sizep;
>> -	const u64 *basep;
>> +	const __be32 *sizep;
>> +	const __be64 *basep;
>>   	struct tpm_bios_log *log;
>>   	u32 size;
>>   	u64 base;
>> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>   	if (sizep == NULL || basep == NULL)
>>   		return -EIO;
>>   
>> -	/*
>> -	 * For both vtpm/tpm, firmware has log addr and log size in big
>> -	 * endian format. But in case of vtpm, there is a method called
>> -	 * sml-handover which is run during kernel init even before
>> -	 * device tree is setup. This sml-handover function takes care
>> -	 * of endianness and writes to sml-base and sml-size in little
>> -	 * endian format. For this reason, vtpm doesn't need conversion
>> -	 * but physical tpm needs the conversion.
>> -	 */
>> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> -	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>> -		size = be32_to_cpup((__force __be32 *)sizep);
>> -		base = be64_to_cpup((__force __be64 *)basep);
>> -	} else {
>> -		size = *sizep;
>> -		base = *basep;
>> -	}
>> +	size = be32_to_cpup(sizep);
>> +	base = be64_to_cpup(basep);
>>   
>>   	if (size == 0) {
>>   		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-11 12:47     ` Stefan Berger
  0 siblings, 0 replies; 18+ messages in thread
From: Stefan Berger @ 2023-07-11 12:47 UTC (permalink / raw)
  To: Jarkko Sakkinen, Michael Ellerman, linuxppc-dev
  Cc: jgg, linux-integrity, eajames, peterhuewe, yangyingliang



On 7/10/23 17:23, Jarkko Sakkinen wrote:
> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>> Measurement Log) from OF to Linux, before Linux shuts down Open
>> Firmware.
>>
>> This involves creating a buffer to hold the SML, and creating two device
>> tree properties to record its base address and size. The kernel then
>> later reads those properties from the device tree to find the SML.
>>
>> When the code was initially added in commit 4a727429abec ("PPC64: Add
>> support for instantiating SML from Open Firmware") the powerpc kernel
>> was always built big endian, so the properties were created big endian
>> by default.
>>
>> However since then little endian support was added to powerpc, and now
>> the code lacks conversions to big endian when creating the properties.
>>
>> This means on little endian kernels the device tree properties are
>> little endian, which is contrary to the device tree spec, and in
>> contrast to all other device tree properties.
>>
>> To cope with that a workaround was added in tpm_read_log_of() to skip
>> the endian conversion if the properties were created via the SML
>> handover.
>>
>> A better solution is to encode the properties as big endian as they
>> should be, and remove the workaround.
>>
>> Typically changing the encoding of a property like this would present
>> problems for kexec. However the SML is not propagated across kexec, so
>> changing the encoding of the properties is a non-issue.
>>
>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>>   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>>   2 files changed, 10 insertions(+), 21 deletions(-)
> 
> Split into two patches (producer and consumer).

I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.

     Stefan
> 
> BR, Jarkko
> 
>>
>> v2: Add Stefan's reviewed-by.
>>
>> diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
>> index d464ba412084..72fe306b6820 100644
>> --- a/arch/powerpc/kernel/prom_init.c
>> +++ b/arch/powerpc/kernel/prom_init.c
>> @@ -1900,6 +1900,7 @@ static void __init prom_instantiate_sml(void)
>>   	u32 entry = 0, size = 0, succ = 0;
>>   	u64 base;
>>   	__be32 val;
>> +	__be64 val64;
>>   
>>   	prom_debug("prom_instantiate_sml: start...\n");
>>   
>> @@ -1956,10 +1957,13 @@ static void __init prom_instantiate_sml(void)
>>   
>>   	reserve_mem(base, size);
>>   
>> +	val64 = cpu_to_be64(base);
>>   	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-base",
>> -		     &base, sizeof(base));
>> +		     &val64, sizeof(val64));
>> +
>> +	val = cpu_to_be32(size);
>>   	prom_setprop(ibmvtpm_node, "/vdevice/vtpm", "linux,sml-size",
>> -		     &size, sizeof(size));
>> +		     &val, sizeof(val));
>>   
>>   	prom_debug("sml base     = 0x%llx\n", base);
>>   	prom_debug("sml size     = 0x%x\n", size);
>> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
>> index 930fe43d5daf..0bc0cb6333c6 100644
>> --- a/drivers/char/tpm/eventlog/of.c
>> +++ b/drivers/char/tpm/eventlog/of.c
>> @@ -51,8 +51,8 @@ static int tpm_read_log_memory_region(struct tpm_chip *chip)
>>   int tpm_read_log_of(struct tpm_chip *chip)
>>   {
>>   	struct device_node *np;
>> -	const u32 *sizep;
>> -	const u64 *basep;
>> +	const __be32 *sizep;
>> +	const __be64 *basep;
>>   	struct tpm_bios_log *log;
>>   	u32 size;
>>   	u64 base;
>> @@ -73,23 +73,8 @@ int tpm_read_log_of(struct tpm_chip *chip)
>>   	if (sizep == NULL || basep == NULL)
>>   		return -EIO;
>>   
>> -	/*
>> -	 * For both vtpm/tpm, firmware has log addr and log size in big
>> -	 * endian format. But in case of vtpm, there is a method called
>> -	 * sml-handover which is run during kernel init even before
>> -	 * device tree is setup. This sml-handover function takes care
>> -	 * of endianness and writes to sml-base and sml-size in little
>> -	 * endian format. For this reason, vtpm doesn't need conversion
>> -	 * but physical tpm needs the conversion.
>> -	 */
>> -	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> -	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>> -		size = be32_to_cpup((__force __be32 *)sizep);
>> -		base = be64_to_cpup((__force __be64 *)basep);
>> -	} else {
>> -		size = *sizep;
>> -		base = *basep;
>> -	}
>> +	size = be32_to_cpup(sizep);
>> +	base = be64_to_cpup(basep);
>>   
>>   	if (size == 0) {
>>   		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-07-11 12:47     ` Stefan Berger
@ 2023-07-11 21:54       ` Jarkko Sakkinen
  -1 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-11 21:54 UTC (permalink / raw)
  To: Stefan Berger, Michael Ellerman, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe

On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> 
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> > > Firmware.
> > > 
> > > This involves creating a buffer to hold the SML, and creating two device
> > > tree properties to record its base address and size. The kernel then
> > > later reads those properties from the device tree to find the SML.
> > > 
> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > > support for instantiating SML from Open Firmware") the powerpc kernel
> > > was always built big endian, so the properties were created big endian
> > > by default.
> > > 
> > > However since then little endian support was added to powerpc, and now
> > > the code lacks conversions to big endian when creating the properties.
> > > 
> > > This means on little endian kernels the device tree properties are
> > > little endian, which is contrary to the device tree spec, and in
> > > contrast to all other device tree properties.
> > > 
> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> > > the endian conversion if the properties were created via the SML
> > > handover.
> > > 
> > > A better solution is to encode the properties as big endian as they
> > > should be, and remove the workaround.
> > > 
> > > Typically changing the encoding of a property like this would present
> > > problems for kexec. However the SML is not propagated across kexec, so
> > > changing the encoding of the properties is a non-issue.
> > > 
> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > ---
> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
> > >   2 files changed, 10 insertions(+), 21 deletions(-)
> > 
> > Split into two patches (producer and consumer).
> 
> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.

I think it would help if the commit message would better explain
what is going on. It is somewhat difficult to decipher, if you
don't have deep knowledge of the powerpc architecture.

BR, Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-11 21:54       ` Jarkko Sakkinen
  0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-11 21:54 UTC (permalink / raw)
  To: Stefan Berger, Michael Ellerman, linuxppc-dev
  Cc: jgg, linux-integrity, eajames, peterhuewe, yangyingliang

On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> 
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> > > Firmware.
> > > 
> > > This involves creating a buffer to hold the SML, and creating two device
> > > tree properties to record its base address and size. The kernel then
> > > later reads those properties from the device tree to find the SML.
> > > 
> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> > > support for instantiating SML from Open Firmware") the powerpc kernel
> > > was always built big endian, so the properties were created big endian
> > > by default.
> > > 
> > > However since then little endian support was added to powerpc, and now
> > > the code lacks conversions to big endian when creating the properties.
> > > 
> > > This means on little endian kernels the device tree properties are
> > > little endian, which is contrary to the device tree spec, and in
> > > contrast to all other device tree properties.
> > > 
> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> > > the endian conversion if the properties were created via the SML
> > > handover.
> > > 
> > > A better solution is to encode the properties as big endian as they
> > > should be, and remove the workaround.
> > > 
> > > Typically changing the encoding of a property like this would present
> > > problems for kexec. However the SML is not propagated across kexec, so
> > > changing the encoding of the properties is a non-issue.
> > > 
> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> > > ---
> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
> > >   2 files changed, 10 insertions(+), 21 deletions(-)
> > 
> > Split into two patches (producer and consumer).
> 
> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.

I think it would help if the commit message would better explain
what is going on. It is somewhat difficult to decipher, if you
don't have deep knowledge of the powerpc architecture.

BR, Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-07-11 12:47     ` Stefan Berger
@ 2023-07-12 12:34       ` Michael Ellerman
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-07-12 12:34 UTC (permalink / raw)
  To: Stefan Berger, Jarkko Sakkinen, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe

Stefan Berger <stefanb@linux.ibm.com> writes:
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
>> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>>> Measurement Log) from OF to Linux, before Linux shuts down Open
>>> Firmware.
>>>
>>> This involves creating a buffer to hold the SML, and creating two device
>>> tree properties to record its base address and size. The kernel then
>>> later reads those properties from the device tree to find the SML.
>>>
>>> When the code was initially added in commit 4a727429abec ("PPC64: Add
>>> support for instantiating SML from Open Firmware") the powerpc kernel
>>> was always built big endian, so the properties were created big endian
>>> by default.
>>>
>>> However since then little endian support was added to powerpc, and now
>>> the code lacks conversions to big endian when creating the properties.
>>>
>>> This means on little endian kernels the device tree properties are
>>> little endian, which is contrary to the device tree spec, and in
>>> contrast to all other device tree properties.
>>>
>>> To cope with that a workaround was added in tpm_read_log_of() to skip
>>> the endian conversion if the properties were created via the SML
>>> handover.
>>>
>>> A better solution is to encode the properties as big endian as they
>>> should be, and remove the workaround.
>>>
>>> Typically changing the encoding of a property like this would present
>>> problems for kexec. However the SML is not propagated across kexec, so
>>> changing the encoding of the properties is a non-issue.
>>>
>>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>>>   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>>>   2 files changed, 10 insertions(+), 21 deletions(-)
>> 
>> Split into two patches (producer and consumer).
>
> I think this wouldn't be right since it would break the system when
> only one patch is applied since it would be reading the fields in the
> wrong endianess.

Yes that's right.

Typically we would split the patch by subsystem, but in this case it
will just create a known-broken point in the git history, which helps no
one.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-12 12:34       ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-07-12 12:34 UTC (permalink / raw)
  To: Stefan Berger, Jarkko Sakkinen, linuxppc-dev
  Cc: jgg, linux-integrity, eajames, peterhuewe, yangyingliang

Stefan Berger <stefanb@linux.ibm.com> writes:
> On 7/10/23 17:23, Jarkko Sakkinen wrote:
>> On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>>> There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>>> Measurement Log) from OF to Linux, before Linux shuts down Open
>>> Firmware.
>>>
>>> This involves creating a buffer to hold the SML, and creating two device
>>> tree properties to record its base address and size. The kernel then
>>> later reads those properties from the device tree to find the SML.
>>>
>>> When the code was initially added in commit 4a727429abec ("PPC64: Add
>>> support for instantiating SML from Open Firmware") the powerpc kernel
>>> was always built big endian, so the properties were created big endian
>>> by default.
>>>
>>> However since then little endian support was added to powerpc, and now
>>> the code lacks conversions to big endian when creating the properties.
>>>
>>> This means on little endian kernels the device tree properties are
>>> little endian, which is contrary to the device tree spec, and in
>>> contrast to all other device tree properties.
>>>
>>> To cope with that a workaround was added in tpm_read_log_of() to skip
>>> the endian conversion if the properties were created via the SML
>>> handover.
>>>
>>> A better solution is to encode the properties as big endian as they
>>> should be, and remove the workaround.
>>>
>>> Typically changing the encoding of a property like this would present
>>> problems for kexec. However the SML is not propagated across kexec, so
>>> changing the encoding of the properties is a non-issue.
>>>
>>> Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>>> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>>> Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>>> ---
>>>   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>>>   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>>>   2 files changed, 10 insertions(+), 21 deletions(-)
>> 
>> Split into two patches (producer and consumer).
>
> I think this wouldn't be right since it would break the system when
> only one patch is applied since it would be reading the fields in the
> wrong endianess.

Yes that's right.

Typically we would split the patch by subsystem, but in this case it
will just create a known-broken point in the git history, which helps no
one.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-07-11 21:54       ` Jarkko Sakkinen
@ 2023-07-12 12:39         ` Michael Ellerman
  -1 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-07-12 12:39 UTC (permalink / raw)
  To: Jarkko Sakkinen, Stefan Berger, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe

Jarkko Sakkinen <jarkko@kernel.org> writes:
> On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
>> On 7/10/23 17:23, Jarkko Sakkinen wrote:
>> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>> > > Measurement Log) from OF to Linux, before Linux shuts down Open
>> > > Firmware.
>> > > 
>> > > This involves creating a buffer to hold the SML, and creating two device
>> > > tree properties to record its base address and size. The kernel then
>> > > later reads those properties from the device tree to find the SML.
>> > > 
>> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
>> > > support for instantiating SML from Open Firmware") the powerpc kernel
>> > > was always built big endian, so the properties were created big endian
>> > > by default.
>> > > 
>> > > However since then little endian support was added to powerpc, and now
>> > > the code lacks conversions to big endian when creating the properties.
>> > > 
>> > > This means on little endian kernels the device tree properties are
>> > > little endian, which is contrary to the device tree spec, and in
>> > > contrast to all other device tree properties.
>> > > 
>> > > To cope with that a workaround was added in tpm_read_log_of() to skip
>> > > the endian conversion if the properties were created via the SML
>> > > handover.
>> > > 
>> > > A better solution is to encode the properties as big endian as they
>> > > should be, and remove the workaround.
>> > > 
>> > > Typically changing the encoding of a property like this would present
>> > > problems for kexec. However the SML is not propagated across kexec, so
>> > > changing the encoding of the properties is a non-issue.
>> > > 
>> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> > > ---
>> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>> > >   2 files changed, 10 insertions(+), 21 deletions(-)
>> > 
>> > Split into two patches (producer and consumer).
>> 
>> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.
>
> I think it would help if the commit message would better explain
> what is going on. It is somewhat difficult to decipher, if you
> don't have deep knowledge of the powerpc architecture.

I mean, it's already 8 paragraphs ¯\_(ツ)_/¯

But I'm happy to expand it. I just don't really know what extra detail
is needed to make it clearer.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-12 12:39         ` Michael Ellerman
  0 siblings, 0 replies; 18+ messages in thread
From: Michael Ellerman @ 2023-07-12 12:39 UTC (permalink / raw)
  To: Jarkko Sakkinen, Stefan Berger, linuxppc-dev
  Cc: jgg, linux-integrity, eajames, peterhuewe, yangyingliang

Jarkko Sakkinen <jarkko@kernel.org> writes:
> On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
>> On 7/10/23 17:23, Jarkko Sakkinen wrote:
>> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
>> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
>> > > Measurement Log) from OF to Linux, before Linux shuts down Open
>> > > Firmware.
>> > > 
>> > > This involves creating a buffer to hold the SML, and creating two device
>> > > tree properties to record its base address and size. The kernel then
>> > > later reads those properties from the device tree to find the SML.
>> > > 
>> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
>> > > support for instantiating SML from Open Firmware") the powerpc kernel
>> > > was always built big endian, so the properties were created big endian
>> > > by default.
>> > > 
>> > > However since then little endian support was added to powerpc, and now
>> > > the code lacks conversions to big endian when creating the properties.
>> > > 
>> > > This means on little endian kernels the device tree properties are
>> > > little endian, which is contrary to the device tree spec, and in
>> > > contrast to all other device tree properties.
>> > > 
>> > > To cope with that a workaround was added in tpm_read_log_of() to skip
>> > > the endian conversion if the properties were created via the SML
>> > > handover.
>> > > 
>> > > A better solution is to encode the properties as big endian as they
>> > > should be, and remove the workaround.
>> > > 
>> > > Typically changing the encoding of a property like this would present
>> > > problems for kexec. However the SML is not propagated across kexec, so
>> > > changing the encoding of the properties is a non-issue.
>> > > 
>> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
>> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
>> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
>> > > ---
>> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
>> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
>> > >   2 files changed, 10 insertions(+), 21 deletions(-)
>> > 
>> > Split into two patches (producer and consumer).
>> 
>> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.
>
> I think it would help if the commit message would better explain
> what is going on. It is somewhat difficult to decipher, if you
> don't have deep knowledge of the powerpc architecture.

I mean, it's already 8 paragraphs ¯\_(ツ)_/¯

But I'm happy to expand it. I just don't really know what extra detail
is needed to make it clearer.

cheers

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
  2023-07-12 12:39         ` Michael Ellerman
@ 2023-07-17 13:13           ` Jarkko Sakkinen
  -1 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-17 13:13 UTC (permalink / raw)
  To: Michael Ellerman, Stefan Berger, linuxppc-dev
  Cc: eajames, jgg, yangyingliang, linux-integrity, peterhuewe

On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote:
> Jarkko Sakkinen <jarkko@kernel.org> writes:
> > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> >> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > > Firmware.
> >> > > 
> >> > > This involves creating a buffer to hold the SML, and creating two device
> >> > > tree properties to record its base address and size. The kernel then
> >> > > later reads those properties from the device tree to find the SML.
> >> > > 
> >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > > was always built big endian, so the properties were created big endian
> >> > > by default.
> >> > > 
> >> > > However since then little endian support was added to powerpc, and now
> >> > > the code lacks conversions to big endian when creating the properties.
> >> > > 
> >> > > This means on little endian kernels the device tree properties are
> >> > > little endian, which is contrary to the device tree spec, and in
> >> > > contrast to all other device tree properties.
> >> > > 
> >> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > > the endian conversion if the properties were created via the SML
> >> > > handover.
> >> > > 
> >> > > A better solution is to encode the properties as big endian as they
> >> > > should be, and remove the workaround.
> >> > > 
> >> > > Typically changing the encoding of a property like this would present
> >> > > problems for kexec. However the SML is not propagated across kexec, so
> >> > > changing the encoding of the properties is a non-issue.
> >> > > 
> >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> >> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> >> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> >> > > ---
> >> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
> >> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
> >> > >   2 files changed, 10 insertions(+), 21 deletions(-)
> >> > 
> >> > Split into two patches (producer and consumer).
> >> 
> >> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.
> >
> > I think it would help if the commit message would better explain
> > what is going on. It is somewhat difficult to decipher, if you
> > don't have deep knowledge of the powerpc architecture.
>
> I mean, it's already 8 paragraphs ¯\_(ツ)_/¯
>
> But I'm happy to expand it. I just don't really know what extra detail
> is needed to make it clearer.

Adding more text is not the right way to clarify things. I'd start
by explaining shortly SML and then move to the handover. It can't
be that hard, right?

Just adding new paragraphs would probably just make it even more
confusing.

BR, Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian
@ 2023-07-17 13:13           ` Jarkko Sakkinen
  0 siblings, 0 replies; 18+ messages in thread
From: Jarkko Sakkinen @ 2023-07-17 13:13 UTC (permalink / raw)
  To: Michael Ellerman, Stefan Berger, linuxppc-dev
  Cc: jgg, linux-integrity, eajames, peterhuewe, yangyingliang

On Wed Jul 12, 2023 at 12:39 PM UTC, Michael Ellerman wrote:
> Jarkko Sakkinen <jarkko@kernel.org> writes:
> > On Tue, 2023-07-11 at 08:47 -0400, Stefan Berger wrote:
> >> On 7/10/23 17:23, Jarkko Sakkinen wrote:
> >> > On Thu, 2023-06-15 at 22:37 +1000, Michael Ellerman wrote:
> >> > > There's code in prom_instantiate_sml() to do a "SML handover" (Stored
> >> > > Measurement Log) from OF to Linux, before Linux shuts down Open
> >> > > Firmware.
> >> > > 
> >> > > This involves creating a buffer to hold the SML, and creating two device
> >> > > tree properties to record its base address and size. The kernel then
> >> > > later reads those properties from the device tree to find the SML.
> >> > > 
> >> > > When the code was initially added in commit 4a727429abec ("PPC64: Add
> >> > > support for instantiating SML from Open Firmware") the powerpc kernel
> >> > > was always built big endian, so the properties were created big endian
> >> > > by default.
> >> > > 
> >> > > However since then little endian support was added to powerpc, and now
> >> > > the code lacks conversions to big endian when creating the properties.
> >> > > 
> >> > > This means on little endian kernels the device tree properties are
> >> > > little endian, which is contrary to the device tree spec, and in
> >> > > contrast to all other device tree properties.
> >> > > 
> >> > > To cope with that a workaround was added in tpm_read_log_of() to skip
> >> > > the endian conversion if the properties were created via the SML
> >> > > handover.
> >> > > 
> >> > > A better solution is to encode the properties as big endian as they
> >> > > should be, and remove the workaround.
> >> > > 
> >> > > Typically changing the encoding of a property like this would present
> >> > > problems for kexec. However the SML is not propagated across kexec, so
> >> > > changing the encoding of the properties is a non-issue.
> >> > > 
> >> > > Fixes: e46e22f12b19 ("tpm: enhance read_log_of() to support Physical TPM event log")
> >> > > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> >> > > Reviewed-by: Stefan Berger <stefanb@linux.ibm.com>
> >> > > ---
> >> > >   arch/powerpc/kernel/prom_init.c |  8 ++++++--
> >> > >   drivers/char/tpm/eventlog/of.c  | 23 ++++-------------------
> >> > >   2 files changed, 10 insertions(+), 21 deletions(-)
> >> > 
> >> > Split into two patches (producer and consumer).
> >> 
> >> I think this wouldn't be right since it would break the system when only one patch is applied since it would be reading the fields in the wrong endianess.
> >
> > I think it would help if the commit message would better explain
> > what is going on. It is somewhat difficult to decipher, if you
> > don't have deep knowledge of the powerpc architecture.
>
> I mean, it's already 8 paragraphs ¯\_(ツ)_/¯
>
> But I'm happy to expand it. I just don't really know what extra detail
> is needed to make it clearer.

Adding more text is not the right way to clarify things. I'd start
by explaining shortly SML and then move to the handover. It can't
be that hard, right?

Just adding new paragraphs would probably just make it even more
confusing.

BR, Jarkko

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2023-07-17 13:15 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 12:37 [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian Michael Ellerman
2023-06-15 12:37 ` Michael Ellerman
2023-06-15 12:37 ` [PATCH v2 2/2] powerpc/tpm: Reserve SML log when kexec'ing with kexec_file_load() Michael Ellerman
2023-06-15 12:37   ` Michael Ellerman
2023-06-19  0:26   ` Stefan Berger
2023-06-19  0:26     ` Stefan Berger
2023-07-10 21:23 ` [PATCH v2 1/2] powerpc/tpm: Create linux,sml-base/size as big endian Jarkko Sakkinen
2023-07-10 21:23   ` Jarkko Sakkinen
2023-07-11 12:47   ` Stefan Berger
2023-07-11 12:47     ` Stefan Berger
2023-07-11 21:54     ` Jarkko Sakkinen
2023-07-11 21:54       ` Jarkko Sakkinen
2023-07-12 12:39       ` Michael Ellerman
2023-07-12 12:39         ` Michael Ellerman
2023-07-17 13:13         ` Jarkko Sakkinen
2023-07-17 13:13           ` Jarkko Sakkinen
2023-07-12 12:34     ` Michael Ellerman
2023-07-12 12:34       ` Michael Ellerman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.