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

Stefan Berger
2022-07-06T15:07:34Z

On 7/6/22 10:00, Jonathan McDowell wrote:
> On Tue, Jul 05, 2022 at 06:46:54PM -0400, Mimi Zohar wrote:
>> [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
>
> None of this looks difficult to re-do on top of my changes that are in
> -next; the only thing to watch out for is a couple of functions have
> moved into the __init section but that looks appropriate for your TPM
> log carry-over too.

Yes, I am rebasing my series now and will post v5 of this series with
your patch prepended as well.

Stefan

>
>>> ---
>>> 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,