Carry forward IMA measurement log on kexec on ARM64

The function prototype for the functions defined in ima.c for powerpc are given in the header file ima.h. But this header file is not included in ima.c resulting in compilation errors such as given below.

arch/powerpc/kexec/ima.c:56:5: error: no previous prototype for 'ima_get_kexec_buffer' [-Werror=missing-prototypes]
56 | int ima_get_kexec_buffer(void **addr, size_t *size)

      |     ^~~~~~~~~~~~~~~~~~~~

The function parameters for remove_ima_buffer() and arch_ima_add_kexec_buffer() are not described in the function header resulting in warnings such as given below.

arch/powerpc/kexec/ima.c:111: warning: Function parameter or member 'fdt' not described in 'remove_ima_buffer'

Include ima.h in ima.c for powerpc. Describe the function parameters for remove_ima_buffer() and arch_ima_add_kexec_buffer().

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 arch/powerpc/include/asm/kexec.h |  1 -
 arch/powerpc/kexec/file_load.c   | 32 -----------------
 drivers/of/Makefile              |  1 +
 drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++
 include/linux/kexec.h            |  5 +++
 5 files changed, 67 insertions(+), 33 deletions(-)
 create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void *slave_code,
 int setup_new_fdt(const struct kimage *image, void *fdt,
 		  unsigned long initrd_load_addr, unsigned long initrd_len,
 		  const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
 
 #ifdef CONFIG_PPC64
 struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -32

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const void *slave_code,
 	return 0;
 }
 
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and size
- *
- * Return: 0 on success, or negative errno on error.
- */
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
-	int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-	for (i = 0; i < num_rsvs; i++) {
-		uint64_t rsv_start, rsv_size;
-
-		ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-		if (ret) {
-			pr_err("Malformed device tree.\n");
-			return -EINVAL;
-		}
-
-		if (rsv_start == start && rsv_size == size) {
-			ret = fdt_del_mem_rsv(fdt, i);
-			if (ret) {
-				pr_err("Error deleting device tree reservation.\n");
-				return -EINVAL;
-			}
-
-			return 0;
-		}
-	}
-
-	return -ENOENT;
-}
-
 /*
  * setup_new_fdt - modify /chosen and memory reservation for the next kernel
  * @image:		kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644

drivers/of/Makefile +0 -0

 obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/kexec.c new file +25 -0

new file mode 100644
index 000000000000..b7d59105fcb8
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian ([email protected])
+ *
+ * File: kexec.c
+ *	Defines kexec related functions.
+ */
+
+#define pr_fmt(fmt)	"OF: kexec: " fmt
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+
+/**
+ * delete_fdt_mem_rsv - delete memory reservation with given address and size
+ *
+ * @fdt: Flattened Device Tree to update
+ * @start: Starting address of the reservation to delete

drivers/of/kexec.c new file +16 -0

+
+	num_rsvs = fdt_num_mem_rsv(fdt);
+	for (i = 0; i < num_rsvs; i++) {
+		uint64_t rsv_start, rsv_size;
+
+		ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
+		if (ret) {
+			pr_err("Malformed device tree.\n");
+			return -EINVAL;
+		}
+
+		if (rsv_start == start && rsv_size == size) {
+			ret = fdt_del_mem_rsv(fdt, i);
+			if (ret) {
+				pr_err("Error deleting device tree reservation.\n");
+				return -EINVAL;

include/linux/kexec.h +4 -0

+extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
+			      unsigned long size);
+#endif /* CONFIG_OF_FLATTREE */
+
 #endif /* !defined(__ASSEBMLY__) */
 
 #endif /* LINUX_KEXEC_H */

Hello Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Quoted 1 line

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

s/kexec_fdt.c/kexec.c/

Quoted 74 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
  arch/powerpc/include/asm/kexec.h |  1 -
  arch/powerpc/kexec/file_load.c   | 32 -----------------
  drivers/of/Makefile              |  1 +
  drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++
  include/linux/kexec.h            |  5 +++
  5 files changed, 67 insertions(+), 33 deletions(-)

create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void *slave_code,
  int setup_new_fdt(const struct kimage *image, void *fdt,
  		  unsigned long initrd_load_addr, unsigned long initrd_len,
  		  const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
 
  #ifdef CONFIG_PPC64
  struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -32

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const void *slave_code,
  	return 0;
  }
 
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and size
- *
- * Return: 0 on success, or negative errno on error.
- */
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
-	int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-	for (i = 0; i < num_rsvs; i++) {
-		uint64_t rsv_start, rsv_size;
-
-		ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-		if (ret) {
-			pr_err("Malformed device tree.\n");
-			return -EINVAL;
-		}
-
-		if (rsv_start == start && rsv_size == size) {
-			ret = fdt_del_mem_rsv(fdt, i);
-			if (ret) {
-				pr_err("Error deleting device tree reservation.\n");
-				return -EINVAL;
-			}
-
-			return 0;
-		}
-	}
-
-	return -ENOENT;
-}
-
  /*
   * setup_new_fdt - modify /chosen and memory reservation for the next kernel
   * @image:		kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644

drivers/of/Makefile obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o · line 16

Quoted 4 lines

drivers/of/Makefile +1 -0

@@ -13,5 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
  obj-$(CONFIG_OF_RESOLVE)  += resolver.o
  obj-$(CONFIG_OF_OVERLAY) += overlay.o
  obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_FLATTREE) += kexec.o

Isn't this too broad? kexec.o will only be useful to kernel configs which enable CONFIG_KEXEC_FILE, so perhaps do:

ifdef CONFIG_OF_FLATTREE
ifdef CONFIG_KEXEC_FILE
obj-y += kexec.o
endif
endif

What do you think?

Quoted 38 lines

drivers/of/Makefile +0 -0

  obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/kexec.c new file +30 -0

new file mode 100644
index 000000000000..b7d59105fcb8
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian ([email protected])
+ *
+ * File: kexec.c
+ *	Defines kexec related functions.
+ */
+
+#define pr_fmt(fmt)	"OF: kexec: " fmt
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+
+/**
+ * delete_fdt_mem_rsv - delete memory reservation with given address and size
+ *
+ * @fdt: Flattened Device Tree to update
+ * @start: Starting address of the reservation to delete
+ * @size: Size of the reservation to delete
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)

drivers/of/kexec.c line 35

Quoted 5 lines

drivers/of/kexec.c new file +5 -0

+{
+	int i, ret, num_rsvs;
+
+	if (!IS_ENABLED(CONFIG_KEXEC_FILE))
+		return 0;

If you agree with my suggestion, then this IS_ENABLED() wouldn't be needed.

Quoted 16 lines

drivers/of/kexec.c new file +16 -0

+
+	num_rsvs = fdt_num_mem_rsv(fdt);
+	for (i = 0; i < num_rsvs; i++) {
+		uint64_t rsv_start, rsv_size;
+
+		ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
+		if (ret) {
+			pr_err("Malformed device tree.\n");
+			return -EINVAL;
+		}
+
+		if (rsv_start == start && rsv_size == size) {
+			ret = fdt_del_mem_rsv(fdt, i);
+			if (ret) {
+				pr_err("Error deleting device tree reservation.\n");
+				return -EINVAL;

drivers/of/kexec.c line 61

Quoted 10 lines

drivers/of/kexec.c new file +10 -0

+			}
+
+			pr_debug("Freed reserved memory at %lu of size %lu\n",
+				 start, size);
+			return 0;
+		}
+	}
+
+	return -ENOENT;
+}

The function code is unchanged apart from the addition of the IS_ENABLED() and the pr_debug(), so that is good.

include/linux/kexec.h static inline int kexec_crash_loaded(void) { return 0; } · line 410

Quoted 9 lines

include/linux/kexec.h +1 -0

index 9e93bef52968..d0234c4815da 100644
@@ -407,6 +407,11 @@ static inline int kexec_crash_loaded(void) { return 0; }
  #define kexec_in_progress false
  #endif /* CONFIG_KEXEC_CORE */
 
+#if defined(CONFIG_OF_FLATTREE)

