[PATCH v7 0/8] powerpc: Enabling IMA arch specific secure boot policies

[PATCH v7 0/8] powerpc: Enabling IMA arch specific secure boot policies

[Nayna Jain]

This patchset extends the previous version of the patchset[1] by adding
the support for checking against the binary blacklisted hashes.

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.

PowerNV systems uses the linux based bootloader and kexec the Host OS.
It rely on IMA for signature verification of the kernel before doing the
kexec. This patchset adds support for powerpc arch specific ima policies
that are defined based on system's OS secureboot and trustedboot state.
The OS secureboot and trustedboot state are determined via device-tree
properties.

The verification needs to be done only for the binaries which are not
blacklisted. The kernel currently checks against the blacklisted keys.
However that results in blacklisting all the binaries that are signed by
that key. In order to prevent single binary from loading, it is required
to support checking against blacklisting of the binary hash. This patchset
adds the support in IMA to check against blacklisted hashes for the files
signed by appended signature.

[1] http://patchwork.ozlabs.org/cover/1149262/ 

Changelog:
v7:
* Removes patch related to dt-bindings as per input from Rob Herring. 
* fixes Patch 1/8 to use new device-tree updates as per Oliver
  feedback to device-tree documentation in skiboot mailing list.
(https://lists.ozlabs.org/pipermail/skiboot/2019-September/015329.html)
* Includes feedbacks from Mimi, Thiago
  * moves function get_powerpc_fw_sb_node() from Patch 1 to Patch 3 
  * fixes Patch 2/8 to use CONFIG_MODULE_SIG_FORCE.
  * updates Patch description in Patch 5/8
  * adds a new patch to add wrapper is_binary_blacklisted()
  * removes the patch that deprecated permit_directio

v6:
* includes feedbacks from Michael Ellerman on the patchset v5
  * removed email ids from comments
  * add the doc for the device-tree
  * renames the secboot.c to secure_boot.c and secboot.h to secure_boot.h
  * other code specific fixes
* split the patches to differentiate between secureboot and trustedboot
state of the system
* adds the patches to support the blacklisting of the binary hash.

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
*** BLURB HERE ***

Nayna Jain (8):
  powerpc: detect the secure boot mode of the system
  powerpc: add support to initialize ima policy rules
  powerpc: detect the trusted boot state of the system
  powerpc/ima: add measurement rules to ima arch specific policy
  ima: make process_buffer_measurement() generic
  certs: add wrapper function to check blacklisted binary hash
  ima: check against blacklisted hashes for files with modsig
  powerpc/ima: update ima arch policy to check for blacklist

 Documentation/ABI/testing/ima_policy   |  1 +
 arch/powerpc/Kconfig                   | 12 ++++
 arch/powerpc/include/asm/secure_boot.h | 35 ++++++++++++
 arch/powerpc/kernel/Makefile           |  2 +
 arch/powerpc/kernel/ima_arch.c         | 72 ++++++++++++++++++++++++
 arch/powerpc/kernel/secure_boot.c      | 77 ++++++++++++++++++++++++++
 certs/blacklist.c                      |  9 +++
 include/keys/system_keyring.h          |  6 ++
 include/linux/ima.h                    |  3 +-
 security/integrity/ima/ima.h           | 12 ++++
 security/integrity/ima/ima_appraise.c  | 39 +++++++++++++
 security/integrity/ima/ima_main.c      | 41 +++++++-------
 security/integrity/ima/ima_policy.c    | 10 +++-
 security/integrity/integrity.h         |  1 +
 14 files changed, 298 insertions(+), 22 deletions(-)
 create mode 100644 arch/powerpc/include/asm/secure_boot.h
 create mode 100644 arch/powerpc/kernel/ima_arch.c
 create mode 100644 arch/powerpc/kernel/secure_boot.c

-- 
2.20.1

[PATCH v7 1/8] powerpc: detect the secure boot mode of the system

[Nayna Jain]

Secure boot on PowerNV 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                   | 10 ++++++
 arch/powerpc/include/asm/secure_boot.h | 29 ++++++++++++++++++
 arch/powerpc/kernel/Makefile           |  2 ++
 arch/powerpc/kernel/secure_boot.c      | 42 ++++++++++++++++++++++++++
 4 files changed, 83 insertions(+)
 create mode 100644 arch/powerpc/include/asm/secure_boot.h
 create mode 100644 arch/powerpc/kernel/secure_boot.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 3e56c9c2f16e..b4a221886fcf 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -934,6 +934,16 @@ config PPC_MEM_KEYS
 
 	  If unsure, say y.
 
+config PPC_SECURE_BOOT
+	prompt "Enable secure boot support"
+	bool
+	depends on PPC_POWERNV
+	help
+	  Systems 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 systems that have firmware support for
+	  it. If in doubt say N.
+
 endmenu
 
 config ISA_DMA_API
diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
new file mode 100644
index 000000000000..23d2ef2f1f7b
--- /dev/null
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Secure boot definitions
+ *
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+#ifndef _ASM_POWER_SECURE_BOOT_H
+#define _ASM_POWER_SECURE_BOOT_H
+
+#ifdef CONFIG_PPC_SECURE_BOOT
+
+bool is_powerpc_os_secureboot_enabled(void);
+struct device_node *get_powerpc_os_sb_node(void);
+
+#else
+
+static inline bool is_powerpc_os_secureboot_enabled(void)
+{
+	return false;
+}
+
+static inline struct device_node *get_powerpc_os_sb_node(void)
+{
+	return NULL;
+}
+
+#endif
+#endif
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index a7ca8fe62368..e2a54fa240ac 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -161,6 +161,8 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),)
 obj-y				+= ucall.o
 endif
 
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.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/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
new file mode 100644
index 000000000000..0488dbcab6b9
--- /dev/null
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -0,0 +1,42 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+#include <linux/types.h>
+#include <linux/of.h>
+#include <asm/secure_boot.h>
+
+struct device_node *get_powerpc_os_sb_node(void)
+{
+	return of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
+}
+
+bool is_powerpc_os_secureboot_enabled(void)
+{
+	struct device_node *node;
+
+	node = get_powerpc_os_sb_node();
+	if (!node)
+		goto disabled;
+
+	if (!of_device_is_available(node)) {
+		pr_err("Secure variables support is in error state, fail secure\n");
+		goto enabled;
+	}
+
+	/*
+	 * secureboot is enabled if os-secure-enforcing property exists,
+	 * else disabled.
+	 */
+	if (!of_find_property(node, "os-secure-enforcing", NULL))
+		goto disabled;
+
+enabled:
+	pr_info("secureboot mode enabled\n");
+	return true;
+
+disabled:
+	pr_info("secureboot mode disabled\n");
+	return false;
+}
-- 
2.20.1

[PATCH v7 2/8] powerpc: add support to initialize ima policy rules

[Nayna Jain]

PowerNV systems uses kernel based bootloader, thus its secure boot
implementation uses kernel IMA security subsystem to verify the kernel
before kexec. Since the verification policy might differ based on the
secure boot mode of the system, the policies are defined at runtime.

This patch implements the arch-specific support to define the IMA policy
rules based on the runtime secure boot mode of the system.

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 | 33 +++++++++++++++++++++++++++++++++
 include/linux/ima.h            |  3 ++-
 4 files changed, 38 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/kernel/ima_arch.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index b4a221886fcf..deb19ec6ba3d 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -938,6 +938,8 @@ config PPC_SECURE_BOOT
 	prompt "Enable secure boot support"
 	bool
 	depends on PPC_POWERNV
+	depends on IMA
+	depends on IMA_ARCH_POLICY
 	help
 	  Systems with firmware secure boot enabled needs to define security
 	  policies to extend secure boot to the OS. This config allows user
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index e2a54fa240ac..e8eb2955b7d5 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -161,7 +161,7 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),)
 obj-y				+= ucall.o
 endif
 
-obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.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..c22d82965eb4
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 IBM Corporation
+ * Author: Nayna Jain
+ */
+
+#include <linux/ima.h>
+#include <asm/secure_boot.h>
+
+bool arch_ima_get_secureboot(void)
+{
+	return is_powerpc_os_secureboot_enabled();
+}
+
+/* Defines IMA appraise rules for secureboot */
+static const char *const arch_rules[] = {
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
+/*
+ * Returns the relevant IMA arch policies based on the system secureboot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+	if (is_powerpc_os_secureboot_enabled())
+		return arch_rules;
+
+	return NULL;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 1c37f17f7203..6d904754d858 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

[PATCH v7 3/8] powerpc: detect the trusted boot state of the system

[Nayna Jain]

PowerNV systems enables the IMA measurement rules only if the
trusted boot is enabled on the system.

This patch adds the function to detect if the system has trusted
boot enabled.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/include/asm/secure_boot.h |  6 +++++
 arch/powerpc/kernel/secure_boot.c      | 35 ++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
index 23d2ef2f1f7b..ecd08515e301 100644
--- a/arch/powerpc/include/asm/secure_boot.h
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -12,6 +12,7 @@
 
 bool is_powerpc_os_secureboot_enabled(void);
 struct device_node *get_powerpc_os_sb_node(void);
+bool is_powerpc_trustedboot_enabled(void);
 
 #else
 
@@ -25,5 +26,10 @@ static inline struct device_node *get_powerpc_os_sb_node(void)
 	return NULL;
 }
 
+static inline bool is_powerpc_os_trustedboot_enabled(void)
+{
+	return false;
+}
+
 #endif
 #endif
diff --git a/arch/powerpc/kernel/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
index 0488dbcab6b9..9d5ac1b39e46 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -7,6 +7,27 @@
 #include <linux/of.h>
 #include <asm/secure_boot.h>
 
+static const char * const fwsecureboot_compat[] = {
+	"ibm,secureboot-v1",
+	"ibm,secureboot-v2",
+	NULL,
+};
+
+static struct device_node *get_powerpc_fw_sb_node(void)
+{
+	struct device_node *node;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(fwsecureboot_compat); ++i) {
+		node = of_find_compatible_node(NULL, NULL,
+					       fwsecureboot_compat[i]);
+		if (node)
+			return node;
+	}
+
+	return NULL;
+}
+
 struct device_node *get_powerpc_os_sb_node(void)
 {
 	return of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
@@ -40,3 +61,17 @@ bool is_powerpc_os_secureboot_enabled(void)
 	pr_info("secureboot mode disabled\n");
 	return false;
 }
+
+bool is_powerpc_trustedboot_enabled(void)
+{
+	struct device_node *node;
+
+	node = get_powerpc_fw_sb_node();
+	if (node && (of_find_property(node, "trusted-enabled", NULL))) {
+		pr_info("trustedboot mode enabled\n");
+		return true;
+	}
+
+	pr_info("trustedboot mode disabled\n");
+	return false;
+}
-- 
2.20.1

[PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy

[Nayna Jain]

This patch adds the measurement rules to the arch specific policies on
trusted boot enabled systems.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
---
 arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++---
 1 file changed, 42 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index c22d82965eb4..88bfe4a1a9a5 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void)
 	return is_powerpc_os_secureboot_enabled();
 }
 
-/* Defines IMA appraise rules for secureboot */
+/*
+ * The "arch_rules" contains both the securebot and trustedboot rules for adding
+ * the kexec kernel image and kernel modules file hashes to the IMA measurement
+ * list and verifying the file signatures against known good values.
+ *
+ * The "appraise_type=imasig|modsig" option allows the good signature to be
+ * stored as an xattr or as an appended signature. The "template=ima-modsig"
+ * option includes the appended signature, when available, in the IMA
+ * measurement list.
+ */
 static const char *const arch_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
+	"measure func=MODULE_CHECK template=ima-modsig",
 	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
 #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
 	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
@@ -22,12 +33,40 @@ static const char *const arch_rules[] = {
 };
 
 /*
- * Returns the relevant IMA arch policies based on the system secureboot state.
+ * The "measure_rules" are enabled only on "trustedboot" enabled systems.
+ * These rules add the kexec kernel image and kernel modules file hashes to
+ * the IMA measurement list.
+ */
+static const char *const measure_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK",
+	"measure func=MODULE_CHECK",
+	NULL
+};
+
+/*
+ * Returns the relevant IMA arch policies based on the system secureboot
+ * and trustedboot state.
  */
 const char *const *arch_get_ima_policy(void)
 {
-	if (is_powerpc_os_secureboot_enabled())
+	const char *const *rules;
+	int offset = 0;
+
+	for (rules = arch_rules; *rules != NULL; rules++) {
+		if (strncmp(*rules, "appraise", 8) == 0)
+			break;
+		offset++;
+	}
+
+	if (is_powerpc_os_secureboot_enabled()
+	    && is_powerpc_trustedboot_enabled())
 		return arch_rules;
 
+	if (is_powerpc_os_secureboot_enabled())
+		return arch_rules + offset;
+
+	if (is_powerpc_trustedboot_enabled())
+		return measure_rules;
+
 	return NULL;
 }
-- 
2.20.1

[PATCH v7 5/8] ima: make process_buffer_measurement() generic

[Nayna Jain]

An additional measurement record is needed to indicate the blacklisted
binary. The record will measure the blacklisted binary hash.

This patch makes the function process_buffer_measurement() generic to be
called by the blacklisting function. It modifies the function to handle
more than just the KEXEC_CMDLINE.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 security/integrity/ima/ima.h      |  3 +++
 security/integrity/ima/ima_main.c | 29 ++++++++++++++---------------
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3689081aaf38..ed86c1f70d7f 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -217,6 +217,9 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
 			   struct evm_ima_xattr_data *xattr_value,
 			   int xattr_len, const struct modsig *modsig, int pcr,
 			   struct ima_template_desc *template_desc);
+void process_buffer_measurement(const void *buf, int size,
+				const char *eventname, int pcr,
+				struct ima_template_desc *template_desc);
 void ima_audit_measurement(struct integrity_iint_cache *iint,
 			   const unsigned char *filename);
 int ima_alloc_init_template(struct ima_event_data *event_data,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 60027c643ecd..77115e884496 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id)
  * @buf: pointer to the buffer that needs to be added to the log.
  * @size: size of buffer(in bytes).
  * @eventname: event name to be used for the buffer entry.
- * @cred: a pointer to a credentials structure for user validation.
- * @secid: the secid of the task to be validated.
+ * @pcr: pcr to extend the measurement
+ * @template_desc: template description
  *
  * Based on policy, the buffer is measured into the ima log.
  */
-static void process_buffer_measurement(const void *buf, int size,
-				       const char *eventname,
-				       const struct cred *cred, u32 secid)
+void process_buffer_measurement(const void *buf, int size,
+				const char *eventname, int pcr,
+				struct ima_template_desc *template_desc)
 {
 	int ret = 0;
 	struct ima_template_entry *entry = NULL;
@@ -642,19 +642,11 @@ static void process_buffer_measurement(const void *buf, int size,
 					    .filename = eventname,
 					    .buf = buf,
 					    .buf_len = size};
-	struct ima_template_desc *template_desc = NULL;
 	struct {
 		struct ima_digest_data hdr;
 		char digest[IMA_MAX_DIGEST_SIZE];
 	} hash = {};
 	int violation = 0;
-	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
-	int action = 0;
-
-	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
-				&template_desc);
-	if (!(action & IMA_MEASURE))
-		return;
 
 	iint.ima_hash = &hash.hdr;
 	iint.ima_hash->algo = ima_hash_algo;
