From: Prakhar Srivastava <prsriva@linux•microsoft.com>
To: Thiago Jung Bauermann <bauerman@linux•ibm.com>
Cc: kstewart@linuxfoundation•org, mark.rutland@arm•com,
gregkh@linuxfoundation•org, bhsharma@redhat•com, tao.li@vivo•com,
zohar@linux•ibm.com, paulus@samba•org, vincenzo.frascino@arm•com,
will@kernel•org, nramas@linux•microsoft.com,
frowand.list@gmail•com, masahiroy@kernel•org, jmorris@namei•org,
takahiro.akashi@linaro•org, linux-arm-kernel@lists•infradead.org,
catalin.marinas@arm•com, serge@hallyn•com,
devicetree@vger•kernel.org, pasha.tatashin@soleen•com,
robh+dt@kernel•org, hsinyi@chromium•org,
tusharsu@linux•microsoft.com, tglx@linutronix•de,
allison@lohutok•net, christophe.leroy@c-s•fr, mbrugger@suse•com,
balajib@linux•microsoft.com, dmitry.kasatkin@gmail•com,
linux-kernel@vger•kernel.org,
linux-security-module@vger•kernel.org, james.morse@arm•com,
linux-integrity@vger•kernel.org, linuxppc-dev@lists•ozlabs.org
Subject: Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
Date: Mon, 13 Jul 2020 13:30:59 -0700 [thread overview]
Message-ID: <1385c8bb-cd25-8dc4-7224-8e27135f3356@linux.microsoft.com> (raw)
In-Reply-To: <87o8per3m0.fsf@morokweng.localdomain>
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
>
next prev parent reply other threads:[~2020-07-13 20:32 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-18 7:10 [V2 PATCH 0/3] Adding support for carrying IMA measurement logs Prakhar Srivastava
2020-06-18 7:10 ` [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima Prakhar Srivastava
2020-06-20 0:19 ` Thiago Jung Bauermann
2020-07-13 20:30 ` Prakhar Srivastava [this message]
2020-07-16 17:51 ` Thiago Jung Bauermann
2020-06-18 7:10 ` [V2 PATCH 2/3] dt-bindings: chosen: Document ima-kexec-buffer Prakhar Srivastava
2020-06-20 0:41 ` Thiago Jung Bauermann
2020-07-13 20:32 ` Prakhar Srivastava
2020-06-18 7:10 ` [V2 PATCH 3/3] Add support for arm64 to carry over IMA measurement logs Prakhar Srivastava
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1385c8bb-cd25-8dc4-7224-8e27135f3356@linux.microsoft.com \
--to=prsriva@linux$(echo .)microsoft.com \
--cc=allison@lohutok$(echo .)net \
--cc=balajib@linux$(echo .)microsoft.com \
--cc=bauerman@linux$(echo .)ibm.com \
--cc=bhsharma@redhat$(echo .)com \
--cc=catalin.marinas@arm$(echo .)com \
--cc=christophe.leroy@c-s$(echo .)fr \
--cc=devicetree@vger$(echo .)kernel.org \
--cc=dmitry.kasatkin@gmail$(echo .)com \
--cc=frowand.list@gmail$(echo .)com \
--cc=gregkh@linuxfoundation$(echo .)org \
--cc=hsinyi@chromium$(echo .)org \
--cc=james.morse@arm$(echo .)com \
--cc=jmorris@namei$(echo .)org \
--cc=kstewart@linuxfoundation$(echo .)org \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
--cc=linux-integrity@vger$(echo .)kernel.org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=linux-security-module@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=mark.rutland@arm$(echo .)com \
--cc=masahiroy@kernel$(echo .)org \
--cc=mbrugger@suse$(echo .)com \
--cc=nramas@linux$(echo .)microsoft.com \
--cc=pasha.tatashin@soleen$(echo .)com \
--cc=paulus@samba$(echo .)org \
--cc=robh+dt@kernel$(echo .)org \
--cc=serge@hallyn$(echo .)com \
--cc=takahiro.akashi@linaro$(echo .)org \
--cc=tao.li@vivo$(echo .)com \
--cc=tglx@linutronix$(echo .)de \
--cc=tusharsu@linux$(echo .)microsoft.com \
--cc=vincenzo.frascino@arm$(echo .)com \
--cc=will@kernel$(echo .)org \
--cc=zohar@linux$(echo .)ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox