Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
Hello Prakhar,
Prakhar Srivastava writes:
> On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
>>
>> Prakhar Srivastava 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.
No problem.
>
> 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.
If CONFIG_IMA_KEXEC is set, then yes. If it isn't then that needs to
happen in the function above.
> 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(_cells, _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
>>>
Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
On 6/19/20 5:19 PM, Thiago Jung Bauermann wrote:
Prakhar Srivastava 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(_cells, _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
#include
#include
+#include
+#include
+#include
#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,
Re: [V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
Prakhar Srivastava 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.
>
> ---
> 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.
> @@ -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(_cells, _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
> #include
> #include
> +#include
> +#include
> +#include
> #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(_cells, _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;
> +}
> +
>
[V2 PATCH 1/3] Refactoring powerpc code for carrying over IMA measurement logs, to move non architecture specific code to security/ima.
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.
---
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;
}
#endif /* CONFIG_IMA_KEXEC */
diff --git a/arch/powerpc/kexec/ima.c b/arch/powerpc/kexec/ima.c
index 720e50e490b6..6054ce91d2a6 100644
--- a/arch/powerpc/kexec/ima.c
+++ b/arch/powerpc/kexec/ima.c
@@ -12,121 +12,6 @@
#include
#include
-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(_cells, _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", );
- if (!prop)
- return -ENOENT;
-
- ret = do_get_kexec_buffer(prop, len, _addr, _size);
- if (ret)
- return ret;
-
- *addr = __va(tmp_addr);
- *size = tmp_size;
-
- return 0;
-}
-
-/**
- * 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, , );
- if (ret)
- return ret;
-
- ret = of_remove_property(of_chosen, prop);
- if (ret)
- return ret;
-
- return memblock_free(addr, size);
-
-}
-
-/**
- * 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", );
- if (!prop)
- return;
-
- ret = do_get_kexec_buffer(prop, len, , );
- fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
- if (ret)
- return;
-
- 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
@@ -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);
if (!image->arch.ima_buffer_size)
