| Message ID | 20201030174429.29893-3-nramas@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Carry forward IMA measurement log on kexec on ARM64 | expand |
Hi Lakshmi, On Fri, 2020-10-30 at 10:44 -0700, Lakshmi Ramasubramanian wrote: > The functions ima_get_kexec_buffer() and ima_free_kexec_buffer(), > 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. > > Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to IMA > subsystem. A later patch in this series will use these functions for > carrying forward the IMA measurement log for ARM64. > > With the above refactoring arch/powerpc/kexec/ima.c contains only > functions used when CONFIG_IMA_KEXEC is enabled. Update Makefile > in arch/powerpc/kexec to include arch/powerpc/kexec/ima.c only > when CONFIG_IMA_KEXEC is enabled. > > Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> Similar comments to 1/4. - Last line of first paragraph can be rephrased like " ... on kexec, do not contain architecture specific code, but are currently limited to powerpc." - This patch should be limited to moving existing functions. Truncate the Subject line to "Move arch independent IMA kexec functions to ima_kexec.c." - Don't refer to a later patch, but explain the purpose here. For example, "Move ... , making them accessible to other archs." - The definition of "FDT_PROP_IMA_KEXEC_BUFFER" should be made as a separate, prepartory patch, prior to the existing 1/4. The resulting code being moved in this patch (and similarly for 1/4) will be exactly the same as the code being deleted. thanks, Mimi
On 11/3/20 6:55 AM, Mimi Zohar wrote: Hi Mimi, > > On Fri, 2020-10-30 at 10:44 -0700, Lakshmi Ramasubramanian wrote: >> The functions ima_get_kexec_buffer() and ima_free_kexec_buffer(), >> 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. >> >> Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to IMA >> subsystem. A later patch in this series will use these functions for >> carrying forward the IMA measurement log for ARM64. >> >> With the above refactoring arch/powerpc/kexec/ima.c contains only >> functions used when CONFIG_IMA_KEXEC is enabled. Update Makefile >> in arch/powerpc/kexec to include arch/powerpc/kexec/ima.c only >> when CONFIG_IMA_KEXEC is enabled. >> >> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > Similar comments to 1/4. > - Last line of first paragraph can be rephrased like " ... on kexec, > do not contain architecture specific code, but are currently limited to > powerpc." Sure. > - This patch should be limited to moving existing functions. > Truncate the Subject line to "Move arch independent IMA kexec functions > to ima_kexec.c." Will do. > - Don't refer to a later patch, but explain the purpose here. For > example, "Move ... , making them accessible to other archs." Sure. > - The definition of "FDT_PROP_IMA_KEXEC_BUFFER" should be made as a > separate, prepartory patch, prior to the existing 1/4. The resulting > code being moved in this patch (and similarly for 1/4) will be exactly > the same as the code being deleted. Definition of FDT_PROP_IMA_KEXEC_BUFFER will be made as a preparatory patch as you'd mentioned in the comments for [PATCH 1/4]. Will split [PATCH 2/4] as listed below: PATCH #1: Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to IMA, along with deleting them in arch/powerpc/kexec/ima.c PATCH #2: Update arch/powerpc/kexec/Makefile and arch/powerpc/kexec/ima.c to compile when CONFIG_IMA_KEXEC is defined. thanks, -lakshmi
On Tue, 2020-11-03 at 11:23 -0800, Lakshmi Ramasubramanian wrote: > On 11/3/20 6:55 AM, Mimi Zohar wrote: > > Hi Mimi, > > > > > On Fri, 2020-10-30 at 10:44 -0700, Lakshmi Ramasubramanian wrote: > >> The functions ima_get_kexec_buffer() and ima_free_kexec_buffer(), > >> 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. > >> > >> Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to IMA > >> subsystem. A later patch in this series will use these functions for > >> carrying forward the IMA measurement log for ARM64. > >> > >> With the above refactoring arch/powerpc/kexec/ima.c contains only > >> functions used when CONFIG_IMA_KEXEC is enabled. Update Makefile > >> in arch/powerpc/kexec to include arch/powerpc/kexec/ima.c only > >> when CONFIG_IMA_KEXEC is enabled. > >> > >> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > >> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> > > > > Similar comments to 1/4. > > - Last line of first paragraph can be rephrased like " ... on kexec, > > do not contain architecture specific code, but are currently limited to > > powerpc." > Sure. > > > - This patch should be limited to moving existing functions. > > Truncate the Subject line to "Move arch independent IMA kexec functions > > to ima_kexec.c." > Will do. > > > - Don't refer to a later patch, but explain the purpose here. For > > example, "Move ... , making them accessible to other archs." > Sure. > > > - The definition of "FDT_PROP_IMA_KEXEC_BUFFER" should be made as a > > separate, prepartory patch, prior to the existing 1/4. The resulting > > code being moved in this patch (and similarly for 1/4) will be exactly > > the same as the code being deleted. > > Definition of FDT_PROP_IMA_KEXEC_BUFFER will be made as a preparatory > patch as you'd mentioned in the comments for [PATCH 1/4]. > > Will split [PATCH 2/4] as listed below: > > PATCH #1: Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to > IMA, along with deleting them in arch/powerpc/kexec/ima.c No, other than the comments above, this patch is fine. It moves ima_get_kexec_buffer() and ima_free_kexec_buffer() to ima_kexec.c. Mimi
On 11/3/20 11:50 AM, Mimi Zohar wrote: > On Tue, 2020-11-03 at 11:23 -0800, Lakshmi Ramasubramanian wrote: >> On 11/3/20 6:55 AM, Mimi Zohar wrote: >> >> Hi Mimi, >> >>> >>> On Fri, 2020-10-30 at 10:44 -0700, Lakshmi Ramasubramanian wrote: >>>> The functions ima_get_kexec_buffer() and ima_free_kexec_buffer(), >>>> 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. >>>> >>>> Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to IMA >>>> subsystem. A later patch in this series will use these functions for >>>> carrying forward the IMA measurement log for ARM64. >>>> >>>> With the above refactoring arch/powerpc/kexec/ima.c contains only >>>> functions used when CONFIG_IMA_KEXEC is enabled. Update Makefile >>>> in arch/powerpc/kexec to include arch/powerpc/kexec/ima.c only >>>> when CONFIG_IMA_KEXEC is enabled. >>>> >>>> Co-developed-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>> >>> Similar comments to 1/4. >>> - Last line of first paragraph can be rephrased like " ... on kexec, >>> do not contain architecture specific code, but are currently limited to >>> powerpc." >> Sure. >> >>> - This patch should be limited to moving existing functions. >>> Truncate the Subject line to "Move arch independent IMA kexec functions >>> to ima_kexec.c." >> Will do. >> >>> - Don't refer to a later patch, but explain the purpose here. For >>> example, "Move ... , making them accessible to other archs." >> Sure. >> >>> - The definition of "FDT_PROP_IMA_KEXEC_BUFFER" should be made as a >>> separate, prepartory patch, prior to the existing 1/4. The resulting >>> code being moved in this patch (and similarly for 1/4) will be exactly >>> the same as the code being deleted. >> >> Definition of FDT_PROP_IMA_KEXEC_BUFFER will be made as a preparatory >> patch as you'd mentioned in the comments for [PATCH 1/4]. >> >> Will split [PATCH 2/4] as listed below: >> >> PATCH #1: Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to >> IMA, along with deleting them in arch/powerpc/kexec/ima.c > > No, other than the comments above, this patch is fine. It moves > ima_get_kexec_buffer() and ima_free_kexec_buffer() to ima_kexec.c. > Ok - I will do the updates in the patch description only and keep the code changes as is. Thanks for clarifying. -lakshmi
diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h index 6355a85a3289..8975f5e0cab8 100644 --- a/arch/powerpc/include/asm/ima.h +++ b/arch/powerpc/include/asm/ima.h @@ -6,9 +6,6 @@ struct kimage; -int ima_get_kexec_buffer(void **addr, size_t *size); -int ima_free_kexec_buffer(void); - #ifdef CONFIG_IMA_KEXEC int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr, size_t size); diff --git a/arch/powerpc/kexec/Makefile b/arch/powerpc/kexec/Makefile index 4aff6846c772..f54a9dbff4c8 100644 --- a/arch/powerpc/kexec/Makefile +++ b/arch/powerpc/kexec/Makefile @@ -9,12 +9,7 @@ obj-$(CONFIG_PPC32) += relocate_32.o obj-$(CONFIG_KEXEC_FILE) += file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o -ifdef CONFIG_HAVE_IMA_KEXEC -ifdef CONFIG_IMA -obj-y += ima.o -endif -endif - +obj-$(CONFIG_IMA_KEXEC) += ima.o # Disable GCOV, KCOV & sanitizers in odd or sensitive code GCOV_PROFILE_core_$(BITS).o := n diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c index 2b790230ea15..4fadd8d16891 100644 --- a/arch/powerpc/kexec/ima.c +++ b/arch/powerpc/kexec/ima.c @@ -30,61 +30,6 @@ static int get_addr_size_cells(int *addr_cells, int *size_cells) return 0; } -/** - * ima_get_kexec_buffer - get IMA buffer from the previous kernel - * @addr: On successful return, set to point to the buffer contents. - * @size: On successful return, set to the buffer size. - * - * Return: 0 on success, negative errno on error. - */ -int ima_get_kexec_buffer(void **addr, size_t *size) -{ - int ret, len; - unsigned long tmp_addr; - size_t tmp_size; - const void *prop; - - prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); - if (!prop) - return -ENOENT; - - ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size); - if (ret) - return ret; - - *addr = __va(tmp_addr); - *size = tmp_size; - - return 0; -} - -/** - * ima_free_kexec_buffer - free memory used by the IMA buffer - */ -int ima_free_kexec_buffer(void) -{ - int ret; - unsigned long addr; - size_t size; - struct property *prop; - - prop = of_find_property(of_chosen, "linux,ima-kexec-buffer", NULL); - if (!prop) - return -ENOENT; - - ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size); - if (ret) - return ret; - - ret = of_remove_property(of_chosen, prop); - if (ret) - return ret; - - return memblock_free(addr, size); - -} - -#ifdef CONFIG_IMA_KEXEC /** * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer * @@ -177,4 +122,3 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) return 0; } -#endif /* CONFIG_IMA_KEXEC */ diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c index 121de3e04af2..24be285ec558 100644 --- a/security/integrity/ima/ima_kexec.c +++ b/security/integrity/ima/ima_kexec.c @@ -10,8 +10,65 @@ #include <linux/seq_file.h> #include <linux/vmalloc.h> #include <linux/kexec.h> +#include <linux/of.h> +#include <linux/memblock.h> +#include <linux/libfdt.h> +#include <linux/ima.h> #include "ima.h" +/** + * ima_get_kexec_buffer - get IMA buffer from the previous kernel + * @addr: On successful return, set to point to the buffer contents. + * @size: On successful return, set to the buffer size. + * + * Return: 0 on success, negative errno on error. + */ +static int ima_get_kexec_buffer(void **addr, size_t *size) +{ + int ret, len; + unsigned long tmp_addr; + size_t tmp_size; + const void *prop; + + prop = of_get_property(of_chosen, FDT_PROP_IMA_KEXEC_BUFFER, &len); + if (!prop) + return -ENOENT; + + ret = do_get_kexec_buffer(prop, len, &tmp_addr, &tmp_size); + if (ret) + return ret; + + *addr = __va(tmp_addr); + *size = tmp_size; + return 0; +} + +/** + * ima_free_kexec_buffer - free memory used by the IMA buffer + */ +static int ima_free_kexec_buffer(void) +{ + int ret; + unsigned long addr; + size_t size; + struct property *prop; + + prop = of_find_property(of_chosen, FDT_PROP_IMA_KEXEC_BUFFER, NULL); + if (!prop) + return -ENOENT; + + ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size); + if (ret) + return ret; + + ret = of_remove_property(of_chosen, prop); + if (ret) + return ret; + + return memblock_free(addr, size); +} + + #ifdef CONFIG_IMA_KEXEC static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, unsigned long segment_size)