On 6/14/22 13:48, Rob Herring wrote:
(),On Tue, Jun 14, 2022 at 10:13 AM Stefan Berger <stefanb@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
Cc: Rob Herring <robh+dt@xxxxxxxxxx>
Cc: Frank Rowand <frowand.list@xxxxxxxxx>
Cc: Eric Biederman <ebiederm@xxxxxxxxxxxx>
---
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
