| Message ID | 20210204164135.29856-1-nramas@linux.microsoft.com |
|---|---|
| Headers | show |
| Series | Carry forward IMA measurement log on kexec on ARM64 | expand |
On 2/4/21 11:26 AM, Rob Herring wrote: > On Thu, Feb 4, 2021 at 10:42 AM Lakshmi Ramasubramanian > <nramas@linux.microsoft.com> wrote: >> >> of_alloc_and_init_fdt() and of_free_fdt() have been defined in >> drivers/of/kexec.c to allocate and free memory for FDT. >> >> Use of_alloc_and_init_fdt() and of_free_fdt() to allocate and >> initialize the FDT, and to free the FDT respectively. >> >> powerpc sets the FDT address in image_loader_data field in >> "struct kimage" and the memory is freed in >> kimage_file_post_load_cleanup(). This cleanup function uses kfree() >> to free the memory. But since of_alloc_and_init_fdt() uses kvmalloc() >> for allocation, the buffer needs to be freed using kvfree(). > > You could just change the kexec core to call kvfree() instead. > >> Define "fdt" field in "struct kimage_arch" for powerpc to store >> the address of FDT, and free the memory in powerpc specific >> arch_kimage_file_post_load_cleanup(). > > However, given all the other buffers have an explicit field in kimage > or kimage_arch, changing powerpc is to match arm64 is better IMO. Just to be clear: I'll leave this as is - free FDT buffer in powerpc's arch_kimage_file_post_load_cleanup() to match arm64 behavior. Will not change "kexec core" to call kvfree() - doing that change would require changing all architectures to use kvmalloc() for image_loader_data allocation. > >> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >> Suggested-by: Rob Herring <robh@kernel.org> >> Suggested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >> --- >> arch/powerpc/include/asm/kexec.h | 2 ++ >> arch/powerpc/kexec/elf_64.c | 26 ++++++++++++++++---------- >> arch/powerpc/kexec/file_load_64.c | 3 +++ >> 3 files changed, 21 insertions(+), 10 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h >> index 2c0be93d239a..d7d13cac4d31 100644 >> --- a/arch/powerpc/include/asm/kexec.h >> +++ b/arch/powerpc/include/asm/kexec.h >> @@ -111,6 +111,8 @@ struct kimage_arch { >> unsigned long elf_headers_mem; >> unsigned long elf_headers_sz; >> void *elf_headers; >> + >> + void *fdt; >> }; >> >> char *setup_kdump_cmdline(struct kimage *image, char *cmdline, >> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c >> index d0e459bb2f05..51d2d8eb6c1b 100644 >> --- a/arch/powerpc/kexec/elf_64.c >> +++ b/arch/powerpc/kexec/elf_64.c >> @@ -19,6 +19,7 @@ >> #include <linux/kexec.h> >> #include <linux/libfdt.h> >> #include <linux/module.h> >> +#include <linux/of.h> >> #include <linux/of_fdt.h> >> #include <linux/slab.h> >> #include <linux/types.h> >> @@ -32,7 +33,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >> unsigned int fdt_size; >> unsigned long kernel_load_addr; >> unsigned long initrd_load_addr = 0, fdt_load_addr; >> - void *fdt; >> + void *fdt = NULL; >> const void *slave_code; >> struct elfhdr ehdr; >> char *modified_cmdline = NULL; >> @@ -103,18 +104,12 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >> } >> >> fdt_size = fdt_totalsize(initial_boot_params) * 2; >> - fdt = kmalloc(fdt_size, GFP_KERNEL); >> + fdt = of_alloc_and_init_fdt(fdt_size); >> if (!fdt) { >> pr_err("Not enough memory for the device tree.\n"); >> ret = -ENOMEM; >> goto out; >> } >> - ret = fdt_open_into(initial_boot_params, fdt, fdt_size); >> - if (ret < 0) { >> - pr_err("Error setting up the new device tree.\n"); >> - ret = -EINVAL; >> - goto out; >> - } >> >> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr, > > The first thing this function does is call setup_new_fdt() which first > calls of_kexec_setup_new_fdt(). (Note, I really don't understand the > PPC code split. It looks like there's a 32-bit and 64-bit split, but > 32-bit looks broken to me. Nothing ever calls setup_new_fdt() except > setup_new_fdt_ppc64()). The arm64 version is calling > of_alloc_and_init_fdt() and then of_kexec_setup_new_fdt() directly. > > So we can just make of_alloc_and_init_fdt() also call > of_kexec_setup_new_fdt() (really, just tweak of_kexec_setup_new_fdt do > the alloc and copy). ok - will move fdt allocation into of_kexec_setup_new_fdt(). I don't think the architecture needs to pick the > size either. It's doubtful that either one is that sensitive to the > amount of extra space. I am not clear about the above comment - are you saying the architectures don't need to pass FDT size to the alloc function? arm64 is adding command line string length and some extra space to the size computed from initial_boot_params for FDT Size: buf_size = fdt_totalsize(initial_boot_params) + cmdline_len + DTB_EXTRA_SPACE; powerpc is just using twice the size computed from initial_boot_params fdt_size = fdt_totalsize(initial_boot_params) * 2; I think it would be safe to let arm64 and powerpc pass the required FDT size, along with the other params to of_kexec_setup_new_fdt() - and in this function we allocate FDT and set it up. And, for powerpc leave the remaining code in setup_new_fdt_ppc64(). Would that be ok? > >> initrd_len, cmdline); >> @@ -131,6 +126,10 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >> ret = kexec_add_buffer(&kbuf); >> if (ret) >> goto out; >> + >> + /* FDT will be freed in arch_kimage_file_post_load_cleanup */ >> + image->arch.fdt = fdt; >> + >> fdt_load_addr = kbuf.mem; >> >> pr_debug("Loaded device tree at 0x%lx\n", fdt_load_addr); >> @@ -145,8 +144,15 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >> kfree(modified_cmdline); >> kexec_free_elf_info(&elf_info); >> >> - /* Make kimage_file_post_load_cleanup free the fdt buffer for us. */ >> - return ret ? ERR_PTR(ret) : fdt; >> + /* >> + * Once FDT buffer has been successfully passed to kexec_add_buffer(), >> + * the FDT buffer address is saved in image->arch.fdt. In that case, >> + * the memory cannot be freed here in case of any other error. >> + */ >> + if (ret && !image->arch.fdt) >> + of_free_fdt(fdt); > > Just call kvfree() directly. Sure - will do. -lakshmi > >> + >> + return ret ? ERR_PTR(ret) : NULL; >> } >> >> const struct kexec_file_ops kexec_elf64_ops = { >> diff --git a/arch/powerpc/kexec/file_load_64.c b/arch/powerpc/kexec/file_load_64.c >> index 3cab318aa3b9..d9d5b5569a6d 100644 >> --- a/arch/powerpc/kexec/file_load_64.c >> +++ b/arch/powerpc/kexec/file_load_64.c >> @@ -1113,5 +1113,8 @@ int arch_kimage_file_post_load_cleanup(struct kimage *image) >> image->arch.elf_headers = NULL; >> image->arch.elf_headers_sz = 0; >> >> + of_free_fdt(image->arch.fdt); >> + image->arch.fdt = NULL; >> + >> return kexec_image_post_load_cleanup_default(image); >> } >> -- >> 2.30.0 >>
On 2/4/21 3:36 PM, Rob Herring wrote: > On Thu, Feb 4, 2021 at 5:23 PM Lakshmi Ramasubramanian > <nramas@linux.microsoft.com> wrote: >> >> On 2/4/21 11:26 AM, Rob Herring wrote: >>> On Thu, Feb 4, 2021 at 10:42 AM Lakshmi Ramasubramanian >>> <nramas@linux.microsoft.com> wrote: >>>> >>>> of_alloc_and_init_fdt() and of_free_fdt() have been defined in >>>> drivers/of/kexec.c to allocate and free memory for FDT. >>>> >>>> Use of_alloc_and_init_fdt() and of_free_fdt() to allocate and >>>> initialize the FDT, and to free the FDT respectively. >>>> >>>> powerpc sets the FDT address in image_loader_data field in >>>> "struct kimage" and the memory is freed in >>>> kimage_file_post_load_cleanup(). This cleanup function uses kfree() >>>> to free the memory. But since of_alloc_and_init_fdt() uses kvmalloc() >>>> for allocation, the buffer needs to be freed using kvfree(). >>> >>> You could just change the kexec core to call kvfree() instead. >> >>> >>>> Define "fdt" field in "struct kimage_arch" for powerpc to store >>>> the address of FDT, and free the memory in powerpc specific >>>> arch_kimage_file_post_load_cleanup(). >>> >>> However, given all the other buffers have an explicit field in kimage >>> or kimage_arch, changing powerpc is to match arm64 is better IMO. >> >> Just to be clear: >> I'll leave this as is - free FDT buffer in powerpc's >> arch_kimage_file_post_load_cleanup() to match arm64 behavior. > > Yes. ok > >> Will not change "kexec core" to call kvfree() - doing that change would >> require changing all architectures to use kvmalloc() for >> image_loader_data allocation. > > Actually, no. AIUI, kvfree() can be used whether you used kvmalloc, > vmalloc, or kmalloc for the alloc. That is good information. Thanks. > >>>> Signed-off-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com> >>>> Suggested-by: Rob Herring <robh@kernel.org> >>>> Suggested-by: Thiago Jung Bauermann <bauerman@linux.ibm.com> >>>> --- >>>> arch/powerpc/include/asm/kexec.h | 2 ++ >>>> arch/powerpc/kexec/elf_64.c | 26 ++++++++++++++++---------- >>>> arch/powerpc/kexec/file_load_64.c | 3 +++ >>>> 3 files changed, 21 insertions(+), 10 deletions(-) >>>> >>>> diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h >>>> index 2c0be93d239a..d7d13cac4d31 100644 >>>> --- a/arch/powerpc/include/asm/kexec.h >>>> +++ b/arch/powerpc/include/asm/kexec.h >>>> @@ -111,6 +111,8 @@ struct kimage_arch { >>>> unsigned long elf_headers_mem; >>>> unsigned long elf_headers_sz; >>>> void *elf_headers; >>>> + >>>> + void *fdt; >>>> }; >>>> >>>> char *setup_kdump_cmdline(struct kimage *image, char *cmdline, >>>> diff --git a/arch/powerpc/kexec/elf_64.c b/arch/powerpc/kexec/elf_64.c >>>> index d0e459bb2f05..51d2d8eb6c1b 100644 >>>> --- a/arch/powerpc/kexec/elf_64.c >>>> +++ b/arch/powerpc/kexec/elf_64.c >>>> @@ -19,6 +19,7 @@ >>>> #include <linux/kexec.h> >>>> #include <linux/libfdt.h> >>>> #include <linux/module.h> >>>> +#include <linux/of.h> >>>> #include <linux/of_fdt.h> >>>> #include <linux/slab.h> >>>> #include <linux/types.h> >>>> @@ -32,7 +33,7 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >>>> unsigned int fdt_size; >>>> unsigned long kernel_load_addr; >>>> unsigned long initrd_load_addr = 0, fdt_load_addr; >>>> - void *fdt; >>>> + void *fdt = NULL; >>>> const void *slave_code; >>>> struct elfhdr ehdr; >>>> char *modified_cmdline = NULL; >>>> @@ -103,18 +104,12 @@ static void *elf64_load(struct kimage *image, char *kernel_buf, >>>> } >>>> >>>> fdt_size = fdt_totalsize(initial_boot_params) * 2; >>>> - fdt = kmalloc(fdt_size, GFP_KERNEL); >>>> + fdt = of_alloc_and_init_fdt(fdt_size); >>>> if (!fdt) { >>>> pr_err("Not enough memory for the device tree.\n"); >>>> ret = -ENOMEM; >>>> goto out; >>>> } >>>> - ret = fdt_open_into(initial_boot_params, fdt, fdt_size); >>>> - if (ret < 0) { >>>> - pr_err("Error setting up the new device tree.\n"); >>>> - ret = -EINVAL; >>>> - goto out; >>>> - } >>>> >>>> ret = setup_new_fdt_ppc64(image, fdt, initrd_load_addr, >>> >>> The first thing this function does is call setup_new_fdt() which first >>> calls of_kexec_setup_new_fdt(). (Note, I really don't understand the >>> PPC code split. It looks like there's a 32-bit and 64-bit split, but >>> 32-bit looks broken to me. Nothing ever calls setup_new_fdt() except >>> setup_new_fdt_ppc64()). The arm64 version is calling >>> of_alloc_and_init_fdt() and then of_kexec_setup_new_fdt() directly. >>> >>> So we can just make of_alloc_and_init_fdt() also call >>> of_kexec_setup_new_fdt() (really, just tweak of_kexec_setup_new_fdt do >>> the alloc and copy). >> ok - will move fdt allocation into of_kexec_setup_new_fdt(). >> >> I don't think the architecture needs to pick the >>> size either. It's doubtful that either one is that sensitive to the >>> amount of extra space. >> I am not clear about the above comment - >> are you saying the architectures don't need to pass FDT size to the >> alloc function? >> >> arm64 is adding command line string length and some extra space to the >> size computed from initial_boot_params for FDT Size: >> >> buf_size = fdt_totalsize(initial_boot_params) >> + cmdline_len + DTB_EXTRA_SPACE; >> >> powerpc is just using twice the size computed from initial_boot_params >> >> fdt_size = fdt_totalsize(initial_boot_params) * 2; >> >> I think it would be safe to let arm64 and powerpc pass the required FDT >> size, along with the other params to of_kexec_setup_new_fdt() - and in >> this function we allocate FDT and set it up. > > It's pretty clear that someone just picked something that 'should be > enough'. The only thing I can guess for the difference is that arm > DT's tend to be a bit larger. So doubling the size would be even more > excessive. Either way, we're talking 10s kB to few 100kB. I'd go with > DTB_EXTRA_SPACE and we can bump it up if someone has problems. ok - I'll use DTB_EXTRA_SPACE for the fdt allocation. > > Also, I would like for 'initial_boot_params' to be private ultimately, > so removing any references is helpful. ok > >> And, for powerpc leave the remaining code in setup_new_fdt_ppc64(). > > Right. ok thanks, -lakshmi
On kexec file load Integrity Measurement Architecture (IMA) subsystem may verify the IMA signature of the kernel and initramfs, and measure it. The command line parameters passed to the kernel in the kexec call may also be measured by IMA. A remote attestation service can verify a TPM quote based on the TPM event log, the IMA measurement list, and the TPM PCR data. This can be achieved only if the IMA measurement log is carried over from the current kernel to the next kernel across the kexec call. powerpc already supports carrying forward the IMA measurement log on kexec. This patch set adds support for carrying forward the IMA measurement log on kexec on ARM64. This patch set moves the platform independent code defined for powerpc such that it can be reused for other platforms as well. A chosen node "linux,ima-kexec-buffer" is added to the DTB for ARM64 to hold the address and the size of the memory reserved to carry the IMA measurement log. This patch set has been tested for ARM64 platform using QEMU. I would like help from the community for testing this change on powerpc. Thanks. This patch set is based on commit b3f82afc1041 ("IMA: Measure kernel version in early boot") in https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git "next-integrity" branch. Changelog: v16 - Defined functions to allocate and free buffer for FDT for powerpc and arm64. - Moved ima_buffer_addr and ima_buffer_size fields from "struct kimage_arch" in powerpc to "struct kimage" v15 - Included Rob's patches in the patch set, and rebased the changes to "next-integrity" branch. - Allocate memory for DTB, on arm64, using kmalloc() instead of vmalloc() to keep it consistent with powerpc implementation. - Call of_kexec_setup_new_fdt() from setup_new_fdt_ppc64() and remove setup_new_fdt() in the same patch to keep it bisect safe. v14 - Select CONFIG_HAVE_IMA_KEXEC for CONFIG_KEXEC_FILE, for powerpc and arm64, if CONFIG_IMA is enabled. - Use IS_ENABLED() macro instead of "#ifdef" in remove_ima_buffer(), ima_get_kexec_buffer(), and ima_free_kexec_buffer(). - Call of_kexec_setup_new_fdt() from setup_new_fdt_ppc64() and remove setup_new_fdt() in "arch/powerpc/kexec/file_load.c". v13 - Moved the arch independent functions to drivers/of/kexec.c and then refactored the code. - Moved arch_ima_add_kexec_buffer() to security/integrity/ima/ima_kexec.c v12 - Use fdt_appendprop_addrrange() in setup_ima_buffer() to setup the IMA measurement list property in the device tree. - Moved architecture independent functions from "arch/powerpc/kexec/ima.c" to "drivers/of/kexec." - Deleted "arch/powerpc/kexec/ima.c" and "arch/powerpc/include/asm/ima.h". v11 - Rebased the changes on the kexec code refactoring done by Rob Herring in his "dt/kexec" branch - Removed "extern" keyword in function declarations - Removed unnecessary header files included in C files - Updated patch descriptions per Thiago's comments v10 - Moved delete_fdt_mem_rsv(), remove_ima_buffer(), get_ima_kexec_buffer, and get_root_addr_size_cells() to drivers/of/kexec.c - Moved arch_ima_add_kexec_buffer() to security/integrity/ima/ima_kexec.c - Conditionally define IMA buffer fields in struct kimage_arch v9 - Moved delete_fdt_mem_rsv() to drivers/of/kexec_fdt.c - Defined a new function get_ima_kexec_buffer() in drivers/of/ima_kexec.c to replace do_get_kexec_buffer() - Changed remove_ima_kexec_buffer() to the original function name remove_ima_buffer() - Moved remove_ima_buffer() to drivers/of/ima_kexec.c - Moved ima_get_kexec_buffer() and ima_free_kexec_buffer() to security/integrity/ima/ima_kexec.c v8: - Moved remove_ima_kexec_buffer(), do_get_kexec_buffer(), and delete_fdt_mem_rsv() to drivers/of/fdt.c - Moved ima_dump_measurement_list() and ima_add_kexec_buffer() back to security/integrity/ima/ima_kexec.c v7: - Renamed remove_ima_buffer() to remove_ima_kexec_buffer() and moved this function definition to kernel. - Moved delete_fdt_mem_rsv() definition to kernel - Moved ima_dump_measurement_list() and ima_add_kexec_buffer() to a new file namely ima_kexec_fdt.c in IMA v6: - Remove any existing FDT_PROP_IMA_KEXEC_BUFFER property in the device tree and also its corresponding memory reservation in the currently running kernel. - Moved the function remove_ima_buffer() defined for powerpc to IMA and renamed the function to ima_remove_kexec_buffer(). Also, moved delete_fdt_mem_rsv() from powerpc to IMA. v5: - Merged get_addr_size_cells() and do_get_kexec_buffer() into a single function when moving the arch independent code from powerpc to IMA - Reverted the change to use FDT functions in powerpc code and added back the original code in get_addr_size_cells() and do_get_kexec_buffer() for powerpc. - Added fdt_add_mem_rsv() for ARM64 to reserve the memory for the IMA log buffer during kexec. - Fixed the warning reported by kernel test bot for ARM64 arch_ima_add_kexec_buffer() - moved this function to a new file namely arch/arm64/kernel/ima_kexec.c v4: - Submitting the patch series on behalf of the original author Prakhar Srivastava <prsriva@linux.microsoft.com> - Moved FDT_PROP_IMA_KEXEC_BUFFER ("linux,ima-kexec-buffer") to libfdt.h so that it can be shared by multiple platforms. v3: Breakup patches further into separate patches. - Refactoring non architecture specific code out of powerpc - Update powerpc related code to use fdt functions - Update IMA buffer read related code to use of functions - Add support to store the memory information of the IMA measurement logs to be carried forward. - Update the property strings to align with documented nodes https://github.com/devicetree-org/dt-schema/pull/46 v2: Break patches into separate patches. - Powerpc related Refactoring - Updating the docuemntation for chosen node - Updating arm64 to support IMA buffer pass v1: Refactoring carrying over IMA measuremnet logs over Kexec. This patch moves the non-architecture specific code out of powerpc and adds to security/ima.(Suggested by Thiago) Add Documentation regarding the ima-kexec-buffer node in the chosen node documentation v0: Add a layer of abstraction to use the memory reserved by device tree for ima buffer pass. Add support for ima buffer pass using reserved memory for arm64 kexec. Update the arch sepcific code path in kexec file load to store the ima buffer in the reserved memory. The same reserved memory is read on kexec or cold boot. Lakshmi Ramasubramanian (8): powerpc: Move ima buffer fields to struct kimage powerpc: Move arch independent ima kexec functions to drivers/of/kexec.c kexec: Use fdt_appendprop_addrrange() to add ima buffer to FDT powerpc: Delete unused function delete_fdt_mem_rsv() of: Define functions to allocate and free FDT arm64: Use OF alloc and free functions for FDT powerpc: Use OF alloc and free for FDT arm64: Enable passing IMA log to next kernel on kexec Rob Herring (4): powerpc: Rename kexec elfcorehdr_addr to elf_headers_mem of: Add a common kexec FDT setup function arm64: Use common of_kexec_setup_new_fdt() powerpc: Use common of_kexec_setup_new_fdt() arch/arm64/Kconfig | 1 + arch/arm64/kernel/machine_kexec_file.c | 158 +------- arch/powerpc/Kconfig | 2 +- arch/powerpc/include/asm/ima.h | 30 -- arch/powerpc/include/asm/kexec.h | 11 +- arch/powerpc/kexec/Makefile | 7 - arch/powerpc/kexec/elf_64.c | 26 +- arch/powerpc/kexec/file_load.c | 184 +--------- arch/powerpc/kexec/file_load_64.c | 11 +- arch/powerpc/kexec/ima.c | 219 ----------- drivers/of/Makefile | 1 + drivers/of/kexec.c | 488 +++++++++++++++++++++++++ include/linux/kexec.h | 5 + include/linux/of.h | 15 +- security/integrity/ima/ima.h | 4 - security/integrity/ima/ima_kexec.c | 3 +- 16 files changed, 552 insertions(+), 613 deletions(-) delete mode 100644 arch/powerpc/include/asm/ima.h delete mode 100644 arch/powerpc/kexec/ima.c create mode 100644 drivers/of/kexec.c