[PATCH 2/3] tpm/kexec: Duplicate TPM measurement log in of-tree for kexec
Rob Herring
robh at kernel.org
Wed Jun 15 13:14:53 PDT 2022
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 at 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 at linux.ibm.com>
> > > Cc: Rob Herring <robh+dt at kernel.org>
> > > Cc: Frank Rowand <frowand.list at gmail.com>
> > > Cc: Eric Biederman <ebiederm at 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
More information about the kexec
mailing list