| Message ID | 20190913225009.3406-2-prsriva@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for arm64 to carry ima measurement log in kexec_file_load | expand |
Hi Prahkar, On Fri, 2019-09-13 at 15:50 -0700, Prakhar Srivastava wrote: > During kexec_file_load, carrying forward the ima measurement log allows > a verifying party to get the entire runtime event log since the last > full reboot since that is when PCRs were last reset. > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> Missing is the Changelog between versions. (v1 has now been posted 3 times.) There's a number of trailing whitespaces, blanks before tabs, and other warnings. Please use scripts/checkpatch.pl before posting patches. > --- > arch/arm64/Kconfig | 7 + > arch/arm64/include/asm/ima.h | 29 ++++ > arch/arm64/include/asm/kexec.h | 5 + > arch/arm64/kernel/Makefile | 3 +- > arch/arm64/kernel/ima_kexec.c | 213 +++++++++++++++++++++++++ > arch/arm64/kernel/machine_kexec_file.c | 6 + > 6 files changed, 262 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/include/asm/ima.h > create mode 100644 arch/arm64/kernel/ima_kexec.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 3adcec05b1f6..f39b12dbf9e8 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -976,6 +976,13 @@ config KEXEC_VERIFY_SIG > verification for the corresponding kernel image type being > loaded in order for this to work. > > +config HAVE_IMA_KEXEC > + bool "Carry over IMA measurement log during kexec_file_load() syscall" > + depends on KEXEC_FILE > + help > + Select this option to carry over IMA measurement log during > + kexec_file_load. > + > config KEXEC_IMAGE_VERIFY_SIG > bool "Enable Image signature verification support" > default y > diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h > new file mode 100644 > index 000000000000..e23cee84729f > --- /dev/null > +++ b/arch/arm64/include/asm/ima.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_ARM64_IMA_H > +#define _ASM_ARM64_IMA_H > + > +struct kimage; > + > +int ima_get_kexec_buffer(void **addr, size_t *size); > +int ima_free_kexec_buffer(void); > + > +#ifdef CONFIG_IMA > +void remove_ima_buffer(void *fdt, int chosen_node); > +#else > +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} > +#endif > + > +#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, > + int chosen_node) > +{ > + remove_ima_buffer(fdt, chosen_node); > + return 0; > +} > +#endif /* CONFIG_IMA_KEXEC */ > +#endif /* _ASM_ARM64_IMA_H */ > diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h > index 12a561a54128..e8d2412066e7 100644 > --- a/arch/arm64/include/asm/kexec.h > +++ b/arch/arm64/include/asm/kexec.h > @@ -96,6 +96,11 @@ static inline void crash_post_resume(void) {} > struct kimage_arch { > void *dtb; > unsigned long dtb_mem; > + > +#ifdef CONFIG_IMA_KEXEC > + phys_addr_t ima_buffer_addr; > + size_t ima_buffer_size; > +#endif > }; > > extern const struct kexec_file_ops kexec_image_ops; > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile > index 478491f07b4f..580238f2e9a7 100644 > --- a/arch/arm64/kernel/Makefile > +++ b/arch/arm64/kernel/Makefile > @@ -55,7 +55,8 @@ obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o > obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o > obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \ > cpu-reset.o > -obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o > +obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o \ > + ima_kexec.o The kernel may be built with/without IMA enabled or without IMA_KEXEC support. Here you're requiring both IMA and IMA_KEXEC to be configured. Please refer to the powerpc example. > obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o > arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o > obj-$(CONFIG_CRASH_DUMP) += crash_dump.o > diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c > new file mode 100644 > index 000000000000..b14326d541f3 > --- /dev/null > +++ b/arch/arm64/kernel/ima_kexec.c > @@ -0,0 +1,213 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2019 Microsoft Corporation. > + * > + * Authors: > + * Prakhar Srivastava <prsriva@linux.microsoft.com> > + */ > + > +#include <linux/slab.h> > +#include <linux/kexec.h> > +#include <linux/of.h> > +#include <linux/memblock.h> > +#include <linux/libfdt.h> > + > + Extraneous blank line. > +/** > + * delete_fdt_mem_rsv - delete memory reservation with given address and size > + * @fdt - pointer to the fdt. > + * @start - start address of the memory. > + * @size - number of cells to be deletd. > + * > + * Return: 0 on success, or negative errno on error. > + */ > +int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size) > +{ > + int i, ret, num_rsvs = fdt_num_mem_rsv(fdt); > + > + for (i = 0; i < num_rsvs; i++) { > + uint64_t rsv_start, rsv_size; > + > + ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size); > + if (ret) { > + pr_err("Malformed device tree\n"); > + return -EINVAL; > + } > + > + if (rsv_start == start && rsv_size == size) { > + ret = fdt_del_mem_rsv(fdt, i); > + if (ret) { > + pr_err("Error deleting device tree reservation\n"); > + return -EINVAL; > + } > + > + return 0; > + } > + } > + > + return -ENOENT; > +} > + > +/** > + * remove_ima_buffer - remove the IMA buffer property and reservation > + * @fdt - pointer the fdt. > + * @chosen_node - node under which property can be found. > + * > + * The IMA measurement buffer is either read by now and freeed or a kexec call > + * needs to replace the ima measurement buffer, clear the property and memory > + * reservation. > + */ > +void remove_ima_buffer(void *fdt, int chosen_node) > +{ > + int ret, len; > + const void *prop; > + uint64_t tmp_start, tmp_end; > + > + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); > + if (prop) { > + tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop)); > + > + prop = fdt_getprop(fdt, chosen_node, > + "linux,ima-kexec-buffer-end", &len); > + if (!prop) > + return; > + > + tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop)); > + > + ret = delete_fdt_mem_rsv(fdt, tmp_start, tmp_end - tmp_start); > + > + if (ret == 0) > + pr_debug("Removed old IMA buffer reservation.\n"); > + else if (ret != -ENOENT) > + return; > + > + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); > + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer-end"); > + } > +} > + > +/** > + * 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 len; > + const void *prop; > + uint64_t tmp_start, tmp_end; > + > + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); > + if (!prop) > + return -ENOENT; > + > + tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop)); > + > + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer-end", &len); > + if (!prop) > + return -ENOENT; > + > + tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop)); > + > + *addr = __va(tmp_start); > + *size = tmp_end - tmp_start; > + > + return 0; > +} > + > +/** > + * ima_free_kexec_buffer - free memory used by the IMA buffer > + * > + * Return: 0 on success, negative errno on error. > + */ > +int ima_free_kexec_buffer(void) > +{ > + int ret; > + void *propStart, *propEnd; Is there a reason for using mixed case variables? Why does the variable need to be prefixed with "prop"? Wouldn't ima_start/_stop, buffer_start/_stop, or even buf_start/_stop be fine? > + uint64_t tmp_start, tmp_end; > + > + propStart = of_find_property(of_chosen, "linux,ima-kexec-buffer", > + NULL); > + if (propStart) { > + tmp_start = fdt64_to_cpu(*((const fdt64_t *) propStart)); > + ret = of_remove_property(of_chosen, propStart); > + if (!ret) { > + return ret; > + } > + > + propEnd = of_find_property(of_chosen, > + "linux,ima-kexec-buffer-end", NULL); > + if (!propEnd) { > + return -EINVAL; > + } > + > + tmp_end = fdt64_to_cpu(*((const fdt64_t *) propEnd)); > + > + ret = of_remove_property(of_chosen, propEnd); > + if (!ret) { > + return ret; > + } There seems to be quite a bit of code duplication in this function and in ima_get_kexec_buffer(). It could probably be cleaned up with some refactoring. Mimi > + > + return memblock_free(tmp_start, tmp_end - tmp_start); > + } > + return 0; > +} > + > +#ifdef CONFIG_IMA_KEXEC > +/** > + * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA > + * measurement log. > + * @image: - pointer to the kimage, to store the address and size of the > + * IMA measurement log. > + * @load_addr: - the address where the IMA measurement log is stored. > + * @size - size of the IMA measurement log. > + * > + * 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; > +} > + > +/** > + * setup_ima_buffer - update the fdt to contain the ima mesasurement log > + * @image: - pointer to the kimage, containing the address and size of > + * the IMA measurement log. > + * @fdt: - pointer to the fdt. > + * @chosen_node: - node under which property is to be defined. > + * > + * Return: 0 on success, negative errno on error. > + */ > +int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) > +{ > + int ret; > + > + remove_ima_buffer(fdt, chosen_node); > + > + if (!image->arch.ima_buffer_size) > + return 0; > + > + ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer", > + image->arch.ima_buffer_addr); > + if (ret < 0) > + return ret; > + > + ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer-end", > + image->arch.ima_buffer_addr + > + image->arch.ima_buffer_size); > + if (ret < 0) > + return ret; > + > + ret = fdt_add_mem_rsv(fdt, image->arch.ima_buffer_addr, > + image->arch.ima_buffer_size); > + if (ret < 0) > + return ret; > + > + return 0; > +} > +#endif /* CONFIG_IMA_KEXEC */ > diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c > index 58871333737a..de5452539c67 100644 > --- a/arch/arm64/kernel/machine_kexec_file.c > +++ b/arch/arm64/kernel/machine_kexec_file.c > @@ -21,6 +21,7 @@ > #include <linux/types.h> > #include <linux/vmalloc.h> > #include <asm/byteorder.h> > +#include <asm/ima.h> > > /* relevant device tree properties */ > #define FDT_PROP_INITRD_START "linux,initrd-start" > @@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image, > goto out; > } > > + /* add ima measuremnet log buffer */ > + ret = setup_ima_buffer(image, dtb, off); > + if (ret) > + goto out; > + > /* add kaslr-seed */ > ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED); > if (ret == -FDT_ERR_NOTFOUND)
On Wed, 2019-09-18 at 10:15 -0400, Mimi Zohar wrote: > > + uint64_t tmp_start, tmp_end; > > + > > + propStart = of_find_property(of_chosen, "linux,ima-kexec-buffer", > > + NULL); > > + if (propStart) { > > + tmp_start = fdt64_to_cpu(*((const fdt64_t *) propStart)); > > + ret = of_remove_property(of_chosen, propStart); > > + if (!ret) { > > + return ret; > > + } > > + > > + propEnd = of_find_property(of_chosen, > > + "linux,ima-kexec-buffer-end", NULL); > > + if (!propEnd) { > > + return -EINVAL; > > + } > > + > > + tmp_end = fdt64_to_cpu(*((const fdt64_t *) propEnd)); > > + > > + ret = of_remove_property(of_chosen, propEnd); > > + if (!ret) { > > + return ret; > > + } > > There seems to be quite a bit of code duplication in this function and > in ima_get_kexec_buffer(). It could probably be cleaned up with some > refactoring. Sorry, my mistake. One calls of_get_property(), while the other calls of_find_property(). Mimi
Mimi Zohar <zohar@linux.ibm.com> writes: > On Wed, 2019-09-18 at 10:15 -0400, Mimi Zohar wrote: > >> > + uint64_t tmp_start, tmp_end; >> > + >> > + propStart = of_find_property(of_chosen, "linux,ima-kexec-buffer", >> > + NULL); >> > + if (propStart) { >> > + tmp_start = fdt64_to_cpu(*((const fdt64_t *) propStart)); >> > + ret = of_remove_property(of_chosen, propStart); >> > + if (!ret) { >> > + return ret; >> > + } >> > + >> > + propEnd = of_find_property(of_chosen, >> > + "linux,ima-kexec-buffer-end", NULL); >> > + if (!propEnd) { >> > + return -EINVAL; >> > + } >> > + >> > + tmp_end = fdt64_to_cpu(*((const fdt64_t *) propEnd)); >> > + >> > + ret = of_remove_property(of_chosen, propEnd); >> > + if (!ret) { >> > + return ret; >> > + } >> >> There seems to be quite a bit of code duplication in this function and >> in ima_get_kexec_buffer(). It could probably be cleaned up with some >> refactoring. > > Sorry, my mistake. One calls of_get_property(), while the other calls > of_find_property(). of_get_property() is a thin wrapper around of_find_property(), so if that's the only difference I think they can still be merged.
On 9/18/2019 8:59 PM, Thiago Jung Bauermann wrote: > Mimi Zohar <zohar@linux.ibm.com> writes: > >> On Wed, 2019-09-18 at 10:15 -0400, Mimi Zohar wrote: >> >>>> + uint64_t tmp_start, tmp_end; >>>> + >>>> + propStart = of_find_property(of_chosen, "linux,ima-kexec-buffer", >>>> + NULL); >>>> + if (propStart) { >>>> + tmp_start = fdt64_to_cpu(*((const fdt64_t *) propStart)); >>>> + ret = of_remove_property(of_chosen, propStart); >>>> + if (!ret) { >>>> + return ret; >>>> + } >>>> + >>>> + propEnd = of_find_property(of_chosen, >>>> + "linux,ima-kexec-buffer-end", NULL); >>>> + if (!propEnd) { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + tmp_end = fdt64_to_cpu(*((const fdt64_t *) propEnd)); >>>> + >>>> + ret = of_remove_property(of_chosen, propEnd); >>>> + if (!ret) { >>>> + return ret; >>>> + } >>> There seems to be quite a bit of code duplication in this function and >>> in ima_get_kexec_buffer(). It could probably be cleaned up with some >>> refactoring. >> Sorry, my mistake. One calls of_get_property(), while the other calls >> of_find_property(). > of_get_property() is a thin wrapper around of_find_property(), so if > that's the only difference I think they can still be merged. I will move to using of_get_property and see if i can reduce the code further. Also address other comments by Mimi in my next iteration. Thanks, Prakhar Srivastava
Hello Prakhar, Prakhar Srivastava <prsriva@linux.microsoft.com> writes: > During kexec_file_load, carrying forward the ima measurement log allows > a verifying party to get the entire runtime event log since the last > full reboot since that is when PCRs were last reset. > > Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> > --- > arch/arm64/Kconfig | 7 + > arch/arm64/include/asm/ima.h | 29 ++++ > arch/arm64/include/asm/kexec.h | 5 + > arch/arm64/kernel/Makefile | 3 +- > arch/arm64/kernel/ima_kexec.c | 213 +++++++++++++++++++++++++ > arch/arm64/kernel/machine_kexec_file.c | 6 + > 6 files changed, 262 insertions(+), 1 deletion(-) > create mode 100644 arch/arm64/include/asm/ima.h > create mode 100644 arch/arm64/kernel/ima_kexec.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index 3adcec05b1f6..f39b12dbf9e8 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -976,6 +976,13 @@ config KEXEC_VERIFY_SIG > verification for the corresponding kernel image type being > loaded in order for this to work. > > +config HAVE_IMA_KEXEC > + bool "Carry over IMA measurement log during kexec_file_load() syscall" > + depends on KEXEC_FILE > + help > + Select this option to carry over IMA measurement log during > + kexec_file_load. > + > config KEXEC_IMAGE_VERIFY_SIG > bool "Enable Image signature verification support" > default y This is not right. As it stands, HAVE_IMA_KEXEC is essentially a synonym for IMA_KEXEC. It's not meant to be user-visible in the config process. Instead, it's meant to be selected by the arch Kconfig (probably by the ARM64 config symbol) to signal to IMA's Kconfig that it can offer the IMA_KEXEC option. I also mentioned in my previous review that config HAVE_IMA_KEXEC should be defined in arch/Kconfig, not separately in both arch/arm64/Kconfig and arch/powerpc/Kconfig. > diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h > new file mode 100644 > index 000000000000..e23cee84729f > --- /dev/null > +++ b/arch/arm64/include/asm/ima.h > @@ -0,0 +1,29 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_ARM64_IMA_H > +#define _ASM_ARM64_IMA_H > + > +struct kimage; > + > +int ima_get_kexec_buffer(void **addr, size_t *size); > +int ima_free_kexec_buffer(void); > + > +#ifdef CONFIG_IMA > +void remove_ima_buffer(void *fdt, int chosen_node); > +#else > +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} > +#endif I mentioned in my previous review that remove_ima_buffer() should exist even if CONFIG_IMA isn't set. Did you arrive at a different conclusion? > + > +#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, > + int chosen_node) > +{ > + remove_ima_buffer(fdt, chosen_node); > + return 0; > +} > +#endif /* CONFIG_IMA_KEXEC */ > +#endif /* _ASM_ARM64_IMA_H */ > diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c > new file mode 100644 > index 000000000000..b14326d541f3 > --- /dev/null > +++ b/arch/arm64/kernel/ima_kexec.c In the previous patch, you took the powerpc file and made a few modifications to fit your needs. This file is now somewhat different than the powerpc version, but I don't understand to what purpose. It's not different in any significant way. Based on review comments from your previous patch, I was expecting to see code from the powerpc file moved to an arch-independent part of the the kernel and possibly adapted so that both arm64 and powerpc could use it. Can you explain why you chose this approach instead? What is the advantage of having superficially different but basically equivalent code in the two architectures? Actually, there's one change that is significant: instead of a single linux,ima-kexec-buffer property holding the start address and size of the buffer, ARM64 is now using two properties (linux,ima-kexec-buffer and linux,ima-kexec-buffer-end) for the start and end addresses. In my opinion, unless there's a good reason for it Linux should be consistent accross architectures when possible. -- Thiago Jung Bauermann IBM Linux Technology Center
On 9/19/19 8:07 PM, Thiago Jung Bauermann wrote: > Hello Prakhar, > > Prakhar Srivastava <prsriva@linux.microsoft.com> writes: > >> During kexec_file_load, carrying forward the ima measurement log allows >> a verifying party to get the entire runtime event log since the last >> full reboot since that is when PCRs were last reset. >> >> Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> >> --- >> arch/arm64/Kconfig | 7 + >> arch/arm64/include/asm/ima.h | 29 ++++ >> arch/arm64/include/asm/kexec.h | 5 + >> arch/arm64/kernel/Makefile | 3 +- >> arch/arm64/kernel/ima_kexec.c | 213 +++++++++++++++++++++++++ >> arch/arm64/kernel/machine_kexec_file.c | 6 + >> 6 files changed, 262 insertions(+), 1 deletion(-) >> create mode 100644 arch/arm64/include/asm/ima.h >> create mode 100644 arch/arm64/kernel/ima_kexec.c >> >> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >> index 3adcec05b1f6..f39b12dbf9e8 100644 >> --- a/arch/arm64/Kconfig >> +++ b/arch/arm64/Kconfig >> @@ -976,6 +976,13 @@ config KEXEC_VERIFY_SIG >> verification for the corresponding kernel image type being >> loaded in order for this to work. >> >> +config HAVE_IMA_KEXEC >> + bool "Carry over IMA measurement log during kexec_file_load() syscall" >> + depends on KEXEC_FILE >> + help >> + Select this option to carry over IMA measurement log during >> + kexec_file_load. >> + >> config KEXEC_IMAGE_VERIFY_SIG >> bool "Enable Image signature verification support" >> default y > This is not right. As it stands, HAVE_IMA_KEXEC is essentially a synonym > for IMA_KEXEC. > > It's not meant to be user-visible in the config process. Instead, it's > meant to be selected by the arch Kconfig (probably by the ARM64 config > symbol) to signal to IMA's Kconfig that it can offer the IMA_KEXEC > option. > > I also mentioned in my previous review that config HAVE_IMA_KEXEC should > be defined in arch/Kconfig, not separately in both arch/arm64/Kconfig > and arch/powerpc/Kconfig. I see the entry exists in arch/Kconfig and is overwritten. I will remove entries both from powerpc and arm64. How do i cross-compile for powerpc? > >> diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h >> new file mode 100644 >> index 000000000000..e23cee84729f >> --- /dev/null >> +++ b/arch/arm64/include/asm/ima.h >> @@ -0,0 +1,29 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +#ifndef _ASM_ARM64_IMA_H >> +#define _ASM_ARM64_IMA_H >> + >> +struct kimage; >> + >> +int ima_get_kexec_buffer(void **addr, size_t *size); >> +int ima_free_kexec_buffer(void); >> + >> +#ifdef CONFIG_IMA >> +void remove_ima_buffer(void *fdt, int chosen_node); >> +#else >> +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} >> +#endif > I mentioned in my previous review that remove_ima_buffer() should exist > even if CONFIG_IMA isn't set. Did you arrive at a different conclusion? I made the needed changed in makefile, missed removing the configs here. Thanks for pointing this out. >> + >> +#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, >> + int chosen_node) >> +{ >> + remove_ima_buffer(fdt, chosen_node); >> + return 0; >> +} >> +#endif /* CONFIG_IMA_KEXEC */ >> +#endif /* _ASM_ARM64_IMA_H */ >> diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c >> new file mode 100644 >> index 000000000000..b14326d541f3 >> --- /dev/null >> +++ b/arch/arm64/kernel/ima_kexec.c > In the previous patch, you took the powerpc file and made a few > modifications to fit your needs. This file is now somewhat different > than the powerpc version, but I don't understand to what purpose. It's > not different in any significant way. > > Based on review comments from your previous patch, I was expecting to > see code from the powerpc file moved to an arch-independent part of the > the kernel and possibly adapted so that both arm64 and powerpc could use > it. Can you explain why you chose this approach instead? What is the > advantage of having superficially different but basically equivalent > code in the two architectures? > > Actually, there's one change that is significant: instead of a single > linux,ima-kexec-buffer property holding the start address and size of > the buffer, ARM64 is now using two properties (linux,ima-kexec-buffer > and linux,ima-kexec-buffer-end) for the start and end addresses. In my > opinion, unless there's a good reason for it Linux should be consistent > accross architectures when possible. > > -- > Thiago Jung Bauermann > IBM Linux Technology Center > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hello, prsriva <prsriva@linux.microsoft.com> writes: > On 9/19/19 8:07 PM, Thiago Jung Bauermann wrote: >> Hello Prakhar, >> >> Prakhar Srivastava <prsriva@linux.microsoft.com> writes: >> >>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig >>> index 3adcec05b1f6..f39b12dbf9e8 100644 >>> --- a/arch/arm64/Kconfig >>> +++ b/arch/arm64/Kconfig >>> @@ -976,6 +976,13 @@ config KEXEC_VERIFY_SIG >>> verification for the corresponding kernel image type being >>> loaded in order for this to work. >>> >>> +config HAVE_IMA_KEXEC >>> + bool "Carry over IMA measurement log during kexec_file_load() syscall" >>> + depends on KEXEC_FILE >>> + help >>> + Select this option to carry over IMA measurement log during >>> + kexec_file_load. >>> + >>> config KEXEC_IMAGE_VERIFY_SIG >>> bool "Enable Image signature verification support" >>> default y >> This is not right. As it stands, HAVE_IMA_KEXEC is essentially a synonym >> for IMA_KEXEC. >> >> It's not meant to be user-visible in the config process. Instead, it's >> meant to be selected by the arch Kconfig (probably by the ARM64 config >> symbol) to signal to IMA's Kconfig that it can offer the IMA_KEXEC >> option. >> >> I also mentioned in my previous review that config HAVE_IMA_KEXEC should >> be defined in arch/Kconfig, not separately in both arch/arm64/Kconfig >> and arch/powerpc/Kconfig. > > I see the entry exists in arch/Kconfig and is overwritten. > I will remove entries both from powerpc and arm64. > > How do i cross-compile for powerpc? There are some instructions here: https://github.com/linuxppc/wiki/wiki/Building-powerpc-kernels >>> diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h >>> new file mode 100644 >>> index 000000000000..e23cee84729f >>> --- /dev/null >>> +++ b/arch/arm64/include/asm/ima.h >>> @@ -0,0 +1,29 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +#ifndef _ASM_ARM64_IMA_H >>> +#define _ASM_ARM64_IMA_H >>> + >>> +struct kimage; >>> + >>> +int ima_get_kexec_buffer(void **addr, size_t *size); >>> +int ima_free_kexec_buffer(void); >>> + >>> +#ifdef CONFIG_IMA >>> +void remove_ima_buffer(void *fdt, int chosen_node); >>> +#else >>> +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} >>> +#endif >> I mentioned in my previous review that remove_ima_buffer() should exist >> even if CONFIG_IMA isn't set. Did you arrive at a different conclusion? > > I made the needed changed in makefile, missed removing the > > configs here. Thanks for pointing this out. Thanks.
On 9/19/19 8:07 PM, Thiago Jung Bauermann wrote: > > Hello Prakhar, > > Prakhar Srivastava <prsriva@linux.microsoft.com> writes: > >> During kexec_file_load, carrying forward the ima measurement log allows >> a verifying party to get the entire runtime event log since the last >> full reboot since that is when PCRs were last reset. <snip> > In the previous patch, you took the powerpc file and made a few > modifications to fit your needs. This file is now somewhat different > than the powerpc version, but I don't understand to what purpose. It's > not different in any significant way. > > Based on review comments from your previous patch, I was expecting to > see code from the powerpc file moved to an arch-independent part of the > the kernel and possibly adapted so that both arm64 and powerpc could use > it. Can you explain why you chose this approach instead? What is the > advantage of having superficially different but basically equivalent > code in the two architectures? > > Actually, there's one change that is significant: instead of a single > linux,ima-kexec-buffer property holding the start address and size of > the buffer, ARM64 is now using two properties (linux,ima-kexec-buffer > and linux,ima-kexec-buffer-end) for the start and end addresses. In my > opinion, unless there's a good reason for it Linux should be consistent > accross architectures when possible. > > -- > Thiago Jung Bauermann > IBM Linux Technology Center I looked at the of_ drivers are it became apparent that the driver calls were already available for consumption. Adding ima specific code will be same as adding wrapper code for any other property. Which is true for all properties, effectively setting the property name and pass through for other parameters. I still like to move both implementations to a arch independent code path, i could not convince my self that of_*ima is probably the place, but if that's the best place?, then i will go ahead and make that change as well. Regarding using two properties, it just seemed more consistent how the properties(start-end) are being used in the kexec, and hides the inner details for the cell structures, thats all. Its just the placement of the wrapper functions, but once thats done both archs will call the same. Thanks, Prakhar Srivastava > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 3adcec05b1f6..f39b12dbf9e8 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -976,6 +976,13 @@ config KEXEC_VERIFY_SIG verification for the corresponding kernel image type being loaded in order for this to work. +config HAVE_IMA_KEXEC + bool "Carry over IMA measurement log during kexec_file_load() syscall" + depends on KEXEC_FILE + help + Select this option to carry over IMA measurement log during + kexec_file_load. + config KEXEC_IMAGE_VERIFY_SIG bool "Enable Image signature verification support" default y diff --git a/arch/arm64/include/asm/ima.h b/arch/arm64/include/asm/ima.h new file mode 100644 index 000000000000..e23cee84729f --- /dev/null +++ b/arch/arm64/include/asm/ima.h @@ -0,0 +1,29 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _ASM_ARM64_IMA_H +#define _ASM_ARM64_IMA_H + +struct kimage; + +int ima_get_kexec_buffer(void **addr, size_t *size); +int ima_free_kexec_buffer(void); + +#ifdef CONFIG_IMA +void remove_ima_buffer(void *fdt, int chosen_node); +#else +static inline void remove_ima_buffer(void *fdt, int chosen_node) {} +#endif + +#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, + int chosen_node) +{ + remove_ima_buffer(fdt, chosen_node); + return 0; +} +#endif /* CONFIG_IMA_KEXEC */ +#endif /* _ASM_ARM64_IMA_H */ diff --git a/arch/arm64/include/asm/kexec.h b/arch/arm64/include/asm/kexec.h index 12a561a54128..e8d2412066e7 100644 --- a/arch/arm64/include/asm/kexec.h +++ b/arch/arm64/include/asm/kexec.h @@ -96,6 +96,11 @@ static inline void crash_post_resume(void) {} struct kimage_arch { void *dtb; unsigned long dtb_mem; + +#ifdef CONFIG_IMA_KEXEC + phys_addr_t ima_buffer_addr; + size_t ima_buffer_size; +#endif }; extern const struct kexec_file_ops kexec_image_ops; diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index 478491f07b4f..580238f2e9a7 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -55,7 +55,8 @@ obj-$(CONFIG_RANDOMIZE_BASE) += kaslr.o obj-$(CONFIG_HIBERNATION) += hibernate.o hibernate-asm.o obj-$(CONFIG_KEXEC_CORE) += machine_kexec.o relocate_kernel.o \ cpu-reset.o -obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o +obj-$(CONFIG_KEXEC_FILE) += machine_kexec_file.o kexec_image.o \ + ima_kexec.o obj-$(CONFIG_ARM64_RELOC_TEST) += arm64-reloc-test.o arm64-reloc-test-y := reloc_test_core.o reloc_test_syms.o obj-$(CONFIG_CRASH_DUMP) += crash_dump.o diff --git a/arch/arm64/kernel/ima_kexec.c b/arch/arm64/kernel/ima_kexec.c new file mode 100644 index 000000000000..b14326d541f3 --- /dev/null +++ b/arch/arm64/kernel/ima_kexec.c @@ -0,0 +1,213 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Microsoft Corporation. + * + * Authors: + * Prakhar Srivastava <prsriva@linux.microsoft.com> + */ + +#include <linux/slab.h> +#include <linux/kexec.h> +#include <linux/of.h> +#include <linux/memblock.h> +#include <linux/libfdt.h> + + +/** + * delete_fdt_mem_rsv - delete memory reservation with given address and size + * @fdt - pointer to the fdt. + * @start - start address of the memory. + * @size - number of cells to be deletd. + * + * Return: 0 on success, or negative errno on error. + */ +int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size) +{ + int i, ret, num_rsvs = fdt_num_mem_rsv(fdt); + + for (i = 0; i < num_rsvs; i++) { + uint64_t rsv_start, rsv_size; + + ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size); + if (ret) { + pr_err("Malformed device tree\n"); + return -EINVAL; + } + + if (rsv_start == start && rsv_size == size) { + ret = fdt_del_mem_rsv(fdt, i); + if (ret) { + pr_err("Error deleting device tree reservation\n"); + return -EINVAL; + } + + return 0; + } + } + + return -ENOENT; +} + +/** + * remove_ima_buffer - remove the IMA buffer property and reservation + * @fdt - pointer the fdt. + * @chosen_node - node under which property can be found. + * + * The IMA measurement buffer is either read by now and freeed or a kexec call + * needs to replace the ima measurement buffer, clear the property and memory + * reservation. + */ +void remove_ima_buffer(void *fdt, int chosen_node) +{ + int ret, len; + const void *prop; + uint64_t tmp_start, tmp_end; + + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); + if (prop) { + tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop)); + + prop = fdt_getprop(fdt, chosen_node, + "linux,ima-kexec-buffer-end", &len); + if (!prop) + return; + + tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop)); + + ret = delete_fdt_mem_rsv(fdt, tmp_start, tmp_end - tmp_start); + + if (ret == 0) + pr_debug("Removed old IMA buffer reservation.\n"); + else if (ret != -ENOENT) + return; + + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer-end"); + } +} + +/** + * 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 len; + const void *prop; + uint64_t tmp_start, tmp_end; + + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer", &len); + if (!prop) + return -ENOENT; + + tmp_start = fdt64_to_cpu(*((const fdt64_t *) prop)); + + prop = of_get_property(of_chosen, "linux,ima-kexec-buffer-end", &len); + if (!prop) + return -ENOENT; + + tmp_end = fdt64_to_cpu(*((const fdt64_t *) prop)); + + *addr = __va(tmp_start); + *size = tmp_end - tmp_start; + + return 0; +} + +/** + * ima_free_kexec_buffer - free memory used by the IMA buffer + * + * Return: 0 on success, negative errno on error. + */ +int ima_free_kexec_buffer(void) +{ + int ret; + void *propStart, *propEnd; + uint64_t tmp_start, tmp_end; + + propStart = of_find_property(of_chosen, "linux,ima-kexec-buffer", + NULL); + if (propStart) { + tmp_start = fdt64_to_cpu(*((const fdt64_t *) propStart)); + ret = of_remove_property(of_chosen, propStart); + if (!ret) { + return ret; + } + + propEnd = of_find_property(of_chosen, + "linux,ima-kexec-buffer-end", NULL); + if (!propEnd) { + return -EINVAL; + } + + tmp_end = fdt64_to_cpu(*((const fdt64_t *) propEnd)); + + ret = of_remove_property(of_chosen, propEnd); + if (!ret) { + return ret; + } + + return memblock_free(tmp_start, tmp_end - tmp_start); + } + return 0; +} + +#ifdef CONFIG_IMA_KEXEC +/** + * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA + * measurement log. + * @image: - pointer to the kimage, to store the address and size of the + * IMA measurement log. + * @load_addr: - the address where the IMA measurement log is stored. + * @size - size of the IMA measurement log. + * + * 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; +} + +/** + * setup_ima_buffer - update the fdt to contain the ima mesasurement log + * @image: - pointer to the kimage, containing the address and size of + * the IMA measurement log. + * @fdt: - pointer to the fdt. + * @chosen_node: - node under which property is to be defined. + * + * Return: 0 on success, negative errno on error. + */ +int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) +{ + int ret; + + remove_ima_buffer(fdt, chosen_node); + + if (!image->arch.ima_buffer_size) + return 0; + + ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer", + image->arch.ima_buffer_addr); + if (ret < 0) + return ret; + + ret = fdt_setprop_u64(fdt, chosen_node, "linux,ima-kexec-buffer-end", + image->arch.ima_buffer_addr + + image->arch.ima_buffer_size); + if (ret < 0) + return ret; + + ret = fdt_add_mem_rsv(fdt, image->arch.ima_buffer_addr, + image->arch.ima_buffer_size); + if (ret < 0) + return ret; + + return 0; +} +#endif /* CONFIG_IMA_KEXEC */ diff --git a/arch/arm64/kernel/machine_kexec_file.c b/arch/arm64/kernel/machine_kexec_file.c index 58871333737a..de5452539c67 100644 --- a/arch/arm64/kernel/machine_kexec_file.c +++ b/arch/arm64/kernel/machine_kexec_file.c @@ -21,6 +21,7 @@ #include <linux/types.h> #include <linux/vmalloc.h> #include <asm/byteorder.h> +#include <asm/ima.h> /* relevant device tree properties */ #define FDT_PROP_INITRD_START "linux,initrd-start" @@ -85,6 +86,11 @@ static int setup_dtb(struct kimage *image, goto out; } + /* add ima measuremnet log buffer */ + ret = setup_ima_buffer(image, dtb, off); + if (ret) + goto out; + /* add kaslr-seed */ ret = fdt_delprop(dtb, off, FDT_PROP_KASLR_SEED); if (ret == -FDT_ERR_NOTFOUND)
During kexec_file_load, carrying forward the ima measurement log allows a verifying party to get the entire runtime event log since the last full reboot since that is when PCRs were last reset. Signed-off-by: Prakhar Srivastava <prsriva@linux.microsoft.com> --- arch/arm64/Kconfig | 7 + arch/arm64/include/asm/ima.h | 29 ++++ arch/arm64/include/asm/kexec.h | 5 + arch/arm64/kernel/Makefile | 3 +- arch/arm64/kernel/ima_kexec.c | 213 +++++++++++++++++++++++++ arch/arm64/kernel/machine_kexec_file.c | 6 + 6 files changed, 262 insertions(+), 1 deletion(-) create mode 100644 arch/arm64/include/asm/ima.h create mode 100644 arch/arm64/kernel/ima_kexec.c