Re: [PATCH v4 4/5] of: kexec: Refactor IMA buffer related functions to make them reusable

Mimi Zohar
2022-07-05T22:47:33Z

[Cc'ing Borislav Petkov <bp@suse.de>, Jonathan McDowell <noodles@fb.com
>]

Hi Stefan,

On Thu, 2022-06-30 at 22:26 -0400, Stefan Berger wrote:
> 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>
> Cc: Mimi Zohar <zohar@linux.ibm.com>

Refactoring the ima_get_kexec_buffer sounds good, but there's a merge
conflict with Jonathan McDowell's commit "b69a2afd5afc x86/kexec: Carry
forward IMA measurement log on kexec".
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/of/kexec.c

thanks,

Mimi

>
> ---
> v4:
> - Move debug output into setup_buffer()
> ---
> drivers/of/kexec.c | 131 ++++++++++++++++++++++++++-------------------
> 1 file changed, 76 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index c4f9b6655a2e..0710703acfb0 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -115,48 +115,59 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
> return 0;
> }
>
> -/**
> - * ima_get_kexec_buffer - get IMA buffer from the previous kernel
> - * @addr: On successful return, set to point to the buffer contents.
> - * @size: On successful return, set to the buffer size.
> - *
> - * Return: 0 on success, negative errno on error.
> - */
> -int ima_get_kexec_buffer(void **addr, size_t *size)
> +static int get_kexec_buffer(const char *name, unsigned long *addr, size_t *size)
> {
> int ret, len;
> - unsigned long tmp_addr;
> unsigned long start_pfn, end_pfn;
> - 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);
> + prop = of_get_property(of_chosen, name, &len);
> if (!prop)
> return -ENOENT;
>
> - ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
> + ret = do_get_kexec_buffer(prop, len, addr, size);
> if (ret)
> return ret;
>
> - /* Do some sanity on the returned size for the ima-kexec buffer */
> - if (!tmp_size)
> + /* Do some sanity on the returned size for the kexec buffer */
> + if (!*size)
> return -ENOENT;
>
> /*
> * Calculate the PFNs for the buffer and ensure
> * they are with in addressable memory.
> */
> - start_pfn = PHYS_PFN(tmp_addr);
> - end_pfn = PHYS_PFN(tmp_addr + tmp_size - 1);
> + start_pfn = PHYS_PFN(*addr);
> + end_pfn = PHYS_PFN(*addr + *size - 1);
> if (!page_is_ram(start_pfn) || !page_is_ram(end_pfn)) {
> - pr_warn("IMA buffer at 0x%lx, size = 0x%zx beyond memory\n",
> - tmp_addr, tmp_size);
> + pr_warn("%s buffer at 0x%lx, size = 0x%zx beyond memory\n",
> + name, *addr, *size);
> return -EINVAL;
> }
>
> + return 0;
> +}
> +
> +/**
> + * ima_get_kexec_buffer - get IMA buffer from the previous kernel
> + * @addr: On successful return, set to point to the buffer contents.
> + * @size: On successful return, set to the buffer size.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int ima_get_kexec_buffer(void **addr, size_t *size)
> +{
> + int ret;
> + unsigned long tmp_addr;
> + size_t tmp_size;
> +
> + if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> + return -ENOTSUPP;
> +
> + ret = get_kexec_buffer("linux,ima-kexec-buffer", &tmp_addr, &tmp_size);
> + if (ret)
> + return ret;
> +
> *addr = __va(tmp_addr);
> *size = tmp_size;
>
> @@ -191,72 +202,82 @@ int ima_free_kexec_buffer(void)
> return memblock_phys_free(addr, size);
> }
>
> -/**
> - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> - *
> - * @fdt: Flattened Device Tree to update
> - * @chosen_node: Offset to the chosen node in the device tree
> - *
> - * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> - * remove it from the device tree.
> - */
> -static void remove_ima_buffer(void *fdt, int chosen_node)
> +static int remove_buffer(void *fdt, int chosen_node, const char *name)
> {
> int ret, len;
> unsigned long addr;
> size_t size;
> const void *prop;
>
> - if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> - return;
> -
> - prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
> + prop = fdt_getprop(fdt, chosen_node, name, &len);
> if (!prop)
> - return;
> + return -ENOENT;
>
> ret = do_get_kexec_buffer(prop, len, &addr, &size);
> - fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> + fdt_delprop(fdt, chosen_node, name);
> if (ret)
> - return;
> + return ret;
>
> ret = fdt_find_and_del_mem_rsv(fdt, addr, size);
> if (!ret)
> - pr_debug("Removed old IMA buffer reservation.\n");
> + pr_debug("Remove old %s buffer reserveration", name);
> + return ret;
> }
>
> -#ifdef CONFIG_IMA_KEXEC
> /**
> - * setup_ima_buffer - add IMA 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.
> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> *
> - * Return: 0 on success, or negative errno on error.
> + * @fdt: Flattened Device Tree to update
> + * @chosen_node: Offset to the chosen node in the device tree
> + *
> + * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> + * remove it from the device tree.
> */
> -static int setup_ima_buffer(const struct kimage *image, void *fdt,
> - int chosen_node)
> +static void remove_ima_buffer(void *fdt, int chosen_node)
> +{
> + if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
> + return;
> +
> + remove_buffer(fdt, chosen_node, "linux,ima-kexec-buffer");
> +}
> +
> +#ifdef CONFIG_IMA_KEXEC
> +static int setup_buffer(void *fdt, int chosen_node, const char *name,
> + phys_addr_t addr, size_t size)
> {
> int ret;
>
> - if (!image->ima_buffer_size)
> + if (!size)
> return 0;
>
> ret = fdt_appendprop_addrrange(fdt, 0, chosen_node,
> - "linux,ima-kexec-buffer",
> - image->ima_buffer_addr,
> - image->ima_buffer_size);
> + name, addr, size);
> if (ret < 0)
> return -EINVAL;
>
> - ret = fdt_add_mem_rsv(fdt, image->ima_buffer_addr,
> - image->ima_buffer_size);
> + ret = fdt_add_mem_rsv(fdt, addr, size);
> if (ret)
> return -EINVAL;
>
> - pr_debug("IMA buffer at 0x%pa, size = 0x%zx\n",
> - &image->ima_buffer_addr, image->ima_buffer_size);
> + pr_debug("%s at 0x%pa, size = 0x%zx\n", name, &addr, size);
>
> return 0;
> +
> +}
> +
> +/**
> + * setup_ima_buffer - add IMA 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_ima_buffer(const struct kimage *image, void *fdt,
> + int chosen_node)
> +{
> + return setup_buffer(fdt, chosen_node, "linux,ima-kexec-buffer",
> + image->ima_buffer_addr, image->ima_buffer_size);
> }
> #else /* CONFIG_IMA_KEXEC */
> static inline int setup_ima_buffer(const struct kimage *image, void *fdt,