This would also change to require CONFIG_KEXEC_FILE.

Quoted 7 lines

include/linux/kexec.h +4 -0

+extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
+			      unsigned long size);
+#endif /* CONFIG_OF_FLATTREE */
+
  #endif /* !defined(__ASSEBMLY__) */
 
  #endif /* LINUX_KEXEC_H */

On 12/4/20 6:22 PM, Thiago Jung Bauermann wrote

Quoted 12 lines

Hello Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Quoted 3 lines

Quoted 1 line

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

s/kexec_fdt.c/kexec.c/

Missed that in the patch description. Will fix it. Thanks.

Quoted 91 lines

Quoted 77 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
  arch/powerpc/include/asm/kexec.h |  1 - arch/powerpc/kexec/file_load.c   | 32 ----------------- drivers/of/Makefile              |  1 +
  drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++ include/linux/kexec.h            |  5 +++

  5 files changed, 67 insertions(+), 33 deletions(-)

  create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const 
 void *slave_code,
   int setup_new_fdt(const struct kimage *image, void *fdt,
             unsigned long initrd_load_addr, unsigned long initrd_len,
             const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long 
 size);
   #ifdef CONFIG_PPC64

  struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -2

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const 
 void *slave_code,
       return 0;
   }
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address 

and size
- *
- * Return: 0 on success, or negative errno on error.
- */
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)

arch/powerpc/kexec/file_load.c +0 -26

-{
-    int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-    for (i = 0; i < num_rsvs; i++) {
-        uint64_t rsv_start, rsv_size;
-
-        ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-        if (ret) {
-            pr_err("Malformed device tree.\n");
-            return -EINVAL;
-        }
-
-        if (rsv_start == start && rsv_size == size) {
-            ret = fdt_del_mem_rsv(fdt, i);
-            if (ret) {
-                pr_err("Error deleting device tree reservation.\n");
-                return -EINVAL;
-            }
-
-            return 0;
-        }
-    }
-
-    return -ENOENT;
-}
-

  /*
   * setup_new_fdt - modify /chosen and memory reservation for the next kernel
   * @image:        kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644
@@ -13,5 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o

Quoted 4 lines

  obj-$(CONFIG_OF_RESOLVE)  += resolver.o
  obj-$(CONFIG_OF_OVERLAY) += overlay.o
  obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_FLATTREE) += kexec.o

Isn't this too broad? kexec.o will only be useful to kernel configs which enable CONFIG_KEXEC_FILE, so perhaps do:

ifdef CONFIG_OF_FLATTREE
ifdef CONFIG_KEXEC_FILE
obj-y += kexec.o
endif
endif

Quoted 1 line

What do you think?

Per Rob's feedback on v9 patch set, I have moved all the architecture independent ima kexec functions to a single file "drivers/of/kexec.c"

Since these functions are enabled on different kernel CONFIGs, I have used IS_ENABLED(CONFIG_XYZ) macro instead of "#ifdef" in the C file to conditionally compile.

Quoted 41 lines

Quoted 41 lines

  obj-$(CONFIG_OF_UNITTEST) += unittest-data/

drivers/of/kexec.c new file +22 -0

new file mode 100644
index 000000000000..b7d59105fcb8
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2020 Microsoft Corporation
+ *
+ * Author: Lakshmi Ramasubramanian ([email protected])
+ *
+ * File: kexec.c
+ *    Defines kexec related functions.
+ */
+
+#define pr_fmt(fmt)    "OF: kexec: " fmt
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/memblock.h>
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
+
+/**
+ * delete_fdt_mem_rsv - delete memory reservation with given address 

and size
+ *
+ * @fdt: Flattened Device Tree to update
+ * @start: Starting address of the reservation to delete
+ * @size: Size of the reservation to delete
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)

Quoted 9 lines

drivers/of/kexec.c line 35

Quoted 5 lines

drivers/of/kexec.c new file +5 -0

+{
+    int i, ret, num_rsvs;
+
+    if (!IS_ENABLED(CONFIG_KEXEC_FILE))
+        return 0;

If you agree with my suggestion, then this IS_ENABLED() wouldn't be needed.

Please see my response above.

If there is a way to keep all the ima kexec functions in a single file and yet not use "#ifdef" in C file to conditionally compile, please let me know. I'll update.

thanks,
 -lakshmi

Quoted 50 lines

Quoted 16 lines

drivers/of/kexec.c new file +16 -0

+
+    num_rsvs = fdt_num_mem_rsv(fdt);
+    for (i = 0; i < num_rsvs; i++) {
+        uint64_t rsv_start, rsv_size;
+
+        ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
+        if (ret) {
+            pr_err("Malformed device tree.\n");
+            return -EINVAL;
+        }
+
+        if (rsv_start == start && rsv_size == size) {
+            ret = fdt_del_mem_rsv(fdt, i);
+            if (ret) {
+                pr_err("Error deleting device tree reservation.\n");
+                return -EINVAL;

drivers/of/kexec.c line 61

Quoted 10 lines

drivers/of/kexec.c new file +10 -0

+            }
+
+            pr_debug("Freed reserved memory at %lu of size %lu\n",
+                 start, size);
+            return 0;
+        }
+    }
+
+    return -ENOENT;
+}

The function code is unchanged apart from the addition of the IS_ENABLED() and the pr_debug(), so that is good.

include/linux/kexec.h static inline int kexec_crash_loaded(void) { · line 410

Quoted 9 lines

include/linux/kexec.h +1 -0

index 9e93bef52968..d0234c4815da 100644
@@ -407,6 +407,11 @@ static inline int kexec_crash_loaded(void) { 
 return 0; }
   #define kexec_in_progress false
   #endif /* CONFIG_KEXEC_CORE */
+#if defined(CONFIG_OF_FLATTREE)

This would also change to require CONFIG_KEXEC_FILE.

Quoted 6 lines

include/linux/kexec.h +4 -0

+extern int delete_fdt_mem_rsv(void *fdt, unsigned long start,
+                  unsigned long size);
+#endif /* CONFIG_OF_FLATTREE */
+
   #endif /* !defined(__ASSEBMLY__) */
   #endif /* LINUX_KEXEC_H */

On 12/6/20 5:50 PM, Lakshmi Ramasubramanian wrote:

Hi Thiago,

Quoted 109 lines

On 12/4/20 6:22 PM, Thiago Jung Bauermann wrote

Quoted 12 lines

Hello Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Quoted 3 lines

Quoted 1 line

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

s/kexec_fdt.c/kexec.c/

Missed that in the patch description. Will fix it. Thanks.

Quoted 91 lines

Quoted 77 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
  arch/powerpc/include/asm/kexec.h |  1 -
  arch/powerpc/kexec/file_load.c   | 32 -----------------
  drivers/of/Makefile              |  1 +
  drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++
  include/linux/kexec.h            |  5 +++
  5 files changed, 67 insertions(+), 33 deletions(-)

create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
  int setup_new_fdt(const struct kimage *image, void *fdt,
            unsigned long initrd_load_addr, unsigned long initrd_len,
            const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
  #ifdef CONFIG_PPC64
  struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -2

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
      return 0;
  }
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and

size
- *
- * Return: 0 on success, or negative errno on error.
- */

arch/powerpc/kexec/file_load.c +0 -27

