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

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

[Nayna Jain]

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

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

PowerNV system uses a Linux-based bootloader to kexec the OS. The
bootloader kernel relies on IMA for signature verification of the OS
kernel before doing the kexec. This patchset adds support for powerpc
arch-specific IMA policies that are conditionally defined based on a
system's secure boot and trusted boot states. The OS secure boot and
trusted boot states are determined via device-tree properties.

The verification needs to be performed only for binaries that are not
blacklisted. The kernel currently only checks against the blacklist of
keys. However, doing so results in blacklisting all the binaries that
are signed by the same key. In order to prevent just one particular
binary from being loaded, it must be checked against a blacklist of
binary hashes. This patchset also adds support to IMA for checking
against a hash blacklist for files. signed by appended signature.

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

Changelog:

v9:
* Includes feedbacks from Michael
  * fix the missing of_node_put()
* Includes Mimi's feedbacks
  * fix the policy show() function to display check_blacklist
  * fix the other comment related and patch description
  * add the example of blacklist in the Patch 7/8
Note: Patch 7/8 is giving errors when checkpatch.pl is run because
of the format of showing measurement record as part of the example. I am
not very sure if that can be fixed as we need to represent the
measurements as is.

v8:
* Updates the Patch Description as per Michael's and Mimi's feedback
* Includes feedbacks from Michael for the device tree and policies
  * removes the arch-policy hack by defining three arrays.
  * fixes related to device-tree calls 
  * other code specific feedbacks
* Includes feedbacks from Mimi on the blacklist
  * generic blacklist function is modified than previous version
  * other coding fixes

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

Nayna Jain (8):
  powerpc: detect the secure boot mode of the system
  powerpc/ima: add support to initialize ima policy rules
  powerpc: detect the trusted boot state of the system
  powerpc/ima: define trusted boot 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   |  4 ++
 arch/powerpc/Kconfig                   | 11 ++++
 arch/powerpc/include/asm/secure_boot.h | 29 ++++++++++
 arch/powerpc/kernel/Makefile           |  2 +
 arch/powerpc/kernel/ima_arch.c         | 74 ++++++++++++++++++++++++++
 arch/powerpc/kernel/secure_boot.c      | 58 ++++++++++++++++++++
 certs/blacklist.c                      |  9 ++++
 include/keys/system_keyring.h          |  6 +++
 include/linux/ima.h                    |  3 +-
 security/integrity/ima/ima.h           | 11 ++++
 security/integrity/ima/ima_appraise.c  | 33 ++++++++++++
 security/integrity/ima/ima_main.c      | 63 ++++++++++++++--------
 security/integrity/ima/ima_policy.c    | 12 ++++-
 security/integrity/integrity.h         |  1 +
 14 files changed, 291 insertions(+), 25 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 v9 1/8] powerpc: detect the secure boot mode of the system

[Nayna Jain]

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

The PPC_SECURE_BOOT config represents the base enablement of secure boot
for powerpc.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
---
 arch/powerpc/Kconfig                   | 10 ++++++++
 arch/powerpc/include/asm/secure_boot.h | 23 ++++++++++++++++++
 arch/powerpc/kernel/Makefile           |  2 ++
 arch/powerpc/kernel/secure_boot.c      | 32 ++++++++++++++++++++++++++
 4 files changed, 67 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..56ea0019b616 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 need to define security
+	  policies to extend secure boot to the OS. This config allows a 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..07d0fe0ca81f
--- /dev/null
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -0,0 +1,23 @@
+/* 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_ppc_secureboot_enabled(void);
+
+#else
+
+static inline bool is_ppc_secureboot_enabled(void)
+{
+	return false;
+}
+
+#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..63dc82c50862
--- /dev/null
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -0,0 +1,32 @@
+// 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>
+
+bool is_ppc_secureboot_enabled(void)
+{
+	struct device_node *node;
+	bool enabled = false;
+
+	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
+	if (!of_device_is_available(node)) {
+		pr_err("Cannot find secure variable node in device tree; failing to secure state\n");
+		goto out;
+	}
+
+	/*
+	 * secureboot is enabled if os-secure-enforcing property exists,
+	 * else disabled.
+	 */
+	enabled = of_property_read_bool(node, "os-secure-enforcing");
+
+out:
+	of_node_put(node);
+
+	pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
+	return enabled;
+}
-- 
2.20.1

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

[Nayna Jain]

PowerNV system use a Linux-based bootloader, which relies on the IMA
subsystem to enforce different secure boot modes. Since the verification
policy may differ based on the secure boot mode of the system, the
policies must be defined at runtime.

This patch implements arch-specific support to define 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           |  1 +
 arch/powerpc/kernel/Makefile   |  2 +-
 arch/powerpc/kernel/ima_arch.c | 43 ++++++++++++++++++++++++++++++++++
 include/linux/ima.h            |  3 ++-
 4 files changed, 47 insertions(+), 2 deletions(-)
 create mode 100644 arch/powerpc/kernel/ima_arch.c

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 56ea0019b616..c795039bdc73 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -938,6 +938,7 @@ config PPC_SECURE_BOOT
 	prompt "Enable secure boot support"
 	bool
 	depends on PPC_POWERNV
+	depends on IMA_ARCH_POLICY
 	help
 	  Systems with firmware secure boot enabled need to define security
 	  policies to extend secure boot to the OS. This config allows a 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..d88913dc0da7
