Message-ID: <875z8ju1hc.fsf@morokweng.localdomain>
Date:   Sat, 12 Sep 2020 00:18:23 -0300
From:   Thiago Jung Bauermann <bauerman@...ux.ibm.com>
To:     Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
Cc:     zohar@...ux.ibm.com, robh@...nel.org, gregkh@...uxfoundation.org,
        james.morse@....com, catalin.marinas@....com, sashal@...nel.org,
        will@...nel.org, mpe@...erman.id.au, benh@...nel.crashing.org,
        paulus@...ba.org, robh+dt@...nel.org, frowand.list@...il.com,
        vincenzo.frascino@....com, mark.rutland@....com,
        dmitry.kasatkin@...il.com, jmorris@...ei.org, serge@...lyn.com,
        pasha.tatashin@...een.com, allison@...utok.net,
        kstewart@...uxfoundation.org, takahiro.akashi@...aro.org,
        tglx@...utronix.de, masahiroy@...nel.org, bhsharma@...hat.com,
        mbrugger@...e.com, hsinyi@...omium.org, tao.li@...o.com,
        christophe.leroy@....fr, linux-integrity@...r.kernel.org,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        prsriva@...ux.microsoft.com, balajib@...ux.microsoft.com
Subject: Re: [PATCH v6 1/3] powerpc: Refactor kexec functions to move arch
 independent code to IMA


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@...ux.microsoft.com> 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@...ux.microsoft.com)
> + *
> + * 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

Powered by blists - more mailing lists