All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 0/2] powerpc: Enabling IMA arch specific secure boot policies
@ 2019-08-19 12:35 ` Nayna Jain
  0 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Michael Ellerman, Paul Mackerras, Benjamin Herrenschmidt,
	Ard Biesheuvel, Jeremy Kerr, Matthew Garret, Mimi Zohar,
	Claudio Carvalho, Elaine Palmer, George Wilson, Eric Ricther,
	Nayna Jain

IMA subsystem supports custom, built-in, arch-specific policies to define
the files to be measured and appraised. These policies are honored based
on the priority where arch-specific policies is the highest and custom
is the lowest.

OpenPOWER systems rely on IMA for signature verification of the kernel.
This patchset adds support for powerpc specific arch policies that are
defined based on system's OS secureboot state. The OS secureboot state
of the system is determined via device-tree entry.

Changelog:
v5:
* secureboot state is now read via device tree entry rather than OPAL
secure variables
* ima arch policies are updated to use policy based template for
measurement rules

v4:
* Fixed the build issue as reported by Satheesh Rajendran.

v3:
* OPAL APIs in Patch 1 are updated to provide generic interface based on
key/keylen. This patchset updates kernel OPAL APIs to be compatible with
generic interface.
* Patch 2 is cleaned up to use new OPAL APIs.
* Since OPAL can support different types of backend which can vary in the
variable interpretation, the Patch 2 is updated to add a check for the
backend version
* OPAL API now expects consumer to first check the supported backend version
before calling other secvar OPAL APIs. This check is now added in patch 2.
* IMA policies in Patch 3 is updated to specify appended signature and
per policy template.
* The patches now are free of any EFIisms.

v2:

* Removed Patch 1: powerpc/include: Override unneeded early ioremap
functions
* Updated Subject line and patch description of the Patch 1 of this series
* Removed dependency of OPAL_SECVAR on EFI, CPU_BIG_ENDIAN and UCS2_STRING
* Changed OPAL APIs from static to non-static. Added opal-secvar.h for the
same
* Removed EFI hooks from opal_secvar.c
* Removed opal_secvar_get_next(), opal_secvar_enqueue() and
opal_query_variable_info() function
* get_powerpc_sb_mode() in secboot.c now directly calls OPAL Runtime API
rather than via EFI hooks.
* Fixed log messages in get_powerpc_sb_mode() function.
* Added dependency for PPC_SECURE_BOOT on configs PPC64 and OPAL_SECVAR
* Replaced obj-$(CONFIG_IMA) with obj-$(CONFIG_PPC_SECURE_BOOT) in
arch/powerpc/kernel/Makefile

Nayna Jain (2):
  powerpc: detect the secure boot mode of the system
  powerpc: Add support to initialize ima policy rules

 arch/powerpc/Kconfig               | 13 ++++++
 arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
 arch/powerpc/kernel/Makefile       |  2 +
 arch/powerpc/kernel/ima_arch.c     | 50 +++++++++++++++++++++
 arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
 include/linux/ima.h                |  3 +-
 6 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/secboot.h
 create mode 100644 arch/powerpc/kernel/ima_arch.c
 create mode 100644 arch/powerpc/kernel/secboot.c

-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v5 0/2] powerpc: Enabling IMA arch specific secure boot policies
@ 2019-08-19 12:35 ` Nayna Jain
  0 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
	Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
	Elaine Palmer, George Wilson

IMA subsystem supports custom, built-in, arch-specific policies to define
the files to be measured and appraised. These policies are honored based
on the priority where arch-specific policies is the highest and custom
is the lowest.

OpenPOWER systems rely on IMA for signature verification of the kernel.
This patchset adds support for powerpc specific arch policies that are
defined based on system's OS secureboot state. The OS secureboot state
of the system is determined via device-tree entry.

Changelog:
v5:
* secureboot state is now read via device tree entry rather than OPAL
secure variables
* ima arch policies are updated to use policy based template for
measurement rules

v4:
* Fixed the build issue as reported by Satheesh Rajendran.

v3:
* OPAL APIs in Patch 1 are updated to provide generic interface based on
key/keylen. This patchset updates kernel OPAL APIs to be compatible with
generic interface.
* Patch 2 is cleaned up to use new OPAL APIs.
* Since OPAL can support different types of backend which can vary in the
variable interpretation, the Patch 2 is updated to add a check for the
backend version
* OPAL API now expects consumer to first check the supported backend version
before calling other secvar OPAL APIs. This check is now added in patch 2.
* IMA policies in Patch 3 is updated to specify appended signature and
per policy template.
* The patches now are free of any EFIisms.

v2:

* Removed Patch 1: powerpc/include: Override unneeded early ioremap
functions
* Updated Subject line and patch description of the Patch 1 of this series
* Removed dependency of OPAL_SECVAR on EFI, CPU_BIG_ENDIAN and UCS2_STRING
* Changed OPAL APIs from static to non-static. Added opal-secvar.h for the
same
* Removed EFI hooks from opal_secvar.c
* Removed opal_secvar_get_next(), opal_secvar_enqueue() and
opal_query_variable_info() function
* get_powerpc_sb_mode() in secboot.c now directly calls OPAL Runtime API
rather than via EFI hooks.
* Fixed log messages in get_powerpc_sb_mode() function.
* Added dependency for PPC_SECURE_BOOT on configs PPC64 and OPAL_SECVAR
* Replaced obj-$(CONFIG_IMA) with obj-$(CONFIG_PPC_SECURE_BOOT) in
arch/powerpc/kernel/Makefile

Nayna Jain (2):
  powerpc: detect the secure boot mode of the system
  powerpc: Add support to initialize ima policy rules

 arch/powerpc/Kconfig               | 13 ++++++
 arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
 arch/powerpc/kernel/Makefile       |  2 +
 arch/powerpc/kernel/ima_arch.c     | 50 +++++++++++++++++++++
 arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
 include/linux/ima.h                |  3 +-
 6 files changed, 165 insertions(+), 1 deletion(-)
 create mode 100644 arch/powerpc/include/asm/secboot.h
 create mode 100644 arch/powerpc/kernel/ima_arch.c
 create mode 100644 arch/powerpc/kernel/secboot.c

-- 
2.20.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
  2019-08-19 12:35 ` Nayna Jain
@ 2019-08-19 12:35   ` Nayna Jain
  -1 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Michael Ellerman, Paul Mackerras, Benjamin Herrenschmidt,
	Ard Biesheuvel, Jeremy Kerr, Matthew Garret, Mimi Zohar,
	Claudio Carvalho, Elaine Palmer, George Wilson, Eric Ricther,
	Nayna Jain

Secure boot on POWER defines different IMA policies based on the secure
boot state of the system.

This patch defines a function to detect the secure boot state of the
system.

The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig               | 11 +++++
 arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
 arch/powerpc/kernel/Makefile       |  2 +
 arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)
 create mode 100644 arch/powerpc/include/asm/secboot.h
 create mode 100644 arch/powerpc/kernel/secboot.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 77f6ebf97113..c902a39124dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,17 @@ config PPC_MEM_KEYS
 
 	  If unsure, say y.
 