--- /dev/null
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -0,0 +1,43 @@
+// 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_ppc_secureboot_enabled();
+}
+
+/*
+ * The "secure_rules" are enabled only on "secureboot" enabled systems.
+ * These rules verify the file signatures against known good values.
+ * The "appraise_type=imasig|modsig" option allows the known good signature
+ * to be stored as an xattr or as an appended signature.
+ *
+ * To avoid duplicate signature verification as much as possible, the IMA
+ * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE
+ * is not enabled.
+ */
+static const char *const secure_rules[] = {
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+#ifndef CONFIG_MODULE_SIG_FORCE
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
+/*
+ * Returns the relevant IMA arch-specific policies based on the system secure
+ * boot state.
+ */
+const char *const *arch_get_ima_policy(void)
+{
+	if (is_ppc_secureboot_enabled())
+		return secure_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 v9 3/8] powerpc: detect the trusted boot state of the system

[Nayna Jain]

While secure boot permits only properly verified signed kernels to be
booted, trusted boot calculates the file hash of the kernel image and
stores the measurement prior to boot, that can be subsequently compared
against good known values via attestation services.

This patch reads the trusted boot state of a PowerNV system. The state
is used to conditionally enable additional measurement rules in the IMA
arch-specific policies.

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

diff --git a/arch/powerpc/include/asm/secure_boot.h b/arch/powerpc/include/asm/secure_boot.h
index 07d0fe0ca81f..a2ff556916c6 100644
--- a/arch/powerpc/include/asm/secure_boot.h
+++ b/arch/powerpc/include/asm/secure_boot.h
@@ -11,6 +11,7 @@
 #ifdef CONFIG_PPC_SECURE_BOOT
 
 bool is_ppc_secureboot_enabled(void);
+bool is_ppc_trustedboot_enabled(void);
 
 #else
 
@@ -19,5 +20,10 @@ static inline bool is_ppc_secureboot_enabled(void)
 	return false;
 }
 
+static inline bool is_ppc_trustedboot_enabled(void)
+{
+	return false;
+}
+
 #endif
 #endif
diff --git a/arch/powerpc/kernel/secure_boot.c b/arch/powerpc/kernel/secure_boot.c
index 63dc82c50862..a6a5f17ede03 100644
--- a/arch/powerpc/kernel/secure_boot.c
+++ b/arch/powerpc/kernel/secure_boot.c
@@ -7,6 +7,17 @@
 #include <linux/of.h>
 #include <asm/secure_boot.h>
 
+static struct device_node *get_ppc_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);
+}
+
 bool is_ppc_secureboot_enabled(void)
 {
 	struct device_node *node;
@@ -30,3 +41,18 @@ bool is_ppc_secureboot_enabled(void)
 	pr_info("Secure boot mode %s\n", enabled ? "enabled" : "disabled");
 	return enabled;
 }
+
+bool is_ppc_trustedboot_enabled(void)
+{
+	struct device_node *node;
+	bool enabled = false;
+
+	node = get_ppc_fw_sb_node();
+	enabled = of_property_read_bool(node, "trusted-enabled");
+
+	of_node_put(node);
+
+	pr_info("Trusted boot mode %s\n", enabled ? "enabled" : "disabled");
+
+	return enabled;
+}
-- 
2.20.1

[PATCH v9 4/8] powerpc/ima: define trusted boot policy

[Nayna Jain]

This patch defines an arch-specific trusted boot only policy and a
combined secure and trusted boot policy.

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

diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index d88913dc0da7..0ef5956c9753 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -30,6 +30,32 @@ static const char *const secure_rules[] = {
 	NULL
 };
 
+/*
+ * The "trusted_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 trusted_rules[] = {
+	"measure func=KEXEC_KERNEL_CHECK",
+	"measure func=MODULE_CHECK",
+	NULL
+};
+
+/*
+ * The "secure_and_trusted_rules" contains rules for both the secure boot and
+ * trusted boot. The "template=ima-modsig" option includes the appended
+ * signature, when available, in the IMA measurement list.
+ */
+static const char *const secure_and_trusted_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",
+#ifndef CONFIG_MODULE_SIG_FORCE
+	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
+#endif
+	NULL
+};
+
 /*
  * Returns the relevant IMA arch-specific policies based on the system secure
  * boot state.
@@ -37,7 +63,12 @@ static const char *const secure_rules[] = {
 const char *const *arch_get_ima_policy(void)
 {
 	if (is_ppc_secureboot_enabled())
-		return secure_rules;
+		if (is_ppc_trustedboot_enabled())
+			return secure_and_trusted_rules;
+		else
+			return secure_rules;
+	else if (is_ppc_trustedboot_enabled())
+		return trusted_rules;
 
 	return NULL;
 }
-- 
2.20.1

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

[Nayna Jain]

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 (e.g.
blacklisted binary hashes or key hashes).

process_buffer_measurement() may be called directly from an IMA
hook or as an auxiliary measurement record. In both cases the buffer
measurement is based on policy. This patch modifies the function to
conditionally retrieve the policy defined PCR and template for the IMA
hook case.

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

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 3689081aaf38..a65772ffa427 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, enum ima_hooks func,
+				int pcr);
 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..fe0b704ffdeb 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.
+ * @func: IMA hook
+ * @pcr: pcr to extend the measurement
  *
  * 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, enum ima_hooks func,
+				int pcr)
 {
 	int ret = 0;
 	struct ima_template_entry *entry = NULL;
@@ -642,19 +642,38 @@ 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 ima_template_desc *template = 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;
+	u32 secid;
 
-	action = ima_get_action(NULL, cred, secid, 0, KEXEC_CMDLINE, &pcr,
-				&template_desc);
-	if (!(action & IMA_MEASURE))
-		return;
+	if (func) {
+		security_task_getsecid(current, &secid);
+		action = ima_get_action(NULL, current_cred(), secid, 0, func,
+					&pcr, &template);
+		if (!(action & IMA_MEASURE))
+			return;
+	}
+
+	if (!pcr)
+		pcr = CONFIG_IMA_MEASURE_PCR_IDX;
+
+	if (!template) {
+		template = lookup_template_desc("ima-buf");
+		ret = template_desc_init_fields(template->fmt,
+						&(template->fields),
+						&(template->num_fields));
+		if (ret < 0) {
+			pr_err("template %s init failed, result: %d\n",
+			       (strlen(template->name) ?
+				template->name : template->fmt), ret);
+			return;
+		}
+	}
 
 	iint.ima_hash = &hash.hdr;
 	iint.ima_hash->algo = ima_hash_algo;
@@ -664,7 +683,7 @@ static void process_buffer_measurement(const void *buf, int size,
 	if (ret < 0)
 		goto out;
 
-	ret = ima_alloc_init_template(&event_data, &entry, template_desc);
+	ret = ima_alloc_init_template(&event_data, &entry, template);
 	if (ret < 0)
 		goto out;
 
@@ -686,13 +705,9 @@ static void process_buffer_measurement(const void *buf, int size,
  */
 void ima_kexec_cmdline(const void *buf, int size)
 {
-	u32 secid;
-
-	if (buf && size != 0) {
-		security_task_getsecid(current, &secid);
+	if (buf && size != 0)
 		process_buffer_measurement(buf, size, "kexec-cmdline",
-					   current_cred(), secid);
-	}
+					   KEXEC_CMDLINE, 0);
 }
 
 static int __init init_ima(void)
