[PATCH 0/3] tpm: Preserve TPM measurement log across kexec

[PATCH 0/3] tpm: Preserve TPM measurement log across kexec

[Stefan Berger]

The current of-tree driver does not currently preserve the vTPM 1.2 and
vTPM 2.0 measurement log across a kexec. This series fixes this for the
kexec_file_load() syscall using the flattened device tree (fdt) to
carry the measurement log's buffer across kexec.

   Stefan

Stefan Berger (3):
  of: kexec: Refactor IMA buffer related functions to make them reusable
  tpm/kexec: Duplicate TPM measurement log in of-tree for kexec
  tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

 drivers/char/tpm/eventlog/of.c |  31 +---
 drivers/of/device.c            |  24 +++
 drivers/of/kexec.c             | 284 +++++++++++++++++++++++++++++----
 include/linux/kexec.h          |   6 +
 include/linux/of.h             |   5 +
 include/linux/of_device.h      |   3 +
 kernel/kexec_file.c            |   6 +
 7 files changed, 298 insertions(+), 61 deletions(-)

-- 
2.35.1

[PATCH 0/3] tpm: Preserve TPM measurement log across kexec

[Stefan Berger]

The current of-tree driver does not currently preserve the vTPM 1.2 and
vTPM 2.0 measurement log across a kexec. This series fixes this for the
kexec_file_load() syscall using the flattened device tree (fdt) to
carry the measurement log's buffer across kexec.

   Stefan

Stefan Berger (3):
  of: kexec: Refactor IMA buffer related functions to make them reusable
  tpm/kexec: Duplicate TPM measurement log in of-tree for kexec
  tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

 drivers/char/tpm/eventlog/of.c |  31 +---
 drivers/of/device.c            |  24 +++
 drivers/of/kexec.c             | 284 +++++++++++++++++++++++++++++----
 include/linux/kexec.h          |   6 +
 include/linux/of.h             |   5 +
 include/linux/of_device.h      |   3 +
 kernel/kexec_file.c            |   6 +
 7 files changed, 298 insertions(+), 61 deletions(-)

-- 
2.35.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable

[Stefan Berger]

Refactor IMA buffer related functions to make them reusable for carrying
TPM logs across kexec.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
---
 drivers/of/kexec.c | 98 +++++++++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index b9bd1cff1793..eef6f3b9939c 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -115,6 +115,18 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
 	return 0;
 }
 
+static int get_kexec_buffer(const char *name, unsigned long *addr, size_t *size)
+{
+	const void *prop;
+	int len;
+
+	prop = of_get_property(of_chosen, name, &len);
+	if (!prop)
+		return -ENOENT;
+
+	return do_get_kexec_buffer(prop, len, addr, size);
+}
+
 /**
  * ima_get_kexec_buffer - get IMA buffer from the previous kernel
  * @addr:	On successful return, set to point to the buffer contents.
@@ -124,19 +136,14 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
  */
 int ima_get_kexec_buffer(void **addr, size_t *size)
 {
-	int ret, len;
+	int ret;
 	unsigned long tmp_addr;
 	size_t tmp_size;
-	const void *prop;
 
 	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
 		return -ENOTSUPP;
 
-	prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
-	if (!prop)
-		return -ENOENT;
-
-	ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
+	ret = get_kexec_buffer("linux,ima-kexec-buffer", &tmp_addr, &tmp_size);
 	if (ret)
 		return ret;
 
@@ -174,6 +181,25 @@ int ima_free_kexec_buffer(void)
 	return memblock_phys_free(addr, size);
 }
 
+static int remove_buffer(void *fdt, int chosen_node, const char *name)
+{
+	int ret, len;
+	unsigned long addr;
+	size_t size;
+	const void *prop;
+
+	prop = fdt_getprop(fdt, chosen_node, name, &len);
+	if (!prop)
+		return -ENOENT;
+
+	ret = do_get_kexec_buffer(prop, len, &addr, &size);
+	fdt_delprop(fdt, chosen_node, name);
+	if (ret)
+		return ret;
+
+	return fdt_find_and_del_mem_rsv(fdt, addr, size);
+}
+
 /**
  * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
  *
@@ -185,29 +211,38 @@ int ima_free_kexec_buffer(void)
  */
 static void remove_ima_buffer(void *fdt, int chosen_node)
 {
-	int ret, len;
-	unsigned long addr;
-	size_t size;
-	const void *prop;
+	int ret;
 
 	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
 		return;
 
-	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
-	if (!prop)
-		return;
-
-	ret = do_get_kexec_buffer(prop, len, &addr, &size);
-	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
-	if (ret)
-		return;
-
-	ret = fdt_find_and_del_mem_rsv(fdt, addr, size);
+	ret = remove_buffer(fdt, chosen_node, "linux,ima-kexec-buffer");
 	if (!ret)
 		pr_debug("Removed old IMA buffer reservation.\n");
 }
 
 #ifdef CONFIG_IMA_KEXEC
+static int setup_buffer(void *fdt, int chosen_node, const char *name,
+			uint64_t addr, uint64_t size)
+{
+	int ret;
+
+	if (!size)
+		return 0;
+
+	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
+				       name, addr, size);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = fdt_add_mem_rsv(fdt, addr, size);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+
+}
+
 /**
  * setup_ima_buffer - add IMA buffer information to the fdt
  * @image:		kexec image being loaded.
@@ -221,23 +256,14 @@ static int setup_ima_buffer(const struct kimage *image, void *fdt,
 {
 	int ret;
 
-	if (!image->ima_buffer_size)
-		return 0;
-
-	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
-				       "linux,ima-kexec-buffer",
-				       image->ima_buffer_addr,
-				       image->ima_buffer_size);
-	if (ret < 0)
-		return -EINVAL;
-
-	ret = fdt_add_mem_rsv(fdt, image->ima_buffer_addr,
-			      image->ima_buffer_size);
+	ret = setup_buffer(fdt, chosen_node, "linux,ima-kexec-buffer",
+			   image->ima_buffer_addr, image->ima_buffer_size);
 	if (ret)
-		return -EINVAL;
+		return ret;
 
-	pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n",
-		 image->ima_buffer_addr, image->ima_buffer_size);
+	if (image->ima_buffer_addr)
+		pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n",
+			 image->ima_buffer_addr, image->ima_buffer_size);
 
 	return 0;
 }
-- 
2.35.1

[PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable

[Stefan Berger]

Refactor IMA buffer related functions to make them reusable for carrying
TPM logs across kexec.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
---
 drivers/of/kexec.c | 98 +++++++++++++++++++++++++++++-----------------
 1 file changed, 62 insertions(+), 36 deletions(-)

diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index b9bd1cff1793..eef6f3b9939c 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -115,6 +115,18 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
 	return 0;
 }
 
+static int get_kexec_buffer(const char *name, unsigned long *addr, size_t *size)
+{
+	const void *prop;
+	int len;
+
+	prop = of_get_property(of_chosen, name, &len);
+	if (!prop)
+		return -ENOENT;
+
+	return do_get_kexec_buffer(prop, len, addr, size);
+}
+
 /**
  * ima_get_kexec_buffer - get IMA buffer from the previous kernel
  * @addr:	On successful return, set to point to the buffer contents.
@@ -124,19 +136,14 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
  */
 int ima_get_kexec_buffer(void **addr, size_t *size)
 {
-	int ret, len;
+	int ret;
 	unsigned long tmp_addr;
 	size_t tmp_size;
-	const void *prop;
 
 	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
 		return -ENOTSUPP;
 
-	prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
-	if (!prop)
-		return -ENOENT;
-
-	ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
+	ret = get_kexec_buffer("linux,ima-kexec-buffer", &tmp_addr, &tmp_size);
 	if (ret)
 		return ret;
 
@@ -174,6 +181,25 @@ int ima_free_kexec_buffer(void)
 	return memblock_phys_free(addr, size);
 }
 
+static int remove_buffer(void *fdt, int chosen_node, const char *name)
+{
+	int ret, len;
+	unsigned long addr;
+	size_t size;
+	const void *prop;
+
+	prop = fdt_getprop(fdt, chosen_node, name, &len);
+	if (!prop)
+		return -ENOENT;
+
+	ret = do_get_kexec_buffer(prop, len, &addr, &size);
+	fdt_delprop(fdt, chosen_node, name);
+	if (ret)
+		return ret;
+
+	return fdt_find_and_del_mem_rsv(fdt, addr, size);
+}
+
 /**
  * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
  *
@@ -185,29 +211,38 @@ int ima_free_kexec_buffer(void)
  */
 static void remove_ima_buffer(void *fdt, int chosen_node)
 {
-	int ret, len;
-	unsigned long addr;
-	size_t size;
-	const void *prop;
+	int ret;
 
 	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
 		return;
 
-	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
-	if (!prop)
-		return;
-
-	ret = do_get_kexec_buffer(prop, len, &addr, &size);
-	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
-	if (ret)
-		return;
-
-	ret = fdt_find_and_del_mem_rsv(fdt, addr, size);
+	ret = remove_buffer(fdt, chosen_node, "linux,ima-kexec-buffer");
 	if (!ret)
 		pr_debug("Removed old IMA buffer reservation.\n");
 }
 
 #ifdef CONFIG_IMA_KEXEC
+static int setup_buffer(void *fdt, int chosen_node, const char *name,
+			uint64_t addr, uint64_t size)
+{
+	int ret;
+
+	if (!size)
+		return 0;
+
+	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
+				       name, addr, size);
+	if (ret < 0)
+		return -EINVAL;
+
+	ret = fdt_add_mem_rsv(fdt, addr, size);
+	if (ret)
+		return -EINVAL;
+
+	return 0;
+
+}
+
 /**
  * setup_ima_buffer - add IMA buffer information to the fdt
  * @image:		kexec image being loaded.
@@ -221,23 +256,14 @@ static int setup_ima_buffer(const struct kimage *image, void *fdt,
 {
 	int ret;
 
-	if (!image->ima_buffer_size)
-		return 0;
-
-	ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
-				       "linux,ima-kexec-buffer",
-				       image->ima_buffer_addr,
-				       image->ima_buffer_size);
-	if (ret < 0)
-		return -EINVAL;
-
-	ret = fdt_add_mem_rsv(fdt, image->ima_buffer_addr,
-			      image->ima_buffer_size);
+	ret = setup_buffer(fdt, chosen_node, "linux,ima-kexec-buffer",
+			   image->ima_buffer_addr, image->ima_buffer_size);
 	if (ret)
-		return -EINVAL;
+		return ret;
 
-	pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n",
-		 image->ima_buffer_addr, image->ima_buffer_size);
+	if (image->ima_buffer_addr)
+		pr_debug("IMA buffer at 0x%llx, size = 0x%zx\n",
+			 image->ima_buffer_addr, image->ima_buffer_size);
 
 	return 0;
 }
-- 
2.35.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

The memory area of the TPM measurement log is currently not properly
duplicated for carrying it across kexec when an Open Firmware
Devicetree is used. Therefore, the contents of the log get corrupted.
Fix this for the kexec_file_load() syscall by allocating a buffer and
copying the contents of the existing log into it. The new buffer is
preserved across the kexec and a pointer to it is available when the new
kernel is started. To achieve this, store the allocated buffer's address
in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
and search for this entry early in the kernel startup before the TPM
subsystem starts up. Adjust the pointer in the of-tree stored under
linux,sml-base to point to this buffer holding the preserved log. The
TPM driver can then read the base address from this entry when making
the log available.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Eric Biederman <ebiederm@xmission.com>
---
 drivers/of/device.c       |  24 +++++
 drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
 include/linux/kexec.h     |   6 ++
 include/linux/of.h        |   5 +
 include/linux/of_device.h |   3 +
 kernel/kexec_file.c       |   6 ++
 6 files changed, 233 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 874f031442dc..0cbd47b1cabc 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
+{
+	const u32 *sizep;
+	const u64 *basep;
+
+	sizep = of_get_property(np, "linux,sml-size", NULL);
+	basep = of_get_property(np, "linux,sml-base", NULL);
+	if (sizep == NULL && basep == NULL)
+		return -ENODEV;
+	if (sizep == NULL || basep == NULL)
+		return -EIO;
+
+	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;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index eef6f3b9939c..db5441123a70 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -14,6 +14,7 @@
 #include <linux/memblock.h>
 #include <linux/libfdt.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_fdt.h>
 #include <linux/random.h>
 #include <linux/slab.h>
@@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
 		pr_debug("Removed old IMA buffer reservation.\n");
 }
 
-#ifdef CONFIG_IMA_KEXEC
 static int setup_buffer(void *fdt, int chosen_node, const char *name,
 			uint64_t addr, uint64_t size)
 {
@@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
 
 }
 
+#ifdef CONFIG_IMA_KEXEC
 /**
  * setup_ima_buffer - add IMA buffer information to the fdt
  * @image:		kexec image being loaded.
@@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
 }
 #endif /* CONFIG_IMA_KEXEC */
 
+/**
+ * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
+ * @phyaddr:	On successful return, set to physical address of buffer
+ * @size:	On successful return, set to the buffer size.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
+{
+	int ret;
+	unsigned long tmp_addr;
+	size_t tmp_size;
+
+	ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);
+	if (ret)
+		return ret;
+
+	*phyaddr = (void *)tmp_addr;
+	*size = tmp_size;
+
+	return 0;
+}
+
+/**
+ * tpm_free_kexec_buffer - free memory used by the IMA buffer
+ */
+static int tpm_of_remove_kexec_buffer(void)
+{
+	struct property *prop;
+
+	prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
+	if (!prop)
+		return -ENOENT;
+
+	return of_remove_property(of_chosen, prop);
+}
+
+/**
+ * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
+ *
+ * @fdt: Flattened Device Tree to update
+ * @chosen_node: Offset to the chosen node in the device tree
+ *
+ * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
+ * remove it from the device tree.
+ */
+static void remove_tpm_buffer(void *fdt, int chosen_node)
+{
+	int ret;
+
+	ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
+	if (!ret)
+		pr_debug("Removed old TPM log buffer reservation.\n");
+}
+
+/**
+ * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
+ * @image:		kexec image being loaded.
+ * @fdt:		Flattened device tree for the next kernel.
+ * @chosen_node:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+static int setup_tpm_buffer(const struct kimage *image, void *fdt,
+			    int chosen_node)
+{
+	return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
+			    image->tpm_buffer_addr, image->tpm_buffer_size);
+}
+
+void tpm_add_kexec_buffer(struct kimage *image)
+{
+	struct kexec_buf kbuf = { .image = image, .buf_align = 1,
+				  .buf_min = 0, .buf_max = ULONG_MAX,
+				  .top_down = true };
+	struct device_node *np;
+	void *buffer;
+	u32 size;
+	u64 base;
+	int ret;
+
+	np = of_find_node_by_name(NULL, "vtpm");
+	if (!np)
+		return;
+
+	if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
+		return;
+
+	buffer = vmalloc(size);
+	if (!buffer)
+		return;
+	memcpy(buffer, __va(base), size);
+
+	kbuf.buffer = buffer;
+	kbuf.bufsz = size;
+	kbuf.memsz = size;
+	ret = kexec_add_buffer(&kbuf);
+	if (ret) {
+		pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
+		       ret);
+		return;
+	}
+
+	image->tpm_buffer = buffer;
+	image->tpm_buffer_addr = kbuf.mem;
+	image->tpm_buffer_size = size;
+}
+
+/**
+ * tpm_post_kexec - Make stored TPM log buffer available in of-tree
+ */
+static int __init tpm_post_kexec(void)
+{
+	struct property *newprop;
+	struct device_node *np;
+	void *phyaddr;
+	size_t size;
+	u32 oflogsize;
+	u64 unused;
+	int ret;
+
+	ret = tpm_get_kexec_buffer(&phyaddr, &size);
+	if (ret)
+		return 0;
+
+	/*
+	 * If any one of the following steps fails then the next kexec will
+	 * cause issues due to linux,sml-base pointing to a stale buffer.
+	 */
+	np = of_find_node_by_name(NULL, "vtpm");
+	if (!np)
+		goto err_free_memblock;
+
+	/* logsize must not have changed */
+	if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
+		goto err_free_memblock;
+	if (oflogsize != size)
+		goto err_free_memblock;
+
+	/* replace linux,sml-base with new physical address of buffer */
+	ret = -ENOMEM;
+	newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
+	if (!newprop)
+		goto err_free_memblock;
+
+	newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
+	if (!newprop->name)
+		goto err_free_newprop;
+
+	newprop->length = sizeof(phyaddr);
+
+	newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
+	if (!newprop->value)
+		goto err_free_newprop_struct;
+
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
+	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
+		ret = -ENODEV;
+		goto err_free_newprop_struct;
+	} else {
+		*(u64 *)newprop->value = (u64)phyaddr;
+	}
+
+	ret = of_update_property(np, newprop);
+	if (ret) {
+		pr_err("Could not update linux,sml-base with new address");
+		goto err_free_newprop_struct;
+	}
+
+	goto exit;
+
+err_free_newprop_struct:
+	kfree(newprop->name);
+err_free_newprop:
+	kfree(newprop);
+err_free_memblock:
+	memblock_phys_free((phys_addr_t)phyaddr, size);
+exit:
+	tpm_of_remove_kexec_buffer();
+
+	return ret;
+}
+postcore_initcall(tpm_post_kexec);
+
 /*
  * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
  *
@@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 	remove_ima_buffer(fdt, chosen_node);
 	ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
 
+	remove_tpm_buffer(fdt, chosen_node);
+	ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
+
 out:
 	if (ret) {
 		kvfree(fdt);
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 58d1b58a971e..a0fd7ac0398c 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -319,6 +319,12 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* Virtual address of TPM log buffer for kexec syscall */
+	void *tpm_buffer;
+
+	phys_addr_t tpm_buffer_addr;
+	size_t tpm_buffer_size;
 };
 
 /* kexec interface functions */