+config PPC_SECURE_BOOT
+	prompt "Enable PowerPC Secure Boot"
+	bool
+	default n
+	depends on PPC64
+	help
+	  Linux on POWER with firmware secure boot enabled needs to define
+	  security policies to extend secure boot to the OS.This config
+	  allows user to enable OS Secure Boot on PowerPC systems that
+	  have firmware secure boot support.
+
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h
new file mode 100644
index 000000000000..e726261bb00b
--- /dev/null
+++ b/arch/powerpc/include/asm/secboot.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ */
+#ifndef POWERPC_SECBOOT_H
+#define POWERPC_SECBOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+extern struct device_node *is_powerpc_secvar_supported(void);
+extern bool get_powerpc_secureboot(void);
+#else
+static inline struct device_node *is_powerpc_secvar_supported(void)
+{
+	return NULL;
+}
+
+static inline bool get_powerpc_secureboot(void)
+{
+	return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..d310ebb4e526 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
+
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
 KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
new file mode 100644
index 000000000000..5ea0d52d64ef
--- /dev/null
+++ b/arch/powerpc/kernel/secboot.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * secboot.c
+ *      - util function to get powerpc secboot state
+ */
+#include <linux/types.h>
+#include <linux/of.h>
+#include <asm/secboot.h>
+
+struct device_node *is_powerpc_secvar_supported(void)
+{
+	struct device_node *np;
+	int status;
+
+	np = of_find_node_by_name(NULL, "ibm,secureboot");
+	if (!np) {
+		pr_info("secureboot node is not found\n");
+		return NULL;
+	}
+
+	status = of_device_is_compatible(np, "ibm,secureboot-v3");
+	if (!status) {
+		pr_info("Secure variables are not supported by this firmware\n");
+		return NULL;
+	}
+
+	return np;
+}
+
+bool get_powerpc_secureboot(void)
+{
+	struct device_node *np;
+	struct device_node *secvar_np;
+	const u64 *psecboot;
+	u64 secboot = 0;
+
+	np = is_powerpc_secvar_supported();
+	if (!np)
+		goto disabled;
+
+	/* Fail-safe for any failure related to secvar */
+	secvar_np = of_get_child_by_name(np, "secvar");
+	if (!secvar_np) {
+		pr_err("Expected secure variables support, fail-safe\n");
+		goto enabled;
+	}
+
+	if (!of_device_is_available(secvar_np)) {
+		pr_err("Secure variables support is in error state, fail-safe\n");
+		goto enabled;
+	}
+
+	psecboot = of_get_property(secvar_np, "secure-mode", NULL);
+	if (!psecboot)
+		goto enabled;
+
+	secboot = be64_to_cpup((__be64 *)psecboot);
+	if (!(secboot & (~0x0)))
+		goto disabled;
+
+enabled:
+	pr_info("secureboot mode enabled\n");
+	return true;
+
+disabled:
+	pr_info("secureboot mode disabled\n");
+	return false;
+}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
@ 2019-08-19 12:35   ` Nayna Jain
  0 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
	Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
	Elaine Palmer, George Wilson

Secure boot on POWER defines different IMA policies based on the secure
boot state of the system.

This patch defines a function to detect the secure boot state of the
system.

The PPC_SECURE_BOOT config represents the base enablement of secureboot
on POWER.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig               | 11 +++++
 arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
 arch/powerpc/kernel/Makefile       |  2 +
 arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+)
 create mode 100644 arch/powerpc/include/asm/secboot.h
 create mode 100644 arch/powerpc/kernel/secboot.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 77f6ebf97113..c902a39124dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -912,6 +912,17 @@ config PPC_MEM_KEYS
 
 	  If unsure, say y.
 
+config PPC_SECURE_BOOT
+	prompt "Enable PowerPC Secure Boot"
+	bool
+	default n
+	depends on PPC64
+	help
+	  Linux on POWER with firmware secure boot enabled needs to define
+	  security policies to extend secure boot to the OS.This config
+	  allows user to enable OS Secure Boot on PowerPC systems that
+	  have firmware secure boot support.
+
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h
new file mode 100644
index 000000000000..e726261bb00b
--- /dev/null
+++ b/arch/powerpc/include/asm/secboot.h
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * PowerPC secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ */
+#ifndef POWERPC_SECBOOT_H
+#define POWERPC_SECBOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+extern struct device_node *is_powerpc_secvar_supported(void);
+extern bool get_powerpc_secureboot(void);
+#else
+static inline struct device_node *is_powerpc_secvar_supported(void)
+{
+	return NULL;
+}
+
+static inline bool get_powerpc_secureboot(void)
+{
+	return false;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index ea0c69236789..d310ebb4e526 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,6 +157,8 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
+
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
 KCOV_INSTRUMENT_prom_init.o := n
diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
new file mode 100644
index 000000000000..5ea0d52d64ef
--- /dev/null
+++ b/arch/powerpc/kernel/secboot.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * secboot.c
+ *      - util function to get powerpc secboot state
+ */
+#include <linux/types.h>
+#include <linux/of.h>
+#include <asm/secboot.h>
+
+struct device_node *is_powerpc_secvar_supported(void)
+{
+	struct device_node *np;
+	int status;
+
+	np = of_find_node_by_name(NULL, "ibm,secureboot");
+	if (!np) {
+		pr_info("secureboot node is not found\n");
+		return NULL;
+	}
+
+	status = of_device_is_compatible(np, "ibm,secureboot-v3");
+	if (!status) {
+		pr_info("Secure variables are not supported by this firmware\n");
+		return NULL;
+	}
+
+	return np;
+}
+
+bool get_powerpc_secureboot(void)
+{
+	struct device_node *np;
+	struct device_node *secvar_np;
+	const u64 *psecboot;
+	u64 secboot = 0;
+
+	np = is_powerpc_secvar_supported();
+	if (!np)
+		goto disabled;
+
+	/* Fail-safe for any failure related to secvar */
+	secvar_np = of_get_child_by_name(np, "secvar");
+	if (!secvar_np) {
+		pr_err("Expected secure variables support, fail-safe\n");
+		goto enabled;
+	}
+
+	if (!of_device_is_available(secvar_np)) {
+		pr_err("Secure variables support is in error state, fail-safe\n");
+		goto enabled;
+	}
+
+	psecboot = of_get_property(secvar_np, "secure-mode", NULL);
+	if (!psecboot)
+		goto enabled;
+
+	secboot = be64_to_cpup((__be64 *)psecboot);
+	if (!(secboot & (~0x0)))
+		goto disabled;
+
+enabled:
+	pr_info("secureboot mode enabled\n");
+	return true;
+
+disabled:
+	pr_info("secureboot mode disabled\n");
+	return false;
+}
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
  2019-08-19 12:35 ` Nayna Jain
@ 2019-08-19 12:35   ` Nayna Jain
  -1 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Michael Ellerman, Paul Mackerras, Benjamin Herrenschmidt,
	Ard Biesheuvel, Jeremy Kerr, Matthew Garret, Mimi Zohar,
	Claudio Carvalho, Elaine Palmer, George Wilson, Eric Ricther,
	Nayna Jain

POWER secure boot relies on the kernel IMA security subsystem to
perform the OS kernel image signature verification. Since each secure
boot mode has different IMA policy requirements, dynamic definition of
the policy rules based on the runtime secure boot mode of the system is
required. On systems that support secure boot, but have it disabled,
only measurement policy rules of the kernel image and modules are
defined.

This patch defines the arch-specific implementation to retrieve the
secure boot mode of the system and accordingly configures the IMA policy
rules.

This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig           |  2 ++
 arch/powerpc/kernel/Makefile   |  2 +-
 arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
 include/linux/ima.h            |  3 +-
 4 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/kernel/ima_arch.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c902a39124dc..42109682b727 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
 	bool
 	default n
 	depends on PPC64
+	depends on IMA
+	depends on IMA_ARCH_POLICY
 	help
 	  Linux on POWER with firmware secure boot enabled needs to define
 	  security policies to extend secure boot to the OS.This config
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index d310ebb4e526..520b1c814197 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
-obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
 
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index 000000000000..ac90fac83338
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * ima_arch.c
+ *      - initialize ima policies for PowerPC Secure Boot
+ */
+
+#include <linux/ima.h>
+#include <asm/secboot.h>
+
+bool arch_ima_get_secureboot(void)
+{
+	return get_powerpc_secureboot();
+}
+
+/*
+ * File signature verification is not needed, include only measurements
+ */
+static const char *const default_arch_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK",
+	"measure func=MODULE_CHECK",
+	NULL
+};
+
+/* Both file signature verification and measurements are needed */
+static const char *const sb_arch_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if IS_ENABLED(CONFIG_MODULE_SIG)
+	"measure func=MODULE_CHECK",
+#else
+	"measure func=MODULE_CHECK template=ima-modsig",
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
+/*
+ * On PowerPC, file measurements are to be added to the IMA measurement list
+ * irrespective of the secure boot state of the system. Signature verification
+ * is conditionally enabled based on the secure boot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
+		return sb_arch_rules;
+	return default_arch_rules;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..10af09b5b478 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
+#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
+	|| defined(CONFIG_PPC_SECURE_BOOT)
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
@ 2019-08-19 12:35   ` Nayna Jain
  0 siblings, 0 replies; 13+ messages in thread
From: Nayna Jain @ 2019-08-19 12:35 UTC (permalink / raw)
  To: linuxppc-dev, linux-integrity, linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
	Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
	Elaine Palmer, George Wilson

POWER secure boot relies on the kernel IMA security subsystem to
perform the OS kernel image signature verification. Since each secure
boot mode has different IMA policy requirements, dynamic definition of
the policy rules based on the runtime secure boot mode of the system is
required. On systems that support secure boot, but have it disabled,
only measurement policy rules of the kernel image and modules are
defined.

This patch defines the arch-specific implementation to retrieve the
secure boot mode of the system and accordingly configures the IMA policy
rules.

This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
config is enabled.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig           |  2 ++
 arch/powerpc/kernel/Makefile   |  2 +-
 arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
 include/linux/ima.h            |  3 +-
 4 files changed, 55 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/kernel/ima_arch.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index c902a39124dc..42109682b727 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
 	bool
 	default n
 	depends on PPC64
+	depends on IMA
+	depends on IMA_ARCH_POLICY
 	help
 	  Linux on POWER with firmware secure boot enabled needs to define
 	  security policies to extend secure boot to the OS.This config
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index d310ebb4e526..520b1c814197 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -157,7 +157,7 @@ endif
 obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
 obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
 
-obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
 
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
 GCOV_PROFILE_prom_init.o := n
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
new file mode 100644
index 000000000000..ac90fac83338
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,50 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain <nayna@linux.ibm.com>
+ *
+ * ima_arch.c
+ *      - initialize ima policies for PowerPC Secure Boot
+ */
+
+#include <linux/ima.h>
+#include <asm/secboot.h>
+
+bool arch_ima_get_secureboot(void)
+{
+	return get_powerpc_secureboot();
+}
+
+/*
+ * File signature verification is not needed, include only measurements
+ */
+static const char *const default_arch_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK",
+	"measure func=MODULE_CHECK",
+	NULL
+};
+
+/* Both file signature verification and measurements are needed */
+static const char *const sb_arch_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if IS_ENABLED(CONFIG_MODULE_SIG)
+	"measure func=MODULE_CHECK",
+#else
+	"measure func=MODULE_CHECK template=ima-modsig",
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
+/*
+ * On PowerPC, file measurements are to be added to the IMA measurement list
+ * irrespective of the secure boot state of the system. Signature verification
+ * is conditionally enabled based on the secure boot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
+		return sb_arch_rules;
+	return default_arch_rules;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index a20ad398d260..10af09b5b478 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
-#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
+#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
+	|| defined(CONFIG_PPC_SECURE_BOOT)
 extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
  2019-08-19 12:35   ` Nayna Jain
@ 2019-09-02 11:52     ` Michael Ellerman
  -1 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-09-02 11:52 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-integrity, linux-kernel
  Cc: Paul Mackerras, Benjamin Herrenschmidt, Ard Biesheuvel,
	Jeremy Kerr, Matthew Garret, Mimi Zohar, Claudio Carvalho,
	Elaine Palmer, George Wilson, Eric Ricther, Nayna Jain

Hi Nayna,

Sorry I've taken so long to get to this series, there's just too many
patches that need reviewing :/

Nayna Jain <nayna@linux.ibm.com> writes:
> Secure boot on POWER defines different IMA policies based on the secure
> boot state of the system.

The terminology throughout is a bit vague, we have POWER, PowerPC, Linux
on POWER etc.

What this patch is talking about is a particular implemention of secure
boot on some OpenPOWER machines running bare metal - am I right?

So saying "Secure boot on POWER defines different IMA policies" is a bit
broad I think. Really we've just decided that a way to implement secure
boot is to use IMA policies.

> This patch defines a function to detect the secure boot state of the
> system.
>
> The PPC_SECURE_BOOT config represents the base enablement of secureboot
> on POWER.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig               | 11 +++++
>  arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
>  arch/powerpc/kernel/Makefile       |  2 +
>  arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+)
>  create mode 100644 arch/powerpc/include/asm/secboot.h
>  create mode 100644 arch/powerpc/kernel/secboot.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 77f6ebf97113..c902a39124dc 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -912,6 +912,17 @@ config PPC_MEM_KEYS
>  
>  	  If unsure, say y.
>  
> +config PPC_SECURE_BOOT
> +	prompt "Enable PowerPC Secure Boot"

How about "Enable secure boot support"

> +	bool
> +	default n

The default is 'n', so you don't need that default line.

> +	depends on PPC64

Should it just depend on POWERNV for now? AFAIK there's nothing in here
that's necessarily going to be shared with the guest secure boot code is
there?

> +	help
> +	  Linux on POWER with firmware secure boot enabled needs to define
> +	  security policies to extend secure boot to the OS.This config
> +	  allows user to enable OS Secure Boot on PowerPC systems that
> +	  have firmware secure boot support.

Again POWER vs PowerPC.

I think something like:

"Enable support for secure boot on some systems that have firmware
support for it. If in doubt say N."


> diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h

secure_boot.h would be fine.

> new file mode 100644
> index 000000000000..e726261bb00b
> --- /dev/null
> +++ b/arch/powerpc/include/asm/secboot.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PowerPC secure boot definitions
> + *
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>

I prefer to not have email addresses in copyright headers, as they just
bit rot. Your email is in the git log.

> + *
> + */
> +#ifndef POWERPC_SECBOOT_H
> +#define POWERPC_SECBOOT_H

