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

[Rob Herring <robh@kernel.org>]

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