| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Sun, 6 Dec 2020 17:57:25 -0800
From: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
To: Thiago Jung Bauermann <bauerman@...ux.ibm.com>
Cc: zohar@...ux.ibm.com, robh@...nel.org, gregkh@...uxfoundation.org,
james.morse@....com, catalin.marinas@....com, sashal@...nel.org,
will@...nel.org, mpe@...erman.id.au, benh@...nel.crashing.org,
paulus@...ba.org, robh+dt@...nel.org, frowand.list@...il.com,
vincenzo.frascino@....com, mark.rutland@....com,
dmitry.kasatkin@...il.com, jmorris@...ei.org, serge@...lyn.com,
pasha.tatashin@...een.com, allison@...utok.net,
kstewart@...uxfoundation.org, takahiro.akashi@...aro.org,
tglx@...utronix.de, masahiroy@...nel.org, bhsharma@...hat.com,
mbrugger@...e.com, hsinyi@...omium.org, tao.li@...o.com,
christophe.leroy@....fr, linux-integrity@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
prsriva@...ux.microsoft.com, balajib@...ux.microsoft.com
Subject: Re: [PATCH v10 5/8] powerpc: Move remove_ima_buffer() to
drivers/of/kexec.c
On 12/5/20 12:14 PM, Thiago Jung Bauermann wrote:
>
> Lakshmi Ramasubramanian <nramas@...ux.microsoft.com> writes:
>
>> remove_ima_buffer() removes the chosen node "linux,ima-kexec-buffer"
>> from the device tree and frees the memory reserved for carrying forward
>> the IMA measurement logs on kexec. This function does not have
>> architecture specific code, but is currently limited to powerpc.
>>
>> Move remove_ima_buffer() to "drivers/of/ima_kexec.c" so that it is
>
> s/ima_kexec./kexec.c/
Will fix it.
>
>> accessible for other architectures as well.
>>
>> Co-developed-by: Prakhar Srivastava <prsriva@...ux.microsoft.com>
>> Signed-off-by: Prakhar Srivastava <prsriva@...ux.microsoft.com>
>> Signed-off-by: Lakshmi Ramasubramanian <nramas@...ux.microsoft.com>
>
> Looks good. Just minor comments below. Nevertheless:
>
> Reviewed-by: Thiago Jung Bauermann <bauerman@...ux.ibm.com>
>
>> ---
>> arch/powerpc/include/asm/ima.h | 8 ++------
>> arch/powerpc/kexec/ima.c | 26 --------------------------
>> drivers/of/kexec.c | 29 +++++++++++++++++++++++++++++
>> include/linux/kexec.h | 1 +
>> 4 files changed, 32 insertions(+), 32 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/ima.h b/arch/powerpc/include/asm/ima.h
>> index ead488cf3981..a2fc71bc3b23 100644
>> --- a/arch/powerpc/include/asm/ima.h
>> +++ b/arch/powerpc/include/asm/ima.h
>> @@ -2,17 +2,13 @@
>> #ifndef _ASM_POWERPC_IMA_H
>> #define _ASM_POWERPC_IMA_H
>>
>> +#include <linux/kexec.h>
>> +
>> struct kimage;
>
> When you include <linux/kexec.h>, there's no need anymore for the
> `struct kimage` forward declaration so you can remove it.
Sure - i'll remove the forward declaration.
>
>>
>> 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
>> -
>
> This patch introduces a slight behaviour change on powerpc when
> CONFIG_IMA isn't set: since remove_ima_buffer() is now defined,
> kexec_file_load() will remove the IMA kexec buffer when creating the FDT
> for the next kernel. Before this patch, it will leave it there.
>
> I think it's actually an improvement since a stale IMA kexec buffer
> isn't useful and just wastes memory. This should be mentioned on the
> commit message.
Thanks Thiago.
Appreciate if you could test this change for powerpc and let me know if
you see any issues.
>> #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/ima.c b/arch/powerpc/kexec/ima.c
>> index 906e8212435d..68017123b07d 100644
>> --- a/arch/powerpc/kexec/ima.c
>> +++ b/arch/powerpc/kexec/ima.c
>> @@ -61,32 +61,6 @@ int ima_free_kexec_buffer(void)
>> return memblock_free(addr, size);
>> }
>>
>> -/**
>> - * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
>> - *
>> - * @fdt: Flattened Device Tree to update
>> - * @chosen_node: Offset to the chosen node in the device tree
>> - *
>> - * 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;
>> - unsigned long addr;
>> - size_t size;
>> -
>> - ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
>> - if (ret)
>> - return;
>> -
>> - fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> -
>> - ret = delete_fdt_mem_rsv(fdt, addr, size);
>> - if (!ret)
>> - pr_debug("Removed old IMA buffer reservation.\n");
>> -}
>> -
>> #ifdef CONFIG_IMA_KEXEC
>> /**
>> * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index 516b86f7113a..42d16dfff78d 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -129,3 +129,32 @@ int get_ima_kexec_buffer(void *fdt, int chosen_node,
>>
>> return 0;
>> }
>> +
>> +/**
>> + * remove_ima_buffer - remove the IMA buffer property and reservation from @fdt
>> + *
>> + * @fdt: Flattened Device Tree to update
>> + * @chosen_node: Offset to the chosen node in the device tree
>> + *
>> + * 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;
>> + unsigned long addr;
>> + size_t size;
>> +
>> + if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
>> + return;
>> +
>> + ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
>> + if (ret)
>> + return;
>> +
>> + fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
>> +
>> + ret = delete_fdt_mem_rsv(fdt, addr, size);
>> + if (!ret)
>> + pr_debug("Removed old IMA buffer reservation.\n");
>> +}
>> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
>> index 10ff704ab670..52a0efff184d 100644
>> --- a/include/linux/kexec.h
>> +++ b/include/linux/kexec.h
>> @@ -411,6 +411,7 @@ static inline int kexec_crash_loaded(void) { return 0; }
>> extern int get_root_addr_size_cells(int *addr_cells, int *size_cells);
>> extern int get_ima_kexec_buffer(void *fdt, int chosen_node,
>> unsigned long *addr, size_t *size);
>> +extern void remove_ima_buffer(void *fdt, int chosen_node);
>> extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
>> unsigned long size);
>> #endif /* CONFIG_OF_FLATTREE */
>
> Same comment as before: remove the `extern` keyword.
>
Will remove "extern" keyword.
-lakshmi
Powered by blists - more mailing lists