We usually do _ASM_POWERPC_SECBOOT_H (or _ASM_POWERPC_SECURE_BOOT_H).

> +#ifdef CONFIG_PPC_SECURE_BOOT
> +extern struct device_node *is_powerpc_secvar_supported(void);
> +extern bool get_powerpc_secureboot(void);

You don't need 'extern' for functions in headers.

> +#else
> +static inline struct device_node *is_powerpc_secvar_supported(void)
> +{
> +	return NULL;
> +}
> +
> +static inline bool get_powerpc_secureboot(void)
> +{
> +	return false;
> +}
> +
> +#endif
> +#endif
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index ea0c69236789..d310ebb4e526 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -157,6 +157,8 @@ endif
>  obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>  obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>  
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
> +
>  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>  GCOV_PROFILE_prom_init.o := n
>  KCOV_INSTRUMENT_prom_init.o := n
> diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
> new file mode 100644
> index 000000000000..5ea0d52d64ef
> --- /dev/null
> +++ b/arch/powerpc/kernel/secboot.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * secboot.c
> + *      - util function to get powerpc secboot state

That's not really necessary.

> + */
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <asm/secboot.h>
> +
> +struct device_node *is_powerpc_secvar_supported(void)

This is a pretty weird signature. The "is_" implies it will return a
bool, but then it actually returns a device node *.

