Messages in this thread | | | Subject | Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima. | From | Prakhar Srivastava <> | Date | Mon, 13 Jul 2020 13:30:59 -0700 |
| |
On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote: > > Prakhar Srivastava <prsriva@linux.microsoft.com> writes: > >> Powerpc has support to carry over the IMA measurement logs. Refatoring the >> non-architecture specific code out of arch/powerpc and into security/ima. >> >> The code adds support for reserving and freeing up of memory for IMA measurement >> logs. > > Last week, Mimi provided this feedback: > > "From your patch description, this patch should be broken up. Moving > the non-architecture specific code out of powerpc should be one patch. > Additional support should be in another patch. After each patch, the > code should work properly." > > That's not what you do here. You move the code, but you also make other > changes at the same time. This has two problems: > > 1. It makes the patch harder to review, because it's very easy to miss a > change. > > 2. If in the future a git bisect later points to this patch, it's not > clear whether the problem is because of the code movement, or because > of the other changes. > > When you move code, ideally the patch should only make the changes > necessary to make the code work at its new location. The patch which > does code movement should not cause any change in behavior. > > Other changes should go in separate patches, either before or after the > one moving the code. > > More comments below. > Hi Thiago,
Apologies for the delayed response i was away for a few days. I am working on breaking up the changes so that its easier to review and update as well.
Thanks, Prakhar Srivastava
>> >> --- >> arch/powerpc/include/asm/ima.h | 10 --- >> arch/powerpc/kexec/ima.c | 126 ++--------------------------- >> security/integrity/ima/ima_kexec.c | 116 ++++++++++++++++++++++++++ >> 3 files changed, 124 insertions(+), 128 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h >> index ead488cf3981..c29ec86498f8 100644 >> --- a/arch/powerpc/include/asm/ima.h >> +++ b/arch/powerpc/include/asm/ima.h >> @@ -4,15 +4,6 @@ >> >> 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); >> @@ -22,7 +13,6 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node); >> static inline int setup_ima_buffer(const struct kimage *image, void *fdt, >> int chosen_node) >> { >> - remove_ima_buffer(fdt, chosen_node); >> return 0; >> } > > This is wrong. Even if the currently running kernel doesn't have > CONFIG_IMA_KEXEC, it should remove the IMA buffer property and memory > reservation from the FDT that is being prepared for the next kernel. > > This is because the IMA kexec buffer is useless for the next kernel, > regardless of whether the current kernel supports CONFIG_IMA_KEXEC or > not. Keeping it around would be a waste of memory. > I will keep it in my next revision. My understanding was the reserved memory is freed and property removed when IMA loads the logs on init. During setup_fdt in kexec, a duplicate copy of the dt is used, but memory still needs to be allocated, thus the property itself indicats presence of reserved memory.
>> @@ -179,13 +64,18 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node) >> int ret, addr_cells, size_cells, entry_size; >> u8 value[16]; >> >> - remove_ima_buffer(fdt, chosen_node); > > This is wrong, for the same reason stated above. > >> if (!image->arch.ima_buffer_size) >> return 0; >> >> - ret = get_addr_size_cells(&addr_cells, &size_cells); >> - if (ret) >> + ret = fdt_address_cells(fdt, chosen_node); >> + if (ret < 0) >> + return ret; >> + addr_cells = ret; >> + >> + ret = fdt_size_cells(fdt, chosen_node); >> + if (ret < 0) >> return ret; >> + size_cells = ret; >> >> entry_size = 4 * (addr_cells + size_cells); >> > > I liked this change. Thanks! I agree it's better to use > fdt_address_cells() and fdt_size_cells() here. > > But it should be in a separate patch. Either before or after the one > moving the code. > >> diff --git a/security/integrity/ima/ima_kexec.c b/security/integrity/ima/ima_kexec.c >> index 121de3e04af2..e1e6d6154015 100644 >> --- a/security/integrity/ima/ima_kexec.c >> +++ b/security/integrity/ima/ima_kexec.c >> @@ -10,8 +10,124 @@ >> #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 "ima.h" >> >> +static int get_addr_size_cells(int *addr_cells, int *size_cells) >> +{ >> + struct device_node *root; >> + >> + root = of_find_node_by_path("/"); >> + if (!root) >> + return -EINVAL; >> + >> + *addr_cells = of_n_addr_cells(root); >> + *size_cells = of_n_size_cells(root); >> + >> + of_node_put(root); >> + >> + return 0; >> +} >> + >> +static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr, >> + size_t *size) >> +{ >> + int ret, addr_cells, size_cells; >> + >> + ret = get_addr_size_cells(&addr_cells, &size_cells); >> + if (ret) >> + return ret; >> + >> + if (len < 4 * (addr_cells + size_cells)) >> + return -ENOENT; >> + >> + *addr = of_read_number(prop, addr_cells); >> + *size = of_read_number(prop + 4 * addr_cells, 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; >> +} > > The functions above were moved without being changed. Good. > >> +/** >> + * 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(__pa(addr), size); > > Here you added a __pa() call. Do you store a virtual address in > linux,ima-kexec-buffer property? Doesn't it make more sense to store a > physical address? > trying to minimize the changes here as do_get_kexec_buffer return the va. I will refactor this to remove the double translation.
> Even if making this change is the correct thing to do, it should be a > separate patch, unless it can't be avoided. And if that is the case, > then it should be explained in the commit message. > >> + >> +} >> + >> +/** >> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt >> + * >> + * The IMA measurement buffer is of no use to a subsequent kernel, so we always >> + * remove it from the device tree. >> + */ >> +void remove_ima_buffer(void *fdt, int chosen_node) >> +{ >> + int ret, len; >> + unsigned long addr; >> + size_t size; >> + const void *prop; >> + >> + prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len); >> + if (!prop) >> + return; >> + >> + do_get_kexec_buffer(prop, len, &addr, &size); >> + ret = fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer"); >> + if (ret < 0) >> + return; >> + >> + memblock_free(addr, size); >> +} > > Here is another function that changed when moved. This one I know to be > wrong. You're confusing the purposes of remove_ima_buffer() and > ima_free_kexec_buffer(). > > You did send me a question about them nearly three weeks ago which I > only answered today, so I apologize. Also, their names could more > clearly reflect their differences, so it's bad naming on my part. > > With IMA kexec buffers, there are two kernels (and thus their two > respective, separate device trees) to be concerned about: > > 1. the currently running kernel, which uses the live device tree > (accessed with the of_* functions) and the memblock subsystem; > > 2. the kernel which is being loaded by kexec, which will use the FDT > blob being passed around as argument to these functions, and the memory > reservations in the memory reservation table of the FDT blob. > > ima_free_kexec_buffer() is used by IMA in the currently running kernel. > Therefore the device tree it is concerned about is the live one, and > thus uses the of_* functions to access it. And uses memblock to change > the memory reservation. > > remove_ima_buffer() on the other hand is used by the kexec code to > prepare an FDT blob for the kernel that is being loaded. It should not > make any changes to live device tree, nor to memblock allocations. It > should only make changes to the FDT blob. > Thank you for this, greatly appreciate clearing my misunderstandings. > -- > Thiago Jung Bauermann > IBM Linux Technology Center >
| |