Re: [PATCH v10 7/8] powerpc: Move arch_ima_add_kexec_buffer to ima

[Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>]

On 12/5/20 1:36 PM, Thiago Jung Bauermann wrote:
> Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx> writes:
>
> > arch_ima_add_kexec_buffer() sets the address and size of the IMA
> > measurement log in the architecture specific field in struct kimage.
> > This function does not have architecture specific code, but is
> > currently limited to powerpc.
> >
> > Move arch_ima_add_kexec_buffer() to
> > security/integrity/ima/ima_kexec.c so that it is accessible for
> > other architectures as well.
> >
> > Signed-off-by: Lakshmi Ramasubramanian <nramas@xxxxxxxxxxxxxxxxxxx>
>
> Not sure if the maintainers will agree with me (see below), but FWIW:
>
> Reviewed-by: Thiago Jung Bauermann <bauerman@xxxxxxxxxxxxx>
>
> > ---
> >   arch/powerpc/include/asm/ima.h     |  3 ---
> >   arch/powerpc/kexec/ima.c           | 21 ---------------------
> >   security/integrity/ima/ima_kexec.c | 22 ++++++++++++++++++++++
> >   3 files changed, 22 insertions(+), 24 deletions(-)
> >
> > diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
> > index d8444d27f0d8..d6ab5d944dcd 100644
> > --- a/arch/powerpc/include/asm/ima.h
> > +++ b/arch/powerpc/include/asm/ima.h
> > @@ -7,9 +7,6 @@
> >   struct kimage;
> >   
> >   #ifdef CONFIG_IMA_KEXEC
> > -int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
> > -			      size_t size);
> > -
> >   int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
> >   #else
> >   static inline int setup_ima_buffer(const struct kimage *image, void *fdt,
> > diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
> > index bf7084c0c4da..b2793be353a9 100644
> > --- a/arch/powerpc/kexec/ima.c
> > +++ b/arch/powerpc/kexec/ima.c
> > @@ -13,27 +13,6 @@
> >   #include <linux/libfdt.h>
> >   #include <asm/ima.h>
> >   
> > -/**
> > - * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> > - *
> > - * @image: kimage struct to set IMA buffer data
> > - * @load_addr: Starting address where IMA buffer is loaded at
> > - * @size: Number of bytes in the IMA buffer
> > - *
> > - * Architectures should use this function to pass on the IMA buffer
> > - * information to the next kernel.
> > - *
> > - * Return: 0 on success, negative errno on error.
> > - */
> > -int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
> > -			      size_t size)
> > -{
> > -	image->arch.ima_buffer_addr = load_addr;
> > -	image->arch.ima_buffer_size = size;
> > -
> > -	return 0;
> > -}
> > -
> >   static int write_number(void *p, u64 value, int cells)
> >   {
> >   	if (cells == 1) {
> > diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c
> > index 4d354593aecf..5263dafe8f4d 100644
> > --- a/security/integrity/ima/ima_kexec.c
> > +++ b/security/integrity/ima/ima_kexec.c
> > @@ -74,6 +74,28 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
> >   	return ret;
> >   }
> >   
> > +/**
> > + * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
> > + *
> > + * @image: kimage struct to set IMA buffer data
> > + * @load_addr: Starting address where IMA buffer is loaded at
> > + * @size: Number of bytes in the IMA buffer
> > + *
> > + * Architectures should use this function to pass on the IMA buffer
> > + * information to the next kernel.
> > + *
> > + * Return: 0 on success, negative errno on error.
> > + */
> > +static int arch_ima_add_kexec_buffer(struct kimage *image,
> > +				     unsigned long load_addr,
> > +				     size_t size)
> > +{
> > +	image->arch.ima_buffer_addr = load_addr;
> > +	image->arch.ima_buffer_size = size;
> > +
> > +	return 0;
> > +}
> > +
>
> Both powerpc and arm64 use the definition above for
> arch_ima_add_kexec_buffer(), so it makes sense to share them as you do
> in this patch. This file isn't the best one to put arch-specific code
> which happens to be identical among architectures, but I can't think of
> somewhere else to put it.
>
> For now this isn't an issue since powerpc and arm64 are the only arches
> implementing tihs feature. If a third arch implemented it and also used
> the same function definition as above, it wouldn't be an issue either so
> perhaps this is good enough for the time being? :-)

If Mimi doesn't have any objection, I'll leave this function in this 
file. The other option is to move this function also to 
"drivers/of/kexec.c".

Please let me know.

> With this patch, the `#include <asm/ima.h>` in
> security/integrity/ima/ima.h can be removed. It was there just to
> provide a declaration of arch_ima_add_kexec_buffer().

Sure - will remove this #include.

thanks,
 -lakshmi