@@ -686,12 +678,19 @@ static void process_buffer_measurement(const void *buf, int size,
  */
 void ima_kexec_cmdline(const void *buf, int size)
 {
+	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+	struct ima_template_desc *template_desc = NULL;
+	int action;
 	u32 secid;
 
 	if (buf && size != 0) {
 		security_task_getsecid(current, &secid);
-		process_buffer_measurement(buf, size, "kexec-cmdline",
-					   current_cred(), secid);
+		action = ima_get_action(NULL, current_cred(), secid, 0,
+					KEXEC_CMDLINE, &pcr, &template_desc);
+		if (!(action & IMA_MEASURE))
+			return;
+		process_buffer_measurement(buf, size, "kexec-cmdline", pcr,
+					   template_desc);
 	}
 }
 
-- 
2.20.1

[PATCH v7 6/8] certs: add wrapper function to check blacklisted binary hash

[Nayna Jain]

The existing is_hash_blacklisted() function returns -EKEYREJECTED
error code for both the blacklisted keys and binaries.

This patch adds a wrapper function is_binary_blacklisted() to check
against binary hashes and returns -EPERM.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 certs/blacklist.c             | 9 +++++++++
 include/keys/system_keyring.h | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/certs/blacklist.c b/certs/blacklist.c
index ec00bf337eb6..6514f9ebc943 100644
--- a/certs/blacklist.c
+++ b/certs/blacklist.c
@@ -135,6 +135,15 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
 }
 EXPORT_SYMBOL_GPL(is_hash_blacklisted);
 
+int is_binary_blacklisted(const u8 *hash, size_t hash_len)
+{
+	if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
+		return -EPERM;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(is_binary_blacklisted);
+
 /*
  * Initialise the blacklist
  */
diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
index c1a96fdf598b..fb8b07daa9d1 100644
--- a/include/keys/system_keyring.h
+++ b/include/keys/system_keyring.h
@@ -35,12 +35,18 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
 extern int mark_hash_blacklisted(const char *hash);
 extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 			       const char *type);
+extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
 #else
 static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
 				      const char *type)
 {
 	return 0;
 }
+
+static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
+{
+	return 0;
+}
 #endif
 
 #ifdef CONFIG_IMA_BLACKLIST_KEYRING
-- 
2.20.1

[PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig

[Nayna Jain]

Asymmetric private keys are used to sign multiple files. The kernel
currently support checking against the blacklisted keys. However, if the
public key is blacklisted, any file signed by the blacklisted key will
automatically fail signature verification. We might not want to blacklist
all the files signed by a particular key, but just a single file.
Blacklisting the public key is not fine enough granularity.

This patch adds support for blacklisting binaries with appended signatures,
based on the IMA policy.  Defined is a new policy option
"appraise_flag=check_blacklist".

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 Documentation/ABI/testing/ima_policy  |  1 +
 security/integrity/ima/ima.h          |  9 +++++++
 security/integrity/ima/ima_appraise.c | 39 +++++++++++++++++++++++++++
 security/integrity/ima/ima_main.c     | 12 ++++++---
 security/integrity/ima/ima_policy.c   | 10 +++++--
 security/integrity/integrity.h        |  1 +
 6 files changed, 66 insertions(+), 6 deletions(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29ebe9afdac4..4c97afcc0f3c 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -25,6 +25,7 @@ Description:
 			lsm:	[[subj_user=] [subj_role=] [subj_type=]
 				 [obj_user=] [obj_role=] [obj_type=]]
 			option:	[[appraise_type=]] [template=] [permit_directio]
+				[appraise_flag=[check_blacklist]]
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ed86c1f70d7f..63e20ccc91ce 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v);
 #define IMA_APPRAISE_KEXEC	0x40
 
 #ifdef CONFIG_IMA_APPRAISE
+int ima_check_blacklist(struct integrity_iint_cache *iint,
+			const struct modsig *modsig, int action, int pcr);
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
 			     struct file *file, const unsigned char *filename,
@@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
 
 #else
+static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
+				      const struct modsig *modsig, int action,
+				      int pcr)
+{
+	return 0;
+}
+
 static inline int ima_appraise_measurement(enum ima_hooks func,
 					   struct integrity_iint_cache *iint,
 					   struct file *file,
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 136ae4e0ee92..fe34d64a684c 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -12,6 +12,7 @@
 #include <linux/magic.h>
 #include <linux/ima.h>
 #include <linux/evm.h>
+#include <keys/system_keyring.h>
 
 #include "ima.h"
 
@@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
 	return rc;
 }
 