-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
-    int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-    for (i = 0; i < num_rsvs; i++) {
-        uint64_t rsv_start, rsv_size;
-
-        ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-        if (ret) {
-            pr_err("Malformed device tree.\n");
-            return -EINVAL;
-        }
-
-        if (rsv_start == start && rsv_size == size) {
-            ret = fdt_del_mem_rsv(fdt, i);
-            if (ret) {
-                pr_err("Error deleting device tree reservation.\n");
-                return -EINVAL;
-            }
-
-            return 0;
-        }
-    }
-
-    return -ENOENT;
-}
-
  /*
   * setup_new_fdt - modify /chosen and memory reservation for the next

kernel
* @image: kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644

drivers/of/Makefile obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o · line 16

Quoted 4 lines

drivers/of/Makefile +1 -0

@@ -13,5 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
  obj-$(CONFIG_OF_RESOLVE)  += resolver.o
  obj-$(CONFIG_OF_OVERLAY) += overlay.o
  obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_FLATTREE) += kexec.o

Isn't this too broad? kexec.o will only be useful to kernel configs which enable CONFIG_KEXEC_FILE, so perhaps do:

ifdef CONFIG_OF_FLATTREE
ifdef CONFIG_KEXEC_FILE
obj-y += kexec.o
endif
endif

Quoted 6 lines

Quoted 1 line

What do you think?

Per Rob's feedback on v9 patch set, I have moved all the architecture independent ima kexec functions to a single file "drivers/of/kexec.c" Since these functions are enabled on different kernel CONFIGs, I have used IS_ENABLED(CONFIG_XYZ) macro instead of "#ifdef" in the C file to conditionally compile.

Per Rob's feedback on the v9 patch, I'll keep the ima kexec functions in a single file (in "drivers/of/kexec.c") and use IS_ENABLED() macro to handle the function calls.

I'll make the other changes you'd suggested on v10 patches and will post v11 patch set shortly.

Hi Lakshmi,

Quoted 119 lines · Lakshmi Ramasubramanian

On 12/6/20 5:50 PM, Lakshmi Ramasubramanian wrote:

Hi Thiago,

Quoted 109 lines

On 12/4/20 6:22 PM, Thiago Jung Bauermann wrote

Quoted 12 lines

Hello Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Quoted 3 lines

Quoted 1 line

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

s/kexec_fdt.c/kexec.c/

Missed that in the patch description. Will fix it. Thanks.

Quoted 91 lines

Quoted 77 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
   arch/powerpc/include/asm/kexec.h |  1 -
   arch/powerpc/kexec/file_load.c   | 32 -----------------
   drivers/of/Makefile              |  1 +
   drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++
   include/linux/kexec.h            |  5 +++
   5 files changed, 67 insertions(+), 33 deletions(-)

create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
   int setup_new_fdt(const struct kimage *image, void *fdt,
             unsigned long initrd_load_addr, unsigned long initrd_len,
             const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
   #ifdef CONFIG_PPC64
   struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -2

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
       return 0;
   }
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and

size
- *
- * Return: 0 on success, or negative errno on error.
- */

arch/powerpc/kexec/file_load.c +0 -27

-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
-    int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-    for (i = 0; i < num_rsvs; i++) {
-        uint64_t rsv_start, rsv_size;
-
-        ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-        if (ret) {
-            pr_err("Malformed device tree.\n");
-            return -EINVAL;
-        }
-
-        if (rsv_start == start && rsv_size == size) {
-            ret = fdt_del_mem_rsv(fdt, i);
-            if (ret) {
-                pr_err("Error deleting device tree reservation.\n");
-                return -EINVAL;
-            }
-
-            return 0;
-        }
-    }
-
-    return -ENOENT;
-}
-
   /*
    * setup_new_fdt - modify /chosen and memory reservation for the next

kernel
* @image: kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644

drivers/of/Makefile obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o · line 16

Quoted 4 lines

drivers/of/Makefile +1 -0

@@ -13,5 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
   obj-$(CONFIG_OF_RESOLVE)  += resolver.o
   obj-$(CONFIG_OF_OVERLAY) += overlay.o
   obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_FLATTREE) += kexec.o

Isn't this too broad? kexec.o will only be useful to kernel configs which enable CONFIG_KEXEC_FILE, so perhaps do:

ifdef CONFIG_OF_FLATTREE
ifdef CONFIG_KEXEC_FILE
obj-y += kexec.o
endif
endif

Quoted 6 lines

Quoted 1 line

What do you think?

Per Rob's feedback on v9 patch set, I have moved all the architecture independent ima kexec functions to a single file "drivers/of/kexec.c" Since these functions are enabled on different kernel CONFIGs, I have used IS_ENABLED(CONFIG_XYZ) macro instead of "#ifdef" in the C file to conditionally compile.

Quoted 8 lines

Per Rob's feedback on the v9 patch, I'll keep the ima kexec functions in a single file (in "drivers/of/kexec.c") and use IS_ENABLED() macro to handle the function calls.

I'll make the other changes you'd suggested on v10 patches and will post v11 patch set shortly.

From a cursory look at the use of functions in this file, I got the

impression that there wouldn't be any reference to them in kernel configs that didn't have CONFIG_KEXEC_FILE enabled, which is why I suggested the change above. I think you can make it without any other changes to the code.

I could be wrong though, and there could be some config which tried to use some of these functions even when CONFIG_KEXEC_FILE is disabled. In that case, the customary way to resolve it is to provide static inline stub versions in a header file (not in a .c file) of just those functions that are needed.

The reason why placing stub functions in header files is better is that then the compiler has visibility of the dummy function when compiling the source file which uses the function, and is able to eliminate the dead code that arises from the function always returning one value.

Using IS_ENABLED() to do an early return as the first operation in the function in a separate .c file means that the compiler doesn't know anything and has to put a jump to the dummy function (only to immediately return), and retain the code that deals with the possibility of different values being returned.

It's not a big deal in this case because none of these functions is in a hot path, but it does make the kernel text a tiny bit bigger than necessary.

Quoted 130 lines · Thiago Jung Bauermann

Hi Lakshmi,

Quoted 119 lines · Lakshmi Ramasubramanian

On 12/6/20 5:50 PM, Lakshmi Ramasubramanian wrote:

Hi Thiago,

Quoted 109 lines

On 12/4/20 6:22 PM, Thiago Jung Bauermann wrote

Quoted 12 lines

Hello Lakshmi,

Quoted 7 lines · Lakshmi Ramasubramanian

delete_fdt_mem_rsv() retrieves the memory reserve map entry, for the given starting address and size, from the device tree blob, and removes the entry from the device tree blob. This function is called to free the resources reserved for the buffer used for carrying forward the IMA measurement logs on kexec. This function does not have architecture specific code, but is currently limited to powerpc.

Quoted 3 lines

Quoted 1 line

Move delete_fdt_mem_rsv() to "drivers/of/kexec_fdt.c" so that it is

s/kexec_fdt.c/kexec.c/

Missed that in the patch description. Will fix it. Thanks.

Quoted 91 lines

Quoted 77 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
   arch/powerpc/include/asm/kexec.h |  1 -
   arch/powerpc/kexec/file_load.c   | 32 -----------------
   drivers/of/Makefile              |  1 +
   drivers/of/kexec.c               | 61 ++++++++++++++++++++++++++++++++
   include/linux/kexec.h            |  5 +++
   5 files changed, 67 insertions(+), 33 deletions(-)

create mode 100644 drivers/of/kexec.c

arch/powerpc/include/asm/kexec.h +0 -1

index 55d6ede30c19..7c223031ecdd 100644
@@ -126,7 +126,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
   int setup_new_fdt(const struct kimage *image, void *fdt,
             unsigned long initrd_load_addr, unsigned long initrd_len,
             const char *cmdline);
-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size);
   #ifdef CONFIG_PPC64
   struct kexec_buf;

arch/powerpc/kexec/file_load.c +0 -2

index 9a232bc36c8f..9efc98b1e2ae 100644
@@ -109,38 +109,6 @@ int setup_purgatory(struct kimage *image, const void
 *slave_code,
       return 0;
   }
-/**
- * delete_fdt_mem_rsv - delete memory reservation with given address and

size
- *
- * Return: 0 on success, or negative errno on error.
- */

arch/powerpc/kexec/file_load.c +0 -27

