Re: [PATCH v6 1/3] powerpc: Refactor kexec functions to move arch independent code to IMA

From: Thiago Jung Bauermann
Date: Fri Sep 11 2020 - 23:19:41 EST

Next message: Matthew Wilcox: "Re: [PATCH v2 7/8] mm/shmem: Return head page from find_lock_entry"
Previous message: Lu Baolu: "Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu"
In reply to: Lakshmi Ramasubramanian: "[PATCH v6 1/3] powerpc: Refactor kexec functions to move arch independent code to IMA"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hello Lakshmi,

The code itself is good, I have just some minor comments about it.

But movement of powerpc code is unfortunately creating some issues. More
details below.

Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:

> diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
> index 720e50e490b6..467647886064 100644
> --- a/arch/powerpc/kexec/ima.c
> +++ b/arch/powerpc/kexec/ima.c
> @@ -9,6 +9,7 @@
> #include <linux/slab.h>
> #include <linux/kexec.h>
> #include <linux/of.h>
> +#include <linux/ima.h>
> #include <linux/memblock.h>
> #include <linux/libfdt.h>
>
> @@ -28,105 +29,6 @@ static int get_addr_size_cells(int *addr_cells, int *size_cells)
> return 0;
> }
>
> -static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
> - size_t *size)
> -{
> - int ret, addr_cells, size_cells;
> -
> - ret = get_addr_size_cells(&addr_cells, &size_cells);
> - if (ret)
> - return ret;
> -
> - if (len < 4 * (addr_cells + size_cells))
> - return -ENOENT;
> -
> - *addr = of_read_number(prop, addr_cells);
> - *size = of_read_number(prop + 4 * addr_cells, size_cells);
> -
> - 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, len;
> - unsigned long tmp_addr;
> - size_t tmp_size;
> - const void *prop;
> -
> - prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len);
> - if (!prop)
> - return -ENOENT;
> -
> - ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
> - if (ret)
> - return ret;
> -
> - *addr = __va(tmp_addr);
> - *size = tmp_size;
> -
> - return 0;
> -}
> -
> -/**
> - * ima_free_kexec_buffer - free memory used by the IMA buffer
> - */
> -int ima_free_kexec_buffer(void)
> -{
> - int ret;
> - unsigned long addr;
> - size_t size;
> - struct property *prop;
> -
> - prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL);
> - if (!prop)
> - return -ENOENT;
> -
> - ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size);
> - if (ret)
> - return ret;
> -
> - ret = of_remove_property(of_chosen, prop);
> - if (ret)
> - return ret;
> -
> - return memblock_free(addr, size);
> -
> -}
> -
> -/**
> - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
> - *
> - * The IMA measurement buffer is of no use to a subsequent kernel, so we always
> - * remove it from the device tree.
> - */
> -void remove_ima_buffer(void *fdt, int chosen_node)
> -{
> - int ret, len;
> - unsigned long addr;
> - size_t size;
> - const void *prop;
> -
> - prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
> - if (!prop)
> - return;
> -
> - ret = do_get_kexec_buffer(prop, len, &addr, &size);
> - fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> - if (ret)
> - return;
> -
> - ret = delete_fdt_mem_rsv(fdt, addr, size);
> - if (!ret)
> - pr_debug("Removed old IMA buffer reservation.\n");
> -}
> -

With this change if CONFIG_IMA=y but CONFIG_IMA_KEXEC=n, this file will
only have static int get_addr_size_cells() as its contents, which isn't
useful (and will print a warning about the function not being used). You
should change the Makefile to only build this file if CONFIG_IMA_KEXEC=y.
Then you can also remove the #ifdef CONFIG_IMA_KEXEC/#endif within it as
well.

> #ifdef CONFIG_IMA_KEXEC
> /**
> * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> @@ -179,7 +81,7 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
> int ret, addr_cells, size_cells, entry_size;
> u8 value[16];
>
> - remove_ima_buffer(fdt, chosen_node);
> + ima_remove_kexec_buffer(fdt, chosen_node);
> if (!image->arch.ima_buffer_size)
> return 0;
>
> @@ -201,7 +103,7 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
> if (ret)
> return ret;
>
> - ret = fdt_setprop(fdt, chosen_node, "linux,ima-kexec-buffer", value,
> + ret = fdt_setprop(fdt, chosen_node, FDT_PROP_IMA_KEXEC_BUFFER, value,
> entry_size);
> if (ret < 0)
> return -EINVAL;

<snip>

> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 9e93bef52968..00a60dcc7075 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -223,8 +223,19 @@ extern int crash_exclude_mem_range(struct crash_mem *mem,
> unsigned long long mend);
> extern int crash_prepare_elf64_headers(struct crash_mem *mem, int kernel_map,
> void **addr, unsigned long *sz);
> +
> +extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
> + unsigned long size);
> #endif /* CONFIG_KEXEC_FILE */
>
> +#ifndef CONFIG_KEXEC_FILE

I would use an `#else /* CONFIG_KEXEC_FILE */` here instead of #endif / #ifndef

> +static inline int delete_fdt_mem_rsv(void *fdt, unsigned long start,
> + unsigned long size)
> +{
> + return 0;
> +}
> +#endif /* ifndef CONFIG_KEXEC_FILE */
> +
> #ifdef CONFIG_KEXEC_ELF
> struct kexec_elf_info {
> /*
> diff --git a/include/linux/libfdt.h b/include/linux/libfdt.h
> index 90ed4ebfa692..75fb40aa013b 100644
> --- a/include/linux/libfdt.h
> +++ b/include/linux/libfdt.h
> @@ -5,4 +5,7 @@
> #include <linux/libfdt_env.h>
> #include "../../scripts/dtc/libfdt/libfdt.h"
>
> +/* Common device tree properties */
> +#define FDT_PROP_IMA_KEXEC_BUFFER "linux,ima-kexec-buffer"
> +
> #endif /* _INCLUDE_LIBFDT_H_ */
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index 67dabca670e2..05719fd28aec 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -7,10 +7,11 @@
> obj-$(CONFIG_IMA) += ima.o
>
> ima-y := ima_fs.o ima_queue.o ima_init.o ima_main.o ima_crypto.o ima_api.o \
> - ima_policy.o ima_template.o ima_template_lib.o
> + ima_policy.o ima_template.o ima_template_lib.o ima_fdt.o
> ima-$(CONFIG_IMA_APPRAISE) += ima_appraise.o
> ima-$(CONFIG_IMA_APPRAISE_MODSIG) += ima_modsig.o
> ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o
> ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o
> ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
> ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
> +ima-$(CONFIG_KEXEC_FILE) += ima_kexec_file.o

Even though you are using CONFIG_KEXEC_FILE here, AFAIK this will break
the powerpc build in the case of CONFIG_IMA=n and CONFIG_KEXEC_FILE=y
because the build system won't visit security/integrity/ima in that
case.

<snip>

> +/**
> + * ima_remove_kexec_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.
> + */
> +void ima_remove_kexec_buffer(void *fdt, int chosen_node)
> +{
> + int ret, len;
> + unsigned long addr;
> + size_t size;
> + const void *prop;
> +
> + prop = fdt_getprop(fdt, chosen_node, FDT_PROP_IMA_KEXEC_BUFFER, &len);
> + if (!prop) {
> + pr_err("Unable to find the ima kexec buffer node\n");

It's not an error if there's no IMA kexec buffer node. In case the
previous kernel didn't pass a buffer to this kernel there won't be one,
and it's fine. You should remove this pr_err().

> + return;
> + }
> +
> + ret = do_get_kexec_buffer(prop, len, &addr, &size);
> + fdt_delprop(fdt, chosen_node, FDT_PROP_IMA_KEXEC_BUFFER);
> + if (ret) {
> + pr_err("Unable to delete the ima kexec buffer node\n");
> + return;
> + }
> +
> + ret = delete_fdt_mem_rsv(fdt, addr, size);
> + if (!ret)
> + pr_err("Removed old IMA buffer reservation.\n");

Here you're printing an error when the function actually succeeded. You
should either change this back to a pr_debug(), or remove it.

> +}
> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 121de3e04af2..ed70f99aeedc 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -10,8 +10,66 @@
> #include <linux/seq_file.h>
> #include <linux/vmalloc.h>
> #include <linux/kexec.h>
> +#include <linux/of.h>
> +#include <linux/memblock.h>
> +#include <linux/libfdt.h>
> +#include <linux/ima.h>
> #include "ima.h"
>
> +/**
> + * 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.
> + */
> +static int ima_get_kexec_buffer(void **addr, size_t *size)
> +{
> + int ret, len;
> + unsigned long tmp_addr;
> + size_t tmp_size;
> + const void *prop;
> +
> + prop = of_get_property(of_chosen, FDT_PROP_IMA_KEXEC_BUFFER, &len);
> + if (!prop)
> + return -ENOENT;
> +
> + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size);
> + if (ret)
> + return ret;
> +
> + *addr = __va(tmp_addr);
> + *size = tmp_size;
> +
> + return 0;
> +}
> +
> +/**
> + * ima_free_kexec_buffer - free memory used by the IMA buffer
> + */
> +static int ima_free_kexec_buffer(void)
> +{
> + int ret;
> + unsigned long addr;
> + size_t size;
> + struct property *prop;
> +
> + prop = of_find_property(of_chosen, FDT_PROP_IMA_KEXEC_BUFFER, NULL);
> + if (!prop)
> + return -ENOENT;
> +
> + ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size);
> + if (ret)
> + return ret;
> +
> + ret = of_remove_property(of_chosen, prop);
> + if (ret)
> + return ret;
> +
> + return memblock_free(addr, size);
> +
> +}
> +

Since you created ima_fdt.c, shouldn't the two functions above go in
that file instead of here, since they work with the device tree?

> #ifdef CONFIG_IMA_KEXEC
> static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> unsigned long segment_size)
> diff --git a/security/integrity/ima/ima_kexec_file.c b/security/integrity/ima/ima_kexec_file.c
> new file mode 100644
> index 000000000000..225e913e6ef2
> --- /dev/null
> +++ b/security/integrity/ima/ima_kexec_file.c
> @@ -0,0 +1,51 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2020 Microsoft Corporation
> + *
> + * Author: Lakshmi Ramasubramanian (nramas@xxxxxxxxxxxxxxxxxxx)
> + *
> + * ima_kexec_file.c
> + * Implements functions to handle device tree operations
> + */
> +
> +#include <linux/libfdt.h>
> +#include <linux/kexec.h>
> +#include <linux/types.h>
> +
> +/**
> + * delete_fdt_mem_rsv - delete memory reservation with given address and size
> + *
> + * @fdt: Flattened Device Tree to update
> + * @start: Starting address of the property to update

s/property to update/reservation to delete/

> + * @size: Size of the buffer corresponding to the given property

s/buffer corresponding to the given property/reservation to delete/

> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)

This function can't go in an IMA-specific directory, because it's used
in arch/powerpc/kexec/file_load.c even if IMA isn't enabled.

I'm not sure where would be a good place for it. Perhaps a new file
called kernel/kexec_file_fdt.c?

> +{
> + int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
> +
> + for (i = 0; i < num_rsvs; i++) {
> + uint64_t rsv_start, rsv_size;
> +
> + ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
> + if (ret) {
> + pr_err("Malformed device tree.\n");
> + return -EINVAL;
> + }
> +
> + if (rsv_start == start && rsv_size == size) {
> + ret = fdt_del_mem_rsv(fdt, i);
> + if (ret) {
> + pr_err("Error deleting device tree reservation.\n");
> + return -EINVAL;
> + }
> +
> + pr_err("Freed the reserved memory at %lu of size %lu\n",
> + start, size);

This isn't an error. You should change it to a pr_debug() or remove it.

> + return 0;
> + }
> + }
> +
> + return -ENOENT;
> +}

--
Thiago Jung Bauermann
IBM Linux Technology Center

Next message: Matthew Wilcox: "Re: [PATCH v2 7/8] mm/shmem: Return head page from find_lock_entry"
Previous message: Lu Baolu: "Re: [PATCH V2 5/5] DO NOT MERGE: iommu: disable list appending in dma-iommu"
In reply to: Lakshmi Ramasubramanian: "[PATCH v6 1/3] powerpc: Refactor kexec functions to move arch independent code to IMA"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]