diff --git a/include/linux/of.h b/include/linux/of.h
index 04971e85fbc9..922f51c2c072 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -443,6 +443,7 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 				   const char *cmdline, size_t extra_fdt_size);
 int ima_get_kexec_buffer(void **addr, size_t *size);
 int ima_free_kexec_buffer(void);
+void tpm_add_kexec_buffer(struct kimage *image);
 #else /* CONFIG_OF */
 
 static inline void of_core_init(void)
@@ -844,6 +845,10 @@ static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
 	return PHYS_ADDR_MAX;
 }
 
+static inline void tpm_add_kexec_buffer(struct kimage *image)
+{
+}
+
 #define of_match_ptr(_ptr)	NULL
 #define of_match_node(_matches, _node)	NULL
 #endif /* CONFIG_OF */
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 1d7992a02e36..bac04b0b9c48 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -56,6 +56,9 @@ static inline int of_dma_configure(struct device *dev,
 {
 	return of_dma_configure_id(dev, np, force_dma, NULL);
 }
+
+int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size);
+
 #else /* CONFIG_OF */
 
 static inline int of_driver_match_device(struct device *dev,
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..27661eb6112d 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/fs.h>
 #include <linux/ima.h>
+#include <linux/tpm.h>
 #include <crypto/hash.h>
 #include <crypto/sha2.h>
 #include <linux/elf.h>
@@ -171,6 +172,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->ima_buffer = NULL;
 #endif /* CONFIG_IMA_KEXEC */
 
+	vfree(image->tpm_buffer);
+	image->tpm_buffer = NULL;
+
 	/* See if architecture has anything to cleanup post load */
 	arch_kimage_file_post_load_cleanup(image);
 
@@ -277,6 +281,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 	/* IMA needs to pass the measurement list to the next kernel. */
 	ima_add_kexec_buffer(image);
+	/* Pass the TPM measurement log to next kernel */
+	tpm_add_kexec_buffer(image);
 
 	/* Call arch image load handlers */
 	ldata = arch_kexec_kernel_image_load(image);
-- 
2.35.1

[PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

The memory area of the TPM measurement log is currently not properly
duplicated for carrying it across kexec when an Open Firmware
Devicetree is used. Therefore, the contents of the log get corrupted.
Fix this for the kexec_file_load() syscall by allocating a buffer and
copying the contents of the existing log into it. The new buffer is
preserved across the kexec and a pointer to it is available when the new
kernel is started. To achieve this, store the allocated buffer's address
in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
and search for this entry early in the kernel startup before the TPM
subsystem starts up. Adjust the pointer in the of-tree stored under
linux,sml-base to point to this buffer holding the preserved log. The
TPM driver can then read the base address from this entry when making
the log available.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Frank Rowand <frowand.list@gmail.com>
Cc: Eric Biederman <ebiederm@xmission.com>
---
 drivers/of/device.c       |  24 +++++
 drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
 include/linux/kexec.h     |   6 ++
 include/linux/of.h        |   5 +
 include/linux/of_device.h |   3 +
 kernel/kexec_file.c       |   6 ++
 6 files changed, 233 insertions(+), 1 deletion(-)

diff --git a/drivers/of/device.c b/drivers/of/device.c
index 874f031442dc..0cbd47b1cabc 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
+
+int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
+{
+	const u32 *sizep;
+	const u64 *basep;
+
+	sizep = of_get_property(np, "linux,sml-size", NULL);
+	basep = of_get_property(np, "linux,sml-base", NULL);
+	if (sizep == NULL && basep == NULL)
+		return -ENODEV;
+	if (sizep == NULL || basep == NULL)
+		return -EIO;
+
+	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;
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index eef6f3b9939c..db5441123a70 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -14,6 +14,7 @@
 #include <linux/memblock.h>
 #include <linux/libfdt.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/of_fdt.h>
 #include <linux/random.h>
 #include <linux/slab.h>
@@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
 		pr_debug("Removed old IMA buffer reservation.\n");
 }
 
-#ifdef CONFIG_IMA_KEXEC
 static int setup_buffer(void *fdt, int chosen_node, const char *name,
 			uint64_t addr, uint64_t size)
 {
@@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
 
 }
 
+#ifdef CONFIG_IMA_KEXEC
 /**
  * setup_ima_buffer - add IMA buffer information to the fdt
  * @image:		kexec image being loaded.
@@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
 }
 #endif /* CONFIG_IMA_KEXEC */
 
+/**
+ * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
+ * @phyaddr:	On successful return, set to physical address of buffer
+ * @size:	On successful return, set to the buffer size.
+ *
+ * Return: 0 on success, negative errno on error.
+ */
+static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
+{
+	int ret;
+	unsigned long tmp_addr;
+	size_t tmp_size;
+
+	ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);
+	if (ret)
+		return ret;
+
+	*phyaddr = (void *)tmp_addr;
+	*size = tmp_size;
+
+	return 0;
+}
+
+/**
+ * tpm_free_kexec_buffer - free memory used by the IMA buffer
+ */
+static int tpm_of_remove_kexec_buffer(void)
+{
+	struct property *prop;
+
+	prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
+	if (!prop)
+		return -ENOENT;
+
+	return of_remove_property(of_chosen, prop);
+}
+
+/**
+ * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
+ *
+ * @fdt: Flattened Device Tree to update
+ * @chosen_node: Offset to the chosen node in the device tree
+ *
+ * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
+ * remove it from the device tree.
+ */
+static void remove_tpm_buffer(void *fdt, int chosen_node)
+{
+	int ret;
+
+	ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
+	if (!ret)
+		pr_debug("Removed old TPM log buffer reservation.\n");
+}
+
+/**
+ * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
+ * @image:		kexec image being loaded.
+ * @fdt:		Flattened device tree for the next kernel.
+ * @chosen_node:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+static int setup_tpm_buffer(const struct kimage *image, void *fdt,
+			    int chosen_node)
+{
+	return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
+			    image->tpm_buffer_addr, image->tpm_buffer_size);
+}
+
+void tpm_add_kexec_buffer(struct kimage *image)
+{
+	struct kexec_buf kbuf = { .image = image, .buf_align = 1,
+				  .buf_min = 0, .buf_max = ULONG_MAX,
+				  .top_down = true };
+	struct device_node *np;
+	void *buffer;
+	u32 size;
+	u64 base;
+	int ret;
+
+	np = of_find_node_by_name(NULL, "vtpm");
+	if (!np)
+		return;
+
+	if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
+		return;
+
+	buffer = vmalloc(size);
+	if (!buffer)
+		return;
+	memcpy(buffer, __va(base), size);
+
+	kbuf.buffer = buffer;
+	kbuf.bufsz = size;
+	kbuf.memsz = size;
+	ret = kexec_add_buffer(&kbuf);
+	if (ret) {
+		pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
+		       ret);
+		return;
+	}
+
+	image->tpm_buffer = buffer;
+	image->tpm_buffer_addr = kbuf.mem;
+	image->tpm_buffer_size = size;
+}
+
+/**
+ * tpm_post_kexec - Make stored TPM log buffer available in of-tree
+ */
+static int __init tpm_post_kexec(void)
+{
+	struct property *newprop;
+	struct device_node *np;
+	void *phyaddr;
+	size_t size;
+	u32 oflogsize;
+	u64 unused;
+	int ret;
+
+	ret = tpm_get_kexec_buffer(&phyaddr, &size);
+	if (ret)
+		return 0;
+
+	/*
+	 * If any one of the following steps fails then the next kexec will
+	 * cause issues due to linux,sml-base pointing to a stale buffer.
+	 */
+	np = of_find_node_by_name(NULL, "vtpm");
+	if (!np)
+		goto err_free_memblock;
+
+	/* logsize must not have changed */
+	if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
+		goto err_free_memblock;
+	if (oflogsize != size)
+		goto err_free_memblock;
+
+	/* replace linux,sml-base with new physical address of buffer */
+	ret = -ENOMEM;
+	newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
+	if (!newprop)
+		goto err_free_memblock;
+
+	newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
+	if (!newprop->name)
+		goto err_free_newprop;
+
+	newprop->length = sizeof(phyaddr);
+
+	newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
+	if (!newprop->value)
+		goto err_free_newprop_struct;
+
+	if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
+	    of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
+		ret = -ENODEV;
+		goto err_free_newprop_struct;
+	} else {
+		*(u64 *)newprop->value = (u64)phyaddr;
+	}
+
+	ret = of_update_property(np, newprop);
+	if (ret) {
+		pr_err("Could not update linux,sml-base with new address");
+		goto err_free_newprop_struct;
+	}
+
+	goto exit;
+
+err_free_newprop_struct:
+	kfree(newprop->name);
+err_free_newprop:
+	kfree(newprop);
+err_free_memblock:
+	memblock_phys_free((phys_addr_t)phyaddr, size);
+exit:
+	tpm_of_remove_kexec_buffer();
+
+	return ret;
+}
+postcore_initcall(tpm_post_kexec);
+
 /*
  * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
  *
@@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 	remove_ima_buffer(fdt, chosen_node);
 	ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
 
+	remove_tpm_buffer(fdt, chosen_node);
+	ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
+
 out:
 	if (ret) {
 		kvfree(fdt);
diff --git a/include/linux/kexec.h b/include/linux/kexec.h
index 58d1b58a971e..a0fd7ac0398c 100644
--- a/include/linux/kexec.h
+++ b/include/linux/kexec.h
@@ -319,6 +319,12 @@ struct kimage {
 	void *elf_headers;
 	unsigned long elf_headers_sz;
 	unsigned long elf_load_addr;
+
+	/* Virtual address of TPM log buffer for kexec syscall */
+	void *tpm_buffer;
+
+	phys_addr_t tpm_buffer_addr;
+	size_t tpm_buffer_size;
 };
 
 /* kexec interface functions */
diff --git a/include/linux/of.h b/include/linux/of.h
index 04971e85fbc9..922f51c2c072 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -443,6 +443,7 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 				   const char *cmdline, size_t extra_fdt_size);
 int ima_get_kexec_buffer(void **addr, size_t *size);
 int ima_free_kexec_buffer(void);
+void tpm_add_kexec_buffer(struct kimage *image);
 #else /* CONFIG_OF */
 
 static inline void of_core_init(void)
@@ -844,6 +845,10 @@ static inline phys_addr_t of_dma_get_max_cpu_address(struct device_node *np)
 	return PHYS_ADDR_MAX;
 }
 
+static inline void tpm_add_kexec_buffer(struct kimage *image)
+{
+}
+
 #define of_match_ptr(_ptr)	NULL
 #define of_match_node(_matches, _node)	NULL
 #endif /* CONFIG_OF */
diff --git a/include/linux/of_device.h b/include/linux/of_device.h
index 1d7992a02e36..bac04b0b9c48 100644
--- a/include/linux/of_device.h
+++ b/include/linux/of_device.h
@@ -56,6 +56,9 @@ static inline int of_dma_configure(struct device *dev,
 {
 	return of_dma_configure_id(dev, np, force_dma, NULL);
 }
+
+int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size);
+
 #else /* CONFIG_OF */
 
 static inline int of_driver_match_device(struct device *dev,
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 8347fc158d2b..27661eb6112d 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -19,6 +19,7 @@
 #include <linux/list.h>
 #include <linux/fs.h>
 #include <linux/ima.h>
+#include <linux/tpm.h>
 #include <crypto/hash.h>
 #include <crypto/sha2.h>
 #include <linux/elf.h>
@@ -171,6 +172,9 @@ void kimage_file_post_load_cleanup(struct kimage *image)
 	image->ima_buffer = NULL;
 #endif /* CONFIG_IMA_KEXEC */
 
+	vfree(image->tpm_buffer);
+	image->tpm_buffer = NULL;
+
 	/* See if architecture has anything to cleanup post load */
 	arch_kimage_file_post_load_cleanup(image);
 
@@ -277,6 +281,8 @@ kimage_file_prepare_segments(struct kimage *image, int kernel_fd, int initrd_fd,
 
 	/* IMA needs to pass the measurement list to the next kernel. */
 	ima_add_kexec_buffer(image);
+	/* Pass the TPM measurement log to next kernel */
+	tpm_add_kexec_buffer(image);
 
 	/* Call arch image load handlers */
 	ldata = arch_kexec_kernel_image_load(image);
-- 
2.35.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Stefan Berger]

Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
the base and size of the TPM log.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
---
 drivers/char/tpm/eventlog/of.c | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index a9ce66d09a75..e272c7a0765f 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -12,6 +12,7 @@
 
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
@@ -20,11 +21,10 @@
 int tpm_read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
-	const u32 *sizep;
-	const u64 *basep;
 	struct tpm_bios_log *log;
 	u32 size;
 	u64 base;
+	int ret;
 
 	log = &chip->log;
 	if (chip->dev.parent && chip->dev.parent->of_node)
@@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (of_property_read_bool(np, "powered-while-suspended"))
 		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
 
-	sizep = of_get_property(np, "linux,sml-size", NULL);
-	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (sizep == NULL && basep == NULL)
-		return -ENODEV;
-	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;
-	}
+	ret = of_tpm_get_sml_parameters(np, &base, &size);
+	if (ret < 0)
+		return ret;
 
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
-- 
2.35.1

[PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Stefan Berger]

Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
the base and size of the TPM log.

Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
Cc: Jarkko Sakkinen <jarkko@kernel.org>
Cc: Jason Gunthorpe <jgg@ziepe.ca>
---
 drivers/char/tpm/eventlog/of.c | 31 +++++--------------------------
 1 file changed, 5 insertions(+), 26 deletions(-)

diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
index a9ce66d09a75..e272c7a0765f 100644
--- a/drivers/char/tpm/eventlog/of.c
+++ b/drivers/char/tpm/eventlog/of.c
@@ -12,6 +12,7 @@
 
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/tpm_eventlog.h>
 
 #include "../tpm.h"