-int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
-{
-    int i, ret, num_rsvs = fdt_num_mem_rsv(fdt);
-
-    for (i = 0; i < num_rsvs; i++) {
-        uint64_t rsv_start, rsv_size;
-
-        ret = fdt_get_mem_rsv(fdt, i, &rsv_start, &rsv_size);
-        if (ret) {
-            pr_err("Malformed device tree.\n");
-            return -EINVAL;
-        }
-
-        if (rsv_start == start && rsv_size == size) {
-            ret = fdt_del_mem_rsv(fdt, i);
-            if (ret) {
-                pr_err("Error deleting device tree reservation.\n");
-                return -EINVAL;
-            }
-
-            return 0;
-        }
-    }
-
-    return -ENOENT;
-}
-
   /*
    * setup_new_fdt - modify /chosen and memory reservation for the next

kernel
* @image: kexec image being loaded.

drivers/of/Makefile +0 -0

index 6e1e5212f058..77d24712c0c8 100644

drivers/of/Makefile obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o · line 16

Quoted 4 lines

drivers/of/Makefile +1 -0

@@ -13,5 +13,6 @@ obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
   obj-$(CONFIG_OF_RESOLVE)  += resolver.o
   obj-$(CONFIG_OF_OVERLAY) += overlay.o
   obj-$(CONFIG_OF_NUMA) += of_numa.o
+obj-$(CONFIG_OF_FLATTREE) += kexec.o

Isn't this too broad? kexec.o will only be useful to kernel configs which enable CONFIG_KEXEC_FILE, so perhaps do:

ifdef CONFIG_OF_FLATTREE
ifdef CONFIG_KEXEC_FILE
obj-y += kexec.o
endif
endif

Quoted 6 lines

Quoted 1 line

What do you think?

Per Rob's feedback on v9 patch set, I have moved all the architecture independent ima kexec functions to a single file "drivers/of/kexec.c" Since these functions are enabled on different kernel CONFIGs, I have used IS_ENABLED(CONFIG_XYZ) macro instead of "#ifdef" in the C file to conditionally compile.

Quoted 8 lines

Per Rob's feedback on the v9 patch, I'll keep the ima kexec functions in a single file (in "drivers/of/kexec.c") and use IS_ENABLED() macro to handle the function calls.

I'll make the other changes you'd suggested on v10 patches and will post v11 patch set shortly.

From a cursory look at the use of functions in this file, I got the

Quoted 13 lines

impression that there wouldn't be any reference to them in kernel configs that didn't have CONFIG_KEXEC_FILE enabled, which is why I suggested the change above. I think you can make it without any other changes to the code.
I could be wrong though, and there could be some config which tried to use some of these functions even when CONFIG_KEXEC_FILE is disabled. In that case, the customary way to resolve it is to provide static inline stub versions in a header file (not in a .c file) of just those functions that are needed.
The reason why placing stub functions in header files is better is that then the compiler has visibility of the dummy function when compiling the source file which uses the function, and is able to eliminate the dead code that arises from the function always returning one value.

I agree with you Thiago.

Is there a way to keep all the relevant functions in a single C file, not use "#ifdef" in C file, and follow the coding pattern you've described above (i mean, "defining a stub function in a header file when the config conditions are not met")?

The functions do_get_kexec_buffer() and get_addr_size_cells(), defined in arch/powerpc/kexec/ima.c, retrieve the address and size of the given property from the device tree blob. These functions do not have architecture specific code, but are currently limited to powerpc. do_get_kexec_buffer() correctly handles a device tree property that is a child node of the root node, but not anything other than the immediate root child nodes.

Move architecture independent functions get_ima_kexec_buffer() and get_root_addr_size_cells() to "drivers/of/kexec.c". These functions retrieve the chosen node "linux,ima-kexec-buffer" from the device tree, and return the address and size of the buffer used for carrying forward the IMA measurement log across kexec system call.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 drivers/of/kexec.c    | 70 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/kexec.h |  3 ++
 2 files changed, 73 insertions(+)

drivers/of/kexec.c +70 -0

index b7d59105fcb8..516b86f7113a 100644
@@ -59,3 +59,73 @@ int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
 
 	return -ENOENT;
 }
+
+/**
+ * get_root_addr_size_cells - Get address and size of root node
+ *
+ * @addr_cells: Return address of the root node
+ * @size_cells: Return size of the root node
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int get_root_addr_size_cells(int *addr_cells, int *size_cells)
+{
+	struct device_node *root;
+
+	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
+		return -EOPNOTSUPP;
+
+	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;
+}
+
+/**
+ * get_ima_kexec_buffer - Get address and size of IMA kexec buffer
+ *
+ * @fdt: Flattened Device Tree
+ * @chosen_node: Offset of chosen node in the FDT
+ * @addr: Return address of the node
+ * @size: Return size of the node
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int get_ima_kexec_buffer(void *fdt, int chosen_node,
+			 unsigned long *addr, size_t *size)
+{
+	const void *prop;
+	int addr_cells, size_cells, prop_len;
+	int rc;
+
+	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
+		return -EOPNOTSUPP;
+
+	rc = get_root_addr_size_cells(&addr_cells, &size_cells);
+	if (rc)
+		return rc;
+
+	if (fdt)
+		prop = fdt_getprop(fdt, chosen_node,
+				   "linux,ima-kexec-buffer", &prop_len);
+	else
+		prop = of_get_property(of_chosen,
+				       "linux,ima-kexec-buffer", &prop_len);
+
+	if (!prop)
+		return -ENOENT;
+
+	if (prop_len < 4 * (addr_cells + size_cells))
+		return -EINVAL;
+
+	*addr = of_read_number(prop, addr_cells);
+	*size = of_read_number(prop + 4 * addr_cells, size_cells);
+
+	return 0;
+}

include/linux/kexec.h +0 -0

index d0234c4815da..10ff704ab670 100644

Quoted 14 lines · Lakshmi Ramasubramanian

The functions do_get_kexec_buffer() and get_addr_size_cells(), defined in arch/powerpc/kexec/ima.c, retrieve the address and size of the given property from the device tree blob. These functions do not have architecture specific code, but are currently limited to powerpc. do_get_kexec_buffer() correctly handles a device tree property that is a child node of the root node, but not anything other than the immediate root child nodes.

Move architecture independent functions get_ima_kexec_buffer() and get_root_addr_size_cells() to "drivers/of/kexec.c". These functions retrieve the chosen node "linux,ima-kexec-buffer" from the device tree, and return the address and size of the buffer used for carrying forward the IMA measurement log across kexec system call.

Quoted 3 lines

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

I'd personally squash this patch with the next one, but I'll leave it just as a suggestion since maintainers are the ones who know best what works well in this regard.

The code is good, except for a nit below:

Reviewed-by Thiago Jung Bauermann <[email protected]>

Quoted 89 lines

---
  drivers/of/kexec.c    | 70 +++++++++++++++++++++++++++++++++++++++++++
  include/linux/kexec.h |  3 ++
  2 files changed, 73 insertions(+)

drivers/of/kexec.c +70 -0

index b7d59105fcb8..516b86f7113a 100644
@@ -59,3 +59,73 @@ int delete_fdt_mem_rsv(void *fdt, unsigned long start, unsigned long size)
 
  	return -ENOENT;
  }
+
+/**
+ * get_root_addr_size_cells - Get address and size of root node
+ *
+ * @addr_cells: Return address of the root node
+ * @size_cells: Return size of the root node
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int get_root_addr_size_cells(int *addr_cells, int *size_cells)
+{
+	struct device_node *root;
+
+	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
+		return -EOPNOTSUPP;
+
+	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;
+}
+
+/**
+ * get_ima_kexec_buffer - Get address and size of IMA kexec buffer
+ *
+ * @fdt: Flattened Device Tree
+ * @chosen_node: Offset of chosen node in the FDT
+ * @addr: Return address of the node
+ * @size: Return size of the node
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int get_ima_kexec_buffer(void *fdt, int chosen_node,
+			 unsigned long *addr, size_t *size)
+{
+	const void *prop;
+	int addr_cells, size_cells, prop_len;
+	int rc;
+
+	if (!IS_ENABLED(CONFIG_HAVE_IMA_KEXEC))
+		return -EOPNOTSUPP;
+
+	rc = get_root_addr_size_cells(&addr_cells, &size_cells);
+	if (rc)
+		return rc;
+
+	if (fdt)
+		prop = fdt_getprop(fdt, chosen_node,
+				   "linux,ima-kexec-buffer", &prop_len);
+	else
+		prop = of_get_property(of_chosen,
+				       "linux,ima-kexec-buffer", &prop_len);
+
+	if (!prop)
+		return -ENOENT;
+
+	if (prop_len < 4 * (addr_cells + size_cells))
+		return -EINVAL;
+
+	*addr = of_read_number(prop, addr_cells);
+	*size = of_read_number(prop + 4 * addr_cells, size_cells);
+
+	return 0;
+}

include/linux/kexec.h +0 -0

index d0234c4815da..10ff704ab670 100644

include/linux/kexec.h static inline int kexec_crash_loaded(void) { return 0; } · line 416

Quoted 9 lines

include/linux/kexec.h +3 -0

@@ -408,6 +408,9 @@ static inline int kexec_crash_loaded(void) { return 0; }
  #endif /* CONFIG_KEXEC_CORE */
 
  #if defined(CONFIG_OF_FLATTREE)
