Re: [PATCH v5 3/3] arm64: Add IMA kexec buffer to DTB

[Lakshmi Ramasubramanian]

On 9/3/20 3:11 PM, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
>
> > The address and size of the current kernel's IMA measurement log
> > need to be added to the device tree's IMA kexec buffer node for
> > the log to be carried over to the next kernel on the kexec call.
> >
> > Add the IMA measurement log buffer properties to the device tree for
> > ARM64 and reserve the memory for storing the IMA log.
> > Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC to
> > indicate that the IMA measurement log information is present in
> > the device tree.
> >
> > Co-developed-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Prakhar Srivastava <prsriva@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
> > ---
> >   arch/arm64/Kconfig                     |  1 +
> >   arch/arm64/kernel/machine_kexec_file.c | 15 +++++++++++++++
> >   2 files changed, 16 insertions(+)
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 6d232837cbee..9f03c8245e5b 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -1077,6 +1077,7 @@ config KEXEC
> >   config KEXEC_FILE
> >   	bool "kexec file based system call"
> >   	select KEXEC_CORE
> > +	select HAVE_IMA_KEXEC
> >   	help
> >   	  This is new version of kexec system call. This system call is
> >   	  file based and takes file descriptors as system call argument
> > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c
> > index 361a1143e09e..0fe3d629eefe 100644
> > --- a/arch/arm64/kernel/machine_kexec_file.c
> > +++ b/arch/arm64/kernel/machine_kexec_file.c
> > @@ -136,6 +136,21 @@ static int setup_dtb(struct kimage *image,
> >   				FDT_PROP_KASLR_SEED);
> >   	}
> >   
> > +	/* add ima-kexec-buffer */
> > +	if (image->arch.ima_buffer_size > 0) {
> > +		ret = fdt_appendprop_addrrange(dtb, 0, off,
> > +				FDT_PROP_IMA_KEXEC_BUFFER,
> > +				image->arch.ima_buffer_addr,
> > +				image->arch.ima_buffer_size);
> > +		if (ret)
> > +			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
> > +
> > +		ret = fdt_add_mem_rsv(dtb, image->arch.ima_buffer_addr,
> > +				      image->arch.ima_buffer_size);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> >   	/* add rng-seed */
> >   	if (rng_is_initialized()) {
> >   		void *rng_seed;
>
> I just noticed one more thing this code isn't doing compared to the
> powerpc version (sorry to bring these issues piecemeal, I didn't realize
> this before):
>
> You're not checking whether there already is a device tree property and
> corresponding memory reservation for an IMA kexec buffer that the
> currently running kernel might have received from a previous kernel. In
> that case, this code will do the wrong thing because
> fdt_appendprop_addrrange() will append the range to the existing
> property, which is not what you want. You'll also have a memory
> reservation entry for a stale IMA kexec buffer, which just wastes
> memory.
>
> So one thing you need to do, whether or not there's an IMA kexec buffer
> to be passed to the next kernel, is to remove any existing
> FDT_PROP_IMA_KEXEC_BUFFER property and also its corresponding memory
> reservation, so that you avoid accumulating stale memory reservations
> for non-existing IMA kexec buffers from previous kexecs.

That's good catch Thiago. Thanks for the feedback. I'll make this change 
and update.

thanks,
 -lakshmi