-- 
2.20.1

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

[Nayna Jain]

The -EKEYREJECTED error returned by existing is_hash_blacklisted() is
misleading when called for checking against blacklisted hash of a
binary.

This patch adds a wrapper function is_binary_blacklisted() to return
-EPERM error if binary is blacklisted.

Signed-off-by: Nayna Jain <nayna@linux.ibm.com>
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
-- 
2.20.1

[PATCH v9 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 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 checking against the blacklisted hash of the
file based on the IMA policy. The blacklisted hash is the file hash
without the appended signature. Defined is a new policy option
"appraise_flag=check_blacklist".

In addition to the blacklisted binary hashes stored in the firmware "dbx"
variable, the Linux kernel may be configured to load blacklisted binary
hashes onto the .blacklist keyring as well.  The following example shows
how to blacklist a kernel module.

$ sha256sum kernel/kheaders.ko
77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3  kern
el/kheaders.ko

$ grep BLACKLIST .config
# CONFIG_IMA_BLACKLIST_KEYRING is not set
CONFIG_SYSTEM_BLACKLIST_KEYRING=y
CONFIG_SYSTEM_BLACKLIST_HASH_LIST="blacklist-hash-list"

$ cat certs/blacklist-hash-list
"bin:77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3"

Update the IMA custom measurement and appraisal policy rules
(/etc/ima-policy):

measure func=MODULE_CHECK template=ima-modsig
appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig

After building, installing, and rebooting the kernel:

# keyctl show %keyring:.blacklist | grep 77fa889b35a05
 545660333 ---lswrv      0     0   \_ blacklist:
bin:77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3

# cat /sys/kernel/security/ima/policy | grep MODULE_CHECK
measure func=MODULE_CHECK template=ima-modsig
appraise func=MODULE_CHECK appraise_flag=check_blacklist
appraise_type=imasig|modsig

# modprobe kheaders
modprobe: ERROR: could not insert 'kheaders': Permission denied

# cat /sys/kernel/security/ima/ascii_runtime_measurements
10 0c9834db5a0182c1fb0cdc5d3adcf11a11fd83dd ima-sig
sha256:3bc6ed4f0b4d6e31bc1dbc9ef844605abc7afdc6d81a57d77a1ec9407997c40
2 /usr/lib/modules/5.4.0-rc3+/kernel/kernel/kheaders.ko

10 82aad2bcc3fa8ed94762356b5c14838f3bcfa6a0 ima-modsig
sha256:3bc6ed4f0b4d6e31bc1dbc9ef844605abc7afdc6d81a57d77a1ec9407997c40
2 /usr/lib/modules/5.4.0rc3+/kernel/kernel/kheaders.ko  sha256:77fa889b3
5a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3
3082029a06092a864886f70d010702a082028b30820287020101310d300b0609608648
016503040201300b06092a864886f70d01070131820264....

10 25b72217cc1152b44b134ce2cd68f12dfb71acb3 ima-buf
sha256:8b58427fedcf8f4b20bc8dc007f2e232bf7285d7b93a66476321f9c2a3aa132
b blacklisted-hash
77fa889b35a05338ec52e51591c1b89d4c8d1c99a21251d7c22b1a8642a6bad3

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

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 29ebe9afdac4..29aaedf33246 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=]
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
@@ -38,6 +39,9 @@ Description:
 			fowner:= decimal value
 		lsm:  	are LSM specific
 		option:	appraise_type:= [imasig] [imasig|modsig]
+			appraise_flag:= [check_blacklist]
+			Currently, blacklist check is only for files signed with appended
+			signature.
 			template:= name of a defined IMA template type
 			(eg, ima-ng). Only valid when action is "measure".
 			pcr:= decimal value
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index a65772ffa427..df4ca482fb53 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 pcr);
 int ima_appraise_measurement(enum ima_hooks func,
 			     struct integrity_iint_cache *iint,
 			     struct file *file, const unsigned char *filename,
@@ -271,6 +273,12 @@ 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 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..300c8d2943c5 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,38 @@ static int modsig_verify(enum ima_hooks func, const struct modsig *modsig,
 	return rc;
 }
 
+/*
+ * ima_check_blacklist - determine if the binary is blacklisted.
+ *
+ * Add the hash of the blacklisted binary to the measurement list, based
+ * on policy.
+ *
+ * Returns -EPERM if the hash is blacklisted.
+ */
+int ima_check_blacklist(struct integrity_iint_cache *iint,
+			const struct modsig *modsig, int pcr)
+{
+	enum hash_algo hash_algo;
+	const u8 *digest = NULL;
+	u32 digestsize = 0;
+	int rc = 0;
+
+	if (!(iint->flags & IMA_CHECK_BLACKLIST))
+		return 0;
+
+	if (iint->flags & IMA_MODSIG_ALLOWED && modsig) {
+		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
+
+		rc = is_binary_blacklisted(digest, digestsize);
+		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
+			process_buffer_measurement(digest, digestsize,
+						   "blacklisted-hash", NONE,
+						   pcr);
+	}
+
+	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 fe0b704ffdeb..13a0d64580ef 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, 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..f19a895ad7cd 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;
@@ -1500,6 +1506,8 @@ int ima_policy_show(struct seq_file *m, void *v)
 		else
 			seq_puts(m, "appraise_type=imasig ");
 	}
+	if (entry->flags & IMA_CHECK_BLACKLIST)
+		seq_puts(m, "appraise_flag=check_blacklist ");
 	if (entry->flags & IMA_PERMIT_DIRECTIO)
 		seq_puts(m, "permit_directio ");
 	rcu_read_unlock();
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 v9 8/8] powerpc/ima: update ima arch policy to check for blacklist