+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 int delete_fdt_mem_rsv(void *fdt, unsigned long start,
  			      unsigned long size);
  #endif /* CONFIG_OF_FLATTREE */

The extern keyword on function prototypes doesn't have any meaning. It's better to drop them (I should have mentioned this on the previous patch as well).

do_get_kexec_buffer() and get_addr_size_cells() are called by ima_get_kexec_buffer(), ima_free_kexec_buffer, and remove_ima_buffer() to retrieve the address and size of the buffer used for carrying forward the IMA measurement log across kexec system call. These functions correctly handle a device tree property that is a child node of the root node, but not anything other than the immediate root child nodes.

Use the architecture independent functions get_ima_kexec_buffer() and get_root_addr_size_cells() defined in "drivers/of/ima_kexec.c",

to get the address and size of the IMA measurement log buffer from the device tree. Remove do_get_kexec_buffer() and get_addr_size_cells() since they are not used anymore.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 arch/powerpc/kexec/ima.c | 58 +++++-----------------------------------
 1 file changed, 7 insertions(+), 51 deletions(-)

arch/powerpc/kexec/ima.c +7 -51

index a36c39db4b1a..906e8212435d 100644
@@ -13,40 +13,6 @@
 #include <linux/libfdt.h>
 #include <asm/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.

@@ -56,16 +22,11 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
  */
 int ima_get_kexec_buffer(void **addr, size_t *size)
 {
-	int ret, len;
+	int ret;
 	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);
+	ret = get_ima_kexec_buffer(NULL, 0, &tmp_addr, &tmp_size);
 	if (ret)
 		return ret;
 

@@ -89,7 +50,7 @@ int ima_free_kexec_buffer(void)
 	if (!prop)
 		return -ENOENT;
 
-	ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size);
+	ret = get_ima_kexec_buffer(NULL, 0, &addr, &size);
 	if (ret)
 		return ret;
 

@@ -98,7 +59,6 @@ int ima_free_kexec_buffer(void)
 		return ret;
 
 	return memblock_free(addr, size);
-
 }
 
 /**

@@ -112,19 +72,15 @@ int ima_free_kexec_buffer(void)
  */
 void remove_ima_buffer(void *fdt, int chosen_node)
 {
-	int ret, len;
+	int ret;
 	unsigned long addr;
 	size_t size;
-	const void *prop;
 
-	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
-	if (!prop)
+	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
+	if (ret)
 		return;
 
-	ret = do_get_kexec_buffer(prop, len, &addr, &size);
 	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
-	if (ret)
-		return;
 
 	ret = delete_fdt_mem_rsv(fdt, addr, size);
 	if (!ret)

@@ -191,7 +147,7 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
 	if (!image->arch.ima_buffer_size)
 		return 0;
 
-	ret = get_addr_size_cells(&addr_cells, &size_cells);
+	ret = get_root_addr_size_cells(&addr_cells, &size_cells);
 	if (ret)
 		return ret;

Quoted 8 lines · Lakshmi Ramasubramanian

do_get_kexec_buffer() and get_addr_size_cells() are called by ima_get_kexec_buffer(), ima_free_kexec_buffer, and remove_ima_buffer() to retrieve the address and size of the buffer used for carrying forward the IMA measurement log across kexec system call. These functions correctly handle a device tree property that is a child node of the root node, but not anything other than the immediate root child nodes.

Quoted 2 lines

Use the architecture independent functions get_ima_kexec_buffer() and get_root_addr_size_cells() defined in "drivers/of/ima_kexec.c",

s/ima_kexec.c/kexec.c/

Quoted 5 lines

to get the address and size of the IMA measurement log buffer from the device tree. Remove do_get_kexec_buffer() and get_addr_size_cells() since they are not used anymore.

Quoted 3 lines

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Looks good. Thanks!

Reviewed-by Thiago Jung Bauermann <[email protected]>

Quoted 116 lines

---
  arch/powerpc/kexec/ima.c | 58 +++++-----------------------------------
  1 file changed, 7 insertions(+), 51 deletions(-)

arch/powerpc/kexec/ima.c +7 -51

index a36c39db4b1a..906e8212435d 100644
@@ -13,40 +13,6 @@
  #include <linux/libfdt.h>
  #include <asm/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.

@@ -56,16 +22,11 @@ static int do_get_kexec_buffer(const void *prop, int len, unsigned long *addr,
   */
  int ima_get_kexec_buffer(void **addr, size_t *size)
  {
-	int ret, len;
+	int ret;
  	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);
+	ret = get_ima_kexec_buffer(NULL, 0, &tmp_addr, &tmp_size);
  	if (ret)
  		return ret;
 

@@ -89,7 +50,7 @@ int ima_free_kexec_buffer(void)
  	if (!prop)
  		return -ENOENT;
 
-	ret = do_get_kexec_buffer(prop->value, prop->length, &addr, &size);
+	ret = get_ima_kexec_buffer(NULL, 0, &addr, &size);
  	if (ret)
  		return ret;
 

@@ -98,7 +59,6 @@ int ima_free_kexec_buffer(void)
  		return ret;
 
  	return memblock_free(addr, size);
-
  }
 
  /**

@@ -112,19 +72,15 @@ int ima_free_kexec_buffer(void)
   */
  void remove_ima_buffer(void *fdt, int chosen_node)
  {
-	int ret, len;
+	int ret;
  	unsigned long addr;
  	size_t size;
-	const void *prop;
 
-	prop = fdt_getprop(fdt, chosen_node, "linux,ima-kexec-buffer", &len);
-	if (!prop)
+	ret = get_ima_kexec_buffer(fdt, chosen_node, &addr, &size);
+	if (ret)
  		return;
 
-	ret = do_get_kexec_buffer(prop, len, &addr, &size);
  	fdt_delprop(fdt, chosen_node, "linux,ima-kexec-buffer");
-	if (ret)
-		return;
 
  	ret = delete_fdt_mem_rsv(fdt, addr, size);
  	if (!ret)

@@ -191,7 +147,7 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
  	if (!image->arch.ima_buffer_size)
  		return 0;
 
-	ret = get_addr_size_cells(&addr_cells, &size_cells);
+	ret = get_root_addr_size_cells(&addr_cells, &size_cells);
  	if (ret)
  		return ret;

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

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 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(-)

arch/powerpc/include/asm/ima.h +0 -0

index ead488cf3981..a2fc71bc3b23 100644

arch/powerpc/include/asm/ima.h +0 -0

 #ifdef CONFIG_IMA_KEXEC
 int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
 			      size_t size);

arch/powerpc/kexec/ima.c +0 -26

index 906e8212435d..68017123b07d 100644
@@ -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

drivers/of/kexec.c +29 -0

index 516b86f7113a..42d16dfff78d 100644
@@ -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");
+}

include/linux/kexec.h +0 -0

index 10ff704ab670..52a0efff184d 100644

Quoted 6 lines · Lakshmi Ramasubramanian

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/

Quoted 6 lines

accessible for other architectures as well.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Looks good. Just minor comments below. Nevertheless:

Reviewed-by Thiago Jung Bauermann <[email protected]>

Quoted 15 lines

---
  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(-)

arch/powerpc/include/asm/ima.h +0 -0

index ead488cf3981..a2fc71bc3b23 100644
@@ -2,17 +2,13 @@
  #ifndef _ASM_POWERPC_IMA_H
  #define _ASM_POWERPC_IMA_H
 