> +{
> +	struct device_node *np;
> +	int status;
> +
> +	np = of_find_node_by_name(NULL, "ibm,secureboot");
> +	if (!np) {
> +		pr_info("secureboot node is not found\n");
> +		return NULL;
> +	}

There's no good reason to search by name. You should just search by compatible.

eg. of_find_compatible_node()

> +	status = of_device_is_compatible(np, "ibm,secureboot-v3");
> +	if (!status) {
> +		pr_info("Secure variables are not supported by this firmware\n");
> +		return NULL;
> +	}
> +
> +	return np;
> +}
> +
> +bool get_powerpc_secureboot(void)
> +{
> +	struct device_node *np;
> +	struct device_node *secvar_np;
> +	const u64 *psecboot;
> +	u64 secboot = 0;
> +
> +	np = is_powerpc_secvar_supported();
> +	if (!np)
> +		goto disabled;
> +
> +	/* Fail-safe for any failure related to secvar */
> +	secvar_np = of_get_child_by_name(np, "secvar");

Finding a child by name is not ideal, it encodes the structure of the
tree in the API. It's better to just search by compatible.

eg. of_find_compatible_node("ibm,secvar-v1")

You should also define what that means, ie. write a little snippet of
doc to define what the expected properties are and their meaning and so
on.

> +	if (!secvar_np) {
> +		pr_err("Expected secure variables support, fail-safe\n");

I'm a bit confused by this. This is the exact opposite of what I
understand fail-safe to mean. We shouldn't tell the user the system is
securely booted unless we're 100% sure it is. Right?

> +		goto enabled;
> +	}
> +
> +	if (!of_device_is_available(secvar_np)) {
> +		pr_err("Secure variables support is in error state, fail-safe\n");
> +		goto enabled;
> +	}

It seems a little weird to use the status property to indicate ok/error
and then also have a "secure-mode" property. Wouldn't just "secure-mode"
be sufficient with several states to represent what we need?

> +	psecboot = of_get_property(secvar_np, "secure-mode", NULL);
> +	if (!psecboot)
> +		goto enabled;

Please use of_read_property_u64() or similar.

> +	secboot = be64_to_cpup((__be64 *)psecboot);
> +	if (!(secboot & (~0x0)))

I'm not sure what that's trying to do.

> +		goto disabled;

> +
> +enabled:
> +	pr_info("secureboot mode enabled\n");
> +	return true;
> +
> +disabled:
> +	pr_info("secureboot mode disabled\n");
> +	return false;
> +}


cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
@ 2019-09-02 11:52     ` Michael Ellerman
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-09-02 11:52 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-integrity, linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
	Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
	Elaine Palmer, George Wilson

Hi Nayna,

Sorry I've taken so long to get to this series, there's just too many
patches that need reviewing :/

Nayna Jain <nayna@linux.ibm.com> writes:
> Secure boot on POWER defines different IMA policies based on the secure
> boot state of the system.

The terminology throughout is a bit vague, we have POWER, PowerPC, Linux
on POWER etc.

What this patch is talking about is a particular implemention of secure
boot on some OpenPOWER machines running bare metal - am I right?

So saying "Secure boot on POWER defines different IMA policies" is a bit
broad I think. Really we've just decided that a way to implement secure
boot is to use IMA policies.

> This patch defines a function to detect the secure boot state of the
> system.
>
> The PPC_SECURE_BOOT config represents the base enablement of secureboot
> on POWER.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig               | 11 +++++
>  arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
>  arch/powerpc/kernel/Makefile       |  2 +
>  arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
>  4 files changed, 111 insertions(+)
>  create mode 100644 arch/powerpc/include/asm/secboot.h
>  create mode 100644 arch/powerpc/kernel/secboot.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 77f6ebf97113..c902a39124dc 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -912,6 +912,17 @@ config PPC_MEM_KEYS
>  
>  	  If unsure, say y.
>  
> +config PPC_SECURE_BOOT
> +	prompt "Enable PowerPC Secure Boot"

How about "Enable secure boot support"

> +	bool
> +	default n

The default is 'n', so you don't need that default line.

> +	depends on PPC64

Should it just depend on POWERNV for now? AFAIK there's nothing in here
that's necessarily going to be shared with the guest secure boot code is
there?

> +	help
> +	  Linux on POWER with firmware secure boot enabled needs to define
> +	  security policies to extend secure boot to the OS.This config
> +	  allows user to enable OS Secure Boot on PowerPC systems that
> +	  have firmware secure boot support.

Again POWER vs PowerPC.

I think something like:

"Enable support for secure boot on some systems that have firmware
support for it. If in doubt say N."


> diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h

secure_boot.h would be fine.

> new file mode 100644
> index 000000000000..e726261bb00b
> --- /dev/null
> +++ b/arch/powerpc/include/asm/secboot.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * PowerPC secure boot definitions
> + *
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>

I prefer to not have email addresses in copyright headers, as they just
bit rot. Your email is in the git log.

> + *
> + */
> +#ifndef POWERPC_SECBOOT_H
> +#define POWERPC_SECBOOT_H

We usually do _ASM_POWERPC_SECBOOT_H (or _ASM_POWERPC_SECURE_BOOT_H).

> +#ifdef CONFIG_PPC_SECURE_BOOT
> +extern struct device_node *is_powerpc_secvar_supported(void);
> +extern bool get_powerpc_secureboot(void);

You don't need 'extern' for functions in headers.

> +#else
> +static inline struct device_node *is_powerpc_secvar_supported(void)
> +{
> +	return NULL;
> +}
> +
> +static inline bool get_powerpc_secureboot(void)
> +{
> +	return false;
> +}
> +
> +#endif
> +#endif
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index ea0c69236789..d310ebb4e526 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -157,6 +157,8 @@ endif
>  obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>  obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>  
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
> +
>  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>  GCOV_PROFILE_prom_init.o := n
>  KCOV_INSTRUMENT_prom_init.o := n
> diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
> new file mode 100644
> index 000000000000..5ea0d52d64ef
> --- /dev/null
> +++ b/arch/powerpc/kernel/secboot.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * secboot.c
> + *      - util function to get powerpc secboot state

That's not really necessary.

> + */
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <asm/secboot.h>
> +
> +struct device_node *is_powerpc_secvar_supported(void)

This is a pretty weird signature. The "is_" implies it will return a
bool, but then it actually returns a device node *.

> +{
> +	struct device_node *np;
> +	int status;
> +
> +	np = of_find_node_by_name(NULL, "ibm,secureboot");
> +	if (!np) {
> +		pr_info("secureboot node is not found\n");
> +		return NULL;
> +	}

There's no good reason to search by name. You should just search by compatible.

eg. of_find_compatible_node()

> +	status = of_device_is_compatible(np, "ibm,secureboot-v3");
> +	if (!status) {
> +		pr_info("Secure variables are not supported by this firmware\n");
> +		return NULL;
> +	}
> +
> +	return np;
> +}
> +
> +bool get_powerpc_secureboot(void)
> +{
> +	struct device_node *np;
> +	struct device_node *secvar_np;
> +	const u64 *psecboot;
> +	u64 secboot = 0;
> +
> +	np = is_powerpc_secvar_supported();
> +	if (!np)
> +		goto disabled;
> +
> +	/* Fail-safe for any failure related to secvar */
> +	secvar_np = of_get_child_by_name(np, "secvar");

Finding a child by name is not ideal, it encodes the structure of the
tree in the API. It's better to just search by compatible.

eg. of_find_compatible_node("ibm,secvar-v1")

You should also define what that means, ie. write a little snippet of
doc to define what the expected properties are and their meaning and so
on.

> +	if (!secvar_np) {
> +		pr_err("Expected secure variables support, fail-safe\n");

I'm a bit confused by this. This is the exact opposite of what I
understand fail-safe to mean. We shouldn't tell the user the system is
securely booted unless we're 100% sure it is. Right?

> +		goto enabled;
> +	}
> +
> +	if (!of_device_is_available(secvar_np)) {
> +		pr_err("Secure variables support is in error state, fail-safe\n");
> +		goto enabled;
> +	}

It seems a little weird to use the status property to indicate ok/error
and then also have a "secure-mode" property. Wouldn't just "secure-mode"
be sufficient with several states to represent what we need?

> +	psecboot = of_get_property(secvar_np, "secure-mode", NULL);
> +	if (!psecboot)
> +		goto enabled;

Please use of_read_property_u64() or similar.

> +	secboot = be64_to_cpup((__be64 *)psecboot);
> +	if (!(secboot & (~0x0)))

I'm not sure what that's trying to do.

> +		goto disabled;

> +
> +enabled:
> +	pr_info("secureboot mode enabled\n");
> +	return true;
> +
> +disabled:
> +	pr_info("secureboot mode disabled\n");
> +	return false;
> +}


cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
  2019-08-19 12:35   ` Nayna Jain
