Re: [PATCH v7 1/4] powerpc: Refactor kexec functions to move arch independent code to kernel

[Lakshmi Ramasubramanian]

On 10/22/20 8:46 PM, Thiago Jung Bauermann wrote:

Hi Thiago,

> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
>
> > On 10/20/20 8:17 PM, Mimi Zohar wrote:
> > > On Tue, 2020-10-20 at 19:25 -0700, Lakshmi Ramasubramanian wrote:
> > > > On 10/20/20 1:00 PM, Mimi Zohar wrote:
> > > > > Hi Lakshmi,
> > > > >
> > > > > On Wed, 2020-09-30 at 13:59 -0700, Lakshmi Ramasubramanian wrote:
> > > > > > The functions remove_ima_buffer() and delete_fdt_mem_rsv() that handle
> > > > > > carrying forward the IMA measurement logs on kexec for powerpc do not
> > > > > > have architecture specific code, but they are currently defined for
> > > > > > powerpc only.
> > > > > >
> > > > > > remove_ima_buffer() and delete_fdt_mem_rsv() are used to remove
> > > > > > the IMA log entry from the device tree and free the memory reserved
> > > > > > for the log. These functions need to be defined even if the current
> > > > > > kernel does not support carrying forward IMA log across kexec since
> > > > > > the previous kernel could have supported that and therefore the current
> > > > > > kernel needs to free the allocation.
> > > > > >
> > > > > > Rename remove_ima_buffer() to remove_ima_kexec_buffer().
> > > > > > Define remove_ima_kexec_buffer() and delete_fdt_mem_rsv() in kernel.
> > > > > > A later patch in this series will use these functions to free
> > > > > > the allocation, if any, made by the previous kernel for ARM64.
> > > > > >
> > > > > > Define FDT_PROP_IMA_KEXEC_BUFFER for the chosen node, namely
> > > > > > "linux,ima-kexec-buffer", that is added to the DTB to hold
> > > > > > the address and the size of the memory reserved to carry
> > > > > > the IMA measurement log.
> > > > >
> > > > > > Co-developed-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > > > > > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > > > > > Reported-by: kernel test robot <lkp@xxxxxxxxx> error: implicit declaration of function 'delete_fdt_mem_rsv' [-Werror,-Wimplicit-function-declaration]
> > > > >
> > > > > Much better!  This version limits unnecessarily changing the existing
> > > > > code to adding a couple of debugging statements, but that looks to be
> > > > > about it.
> > > > Yes Mimi - that's correct.
> > > >
> > > > > Based on Chester Lin's "ima_arch" support for arm64 discussion, the IMA generic
> > > > > EFI support will be defined in ima/ima-efi.c.  Similarly, I think it would make sense to put the generic device tree support in ima/ima_kexec_fdt.c or ima/ima_fdt.c, as opposed to kernel/.  (Refer to my comments on 2/4 about the new file named ima_kexec_fdt.c.)
> > > >
> > > > The functions remove_ima_kexec_buffer() and delete_fdt_mem_rsv(), which
> > > > are defined in kernel/ima_kexec.c and kernel/kexec_file_fdt.c
> > > > respectively, are needed even when CONFIG_IMA is not defined. These
> > > > functions need to be called by the current kernel to free the ima kexec
> > > > buffer resources allocated by the previous kernel. This is the reason,
> > > > these functions are defined under "kernel" instead of
> > > > "security/integrity/ima".
> > > >
> > > > If there is a better location to move the above C files, please let me
> > > > know. I'll move them.
> > > Freeing the previous kernel measurement list is currently called from
> > > ima_load_kexec_buffer(), only after the measurement list has been
> > > restored.  The only other time the memory is freed is when the
> > > allocated memory size isn't sufficient to hold the measurement list,
> > > which could happen if there is a delay between loading and executing
> > > the kexec.
> >
> > There are two "free" operations we need to perform with respect to ima buffer on
> > kexec:
> >
> > 1, The ima_free_kexec_buffer() called from ima_load_kexec_buffer() - the one you
> > have stated above.
> >
> > Here we remove the "ima buffer" node from the "OF" tree and free the memory
> > pages that were allocated for the measurement list.
> >
> > This one is already present in ima and there's no change in that in my patches.
> >
> > 2, The other one is remove_ima_kexec_buffer() called from setup_ima_buffer()
> > defined in "arch/powerpc/kexec/ima.c"
> >
> >   This function removes the "ima buffer" node from the "FDT" and also frees the
> > physical memory reserved for the "ima measurement list" by the previous kernel.
> >
> >   This "free" operation needs to be performed even if the current kernel does not
> > support IMA kexec since the previous kernel could have passed the IMA
> > measurement list (in FDT and reserved physical memory).
> >
> > For this reason, remove_ima_kexec_buffer() cannot be defined in "ima" but some
> > other place which will be built even if ima is not enabled. I chose to define
> > this function in "kernel" since that is guaranteed to be always built.
> >
> > thanks,
> >   -lakshmi
>
> That is true. I believe a more fitting place for these functions is
> drivers/of/fdt.c rather than these new files in kernel/. Both CONFIG_PPC
> and CONFIG_ARM64 select CONFIG_OF and CONFIG_OF_FLATTREE (indirectly,
> via CONFIG_OF_EARLY_FLATTREE) so they will both build that file.

I moved the above mentioned functions to drivers/of/fdt.c => it works.

But I am not sure if "drivers/of" is the right place - this driver is 
handling data from firmware and building FDT. I do not see any kexec 
related operations being handled by this driver in the current 
implementation.

Also, being a driver can it be loaded/unloaded on-demand? If yes, it may 
not be available when "ima kexec" calls are needed.

@Rob Herring - what do you think?

thanks,
 -lakshmi