arch/powerpc/include/asm/ima.h line 7

Quoted 3 lines

arch/powerpc/include/asm/ima.h +2 -0

+#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.

arch/powerpc/include/asm/ima.h line 15

Quoted 11 lines

arch/powerpc/include/asm/ima.h +0 -6

  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.

Quoted 83 lines

arch/powerpc/include/asm/ima.h +0 -0

  #ifdef CONFIG_IMA_KEXEC
  int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
  			      size_t size);

arch/powerpc/kexec/ima.c +0 -26

index 906e8212435d..68017123b07d 100644
@@ -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

drivers/of/kexec.c +29 -0

index 516b86f7113a..42d16dfff78d 100644
@@ -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");
+}

include/linux/kexec.h +0 -0

index 10ff704ab670..52a0efff184d 100644
@@ -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);

include/linux/kexec.h static inline int kexec_crash_loaded(void) { return 0; } · line 417

Quoted 6 lines

include/linux/kexec.h +1 -0

  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.

ima_get_kexec_buffer() retrieves the address and size of the buffer used for carrying forward the IMA measurement logs on kexec from the device tree.

ima_free_kexec_buffer() removes the chosen node "linux,ima-kexec-buffer" from the device tree, and frees the buffer used for carrying forward the IMA measurement logs on kexec.

These functions do not have architecture specific code, but are currently limited to powerpc. Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to ima_kexec.c in IMA so that they are accessible for other architectures as well.

With the above change the functions in arch/powerpc/kexec/ima.c are defined only when the kernel config CONFIG_IMA_KEXEC is enabled. Update the Makefile to build arch/powerpc/kexec/ima.c only when CONFIG_IMA_KEXEC is enabled and remove "#ifdef CONFIG_IMA_KEXEC" in arch/powerpc/kexec/ima.c.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

---
 arch/powerpc/include/asm/ima.h     |  3 --
 arch/powerpc/kexec/Makefile        |  7 +---
 arch/powerpc/kexec/ima.c           | 50 -----------------------------
 security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 59 deletions(-)

arch/powerpc/include/asm/ima.h +0 -3

index a2fc71bc3b23..d8444d27f0d8 100644
@@ -6,9 +6,6 @@
 
 struct kimage;
 
-int ima_get_kexec_buffer(void **addr, size_t *size);
-int ima_free_kexec_buffer(void);
-
 #ifdef CONFIG_IMA_KEXEC
 int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
 			      size_t size);

arch/powerpc/kexec/Makefile +1 -6

index 4aff6846c772..f54a9dbff4c8 100644
@@ -9,12 +9,7 @@ obj-$(CONFIG_PPC32)		+= relocate_32.o
 
 obj-$(CONFIG_KEXEC_FILE)	+= file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o
 
-ifdef CONFIG_HAVE_IMA_KEXEC
-ifdef CONFIG_IMA
-obj-y				+= ima.o
-endif
-endif
-
+obj-$(CONFIG_IMA_KEXEC)		+= ima.o
 
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_core_$(BITS).o := n

arch/powerpc/kexec/ima.c +0 -0

index 68017123b07d..bf7084c0c4da 100644

arch/powerpc/kexec/ima.c +0 -50

-/**
- * 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;
-	unsigned long tmp_addr;
-	size_t tmp_size;
-
-	ret = get_ima_kexec_buffer(NULL, 0, &tmp_addr, &tmp_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 = get_ima_kexec_buffer(NULL, 0, &addr, &size);
-	if (ret)
-		return ret;
-
-	ret = of_remove_property(of_chosen, prop);
-	if (ret)
-		return ret;
-
-	return memblock_free(addr, size);
-}
-
-#ifdef CONFIG_IMA_KEXEC
 /**
  * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
  *

@@ -154,4 +105,3 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
 
 	return 0;
 }
-#endif /* CONFIG_IMA_KEXEC */

Quoted 13 lines · Lakshmi Ramasubramanian

ima_get_kexec_buffer() retrieves the address and size of the buffer used for carrying forward the IMA measurement logs on kexec from the device tree.

ima_free_kexec_buffer() removes the chosen node "linux,ima-kexec-buffer" from the device tree, and frees the buffer used for carrying forward the IMA measurement logs on kexec.

These functions do not have architecture specific code, but are currently limited to powerpc. Move ima_get_kexec_buffer() and ima_free_kexec_buffer() to ima_kexec.c in IMA so that they are accessible for other architectures as well.

Quoted 5 lines

With the above change the functions in arch/powerpc/kexec/ima.c are defined only when the kernel config CONFIG_IMA_KEXEC is enabled. Update the Makefile to build arch/powerpc/kexec/ima.c only when CONFIG_IMA_KEXEC is enabled and remove "#ifdef CONFIG_IMA_KEXEC" in arch/powerpc/kexec/ima.c.

Nice.

Quoted 4 lines

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Reviewed-by Mimi Zohar <[email protected]>

Reviewed-by Thiago Jung Bauermann <[email protected]>

Just a small nit below:

Quoted 44 lines

---
  arch/powerpc/include/asm/ima.h     |  3 --
  arch/powerpc/kexec/Makefile        |  7 +---
  arch/powerpc/kexec/ima.c           | 50 -----------------------------
  security/integrity/ima/ima_kexec.c | 51 ++++++++++++++++++++++++++++++
  4 files changed, 52 insertions(+), 59 deletions(-)

arch/powerpc/include/asm/ima.h +0 -3

index a2fc71bc3b23..d8444d27f0d8 100644
@@ -6,9 +6,6 @@
 
  struct kimage;
 
-int ima_get_kexec_buffer(void **addr, size_t *size);
-int ima_free_kexec_buffer(void);
-
  #ifdef CONFIG_IMA_KEXEC
  int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
  			      size_t size);

arch/powerpc/kexec/Makefile +1 -6

index 4aff6846c772..f54a9dbff4c8 100644
@@ -9,12 +9,7 @@ obj-$(CONFIG_PPC32)		+= relocate_32.o
 
  obj-$(CONFIG_KEXEC_FILE)	+= file_load.o ranges.o file_load_$(BITS).o elf_$(BITS).o
 
-ifdef CONFIG_HAVE_IMA_KEXEC
-ifdef CONFIG_IMA
-obj-y				+= ima.o
-endif
-endif
-
+obj-$(CONFIG_IMA_KEXEC)		+= ima.o
 
  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
  GCOV_PROFILE_core_$(BITS).o := n

arch/powerpc/kexec/ima.c +0 -0

index 68017123b07d..bf7084c0c4da 100644

arch/powerpc/kexec/ima.c line 14

Quoted 2 lines

arch/powerpc/kexec/ima.c +0 -0

@@ -13,55 +13,6 @@
  #include <linux/libfdt.h>
  #include <asm/ima.h>

With this patch, the following includes become unnecessary and can be removed from this file:

#include <linux/of.h>
#include <linux/memblock.h>

Quoted 57 lines

arch/powerpc/kexec/ima.c +0 -50

-/**
- * 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;
-	unsigned long tmp_addr;
-	size_t tmp_size;
-
-	ret = get_ima_kexec_buffer(NULL, 0, &tmp_addr, &tmp_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 = get_ima_kexec_buffer(NULL, 0, &addr, &size);
-	if (ret)
-		return ret;
-
-	ret = of_remove_property(of_chosen, prop);
-	if (ret)
-		return ret;
-
-	return memblock_free(addr, size);
-}
-
-#ifdef CONFIG_IMA_KEXEC
  /**
   * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
   *

@@ -154,4 +105,3 @@ int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node)
 
  	return 0;
  }
-#endif /* CONFIG_IMA_KEXEC */

arch_ima_add_kexec_buffer() sets the address and size of the IMA measurement log in the architecture specific field in struct kimage. This function does not have architecture specific code, but is currently limited to powerpc.

Move arch_ima_add_kexec_buffer() to security/integrity/ima/ima_kexec.c so that it is accessible for other architectures as well.

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 arch/powerpc/include/asm/ima.h     |  3 ---
 arch/powerpc/kexec/ima.c           | 21 ---------------------
 security/integrity/ima/ima_kexec.c | 22 ++++++++++++++++++++++
 3 files changed, 22 insertions(+), 24 deletions(-)