@@ -20,11 +21,10 @@
 int tpm_read_log_of(struct tpm_chip *chip)
 {
 	struct device_node *np;
-	const u32 *sizep;
-	const u64 *basep;
 	struct tpm_bios_log *log;
 	u32 size;
 	u64 base;
+	int ret;
 
 	log = &chip->log;
 	if (chip->dev.parent && chip->dev.parent->of_node)
@@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
 	if (of_property_read_bool(np, "powered-while-suspended"))
 		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
 
-	sizep = of_get_property(np, "linux,sml-size", NULL);
-	basep = of_get_property(np, "linux,sml-base", NULL);
-	if (sizep == NULL && basep == NULL)
-		return -ENODEV;
-	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;
-	}
+	ret = of_tpm_get_sml_parameters(np, &base, &size);
+	if (ret < 0)
+		return ret;
 
 	if (size == 0) {
 		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
-- 
2.35.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable

[Nageswara R Sastry]

> From: Stefan Berger <stefanb@linux.ibm.com>
> Sent: 14 June 2022 9:42 PM
> To: kexec@lists.infradead.org <kexec@lists.infradead.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>
> Cc: nayna@linux.ibm.com <nayna@linux.ibm.com>; Nageswara R Sastry <nasastry@in.ibm.com>; Stefan Berger <stefanb@linux.ibm.com>; Rob Herring <robh+dt@kernel.org>; Frank Rowand <frowand.list@gmail.com>
> Subject: [PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable 
 
> Refactor IMA buffer related functions to make them reusable for carrying
> TPM logs across kexec.

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

Tested-by: Nageswar R Sastry <rnsastry@linux.ibm.com>

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> ---

Re: [PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable

[Nageswara R Sastry]

> From: Stefan Berger <stefanb@linux.ibm.com>
> Sent: 14 June 2022 9:42 PM
> To: kexec@lists.infradead.org <kexec@lists.infradead.org>; devicetree@vger.kernel.org <devicetree@vger.kernel.org>
> Cc: nayna@linux.ibm.com <nayna@linux.ibm.com>; Nageswara R Sastry <nasastry@in.ibm.com>; Stefan Berger <stefanb@linux.ibm.com>; Rob Herring <robh+dt@kernel.org>; Frank Rowand <frowand.list@gmail.com>
> Subject: [PATCH 1/3] of: kexec: Refactor IMA buffer related functions to make them reusable 
 
> Refactor IMA buffer related functions to make them reusable for carrying
> TPM logs across kexec.

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

Tested-by: Nageswar R Sastry <rnsastry@linux.ibm.com>

> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> ---

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Nageswara R Sastry]

> The memory area of the TPM measurement log is currently not properly
> duplicated for carrying it across kexec when an Open Firmware
> Devicetree is used. Therefore, the contents of the log get corrupted.
> Fix this for the kexec_file_load() syscall by allocating a buffer and
> copying the contents of the existing log into it. The new buffer is
> preserved across the kexec and a pointer to it is available when the new
> kernel is started. To achieve this, store the allocated buffer's address
> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> and search for this entry early in the kernel startup before the TPM
> subsystem starts up. Adjust the pointer in the of-tree stored under
> linux,sml-base to point to this buffer holding the preserved log. The
> TPM driver can then read the base address from this entry when making
> the log available.

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

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
...
> -- 
> 2.35.1

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Nageswara R Sastry]

> The memory area of the TPM measurement log is currently not properly
> duplicated for carrying it across kexec when an Open Firmware
> Devicetree is used. Therefore, the contents of the log get corrupted.
> Fix this for the kexec_file_load() syscall by allocating a buffer and
> copying the contents of the existing log into it. The new buffer is
> preserved across the kexec and a pointer to it is available when the new
> kernel is started. To achieve this, store the allocated buffer's address
> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> and search for this entry early in the kernel startup before the TPM
> subsystem starts up. Adjust the pointer in the of-tree stored under
> linux,sml-base to point to this buffer holding the preserved log. The
> TPM driver can then read the base address from this entry when making
> the log available.

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

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
...
> -- 
> 2.35.1

Re: [PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Nageswara R Sastry]

 
> Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
> the base and size of the TPM log.

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

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
...
> -- 
> 2.35.1

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Nageswara R Sastry]

 
> Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
> the base and size of the TPM log.

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

Tested-by: Nageswara R Sastry <rnsastry@linux.ibm.com>
...
> -- 
> 2.35.1

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

(),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> The memory area of the TPM measurement log is currently not properly
> duplicated for carrying it across kexec when an Open Firmware
> Devicetree is used. Therefore, the contents of the log get corrupted.
> Fix this for the kexec_file_load() syscall by allocating a buffer and
> copying the contents of the existing log into it. The new buffer is
> preserved across the kexec and a pointer to it is available when the new
> kernel is started. To achieve this, store the allocated buffer's address
> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> and search for this entry early in the kernel startup before the TPM
> subsystem starts up. Adjust the pointer in the of-tree stored under
> linux,sml-base to point to this buffer holding the preserved log. The
> TPM driver can then read the base address from this entry when making
> the log available.

This series really needs a wider Cc list of folks that worry about
TPMs and kexec.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> ---
>  drivers/of/device.c       |  24 +++++
>  drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>  include/linux/kexec.h     |   6 ++
>  include/linux/of.h        |   5 +
>  include/linux/of_device.h |   3 +
>  kernel/kexec_file.c       |   6 ++
>  6 files changed, 233 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 874f031442dc..0cbd47b1cabc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
> +
> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)

of/device.c is for functions that work on a struct device. This is not
the case here.

> +{
> +       const u32 *sizep;
> +       const u64 *basep;
> +
> +       sizep = of_get_property(np, "linux,sml-size", NULL);
> +       basep = of_get_property(np, "linux,sml-base", NULL);

Any new properties need a schema. For chosen, that's located here[1].
The more common pattern for similar properties is <base size>.

Don't use of_get_property(), but the typed functions instead.

> +       if (sizep == NULL && basep == NULL)
> +               return -ENODEV;
> +       if (sizep == NULL || basep == NULL)
> +               return -EIO;

Do we really need the error distinction?

> +
> +       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;

What? Some platforms aren't properly encoded? Fix the firmware.

> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index eef6f3b9939c..db5441123a70 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -14,6 +14,7 @@
>  #include <linux/memblock.h>
>  #include <linux/libfdt.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_fdt.h>
>  #include <linux/random.h>
>  #include <linux/slab.h>
> @@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
>                 pr_debug("Removed old IMA buffer reservation.\n");
>  }
>
> -#ifdef CONFIG_IMA_KEXEC
>  static int setup_buffer(void *fdt, int chosen_node, const char *name,
>                         uint64_t addr, uint64_t size)
>  {
> @@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
>
>  }
>
> +#ifdef CONFIG_IMA_KEXEC
>  /**
>   * setup_ima_buffer - add IMA buffer information to the fdt
>   * @image:             kexec image being loaded.
> @@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>  }
>  #endif /* CONFIG_IMA_KEXEC */
>
> +/**
> + * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
> + * @phyaddr:   On successful return, set to physical address of buffer
> + * @size:      On successful return, set to the buffer size.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
> +{
> +       int ret;
> +       unsigned long tmp_addr;
> +       size_t tmp_size;
> +
> +       ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);

Again, must be documented.

> +       if (ret)
> +               return ret;
> +
> +       *phyaddr = (void *)tmp_addr;
> +       *size = tmp_size;
> +
> +       return 0;
> +}
> +
> +/**
> + * tpm_free_kexec_buffer - free memory used by the IMA buffer
> + */
> +static int tpm_of_remove_kexec_buffer(void)
> +{
> +       struct property *prop;
> +
> +       prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
> +       if (!prop)
> +               return -ENOENT;
> +
> +       return of_remove_property(of_chosen, prop);
> +}
> +
> +/**
> + * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
> + *
> + * @fdt: Flattened Device Tree to update
> + * @chosen_node: Offset to the chosen node in the device tree
> + *
> + * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
> + * remove it from the device tree.
> + */
> +static void remove_tpm_buffer(void *fdt, int chosen_node)
> +{
> +       int ret;
> +
> +       ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
> +       if (!ret)
> +               pr_debug("Removed old TPM log buffer reservation.\n");

Do we really need this print? If so, perhaps in remove_buffer() rather
than having every caller do it.

> +}
> +
> +/**
> + * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
> + * @image:             kexec image being loaded.
> + * @fdt:               Flattened device tree for the next kernel.
> + * @chosen_node:       Offset to the chosen node.
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +static int setup_tpm_buffer(const struct kimage *image, void *fdt,
> +                           int chosen_node)
> +{
> +       return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
> +                           image->tpm_buffer_addr, image->tpm_buffer_size);
> +}
> +
> +void tpm_add_kexec_buffer(struct kimage *image)
> +{
> +       struct kexec_buf kbuf = { .image = image, .buf_align = 1,
> +                                 .buf_min = 0, .buf_max = ULONG_MAX,
> +                                 .top_down = true };
> +       struct device_node *np;
> +       void *buffer;
> +       u32 size;
> +       u64 base;
> +       int ret;
> +
> +       np = of_find_node_by_name(NULL, "vtpm");
> +       if (!np)
> +               return;
> +
> +       if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
> +               return;
> +
> +       buffer = vmalloc(size);
> +       if (!buffer)
> +               return;
> +       memcpy(buffer, __va(base), size);
> +
> +       kbuf.buffer = buffer;
> +       kbuf.bufsz = size;
> +       kbuf.memsz = size;
> +       ret = kexec_add_buffer(&kbuf);
> +       if (ret) {
> +               pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
> +                      ret);
> +               return;
> +       }
> +
> +       image->tpm_buffer = buffer;
> +       image->tpm_buffer_addr = kbuf.mem;
> +       image->tpm_buffer_size = size;
> +}
> +
> +/**
> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
> + */
> +static int __init tpm_post_kexec(void)
> +{
> +       struct property *newprop;
> +       struct device_node *np;
> +       void *phyaddr;
> +       size_t size;
> +       u32 oflogsize;
> +       u64 unused;
> +       int ret;
> +
> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
> +       if (ret)
> +               return 0;
> +
> +       /*
> +        * If any one of the following steps fails then the next kexec will
> +        * cause issues due to linux,sml-base pointing to a stale buffer.
> +        */
> +       np = of_find_node_by_name(NULL, "vtpm");

This seems pretty IBM specific.

> +       if (!np)
> +               goto err_free_memblock;
> +
> +       /* logsize must not have changed */
> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
> +               goto err_free_memblock;
> +       if (oflogsize != size)
> +               goto err_free_memblock;
> +
> +       /* replace linux,sml-base with new physical address of buffer */
> +       ret = -ENOMEM;
> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> +       if (!newprop)
> +               goto err_free_memblock;
> +
> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
> +       if (!newprop->name)
> +               goto err_free_newprop;
> +
> +       newprop->length = sizeof(phyaddr);
> +
> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
> +       if (!newprop->value)
> +               goto err_free_newprop_struct;
> +
> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> +               ret = -ENODEV;
> +               goto err_free_newprop_struct;
> +       } else {
> +               *(u64 *)newprop->value = (u64)phyaddr;
> +       }
> +
> +       ret = of_update_property(np, newprop);

Just FYI for now, there's some work happening on a better API for
writing nodes and properties.

> +       if (ret) {
> +               pr_err("Could not update linux,sml-base with new address");
> +               goto err_free_newprop_struct;
> +       }
> +
> +       goto exit;
> +
> +err_free_newprop_struct:
> +       kfree(newprop->name);
> +err_free_newprop:
> +       kfree(newprop);
> +err_free_memblock:
> +       memblock_phys_free((phys_addr_t)phyaddr, size);
> +exit:
> +       tpm_of_remove_kexec_buffer();
> +
> +       return ret;
> +}
> +postcore_initcall(tpm_post_kexec);

Would be better if this is called explicitly at the right time when
needed rather than using an initcall.

> +
>  /*
>   * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
>   *
> @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>         remove_ima_buffer(fdt, chosen_node);
>         ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>
> +       remove_tpm_buffer(fdt, chosen_node);
> +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> +
>  out:
>         if (ret) {
>                 kvfree(fdt);
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 58d1b58a971e..a0fd7ac0398c 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -319,6 +319,12 @@ struct kimage {
>         void *elf_headers;
>         unsigned long elf_headers_sz;
>         unsigned long elf_load_addr;
> +
> +       /* Virtual address of TPM log buffer for kexec syscall */
> +       void *tpm_buffer;
> +
> +       phys_addr_t tpm_buffer_addr;
> +       size_t tpm_buffer_size;

Probably should use the same types as other buffers...

>  };

Rob

[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

(),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>
> The memory area of the TPM measurement log is currently not properly
> duplicated for carrying it across kexec when an Open Firmware
> Devicetree is used. Therefore, the contents of the log get corrupted.
> Fix this for the kexec_file_load() syscall by allocating a buffer and
> copying the contents of the existing log into it. The new buffer is
> preserved across the kexec and a pointer to it is available when the new
> kernel is started. To achieve this, store the allocated buffer's address
> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> and search for this entry early in the kernel startup before the TPM
> subsystem starts up. Adjust the pointer in the of-tree stored under
> linux,sml-base to point to this buffer holding the preserved log. The
> TPM driver can then read the base address from this entry when making
> the log available.

This series really needs a wider Cc list of folks that worry about
TPMs and kexec.

> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> ---
>  drivers/of/device.c       |  24 +++++
>  drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>  include/linux/kexec.h     |   6 ++
>  include/linux/of.h        |   5 +
>  include/linux/of_device.h |   3 +
>  kernel/kexec_file.c       |   6 ++
>  6 files changed, 233 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index 874f031442dc..0cbd47b1cabc 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
> +
> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)

of/device.c is for functions that work on a struct device. This is not
the case here.

> +{
> +       const u32 *sizep;
> +       const u64 *basep;
> +
> +       sizep = of_get_property(np, "linux,sml-size", NULL);
> +       basep = of_get_property(np, "linux,sml-base", NULL);

Any new properties need a schema. For chosen, that's located here[1].
The more common pattern for similar properties is <base size>.

Don't use of_get_property(), but the typed functions instead.

> +       if (sizep == NULL && basep == NULL)
> +               return -ENODEV;
> +       if (sizep == NULL || basep == NULL)
> +               return -EIO;

Do we really need the error distinction?

> +
> +       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;

What? Some platforms aren't properly encoded? Fix the firmware.

> +       }
> +       return 0;
> +}
> +EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index eef6f3b9939c..db5441123a70 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -14,6 +14,7 @@
>  #include <linux/memblock.h>
>  #include <linux/libfdt.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/of_fdt.h>
>  #include <linux/random.h>
>  #include <linux/slab.h>
> @@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
>                 pr_debug("Removed old IMA buffer reservation.\n");
>  }
>
> -#ifdef CONFIG_IMA_KEXEC
>  static int setup_buffer(void *fdt, int chosen_node, const char *name,
>                         uint64_t addr, uint64_t size)
>  {
> @@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
>
>  }
>
> +#ifdef CONFIG_IMA_KEXEC
>  /**
>   * setup_ima_buffer - add IMA buffer information to the fdt
>   * @image:             kexec image being loaded.
> @@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>  }
>  #endif /* CONFIG_IMA_KEXEC */
>
> +/**
> + * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
> + * @phyaddr:   On successful return, set to physical address of buffer
> + * @size:      On successful return, set to the buffer size.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
> +{
> +       int ret;
> +       unsigned long tmp_addr;
> +       size_t tmp_size;
> +
> +       ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);

Again, must be documented.

> +       if (ret)
> +               return ret;
> +
> +       *phyaddr = (void *)tmp_addr;
> +       *size = tmp_size;
> +
> +       return 0;
> +}
> +
> +/**
> + * tpm_free_kexec_buffer - free memory used by the IMA buffer
> + */
> +static int tpm_of_remove_kexec_buffer(void)
> +{
> +       struct property *prop;
> +
> +       prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
> +       if (!prop)
> +               return -ENOENT;
> +
> +       return of_remove_property(of_chosen, prop);
> +}
> +
> +/**
> + * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
> + *
> + * @fdt: Flattened Device Tree to update
> + * @chosen_node: Offset to the chosen node in the device tree
> + *
> + * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
> + * remove it from the device tree.
> + */
> +static void remove_tpm_buffer(void *fdt, int chosen_node)
> +{
> +       int ret;
> +
> +       ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
> +       if (!ret)
> +               pr_debug("Removed old TPM log buffer reservation.\n");

Do we really need this print? If so, perhaps in remove_buffer() rather
than having every caller do it.

> +}
> +
> +/**
> + * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
> + * @image:             kexec image being loaded.
> + * @fdt:               Flattened device tree for the next kernel.
> + * @chosen_node:       Offset to the chosen node.
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +static int setup_tpm_buffer(const struct kimage *image, void *fdt,
> +                           int chosen_node)
> +{
> +       return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
> +                           image->tpm_buffer_addr, image->tpm_buffer_size);
> +}
> +
> +void tpm_add_kexec_buffer(struct kimage *image)
> +{
> +       struct kexec_buf kbuf = { .image = image, .buf_align = 1,
> +                                 .buf_min = 0, .buf_max = ULONG_MAX,
> +                                 .top_down = true };
> +       struct device_node *np;
> +       void *buffer;
> +       u32 size;
> +       u64 base;
> +       int ret;
> +
> +       np = of_find_node_by_name(NULL, "vtpm");
> +       if (!np)
> +               return;
> +
> +       if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
> +               return;
> +
> +       buffer = vmalloc(size);
> +       if (!buffer)
> +               return;
> +       memcpy(buffer, __va(base), size);
> +
> +       kbuf.buffer = buffer;
> +       kbuf.bufsz = size;
> +       kbuf.memsz = size;
> +       ret = kexec_add_buffer(&kbuf);
> +       if (ret) {
> +               pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
> +                      ret);
> +               return;
> +       }
> +
> +       image->tpm_buffer = buffer;
> +       image->tpm_buffer_addr = kbuf.mem;
> +       image->tpm_buffer_size = size;
> +}
> +
> +/**
> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
> + */
> +static int __init tpm_post_kexec(void)
> +{
> +       struct property *newprop;
> +       struct device_node *np;
> +       void *phyaddr;
> +       size_t size;
> +       u32 oflogsize;
> +       u64 unused;
> +       int ret;
> +
> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
> +       if (ret)
> +               return 0;
> +
> +       /*
> +        * If any one of the following steps fails then the next kexec will
> +        * cause issues due to linux,sml-base pointing to a stale buffer.
> +        */
> +       np = of_find_node_by_name(NULL, "vtpm");

This seems pretty IBM specific.

> +       if (!np)
> +               goto err_free_memblock;
> +
> +       /* logsize must not have changed */
> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
> +               goto err_free_memblock;
> +       if (oflogsize != size)
> +               goto err_free_memblock;
> +
> +       /* replace linux,sml-base with new physical address of buffer */
> +       ret = -ENOMEM;
> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> +       if (!newprop)
> +               goto err_free_memblock;
> +
> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
> +       if (!newprop->name)
> +               goto err_free_newprop;
> +
> +       newprop->length = sizeof(phyaddr);
> +
> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
> +       if (!newprop->value)
> +               goto err_free_newprop_struct;
> +
> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> +               ret = -ENODEV;
> +               goto err_free_newprop_struct;
> +       } else {
> +               *(u64 *)newprop->value = (u64)phyaddr;
> +       }
> +
> +       ret = of_update_property(np, newprop);