@ 2019-09-02 11:52     ` Michael Ellerman
  -1 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-09-02 11:52 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-integrity, linux-kernel
  Cc: Paul Mackerras, Benjamin Herrenschmidt, Ard Biesheuvel,
	Jeremy Kerr, Matthew Garret, Mimi Zohar, Claudio Carvalho,
	Elaine Palmer, George Wilson, Eric Ricther, Nayna Jain

Hi Nayna,

Some more comments below.

Nayna Jain <nayna@linux.ibm.com> writes:
> POWER secure boot relies on the kernel IMA security subsystem to
> perform the OS kernel image signature verification.

Again this is just a design choice we've made, it's not specified
anywhere or anything like that. And it only applies to bare metal secure
boot, at least so far. AIUI.

> Since each secure
> boot mode has different IMA policy requirements, dynamic definition of
> the policy rules based on the runtime secure boot mode of the system is
> required. On systems that support secure boot, but have it disabled,
> only measurement policy rules of the kernel image and modules are
> defined.

It's probably worth mentioning that we intend to use this in our
Linux-based boot loader, which uses kexec, and that's one of the reasons
why we're particularly interested in defining the rules for kexec?

> This patch defines the arch-specific implementation to retrieve the
> secure boot mode of the system and accordingly configures the IMA policy
> rules.
>
> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
> config is enabled.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig           |  2 ++
>  arch/powerpc/kernel/Makefile   |  2 +-
>  arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
>  include/linux/ima.h            |  3 +-
>  4 files changed, 55 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/kernel/ima_arch.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c902a39124dc..42109682b727 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
>  	bool
>  	default n
>  	depends on PPC64
> +	depends on IMA
> +	depends on IMA_ARCH_POLICY
>  	help
>  	  Linux on POWER with firmware secure boot enabled needs to define
>  	  security policies to extend secure boot to the OS.This config
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index d310ebb4e526..520b1c814197 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -157,7 +157,7 @@ endif
>  obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>  obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>  
> -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
>  
>  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>  GCOV_PROFILE_prom_init.o := n
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> new file mode 100644
> index 000000000000..ac90fac83338
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * ima_arch.c
> + *      - initialize ima policies for PowerPC Secure Boot
> + */
> +
> +#include <linux/ima.h>
> +#include <asm/secboot.h>
> +
> +bool arch_ima_get_secureboot(void)
> +{
> +	return get_powerpc_secureboot();
> +}
> +
> +/*
> + * File signature verification is not needed, include only measurements
> + */
> +static const char *const default_arch_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK",
> +	"measure func=MODULE_CHECK",
> +	NULL
> +};

The rules above seem fairly self explanatory.

> +
> +/* Both file signature verification and measurements are needed */
> +static const char *const sb_arch_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#if IS_ENABLED(CONFIG_MODULE_SIG)
> +	"measure func=MODULE_CHECK",
> +#else
> +	"measure func=MODULE_CHECK template=ima-modsig",
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif

But these ones are not so obvious, at least to me who knows very little
about IMA.

Can you add a one line comment to each of the ones in here saying what
it does and why we want it?

> +	NULL
> +};
> +
> +/*
> + * On PowerPC, file measurements are to be added to the IMA measurement list
> + * irrespective of the secure boot state of the system.

Why? Just because we think it's useful? Would be good to provide some
further justification.

    * Signature verification
> + * is conditionally enabled based on the secure boot state.
> + */
> +const char *const *arch_get_ima_policy(void)
> +{
> +	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
> +		return sb_arch_rules;
> +	return default_arch_rules;
> +}
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..10af09b5b478 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
>  
> -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
> +#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> +	|| defined(CONFIG_PPC_SECURE_BOOT)
>  extern bool arch_ima_get_secureboot(void);
>  extern const char * const *arch_get_ima_policy(void);
>  #else


cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
@ 2019-09-02 11:52     ` Michael Ellerman
  0 siblings, 0 replies; 13+ messages in thread
From: Michael Ellerman @ 2019-09-02 11:52 UTC (permalink / raw)
  To: Nayna Jain, linuxppc-dev, linux-integrity, linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Nayna Jain, Claudio Carvalho,
	Mimi Zohar, Matthew Garret, Paul Mackerras, Jeremy Kerr,
	Elaine Palmer, George Wilson

Hi Nayna,

Some more comments below.

Nayna Jain <nayna@linux.ibm.com> writes:
> POWER secure boot relies on the kernel IMA security subsystem to
> perform the OS kernel image signature verification.

Again this is just a design choice we've made, it's not specified
anywhere or anything like that. And it only applies to bare metal secure
boot, at least so far. AIUI.