+/*
+ * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
+ * yes, then adds the hash of the blacklisted binary to the measurement list.
+ *
+ * Returns -EPERM if the hash is blacklisted.
+ */
+int ima_check_blacklist(struct integrity_iint_cache *iint,
+			const struct modsig *modsig, int action, int pcr)
+{
+	enum hash_algo hash_algo;
+	const u8 *digest = NULL;
+	u32 digestsize = 0;
+	u32 secid;
+	int rc = 0;
+	struct ima_template_desc *template_desc;
+
+	template_desc = lookup_template_desc("ima-buf");
+	template_desc_init_fields(template_desc->fmt, &(template_desc->fields),
+				  &(template_desc->num_fields));
+
+	if (!(iint->flags & IMA_CHECK_BLACKLIST))
+		return 0;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED) {
+		security_task_getsecid(current, &secid);
+		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
+		rc = is_binary_blacklisted(digest, digestsize);
+
+		/* Returns -EPERM on blacklisted hash found */
+		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
+			process_buffer_measurement(digest, digestsize,
+						   "blacklisted-hash", pcr,
+						   template_desc);
+	}
+
+	return rc;
+}
+
 /*
  * ima_appraise_measurement - appraise file measurement
  *
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 77115e884496..40d30ab17cbe 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -335,10 +335,14 @@ static int process_measurement(struct file *file, const struct cred *cred,
 				      xattr_value, xattr_len, modsig, pcr,
 				      template_desc);
 	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
-		inode_lock(inode);
-		rc = ima_appraise_measurement(func, iint, file, pathname,
-					      xattr_value, xattr_len, modsig);
-		inode_unlock(inode);
+		rc = ima_check_blacklist(iint, modsig, action, pcr);
+		if (rc != -EPERM) {
+			inode_lock(inode);
+			rc = ima_appraise_measurement(func, iint, file,
+						      pathname, xattr_value,
+						      xattr_len, modsig);
+			inode_unlock(inode);
+		}
 		if (!rc)
 			rc = mmap_violation_check(func, file, &pathbuf,
 						  &pathname, filename);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 5380aca2b351..bfaae7a8443a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -765,8 +765,8 @@ enum {
 	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
 	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
 	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
-	Opt_appraise_type, Opt_permit_directio,
-	Opt_pcr, Opt_template, Opt_err
+	Opt_appraise_type, Opt_appraise_flag,
+	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
 };
 
 static const match_table_t policy_tokens = {
@@ -798,6 +798,7 @@ static const match_table_t policy_tokens = {
 	{Opt_euid_lt, "euid<%s"},
 	{Opt_fowner_lt, "fowner<%s"},
 	{Opt_appraise_type, "appraise_type=%s"},
+	{Opt_appraise_flag, "appraise_flag=%s"},
 	{Opt_permit_directio, "permit_directio"},
 	{Opt_pcr, "pcr=%s"},
 	{Opt_template, "template=%s"},
@@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else
 				result = -EINVAL;
 			break;
+		case Opt_appraise_flag:
+			ima_log_string(ab, "appraise_flag", args[0].from);
+			if (strstr(args[0].from, "blacklist"))
+				entry->flags |= IMA_CHECK_BLACKLIST;
+			break;
 		case Opt_permit_directio:
 			entry->flags |= IMA_PERMIT_DIRECTIO;
 			break;
diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
index d9323d31a3a8..73fc286834d7 100644
--- a/security/integrity/integrity.h
+++ b/security/integrity/integrity.h
@@ -32,6 +32,7 @@
 #define EVM_IMMUTABLE_DIGSIG	0x08000000
 #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
 #define IMA_MODSIG_ALLOWED	0x20000000
+#define IMA_CHECK_BLACKLIST	0x40000000
 
 #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
 				 IMA_HASH | IMA_APPRAISE_SUBMASK)
-- 
2.20.1

[PATCH v7 8/8] powerpc/ima: update ima arch policy to check for blacklist

[Nayna Jain]

This patch updates the arch specific policies for PowernV systems
to add check against blacklisted binary hashes before doing the
verification.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/kernel/ima_arch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index 88bfe4a1a9a5..4fa41537b846 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -25,9 +25,9 @@ bool arch_ima_get_secureboot(void)
 static const char *const arch_rules[] = {
 	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
 	"measure func=MODULE_CHECK template=ima-modsig",
-	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
 #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
-	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
 #endif
 	NULL
 };
-- 
2.20.1

Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules

[Mimi Zohar]

On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> PowerNV systems uses kernel based bootloader, thus its secure boot
> implementation uses kernel IMA security subsystem to verify the kernel
> before kexec. 

^use a Linux based bootloader, which rely on the IMA subsystem to
enforce different secure boot modes.

> Since the verification policy might differ based on the
> secure boot mode of the system, the policies are defined at runtime.

^the policies need to be defined at runtime.
> 
> This patch implements the arch-specific support to define the IMA policy
> rules based on the runtime secure boot mode of the system.
> 
> 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 | 33 +++++++++++++++++++++++++++++++++
>  include/linux/ima.h            |  3 ++-
>  4 files changed, 38 insertions(+), 2 deletions(-)
>  create mode 100644 arch/powerpc/kernel/ima_arch.c
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index b4a221886fcf..deb19ec6ba3d 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -938,6 +938,8 @@ config PPC_SECURE_BOOT
>  	prompt "Enable secure boot support"
>  	bool
>  	depends on PPC_POWERNV
> +	depends on IMA
> +	depends on IMA_ARCH_POLICY

As IMA_ARCH_POLICY is dependent on IMA, I don't see a need for
depending on both IMA and IMA_ARCH_POLICY.

Mimi

Re: [PATCH v7 5/8] ima: make process_buffer_measurement() generic

[Mimi Zohar]

[Cc'ing Prakhar Srivastava]

On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> An additional measurement record is needed to indicate the blacklisted
> binary. The record will measure the blacklisted binary hash.
> 
> This patch makes the function process_buffer_measurement() generic to be
> called by the blacklisting function. It modifies the function to handle
> more than just the KEXEC_CMDLINE.

The purpose of this patch is to make process_buffer_measurement() more
generic.  The patch description should simply say,
process_buffer_measurement() is limited to measuring the kexec boot
command line.  This patch makes process_buffer_measurement() more
generic, allowing it to measure other types of buffer data (eg.
blacklisted binary hashes).

Mimi

> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  security/integrity/ima/ima.h      |  3 +++
>  security/integrity/ima/ima_main.c | 29 ++++++++++++++---------------
>  2 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 3689081aaf38..ed86c1f70d7f 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -217,6 +217,9 @@ void ima_store_measurement(struct integrity_iint_cache *iint, struct file *file,
>  			   struct evm_ima_xattr_data *xattr_value,
>  			   int xattr_len, const struct modsig *modsig, int pcr,
>  			   struct ima_template_desc *template_desc);
> +void process_buffer_measurement(const void *buf, int size,
> +				const char *eventname, int pcr,
> +				struct ima_template_desc *template_desc);
>  void ima_audit_measurement(struct integrity_iint_cache *iint,
>  			   const unsigned char *filename);
>  int ima_alloc_init_template(struct ima_event_data *event_data,
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 60027c643ecd..77115e884496 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -626,14 +626,14 @@ int ima_load_data(enum kernel_load_data_id id)
>   * @buf: pointer to the buffer that needs to be added to the log.
>   * @size: size of buffer(in bytes).
>   * @eventname: event name to be used for the buffer entry.
> - * @cred: a pointer to a credentials structure for user validation.
> - * @secid: the secid of the task to be validated.
> + * @pcr: pcr to extend the measurement
> + * @template_desc: template description
>   *
>   * Based on policy, the buffer is measured into the ima log.
>   */
> -static void process_buffer_measurement(const void *buf, int size,
> -				       const char *eventname,
> -				       const struct cred *cred, u32 secid)
> +void process_buffer_measurement(const void *buf, int size,
> +				const char *eventname, int pcr,
> +				struct ima_template_desc *template_desc)
>  {
>  	int ret = 0;
>  	struct ima_template_entry *entry = NULL;
> @@ -642,19 +642,11 @@ static void process_buffer_measurement(const void *buf, int size,
>  					    .filename = eventname,
>  					    .buf = buf,
>  					    .buf_len = size};
> -	struct ima_template_desc *template_desc = NULL;
>  	struct {
>  		struct ima_digest_data hdr;
>  		char digest[IMA_MAX_DIGEST_SIZE];
>  	} hash = {};
>  	int violation = 0;
> -	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> -	int action = 0;
> -
> -	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
> -				&template_desc);
> -	if (!(action & IMA_MEASURE))
> -		return;
>  
>  	iint.ima_hash = &hash.hdr;
>  	iint.ima_hash->algo = ima_hash_algo;
> @@ -686,12 +678,19 @@ static void process_buffer_measurement(const void *buf, int size,
>   */
>  void ima_kexec_cmdline(const void *buf, int size)
>  {
> +	int pcr = CONFIG_IMA_MEASURE_PCR_IDX;
> +	struct ima_template_desc *template_desc = NULL;
> +	int action;
>  	u32 secid;
>  
>  	if (buf && size != 0) {
>  		security_task_getsecid(current, &secid);
> -		process_buffer_measurement(buf, size, "kexec-cmdline",
> -					   current_cred(), secid);
> +		action = ima_get_action(NULL, current_cred(), secid, 0,
> +					KEXEC_CMDLINE, &pcr, &template_desc);
> +		if (!(action & IMA_MEASURE))
> +			return;
> +		process_buffer_measurement(buf, size, "kexec-cmdline", pcr,
> +					   template_desc);
>  	}
>  }
>  

Re: [PATCH v7 6/8] certs: add wrapper function to check blacklisted binary hash

[Mimi Zohar]

On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> The existing is_hash_blacklisted() function returns -EKEYREJECTED
> error code for both the blacklisted keys and binaries.
> 
> This patch adds a wrapper function is_binary_blacklisted() to check
> against binary hashes and returns -EPERM.    
> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

This patch description describes what you're doing, not the
motivation.

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---
>  certs/blacklist.c             | 9 +++++++++
>  include/keys/system_keyring.h | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/certs/blacklist.c b/certs/blacklist.c
> index ec00bf337eb6..6514f9ebc943 100644
> --- a/certs/blacklist.c
> +++ b/certs/blacklist.c
> @@ -135,6 +135,15 @@ int is_hash_blacklisted(const u8 *hash, size_t hash_len, const char *type)
>  }
>  EXPORT_SYMBOL_GPL(is_hash_blacklisted);
>  
> +int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> +	if (is_hash_blacklisted(hash, hash_len, "bin") == -EKEYREJECTED)
> +		return -EPERM;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(is_binary_blacklisted);
> +
>  /*
>   * Initialise the blacklist
>   */
> diff --git a/include/keys/system_keyring.h b/include/keys/system_keyring.h
> index c1a96fdf598b..fb8b07daa9d1 100644
> --- a/include/keys/system_keyring.h
> +++ b/include/keys/system_keyring.h
> @@ -35,12 +35,18 @@ extern int restrict_link_by_builtin_and_secondary_trusted(
>  extern int mark_hash_blacklisted(const char *hash);
>  extern int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>  			       const char *type);
> +extern int is_binary_blacklisted(const u8 *hash, size_t hash_len);
>  #else
>  static inline int is_hash_blacklisted(const u8 *hash, size_t hash_len,
>  				      const char *type)
>  {
>  	return 0;
>  }
> +
> +static inline int is_binary_blacklisted(const u8 *hash, size_t hash_len)
> +{
> +	return 0;
> +}
>  #endif
>  
>  #ifdef CONFIG_IMA_BLACKLIST_KEYRING

