| Message ID | 20201204195149.611-8-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Carry forward IMA measurement log on kexec on ARM64 | expand |
Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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@linux.microsoft.com> Not sure if the maintainers will agree with me (see below), but FWIW: Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > --- > 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? :-) 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().
On 12/5/20 1:36 PM, Thiago Jung Bauermann wrote: > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> 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@linux.microsoft.com> > > Not sure if the maintainers will agree with me (see below), but FWIW: > > Reviewed-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> > >> --- >> 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
On Sun, 2020-12-06 at 18:03 -0800, Lakshmi Ramasubramanian wrote: > On 12/5/20 1:36 PM, Thiago Jung Bauermann wrote: > > > > Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes: > > >> 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. Defining arch_ima_add_kexec_buffer() function, as static, here in ima_kexec.c is weird. For this reason, I specifically asked Thiago to look at it. Thanks, Thiago, for looking and reviewing the rest of the patch set. Duplicating the code or defining it in drivers/of, doesn't make sense either. As Thiago suggested, define it here until there is a reason to move it. thanks, Mimi
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; +} + /* * Called during kexec_file_load so that IMA can add a segment to the kexec * image for the measurement list for the next kernel.
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@linux.microsoft.com> --- 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(-)