Date:   Wed, 21 Oct 2020 09:58:36 -0700
From:   Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To:     Mimi Zohar <zohar@...ux.ibm.com>, bauerman@...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
Cc:     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 v7 1/4] powerpc: Refactor kexec functions to move arch
 independent code to kernel

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@...ux.microsoft.com>
>>>> Signed-off-by: Prakhar Srivastava <prsriva@...ux.microsoft.com>
>>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
>>>> Reported-by: kernel test robot <lkp@...el.com> 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

Powered by blists - more mailing lists