Re: [PATCH v7 8/8] powerpc/ima: update ima arch policy to check for blacklist

[Mimi Zohar]

On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> This patch updates the arch specific policies for PowernV systems
> to add check against blacklisted binary hashes before doing the
> verification.

This sentence explains how you're doing something.  A simple tweak in
the wording provides the motivation.

^to make sure that the binary hash is not blacklisted.

> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>

Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>

> ---
>  arch/powerpc/kernel/ima_arch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index 88bfe4a1a9a5..4fa41537b846 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -25,9 +25,9 @@ bool arch_ima_get_secureboot(void)
>  static const char *const arch_rules[] = {
>  	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>  	"measure func=MODULE_CHECK template=ima-modsig",
> -	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>  #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> -	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
>  #endif
>  	NULL
>  };

Re: [PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig

[Mimi Zohar]

On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
> Asymmetric private keys are used to sign multiple files. The kernel
> currently support checking against the blacklisted keys. However, if the
> public key is blacklisted, any file signed by the blacklisted key will
> automatically fail signature verification. We might not want to blacklist
> all the files signed by a particular key, but just a single file.
> Blacklisting the public key is not fine enough granularity.
> 
> This patch adds support for blacklisting binaries with appended signatures,
> based on the IMA policy.  Defined is a new policy option
> "appraise_flag=check_blacklist".

The blacklisted hash is not the same as the file hash, but is the file
hash without the appended signature.  Are there tools for calculating
the blacklisted hash?  Can you provide an example?

> 
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  Documentation/ABI/testing/ima_policy  |  1 +
>  security/integrity/ima/ima.h          |  9 +++++++
>  security/integrity/ima/ima_appraise.c | 39 +++++++++++++++++++++++++++
>  security/integrity/ima/ima_main.c     | 12 ++++++---
>  security/integrity/ima/ima_policy.c   | 10 +++++--
>  security/integrity/integrity.h        |  1 +
>  6 files changed, 66 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
> index 29ebe9afdac4..4c97afcc0f3c 100644
> --- a/Documentation/ABI/testing/ima_policy
> +++ b/Documentation/ABI/testing/ima_policy
> @@ -25,6 +25,7 @@ Description:
>  			lsm:	[[subj_user=] [subj_role=] [subj_type=]
>  				 [obj_user=] [obj_role=] [obj_type=]]
>  			option:	[[appraise_type=]] [template=] [permit_directio]
> +				[appraise_flag=[check_blacklist]]
>  		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
>  				[FIRMWARE_CHECK]
>  				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index ed86c1f70d7f..63e20ccc91ce 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -256,6 +256,8 @@ int ima_policy_show(struct seq_file *m, void *v);
>  #define IMA_APPRAISE_KEXEC	0x40
>  
>  #ifdef CONFIG_IMA_APPRAISE
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> +			const struct modsig *modsig, int action, int pcr);
>  int ima_appraise_measurement(enum ima_hooks func,
>  			     struct integrity_iint_cache *iint,
>  			     struct file *file, const unsigned char *filename,
> @@ -271,6 +273,13 @@ int ima_read_xattr(struct dentry *dentry,
>  		   struct evm_ima_xattr_data **xattr_value);
>  
>  #else
> +static inline int ima_check_blacklist(struct integrity_iint_cache *iint,
> +				      const struct modsig *modsig, int action,
> +				      int pcr)
> +{
> +	return 0;
> +}
> +
>  static inline int ima_appraise_measurement(enum ima_hooks func,
>  					   struct integrity_iint_cache *iint,
>  					   struct file *file,
> diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
> index 136ae4e0ee92..fe34d64a684c 100644
> --- a/security/integrity/ima/ima_appraise.c
> +++ b/security/integrity/ima/ima_appraise.c
> @@ -12,6 +12,7 @@
>  #include <linux/magic.h>
>  #include <linux/ima.h>
>  #include <linux/evm.h>
> +#include <keys/system_keyring.h>
>  
>  #include "ima.h"
>  
> @@ -303,6 +304,44 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
>  	return rc;
>  }
>  
> +/*
> + * ima_blacklist_measurement - Checks whether the binary is blacklisted. If
> + * yes, then adds the hash of the blacklisted binary to the measurement list.
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> +			const struct modsig *modsig, int action, int pcr)
> +{
> +	enum hash_algo hash_algo;
> +	const u8 *digest = NULL;
> +	u32 digestsize = 0;
> +	u32 secid;
> +	int rc = 0;
> +	struct ima_template_desc *template_desc;
> +
> +	template_desc = lookup_template_desc("ima-buf");
> +	template_desc_init_fields(template_desc->fmt, &(template_desc->fields),
> +				  &(template_desc->num_fields));

Before using template_desc, make sure that template_desc isn't NULL.
 For completeness, check the return code of
template_desc_init_fields()

> +
> +	if (!(iint->flags & IMA_CHECK_BLACKLIST))
> +		return 0;

Move this check before getting the template_desc and make sure that
modsig isn't NULL.

> +
> +	if (iint->flags & IMA_MODSIG_ALLOWED) {
> +		security_task_getsecid(current, &secid);

secid isn't being used.

> +		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
> +		rc = is_binary_blacklisted(digest, digestsize);
> +
> +		/* Returns -EPERM on blacklisted hash found */

Now that it is returning a sane errno, the comment isn't needed.

Mimi

> +		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> +			process_buffer_measurement(digest, digestsize,
> +						   "blacklisted-hash", pcr,
> +						   template_desc);
> +	}
> +
> +	return rc;
> +}
> +
>  /*
>   * ima_appraise_measurement - appraise file measurement
>   *
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 77115e884496..40d30ab17cbe 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -335,10 +335,14 @@ static int process_measurement(struct file *file, const struct cred *cred,
>  				      xattr_value, xattr_len, modsig, pcr,
>  				      template_desc);
>  	if (rc == 0 && (action & IMA_APPRAISE_SUBMASK)) {
> -		inode_lock(inode);
> -		rc = ima_appraise_measurement(func, iint, file, pathname,
> -					      xattr_value, xattr_len, modsig);
> -		inode_unlock(inode);
> +		rc = ima_check_blacklist(iint, modsig, action, pcr);
> +		if (rc != -EPERM) {
> +			inode_lock(inode);
> +			rc = ima_appraise_measurement(func, iint, file,
> +						      pathname, xattr_value,
> +						      xattr_len, modsig);
> +			inode_unlock(inode);
> +		}
>  		if (!rc)
>  			rc = mmap_violation_check(func, file, &pathbuf,
>  						  &pathname, filename);
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 5380aca2b351..bfaae7a8443a 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -765,8 +765,8 @@ enum {
>  	Opt_fsuuid, Opt_uid_eq, Opt_euid_eq, Opt_fowner_eq,
>  	Opt_uid_gt, Opt_euid_gt, Opt_fowner_gt,
>  	Opt_uid_lt, Opt_euid_lt, Opt_fowner_lt,
> -	Opt_appraise_type, Opt_permit_directio,
> -	Opt_pcr, Opt_template, Opt_err
> +	Opt_appraise_type, Opt_appraise_flag,
> +	Opt_permit_directio, Opt_pcr, Opt_template, Opt_err
>  };
>  
>  static const match_table_t policy_tokens = {
> @@ -798,6 +798,7 @@ static const match_table_t policy_tokens = {
>  	{Opt_euid_lt, "euid<%s"},
>  	{Opt_fowner_lt, "fowner<%s"},
>  	{Opt_appraise_type, "appraise_type=%s"},
> +	{Opt_appraise_flag, "appraise_flag=%s"},
>  	{Opt_permit_directio, "permit_directio"},
>  	{Opt_pcr, "pcr=%s"},
>  	{Opt_template, "template=%s"},
> @@ -1172,6 +1173,11 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
>  			else
>  				result = -EINVAL;
>  			break;
> +		case Opt_appraise_flag:
> +			ima_log_string(ab, "appraise_flag", args[0].from);
> +			if (strstr(args[0].from, "blacklist"))
> +				entry->flags |= IMA_CHECK_BLACKLIST;
> +			break;
>  		case Opt_permit_directio:
>  			entry->flags |= IMA_PERMIT_DIRECTIO;
>  			break;
> diff --git a/security/integrity/integrity.h b/security/integrity/integrity.h
> index d9323d31a3a8..73fc286834d7 100644
> --- a/security/integrity/integrity.h
> +++ b/security/integrity/integrity.h
> @@ -32,6 +32,7 @@
>  #define EVM_IMMUTABLE_DIGSIG	0x08000000
>  #define IMA_FAIL_UNVERIFIABLE_SIGS	0x10000000
>  #define IMA_MODSIG_ALLOWED	0x20000000
> +#define IMA_CHECK_BLACKLIST	0x40000000
>  
>  #define IMA_DO_MASK		(IMA_MEASURE | IMA_APPRAISE | IMA_AUDIT | \
>  				 IMA_HASH | IMA_APPRAISE_SUBMASK)

Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules

[Michael Ellerman]

Mimi Zohar <zohar@linux.ibm.com> writes:
> On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
...
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index b4a221886fcf..deb19ec6ba3d 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -938,6 +938,8 @@ config PPC_SECURE_BOOT
>>  	prompt "Enable secure boot support"
>>  	bool
>>  	depends on PPC_POWERNV
>> +	depends on IMA
>> +	depends on IMA_ARCH_POLICY
>
> As IMA_ARCH_POLICY is dependent on IMA, I don't see a need for
> depending on both IMA and IMA_ARCH_POLICY.

I agree. And what we actually depend on is the arch part, so it should
depend on just IMA_ARCH_POLICY.

cheers

Re: [PATCH v7 3/8] powerpc: detect the trusted boot state of the system

[Michael Ellerman]

Nayna Jain <nayna@linux.ibm.com> writes:
> PowerNV systems enables the IMA measurement rules only if the
> trusted boot is enabled on the system.

That confused me a lot. But the key is the distinction between appraisal
rules vs measurement rules, right?

I think it would be clearer if it was phrased as a positive statement, eg:

  On PowerNV systems when trusted boot is enabled, additional IMA rules
  are enabled to implement measurement.

Or something like that.

> This patch adds the function to detect if the system has trusted
> boot enabled.

It would probably help people to briefly explain the difference between
secure vs trusted boot.

> diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
> index 23d2ef2f1f7b..ecd08515e301 100644
> --- a/arch/powerpc/include/asm/secure_boot.h
> +++ b/arch/powerpc/include/asm/secure_boot.h
> @@ -12,6 +12,7 @@
>  
>  bool is_powerpc_os_secureboot_enabled(void);
>  struct device_node *get_powerpc_os_sb_node(void);
> +bool is_powerpc_trustedboot_enabled(void);
>  
>  #else
>  
> @@ -25,5 +26,10 @@ static inline struct device_node *get_powerpc_os_sb_node(void)
>  	return NULL;
>  }
>  
> +static inline bool is_powerpc_os_trustedboot_enabled(void)

That has an extra "_os" in it.

> +{
> +	return false;
> +}
> +
>  #endif
>  #endif
> diff --git a/arch/powerpc/kernel/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
> index 0488dbcab6b9..9d5ac1b39e46 100644
> --- a/arch/powerpc/kernel/secure_boot.c
> +++ b/arch/powerpc/kernel/secure_boot.c
> @@ -7,6 +7,27 @@
>  #include <linux/of.h>
>  #include <asm/secure_boot.h>
>  
> +static const char * const fwsecureboot_compat[] = {
> +	"ibm,secureboot-v1",
> +	"ibm,secureboot-v2",
> +	NULL,
> +};
> +
> +static struct device_node *get_powerpc_fw_sb_node(void)
> +{
> +	struct device_node *node;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(fwsecureboot_compat); ++i) {
> +		node = of_find_compatible_node(NULL, NULL,
> +					       fwsecureboot_compat[i]);
> +		if (node)
> +			return node;
> +	}
> +
> +	return NULL;
> +}

You shouldn't need to do that by hand, instead use
of_find_matching_node(), eg:

static struct device_node *get_powerpc_fw_sb_node(void)
{
	static const struct of_device_id ids[] = {
		{ .compatible = "ibm,secureboot-v1", },
		{ .compatible = "ibm,secureboot-v2", },
		{},
	};

	return of_find_matching_node(NULL, ids);
}


> @@ -40,3 +61,17 @@ bool is_powerpc_os_secureboot_enabled(void)
>  	pr_info("secureboot mode disabled\n");
>  	return false;
>  }
> +
> +bool is_powerpc_trustedboot_enabled(void)
> +{
> +	struct device_node *node;
> +
> +	node = get_powerpc_fw_sb_node();
> +	if (node && (of_find_property(node, "trusted-enabled", NULL))) {

Again this can use of_property_read_bool(), which copes with a NULL node
also, so just:

+	if (of_property_read_bool(node, "trusted-enabled"))) {

> +		pr_info("trustedboot mode enabled\n");
> +		return true;
> +	}
> +
> +	pr_info("trustedboot mode disabled\n");
> +	return false;
> +}
> -- 
> 2.20.1


cheers

Re: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy

[Michael Ellerman]

Nayna Jain <nayna@linux.ibm.com> writes:
> This patch adds the measurement rules to the arch specific policies on
> trusted boot enabled systems.
>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
> ---
>  arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++---
>  1 file changed, 42 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> index c22d82965eb4..88bfe4a1a9a5 100644
> --- a/arch/powerpc/kernel/ima_arch.c
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void)
>  	return is_powerpc_os_secureboot_enabled();
>  }
>  
> -/* Defines IMA appraise rules for secureboot */
> +/*
> + * The "arch_rules" contains both the securebot and trustedboot rules for adding
> + * the kexec kernel image and kernel modules file hashes to the IMA measurement
> + * list and verifying the file signatures against known good values.
> + *
> + * The "appraise_type=imasig|modsig" option allows the good signature to be
> + * stored as an xattr or as an appended signature. The "template=ima-modsig"
> + * option includes the appended signature, when available, in the IMA
> + * measurement list.
> + */
>  static const char *const arch_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
> +	"measure func=MODULE_CHECK template=ima-modsig",
>  	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>  #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
>  	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> @@ -22,12 +33,40 @@ static const char *const arch_rules[] = {
>  };
>  
>  /*
> - * Returns the relevant IMA arch policies based on the system secureboot state.
> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
> + * These rules add the kexec kernel image and kernel modules file hashes to
> + * the IMA measurement list.
> + */
> +static const char *const measure_rules[] = {
> +	"measure func=KEXEC_KERNEL_CHECK",
> +	"measure func=MODULE_CHECK",

Why do these ones not have "template=ima-modsig" on the end?

> +	NULL
> +};
> +
> +/*
> + * Returns the relevant IMA arch policies based on the system secureboot
> + * and trustedboot state.
>   */
>  const char *const *arch_get_ima_policy(void)
>  {
> -	if (is_powerpc_os_secureboot_enabled())
> +	const char *const *rules;
> +	int offset = 0;
> +
> +	for (rules = arch_rules; *rules != NULL; rules++) {
> +		if (strncmp(*rules, "appraise", 8) == 0)
> +			break;
> +		offset++;
> +	}

This seems like kind of a hack, doesn't it? :)

What we really want is three sets of rules isn't it? But some of them
are shared between the different sets. But they just have to be flat
arrays of strings.

I think it would probably be cleaner to just use a #define for the
shared part of the rules, eg something like:

#ifdef CONFIG_MODULE_SIG_FORCE
#define APPRAISE_MODULE
#else
#define APPRAISE_MODULE \
	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
#endif

#define APPRAISE_KERNEL \
	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig"

#define MEASURE_KERNEL \
	"measure func=KEXEC_KERNEL_CHECK"

#define MEASURE_MODULE \
	"measure func=MODULE_CHECK"

#define APPEND_TEMPLATE_IMA_MODSIG		\
	" template=ima-modsig"

static const char *const secure_and_trusted_rules[] = {
	MEASURE_KERNEL APPEND_TEMPLATE_IMA_MODSIG,
	MEASURE_MODULE APPEND_TEMPLATE_IMA_MODSIG,
	APPRAISE_KERNEL,
	APPRAISE_MODULE
	NULL
};

static const char *const secure_rules[] = {
	APPRAISE_KERNEL,
	APPRAISE_MODULE
	NULL
};

static const char *const trusted_rules[] = {
	MEASURE_KERNEL,
	MEASURE_MODULE,
	NULL
};


> +
> +	if (is_powerpc_os_secureboot_enabled()
> +	    && is_powerpc_trustedboot_enabled())
>  		return arch_rules;
>  
> +	if (is_powerpc_os_secureboot_enabled())
> +		return arch_rules + offset;
> +
> +	if (is_powerpc_trustedboot_enabled())
> +		return measure_rules;

Those is_foo() routines print each time they're called don't they? So on
a system with only trusted boot I think that will print:

	secureboot mode disabled
	secureboot mode disabled
	trustedboot mode enabled

Which is a bit verbose :)