> Since each secure
> boot mode has different IMA policy requirements, dynamic definition of
> the policy rules based on the runtime secure boot mode of the system is
> required. On systems that support secure boot, but have it disabled,
> only measurement policy rules of the kernel image and modules are
> defined.

It's probably worth mentioning that we intend to use this in our
Linux-based boot loader, which uses kexec, and that's one of the reasons
why we're particularly interested in defining the rules for kexec?

> This patch defines the arch-specific implementation to retrieve the
> secure boot mode of the system and accordingly configures the IMA policy
> rules.
>
> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
> config is enabled.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig           |  2 ++
>  arch/powerpc/kernel/Makefile   |  2 +-
>  arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
>  include/linux/ima.h            |  3 +-
>  4 files changed, 55 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/kernel/ima_arch.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index c902a39124dc..42109682b727 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
>  	bool
>  	default n
>  	depends on PPC64
> +	depends on IMA
> +	depends on IMA_ARCH_POLICY
>  	help
>  	  Linux on POWER with firmware secure boot enabled needs to define
>  	  security policies to extend secure boot to the OS.This config
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index d310ebb4e526..520b1c814197 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -157,7 +157,7 @@ endif
>  obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>  obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>  
> -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
>  
>  # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>  GCOV_PROFILE_prom_init.o := n
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> new file mode 100644
> index 000000000000..ac90fac83338
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain <nayna@linux.ibm.com>
> + *
> + * ima_arch.c
> + *      - initialize ima policies for PowerPC Secure Boot
> + */
> +
> +#include <linux/ima.h>
> +#include <asm/secboot.h>
> +
> +bool arch_ima_get_secureboot(void)
> +{
> +	return get_powerpc_secureboot();
> +}
> +
> +/*
> + * File signature verification is not needed, include only measurements
> + */
> +static const char *const default_arch_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK",
> +	"measure func=MODULE_CHECK",
> +	NULL
> +};

The rules above seem fairly self explanatory.

> +
> +/* Both file signature verification and measurements are needed */
> +static const char *const sb_arch_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#if IS_ENABLED(CONFIG_MODULE_SIG)
> +	"measure func=MODULE_CHECK",
> +#else
> +	"measure func=MODULE_CHECK template=ima-modsig",
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif

But these ones are not so obvious, at least to me who knows very little
about IMA.

Can you add a one line comment to each of the ones in here saying what
it does and why we want it?

> +	NULL
> +};
> +
> +/*
> + * On PowerPC, file measurements are to be added to the IMA measurement list
> + * irrespective of the secure boot state of the system.

Why? Just because we think it's useful? Would be good to provide some
further justification.

    * Signature verification
> + * is conditionally enabled based on the secure boot state.
> + */
> +const char *const *arch_get_ima_policy(void)
> +{
> +	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot())
> +		return sb_arch_rules;
> +	return default_arch_rules;
> +}
> diff --git a/include/linux/ima.h b/include/linux/ima.h
> index a20ad398d260..10af09b5b478 100644
> --- a/include/linux/ima.h
> +++ b/include/linux/ima.h
> @@ -29,7 +29,8 @@ extern void ima_kexec_cmdline(const void *buf, int size);
>  extern void ima_add_kexec_buffer(struct kimage *image);
>  #endif
>  
> -#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390)
> +#if (defined(CONFIG_X86) && defined(CONFIG_EFI)) || defined(CONFIG_S390) \
> +	|| defined(CONFIG_PPC_SECURE_BOOT)
>  extern bool arch_ima_get_secureboot(void);
>  extern const char * const *arch_get_ima_policy(void);
>  #else


cheers

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] powerpc: detect the secure boot mode of the system
  2019-09-02 11:52     ` Michael Ellerman
  (?)
@ 2019-09-05 11:32     ` Nayna
  -1 siblings, 0 replies; 13+ messages in thread
From: Nayna @ 2019-09-05 11:32 UTC (permalink / raw)
  To: Michael Ellerman, Nayna Jain, linuxppc-dev, linux-integrity,
	linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Claudio Carvalho, Mimi Zohar,
	Matthew Garret, Paul Mackerras, Jeremy Kerr, Elaine Palmer,
	George Wilson



On 09/02/2019 07:52 AM, Michael Ellerman wrote:
> Hi Nayna,

Hi Michael,

>
> Sorry I've taken so long to get to this series, there's just too many
> patches that need reviewing :/

No problem. I understand. Thanks for reviewing.

>
> Nayna Jain <nayna@linux.ibm.com> writes:
>> Secure boot on POWER defines different IMA policies based on the secure
>> boot state of the system.
> The terminology throughout is a bit vague, we have POWER, PowerPC, Linux
> on POWER etc.
>
> What this patch is talking about is a particular implemention of secure
> boot on some OpenPOWER machines running bare metal - am I right?
>
> So saying "Secure boot on POWER defines different IMA policies" is a bit
> broad I think. Really we've just decided that a way to implement secure
> boot is to use IMA policies.

I think the idea was to convey that the same design can be reused or 
extended as needed.
But I agree for now it is currently only OpenPOWER machines running on 
bare metal, I will fix the wordings to use "PowerNV" consistently.



>
>> This patch defines a function to detect the secure boot state of the
>> system.
>>
>> The PPC_SECURE_BOOT config represents the base enablement of secureboot
>> on POWER.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig               | 11 +++++
>>   arch/powerpc/include/asm/secboot.h | 27 ++++++++++++
>>   arch/powerpc/kernel/Makefile       |  2 +
>>   arch/powerpc/kernel/secboot.c      | 71 ++++++++++++++++++++++++++++++
>>   4 files changed, 111 insertions(+)
>>   create mode 100644 arch/powerpc/include/asm/secboot.h
>>   create mode 100644 arch/powerpc/kernel/secboot.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 77f6ebf97113..c902a39124dc 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -912,6 +912,17 @@ config PPC_MEM_KEYS
>>   
>>   	  If unsure, say y.
>>   
>> +config PPC_SECURE_BOOT
>> +	prompt "Enable PowerPC Secure Boot"
> How about "Enable secure boot support"

Yes. Sounds better.

>
>> +	bool
>> +	default n
> The default is 'n', so you don't need that default line.

Sure.


>
>> +	depends on PPC64
> Should it just depend on POWERNV for now? AFAIK there's nothing in here
> that's necessarily going to be shared with the guest secure boot code is
> there?

Yes. sounds good.


>
>> +	help
>> +	  Linux on POWER with firmware secure boot enabled needs to define
>> +	  security policies to extend secure boot to the OS.This config
>> +	  allows user to enable OS Secure Boot on PowerPC systems that
>> +	  have firmware secure boot support.
> Again POWER vs PowerPC.
>
> I think something like:
>
> "Enable support for secure boot on some systems that have firmware
> support for it. If in doubt say N."

Sure.

>
>
>> diff --git a/arch/powerpc/include/asm/secboot.h b/arch/powerpc/include/asm/secboot.h
> secure_boot.h would be fine.

Sure.

>
>> new file mode 100644
>> index 000000000000..e726261bb00b
>> --- /dev/null
>> +++ b/arch/powerpc/include/asm/secboot.h
>> @@ -0,0 +1,27 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * PowerPC secure boot definitions
>> + *
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
> I prefer to not have email addresses in copyright headers, as they just
> bit rot. Your email is in the git log.

Sure.