arch/powerpc/include/asm/ima.h +0 -3

index d8444d27f0d8..d6ab5d944dcd 100644
@@ -7,9 +7,6 @@
 struct kimage;
 
 #ifdef CONFIG_IMA_KEXEC
-int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
-			      size_t size);
-
 int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
 #else
 static inline int setup_ima_buffer(const struct kimage *image, void *fdt,

arch/powerpc/kexec/ima.c +0 -21

index bf7084c0c4da..b2793be353a9 100644
@@ -13,27 +13,6 @@
 #include <linux/libfdt.h>
 #include <asm/ima.h>
 
-/**
- * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
- *
- * @image: kimage struct to set IMA buffer data
- * @load_addr: Starting address where IMA buffer is loaded at
- * @size: Number of bytes in the IMA buffer
- *
- * Architectures should use this function to pass on the IMA buffer
- * information to the next kernel.
- *
- * Return: 0 on success, negative errno on error.
- */
-int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
-			      size_t size)
-{
-	image->arch.ima_buffer_addr = load_addr;
-	image->arch.ima_buffer_size = size;
-
-	return 0;
-}
-
 static int write_number(void *p, u64 value, int cells)
 {
 	if (cells == 1) {

security/integrity/ima/ima_kexec.c +12 -0

index 4d354593aecf..5263dafe8f4d 100644
@@ -74,6 +74,28 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
 	return ret;
 }
 
+/**
+ * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
+ *
+ * @image: kimage struct to set IMA buffer data
+ * @load_addr: Starting address where IMA buffer is loaded at
+ * @size: Number of bytes in the IMA buffer
+ *
+ * Architectures should use this function to pass on the IMA buffer
+ * information to the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */

Quoted 10 lines · Lakshmi Ramasubramanian

arch_ima_add_kexec_buffer() sets the address and size of the IMA measurement log in the architecture specific field in struct kimage. This function does not have architecture specific code, but is currently limited to powerpc.

Move arch_ima_add_kexec_buffer() to security/integrity/ima/ima_kexec.c so that it is accessible for other architectures as well.

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Not sure if the maintainers will agree with me (see below), but FWIW:

Reviewed-by Thiago Jung Bauermann <[email protected]>

Quoted 72 lines

---
  arch/powerpc/include/asm/ima.h     |  3 ---
  arch/powerpc/kexec/ima.c           | 21 ---------------------
  security/integrity/ima/ima_kexec.c | 22 ++++++++++++++++++++++
  3 files changed, 22 insertions(+), 24 deletions(-)

arch/powerpc/include/asm/ima.h +0 -3

index d8444d27f0d8..d6ab5d944dcd 100644
@@ -7,9 +7,6 @@
  struct kimage;
 
  #ifdef CONFIG_IMA_KEXEC
-int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
-			      size_t size);
-
  int setup_ima_buffer(const struct kimage *image, void *fdt, int chosen_node);
  #else
  static inline int setup_ima_buffer(const struct kimage *image, void *fdt,

arch/powerpc/kexec/ima.c +0 -21

index bf7084c0c4da..b2793be353a9 100644
@@ -13,27 +13,6 @@
  #include <linux/libfdt.h>
  #include <asm/ima.h>
 
-/**
- * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
- *
- * @image: kimage struct to set IMA buffer data
- * @load_addr: Starting address where IMA buffer is loaded at
- * @size: Number of bytes in the IMA buffer
- *
- * Architectures should use this function to pass on the IMA buffer
- * information to the next kernel.
- *
- * Return: 0 on success, negative errno on error.
- */
-int arch_ima_add_kexec_buffer(struct kimage *image, unsigned long load_addr,
-			      size_t size)
-{
-	image->arch.ima_buffer_addr = load_addr;
-	image->arch.ima_buffer_size = size;
-
-	return 0;
-}
-
  static int write_number(void *p, u64 value, int cells)
  {
  	if (cells == 1) {

security/integrity/ima/ima_kexec.c +12 -0

index 4d354593aecf..5263dafe8f4d 100644
@@ -74,6 +74,28 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  	return ret;
  }
 
+/**
+ * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
+ *
+ * @image: kimage struct to set IMA buffer data
+ * @load_addr: Starting address where IMA buffer is loaded at
+ * @size: Number of bytes in the IMA buffer
+ *
+ * Architectures should use this function to pass on the IMA buffer
+ * information to the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */

security/integrity/ima/ima_kexec.c static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, · line 98

Quoted 10 lines

security/integrity/ima/ima_kexec.c +10 -0

+static int arch_ima_add_kexec_buffer(struct kimage *image,
+				     unsigned long load_addr,
+				     size_t size)
+{
+	image->arch.ima_buffer_addr = load_addr;
+	image->arch.ima_buffer_size = size;
+
+	return 0;
+}
+

Both powerpc and arm64 use the definition above for arch_ima_add_kexec_buffer(), so it makes sense to share them as you do in this patch. This file isn't the best one to put arch-specific code which happens to be identical among architectures, but I can't think of somewhere else to put it.

For now this isn't an issue since powerpc and arm64 are the only arches implementing tihs feature. If a third arch implemented it and also used the same function definition as above, it wouldn't be an issue either so perhaps this is good enough for the time being? :-)

With this patch, the #include <asm/ima.h> in security/integrity/ima/ima.h can be removed. It was there just to provide a declaration of arch_ima_add_kexec_buffer().

Quoted 40 lines · Thiago Jung Bauermann

Quoted 20 lines · Lakshmi Ramasubramanian

security/integrity/ima/ima_kexec.c +12 -0

index 4d354593aecf..5263dafe8f4d 100644
@@ -74,6 +74,28 @@ static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer,
  	return ret;
  }
 
+/**
+ * arch_ima_add_kexec_buffer - do arch-specific steps to add the IMA buffer
+ *
+ * @image: kimage struct to set IMA buffer data
+ * @load_addr: Starting address where IMA buffer is loaded at
+ * @size: Number of bytes in the IMA buffer
+ *
+ * Architectures should use this function to pass on the IMA buffer
+ * information to the next kernel.
+ *
+ * Return: 0 on success, negative errno on error.
+ */

security/integrity/ima/ima_kexec.c static int ima_dump_measurement_list(unsigned long *buffer_size, void **buffer, · line 98

Quoted 10 lines

security/integrity/ima/ima_kexec.c +10 -0

+static int arch_ima_add_kexec_buffer(struct kimage *image,
+				     unsigned long load_addr,
+				     size_t size)
+{
+	image->arch.ima_buffer_addr = load_addr;
+	image->arch.ima_buffer_size = size;
+
+	return 0;
+}
+

Both powerpc and arm64 use the definition above for arch_ima_add_kexec_buffer(), so it makes sense to share them as you do in this patch. This file isn't the best one to put arch-specific code which happens to be identical among architectures, but I can't think of somewhere else to put it.

Quoted 4 lines

For now this isn't an issue since powerpc and arm64 are the only arches implementing tihs feature. If a third arch implemented it and also used the same function definition as above, it wouldn't be an issue either so perhaps this is good enough for the time being? :-)

If Mimi doesn't have any objection, I'll leave this function in this file. The other option is to move this function also to "drivers/of/kexec.c".

Please let me know.

Address and size of the buffer containing the IMA measurement log need to be passed from the current kernel to the next kernel on kexec.

Any existing "linux,ima-kexec-buffer" property in the device tree needs to be removed and its corresponding memory reservation in the currently running kernel needs to be freed. The address and size of the current kernel's IMA measurement log need to be added to the device tree's IMA kexec buffer node and memory for the buffer needs to be reserved for the log to be carried over to the next kernel on the kexec call.

Add address and size fields to "struct kimage_arch" for ARM64 platform to hold the address and size of the IMA measurement log buffer. Remove any existing "linux,ima-kexec-buffer" property in the device tree and free the corresponding memory reservation in the currently running kernel. Add "linux,ima-kexec-buffer" property to the device tree and reserve the memory for storing the IMA log that needs to be passed from the current kernel to the next one.

Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC to indicate that the IMA measurement log information is present in the device tree for ARM64.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