Just FYI for now, there's some work happening on a better API for
writing nodes and properties.

> +       if (ret) {
> +               pr_err("Could not update linux,sml-base with new address");
> +               goto err_free_newprop_struct;
> +       }
> +
> +       goto exit;
> +
> +err_free_newprop_struct:
> +       kfree(newprop->name);
> +err_free_newprop:
> +       kfree(newprop);
> +err_free_memblock:
> +       memblock_phys_free((phys_addr_t)phyaddr, size);
> +exit:
> +       tpm_of_remove_kexec_buffer();
> +
> +       return ret;
> +}
> +postcore_initcall(tpm_post_kexec);

Would be better if this is called explicitly at the right time when
needed rather than using an initcall.

> +
>  /*
>   * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
>   *
> @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>         remove_ima_buffer(fdt, chosen_node);
>         ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>
> +       remove_tpm_buffer(fdt, chosen_node);
> +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> +
>  out:
>         if (ret) {
>                 kvfree(fdt);
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 58d1b58a971e..a0fd7ac0398c 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -319,6 +319,12 @@ struct kimage {
>         void *elf_headers;
>         unsigned long elf_headers_sz;
>         unsigned long elf_load_addr;
> +
> +       /* Virtual address of TPM log buffer for kexec syscall */
> +       void *tpm_buffer;
> +
> +       phys_addr_t tpm_buffer_addr;
> +       size_t tpm_buffer_size;

Probably should use the same types as other buffers...

>  };

Rob

