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@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? 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