---
 arch/arm64/Kconfig                     |  1 +
 arch/arm64/include/asm/ima.h           | 22 ++++++++++++++
 arch/arm64/include/asm/kexec.h         |  5 ++++
 arch/arm64/kernel/Makefile             |  1 +
 arch/arm64/kernel/ima.c                | 41 ++++++++++++++++++++++++++
 arch/arm64/kernel/machine_kexec_file.c |  8 +++++
 6 files changed, 78 insertions(+)
 create mode 100644 arch/arm64/include/asm/ima.h
 create mode 100644 arch/arm64/kernel/ima.c

arch/arm64/Kconfig +1 -0

index 1515f6f153a0..bcca4a467eda 100644
@@ -1094,6 +1094,7 @@ config KEXEC
 config KEXEC_FILE
 	bool "kexec file based system call"
 	select KEXEC_CORE
+	select HAVE_IMA_KEXEC
 	help
 	  This is new version of kexec system call. This system call is
 	  file based and takes file descriptors as system call argument

arch/arm64/include/asm/ima.h new file +0 -0

new file mode 100644
index 000000000000..354adc8b444c

arch/arm64/include/asm/ima.h new file +19 -0

+ *
+ * Author: Prakhar Srivastava <[email protected]>
+ *
+ */
+#ifndef _ASM_ARCH_IMA_H
+#define _ASM_ARCH_IMA_H
+
+struct kimage;
+
+#ifdef CONFIG_IMA_KEXEC
+extern int setup_ima_buffer(struct kimage *image, void *dtb, int off);
+#else
+static inline int setup_ima_buffer(struct kimage *image, void *dtb, int off)
+{
+	return 0;
+}
+#endif /* CONFIG_IMA_KEXEC */
+
+#endif /* _ASM_ARCH_IMA_H */

arch/arm64/kernel/ima.c new file +38 -0

+ *
+ * Authors:
+ * Lakshmi Ramasubramanian <[email protected]>
+ */
+
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/libfdt.h>
+#include <asm/ima.h>
+
+/**
+ * setup_ima_buffer - add IMA buffer information to the fdt
+ *
+ * @image:	kexec image being loaded.
+ * @dtb:	Flattened device tree for the next kernel.
+ * @off:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int setup_ima_buffer(struct kimage *image, void *dtb, int off)
+{
+	int ret = 0;
+
+	/* add ima-kexec-buffer */
+	if (image->arch.ima_buffer_size > 0) {
+		ret = fdt_appendprop_addrrange(dtb, 0, off,
+				"linux,ima-kexec-buffer",
+				image->arch.ima_buffer_addr,
+				image->arch.ima_buffer_size);
+		if (ret)
+			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
+
+		ret = fdt_add_mem_rsv(dtb, image->arch.ima_buffer_addr,
+				      image->arch.ima_buffer_size);
+	}
+
+	return ret;
+}

Quoted 19 lines · Lakshmi Ramasubramanian

Address and size of the buffer containing the IMA measurement log need to be passed from the current kernel to the next kernel on kexec.

Any existing "linux,ima-kexec-buffer" property in the device tree needs to be removed and its corresponding memory reservation in the currently running kernel needs to be freed. The address and size of the current kernel's IMA measurement log need to be added to the device tree's IMA kexec buffer node and memory for the buffer needs to be reserved for the log to be carried over to the next kernel on the kexec call.

Add address and size fields to "struct kimage_arch" for ARM64 platform to hold the address and size of the IMA measurement log buffer. Remove any existing "linux,ima-kexec-buffer" property in the device tree and free the corresponding memory reservation in the currently running kernel. Add "linux,ima-kexec-buffer" property to the device tree and reserve the memory for storing the IMA log that needs to be passed from the current kernel to the next one.

Quoted 7 lines

Update CONFIG_KEXEC_FILE to select CONFIG_HAVE_IMA_KEXEC to indicate that the IMA measurement log information is present in the device tree for ARM64.

Co-developed-by Prakhar Srivastava <[email protected]>

Signed-off-by Prakhar Srivastava <[email protected]>

Signed-off-by Lakshmi Ramasubramanian <[email protected]>

Reviewed-by Thiago Jung Bauermann <[email protected]>

Quoted 26 lines

---
  arch/arm64/Kconfig                     |  1 +
  arch/arm64/include/asm/ima.h           | 22 ++++++++++++++
  arch/arm64/include/asm/kexec.h         |  5 ++++
  arch/arm64/kernel/Makefile             |  1 +
  arch/arm64/kernel/ima.c                | 41 ++++++++++++++++++++++++++
  arch/arm64/kernel/machine_kexec_file.c |  8 +++++
  6 files changed, 78 insertions(+)

create mode 100644 arch/arm64/include/asm/ima.h
create mode 100644 arch/arm64/kernel/ima.c

arch/arm64/Kconfig +1 -0

index 1515f6f153a0..bcca4a467eda 100644
@@ -1094,6 +1094,7 @@ config KEXEC
  config KEXEC_FILE
  	bool "kexec file based system call"
  	select KEXEC_CORE
+	select HAVE_IMA_KEXEC
  	help
  	  This is new version of kexec system call. This system call is
  	  file based and takes file descriptors as system call argument

arch/arm64/include/asm/ima.h new file +0 -0

new file mode 100644
index 000000000000..354adc8b444c

arch/arm64/include/asm/ima.h line 3

Quoted 6 lines

arch/arm64/include/asm/ima.h new file +3 -0

@@ -0,0 +1,22 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * Copyright (C) 2019 Microsoft Corporation

Shouldn't the year here be 2020?

Quoted 19 lines

arch/arm64/include/asm/ima.h new file +19 -0

+ *
+ * Author: Prakhar Srivastava <[email protected]>
+ *
+ */
+#ifndef _ASM_ARCH_IMA_H
+#define _ASM_ARCH_IMA_H
+
+struct kimage;
+
+#ifdef CONFIG_IMA_KEXEC
+extern int setup_ima_buffer(struct kimage *image, void *dtb, int off);
+#else
+static inline int setup_ima_buffer(struct kimage *image, void *dtb, int off)
+{
+	return 0;
+}
+#endif /* CONFIG_IMA_KEXEC */
+
+#endif /* _ASM_ARCH_IMA_H */

<snip>

arch/arm64/kernel/ima.c line 3

Quoted 9 lines

arch/arm64/kernel/ima.c new file +3 -0

new file mode 100644
index 000000000000..d65bdbe1adf6
@@ -0,0 +1,41 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2016 IBM Corporation

Same question: shouldn't the year here be 2020?

Quoted 39 lines

arch/arm64/kernel/ima.c new file +38 -0

+ *
+ * Authors:
+ * Lakshmi Ramasubramanian <[email protected]>
+ */
+
+#include <linux/kexec.h>
+#include <linux/of.h>
+#include <linux/libfdt.h>
+#include <asm/ima.h>
+
+/**
+ * setup_ima_buffer - add IMA buffer information to the fdt
+ *
+ * @image:	kexec image being loaded.
+ * @dtb:	Flattened device tree for the next kernel.
+ * @off:	Offset to the chosen node.
+ *
+ * Return: 0 on success, or negative errno on error.
+ */
+int setup_ima_buffer(struct kimage *image, void *dtb, int off)
+{
+	int ret = 0;
+
+	/* add ima-kexec-buffer */
+	if (image->arch.ima_buffer_size > 0) {
+		ret = fdt_appendprop_addrrange(dtb, 0, off,
+				"linux,ima-kexec-buffer",
+				image->arch.ima_buffer_addr,
+				image->arch.ima_buffer_size);
+		if (ret)
+			return (ret == -FDT_ERR_NOSPACE ? -ENOMEM : -EINVAL);
+
+		ret = fdt_add_mem_rsv(dtb, image->arch.ima_buffer_addr,
+				      image->arch.ima_buffer_size);
+	}
+
+	return ret;
+}