Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
References: <20200618071045.471131-1-prsriva@linux.microsoft.com> <20200618071045.471131-2-prsriva@linux.microsoft.com>
User-agent: mu4e 1.2.0; emacs 26.3
From:   Thiago Jung Bauermann <bauerman@linux.ibm.com>
To:     Prakhar Srivastava <prsriva@linux.microsoft.com>
Cc:     linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, devicetree@vger.kernel.org,
        linux-integrity@vger.kernel.org,
        linux-security-module@vger.kernel.org, catalin.marinas@arm.com,
        will@kernel.org, mpe@ellerman.id.au, benh@kernel.crashing.org,
        paulus@samba.org, robh+dt@kernel.org, frowand.list@gmail.com,
        zohar@linux.ibm.com, dmitry.kasatkin@gmail.com, jmorris@namei.org,
        serge@hallyn.com, pasha.tatashin@soleen.com, allison@lohutok.net,
        kstewart@linuxfoundation.org, takahiro.akashi@linaro.org,
        tglx@linutronix.de, vincenzo.frascino@arm.com,
        mark.rutland@arm.com, masahiroy@kernel.org, james.morse@arm.com,
        bhsharma@redhat.com, mbrugger@suse.com, hsinyi@chromium.org,
        tao.li@vivo.com, christophe.leroy@c-s.fr,
        gregkh@linuxfoundation.org, nramas@linux.microsoft.com,
        tusharsu@linux.microsoft.com, balajib@linux.microsoft.com
Subject: Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
In-reply-to: <20200618071045.471131-2-prsriva@linux.microsoft.com>
Date:   Fri, 19 Jun 2020 21:19:03 -0300
Message-ID: <87o8per3m0.fsf@morokweng.localdomain>
MIME-Version: 1.0
Content-Type: text/plain
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


Prakhar Srivastava <prsriva@linux.microsoft.com> writes:

> Powerpc has support to carry over the IMA measurement logs. Refatoring the
> non-architecture specific code out of arch/powerpc and into security/ima.
>
> The code adds support for reserving and freeing up of memory for IMA measurement
> logs.

Last week, Mimi provided this feedback:

"From your patch description, this patch should be broken up.  Moving
the non-architecture specific code out of powerpc should be one patch.
 Additional support should be in another patch.  After each patch, the
code should work properly."

That's not what you do here. You move the code, but you also make other
changes at the same time. This has two problems:

1. It makes the patch harder to review, because it's very easy to miss a
   change.

2. If in the future a git bisect later points to this patch, it's not
   clear whether the problem is because of the code movement, or because
   of the other changes.

When you move code, ideally the patch should only make the changes
necessary to make the code work at its new location. The patch which
does code movement should not cause any change in behavior.

Other changes should go in separate patches, either before or after the
one moving the code.

More comments below.

>
> ---
>  arch/powerpc/include/asm/ima.h     |  10 ---
>  arch/powerpc/kexec/ima.c           | 126 ++---------------------------
>  security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++
>  3 files changed, 124 insertions(+), 128 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
> index ead488cf3981..c29ec86498f8 100644
> --- a/arch/powerpc/include/asm/ima.h
> +++ b/arch/powerpc/include/asm/ima.h
> @@ -4,15 +4,6 @@
>
>  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);
> @@ -22,7 +13,6 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
>  static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
>  				   int chosen_node)
>  {
> -	remove_ima_buffer(fdt, chosen_node);
>  	return 0;
>  }

This is wrong. Even if the currently running kernel doesn't have
CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory
reservation from the FDT that is being prepared for the next kernel.

This is because the IMA kexec buffer is useless for the next kernel,
regardless of whether the current kernel supports CONFIG_IMA_KEXEC or
not. Keeping it around would be a waste of memory.

> @@ -179,13 +64,18 @@ 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);

This is wrong, for the same reason stated above.

>  	if (!image->arch.ima_buffer_size)
>  		return 0;
>
> -	ret = get_addr_size_cells(&addr_cells, &size_cells);
> -	if (ret)
> +	ret = fdt_address_cells(fdt, chosen_node);
> +	if (ret < 0)
> +		return ret;
> +	addr_cells = ret;
> +
> +	ret = fdt_size_cells(fdt, chosen_node);
> +	if (ret < 0)
>  		return ret;
> +	size_cells = ret;
>
>  	entry_size = 4 * (addr_cells + size_cells);
>

I liked this change. Thanks! I agree it's better to use
fdt_address_cells() and fdt_size_cells() here.

But it should be in a separate patch. Either before or after the one
moving the code.

> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> index 121de3e04af2..e1e6d6154015 100644
> --- a/security/integrity/ima/ima_kexec.c
> +++ b/security/integrity/ima/ima_kexec.c
> @@ -10,8 +10,124 @@
>  #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 "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, "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;
> +}

The functions above were moved without being changed. Good.

> +/**
> + * 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(__pa(addr), size);

Here you added a __pa() call. Do you store a virtual address in
linux,ima-kexec-buffer property? Doesn't it make more sense to store a
physical address?

Even if making this change is the correct thing to do, it should be a
separate patch, unless it can't be avoided. And if that is the case,
then it should be explained in the commit message.

> +
> +}
> +
> +/**
> + * 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;
> +
> +	do_get_kexec_buffer(prop, len, &addr, &size);
> +	ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
> +	if (ret < 0)
> +		return;
> +
> +	memblock_free(addr, size);
> +}

Here is another function that changed when moved. This one I know to be
wrong. You're confusing the purposes of remove_ima_buffer() and
ima_free_kexec_buffer().

You did send me a question about them nearly three weeks ago which I
only answered today, so I apologize. Also, their names could more
clearly reflect their differences, so it's bad naming on my part.

With IMA kexec buffers, there are two kernels (and thus their two
respective, separate device trees) to be concerned about:

1. the currently running kernel, which uses the live device tree
(accessed with the of_* functions) and the memblock subsystem;

2. the kernel which is being loaded by kexec, which will use the FDT
blob being passed around as argument to these functions, and the memory
reservations in the memory reservation table of the FDT blob.

ima_free_kexec_buffer() is used by IMA in the currently running kernel.
Therefore the device tree it is concerned about is the live one, and
thus uses the of_* functions to access it. And uses memblock to change
the memory reservation.

remove_ima_buffer() on the other hand is used by the kexec code to
prepare an FDT blob for the kernel that is being loaded. It should not
make any changes to live device tree, nor to memblock allocations. It
should only make changes to the FDT blob.

--
Thiago Jung Bauermann
IBM Linux Technology Center