[Nayna Jain]

This patch updates the arch-specific policies for PowerNV system 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 | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index 0ef5956c9753..b9de0fb45bb9 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -23,9 +23,9 @@ bool arch_ima_get_secureboot(void)
  * is not enabled.
  */
 static const char *const secure_rules[] = {
-	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
+	"appraise func=KEXEC_KERNEL_CHECK appraise_flag=check_blacklist appraise_type=imasig|modsig",
 #ifndef 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
 };
@@ -49,9 +49,9 @@ static const char *const trusted_rules[] = {
 static const char *const secure_and_trusted_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",
 #ifndef 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 v9 5/8] ima: make process_buffer_measurement() generic

[Lakshmi Ramasubramanian]

On 10/23/19 8:47 PM, Nayna Jain wrote:

Hi Nayna,

> +void process_buffer_measurement(const void *buf, int size,
> +				const char *eventname, enum ima_hooks func,
> +				int pcr)
>   {
>   	int ret = 0;
>   	struct ima_template_entry *entry = NULL;

> +	if (func) {
> +		security_task_getsecid(current, &secid);
> +		action = ima_get_action(NULL, current_cred(), secid, 0, func,
> +					&pcr, &template);
> +		if (!(action & IMA_MEASURE))
> +			return;
> +	}

In your change set process_buffer_measurement is called with NONE for 
the parameter func. So ima_get_action (the above if block) will not be 
executed.

Wouldn't it better to update ima_get_action (and related functions) to 
handle the ima policy (func param)?

thanks,
  -lakshmi

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

[Lakshmi Ramasubramanian]

On 10/23/2019 8:47 PM, Nayna Jain wrote:
> This patch defines a function to detect the secure boot state of a
> PowerNV system.

> +bool is_ppc_secureboot_enabled(void)
> +{
> +	struct device_node *node;
> +	bool enabled = false;
> +
> +	node = of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
> +	if (!of_device_is_available(node)) {
> +		pr_err("Cannot find secure variable node in device tree; failing to secure state\n");
> +		goto out;

Related to "goto out;" above:

Would of_find_compatible_node return NULL if the given node is not found?

If of_device_is_available returns false (say, because node is NULL or it 
does not find the specified node) would it be correct to call of_node_put?

> +
> +out:
> +	of_node_put(node);

  -lakshmi

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

[Lakshmi Ramasubramanian]

On 10/23/2019 8:47 PM, Nayna Jain wrote:

> +/*
> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> + * These rules verify the file signatures against known good values.
> + * The "appraise_type=imasig|modsig" option allows the known good signature
> + * to be stored as an xattr or as an appended signature.
> + *
> + * To avoid duplicate signature verification as much as possible, the IMA
> + * policy rule for module appraisal is added only if CONFIG_MODULE_SIG_FORCE
> + * is not enabled.
> + */
> +static const char *const secure_rules[] = {
> +	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> +#ifndef CONFIG_MODULE_SIG_FORCE
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif
> +	NULL
> +};

Is there any way to not use conditional compilation in the above array 
definition? Maybe define different functions to get "secure_rules" for 
when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
Just a suggestion.

  -lakshmi

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

[Lakshmi Ramasubramanian]

On 10/23/2019 8:47 PM, Nayna Jain wrote:

> +bool is_ppc_trustedboot_enabled(void)
> +{
> +	struct device_node *node;
> +	bool enabled = false;
> +
> +	node = get_ppc_fw_sb_node();
> +	enabled = of_property_read_bool(node, "trusted-enabled");

Can get_ppc_fw_sb_node return NULL?
Would of_property_read_bool handle the case when node is NULL?

  -lakshmi

Re: [PATCH v9 4/8] powerpc/ima: define trusted boot policy

[Lakshmi Ramasubramanian]

On 10/23/2019 8:47 PM, Nayna Jain wrote:

> +/*
> + * The "secure_and_trusted_rules" contains rules for both the secure boot and
> + * trusted boot. The "template=ima-modsig" option includes the appended
> + * signature, when available, in the IMA measurement list.
> + */
> +static const char *const secure_and_trusted_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",
> +#ifndef CONFIG_MODULE_SIG_FORCE
> +	"appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> +#endif
> +	NULL
> +};

Same comment as earlier - any way to avoid using conditional compilation 
in C file?

  -lakshmi

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

[Lakshmi Ramasubramanian]

On 10/23/2019 8:47 PM, Nayna Jain wrote:

> +/*
> + * ima_check_blacklist - determine if the binary is blacklisted.
> + *
> + * Add the hash of the blacklisted binary to the measurement list, based
> + * on policy.
> + *
> + * Returns -EPERM if the hash is blacklisted.
> + */
> +int ima_check_blacklist(struct integrity_iint_cache *iint,
> +			const struct modsig *modsig, int pcr)
> +{
> +	enum hash_algo hash_algo;
> +	const u8 *digest = NULL;
> +	u32 digestsize = 0;
> +	int rc = 0;
> +
> +	if (!(iint->flags & IMA_CHECK_BLACKLIST))
> +		return 0;
> +
> +	if (iint->flags & IMA_MODSIG_ALLOWED && modsig) {
> +		ima_get_modsig_digest(modsig, &hash_algo, &digest, &digestsize);
> +
> +		rc = is_binary_blacklisted(digest, digestsize);
> +		if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
> +			process_buffer_measurement(digest, digestsize,
> +						   "blacklisted-hash", NONE,
> +						   pcr);
> +	}

The enum value "NONE" is being passed to process_buffer_measurement to 
indicate that the check for required action based on ima policy is 
already done by ima_check_blacklist. Not sure, but this can cause 
confusion in the future when someone updates process_buffer_measurement.

Would it instead be better to add another parameter to 
process_buffer_measurement to indicate the above condition?

  -lakshmi

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

[Nayna Jain]

On 10/24/19 12:26 PM, Lakshmi Ramasubramanian wrote:
> On 10/23/2019 8:47 PM, Nayna Jain wrote:
>> This patch defines a function to detect the secure boot state of a
>> PowerNV system.
>
>> +bool is_ppc_secureboot_enabled(void)
>> +{
>> +    struct device_node *node;
>> +    bool enabled = false;
>> +
>> +    node = of_find_compatible_node(NULL, NULL, "ibm,secvar-v1");
>> +    if (!of_device_is_available(node)) {
>> +        pr_err("Cannot find secure variable node in device tree; 
>> failing to secure state\n");
>> +        goto out;
>
> Related to "goto out;" above:
>
> Would of_find_compatible_node return NULL if the given node is not found?
>
> If of_device_is_available returns false (say, because node is NULL or 
> it does not find the specified node) would it be correct to call 
> of_node_put?

of_node_put() handles NULL.

Thanks & Regards,

      - Nayna

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

[Nayna Jain]

On 10/24/19 12:38 PM, Lakshmi Ramasubramanian wrote:
> On 10/23/2019 8:47 PM, Nayna Jain wrote:
>
>> +bool is_ppc_trustedboot_enabled(void)
>> +{
>> +    struct device_node *node;
>> +    bool enabled = false;
>> +
>> +    node = get_ppc_fw_sb_node();
>> +    enabled = of_property_read_bool(node, "trusted-enabled");
>
> Can get_ppc_fw_sb_node return NULL?
> Would of_property_read_bool handle the case when node is NULL?

Yes.

Thanks & Regards,

      - Nayna

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

[Nayna Jain]

On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
> On 10/23/2019 8:47 PM, Nayna Jain wrote:
>
>> +/*
>> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
>> + * These rules verify the file signatures against known good values.
>> + * The "appraise_type=imasig|modsig" option allows the known good 
>> signature
>> + * to be stored as an xattr or as an appended signature.
>> + *
>> + * To avoid duplicate signature verification as much as possible, 
>> the IMA
>> + * policy rule for module appraisal is added only if 
>> CONFIG_MODULE_SIG_FORCE
>> + * is not enabled.
>> + */
>> +static const char *const secure_rules[] = {
>> +    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>> +#ifndef CONFIG_MODULE_SIG_FORCE
>> +    "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>> +#endif
>> +    NULL
>> +};
>
> Is there any way to not use conditional compilation in the above array 
> definition? Maybe define different functions to get "secure_rules" for 
> when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.

How will you decide which function to be called ?

Thanks & Regards,

     - Nayna

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

[Nayna Jain]

On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote:
> On 10/23/19 8:47 PM, Nayna Jain wrote:
>
> Hi Nayna,
>
>> +void process_buffer_measurement(const void *buf, int size,
>> +                const char *eventname, enum ima_hooks func,
>> +                int pcr)
>>   {
>>       int ret = 0;
>>       struct ima_template_entry *entry = NULL;
>
>> +    if (func) {
>> +        security_task_getsecid(current, &secid);
>> +        action = ima_get_action(NULL, current_cred(), secid, 0, func,
>> +                    &pcr, &template);
>> +        if (!(action & IMA_MEASURE))
>> +            return;
>> +    }
>
> In your change set process_buffer_measurement is called with NONE for 
> the parameter func. So ima_get_action (the above if block) will not be 
> executed.
>
> Wouldn't it better to update ima_get_action (and related functions) to 
> handle the ima policy (func param)?


The idea is to use ima-buf template for the auxiliary measurement 
record. The auxiliary measurement record is an additional record to the 
one already created based on the existing policy. When func is passed as 
NONE, it represents it is an additional record. I am not sure what you 
mean by updating ima_get_action, it is already handling the ima policy.

Thanks & Regards,

     - Nayna

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

[Lakshmi Ramasubramanian]

On 10/25/2019 10:24 AM, Nayna Jain wrote:
> 
> On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote:
>> On 10/23/19 8:47 PM, Nayna Jain wrote:
>>
>> Hi Nayna,
>>
>>> +void process_buffer_measurement(const void *buf, int size,
>>> +                const char *eventname, enum ima_hooks func,
>>> +                int pcr)
>>>   {
>>>       int ret = 0;
>>>       struct ima_template_entry *entry = NULL;
>>
>>> +    if (func) {
>>> +        security_task_getsecid(current, &secid);
>>> +        action = ima_get_action(NULL, current_cred(), secid, 0, func,
>>> +                    &pcr, &template);
>>> +        if (!(action & IMA_MEASURE))
>>> +            return;
>>> +    }
>>
>> In your change set process_buffer_measurement is called with NONE for 
>> the parameter func. So ima_get_action (the above if block) will not be 
>> executed.
>>
>> Wouldn't it better to update ima_get_action (and related functions) to 
>> handle the ima policy (func param)?
> 
> 
> The idea is to use ima-buf template for the auxiliary measurement 
> record. The auxiliary measurement record is an additional record to the 
> one already created based on the existing policy. When func is passed as 
> NONE, it represents it is an additional record. I am not sure what you 
> mean by updating ima_get_action, it is already handling the ima policy.
>

I was referring to using "func" in process_buffer_measurement to 
determine ima action. In my opinion, process_buffer_measurement should 
be generic.

ima_get_action() should instead determine the required ima action, 
template, pcr, etc. based on "func" passed to it.

thanks,
  -lakshmi

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

[Nayna Jain]

On 10/24/19 12:48 PM, Lakshmi Ramasubramanian wrote:
> On 10/23/2019 8:47 PM, Nayna Jain wrote:
>
>> +/*
>> + * ima_check_blacklist - determine if the binary is blacklisted.
>> + *
>> + * Add the hash of the blacklisted binary to the measurement list, 
>> based
>> + * on policy.
>> + *
>> + * Returns -EPERM if the hash is blacklisted.
>> + */
>> +int ima_check_blacklist(struct integrity_iint_cache *iint,
>> +            const struct modsig *modsig, int pcr)
>> +{
>> +    enum hash_algo hash_algo;
>> +    const u8 *digest = NULL;
>> +    u32 digestsize = 0;
>> +    int rc = 0;
>> +
>> +    if (!(iint->flags & IMA_CHECK_BLACKLIST))
>> +        return 0;
>> +
>> +    if (iint->flags & IMA_MODSIG_ALLOWED && modsig) {
>> +        ima_get_modsig_digest(modsig, &hash_algo, &digest, 
>> &digestsize);
>> +
>> +        rc = is_binary_blacklisted(digest, digestsize);
>> +        if ((rc == -EPERM) && (iint->flags & IMA_MEASURE))
>> +            process_buffer_measurement(digest, digestsize,
>> +                           "blacklisted-hash", NONE,
>> +                           pcr);
>> +    }
>
> The enum value "NONE" is being passed to process_buffer_measurement to 
> indicate that the check for required action based on ima policy is 
> already done by ima_check_blacklist. Not sure, but this can cause 
> confusion in the future when someone updates process_buffer_measurement.


As I explained in the response to other patch, the purpose is to 
indicate that it is an auxiliary measurement record. By passing func as 
NONE, it implies there is no explicit policy to be queried for the 
template as it is an additional record for an existing policy and is to 
use ima-buf template.

What type of confusion do you mean ?

Thanks & Regards,

      - Nayna

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

[Lakshmi Ramasubramanian]

On 10/25/2019 10:02 AM, Nayna Jain wrote:

 >> Is there any way to not use conditional compilation in
 >> the above array definition? Maybe define different functions to get
 >> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when
 >> it is not defined.
 >
 > How will you decide which function to be called ?

Define the array in the C file:

const char *const secure_rules_kernel_check[] = {
    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
    NULL
};

const char *const secure_rules_kernel_module_check[] = {
    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
    "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
    NULL
};

And, in the header file :

extern const char *const secure_rules_kernel_check;
extern const char *const secure_rules_kernel_module_check;

#ifdef CONFIG_MODULE_SIG_FORCE
const char *secure_rules() { return secure_rules_kernel_check; }
#else
const char *secure_rules() { return secure_rules_kernel_module_check;}
#endif // #ifdef CONFIG_MODULE_SIG_FORCE

If you want to avoid duplication, secure_rules_kernel_check and 
secure_rules_kernel_module_check could be defined in separate C files 
and conditionally compiled (in Makefile).


I was just trying to suggest the guidelines given in
"Section 21) Conditional Compilation" in coding-style.rst.

It says:
Whenever possible don't use preprocessor conditionals (#ifdef, #if) in 
.c files;...

Feel free to do what you think is appropriate.

thanks,
  -lakshmi

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

[Mimi Zohar]

On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote:
> On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
> > On 10/23/2019 8:47 PM, Nayna Jain wrote:
> >
> >> +/*
> >> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> >> + * These rules verify the file signatures against known good values.
> >> + * The "appraise_type=imasig|modsig" option allows the known good 
> >> signature
> >> + * to be stored as an xattr or as an appended signature.
> >> + *
> >> + * To avoid duplicate signature verification as much as possible, 
> >> the IMA
> >> + * policy rule for module appraisal is added only if 
> >> CONFIG_MODULE_SIG_FORCE
> >> + * is not enabled.
> >> + */
> >> +static const char *const secure_rules[] = {
> >> +    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> >> +#ifndef CONFIG_MODULE_SIG_FORCE
> >> +    "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> >> +#endif
> >> +    NULL
> >> +};
> >
> > Is there any way to not use conditional compilation in the above array 
> > definition? Maybe define different functions to get "secure_rules" for 
> > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
> 
> How will you decide which function to be called ?

You could call "is_module_sig_enforced()".

Mimi

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

[Mimi Zohar]

On Fri, 2019-10-25 at 10:32 -0700, Lakshmi Ramasubramanian wrote:
> 
> On 10/25/2019 10:24 AM, Nayna Jain wrote:
> > 
> > On 10/24/19 10:20 AM, Lakshmi Ramasubramanian wrote:
> >> On 10/23/19 8:47 PM, Nayna Jain wrote:
> >>
> >> Hi Nayna,
> >>
> >>> +void process_buffer_measurement(const void *buf, int size,
> >>> +                const char *eventname, enum ima_hooks func,
> >>> +                int pcr)
> >>>   {
> >>>       int ret = 0;
> >>>       struct ima_template_entry *entry = NULL;
> >>
> >>> +    if (func) {

Let's comment this line.  Perhaps something like /*Unnecessary for
auxiliary buffer measurements */
> >>> +        security_task_getsecid(current, &secid);
> >>> +        action = ima_get_action(NULL, current_cred(), secid, 0, func,
> >>> +                    &pcr, &template);
> >>> +        if (!(action & IMA_MEASURE))
> >>> +            return;
> >>> +    }
> >>
> >> In your change set process_buffer_measurement is called with NONE for 
> >> the parameter func. So ima_get_action (the above if block) will not be 
> >> executed.
> >>
> >> Wouldn't it better to update ima_get_action (and related functions) to 
> >> handle the ima policy (func param)?
> > 
> > 
> > The idea is to use ima-buf template for the auxiliary measurement 
> > record. The auxiliary measurement record is an additional record to the 
> > one already created based on the existing policy. When func is passed as 
> > NONE, it represents it is an additional record. I am not sure what you 
> > mean by updating ima_get_action, it is already handling the ima policy.
> >
> 
> I was referring to using "func" in process_buffer_measurement to 
> determine ima action. In my opinion, process_buffer_measurement should 
> be generic.
> 
> ima_get_action() should instead determine the required ima action, 
> template, pcr, etc. based on "func" passed to it.

Nayna's original patch moved ima_get_action() into the caller, but
that resulted in code duplication in each of the callers.  This
solution differentiates between the initial, which requires calling
ima_get_action(), and auxiliary buffer measurement records.

Mimi 

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

[Mimi Zohar]

On Sat, 2019-10-26 at 19:52 -0400, Mimi Zohar wrote:
> On Fri, 2019-10-25 at 12:02 -0500, Nayna Jain wrote:
> > On 10/24/19 12:35 PM, Lakshmi Ramasubramanian wrote:
> > > On 10/23/2019 8:47 PM, Nayna Jain wrote:
> > >
> > >> +/*
> > >> + * The "secure_rules" are enabled only on "secureboot" enabled systems.
> > >> + * These rules verify the file signatures against known good values.
> > >> + * The "appraise_type=imasig|modsig" option allows the known good 
> > >> signature
> > >> + * to be stored as an xattr or as an appended signature.
> > >> + *
> > >> + * To avoid duplicate signature verification as much as possible, 
> > >> the IMA
> > >> + * policy rule for module appraisal is added only if 
> > >> CONFIG_MODULE_SIG_FORCE
> > >> + * is not enabled.
> > >> + */
> > >> +static const char *const secure_rules[] = {
> > >> +    "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
> > >> +#ifndef CONFIG_MODULE_SIG_FORCE
> > >> +    "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
> > >> +#endif
> > >> +    NULL
> > >> +};
> > >
> > > Is there any way to not use conditional compilation in the above array 
> > > definition? Maybe define different functions to get "secure_rules" for 
> > > when CONFIG_MODULE_SIG_FORCE is defined and when it is not defined.
> > 
> > How will you decide which function to be called ?
> 
> You could call "is_module_sig_enforced()".

Calling is_module_sig_enforce() would prevent verifying the same
kernel module appended signature twice, when CONFIG_MODULE_SIG is
enabled, but not CONFIG_MODULE_SIG_FORCE.  This comes at the expense
of having to define additional policies.

Unlike for the kernel image, there is no coordination between lockdown
and IMA for kernel modules signature verification.  I suggest
deferring defining additional policies to when the lockdown/IMA
coordination is addressed.

Mimi

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

[Mimi Zohar]

On Wed, 2019-10-23 at 22:47 -0500, Nayna Jain wrote:
> This patchset extends the previous version[1] by adding support for
> checking against a blacklist of binary hashes.
> 
> The IMA subsystem supports custom, built-in, arch-specific policies to
> define the files to be measured and appraised. These policies are honored
> based on priority, where arch-specific policy is the highest and custom
> is the lowest.
> 
> PowerNV system uses a Linux-based bootloader to kexec the OS. The
> bootloader kernel relies on IMA for signature verification of the OS
> kernel before doing the kexec. This patchset adds support for powerpc
> arch-specific IMA policies that are conditionally defined based on a
> system's secure boot and trusted boot states. The OS secure boot and
> trusted boot states are determined via device-tree properties.
> 
> The verification needs to be performed only for binaries that are not
> blacklisted. The kernel currently only checks against the blacklist of
> keys. However, doing so results in blacklisting all the binaries that
> are signed by the same key. In order to prevent just one particular
> binary from being loaded, it must be checked against a blacklist of
> binary hashes. This patchset also adds support to IMA for checking
> against a hash blacklist for files. signed by appended signature.
> 
> [1] http://patchwork.ozlabs.org/cover/1149262/ 

Thanks, Nayna.

Please feel free to add my Signed-off-by tag on patches (2, 4, 5, 7 &
8).

thanks,

Mimi

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

[Michael Ellerman]

Hi Lakshmi,

Lakshmi Ramasubramanian <nramas@linux.microsoft.com> writes:
> On 10/25/2019 10:02 AM, Nayna Jain wrote:
>
>  >> Is there any way to not use conditional compilation in
>  >> the above array definition? Maybe define different functions to get
>  >> "secure_rules" for when CONFIG_MODULE_SIG_FORCE is defined and when
>  >> it is not defined.
>  >
>  > How will you decide which function to be called ?
>
> Define the array in the C file:
>
> const char *const secure_rules_kernel_check[] = {
>     "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>     NULL
> };
>
> const char *const secure_rules_kernel_module_check[] = {
>     "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig|modsig",
>     "appraise func=MODULE_CHECK appraise_type=imasig|modsig",
>     NULL
> };
>
> And, in the header file :

But there's no reason for any of this to be in a header, it's all
contained in one file.

Moving things into a header purely to avoid a single #ifdef in a C file
is a backward step.

> extern const char *const secure_rules_kernel_check;
> extern const char *const secure_rules_kernel_module_check;
>
> #ifdef CONFIG_MODULE_SIG_FORCE
> const char *secure_rules() { return secure_rules_kernel_check; }
> #else
> const char *secure_rules() { return secure_rules_kernel_module_check;}
> #endif // #ifdef CONFIG_MODULE_SIG_FORCE
>
> If you want to avoid duplication, secure_rules_kernel_check and 
> secure_rules_kernel_module_check could be defined in separate C files 
> and conditionally compiled (in Makefile).

Again that's just lots of added complication for no real benefit.

> I was just trying to suggest the guidelines given in
> "Section 21) Conditional Compilation" in coding-style.rst.
>
> It says:
> Whenever possible don't use preprocessor conditionals (#ifdef, #if) in 
> .c files;...

The key phrase being "guideline" :)

That suggestion is aimed at avoiding code with lots of ifdefs sprinkled
through the body of functions. Code written in that way can be very hard
to read because you have to mentally pre-process it first, and then read
the C-level logic. See below for an example.

Moving the pre-processing out of line into helpers means when you're
reading the function you can just reason about the C control flow.

The reference to ".c files" is really talking about moving logic that is
#ifdef'ed into static inline helpers. Those typically go in headers, but
they don't have to if there's no other reason for them to be in a
header.

So where the code is all in one C file it would be completely fine to
have an #ifdef in the C file around a static inline helper.

But in this case where the #ifdef is just in an array I think it's
entirely fine to just keep the #ifdef. Its presence there doesn't
complicate the logic in anyway.

cheers



This is a "good" (bad) example of what we're trying to avoid:

static long ppc_set_hwdebug(struct task_struct *child,
		     struct ppc_hw_breakpoint *bp_info)
{
#ifdef CONFIG_HAVE_HW_BREAKPOINT
	int len = 0;
	struct thread_struct *thread = &(child->thread);
	struct perf_event *bp;
	struct perf_event_attr attr;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */
#ifndef CONFIG_PPC_ADV_DEBUG_REGS
	struct arch_hw_breakpoint brk;
#endif

	if (bp_info->version != 1)
		return -ENOTSUPP;
#ifdef CONFIG_PPC_ADV_DEBUG_REGS
	/*
	 * Check for invalid flags and combinations
	 */
	if ((bp_info->trigger_type == 0) ||
	    (bp_info->trigger_type & ~(PPC_BREAKPOINT_TRIGGER_EXECUTE |
				       PPC_BREAKPOINT_TRIGGER_RW)) ||
	    (bp_info->addr_mode & ~PPC_BREAKPOINT_MODE_MASK) ||
	    (bp_info->condition_mode &
	     ~(PPC_BREAKPOINT_CONDITION_MODE |
	       PPC_BREAKPOINT_CONDITION_BE_ALL)))
		return -EINVAL;
#if CONFIG_PPC_ADV_DEBUG_DVCS == 0
	if (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
		return -EINVAL;
#endif

	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_EXECUTE) {
		if ((bp_info->trigger_type != PPC_BREAKPOINT_TRIGGER_EXECUTE) ||
		    (bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE))
			return -EINVAL;
		return set_instruction_bp(child, bp_info);
	}
	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
		return set_dac(child, bp_info);

#ifdef CONFIG_PPC_ADV_DEBUG_DAC_RANGE
	return set_dac_range(child, bp_info);
#else
	return -EINVAL;
#endif
#else /* !CONFIG_PPC_ADV_DEBUG_DVCS */
	/*
	 * We only support one data breakpoint
	 */
	if ((bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_RW) == 0 ||
	    (bp_info->trigger_type & ~PPC_BREAKPOINT_TRIGGER_RW) != 0 ||
	    bp_info->condition_mode != PPC_BREAKPOINT_CONDITION_NONE)
		return -EINVAL;

	if ((unsigned long)bp_info->addr >= TASK_SIZE)
		return -EIO;

	brk.address = bp_info->addr & ~7UL;
	brk.type = HW_BRK_TYPE_TRANSLATE;
	brk.len = 8;
	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_READ)
		brk.type |= HW_BRK_TYPE_READ;
	if (bp_info->trigger_type & PPC_BREAKPOINT_TRIGGER_WRITE)
		brk.type |= HW_BRK_TYPE_WRITE;
#ifdef CONFIG_HAVE_HW_BREAKPOINT
	/*
	 * Check if the request is for 'range' breakpoints. We can
	 * support it if range < 8 bytes.
	 */
	if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_RANGE_INCLUSIVE)
		len = bp_info->addr2 - bp_info->addr;
	else if (bp_info->addr_mode == PPC_BREAKPOINT_MODE_EXACT)
		len = 1;
	else
		return -EINVAL;
	bp = thread->ptrace_bps[0];
	if (bp)
		return -ENOSPC;

	/* Create a new breakpoint request if one doesn't exist already */
	hw_breakpoint_init(&attr);
	attr.bp_addr = (unsigned long)bp_info->addr & ~HW_BREAKPOINT_ALIGN;
	attr.bp_len = len;
	arch_bp_generic_fields(brk.type, &attr.bp_type);

	thread->ptrace_bps[0] = bp = register_user_hw_breakpoint(&attr,
					       ptrace_triggered, NULL, child);
	if (IS_ERR(bp)) {
		thread->ptrace_bps[0] = NULL;
		return PTR_ERR(bp);
	}

	return 1;
#endif /* CONFIG_HAVE_HW_BREAKPOINT */

	if (bp_info->addr_mode != PPC_BREAKPOINT_MODE_EXACT)
		return -EINVAL;

	if (child->thread.hw_brk.address)
		return -ENOSPC;

	if (!ppc_breakpoint_available())
		return -ENODEV;

	child->thread.hw_brk = brk;

	return 1;
#endif /* !CONFIG_PPC_ADV_DEBUG_DVCS */
}

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

[Lakshmi Ramasubramanian]

On 10/23/19 8:47 PM, Nayna Jain wrote:

Hi Nayna,

> 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 (e.g.
> blacklisted binary hashes or key hashes).

Now that process_buffer_measurement() is being made generic to measure 
any buffer, it would be good to add a tag to indicate what type of 
buffer is being measured.

For example, if the buffer is kexec command line the log could look like:

  "kexec_cmdline: <command line data>"

Similarly, if the buffer is blacklisted binary hash:

  "blacklist hash: <data>".

If the buffer is key hash:

  "<name of the keyring>: key data".

This would greatly help the consumer of the IMA log to know the type of 
data represented in each IMA log entry.

thanks,
  -lakshmi

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

[Mimi Zohar]

On Wed, 2019-10-30 at 08:22 -0700, Lakshmi Ramasubramanian wrote:
> On 10/23/19 8:47 PM, Nayna Jain wrote:
> 
> Hi Nayna,
> 
> > 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 (e.g.
> > blacklisted binary hashes or key hashes).
> 
> Now that process_buffer_measurement() is being made generic to measure 
> any buffer, it would be good to add a tag to indicate what type of 
> buffer is being measured.
> 
> For example, if the buffer is kexec command line the log could look like:
> 
>   "kexec_cmdline: <command line data>"
> 
> Similarly, if the buffer is blacklisted binary hash:
> 
>   "blacklist hash: <data>".
> 
> If the buffer is key hash:
> 
>   "<name of the keyring>: key data".
> 
> This would greatly help the consumer of the IMA log to know the type of 
> data represented in each IMA log entry.

Both the existing kexec command line and the new blacklist buffer
measurement pass that information in the eventname.   The [PATCH 7/8]
"ima: check against blacklisted hashes for files with modsig" patch
description includes an example.

Mimi