cheers

Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules

[Michael Ellerman]

Nayna Jain <nayna@linux.ibm.com> writes:
> PowerNV systems uses kernel based bootloader, thus its secure boot
> implementation uses kernel IMA security subsystem to verify the kernel
> before kexec. Since the verification policy might differ based on the
> secure boot mode of the system, the policies are defined at runtime.
>
> This patch implements the arch-specific support to define the IMA policy
> rules based on the runtime secure boot mode of the system.
>
> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
> config is enabled.
...
> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
> new file mode 100644
> index 000000000000..c22d82965eb4
> --- /dev/null
> +++ b/arch/powerpc/kernel/ima_arch.c
> @@ -0,0 +1,33 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +
> +#include <linux/ima.h>
> +#include <asm/secure_boot.h>
> +
> +bool arch_ima_get_secureboot(void)
> +{
> +	return is_powerpc_os_secureboot_enabled();
> +}
> +
> +/* Defines IMA appraise rules for secureboot */
> +static const char *const arch_rules[] = {
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif

This confuses me.

If I spell it out we get:

#if IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
	// nothing
#else
	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
#endif

Which is just:

#ifdef CONFIG_MODULE_SIG_FORCE
	// nothing
#else
	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
#endif

But CONFIG_MODULE_SIG_FORCE enabled says that we *do* require modules to
have a valid signature. Isn't that the inverse of what the rules say?

Presumably I'm misunderstanding something :)

cheers

Re: [PATCH v7 1/8] powerpc: detect the secure boot mode of the system

[Michael Ellerman]

Hi Nayna,

Just a few comments.

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

This description has got out of sync with what the patch does I think.
There's no IMA in here. I think you can just drop that sentence.

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

That's what the patch really does ^ - just make it clear that it's only
on powernv.

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

s/POWER/powerpc/.

>
> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
> ---
>  arch/powerpc/Kconfig                   | 10 ++++++
>  arch/powerpc/include/asm/secure_boot.h | 29 ++++++++++++++++++
>  arch/powerpc/kernel/Makefile           |  2 ++
>  arch/powerpc/kernel/secure_boot.c      | 42 ++++++++++++++++++++++++++
>  4 files changed, 83 insertions(+)
>  create mode 100644 arch/powerpc/include/asm/secure_boot.h
>  create mode 100644 arch/powerpc/kernel/secure_boot.c
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 3e56c9c2f16e..b4a221886fcf 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -934,6 +934,16 @@ config PPC_MEM_KEYS
>  
>  	  If unsure, say y.
>  
> +config PPC_SECURE_BOOT
> +	prompt "Enable secure boot support"
> +	bool
> +	depends on PPC_POWERNV
> +	help
> +	  Systems with firmware secure boot enabled needs to define security
                                                        ^
                                                     need
> +	  policies to extend secure boot to the OS. This config allows user
                                                                      ^
                                                                      a
> +	  to enable OS secure boot on systems that have firmware support for
> +	  it. If in doubt say N.
> +
>  endmenu
>  
>  config ISA_DMA_API
> diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
> new file mode 100644
> index 000000000000..23d2ef2f1f7b
> --- /dev/null
> +++ b/arch/powerpc/include/asm/secure_boot.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Secure boot definitions
> + *
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +#ifndef _ASM_POWER_SECURE_BOOT_H
> +#define _ASM_POWER_SECURE_BOOT_H
> +
> +#ifdef CONFIG_PPC_SECURE_BOOT
> +
> +bool is_powerpc_os_secureboot_enabled(void);
> +struct device_node *get_powerpc_os_sb_node(void);

This function is never used outside arch/powerpc/kernel/secure_boot.c
and so doesn't need to be public.

> +#else
> +
> +static inline bool is_powerpc_os_secureboot_enabled(void)
> +{

I know there's a distinction between firmware secureboot and OS
secureboot, but I don't think we need that baked into the name. So just
is_ppc_secureboot_enabled() would be fine.

> +	return false;
> +}
> +
> +static inline struct device_node *get_powerpc_os_sb_node(void)
> +{
> +	return NULL;
> +}
> +
> +#endif
> +#endif
> diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
> index a7ca8fe62368..e2a54fa240ac 100644
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -161,6 +161,8 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),)
>  obj-y				+= ucall.o
>  endif
>  
> +obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.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/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
> new file mode 100644
> index 000000000000..0488dbcab6b9
> --- /dev/null
> +++ b/arch/powerpc/kernel/secure_boot.c
> @@ -0,0 +1,42 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 IBM Corporation
> + * Author: Nayna Jain
> + */
> +#include <linux/types.h>
> +#include <linux/of.h>
> +#include <asm/secure_boot.h>
> +
> +struct device_node *get_powerpc_os_sb_node(void)
> +{
> +	return of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
> +}