>
>> + *
>> + */
>> +#ifndef POWERPC_SECBOOT_H
>> +#define POWERPC_SECBOOT_H
> We usually do _ASM_POWERPC_SECBOOT_H (or _ASM_POWERPC_SECURE_BOOT_H).

Sure.

>
>> +#ifdef CONFIG_PPC_SECURE_BOOT
>> +extern struct device_node *is_powerpc_secvar_supported(void);
>> +extern bool get_powerpc_secureboot(void);
> You don't need 'extern' for functions in headers.

Yes. will fix.

>
>> +#else
>> +static inline struct device_node *is_powerpc_secvar_supported(void)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline bool get_powerpc_secureboot(void)
>> +{
>> +	return false;
>> +}
>> +
>> +#endif
>> +#endif
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index ea0c69236789..d310ebb4e526 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -157,6 +157,8 @@ endif
>>   obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>>   obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>>   
>> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
>> +
>>   # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>>   GCOV_PROFILE_prom_init.o := n
>>   KCOV_INSTRUMENT_prom_init.o := n
>> diff --git a/arch/powerpc/kernel/secboot.c b/arch/powerpc/kernel/secboot.c
>> new file mode 100644
>> index 000000000000..5ea0d52d64ef
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/secboot.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
>> + *
>> + * secboot.c
>> + *      - util function to get powerpc secboot state
> That's not really necessary.

Sure.

>
>> + */
>> +#include <linux/types.h>
>> +#include <linux/of.h>
>> +#include <asm/secboot.h>
>> +
>> +struct device_node *is_powerpc_secvar_supported(void)
> This is a pretty weird signature. The "is_" implies it will return a
> bool, but then it actually returns a device node *.

Yes. Agree. Will fix.