[1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linus/master v5.19-rc2 next-20220614]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220615/202206150235.zvfG6DIM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/input/mouse/synaptics.c:30:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   drivers/input/mouse/synaptics.c:164:27: warning: 'smbus_pnp_ids' defined but not used [-Wunused-const-variable=]
     164 | static const char * const smbus_pnp_ids[] = {
         |                           ^~~~~~~~~~~~~
--
   In file included from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from init/main.c:30:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   init/main.c:769:20: warning: no previous prototype for 'arch_post_acpi_subsys_init' [-Wmissing-prototypes]
     769 | void __init __weak arch_post_acpi_subsys_init(void) { }
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
   init/main.c:781:20: warning: no previous prototype for 'mem_encrypt_init' [-Wmissing-prototypes]
     781 | void __init __weak mem_encrypt_init(void) { }
         |                    ^~~~~~~~~~~~~~~~
   init/main.c:783:20: warning: no previous prototype for 'poking_init' [-Wmissing-prototypes]
     783 | void __init __weak poking_init(void) { }
         |                    ^~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from arch/um/kernel/process.c:24:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   arch/um/kernel/process.c:50:5: warning: no previous prototype for 'pid_to_processor_id' [-Wmissing-prototypes]
      50 | int pid_to_processor_id(int pid)
         |     ^~~~~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:86:7: warning: no previous prototype for '__switch_to' [-Wmissing-prototypes]
      86 | void *__switch_to(struct task_struct *from, struct task_struct *to)
         |       ^~~~~~~~~~~
   arch/um/kernel/process.c: In function 'new_thread_handler':
   arch/um/kernel/process.c:121:28: warning: variable 'n' set but not used [-Wunused-but-set-variable]
     121 |         int (*fn)(void *), n;
         |                            ^
   arch/um/kernel/process.c: At top level:
   arch/um/kernel/process.c:139:6: warning: no previous prototype for 'fork_handler' [-Wmissing-prototypes]
     139 | void fork_handler(void)
         |      ^~~~~~~~~~~~
   arch/um/kernel/process.c:216:6: warning: no previous prototype for 'arch_cpu_idle' [-Wmissing-prototypes]
     216 | void arch_cpu_idle(void)
         |      ^~~~~~~~~~~~~
   arch/um/kernel/process.c:253:5: warning: no previous prototype for 'copy_to_user_proc' [-Wmissing-prototypes]
     253 | int copy_to_user_proc(void __user *to, void *from, int size)
         |     ^~~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:263:5: warning: no previous prototype for 'clear_user_proc' [-Wmissing-prototypes]
     263 | int clear_user_proc(void __user *buf, int size)
         |     ^~~~~~~~~~~~~~~
   arch/um/kernel/process.c:316:12: warning: no previous prototype for 'make_proc_sysemu' [-Wmissing-prototypes]
     316 | int __init make_proc_sysemu(void)
         |            ^~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:356:15: warning: no previous prototype for 'arch_align_stack' [-Wmissing-prototypes]
     356 | unsigned long arch_align_stack(unsigned long sp)
         |               ^~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from arch/um/kernel/time.c:10:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   arch/um/kernel/time.c:778:13: warning: no previous prototype for 'time_init' [-Wmissing-prototypes]
     778 | void __init time_init(void)
         |             ^~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from include/linux/sched/isolation.h:6,
                    from kernel/workqueue.c:51:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from kernel/irq_work.c:17:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/irq_work.c:70:13: warning: no previous prototype for 'arch_irq_work_raise' [-Wmissing-prototypes]
      70 | void __weak arch_irq_work_raise(void)
         |             ^~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from include/linux/sched/isolation.h:6,
                    from kernel/sched/fair.c:38:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/sched/fair.c:675:5: warning: no previous prototype for 'sched_update_scaling' [-Wmissing-prototypes]
     675 | int sched_update_scaling(void)
         |     ^~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from kernel/time/hrtimer.c:32:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/time/hrtimer.c:120:35: warning: initialized field overwritten [-Woverride-init]
     120 |         [CLOCK_REALTIME]        = HRTIMER_BASE_REALTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:120:35: note: (near initialization for 'hrtimer_clock_to_base_table[0]')
   kernel/time/hrtimer.c:121:35: warning: initialized field overwritten [-Woverride-init]
     121 |         [CLOCK_MONOTONIC]       = HRTIMER_BASE_MONOTONIC,
         |                                   ^~~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:121:35: note: (near initialization for 'hrtimer_clock_to_base_table[1]')
   kernel/time/hrtimer.c:122:35: warning: initialized field overwritten [-Woverride-init]
     122 |         [CLOCK_BOOTTIME]        = HRTIMER_BASE_BOOTTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:122:35: note: (near initialization for 'hrtimer_clock_to_base_table[7]')
   kernel/time/hrtimer.c:123:35: warning: initialized field overwritten [-Woverride-init]
     123 |         [CLOCK_TAI]             = HRTIMER_BASE_TAI,
         |                                   ^~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:123:35: note: (near initialization for 'hrtimer_clock_to_base_table[11]')
   kernel/time/hrtimer.c: In function '__run_hrtimer':
   kernel/time/hrtimer.c:1648:14: warning: variable 'expires_in_hardirq' set but not used [-Wunused-but-set-variable]
    1648 |         bool expires_in_hardirq;
         |              ^~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clk-provider.h:9,
                    from lib/vsprintf.c:23:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   lib/vsprintf.c: In function 'va_format':
   lib/vsprintf.c:1681:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1681 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
         |         ^~~


vim +848 include/linux/of.h

   847	
 > 848	static inline void tpm_add_kexec_buffer(struct kimage *image)
   849	{
   850	}
   851	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linus/master v5.19-rc2 next-20220614]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: um-i386_defconfig (https://download.01.org/0day-ci/archive/20220615/202206150235.zvfG6DIM-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=um SUBARCH=i386 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from include/linux/i2c.h:13,
                    from drivers/input/mouse/synaptics.c:30:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   drivers/input/mouse/synaptics.c:164:27: warning: 'smbus_pnp_ids' defined but not used [-Wunused-const-variable=]
     164 | static const char * const smbus_pnp_ids[] = {
         |                           ^~~~~~~~~~~~~
--
   In file included from include/linux/irqdomain.h:35,
                    from include/linux/acpi.h:13,
                    from init/main.c:30:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   init/main.c:769:20: warning: no previous prototype for 'arch_post_acpi_subsys_init' [-Wmissing-prototypes]
     769 | void __init __weak arch_post_acpi_subsys_init(void) { }
         |                    ^~~~~~~~~~~~~~~~~~~~~~~~~~
   init/main.c:781:20: warning: no previous prototype for 'mem_encrypt_init' [-Wmissing-prototypes]
     781 | void __init __weak mem_encrypt_init(void) { }
         |                    ^~~~~~~~~~~~~~~~
   init/main.c:783:20: warning: no previous prototype for 'poking_init' [-Wmissing-prototypes]
     783 | void __init __weak poking_init(void) { }
         |                    ^~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from arch/um/kernel/process.c:24:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   arch/um/kernel/process.c:50:5: warning: no previous prototype for 'pid_to_processor_id' [-Wmissing-prototypes]
      50 | int pid_to_processor_id(int pid)
         |     ^~~~~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:86:7: warning: no previous prototype for '__switch_to' [-Wmissing-prototypes]
      86 | void *__switch_to(struct task_struct *from, struct task_struct *to)
         |       ^~~~~~~~~~~
   arch/um/kernel/process.c: In function 'new_thread_handler':
   arch/um/kernel/process.c:121:28: warning: variable 'n' set but not used [-Wunused-but-set-variable]
     121 |         int (*fn)(void *), n;
         |                            ^
   arch/um/kernel/process.c: At top level:
   arch/um/kernel/process.c:139:6: warning: no previous prototype for 'fork_handler' [-Wmissing-prototypes]
     139 | void fork_handler(void)
         |      ^~~~~~~~~~~~
   arch/um/kernel/process.c:216:6: warning: no previous prototype for 'arch_cpu_idle' [-Wmissing-prototypes]
     216 | void arch_cpu_idle(void)
         |      ^~~~~~~~~~~~~
   arch/um/kernel/process.c:253:5: warning: no previous prototype for 'copy_to_user_proc' [-Wmissing-prototypes]
     253 | int copy_to_user_proc(void __user *to, void *from, int size)
         |     ^~~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:263:5: warning: no previous prototype for 'clear_user_proc' [-Wmissing-prototypes]
     263 | int clear_user_proc(void __user *buf, int size)
         |     ^~~~~~~~~~~~~~~
   arch/um/kernel/process.c:316:12: warning: no previous prototype for 'make_proc_sysemu' [-Wmissing-prototypes]
     316 | int __init make_proc_sysemu(void)
         |            ^~~~~~~~~~~~~~~~
   arch/um/kernel/process.c:356:15: warning: no previous prototype for 'arch_align_stack' [-Wmissing-prototypes]
     356 | unsigned long arch_align_stack(unsigned long sp)
         |               ^~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from arch/um/kernel/time.c:10:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   arch/um/kernel/time.c:778:13: warning: no previous prototype for 'time_init' [-Wmissing-prototypes]
     778 | void __init time_init(void)
         |             ^~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from include/linux/sched/isolation.h:6,
                    from kernel/workqueue.c:51:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from kernel/irq_work.c:17:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/irq_work.c:70:13: warning: no previous prototype for 'arch_irq_work_raise' [-Wmissing-prototypes]
      70 | void __weak arch_irq_work_raise(void)
         |             ^~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from include/linux/sched/isolation.h:6,
                    from kernel/sched/fair.c:38:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/sched/fair.c:675:5: warning: no previous prototype for 'sched_update_scaling' [-Wmissing-prototypes]
     675 | int sched_update_scaling(void)
         |     ^~~~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clocksource.h:19,
                    from include/linux/clockchips.h:14,
                    from include/linux/tick.h:8,
                    from kernel/time/hrtimer.c:32:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   kernel/time/hrtimer.c:120:35: warning: initialized field overwritten [-Woverride-init]
     120 |         [CLOCK_REALTIME]        = HRTIMER_BASE_REALTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:120:35: note: (near initialization for 'hrtimer_clock_to_base_table[0]')
   kernel/time/hrtimer.c:121:35: warning: initialized field overwritten [-Woverride-init]
     121 |         [CLOCK_MONOTONIC]       = HRTIMER_BASE_MONOTONIC,
         |                                   ^~~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:121:35: note: (near initialization for 'hrtimer_clock_to_base_table[1]')
   kernel/time/hrtimer.c:122:35: warning: initialized field overwritten [-Woverride-init]
     122 |         [CLOCK_BOOTTIME]        = HRTIMER_BASE_BOOTTIME,
         |                                   ^~~~~~~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:122:35: note: (near initialization for 'hrtimer_clock_to_base_table[7]')
   kernel/time/hrtimer.c:123:35: warning: initialized field overwritten [-Woverride-init]
     123 |         [CLOCK_TAI]             = HRTIMER_BASE_TAI,
         |                                   ^~~~~~~~~~~~~~~~
   kernel/time/hrtimer.c:123:35: note: (near initialization for 'hrtimer_clock_to_base_table[11]')
   kernel/time/hrtimer.c: In function '__run_hrtimer':
   kernel/time/hrtimer.c:1648:14: warning: variable 'expires_in_hardirq' set but not used [-Wunused-but-set-variable]
    1648 |         bool expires_in_hardirq;
         |              ^~~~~~~~~~~~~~~~~~
--
   In file included from include/linux/clk-provider.h:9,
                    from lib/vsprintf.c:23:
>> include/linux/of.h:848:48: warning: 'struct kimage' declared inside parameter list will not be visible outside of this definition or declaration
     848 | static inline void tpm_add_kexec_buffer(struct kimage *image)
         |                                                ^~~~~~
   lib/vsprintf.c: In function 'va_format':
   lib/vsprintf.c:1681:9: warning: function 'va_format' might be a candidate for 'gnu_printf' format attribute [-Wsuggest-attribute=format]
    1681 |         buf += vsnprintf(buf, end > buf ? end - buf : 0, va_fmt->fmt, va);
         |         ^~~


vim +848 include/linux/of.h

   847	
 > 848	static inline void tpm_add_kexec_buffer(struct kimage *image)
   849	{
   850	}
   851	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linus/master v5.19-rc2 next-20220614]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: x86_64-randconfig-r022-20220613 (https://download.01.org/0day-ci/archive/20220615/202206150856.h2psJLGs-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/of/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/of/kexec.c:306: warning: expecting prototype for tpm_free_kexec_buffer(). Prototype was for tpm_of_remove_kexec_buffer() instead


vim +306 drivers/of/kexec.c

   301	
   302	/**
   303	 * tpm_free_kexec_buffer - free memory used by the IMA buffer
   304	 */
   305	static int tpm_of_remove_kexec_buffer(void)
 > 306	{
   307		struct property *prop;
   308	
   309		prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
   310		if (!prop)
   311			return -ENOENT;
   312	
   313		return of_remove_property(of_chosen, prop);
   314	}
   315	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on linus/master v5.19-rc2 next-20220614]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: x86_64-randconfig-r022-20220613 (https://download.01.org/0day-ci/archive/20220615/202206150856.h2psJLGs-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/of/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/of/kexec.c:306: warning: expecting prototype for tpm_free_kexec_buffer(). Prototype was for tpm_of_remove_kexec_buffer() instead


vim +306 drivers/of/kexec.c

   301	
   302	/**
   303	 * tpm_free_kexec_buffer - free memory used by the IMA buffer
   304	 */
   305	static int tpm_of_remove_kexec_buffer(void)
 > 306	{
   307		struct property *prop;
   308	
   309		prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
   310		if (!prop)
   311			return -ENOENT;
   312	
   313		return of_remove_property(of_chosen, prop);
   314	}
   315	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on linus/master v5.19-rc2 next-20220615]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: x86_64-randconfig-m001-20220613 (https://download.01.org/0day-ci/archive/20220615/202206151737.t3TxlR7I-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: kernel/kexec_file.o: in function `kimage_file_alloc_init':
>> kexec_file.c:(.text+0x880): undefined reference to `tpm_add_kexec_buffer'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[kernel test robot]

Hi Stefan,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on char-misc/char-misc-testing]
[also build test ERROR on linus/master v5.19-rc2 next-20220615]
[cannot apply to robh/for-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 0a35780c755ccec097d15c6b4ff8b246a89f1689
config: x86_64-randconfig-m001-20220613 (https://download.01.org/0day-ci/archive/20220615/202206151737.t3TxlR7I-lkp@intel.com/config)
compiler: gcc-11 (Debian 11.3.0-3) 11.3.0
reproduce (this is a W=1 build):
        # https://github.com/intel-lab-lkp/linux/commit/c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Stefan-Berger/tpm-Preserve-TPM-measurement-log-across-kexec/20220615-001510
        git checkout c42b5be1ad82e8a991a3d35417c9e495e7eb4c93
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        make W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   ld: kernel/kexec_file.o: in function `kimage_file_alloc_init':
>> kexec_file.c:(.text+0x880): undefined reference to `tpm_add_kexec_buffer'

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

On 6/14/22 13:48, Rob Herring wrote:
> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> The memory area of the TPM measurement log is currently not properly
>> duplicated for carrying it across kexec when an Open Firmware
>> Devicetree is used. Therefore, the contents of the log get corrupted.
>> Fix this for the kexec_file_load() syscall by allocating a buffer and
>> copying the contents of the existing log into it. The new buffer is
>> preserved across the kexec and a pointer to it is available when the new
>> kernel is started. To achieve this, store the allocated buffer's address
>> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
>> and search for this entry early in the kernel startup before the TPM
>> subsystem starts up. Adjust the pointer in the of-tree stored under
>> linux,sml-base to point to this buffer holding the preserved log. The
>> TPM driver can then read the base address from this entry when making
>> the log available.
> 
> This series really needs a wider Cc list of folks that worry about
> TPMs and kexec.
> 
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: Eric Biederman <ebiederm@xmission.com>
>> ---
>>   drivers/of/device.c       |  24 +++++
>>   drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>>   include/linux/kexec.h     |   6 ++
>>   include/linux/of.h        |   5 +
>>   include/linux/of_device.h |   3 +
>>   kernel/kexec_file.c       |   6 ++
>>   6 files changed, 233 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 874f031442dc..0cbd47b1cabc 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>          return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>> +
>> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
> 
> of/device.c is for functions that work on a struct device. This is not
> the case here.

Can I put it into platform.c?

I should have probably mentioned it but this function here is a copy of 
code from tpm_read_log_of() from here: 
https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38

3/3 refactors the code in tpm/eventlog/of.c to make use of this new 
function then to avoid code duplication. Therefore, this code here is 
more general than necessary at this point. Maybe I should do the move in 
a patch of its own?


> 
>> +{
>> +       const u32 *sizep;
>> +       const u64 *basep;
>> +
>> +       sizep = of_get_property(np, "linux,sml-size", NULL);
>> +       basep = of_get_property(np, "linux,sml-base", NULL);
> 
> Any new properties need a schema. For chosen, that's located here[1].
> The more common pattern for similar properties is <base size>.
> 
> Don't use of_get_property(), but the typed functions instead.

I think this was done due to the little endian and big endian 
distinction below.


> 
>> +       if (sizep == NULL && basep == NULL)
>> +               return -ENODEV;
>> +       if (sizep == NULL || basep == NULL)
>> +               return -EIO;
> 
> Do we really need the error distinction?

As I mentioned, this code is a copy from the TPM subsystem. There it 
does make a distinction because similar functions for acpi and efi make 
a distinction as well although this particular function's return code 
doesn't matter in the end.

The code I am referring to is this here:

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85

> 
>> +
>> +       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;
> 
> What? Some platforms aren't properly encoded? Fix the firmware.

It's been like this for years...

> 
>> +       }
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index eef6f3b9939c..db5441123a70 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/memblock.h>
>>   #include <linux/libfdt.h>
>>   #include <linux/of.h>
>> +#include <linux/of_device.h>
>>   #include <linux/of_fdt.h>
>>   #include <linux/random.h>
>>   #include <linux/slab.h>
>> @@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
>>                  pr_debug("Removed old IMA buffer reservation.\n");
>>   }
>>
>> -#ifdef CONFIG_IMA_KEXEC
>>   static int setup_buffer(void *fdt, int chosen_node, const char *name,
>>                          uint64_t addr, uint64_t size)
>>   {
>> @@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
>>
>>   }
>>
>> +#ifdef CONFIG_IMA_KEXEC
>>   /**
>>    * setup_ima_buffer - add IMA buffer information to the fdt
>>    * @image:             kexec image being loaded.
>> @@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>>   }
>>   #endif /* CONFIG_IMA_KEXEC */
>>
>> +/**
>> + * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
>> + * @phyaddr:   On successful return, set to physical address of buffer
>> + * @size:      On successful return, set to the buffer size.
>> + *
>> + * Return: 0 on success, negative errno on error.
>> + */
>> +static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
>> +{
>> +       int ret;
>> +       unsigned long tmp_addr;
>> +       size_t tmp_size;
>> +
>> +       ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);
> 
> Again, must be documented.

Ok, I will send a PR to that repo once this is acceptable here.

> 
>> +       if (ret)
>> +               return ret;
>> +
>> +       *phyaddr = (void *)tmp_addr;
>> +       *size = tmp_size;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * tpm_free_kexec_buffer - free memory used by the IMA buffer
>> + */
>> +static int tpm_of_remove_kexec_buffer(void)
>> +{
>> +       struct property *prop;
>> +
>> +       prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
>> +       if (!prop)
>> +               return -ENOENT;
>> +
>> +       return of_remove_property(of_chosen, prop);
>> +}
>> +
>> +/**
>> + * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
>> + *
>> + * @fdt: Flattened Device Tree to update
>> + * @chosen_node: Offset to the chosen node in the device tree
>> + *
>> + * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
>> + * remove it from the device tree.
>> + */
>> +static void remove_tpm_buffer(void *fdt, int chosen_node)
>> +{
>> +       int ret;
>> +
>> +       ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
>> +       if (!ret)
>> +               pr_debug("Removed old TPM log buffer reservation.\n");
> 
> Do we really need this print? If so, perhaps in remove_buffer() rather
> than having every caller do it.

Ok. Let me adjust 1/3 then as well. There I preserved the pr_debug but I 
will push it into the function and not print 'Removed old IMA log buffer 
reservation.' but instead 'Removed old linux,ima-kexec-buffer reservation.'

> 
>> +}
>> +
>> +/**
>> + * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
>> + * @image:             kexec image being loaded.
>> + * @fdt:               Flattened device tree for the next kernel.
>> + * @chosen_node:       Offset to the chosen node.
>> + *
>> + * Return: 0 on success, or negative errno on error.
>> + */
>> +static int setup_tpm_buffer(const struct kimage *image, void *fdt,
>> +                           int chosen_node)
>> +{
>> +       return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
>> +                           image->tpm_buffer_addr, image->tpm_buffer_size);
>> +}
>> +
>> +void tpm_add_kexec_buffer(struct kimage *image)
>> +{
>> +       struct kexec_buf kbuf = { .image = image, .buf_align = 1,
>> +                                 .buf_min = 0, .buf_max = ULONG_MAX,
>> +                                 .top_down = true };
>> +       struct device_node *np;
>> +       void *buffer;
>> +       u32 size;
>> +       u64 base;
>> +       int ret;
>> +
>> +       np = of_find_node_by_name(NULL, "vtpm");
>> +       if (!np)
>> +               return;
>> +
>> +       if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
>> +               return;
>> +
>> +       buffer = vmalloc(size);
>> +       if (!buffer)
>> +               return;
>> +       memcpy(buffer, __va(base), size);
>> +
>> +       kbuf.buffer = buffer;
>> +       kbuf.bufsz = size;
>> +       kbuf.memsz = size;
>> +       ret = kexec_add_buffer(&kbuf);
>> +       if (ret) {
>> +               pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
>> +                      ret);
>> +               return;
>> +       }
>> +
>> +       image->tpm_buffer = buffer;
>> +       image->tpm_buffer_addr = kbuf.mem;
>> +       image->tpm_buffer_size = size;
>> +}
>> +
>> +/**
>> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
>> + */
>> +static int __init tpm_post_kexec(void)
>> +{
>> +       struct property *newprop;
>> +       struct device_node *np;
>> +       void *phyaddr;
>> +       size_t size;
>> +       u32 oflogsize;
>> +       u64 unused;
>> +       int ret;
>> +
>> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
>> +       if (ret)
>> +               return 0;
>> +
>> +       /*
>> +        * If any one of the following steps fails then the next kexec will
>> +        * cause issues due to linux,sml-base pointing to a stale buffer.
>> +        */
>> +       np = of_find_node_by_name(NULL, "vtpm");
> 
> This seems pretty IBM specific.

Yes, it is and I am not sure what to do about it. Should I cover parts 
of the function with a #ifdef CONFIG_PPC_PSERIES ?

> 
>> +       if (!np)
>> +               goto err_free_memblock;
>> +
>> +       /* logsize must not have changed */
>> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
>> +               goto err_free_memblock;
>> +       if (oflogsize != size)
>> +               goto err_free_memblock;
>> +
>> +       /* replace linux,sml-base with new physical address of buffer */
>> +       ret = -ENOMEM;
>> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>> +       if (!newprop)
>> +               goto err_free_memblock;
>> +
>> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
>> +       if (!newprop->name)
>> +               goto err_free_newprop;
>> +
>> +       newprop->length = sizeof(phyaddr);
>> +
>> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
>> +       if (!newprop->value)
>> +               goto err_free_newprop_struct;
>> +
>> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>> +               ret = -ENODEV;
>> +               goto err_free_newprop_struct;
>> +       } else {
>> +               *(u64 *)newprop->value = (u64)phyaddr;
>> +       }
>> +
>> +       ret = of_update_property(np, newprop);
> 
> Just FYI for now, there's some work happening on a better API for
> writing nodes and properties.

Ok.

> 
>> +       if (ret) {
>> +               pr_err("Could not update linux,sml-base with new address");
>> +               goto err_free_newprop_struct;
>> +       }
>> +
>> +       goto exit;
>> +
>> +err_free_newprop_struct:
>> +       kfree(newprop->name);
>> +err_free_newprop:
>> +       kfree(newprop);
>> +err_free_memblock:
>> +       memblock_phys_free((phys_addr_t)phyaddr, size);
>> +exit:
>> +       tpm_of_remove_kexec_buffer();
>> +
>> +       return ret;
>> +}
>> +postcore_initcall(tpm_post_kexec);
> 
> Would be better if this is called explicitly at the right time when
> needed rather than using an initcall.

The 'when needed' would be the TPM subsystem. However, if I was to make 
it dependent on the TPM subsystem we would loose the TPM log if we were 
not to kexec into a kernel with TPM subsystem or the TPM driver wasn't 
activated. I wanted to be able to preserve the log even if a kexec'ed 
kernel didn't support or activate the TPM driver and then a subsequent 
one again has it activated...

> 
>> +
>>   /*
>>    * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
>>    *
>> @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>          remove_ima_buffer(fdt, chosen_node);
>>          ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>>
>> +       remove_tpm_buffer(fdt, chosen_node);
>> +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>> +
>>   out:
>>          if (ret) {
>>                  kvfree(fdt);
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 58d1b58a971e..a0fd7ac0398c 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -319,6 +319,12 @@ struct kimage {
>>          void *elf_headers;
>>          unsigned long elf_headers_sz;
>>          unsigned long elf_load_addr;
>> +
>> +       /* Virtual address of TPM log buffer for kexec syscall */
>> +       void *tpm_buffer;
>> +
>> +       phys_addr_t tpm_buffer_addr;
>> +       size_t tpm_buffer_size;
> 
> Probably should use the same types as other buffers...

It did so following existing support for IMA: 
https://elixir.bootlin.com/linux/latest/source/include/linux/kexec.h

#ifdef CONFIG_IMA_KEXEC
	/* Virtual address of IMA measurement buffer for kexec syscall */
	void *ima_buffer;

	phys_addr_t ima_buffer_addr;
	size_t ima_buffer_size;
#endif

> 
>>   };
> 
> Rob
> 
> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

On 6/14/22 13:48, Rob Herring wrote:
> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>
>> The memory area of the TPM measurement log is currently not properly
>> duplicated for carrying it across kexec when an Open Firmware
>> Devicetree is used. Therefore, the contents of the log get corrupted.
>> Fix this for the kexec_file_load() syscall by allocating a buffer and
>> copying the contents of the existing log into it. The new buffer is
>> preserved across the kexec and a pointer to it is available when the new
>> kernel is started. To achieve this, store the allocated buffer's address
>> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
>> and search for this entry early in the kernel startup before the TPM
>> subsystem starts up. Adjust the pointer in the of-tree stored under
>> linux,sml-base to point to this buffer holding the preserved log. The
>> TPM driver can then read the base address from this entry when making
>> the log available.
> 
> This series really needs a wider Cc list of folks that worry about
> TPMs and kexec.
> 
>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Frank Rowand <frowand.list@gmail.com>
>> Cc: Eric Biederman <ebiederm@xmission.com>
>> ---
>>   drivers/of/device.c       |  24 +++++
>>   drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>>   include/linux/kexec.h     |   6 ++
>>   include/linux/of.h        |   5 +
>>   include/linux/of_device.h |   3 +
>>   kernel/kexec_file.c       |   6 ++
>>   6 files changed, 233 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>> index 874f031442dc..0cbd47b1cabc 100644
>> --- a/drivers/of/device.c
>> +++ b/drivers/of/device.c
>> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>          return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>> +
>> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
> 
> of/device.c is for functions that work on a struct device. This is not
> the case here.

Can I put it into platform.c?

I should have probably mentioned it but this function here is a copy of 
code from tpm_read_log_of() from here: 
https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38

3/3 refactors the code in tpm/eventlog/of.c to make use of this new 
function then to avoid code duplication. Therefore, this code here is 
more general than necessary at this point. Maybe I should do the move in 
a patch of its own?


> 
>> +{
>> +       const u32 *sizep;
>> +       const u64 *basep;
>> +
>> +       sizep = of_get_property(np, "linux,sml-size", NULL);
>> +       basep = of_get_property(np, "linux,sml-base", NULL);
> 
> Any new properties need a schema. For chosen, that's located here[1].
> The more common pattern for similar properties is <base size>.
> 
> Don't use of_get_property(), but the typed functions instead.

I think this was done due to the little endian and big endian 
distinction below.


> 
>> +       if (sizep == NULL && basep == NULL)
>> +               return -ENODEV;
>> +       if (sizep == NULL || basep == NULL)
>> +               return -EIO;
> 
> Do we really need the error distinction?

As I mentioned, this code is a copy from the TPM subsystem. There it 
does make a distinction because similar functions for acpi and efi make 
a distinction as well although this particular function's return code 
doesn't matter in the end.

The code I am referring to is this here:

https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85

> 
>> +
>> +       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;
> 
> What? Some platforms aren't properly encoded? Fix the firmware.

It's been like this for years...

> 
>> +       }
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(of_tpm_get_sml_parameters);
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index eef6f3b9939c..db5441123a70 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -14,6 +14,7 @@
>>   #include <linux/memblock.h>
>>   #include <linux/libfdt.h>
>>   #include <linux/of.h>
>> +#include <linux/of_device.h>
>>   #include <linux/of_fdt.h>
>>   #include <linux/random.h>
>>   #include <linux/slab.h>
>> @@ -221,7 +222,6 @@ static void remove_ima_buffer(void *fdt, int chosen_node)
>>                  pr_debug("Removed old IMA buffer reservation.\n");
>>   }
>>
>> -#ifdef CONFIG_IMA_KEXEC
>>   static int setup_buffer(void *fdt, int chosen_node, const char *name,
>>                          uint64_t addr, uint64_t size)
>>   {
>> @@ -243,6 +243,7 @@ static int setup_buffer(void *fdt, int chosen_node, const char *name,
>>
>>   }
>>
>> +#ifdef CONFIG_IMA_KEXEC
>>   /**
>>    * setup_ima_buffer - add IMA buffer information to the fdt
>>    * @image:             kexec image being loaded.
>> @@ -275,6 +276,190 @@ static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>>   }
>>   #endif /* CONFIG_IMA_KEXEC */
>>
>> +/**
>> + * tpm_get_kexec_buffer - get TPM log buffer from the previous kernel
>> + * @phyaddr:   On successful return, set to physical address of buffer
>> + * @size:      On successful return, set to the buffer size.
>> + *
>> + * Return: 0 on success, negative errno on error.
>> + */
>> +static int tpm_get_kexec_buffer(void **phyaddr, size_t *size)
>> +{
>> +       int ret;
>> +       unsigned long tmp_addr;
>> +       size_t tmp_size;
>> +
>> +       ret = get_kexec_buffer("linux,tpm-kexec-buffer", &tmp_addr, &tmp_size);
> 
> Again, must be documented.

Ok, I will send a PR to that repo once this is acceptable here.

> 
>> +       if (ret)
>> +               return ret;
>> +
>> +       *phyaddr = (void *)tmp_addr;
>> +       *size = tmp_size;
>> +
>> +       return 0;
>> +}
>> +
>> +/**
>> + * tpm_free_kexec_buffer - free memory used by the IMA buffer
>> + */
>> +static int tpm_of_remove_kexec_buffer(void)
>> +{
>> +       struct property *prop;
>> +
>> +       prop = of_find_property(of_chosen, "linux,tpm-kexec-buffer", NULL);
>> +       if (!prop)
>> +               return -ENOENT;
>> +
>> +       return of_remove_property(of_chosen, prop);
>> +}
>> +
>> +/**
>> + * remove_tpm_buffer - remove the TPM log buffer property and reservation from @fdt
>> + *
>> + * @fdt: Flattened Device Tree to update
>> + * @chosen_node: Offset to the chosen node in the device tree
>> + *
>> + * The TPM log measurement buffer is of no use to a subsequent kernel, so we always
>> + * remove it from the device tree.
>> + */
>> +static void remove_tpm_buffer(void *fdt, int chosen_node)
>> +{
>> +       int ret;
>> +
>> +       ret = remove_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer");
>> +       if (!ret)
>> +               pr_debug("Removed old TPM log buffer reservation.\n");
> 
> Do we really need this print? If so, perhaps in remove_buffer() rather
> than having every caller do it.

Ok. Let me adjust 1/3 then as well. There I preserved the pr_debug but I 
will push it into the function and not print 'Removed old IMA log buffer 
reservation.' but instead 'Removed old linux,ima-kexec-buffer reservation.'

> 
>> +}
>> +
>> +/**
>> + * setup_tpm_buffer - add TPM measurement log buffer information to the fdt
>> + * @image:             kexec image being loaded.
>> + * @fdt:               Flattened device tree for the next kernel.
>> + * @chosen_node:       Offset to the chosen node.
>> + *
>> + * Return: 0 on success, or negative errno on error.
>> + */
>> +static int setup_tpm_buffer(const struct kimage *image, void *fdt,
>> +                           int chosen_node)
>> +{
>> +       return setup_buffer(fdt, chosen_node, "linux,tpm-kexec-buffer",
>> +                           image->tpm_buffer_addr, image->tpm_buffer_size);
>> +}
>> +
>> +void tpm_add_kexec_buffer(struct kimage *image)
>> +{
>> +       struct kexec_buf kbuf = { .image = image, .buf_align = 1,
>> +                                 .buf_min = 0, .buf_max = ULONG_MAX,
>> +                                 .top_down = true };
>> +       struct device_node *np;
>> +       void *buffer;
>> +       u32 size;
>> +       u64 base;
>> +       int ret;
>> +
>> +       np = of_find_node_by_name(NULL, "vtpm");
>> +       if (!np)
>> +               return;
>> +
>> +       if (of_tpm_get_sml_parameters(np, &base, &size) < 0)
>> +               return;
>> +
>> +       buffer = vmalloc(size);
>> +       if (!buffer)
>> +               return;
>> +       memcpy(buffer, __va(base), size);
>> +
>> +       kbuf.buffer = buffer;
>> +       kbuf.bufsz = size;
>> +       kbuf.memsz = size;
>> +       ret = kexec_add_buffer(&kbuf);
>> +       if (ret) {
>> +               pr_err("Error passing over kexec TPM measurement log buffer: %d\n",
>> +                      ret);
>> +               return;
>> +       }
>> +
>> +       image->tpm_buffer = buffer;
>> +       image->tpm_buffer_addr = kbuf.mem;
>> +       image->tpm_buffer_size = size;
>> +}
>> +
>> +/**
>> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
>> + */
>> +static int __init tpm_post_kexec(void)
>> +{
>> +       struct property *newprop;
>> +       struct device_node *np;
>> +       void *phyaddr;
>> +       size_t size;
>> +       u32 oflogsize;
>> +       u64 unused;
>> +       int ret;
>> +
>> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
>> +       if (ret)
>> +               return 0;
>> +
>> +       /*
>> +        * If any one of the following steps fails then the next kexec will
>> +        * cause issues due to linux,sml-base pointing to a stale buffer.
>> +        */
>> +       np = of_find_node_by_name(NULL, "vtpm");
> 
> This seems pretty IBM specific.

Yes, it is and I am not sure what to do about it. Should I cover parts 
of the function with a #ifdef CONFIG_PPC_PSERIES ?

> 
>> +       if (!np)
>> +               goto err_free_memblock;
>> +
>> +       /* logsize must not have changed */
>> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
>> +               goto err_free_memblock;
>> +       if (oflogsize != size)
>> +               goto err_free_memblock;
>> +
>> +       /* replace linux,sml-base with new physical address of buffer */
>> +       ret = -ENOMEM;
>> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>> +       if (!newprop)
>> +               goto err_free_memblock;
>> +
>> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
>> +       if (!newprop->name)
>> +               goto err_free_newprop;
>> +
>> +       newprop->length = sizeof(phyaddr);
>> +
>> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
>> +       if (!newprop->value)
>> +               goto err_free_newprop_struct;
>> +
>> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>> +               ret = -ENODEV;
>> +               goto err_free_newprop_struct;
>> +       } else {
>> +               *(u64 *)newprop->value = (u64)phyaddr;
>> +       }
>> +
>> +       ret = of_update_property(np, newprop);
> 
> Just FYI for now, there's some work happening on a better API for
> writing nodes and properties.

Ok.

> 
>> +       if (ret) {
>> +               pr_err("Could not update linux,sml-base with new address");
>> +               goto err_free_newprop_struct;
>> +       }
>> +
>> +       goto exit;
>> +
>> +err_free_newprop_struct:
>> +       kfree(newprop->name);
>> +err_free_newprop:
>> +       kfree(newprop);
>> +err_free_memblock:
>> +       memblock_phys_free((phys_addr_t)phyaddr, size);
>> +exit:
>> +       tpm_of_remove_kexec_buffer();
>> +
>> +       return ret;
>> +}
>> +postcore_initcall(tpm_post_kexec);
> 
> Would be better if this is called explicitly at the right time when
> needed rather than using an initcall.

The 'when needed' would be the TPM subsystem. However, if I was to make 
it dependent on the TPM subsystem we would loose the TPM log if we were 
not to kexec into a kernel with TPM subsystem or the TPM driver wasn't 
activated. I wanted to be able to preserve the log even if a kexec'ed 
kernel didn't support or activate the TPM driver and then a subsequent 
one again has it activated...

> 
>> +
>>   /*
>>    * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
>>    *
>> @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>          remove_ima_buffer(fdt, chosen_node);
>>          ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>>
>> +       remove_tpm_buffer(fdt, chosen_node);
>> +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
>> +
>>   out:
>>          if (ret) {
>>                  kvfree(fdt);
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 58d1b58a971e..a0fd7ac0398c 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -319,6 +319,12 @@ struct kimage {
>>          void *elf_headers;
>>          unsigned long elf_headers_sz;
>>          unsigned long elf_load_addr;
>> +
>> +       /* Virtual address of TPM log buffer for kexec syscall */
>> +       void *tpm_buffer;
>> +
>> +       phys_addr_t tpm_buffer_addr;
>> +       size_t tpm_buffer_size;
> 
> Probably should use the same types as other buffers...

It did so following existing support for IMA: 
https://elixir.bootlin.com/linux/latest/source/include/linux/kexec.h

#ifdef CONFIG_IMA_KEXEC
	/* Virtual address of IMA measurement buffer for kexec syscall */
	void *ima_buffer;

	phys_addr_t ima_buffer_addr;
	size_t ima_buffer_size;
#endif

> 
>>   };
> 
> Rob
> 
> [1] https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/chosen.yaml

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

On Wed, Jun 15, 2022 at 09:08:04AM -0400, Stefan Berger wrote:
> 
> 
> On 6/14/22 13:48, Rob Herring wrote:
> > (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > 
> > > The memory area of the TPM measurement log is currently not properly
> > > duplicated for carrying it across kexec when an Open Firmware
> > > Devicetree is used. Therefore, the contents of the log get corrupted.
> > > Fix this for the kexec_file_load() syscall by allocating a buffer and
> > > copying the contents of the existing log into it. The new buffer is
> > > preserved across the kexec and a pointer to it is available when the new
> > > kernel is started. To achieve this, store the allocated buffer's address
> > > in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> > > and search for this entry early in the kernel startup before the TPM
> > > subsystem starts up. Adjust the pointer in the of-tree stored under
> > > linux,sml-base to point to this buffer holding the preserved log. The
> > > TPM driver can then read the base address from this entry when making
> > > the log available.
> > 
> > This series really needs a wider Cc list of folks that worry about
> > TPMs and kexec.
> > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: Eric Biederman <ebiederm@xmission.com>
> > > ---
> > >   drivers/of/device.c       |  24 +++++
> > >   drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
> > >   include/linux/kexec.h     |   6 ++
> > >   include/linux/of.h        |   5 +
> > >   include/linux/of_device.h |   3 +
> > >   kernel/kexec_file.c       |   6 ++
> > >   6 files changed, 233 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > index 874f031442dc..0cbd47b1cabc 100644
> > > --- a/drivers/of/device.c
> > > +++ b/drivers/of/device.c
> > > @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> > >          return 0;
> > >   }
> > >   EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
> > > +
> > > +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
> > 
> > of/device.c is for functions that work on a struct device. This is not
> > the case here.
> 
> Can I put it into platform.c?

That's for struct platform_device things.

> I should have probably mentioned it but this function here is a copy of code
> from tpm_read_log_of() from here: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38
> 
> 3/3 refactors the code in tpm/eventlog/of.c to make use of this new function
> then to avoid code duplication. Therefore, this code here is more general
> than necessary at this point. Maybe I should do the move in a patch of its
> own?

Maybe you should leave that function there and call it?

Generally, subsystem specific things go into the subsystems. However, 
there's a few special cases like kexec now. That was added primarily to 
avoid per arch duplication.

I've never looked at the TPM code, so sorry, I don't have more specific 
suggestions.

> > > +{
> > > +       const u32 *sizep;
> > > +       const u64 *basep;
> > > +
> > > +       sizep = of_get_property(np, "linux,sml-size", NULL);
> > > +       basep = of_get_property(np, "linux,sml-base", NULL);
> > 
> > Any new properties need a schema. For chosen, that's located here[1].
> > The more common pattern for similar properties is <base size>.
> > 
> > Don't use of_get_property(), but the typed functions instead.
> 
> I think this was done due to the little endian and big endian distinction
> below.


Right.

> > > +       if (sizep == NULL && basep == NULL)
> > > +               return -ENODEV;
> > > +       if (sizep == NULL || basep == NULL)
> > > +               return -EIO;
> > 
> > Do we really need the error distinction?
> 
> As I mentioned, this code is a copy from the TPM subsystem. There it does
> make a distinction because similar functions for acpi and efi make a
> distinction as well although this particular function's return code doesn't
> matter in the end.
> 
> The code I am referring to is this here:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85
> 
> > 
> > > +
> > > +       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;
> > 
> > What? Some platforms aren't properly encoded? Fix the firmware.
> 
> It's been like this for years...

Great! :(

I'm confused how IBM needs this? Only a LE machine with LE DT encoding 
would need this. With Power being historically BE and only recently 
(though I guess that's a few years now) supporting LE, how did the DT 
encoding become LE for this yet not for everything else in DT?

[...]


> > > +/**
> > > + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
> > > + */
> > > +static int __init tpm_post_kexec(void)
> > > +{
> > > +       struct property *newprop;
> > > +       struct device_node *np;
> > > +       void *phyaddr;
> > > +       size_t size;
> > > +       u32 oflogsize;
> > > +       u64 unused;
> > > +       int ret;
> > > +
> > > +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
> > > +       if (ret)
> > > +               return 0;
> > > +
> > > +       /*
> > > +        * If any one of the following steps fails then the next kexec will
> > > +        * cause issues due to linux,sml-base pointing to a stale buffer.
> > > +        */
> > > +       np = of_find_node_by_name(NULL, "vtpm");
> > 
> > This seems pretty IBM specific.
> 
> Yes, it is and I am not sure what to do about it. Should I cover parts of
> the function with a #ifdef CONFIG_PPC_PSERIES ?

#ifdef's aren't great. IS_ENABLED() is a bit better, but really put 
implementation specific things in implementation specific code.

Perhaps each TPM implementation needs its own hook to do stuff?

> > > +       if (!np)
> > > +               goto err_free_memblock;
> > > +
> > > +       /* logsize must not have changed */
> > > +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
> > > +               goto err_free_memblock;
> > > +       if (oflogsize != size)
> > > +               goto err_free_memblock;
> > > +
> > > +       /* replace linux,sml-base with new physical address of buffer */
> > > +       ret = -ENOMEM;
> > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > +       if (!newprop)
> > > +               goto err_free_memblock;
> > > +
> > > +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
> > > +       if (!newprop->name)
> > > +               goto err_free_newprop;
> > > +
> > > +       newprop->length = sizeof(phyaddr);
> > > +
> > > +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
> > > +       if (!newprop->value)
> > > +               goto err_free_newprop_struct;
> > > +
> > > +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> > > +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> > > +               ret = -ENODEV;
> > > +               goto err_free_newprop_struct;
> > > +       } else {
> > > +               *(u64 *)newprop->value = (u64)phyaddr;
> > > +       }
> > > +
> > > +       ret = of_update_property(np, newprop);
> > 
> > Just FYI for now, there's some work happening on a better API for
> > writing nodes and properties.
> 
> Ok.
> 
> > 
> > > +       if (ret) {
> > > +               pr_err("Could not update linux,sml-base with new address");
> > > +               goto err_free_newprop_struct;
> > > +       }
> > > +
> > > +       goto exit;
> > > +
> > > +err_free_newprop_struct:
> > > +       kfree(newprop->name);
> > > +err_free_newprop:
> > > +       kfree(newprop);
> > > +err_free_memblock:
> > > +       memblock_phys_free((phys_addr_t)phyaddr, size);
> > > +exit:
> > > +       tpm_of_remove_kexec_buffer();
> > > +
> > > +       return ret;
> > > +}
> > > +postcore_initcall(tpm_post_kexec);
> > 
> > Would be better if this is called explicitly at the right time when
> > needed rather than using an initcall.
> 
> The 'when needed' would be the TPM subsystem. However, if I was to make it
> dependent on the TPM subsystem we would loose the TPM log if we were not to
> kexec into a kernel with TPM subsystem or the TPM driver wasn't activated. I
> wanted to be able to preserve the log even if a kexec'ed kernel didn't
> support or activate the TPM driver and then a subsequent one again has it
> activated...

Sounds like a TPM problem the TPM code should deal with. Or a scenario 
that just shouldn't be supported. IDK

> > > +
> > >   /*
> > >    * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
> > >    *
> > > @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> > >          remove_ima_buffer(fdt, chosen_node);
> > >          ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> > > 
> > > +       remove_tpm_buffer(fdt, chosen_node);
> > > +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> > > +
> > >   out:
> > >          if (ret) {
> > >                  kvfree(fdt);
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 58d1b58a971e..a0fd7ac0398c 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -319,6 +319,12 @@ struct kimage {
> > >          void *elf_headers;
> > >          unsigned long elf_headers_sz;
> > >          unsigned long elf_load_addr;
> > > +
> > > +       /* Virtual address of TPM log buffer for kexec syscall */
> > > +       void *tpm_buffer;
> > > +
> > > +       phys_addr_t tpm_buffer_addr;
> > > +       size_t tpm_buffer_size;
> > 
> > Probably should use the same types as other buffers...
> 
> It did so following existing support for IMA:
> https://elixir.bootlin.com/linux/latest/source/include/linux/kexec.h
> 
> #ifdef CONFIG_IMA_KEXEC
> 	/* Virtual address of IMA measurement buffer for kexec syscall */
> 	void *ima_buffer;
> 
> 	phys_addr_t ima_buffer_addr;
> 	size_t ima_buffer_size;
> #endif

Ah, nevermind then.

Rob

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

On Wed, Jun 15, 2022 at 09:08:04AM -0400, Stefan Berger wrote:
> 
> 
> On 6/14/22 13:48, Rob Herring wrote:
> > (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> > > 
> > > The memory area of the TPM measurement log is currently not properly
> > > duplicated for carrying it across kexec when an Open Firmware
> > > Devicetree is used. Therefore, the contents of the log get corrupted.
> > > Fix this for the kexec_file_load() syscall by allocating a buffer and
> > > copying the contents of the existing log into it. The new buffer is
> > > preserved across the kexec and a pointer to it is available when the new
> > > kernel is started. To achieve this, store the allocated buffer's address
> > > in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> > > and search for this entry early in the kernel startup before the TPM
> > > subsystem starts up. Adjust the pointer in the of-tree stored under
> > > linux,sml-base to point to this buffer holding the preserved log. The
> > > TPM driver can then read the base address from this entry when making
> > > the log available.
> > 
> > This series really needs a wider Cc list of folks that worry about
> > TPMs and kexec.
> > 
> > > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Frank Rowand <frowand.list@gmail.com>
> > > Cc: Eric Biederman <ebiederm@xmission.com>
> > > ---
> > >   drivers/of/device.c       |  24 +++++
> > >   drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
> > >   include/linux/kexec.h     |   6 ++
> > >   include/linux/of.h        |   5 +
> > >   include/linux/of_device.h |   3 +
> > >   kernel/kexec_file.c       |   6 ++
> > >   6 files changed, 233 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/of/device.c b/drivers/of/device.c
> > > index 874f031442dc..0cbd47b1cabc 100644
> > > --- a/drivers/of/device.c
> > > +++ b/drivers/of/device.c
> > > @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
> > >          return 0;
> > >   }
> > >   EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
> > > +
> > > +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
> > 
> > of/device.c is for functions that work on a struct device. This is not
> > the case here.
> 
> Can I put it into platform.c?

That's for struct platform_device things.

> I should have probably mentioned it but this function here is a copy of code
> from tpm_read_log_of() from here: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38
> 
> 3/3 refactors the code in tpm/eventlog/of.c to make use of this new function
> then to avoid code duplication. Therefore, this code here is more general
> than necessary at this point. Maybe I should do the move in a patch of its
> own?

Maybe you should leave that function there and call it?

Generally, subsystem specific things go into the subsystems. However, 
there's a few special cases like kexec now. That was added primarily to 
avoid per arch duplication.

I've never looked at the TPM code, so sorry, I don't have more specific 
suggestions.

> > > +{
> > > +       const u32 *sizep;
> > > +       const u64 *basep;
> > > +
> > > +       sizep = of_get_property(np, "linux,sml-size", NULL);
> > > +       basep = of_get_property(np, "linux,sml-base", NULL);
> > 
> > Any new properties need a schema. For chosen, that's located here[1].
> > The more common pattern for similar properties is <base size>.
> > 
> > Don't use of_get_property(), but the typed functions instead.
> 
> I think this was done due to the little endian and big endian distinction
> below.


Right.

> > > +       if (sizep == NULL && basep == NULL)
> > > +               return -ENODEV;
> > > +       if (sizep == NULL || basep == NULL)
> > > +               return -EIO;
> > 
> > Do we really need the error distinction?
> 
> As I mentioned, this code is a copy from the TPM subsystem. There it does
> make a distinction because similar functions for acpi and efi make a
> distinction as well although this particular function's return code doesn't
> matter in the end.
> 
> The code I am referring to is this here:
> 
> https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85
> 
> > 
> > > +
> > > +       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;
> > 
> > What? Some platforms aren't properly encoded? Fix the firmware.
> 
> It's been like this for years...

Great! :(

I'm confused how IBM needs this? Only a LE machine with LE DT encoding 
would need this. With Power being historically BE and only recently 
(though I guess that's a few years now) supporting LE, how did the DT 
encoding become LE for this yet not for everything else in DT?

[...]


> > > +/**
> > > + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
> > > + */
> > > +static int __init tpm_post_kexec(void)
> > > +{
> > > +       struct property *newprop;
> > > +       struct device_node *np;
> > > +       void *phyaddr;
> > > +       size_t size;
> > > +       u32 oflogsize;
> > > +       u64 unused;
> > > +       int ret;
> > > +
> > > +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
> > > +       if (ret)
> > > +               return 0;
> > > +
> > > +       /*
> > > +        * If any one of the following steps fails then the next kexec will
> > > +        * cause issues due to linux,sml-base pointing to a stale buffer.
> > > +        */
> > > +       np = of_find_node_by_name(NULL, "vtpm");
> > 
> > This seems pretty IBM specific.
> 
> Yes, it is and I am not sure what to do about it. Should I cover parts of
> the function with a #ifdef CONFIG_PPC_PSERIES ?

#ifdef's aren't great. IS_ENABLED() is a bit better, but really put 
implementation specific things in implementation specific code.

Perhaps each TPM implementation needs its own hook to do stuff?

> > > +       if (!np)
> > > +               goto err_free_memblock;
> > > +
> > > +       /* logsize must not have changed */
> > > +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
> > > +               goto err_free_memblock;
> > > +       if (oflogsize != size)
> > > +               goto err_free_memblock;
> > > +
> > > +       /* replace linux,sml-base with new physical address of buffer */
> > > +       ret = -ENOMEM;
> > > +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
> > > +       if (!newprop)
> > > +               goto err_free_memblock;
> > > +
> > > +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
> > > +       if (!newprop->name)
> > > +               goto err_free_newprop;
> > > +
> > > +       newprop->length = sizeof(phyaddr);
> > > +
> > > +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
> > > +       if (!newprop->value)
> > > +               goto err_free_newprop_struct;
> > > +
> > > +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
> > > +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
> > > +               ret = -ENODEV;
> > > +               goto err_free_newprop_struct;
> > > +       } else {
> > > +               *(u64 *)newprop->value = (u64)phyaddr;
> > > +       }
> > > +
> > > +       ret = of_update_property(np, newprop);
> > 
> > Just FYI for now, there's some work happening on a better API for
> > writing nodes and properties.
> 
> Ok.
> 
> > 
> > > +       if (ret) {
> > > +               pr_err("Could not update linux,sml-base with new address");
> > > +               goto err_free_newprop_struct;
> > > +       }
> > > +
> > > +       goto exit;
> > > +
> > > +err_free_newprop_struct:
> > > +       kfree(newprop->name);
> > > +err_free_newprop:
> > > +       kfree(newprop);
> > > +err_free_memblock:
> > > +       memblock_phys_free((phys_addr_t)phyaddr, size);
> > > +exit:
> > > +       tpm_of_remove_kexec_buffer();
> > > +
> > > +       return ret;
> > > +}
> > > +postcore_initcall(tpm_post_kexec);
> > 
> > Would be better if this is called explicitly at the right time when
> > needed rather than using an initcall.
> 
> The 'when needed' would be the TPM subsystem. However, if I was to make it
> dependent on the TPM subsystem we would loose the TPM log if we were not to
> kexec into a kernel with TPM subsystem or the TPM driver wasn't activated. I
> wanted to be able to preserve the log even if a kexec'ed kernel didn't
> support or activate the TPM driver and then a subsequent one again has it
> activated...

Sounds like a TPM problem the TPM code should deal with. Or a scenario 
that just shouldn't be supported. IDK

> > > +
> > >   /*
> > >    * of_kexec_alloc_and_setup_fdt - Alloc and setup a new Flattened Device Tree
> > >    *
> > > @@ -464,6 +649,9 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
> > >          remove_ima_buffer(fdt, chosen_node);
> > >          ret = setup_ima_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> > > 
> > > +       remove_tpm_buffer(fdt, chosen_node);
> > > +       ret = setup_tpm_buffer(image, fdt, fdt_path_offset(fdt, "/chosen"));
> > > +
> > >   out:
> > >          if (ret) {
> > >                  kvfree(fdt);
> > > diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> > > index 58d1b58a971e..a0fd7ac0398c 100644
> > > --- a/include/linux/kexec.h
> > > +++ b/include/linux/kexec.h
> > > @@ -319,6 +319,12 @@ struct kimage {
> > >          void *elf_headers;
> > >          unsigned long elf_headers_sz;
> > >          unsigned long elf_load_addr;
> > > +
> > > +       /* Virtual address of TPM log buffer for kexec syscall */
> > > +       void *tpm_buffer;
> > > +
> > > +       phys_addr_t tpm_buffer_addr;
> > > +       size_t tpm_buffer_size;
> > 
> > Probably should use the same types as other buffers...
> 
> It did so following existing support for IMA:
> https://elixir.bootlin.com/linux/latest/source/include/linux/kexec.h
> 
> #ifdef CONFIG_IMA_KEXEC
> 	/* Virtual address of IMA measurement buffer for kexec syscall */
> 	void *ima_buffer;
> 
> 	phys_addr_t ima_buffer_addr;
> 	size_t ima_buffer_size;
> #endif

Ah, nevermind then.

Rob

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

On Tue, Jun 14, 2022 at 11:48:30AM -0600, Rob Herring wrote:
> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> > The memory area of the TPM measurement log is currently not properly
> > duplicated for carrying it across kexec when an Open Firmware
> > Devicetree is used. Therefore, the contents of the log get corrupted.
> > Fix this for the kexec_file_load() syscall by allocating a buffer and
> > copying the contents of the existing log into it. The new buffer is
> > preserved across the kexec and a pointer to it is available when the new
> > kernel is started. To achieve this, store the allocated buffer's address
> > in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> > and search for this entry early in the kernel startup before the TPM
> > subsystem starts up. Adjust the pointer in the of-tree stored under
> > linux,sml-base to point to this buffer holding the preserved log. The
> > TPM driver can then read the base address from this entry when making
> > the log available.
> 

> > +{
> > +       const u32 *sizep;
> > +       const u64 *basep;
> > +
> > +       sizep = of_get_property(np, "linux,sml-size", NULL);
> > +       basep = of_get_property(np, "linux,sml-base", NULL);
> 
> Any new properties need a schema. For chosen, that's located here[1].
> The more common pattern for similar properties is <base size>.

I noticed these are already documented and are TPM node properties. I 
was thinking these are chosen node properties. Though it would be good 
to get these common TPM properties converted to schema. That can live in 
the kernel tree.

Rob

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Rob Herring]

On Tue, Jun 14, 2022 at 11:48:30AM -0600, Rob Herring wrote:
> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
> >
> > The memory area of the TPM measurement log is currently not properly
> > duplicated for carrying it across kexec when an Open Firmware
> > Devicetree is used. Therefore, the contents of the log get corrupted.
> > Fix this for the kexec_file_load() syscall by allocating a buffer and
> > copying the contents of the existing log into it. The new buffer is
> > preserved across the kexec and a pointer to it is available when the new
> > kernel is started. To achieve this, store the allocated buffer's address
> > in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
> > and search for this entry early in the kernel startup before the TPM
> > subsystem starts up. Adjust the pointer in the of-tree stored under
> > linux,sml-base to point to this buffer holding the preserved log. The
> > TPM driver can then read the base address from this entry when making
> > the log available.
> 

> > +{
> > +       const u32 *sizep;
> > +       const u64 *basep;
> > +
> > +       sizep = of_get_property(np, "linux,sml-size", NULL);
> > +       basep = of_get_property(np, "linux,sml-base", NULL);
> 
> Any new properties need a schema. For chosen, that's located here[1].
> The more common pattern for similar properties is <base size>.

I noticed these are already documented and are TPM node properties. I 
was thinking these are chosen node properties. Though it would be good 
to get these common TPM properties converted to schema. That can live in 
the kernel tree.

Rob

Re: [PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Jarkko Sakkinen]

On Tue, Jun 14, 2022 at 12:12:58PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
> the base and size of the TPM log.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> ---
>  drivers/char/tpm/eventlog/of.c | 31 +++++--------------------------
>  1 file changed, 5 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..e272c7a0765f 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/tpm_eventlog.h>
>  
>  #include "../tpm.h"
> @@ -20,11 +21,10 @@
>  int tpm_read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
> -	const u32 *sizep;
> -	const u64 *basep;
>  	struct tpm_bios_log *log;
>  	u32 size;
>  	u64 base;
> +	int ret;
>  
>  	log = &chip->log;
>  	if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	if (of_property_read_bool(np, "powered-while-suspended"))
>  		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>  
> -	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (sizep == NULL && basep == NULL)
> -		return -ENODEV;
> -	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;
> -	}
> +	ret = of_tpm_get_sml_parameters(np, &base, &size);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (size == 0) {
>  		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> -- 
> 2.35.1
> 

Looks good to me.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

Re: [PATCH 3/3] tpm: of: Call of_tpm_get_sml_parameters() to get base and size of log

[Jarkko Sakkinen]

On Tue, Jun 14, 2022 at 12:12:58PM -0400, Stefan Berger wrote:
> Simplify tpm_read_log_of() and call of_tpm_get_sml_parameters() to get
> the base and size of the TPM log.
> 
> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> Cc: Jarkko Sakkinen <jarkko@kernel.org>
> Cc: Jason Gunthorpe <jgg@ziepe.ca>
> ---
>  drivers/char/tpm/eventlog/of.c | 31 +++++--------------------------
>  1 file changed, 5 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/char/tpm/eventlog/of.c b/drivers/char/tpm/eventlog/of.c
> index a9ce66d09a75..e272c7a0765f 100644
> --- a/drivers/char/tpm/eventlog/of.c
> +++ b/drivers/char/tpm/eventlog/of.c
> @@ -12,6 +12,7 @@
>  
>  #include <linux/slab.h>
>  #include <linux/of.h>
> +#include <linux/of_device.h>
>  #include <linux/tpm_eventlog.h>
>  
>  #include "../tpm.h"
> @@ -20,11 +21,10 @@
>  int tpm_read_log_of(struct tpm_chip *chip)
>  {
>  	struct device_node *np;
> -	const u32 *sizep;
> -	const u64 *basep;
>  	struct tpm_bios_log *log;
>  	u32 size;
>  	u64 base;
> +	int ret;
>  
>  	log = &chip->log;
>  	if (chip->dev.parent && chip->dev.parent->of_node)
> @@ -35,30 +35,9 @@ int tpm_read_log_of(struct tpm_chip *chip)
>  	if (of_property_read_bool(np, "powered-while-suspended"))
>  		chip->flags |= TPM_CHIP_FLAG_ALWAYS_POWERED;
>  
> -	sizep = of_get_property(np, "linux,sml-size", NULL);
> -	basep = of_get_property(np, "linux,sml-base", NULL);
> -	if (sizep == NULL && basep == NULL)
> -		return -ENODEV;
> -	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;
> -	}
> +	ret = of_tpm_get_sml_parameters(np, &base, &size);
> +	if (ret < 0)
> +		return ret;
>  
>  	if (size == 0) {
>  		dev_warn(&chip->dev, "%s: Event log area empty\n", __func__);
> -- 
> 2.35.1
> 

Looks good to me.

Reviewed-by: Jarkko Sakkinen <jarkko@kernel.org>

BR, Jarkko

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

On 6/15/22 16:14, Rob Herring wrote:
> On Wed, Jun 15, 2022 at 09:08:04AM -0400, Stefan Berger wrote:
>>
>>
>> On 6/14/22 13:48, Rob Herring wrote:
>>> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>> The memory area of the TPM measurement log is currently not properly
>>>> duplicated for carrying it across kexec when an Open Firmware
>>>> Devicetree is used. Therefore, the contents of the log get corrupted.
>>>> Fix this for the kexec_file_load() syscall by allocating a buffer and
>>>> copying the contents of the existing log into it. The new buffer is
>>>> preserved across the kexec and a pointer to it is available when the new
>>>> kernel is started. To achieve this, store the allocated buffer's address
>>>> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
>>>> and search for this entry early in the kernel startup before the TPM
>>>> subsystem starts up. Adjust the pointer in the of-tree stored under
>>>> linux,sml-base to point to this buffer holding the preserved log. The
>>>> TPM driver can then read the base address from this entry when making
>>>> the log available.
>>>
>>> This series really needs a wider Cc list of folks that worry about
>>> TPMs and kexec.
>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>> Cc: Eric Biederman <ebiederm@xmission.com>
>>>> ---
>>>>    drivers/of/device.c       |  24 +++++
>>>>    drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>>>>    include/linux/kexec.h     |   6 ++
>>>>    include/linux/of.h        |   5 +
>>>>    include/linux/of_device.h |   3 +
>>>>    kernel/kexec_file.c       |   6 ++
>>>>    6 files changed, 233 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>> index 874f031442dc..0cbd47b1cabc 100644
>>>> --- a/drivers/of/device.c
>>>> +++ b/drivers/of/device.c
>>>> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>>>           return 0;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>>>> +
>>>> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
>>>
>>> of/device.c is for functions that work on a struct device. This is not
>>> the case here.
>>
>> Can I put it into platform.c?
> 
> That's for struct platform_device things.
> Should I create a new file then?

>> I should have probably mentioned it but this function here is a copy of code
>> from tpm_read_log_of() from here: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38
>>
>> 3/3 refactors the code in tpm/eventlog/of.c to make use of this new function
>> then to avoid code duplication. Therefore, this code here is more general
>> than necessary at this point. Maybe I should do the move in a patch of its
>> own?
> 
> Maybe you should leave that function there and call it?

The function would only be callable if TPM subsystem was enabled. That 
doesn't sound right to make kexec depend on the TPM subsystem.

I could just implement a less generic function that only handles the 
vTPM case then and not target this function to be called from the TPM 
subsystem via the refactoring in 3/3.

> 
> Generally, subsystem specific things go into the subsystems. However,
> there's a few special cases like kexec now. That was added primarily to
> avoid per arch duplication.
> 
> I've never looked at the TPM code, so sorry, I don't have more specific
> suggestions.
> 
>>>> +{
>>>> +       const u32 *sizep;
>>>> +       const u64 *basep;
>>>> +
>>>> +       sizep = of_get_property(np, "linux,sml-size", NULL);
>>>> +       basep = of_get_property(np, "linux,sml-base", NULL);
>>>
>>> Any new properties need a schema. For chosen, that's located here[1].
>>> The more common pattern for similar properties is <base size>.
>>>
>>> Don't use of_get_property(), but the typed functions instead.
>>
>> I think this was done due to the little endian and big endian distinction
>> below.
> 
> 
> Right.
> 
>>>> +       if (sizep == NULL && basep == NULL)
>>>> +               return -ENODEV;
>>>> +       if (sizep == NULL || basep == NULL)
>>>> +               return -EIO;
>>>
>>> Do we really need the error distinction?
>>
>> As I mentioned, this code is a copy from the TPM subsystem. There it does
>> make a distinction because similar functions for acpi and efi make a
>> distinction as well although this particular function's return code doesn't
>> matter in the end.
>>
>> The code I am referring to is this here:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85
>>
>>>
>>>> +
>>>> +       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;
>>>
>>> What? Some platforms aren't properly encoded? Fix the firmware.
>>
>> It's been like this for years...
> 
> Great! :(
> 
> I'm confused how IBM needs this? Only a LE machine with LE DT encoding
> would need this. With Power being historically BE and only recently
> (though I guess that's a few years now) supporting LE, how did the DT
> encoding become LE for this yet not for everything else in DT?
> 


For some reason the TPM log of a hardware TPM I believe is written in 
little endian. I am not aware of the history of it.

> [...]
> 
> 
>>>> +/**
>>>> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
>>>> + */
>>>> +static int __init tpm_post_kexec(void)
>>>> +{
>>>> +       struct property *newprop;
>>>> +       struct device_node *np;
>>>> +       void *phyaddr;
>>>> +       size_t size;
>>>> +       u32 oflogsize;
>>>> +       u64 unused;
>>>> +       int ret;
>>>> +
>>>> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
>>>> +       if (ret)
>>>> +               return 0;
>>>> +
>>>> +       /*
>>>> +        * If any one of the following steps fails then the next kexec will
>>>> +        * cause issues due to linux,sml-base pointing to a stale buffer.
>>>> +        */
>>>> +       np = of_find_node_by_name(NULL, "vtpm");
>>>
>>> This seems pretty IBM specific.
>>
>> Yes, it is and I am not sure what to do about it. Should I cover parts of
>> the function with a #ifdef CONFIG_PPC_PSERIES ?
> 
> #ifdef's aren't great. IS_ENABLED() is a bit better, but really put
> implementation specific things in implementation specific code.
> 
> Perhaps each TPM implementation needs its own hook to do stuff?
> 


 From what I know it's only vTPM with of-tree that needs this.

>>>> +       if (!np)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       /* logsize must not have changed */
>>>> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
>>>> +               goto err_free_memblock;
>>>> +       if (oflogsize != size)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       /* replace linux,sml-base with new physical address of buffer */
>>>> +       ret = -ENOMEM;
>>>> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>>>> +       if (!newprop)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
>>>> +       if (!newprop->name)
>>>> +               goto err_free_newprop;
>>>> +
>>>> +       newprop->length = sizeof(phyaddr);
>>>> +
>>>> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
>>>> +       if (!newprop->value)
>>>> +               goto err_free_newprop_struct;
>>>> +
>>>> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>>>> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>>>> +               ret = -ENODEV;
>>>> +               goto err_free_newprop_struct;
>>>> +       } else {
>>>> +               *(u64 *)newprop->value = (u64)phyaddr;
>>>> +       }
>>>> +
>>>> +       ret = of_update_property(np, newprop);
>>>
>>> Just FYI for now, there's some work happening on a better API for
>>> writing nodes and properties.
>>
>> Ok.
>>
>>>
>>>> +       if (ret) {
>>>> +               pr_err("Could not update linux,sml-base with new address");
>>>> +               goto err_free_newprop_struct;
>>>> +       }
>>>> +
>>>> +       goto exit;
>>>> +
>>>> +err_free_newprop_struct:
>>>> +       kfree(newprop->name);
>>>> +err_free_newprop:
>>>> +       kfree(newprop);
>>>> +err_free_memblock:
>>>> +       memblock_phys_free((phys_addr_t)phyaddr, size);
>>>> +exit:
>>>> +       tpm_of_remove_kexec_buffer();
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +postcore_initcall(tpm_post_kexec);
>>>
>>> Would be better if this is called explicitly at the right time when
>>> needed rather than using an initcall.
>>
>> The 'when needed' would be the TPM subsystem. However, if I was to make it
>> dependent on the TPM subsystem we would loose the TPM log if we were not to
>> kexec into a kernel with TPM subsystem or the TPM driver wasn't activated. I
>> wanted to be able to preserve the log even if a kexec'ed kernel didn't
>> support or activate the TPM driver and then a subsequent one again has it
>> activated...
> 
> Sounds like a TPM problem the TPM code should deal with. Or a scenario
> that just shouldn't be supported. IDK

It only comes down to from where tpm_post_kexec() is called. The 
postcore_initcall() solves the issue this series is addressing in a more 
general way than if it's called from the TPM subsystem. If this initcall 
is unacceptable then I guess we would have to call if from the TPM 
subsystem - no other choice then.


   Thanks.
     Stefan

Re: [PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec

[Stefan Berger]

On 6/15/22 16:14, Rob Herring wrote:
> On Wed, Jun 15, 2022 at 09:08:04AM -0400, Stefan Berger wrote:
>>
>>
>> On 6/14/22 13:48, Rob Herring wrote:
>>> (),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@linux.ibm.com> wrote:
>>>>
>>>> The memory area of the TPM measurement log is currently not properly
>>>> duplicated for carrying it across kexec when an Open Firmware
>>>> Devicetree is used. Therefore, the contents of the log get corrupted.
>>>> Fix this for the kexec_file_load() syscall by allocating a buffer and
>>>> copying the contents of the existing log into it. The new buffer is
>>>> preserved across the kexec and a pointer to it is available when the new
>>>> kernel is started. To achieve this, store the allocated buffer's address
>>>> in the flattened device tree (fdt) under the name linux,tpm-kexec-buffer
>>>> and search for this entry early in the kernel startup before the TPM
>>>> subsystem starts up. Adjust the pointer in the of-tree stored under
>>>> linux,sml-base to point to this buffer holding the preserved log. The
>>>> TPM driver can then read the base address from this entry when making
>>>> the log available.
>>>
>>> This series really needs a wider Cc list of folks that worry about
>>> TPMs and kexec.
>>>
>>>> Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
>>>> Cc: Rob Herring <robh+dt@kernel.org>
>>>> Cc: Frank Rowand <frowand.list@gmail.com>
>>>> Cc: Eric Biederman <ebiederm@xmission.com>
>>>> ---
>>>>    drivers/of/device.c       |  24 +++++
>>>>    drivers/of/kexec.c        | 190 +++++++++++++++++++++++++++++++++++++-
>>>>    include/linux/kexec.h     |   6 ++
>>>>    include/linux/of.h        |   5 +
>>>>    include/linux/of_device.h |   3 +
>>>>    kernel/kexec_file.c       |   6 ++
>>>>    6 files changed, 233 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/of/device.c b/drivers/of/device.c
>>>> index 874f031442dc..0cbd47b1cabc 100644
>>>> --- a/drivers/of/device.c
>>>> +++ b/drivers/of/device.c
>>>> @@ -382,3 +382,27 @@ int of_device_uevent_modalias(struct device *dev, struct kobj_uevent_env *env)
>>>>           return 0;
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(of_device_uevent_modalias);
>>>> +
>>>> +int of_tpm_get_sml_parameters(struct device_node *np, u64 *base, u32 *size)
>>>
>>> of/device.c is for functions that work on a struct device. This is not
>>> the case here.
>>
>> Can I put it into platform.c?
> 
> That's for struct platform_device things.
> Should I create a new file then?

>> I should have probably mentioned it but this function here is a copy of code
>> from tpm_read_log_of() from here: https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/of.c#L38
>>
>> 3/3 refactors the code in tpm/eventlog/of.c to make use of this new function
>> then to avoid code duplication. Therefore, this code here is more general
>> than necessary at this point. Maybe I should do the move in a patch of its
>> own?
> 
> Maybe you should leave that function there and call it?

The function would only be callable if TPM subsystem was enabled. That 
doesn't sound right to make kexec depend on the TPM subsystem.

I could just implement a less generic function that only handles the 
vTPM case then and not target this function to be called from the TPM 
subsystem via the refactoring in 3/3.

> 
> Generally, subsystem specific things go into the subsystems. However,
> there's a few special cases like kexec now. That was added primarily to
> avoid per arch duplication.
> 
> I've never looked at the TPM code, so sorry, I don't have more specific
> suggestions.
> 
>>>> +{
>>>> +       const u32 *sizep;
>>>> +       const u64 *basep;
>>>> +
>>>> +       sizep = of_get_property(np, "linux,sml-size", NULL);
>>>> +       basep = of_get_property(np, "linux,sml-base", NULL);
>>>
>>> Any new properties need a schema. For chosen, that's located here[1].
>>> The more common pattern for similar properties is <base size>.
>>>
>>> Don't use of_get_property(), but the typed functions instead.
>>
>> I think this was done due to the little endian and big endian distinction
>> below.
> 
> 
> Right.
> 
>>>> +       if (sizep == NULL && basep == NULL)
>>>> +               return -ENODEV;
>>>> +       if (sizep == NULL || basep == NULL)
>>>> +               return -EIO;
>>>
>>> Do we really need the error distinction?
>>
>> As I mentioned, this code is a copy from the TPM subsystem. There it does
>> make a distinction because similar functions for acpi and efi make a
>> distinction as well although this particular function's return code doesn't
>> matter in the end.
>>
>> The code I am referring to is this here:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/char/tpm/eventlog/common.c#L85
>>
>>>
>>>> +
>>>> +       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;
>>>
>>> What? Some platforms aren't properly encoded? Fix the firmware.
>>
>> It's been like this for years...
> 
> Great! :(
> 
> I'm confused how IBM needs this? Only a LE machine with LE DT encoding
> would need this. With Power being historically BE and only recently
> (though I guess that's a few years now) supporting LE, how did the DT
> encoding become LE for this yet not for everything else in DT?
> 


For some reason the TPM log of a hardware TPM I believe is written in 
little endian. I am not aware of the history of it.

> [...]
> 
> 
>>>> +/**
>>>> + * tpm_post_kexec - Make stored TPM log buffer available in of-tree
>>>> + */
>>>> +static int __init tpm_post_kexec(void)
>>>> +{
>>>> +       struct property *newprop;
>>>> +       struct device_node *np;
>>>> +       void *phyaddr;
>>>> +       size_t size;
>>>> +       u32 oflogsize;
>>>> +       u64 unused;
>>>> +       int ret;
>>>> +
>>>> +       ret = tpm_get_kexec_buffer(&phyaddr, &size);
>>>> +       if (ret)
>>>> +               return 0;
>>>> +
>>>> +       /*
>>>> +        * If any one of the following steps fails then the next kexec will
>>>> +        * cause issues due to linux,sml-base pointing to a stale buffer.
>>>> +        */
>>>> +       np = of_find_node_by_name(NULL, "vtpm");
>>>
>>> This seems pretty IBM specific.
>>
>> Yes, it is and I am not sure what to do about it. Should I cover parts of
>> the function with a #ifdef CONFIG_PPC_PSERIES ?
> 
> #ifdef's aren't great. IS_ENABLED() is a bit better, but really put
> implementation specific things in implementation specific code.
> 
> Perhaps each TPM implementation needs its own hook to do stuff?
> 


 From what I know it's only vTPM with of-tree that needs this.

>>>> +       if (!np)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       /* logsize must not have changed */
>>>> +       if (of_tpm_get_sml_parameters(np, &unused, &oflogsize) < 0)
>>>> +               goto err_free_memblock;
>>>> +       if (oflogsize != size)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       /* replace linux,sml-base with new physical address of buffer */
>>>> +       ret = -ENOMEM;
>>>> +       newprop = kzalloc(sizeof(*newprop), GFP_KERNEL);
>>>> +       if (!newprop)
>>>> +               goto err_free_memblock;
>>>> +
>>>> +       newprop->name = kstrdup("linux,sml-base", GFP_KERNEL);
>>>> +       if (!newprop->name)
>>>> +               goto err_free_newprop;
>>>> +
>>>> +       newprop->length = sizeof(phyaddr);
>>>> +
>>>> +       newprop->value = kmalloc(sizeof(u64), GFP_KERNEL);
>>>> +       if (!newprop->value)
>>>> +               goto err_free_newprop_struct;
>>>> +
>>>> +       if (of_property_match_string(np, "compatible", "IBM,vtpm") < 0 &&
>>>> +           of_property_match_string(np, "compatible", "IBM,vtpm20") < 0) {
>>>> +               ret = -ENODEV;
>>>> +               goto err_free_newprop_struct;
>>>> +       } else {
>>>> +               *(u64 *)newprop->value = (u64)phyaddr;
>>>> +       }
>>>> +
>>>> +       ret = of_update_property(np, newprop);
>>>
>>> Just FYI for now, there's some work happening on a better API for
>>> writing nodes and properties.
>>
>> Ok.
>>
>>>
>>>> +       if (ret) {
>>>> +               pr_err("Could not update linux,sml-base with new address");
>>>> +               goto err_free_newprop_struct;
>>>> +       }
>>>> +
>>>> +       goto exit;
>>>> +
>>>> +err_free_newprop_struct:
>>>> +       kfree(newprop->name);
>>>> +err_free_newprop:
>>>> +       kfree(newprop);
>>>> +err_free_memblock:
>>>> +       memblock_phys_free((phys_addr_t)phyaddr, size);
>>>> +exit:
>>>> +       tpm_of_remove_kexec_buffer();
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +postcore_initcall(tpm_post_kexec);
>>>
>>> Would be better if this is called explicitly at the right time when
>>> needed rather than using an initcall.
>>
>> The 'when needed' would be the TPM subsystem. However, if I was to make it
>> dependent on the TPM subsystem we would loose the TPM log if we were not to
>> kexec into a kernel with TPM subsystem or the TPM driver wasn't activated. I
>> wanted to be able to preserve the log even if a kexec'ed kernel didn't
>> support or activate the TPM driver and then a subsequent one again has it
>> activated...
> 
> Sounds like a TPM problem the TPM code should deal with. Or a scenario
> that just shouldn't be supported. IDK

It only comes down to from where tpm_post_kexec() is called. The 
postcore_initcall() solves the issue this series is addressing in a more 
general way than if it's called from the TPM subsystem. If this initcall 
is unacceptable then I guess we would have to call if from the TPM 
subsystem - no other choice then.


   Thanks.
     Stefan

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec