Why is linux-arm-msm list CCed on this topic?
Quoting Prakhar Srivastava (2019-08-29 13:05:32)
> Carry ima measurement log for arm64 via kexec_file_load.
> add support to kexec_file_load to pass the ima measurement log
These first two sentences look sort of odd for a commit text.
>
> This patch adds entry for the ima measurement log in the
Don't use 'this patch' in commit text. It's in the wrong voice.
> dtb which is then used in the kexec'ed session to fetch the
> segment and then load the ima measurement log.
>
> Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 3adcec05b1f6..9e1b831e7baa 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -964,6 +964,13 @@ config KEXEC_FILE
> for kernel and initramfs as opposed to list of segments as
> accepted by previous system call.
>
> +config HAVE_IMA_KEXEC
> + bool "enable arch specific ima buffer pass"
> + depends on KEXEC_FILE
> + help
> + This adds support to carry ima log to the next kernel in case
Should ima be all caps here?
> + of kexec_file_load
Add a full-stop?
> +
> config KEXEC_VERIFY_SIG
> bool "Verify kernel signature during kexec_file_load() syscall"
> depends on KEXEC_FILE
> diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h
> new file mode 100644
> index 000000000000..2c504281028d
> --- /dev/null
> +++ b/arch/arm64/include/asm/ima.h
> @@ -0,0 +1,31 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_ARM64_IMA_H
> +#define _ASM_ARM64_IMA_H
> +
> +#define FDT_PROP_KEXEC_BUFFER "linux,ima-kexec-buffer"
Is this documented somewhere in the DT bindings?
> +
> +struct kimage;
> +
> +int ima_get_kexec_buffer(void **addr, size_t *size);
> +int ima_free_kexec_buffer(void);
> +
> +#ifdef CONFIG_IMA
> +void remove_ima_buffer(void *fdt, int chosen_node);
> +#else
> +static inline void remove_ima_buffer(void *fdt, int chosen_node) {}
> +#endif
> +
> +#ifdef CONFIG_IMA_KEXEC
> +int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
> + size_t size);
> +
> +int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
> +#else
> +static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
> + int chosen_node)
> +{
> + remove_ima_buffer(fdt, chosen_node);
> + return 0;
> +}
> +#endif /* CONFIG_IMA_KEXEC */
> +#endif /* _ASM_ARM64_IMA_H */
> diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h
> index 12a561a54128..ca1f9ad5c4d4 100644
> --- a/arch/arm64/include/asm/kexec.h
> +++ b/arch/arm64/include/asm/kexec.h
> @@ -96,6 +96,8 @@ static inline void crash_post_resume(void) {}
> struct kimage_arch {
> void *dtb;
> unsigned long dtb_mem;
> + phys_addr_t ima_buffer_addr;
> + size_t ima_buffer_size;
Should this be in an ifdef?
> };
>
> extern const struct kexec_file_ops kexec_image_ops;
> @@ -107,6 +109,8 @@ extern int load_other_segments(struct kimage *image,
> unsigned long kernel_load_addr, unsigned long kernel_size,
> char *initrd, unsigned long initrd_len,
> char *cmdline);
> +extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
> + unsigned long size);
> #endif
>
> #endif /* __ASSEMBLY__ */
> diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c
> new file mode 100644
> index 000000000000..5ae0d776ec42
> --- /dev/null
> +++ b/arch/arm64/kernel/ima_kexec.c
> @@ -0,0 +1,219 @@
> +/*
> + * Copyright (C) 2016 IBM Corporation
> + *
> + * Authors:
> + * Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
Please use a SPDX tag instead of this boiler plate.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/kexec.h>
> +#include <linux/of.h>
> +#include <linux/memblock.h>
> +#include <linux/libfdt.h>
> +#include <asm/kexec.h>
> +#include <asm/ima.h>
> +
> +static int get_addr_size_cells(int *addr_cells, int *size_cells)
> +{
> + struct device_node *root;
> +
> + root = of_find_node_by_path("/");
> + if (!root)
> + return -EINVAL;
> +
> + *addr_cells = of_n_addr_cells(root);
> + *size_cells = of_n_size_cells(root);
> +
> + of_node_put(root);
> +
> + 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, FDT_PROP_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, FDT_PROP_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);
> +}
> +
> +#ifdef CONFIG_IMA
> +/**
> + * 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, FDT_PROP_KEXEC_BUFFER, &len);
> + if (!prop)
> + return;
> +
> + ret = do_get_kexec_buffer(prop, len, &addr, &size);
> + fdt_delprop(fdt, chosen_node, FDT_PROP_KEXEC_BUFFER);
> + if (ret)
> + return;
> +
> + ret = delete_fdt_mem_rsv(fdt, addr, size);
> + if (!ret)
> + pr_debug("Removed old IMA buffer reservation.\n");
> +}
> +#endif /* CONFIG_IMA */
> +
> +#ifdef CONFIG_IMA_KEXEC
> +/**
> + * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> + *
> + * Architectures should use this function to pass on the IMA buffer
> + * information to the next kernel.
> + *
> + * Return: 0 on success, negative errno on error.
> + */
> +int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
> + size_t size)
> +{
> + image->arch.ima_buffer_addr = load_addr;
> + image->arch.ima_buffer_size = size;
> + return 0;
> +}
> +
> +static int write_number(void *p, u64 value, int cells)
Maybe this should be an of_write_number() API exposed by the DT parsing
code?
> +{
> + if (cells == 1) {
> + u32 tmp;
> +
> + if (value > U32_MAX)
> + return -EINVAL;
> +
> + tmp = cpu_to_be32(value);
> + memcpy(p, &tmp, sizeof(tmp));
> + } else if (cells == 2) {
> + u64 tmp;
> +
> + tmp = cpu_to_be64(value);
> + memcpy(p, &tmp, sizeof(tmp));
> + } else
> + return -EINVAL;
Put braces around this else please.
> + return 0;
> +}
> +
> +/**
> + * setup_ima_buffer - add IMA buffer information to the fdt
> + * @image: kexec image being loaded.
> + * @dtb: Flattened device tree for the next kernel.
> + * @chosen_node: Offset to the chosen node.
Why capitalize Flattened and Offset?
> + *
> + * Return: 0 on success, or negative errno on error.
> + */
> +int setup_ima_buffer(const struct kimage *image, void *dtb, int chosen_node)
> +{
> + int ret, addr_cells, size_cells, entry_size;
> + u8 value[16];
> +
> + remove_ima_buffer(dtb, chosen_node);
> +
> + ret = get_addr_size_cells(&addr_cells, &size_cells);
> + if (ret)
> + return ret;
> +
> + entry_size = 4 * (addr_cells + size_cells);
> +
> + if (entry_size > sizeof(value))
> + return -EINVAL;
> +
> + ret = write_number(value, image->arch.ima_buffer_addr, addr_cells);
> + if (ret)
> + return ret;
> +
> + ret = write_number(value + 4 * addr_cells, image->arch.ima_buffer_size,
> + size_cells);
> + if (ret)
> + return ret;
> +
> + ret = fdt_setprop(dtb, chosen_node, FDT_PROP_KEXEC_BUFFER, value,
> + entry_size);
> + if (ret < 0)
> + return -EINVAL;
> +
> + ret = fdt_add_mem_rsv(dtb, image->segment[0].mem,
> + image->segment[0].memsz);
> + if (ret)
> + return -EINVAL;
> +
> + return 0;
> +}
> +#endif /* CONFIG_IMA_KEXEC */
> diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> index 58871333737a..c05ad6b74b62 100644
> --- a/arch/arm64/kernel/machine_kexec_file.c
> +++ b/arch/arm64/kernel/machine_kexec_file.c
> @@ -21,6 +21,7 @@
> #include <linux/types.h>
> #include <linux/vmalloc.h>
> #include <asm/byteorder.h>
> +#include <asm/ima.h>
>
> /* relevant device tree properties */
> #define FDT_PROP_INITRD_START "linux,initrd-start"
> @@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image,
> goto out;
> }
>
> + /* add ima_buffer */
> + ret = setup_ima_buffer(image, dtb, off);
> + if (ret)
> + goto out;
> +
> /* add kaslr-seed */
> ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED);
> if (ret == -FDT_ERR_NOTFOUND)
> @@ -114,6 +120,39 @@ static int setup_dtb(struct kimage *image,
> */
> #define DTB_EXTRA_SPACE 0x1000
>
> +
> +/**
> + * delete_fdt_mem_rsv - delete memory reservation with given address and size
> + *
Can you document the arguments too?
> + * Return: 0 on success, or negative errno on error.
> + */
> +int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
> +{
> + 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");
Please drop the full-stop on this printk.
> + 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");
Same comment.
> + return -EINVAL;
> + }
> +
> + return 0;
> + }
> + }
> +
> + return -ENOENT;
> +}
A lot of this code looks DT generic. Can it be moved out of the arch
layer to drivers/of/?