>
>> +{
>> +	struct device_node *np;
>> +	int status;
>> +
>> +	np = of_find_node_by_name(NULL, "ibm,secureboot");
>> +	if (!np) {
>> +		pr_info("secureboot node is not found\n");
>> +		return NULL;
>> +	}
> There's no good reason to search by name. You should just search by compatible.
>
> eg. of_find_compatible_node()

Sure.


>
>> +	status = of_device_is_compatible(np, "ibm,secureboot-v3");
>> +	if (!status) {
>> +		pr_info("Secure variables are not supported by this firmware\n");
>> +		return NULL;
>> +	}
>> +
>> +	return np;
>> +}
>> +
>> +bool get_powerpc_secureboot(void)
>> +{
>> +	struct device_node *np;
>> +	struct device_node *secvar_np;
>> +	const u64 *psecboot;
>> +	u64 secboot = 0;
>> +
>> +	np = is_powerpc_secvar_supported();
>> +	if (!np)
>> +		goto disabled;
>> +
>> +	/* Fail-safe for any failure related to secvar */
>> +	secvar_np = of_get_child_by_name(np, "secvar");
> Finding a child by name is not ideal, it encodes the structure of the
> tree in the API. It's better to just search by compatible.
>
> eg. of_find_compatible_node("ibm,secvar-v1")
>
> You should also define what that means, ie. write a little snippet of
> doc to define what the expected properties are and their meaning and so
> on.

It is part of the skiboot patches 
(https://patchwork.ozlabs.org/patch/1157346/)
Sure. I will add one in the kernel as well.


>
>> +	if (!secvar_np) {
>> +		pr_err("Expected secure variables support, fail-safe\n");
> I'm a bit confused by this. This is the exact opposite of what I
> understand fail-safe to mean. We shouldn't tell the user the system is
> securely booted unless we're 100% sure it is. Right?

Yes. Thanks for pointing this out. "Fail secure" is the correct term. 
This is the situation where secure variables are supported. Any failure 
reading the secure variables may be an attack, so we fail securely.


>
>> +		goto enabled;
>> +	}
>> +
>> +	if (!of_device_is_available(secvar_np)) {
>> +		pr_err("Secure variables support is in error state, fail-safe\n");
>> +		goto enabled;
>> +	}
> It seems a little weird to use the status property to indicate ok/error
> and then also have a "secure-mode" property. Wouldn't just "secure-mode"
> be sufficient with several states to represent what we need?

Before we check in which mode the system booted (e.g. setup, user, etc) 
using "secure mode" property, the "status" check helps to ensure that 
the system secure boot initialized correctly. We will look into
combining the two variables.



>
>> +	psecboot = of_get_property(secvar_np, "secure-mode", NULL);
>> +	if (!psecboot)
>> +		goto enabled;
> Please use of_read_property_u64() or similar.

Sure.

>
>> +	secboot = be64_to_cpup((__be64 *)psecboot);
>> +	if (!(secboot & (~0x0)))
> I'm not sure what that's trying to do.

We are exposing secure modes from the skiboot to the kernel via a 
bitfield, partitioned into generic modes and backend specific modes. I 
will update this code so that it is clearer.

Thanks & Regards,
       - Nayna

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
  2019-09-02 11:52     ` Michael Ellerman
@ 2019-09-05 12:31       ` Nayna
  -1 siblings, 0 replies; 13+ messages in thread
From: Nayna @ 2019-09-05 12:31 UTC (permalink / raw)
  To: Michael Ellerman, Nayna Jain, linuxppc-dev, linux-integrity,
	linux-kernel
  Cc: Paul Mackerras, Benjamin Herrenschmidt, Ard Biesheuvel,
	Jeremy Kerr, Matthew Garret, Mimi Zohar, Claudio Carvalho,
	Elaine Palmer, George Wilson, Eric Ricther



On 09/02/2019 07:52 AM, Michael Ellerman wrote:
> Hi Nayna,

Hi Michael,

>
> Some more comments below.
>
> Nayna Jain <nayna@linux.ibm.com> writes:
>> POWER secure boot relies on the kernel IMA security subsystem to
>> perform the OS kernel image signature verification.
> Again this is just a design choice we've made, it's not specified
> anywhere or anything like that. And it only applies to bare metal secure
> boot, at least so far. AIUI.

Yes. I will make it consistent to use "PowerNV".

>> Since each secure
>> boot mode has different IMA policy requirements, dynamic definition of
>> the policy rules based on the runtime secure boot mode of the system is
>> required. On systems that support secure boot, but have it disabled,
>> only measurement policy rules of the kernel image and modules are
>> defined.
> It's probably worth mentioning that we intend to use this in our
> Linux-based boot loader, which uses kexec, and that's one of the reasons
> why we're particularly interested in defining the rules for kexec?

Yes. Agreed. I will update patch description to add this.

>
>> This patch defines the arch-specific implementation to retrieve the
>> secure boot mode of the system and accordingly configures the IMA policy
>> rules.
>>
>> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
>> config is enabled.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig           |  2 ++
>>   arch/powerpc/kernel/Makefile   |  2 +-
>>   arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
>>   include/linux/ima.h            |  3 +-
>>   4 files changed, 55 insertions(+), 2 deletions(-)
>>   create mode 100644 arch/powerpc/kernel/ima_arch.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index c902a39124dc..42109682b727 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
>>   	bool
>>   	default n
>>   	depends on PPC64
>> +	depends on IMA
>> +	depends on IMA_ARCH_POLICY
>>   	help
>>   	  Linux on POWER with firmware secure boot enabled needs to define
>>   	  security policies to extend secure boot to the OS.This config
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index d310ebb4e526..520b1c814197 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -157,7 +157,7 @@ endif
>>   obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>>   obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>>   
>> -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
>> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
>>   
>>   # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>>   GCOV_PROFILE_prom_init.o := n
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> new file mode 100644
>> index 000000000000..ac90fac83338
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
>> + *
>> + * ima_arch.c
>> + *      - initialize ima policies for PowerPC Secure Boot
>> + */
>> +
>> +#include <linux/ima.h>
>> +#include <asm/secboot.h>
>> +
>> +bool arch_ima_get_secureboot(void)
>> +{
>> +	return get_powerpc_secureboot();
>> +}
>> +
>> +/*
>> + * File signature verification is not needed, include only measurements
>> + */
>> +static const char *const default_arch_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK",
>> +	"measure func=MODULE_CHECK",
>> +	NULL
>> +};
> The rules above seem fairly self explanatory.
>
>> +
>> +/* Both file signature verification and measurements are needed */
>> +static const char *const sb_arch_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> +#if IS_ENABLED(CONFIG_MODULE_SIG)
>> +	"measure func=MODULE_CHECK",
>> +#else
>> +	"measure func=MODULE_CHECK template=ima-modsig",
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> +#endif
> But these ones are not so obvious, at least to me who knows very little
> about IMA.
>
> Can you add a one line comment to each of the ones in here saying what
> it does and why we want it?

Sure.

>
>> +	NULL
>> +};
>> +
>> +/*
>> + * On PowerPC, file measurements are to be added to the IMA measurement list
>> + * irrespective of the secure boot state of the system.
> Why? Just because we think it's useful? Would be good to provide some
> further justification.

Sure. I will clarify this in the next version.

Thanks & Regards,
         - Nayna

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules
@ 2019-09-05 12:31       ` Nayna
  0 siblings, 0 replies; 13+ messages in thread
From: Nayna @ 2019-09-05 12:31 UTC (permalink / raw)
  To: Michael Ellerman, Nayna Jain, linuxppc-dev, linux-integrity,
	linux-kernel
  Cc: Ard Biesheuvel, Eric Ricther, Claudio Carvalho, Mimi Zohar,
	Matthew Garret, Paul Mackerras, Jeremy Kerr, Elaine Palmer,
	George Wilson



On 09/02/2019 07:52 AM, Michael Ellerman wrote:
> Hi Nayna,

Hi Michael,

>
> Some more comments below.
>
> Nayna Jain <nayna@linux.ibm.com> writes:
>> POWER secure boot relies on the kernel IMA security subsystem to
>> perform the OS kernel image signature verification.
> Again this is just a design choice we've made, it's not specified
> anywhere or anything like that. And it only applies to bare metal secure
> boot, at least so far. AIUI.

Yes. I will make it consistent to use "PowerNV".

>> Since each secure
>> boot mode has different IMA policy requirements, dynamic definition of
>> the policy rules based on the runtime secure boot mode of the system is
>> required. On systems that support secure boot, but have it disabled,
>> only measurement policy rules of the kernel image and modules are
>> defined.
> It's probably worth mentioning that we intend to use this in our
> Linux-based boot loader, which uses kexec, and that's one of the reasons
> why we're particularly interested in defining the rules for kexec?

Yes. Agreed. I will update patch description to add this.

>
>> This patch defines the arch-specific implementation to retrieve the
>> secure boot mode of the system and accordingly configures the IMA policy
>> rules.
>>
>> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
>> config is enabled.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> ---
>>   arch/powerpc/Kconfig           |  2 ++
>>   arch/powerpc/kernel/Makefile   |  2 +-
>>   arch/powerpc/kernel/ima_arch.c | 50 ++++++++++++++++++++++++++++++++++
>>   include/linux/ima.h            |  3 +-
>>   4 files changed, 55 insertions(+), 2 deletions(-)
>>   create mode 100644 arch/powerpc/kernel/ima_arch.c
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index c902a39124dc..42109682b727 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -917,6 +917,8 @@ config PPC_SECURE_BOOT
>>   	bool
>>   	default n
>>   	depends on PPC64
>> +	depends on IMA
>> +	depends on IMA_ARCH_POLICY
>>   	help
>>   	  Linux on POWER with firmware secure boot enabled needs to define
>>   	  security policies to extend secure boot to the OS.This config
>> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
>> index d310ebb4e526..520b1c814197 100644
>> --- a/arch/powerpc/kernel/Makefile
>> +++ b/arch/powerpc/kernel/Makefile
>> @@ -157,7 +157,7 @@ endif
>>   obj-$(CONFIG_EPAPR_PARAVIRT)	+= epapr_paravirt.o epapr_hcalls.o
>>   obj-$(CONFIG_KVM_GUEST)		+= kvm.o kvm_emul.o
>>   
>> -obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o
>> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secboot.o ima_arch.o
>>   
>>   # Disable GCOV, KCOV & sanitizers in odd or sensitive code
>>   GCOV_PROFILE_prom_init.o := n
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> new file mode 100644
>> index 000000000000..ac90fac83338
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -0,0 +1,50 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain <nayna@linux.ibm.com>
>> + *
>> + * ima_arch.c
>> + *      - initialize ima policies for PowerPC Secure Boot
>> + */
>> +
>> +#include <linux/ima.h>
>> +#include <asm/secboot.h>
>> +
>> +bool arch_ima_get_secureboot(void)
>> +{
>> +	return get_powerpc_secureboot();
>> +}
>> +
>> +/*
>> + * File signature verification is not needed, include only measurements
>> + */
>> +static const char *const default_arch_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK",
>> +	"measure func=MODULE_CHECK",
>> +	NULL
>> +};
> The rules above seem fairly self explanatory.
>
>> +
>> +/* Both file signature verification and measurements are needed */
>> +static const char *const sb_arch_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> +#if IS_ENABLED(CONFIG_MODULE_SIG)
>> +	"measure func=MODULE_CHECK",
>> +#else
>> +	"measure func=MODULE_CHECK template=ima-modsig",
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> +#endif
> But these ones are not so obvious, at least to me who knows very little
> about IMA.
>
> Can you add a one line comment to each of the ones in here saying what
> it does and why we want it?

Sure.

>
>> +	NULL
>> +};
>> +
>> +/*
>> + * On PowerPC, file measurements are to be added to the IMA measurement list
>> + * irrespective of the secure boot state of the system.
> Why? Just because we think it's useful? Would be good to provide some
> further justification.

Sure. I will clarify this in the next version.

Thanks & Regards,
         - Nayna

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-09-05 12:35 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-19 12:35 [PATCH v5 0/2] powerpc: Enabling IMA arch specific secure boot policies Nayna Jain
2019-08-19 12:35 ` Nayna Jain
2019-08-19 12:35 ` [PATCH v5 1/2] powerpc: detect the secure boot mode of the system Nayna Jain
2019-08-19 12:35   ` Nayna Jain
2019-09-02 11:52   ` Michael Ellerman
2019-09-02 11:52     ` Michael Ellerman
2019-09-05 11:32     ` Nayna
2019-08-19 12:35 ` [PATCH v5 2/2] powerpc: Add support to initialize ima policy rules Nayna Jain
2019-08-19 12:35   ` Nayna Jain
2019-09-02 11:52   ` Michael Ellerman
2019-09-02 11:52     ` Michael Ellerman
2019-09-05 12:31     ` Nayna
2019-09-05 12:31       ` Nayna

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.