Given that's only used in this file, once, it should just be inlined
into its caller.

> +
> +bool is_powerpc_os_secureboot_enabled(void)
> +{
> +	struct device_node *node;
> +
> +	node = get_powerpc_os_sb_node();
> +	if (!node)
> +		goto disabled;
> +
> +	if (!of_device_is_available(node)) {
> +		pr_err("Secure variables support is in error state, fail secure\n");
> +		goto enabled;
> +	}
> +
> +	/*
> +	 * secureboot is enabled if os-secure-enforcing property exists,
> +	 * else disabled.
> +	 */
> +	if (!of_find_property(node, "os-secure-enforcing", NULL))

Using of_property_read_bool() is preferable.

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

You could make that tail a bit more concise by doing something like
below, but up to you:

	bool enabled = false;
        ...

	enabled = of_property_read_bool(node, "os-secure-enforcing");
out:
	pr_info("secureboot mode %s\n", enabled ? "enabled" : "disabled");
	return enabled;
}


cheers

Re: [PATCH v7 2/8] powerpc: add support to initialize ima policy rules

[Nayna]

On 10/15/2019 07:29 AM, Michael Ellerman wrote:
> Nayna Jain <nayna@linux.ibm.com> writes:
>> PowerNV systems uses kernel based bootloader, thus its secure boot
>> implementation uses kernel IMA security subsystem to verify the kernel
>> before kexec. Since the verification policy might differ based on the
>> secure boot mode of the system, the policies are defined at runtime.
>>
>> This patch implements the arch-specific support to define the IMA policy
>> rules based on the runtime secure boot mode of the system.
>>
>> This patch provides arch-specific IMA policies if PPC_SECURE_BOOT
>> config is enabled.
> ...
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> new file mode 100644
>> index 000000000000..c22d82965eb4
>> --- /dev/null
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -0,0 +1,33 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2019 IBM Corporation
>> + * Author: Nayna Jain
>> + */
>> +
>> +#include <linux/ima.h>
>> +#include <asm/secure_boot.h>
>> +
>> +bool arch_ima_get_secureboot(void)
>> +{
>> +	return is_powerpc_os_secureboot_enabled();
>> +}
>> +
>> +/* Defines IMA appraise rules for secureboot */
>> +static const char *const arch_rules[] = {
>> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> +#if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
>> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> +#endif
> This confuses me.
>
> If I spell it out we get:
>
> #if IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
> 	// nothing
> #else
> 	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> #endif
>
> Which is just:
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> 	// nothing
> #else
> 	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> #endif
>
> But CONFIG_MODULE_SIG_FORCE enabled says that we *do* require modules to
> have a valid signature. Isn't that the inverse of what the rules say?
>
> Presumably I'm misunderstanding something :)

To avoid duplicate signature verification as much as possible, the IMA 
policy rule is added only if CONFIG_MODULE_SIG_FORCE is not enabled. 
CONFIG_MODULE_SIG_FORCE is part of modules support. IMA signature 
verification is based on policy.

Thanks & Regards,
      - Nayna

Re: [PATCH v7 4/8] powerpc/ima: add measurement rules to ima arch specific policy

[Nayna]

Hi Michael,


On 10/15/2019 07:29 AM, Michael Ellerman wrote:
> Nayna Jain <nayna@linux.ibm.com> writes:
>> This patch adds the measurement rules to the arch specific policies on
>> trusted boot enabled systems.
>>
>> Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
>> Reviewed-by: Mimi Zohar <zohar@linux.ibm.com>
>> ---
>>   arch/powerpc/kernel/ima_arch.c | 45 +++++++++++++++++++++++++++++++---
>>   1 file changed, 42 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
>> index c22d82965eb4..88bfe4a1a9a5 100644
>> --- a/arch/powerpc/kernel/ima_arch.c
>> +++ b/arch/powerpc/kernel/ima_arch.c
>> @@ -12,8 +12,19 @@ bool arch_ima_get_secureboot(void)
>>   	return is_powerpc_os_secureboot_enabled();
>>   }
>>   
>> -/* Defines IMA appraise rules for secureboot */
>> +/*
>> + * The "arch_rules" contains both the securebot and trustedboot rules for adding
>> + * the kexec kernel image and kernel modules file hashes to the IMA measurement
>> + * list and verifying the file signatures against known good values.
>> + *
>> + * The "appraise_type=imasig|modsig" option allows the good signature to be
>> + * stored as an xattr or as an appended signature. The "template=ima-modsig"
>> + * option includes the appended signature, when available, in the IMA
>> + * measurement list.
>> + */
>>   static const char *const arch_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK template=ima-modsig",
>> +	"measure func=MODULE_CHECK template=ima-modsig",
>>   	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>>   #if !IS_ENABLED(CONFIG_MODULE_SIG_FORCE)
>>   	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> @@ -22,12 +33,40 @@ static const char *const arch_rules[] = {
>>   };
>>   
>>   /*
>> - * Returns the relevant IMA arch policies based on the system secureboot state.
>> + * The "measure_rules" are enabled only on "trustedboot" enabled systems.
>> + * These rules add the kexec kernel image and kernel modules file hashes to
>> + * the IMA measurement list.
>> + */
>> +static const char *const measure_rules[] = {
>> +	"measure func=KEXEC_KERNEL_CHECK",
>> +	"measure func=MODULE_CHECK",
> Why do these ones not have "template=ima-modsig" on the end?

ima-modsig template is applicable only when IMA "collects" the appended 
signatures. IMA can then include it in the measurement list.

>
>> +	NULL
>> +};
>> +
>> +/*
>> + * Returns the relevant IMA arch policies based on the system secureboot
>> + * and trustedboot state.
>>    */
>>   const char *const *arch_get_ima_policy(void)
>>   {
>> -	if (is_powerpc_os_secureboot_enabled())
>> +	const char *const *rules;
>> +	int offset = 0;
>> +
>> +	for (rules = arch_rules; *rules != NULL; rules++) {
>> +		if (strncmp(*rules, "appraise", 8) == 0)
>> +			break;
>> +		offset++;
>> +	}
> This seems like kind of a hack, doesn't it? :)
>
> What we really want is three sets of rules isn't it? But some of them
> are shared between the different sets. But they just have to be flat
> arrays of strings.
>
> I think it would probably be cleaner to just use a #define for the
> shared part of the rules, eg something like:
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> #define APPRAISE_MODULE
> #else
> #define APPRAISE_MODULE \
> 	"appraise func=MODULE_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
> #endif
>
> #define APPRAISE_KERNEL \
> 	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig"
>
> #define MEASURE_KERNEL \
> 	"measure func=KEXEC_KERNEL_CHECK"
>
> #define MEASURE_MODULE \
> 	"measure func=MODULE_CHECK"
>
> #define APPEND_TEMPLATE_IMA_MODSIG		\
> 	" template=ima-modsig"
>
> static const char *const secure_and_trusted_rules[] = {
> 	MEASURE_KERNEL APPEND_TEMPLATE_IMA_MODSIG,
> 	MEASURE_MODULE APPEND_TEMPLATE_IMA_MODSIG,
> 	APPRAISE_KERNEL,
> 	APPRAISE_MODULE
> 	NULL
> };
>
> static const char *const secure_rules[] = {
> 	APPRAISE_KERNEL,
> 	APPRAISE_MODULE
> 	NULL
> };
>
> static const char *const trusted_rules[] = {
> 	MEASURE_KERNEL,
> 	MEASURE_MODULE,
> 	NULL
> };

Yes, I agree it is sort of a hack to walk through the rules to find the 
start of the appraise policy.  While trying your suggestion, I realized 
that defining three arrays, with some rule duplication, can fix the hack 
without #defines. This also improves readability of the rules. I have 
just now posted the new version with the changes. Please let me know if 
this looks ok.

Thanks & Regards,
      - Nayna

Re: [PATCH v7 7/8] ima: check against blacklisted hashes for files with modsig

[Nayna]

Hi Mimi,


On 10/11/2019 09:19 AM, Mimi Zohar wrote:
> On Mon, 2019-10-07 at 21:14 -0400, Nayna Jain wrote:
>> Asymmetric private keys are used to sign multiple files. The kernel
>> currently support checking against the blacklisted keys. However, if the
>> public key is blacklisted, any file signed by the blacklisted key will
>> automatically fail signature verification. We might not want to blacklist
>> all the files signed by a particular key, but just a single file.
>> Blacklisting the public key is not fine enough granularity.
>>
>> This patch adds support for blacklisting binaries with appended signatures,
>> based on the IMA policy.  Defined is a new policy option
>> "appraise_flag=check_blacklist".
> The blacklisted hash is not the same as the file hash, but is the file
> hash without the appended signature.  Are there tools for calculating
> the blacklisted hash?  Can you provide an example?

I have updated the patch description to specify that the blacklisted 
hash is the file hash without the appended signature. I hope that makes 
it clear now.

Thanks & Regards,
     - Nayna