[PATCH 0/4] Add support for architecture-specific IMA policies

[PATCH 0/4] Add support for architecture-specific IMA policies

[Eric Richter]

IMA can measure and appraise kernel images, but requires the appropriate
policy to be set to do so. This patch set adds the ability for different
architectures to define their own arch-specific default policies to be
loaded at run-time by implementing the arch_ima_get_policy() function.
This allows for the policy to be loaded based on the current system state,
such as secure boot state.

Included is an example patch that loads a set of IMA appraise rules
requiring the kexec kernel images to be measured and signed when EFI secure
boot is enabled. 

This set also contains a patch to IMA that adds a separate appraise func=
specifically for the kexec_load syscall. IMA cannot appraise images loaded
with kexec_load, and therefore automatically fails the signature check --
effectively disabling the syscall when the appropriate appraise rule is
set. This allows for the kexec_load syscall to be "disabled" via IMA
policy, but not conflict with the existing kexec_file_load signature
verification.

Eric Richter (2):
  ima: add support for KEXEC_ORIG_KERNEL_CHECK
  x86/ima: define arch_get_ima_policy() for x86

Nayna Jain (2):
  ima: add support for arch specific policies
  ima: add support for external setting of ima_appraise

 Documentation/ABI/testing/ima_policy  |   1 +
 arch/x86/kernel/Makefile              |   2 +
 arch/x86/kernel/ima_arch.c            |  27 +++++++++
 include/linux/ima.h                   |  13 +++++
 security/integrity/ima/Kconfig        |   8 +++
 security/integrity/ima/ima.h          |   7 +++
 security/integrity/ima/ima_appraise.c |  11 +++-
 security/integrity/ima/ima_main.c     |   3 +-
 security/integrity/ima/ima_policy.c   | 103 ++++++++++++++++++++++++++++++++++
 9 files changed, 172 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/kernel/ima_arch.c

-- 
2.14.4

[PATCH 1/4] ima: add support for arch specific policies

[Eric Richter]

From: Nayna Jain <nayna@linux.vnet.ibm.com>

Builtin IMA policies can be enabled on the boot command line, and
replaced with a custom policy, normally during early boot in the
initramfs.  Build time IMA policy rules were recently added.  These
rules are automatically enabled on boot and persist after loading a
custom policy.

There is a need for yet another type of policy, an architecture specific
policy, which is derived at runtime during kernel boot, based on the
runtime secure boot flags.  Like the build time policy rules, these
rules persist after loading a custom policy.

This patch adds support for loading an architecture specific IMA
policy.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
- Defined function to convert the arch policy strings to an array of
ima_entry_rules.  The memory can then be freed after loading a custom
policy.
- Rename ima_get_arch_policy to arch_get_ima_policy.
Signed-off-by: Mimi Zohar <zohar@linux.ibm.com>
---
 include/linux/ima.h                 |  5 ++
 security/integrity/ima/ima_policy.c | 95 +++++++++++++++++++++++++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 84806b54b50..7fd272f0b1f 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,6 +30,11 @@ extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
+static inline const char * const *arch_get_ima_policy(void)
+{
+	return NULL;
+}
+
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
 {
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8c9499867c9..b47db4d7fea 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -20,6 +20,7 @@
 #include <linux/rculist.h>
 #include <linux/genhd.h>
 #include <linux/seq_file.h>
+#include <linux/ima.h>
 
 #include "ima.h"
 
@@ -193,6 +194,10 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
 	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
 };
 
+/* An array of architecture specific rules */
+struct ima_rule_entry **arch_policy_rules __ro_after_init;
+struct ima_rule_entry *arch_policy_entry __ro_after_init;
+
 static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);
 static LIST_HEAD(ima_temp_rules);
@@ -473,6 +478,59 @@ static int ima_appraise_flag(enum ima_hooks func)
 	return 0;
 }
 
+static int ima_parse_rule(char *rule, struct ima_rule_entry *entry);
+
+/*
+ * ima_init_arch_policy - convert arch policy strings to rules
+ *
+ * Return number of arch specific rules.
+ */
+static int __init ima_init_arch_policy(void)
+{
+	const char * const *arch_rules;
+	const char * const *rules;
+	int arch_entries = 0;
+	int i = 0;
+
+	arch_rules = arch_get_ima_policy();
+	if (!arch_rules) {
+		pr_info("No architecture policy rules.\n");
+		return arch_entries;
+	}
+
+	/* Get number of rules */
+	for (rules = arch_rules; *rules != NULL; rules++)
+		arch_entries++;
+
+	arch_policy_rules = kcalloc(arch_entries + 1,
+				    sizeof(*arch_policy_rules), GFP_KERNEL);
+	if (!arch_policy_rules)
+		return 0;
+
+	arch_policy_entry = kcalloc(arch_entries + 1,
+				    sizeof(*arch_policy_entry), GFP_KERNEL);
+
+	/* Convert arch policy string rules to struct ima_rule_entry format */
+	for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
+		char rule[255];
+		int result;
+
+		result = strlcpy(rule, *rules, sizeof(rule));
+
+		INIT_LIST_HEAD(&arch_policy_entry[i].list);
+		result = ima_parse_rule(rule, &arch_policy_entry[i]);
+		if (result) {
+			pr_warn("Skipping unknown architecture policy rule: %s\n", rule);
+			memset(&arch_policy_entry[i], 0,
+			       sizeof(*arch_policy_entry));
+			continue;
+		}
+		arch_policy_rules[i] = &arch_policy_entry[i];
+		i++;
+	}
+	return i;
+}
+
 /**
  * ima_init_policy - initialize the default measure rules.
  *
@@ -482,6 +540,7 @@ static int ima_appraise_flag(enum ima_hooks func)
 void __init ima_init_policy(void)
 {
 	int i, measure_entries, appraise_entries, secure_boot_entries;
+	int arch_policy_entries;
 
 	/* if !ima_policy set entries = 0 so we load NO default rules */
 	measure_entries = ima_policy ? ARRAY_SIZE(dont_measure_rules) : 0;
@@ -507,6 +566,33 @@ void __init ima_init_policy(void)
 		break;
 	}
 
+	/*
+	 * Based on runtime secure boot flags, insert arch specific measurement
+	 * and appraise rules requiring file signatures for both the initial
+	 * and custom policies, prior to other appraise rules.
+	 * (Highest priority)
+	 */
+	arch_policy_entries = ima_init_arch_policy();
+	if (arch_policy_entries > 0)
+		pr_info("Adding %d architecture policy rules.\n", arch_policy_entries);
+	for (i = 0; i < arch_policy_entries; i++) {
+		struct ima_rule_entry *entry;
+
+		list_add_tail(&arch_policy_rules[i]->list, &ima_default_rules);
+
+		entry = kmemdup(&arch_policy_entry[i], sizeof(*entry),
+				GFP_KERNEL);
+		if (!entry) {
+			WARN_ONCE(true, "Failed adding architecture rules to custom policy\n");
+			continue;
+		}
+
+		INIT_LIST_HEAD(&entry->list);
+		list_add_tail(&entry->list, &ima_policy_rules);
+		if (entry->action == APPRAISE)
+			build_ima_appraise |= ima_appraise_flag(entry->func);
+	}
+
 	/*
 	 * Insert the builtin "secure_boot" policy rules requiring file
 	 * signatures, prior to any other appraise rules.
@@ -576,6 +662,15 @@ void ima_update_policy(void)
 	if (ima_rules != policy) {
 		ima_policy_flag = 0;
 		ima_rules = policy;
+
+		/*
+		 * IMA architecture specific policy rules are specified
+		 * as strings and converted to an array of ima_entry_rules
+		 * on boot.  After loading a custom policy, free the
+		 * architecture specific rules stored as an array.
+		 */
+		kfree(arch_policy_rules);
+		kfree(arch_policy_entry);
 	}
 	ima_update_policy_flag();
 }
-- 
2.14.4

[PATCH 2/4] ima: add support for external setting of ima_appraise

[Eric Richter]

From: Nayna Jain <nayna@linux.vnet.ibm.com>

The "ima_appraise" mode defaults to enforcing, unless configured to
allow the boot command line "ima_appraise" option. This patch allows
the "ima_appraise" mode to be defined based on the arch setting.

Signed-off-by: Nayna Jain <nayna@linux.vnet.ibm.com>
---
 security/integrity/ima/ima.h          |  5 +++++
 security/integrity/ima/ima_appraise.c | 11 +++++++++--
 security/integrity/ima/ima_policy.c   |  5 ++++-
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 588e4813370..6e5fa7c4280 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -248,6 +248,7 @@ enum hash_algo ima_get_hash_algo(struct evm_ima_xattr_data *xattr_value,
 				 int xattr_len);
 int ima_read_xattr(struct dentry *dentry,
 		   struct evm_ima_xattr_data **xattr_value);
+void set_ima_appraise(char *str);
 
 #else
 static inline int ima_appraise_measurement(enum ima_hooks func,
@@ -290,6 +291,10 @@ static inline int ima_read_xattr(struct dentry *dentry,
 	return 0;
 }
 
+static inline void set_ima_appraise(char *str)
+{
+}
+
 #endif /* CONFIG_IMA_APPRAISE */
 
 /* LSM based policy rules require audit */
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index 8bd7a0733e5..e061613bcb8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -18,15 +18,22 @@
 
 #include "ima.h"
 
-static int __init default_appraise_setup(char *str)
+void set_ima_appraise(char *str)
 {
-#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
 	if (strncmp(str, "off", 3) == 0)
 		ima_appraise = 0;
 	else if (strncmp(str, "log", 3) == 0)
 		ima_appraise = IMA_APPRAISE_LOG;
 	else if (strncmp(str, "fix", 3) == 0)
 		ima_appraise = IMA_APPRAISE_FIX;
+	else if (strncmp(str, "enforce", 7) == 0)
+		ima_appraise = IMA_APPRAISE_ENFORCE;
+}
+
+static int __init default_appraise_setup(char *str)
+{
+#ifdef CONFIG_IMA_APPRAISE_BOOTPARAM
+	set_ima_appraise(str);
 #endif
 	return 1;
 }
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b47db4d7fea..402e5bd1093 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -573,8 +573,11 @@ void __init ima_init_policy(void)
 	 * (Highest priority)
 	 */
 	arch_policy_entries = ima_init_arch_policy();
-	if (arch_policy_entries > 0)
+	if (arch_policy_entries > 0) {
 		pr_info("Adding %d architecture policy rules.\n", arch_policy_entries);
+		set_ima_appraise("enforce");
+	}
+
 	for (i = 0; i < arch_policy_entries; i++) {
 		struct ima_rule_entry *entry;
 
-- 
2.14.4

[PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK

[Eric Richter]

IMA can verify the signature of kernel images loaded with kexec_file_load,
but can not verify images loaded with the regular kexec_load syscall.
Therefore, the appraisal will automatically fail during kexec_load when an
appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used
to effectively disable the kexec_load syscall, while still allowing the
kexec_file_load to operate so long as the target kernel image is signed.

However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is
enabled and there is an appraise rule set, then the target kernel would
have to be verifiable by both IMA and the architecture specific kernel
verification procedure.

This patch adds a new func= for IMA appraisal specifically for the original
kexec_load syscall. Therefore, the kexec_load syscall can be effectively
disabled via IMA policy, leaving the kexec_file_load syscall able to do its
own signature verification, and not require it to be signed via IMA. To
retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is
unchanged, and thus enables appraisal for both kexec syscalls.

Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/ima_policy | 1 +
 security/integrity/ima/ima.h         | 2 ++
 security/integrity/ima/ima_main.c    | 3 ++-
 security/integrity/ima/ima_policy.c  | 5 +++++
 4 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/ima_policy b/Documentation/ABI/testing/ima_policy
index 74c6702de74..031417779ec 100644
--- a/Documentation/ABI/testing/ima_policy
+++ b/Documentation/ABI/testing/ima_policy
@@ -29,6 +29,7 @@ Description:
 		base: 	func:= [BPRM_CHECK][MMAP_CHECK][CREDS_CHECK][FILE_CHECK][MODULE_CHECK]
 				[FIRMWARE_CHECK]
 				[KEXEC_KERNEL_CHECK] [KEXEC_INITRAMFS_CHECK]
+				[KEXEC_ORIG_KERNEL_CHECK]
 			mask:= [[^]MAY_READ] [[^]MAY_WRITE] [[^]MAY_APPEND]
 			       [[^]MAY_EXEC]
 			fsmagic:= hex value
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 6e5fa7c4280..c76e53c982b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -181,6 +181,7 @@ static inline unsigned long ima_hash_key(u8 *digest)
 	hook(MODULE_CHECK)		\
 	hook(FIRMWARE_CHECK)		\
 	hook(KEXEC_KERNEL_CHECK)	\
+	hook(KEXEC_ORIG_KERNEL_CHECK)	\
 	hook(KEXEC_INITRAMFS_CHECK)	\
 	hook(POLICY_CHECK)		\
 	hook(MAX_CHECK)
@@ -233,6 +234,7 @@ int ima_policy_show(struct seq_file *m, void *v);
 #define IMA_APPRAISE_FIRMWARE	0x10
 #define IMA_APPRAISE_POLICY	0x20
 #define IMA_APPRAISE_KEXEC	0x40
+#define IMA_APPRAISE_ORIG_KEXEC	0x80
 
 #ifdef CONFIG_IMA_APPRAISE
 int ima_appraise_measurement(enum ima_hooks func,
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index dce0a8a217b..a7b4220043d 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -512,7 +512,8 @@ int ima_load_data(enum kernel_load_data_id id)
 
 	switch (id) {
 	case LOADING_KEXEC_IMAGE:
-		if (ima_appraise & IMA_APPRAISE_KEXEC) {
+		if (ima_appraise &
+		    (IMA_APPRAISE_ORIG_KEXEC | IMA_APPRAISE_KEXEC)) {
 			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
 		}
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 402e5bd1093..7a33e3f6eca 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -475,6 +475,8 @@ static int ima_appraise_flag(enum ima_hooks func)
 		return IMA_APPRAISE_POLICY;
 	else if (func == KEXEC_KERNEL_CHECK)
 		return IMA_APPRAISE_KEXEC;
+	else if (func == KEXEC_ORIG_KERNEL_CHECK)
+		return IMA_APPRAISE_ORIG_KEXEC;
 	return 0;
 }
 
@@ -879,6 +881,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 			else if (strcmp(args[0].from, "KEXEC_KERNEL_CHECK") ==
 				 0)
 				entry->func = KEXEC_KERNEL_CHECK;
+			else if (strcmp(args[0].from,
+				 "KEXEC_ORIG_KERNEL_CHECK") == 0)
+				entry->func = KEXEC_ORIG_KERNEL_CHECK;
 			else if (strcmp(args[0].from, "KEXEC_INITRAMFS_CHECK")
 				 == 0)
 				entry->func = KEXEC_INITRAMFS_CHECK;
-- 
2.14.4

[PATCH 4/4] x86/ima: define arch_get_ima_policy() for x86

[Eric Richter]

This patch implements an example arch-specific IMA policy for x86 to enable
measurement and appraisal of any kernel images loaded for kexec, and
disables the kexec_load syscall.

To avoid conflicting with the existing CONFIG_KERNEL_VERIFY_SIG option, the
policy only "appraises" the target image on kexec_load. Without this, the
target kexec image would have to be verified by both the above option as
well as by IMA appraisal.

Since signature verification for kexec_load is not possible via appraisal
(or VERIFY_SIG), this results in a failure and thus effectively prevents
the kexec_load syscall from succeeding when set.

Signed-off-by: Eric Richter <erichte@linux.vnet.ibm.com>
---
 arch/x86/kernel/Makefile       |  2 ++
 arch/x86/kernel/ima_arch.c     | 27 +++++++++++++++++++++++++++
 include/linux/ima.h            |  8 ++++++++
 security/integrity/ima/Kconfig |  8 ++++++++
 4 files changed, 45 insertions(+)
 create mode 100644 arch/x86/kernel/ima_arch.c

diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 02d6f5cf4e7..f3e1d76ed9b 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -149,3 +149,5 @@ ifeq ($(CONFIG_X86_64),y)
 	obj-$(CONFIG_MMCONF_FAM10H)	+= mmconf-fam10h_64.o
 	obj-y				+= vsmp_64.o
 endif
+
+obj-$(CONFIG_IMA_ARCH_POLICY) += ima_arch.o
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c
new file mode 100644
index 00000000000..5eb10e29db0
--- /dev/null
+++ b/arch/x86/kernel/ima_arch.c
@@ -0,0 +1,27 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2018 IBM Corporation
+ */
+#include <linux/efi.h>
+#include <linux/ima.h>
+
+extern struct boot_params boot_params;
+
+/* arch rules for audit and user mode */
+static const char * const sb_arch_rules[] = {
+#ifdef CONFIG_KEXEC_VERIFY_SIG
+	"appraise func=KEXEC_ORIG_KERNEL_CHECK appraise_type=imasig",
+#else
+	"appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig",
+#endif /* CONFIG_KEXEC_VERIFY_SIG */
+	"measure func=KEXEC_KERNEL_CHECK",
+	NULL
+};
+
+const char * const *arch_get_ima_policy(void)
+{
+	if (efi_enabled(EFI_BOOT) &&
+	    (boot_params.secure_boot == efi_secureboot_mode_enabled))
+		return sb_arch_rules;
+	return NULL;
+}
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 7fd272f0b1f..495fa290b14 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -30,10 +30,14 @@ extern void ima_post_path_mknod(struct dentry *dentry);
 extern void ima_add_kexec_buffer(struct kimage *image);
 #endif
 
+#if defined(CONFIG_IMA_ARCH_POLICY) && defined(CONFIG_X86)
+extern const char * const *arch_get_ima_policy(void);
+#else
 static inline const char * const *arch_get_ima_policy(void)
 {
 	return NULL;
 }
+#endif
 
 #else
 static inline int ima_bprm_check(struct linux_binprm *bprm)
@@ -77,6 +81,10 @@ static inline void ima_post_path_mknod(struct dentry *dentry)
 	return;
 }
 
+static inline const char * const *arch_get_ima_policy(void)
+{
+	return NULL;
+}
 #endif /* CONFIG_IMA */
 
 #ifndef CONFIG_IMA_KEXEC
diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 13b446328dd..18de132bbda 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -157,6 +157,14 @@ config IMA_APPRAISE
 	  <http://linux-ima.sourceforge.net>
 	  If unsure, say N.
 
+config IMA_ARCH_POLICY
+	bool "Enable loading an IMA architecture specific policy"
+	depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+	default n
+	help
+	  This option enables loading an IMA architecture specific policy
+	  based on run time secure boot flags.
+
 config IMA_APPRAISE_BUILD_POLICY
 	bool "IMA build time configured policy rules"
 	depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
-- 
2.14.4

Re: [PATCH 1/4] ima: add support for arch specific policies

[kbuild test robot]

Hi Nayna,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on integrity/next-integrity]
[also build test WARNING on next-20180727]
[cannot apply to v4.18-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eric-Richter/ima-add-support-for-arch-specific-policies/20180728-072442
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> security/integrity/ima/ima_policy.c:198:23: sparse: symbol 'arch_policy_rules' was not declared. Should it be static?
>> security/integrity/ima/ima_policy.c:199:23: sparse: symbol 'arch_policy_entry' was not declared. Should it be static?
   include/linux/slab.h:631:13: sparse: undefined identifier '__builtin_mul_overflow'
   include/linux/slab.h:631:13: sparse: not a function <noident>
   include/linux/slab.h:631:13: sparse: call with no type!

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[RFC PATCH] ima: arch_policy_rules can be static

[kbuild test robot]

Fixes: b4c0791e0fac ("ima: add support for arch specific policies")
Signed-off-by: kbuild test robot <fengguang.wu@intel.com>
---
 ima_policy.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index b47db4d..e1f2ffd 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -195,8 +195,8 @@ static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
 };
 
 /* An array of architecture specific rules */
-struct ima_rule_entry **arch_policy_rules __ro_after_init;
-struct ima_rule_entry *arch_policy_entry __ro_after_init;
+static struct ima_rule_entry **arch_policy_rules __ro_after_init;
+static struct ima_rule_entry *arch_policy_entry __ro_after_init;
 
 static LIST_HEAD(ima_default_rules);
 static LIST_HEAD(ima_policy_rules);

Re: [PATCH 4/4] x86/ima: define arch_get_ima_policy() for x86

[kbuild test robot]

Hi Eric,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on integrity/next-integrity]
[also build test WARNING on next-20180727]
[cannot apply to v4.18-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Eric-Richter/ima-add-support-for-arch-specific-policies/20180728-072442
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity

smatch warnings:
security/integrity/ima/ima_policy.c:522 ima_init_arch_policy() error: potential null dereference 'arch_policy_entry'.  (kcalloc returns null)

vim +/arch_policy_entry +522 security/integrity/ima/ima_policy.c

b4c0791e Nayna Jain 2018-07-25  484  
b4c0791e Nayna Jain 2018-07-25  485  /*
b4c0791e Nayna Jain 2018-07-25  486   * ima_init_arch_policy - convert arch policy strings to rules
b4c0791e Nayna Jain 2018-07-25  487   *
b4c0791e Nayna Jain 2018-07-25  488   * Return number of arch specific rules.
b4c0791e Nayna Jain 2018-07-25  489   */
b4c0791e Nayna Jain 2018-07-25  490  static int __init ima_init_arch_policy(void)
b4c0791e Nayna Jain 2018-07-25  491  {
b4c0791e Nayna Jain 2018-07-25  492  	const char * const *arch_rules;
b4c0791e Nayna Jain 2018-07-25  493  	const char * const *rules;
b4c0791e Nayna Jain 2018-07-25  494  	int arch_entries = 0;
b4c0791e Nayna Jain 2018-07-25  495  	int i = 0;
b4c0791e Nayna Jain 2018-07-25  496  
b4c0791e Nayna Jain 2018-07-25  497  	arch_rules = arch_get_ima_policy();
b4c0791e Nayna Jain 2018-07-25  498  	if (!arch_rules) {
b4c0791e Nayna Jain 2018-07-25  499  		pr_info("No architecture policy rules.\n");
b4c0791e Nayna Jain 2018-07-25  500  		return arch_entries;
b4c0791e Nayna Jain 2018-07-25  501  	}
b4c0791e Nayna Jain 2018-07-25  502  
b4c0791e Nayna Jain 2018-07-25  503  	/* Get number of rules */
b4c0791e Nayna Jain 2018-07-25  504  	for (rules = arch_rules; *rules != NULL; rules++)
b4c0791e Nayna Jain 2018-07-25  505  		arch_entries++;
b4c0791e Nayna Jain 2018-07-25  506  
b4c0791e Nayna Jain 2018-07-25  507  	arch_policy_rules = kcalloc(arch_entries + 1,
b4c0791e Nayna Jain 2018-07-25  508  				    sizeof(*arch_policy_rules), GFP_KERNEL);
b4c0791e Nayna Jain 2018-07-25  509  	if (!arch_policy_rules)
b4c0791e Nayna Jain 2018-07-25  510  		return 0;
b4c0791e Nayna Jain 2018-07-25  511  
b4c0791e Nayna Jain 2018-07-25  512  	arch_policy_entry = kcalloc(arch_entries + 1,
b4c0791e Nayna Jain 2018-07-25  513  				    sizeof(*arch_policy_entry), GFP_KERNEL);
b4c0791e Nayna Jain 2018-07-25  514  
b4c0791e Nayna Jain 2018-07-25  515  	/* Convert arch policy string rules to struct ima_rule_entry format */
b4c0791e Nayna Jain 2018-07-25  516  	for (rules = arch_rules, i = 0; *rules != NULL; rules++) {
b4c0791e Nayna Jain 2018-07-25  517  		char rule[255];
b4c0791e Nayna Jain 2018-07-25  518  		int result;
b4c0791e Nayna Jain 2018-07-25  519  
b4c0791e Nayna Jain 2018-07-25  520  		result = strlcpy(rule, *rules, sizeof(rule));
b4c0791e Nayna Jain 2018-07-25  521  
b4c0791e Nayna Jain 2018-07-25 @522  		INIT_LIST_HEAD(&arch_policy_entry[i].list);
b4c0791e Nayna Jain 2018-07-25  523  		result = ima_parse_rule(rule, &arch_policy_entry[i]);
b4c0791e Nayna Jain 2018-07-25  524  		if (result) {
b4c0791e Nayna Jain 2018-07-25  525  			pr_warn("Skipping unknown architecture policy rule: %s\n", rule);
b4c0791e Nayna Jain 2018-07-25  526  			memset(&arch_policy_entry[i], 0,
b4c0791e Nayna Jain 2018-07-25  527  			       sizeof(*arch_policy_entry));
b4c0791e Nayna Jain 2018-07-25  528  			continue;
b4c0791e Nayna Jain 2018-07-25  529  		}
b4c0791e Nayna Jain 2018-07-25  530  		arch_policy_rules[i] = &arch_policy_entry[i];
b4c0791e Nayna Jain 2018-07-25  531  		i++;
b4c0791e Nayna Jain 2018-07-25  532  	}
b4c0791e Nayna Jain 2018-07-25  533  	return i;
b4c0791e Nayna Jain 2018-07-25  534  }
b4c0791e Nayna Jain 2018-07-25  535  

:::::: The code at line 522 was first introduced by commit
:::::: b4c0791e0facd968a3e0502a8a544390025a9a38 ima: add support for arch specific policies

:::::: TO: Nayna Jain <nayna@linux.vnet.ibm.com>
:::::: CC: 0day robot <lkp@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Re: [PATCH 1/4] ima: add support for arch specific policies

[Nayna Jain]

On 07/28/2018 07:54 AM, kbuild test robot wrote:
> Hi Nayna,
>
> Thank you for the patch! Perhaps something to improve:

Thanks for the report. I will address or squash the changes with the 
existing PATCH 1/4 in the next version.
The warning about NULL dereference in PATCH 4/4 is related to changes in 
PATCH 1/4, so will fix in PATCH 1/4 itself.

Thanks & Regards,
     - Nayna


>
> [auto build test WARNING on integrity/next-integrity]
> [also build test WARNING on next-20180727]
> [cannot apply to v4.18-rc6]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url:    https://github.com/0day-ci/linux/commits/Eric-Richter/ima-add-support-for-arch-specific-policies/20180728-072442
> base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
> reproduce:
>          # apt-get install sparse
>          make ARCH=x86_64 allmodconfig
>          make C=1 CF=-D__CHECK_ENDIAN__
>
>
> sparse warnings: (new ones prefixed by >>)
>
>>> security/integrity/ima/ima_policy.c:198:23: sparse: symbol 'arch_policy_rules' was not declared. Should it be static?
>>> security/integrity/ima/ima_policy.c:199:23: sparse: symbol 'arch_policy_entry' was not declared. Should it be static?
>     include/linux/slab.h:631:13: sparse: undefined identifier '__builtin_mul_overflow'
>     include/linux/slab.h:631:13: sparse: not a function <noident>
>     include/linux/slab.h:631:13: sparse: call with no type!
>
> Please review and possibly fold the followup patch.
>
> ---
> 0-DAY kernel test infrastructure                Open Source Technology Center
> https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>

Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK

[Seth Forshee]

On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote:
> IMA can verify the signature of kernel images loaded with kexec_file_load,
> but can not verify images loaded with the regular kexec_load syscall.
> Therefore, the appraisal will automatically fail during kexec_load when an
> appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used
> to effectively disable the kexec_load syscall, while still allowing the
> kexec_file_load to operate so long as the target kernel image is signed.
> 
> However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is
> enabled and there is an appraise rule set, then the target kernel would
> have to be verifiable by both IMA and the architecture specific kernel
> verification procedure.
> 
> This patch adds a new func= for IMA appraisal specifically for the original
> kexec_load syscall. Therefore, the kexec_load syscall can be effectively
> disabled via IMA policy, leaving the kexec_file_load syscall able to do its
> own signature verification, and not require it to be signed via IMA. To
> retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is
> unchanged, and thus enables appraisal for both kexec syscalls.

This seems like a roundabout way to disallow the kexec_load syscall.
Wouldn't it make more sense to simply disallow kexec_load any time
CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that
option impotent? Or has that idea already been rejected?

Thanks,
Seth

Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK

[Mimi Zohar]

On Fri, 2018-08-03 at 08:11 -0500, Seth Forshee wrote:
> On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote:
> > IMA can verify the signature of kernel images loaded with kexec_file_load,
> > but can not verify images loaded with the regular kexec_load syscall.
> > Therefore, the appraisal will automatically fail during kexec_load when an
> > appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used
> > to effectively disable the kexec_load syscall, while still allowing the
> > kexec_file_load to operate so long as the target kernel image is signed.
> > 
> > However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is
> > enabled and there is an appraise rule set, then the target kernel would
> > have to be verifiable by both IMA and the architecture specific kernel
> > verification procedure.
> > 
> > This patch adds a new func= for IMA appraisal specifically for the original
> > kexec_load syscall. Therefore, the kexec_load syscall can be effectively
> > disabled via IMA policy, leaving the kexec_file_load syscall able to do its
> > own signature verification, and not require it to be signed via IMA. To
> > retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is
> > unchanged, and thus enables appraisal for both kexec syscalls.
> 
> This seems like a roundabout way to disallow the kexec_load syscall.
> Wouldn't it make more sense to simply disallow kexec_load any time
> CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that
> option impotent? Or has that idea already been rejected?

Agreed!  We can modify the "case LOADING_KEXEC_IMAGE" in
ima_load_data() to prevent the kexec_load based on
CONFIG_KEXEC_VERIFY_SIG.

The architecture specific policy would only include the IMA appraise
rule if CONFIG_KEXEC_VERIFY_SIG was not defined.

Mimi

Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK

[Seth Forshee]

On Fri, Aug 03, 2018 at 10:54:59AM -0400, Mimi Zohar wrote:
> On Fri, 2018-08-03 at 08:11 -0500, Seth Forshee wrote:
> > On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote:
> > > IMA can verify the signature of kernel images loaded with kexec_file_load,
> > > but can not verify images loaded with the regular kexec_load syscall.
> > > Therefore, the appraisal will automatically fail during kexec_load when an
> > > appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used
> > > to effectively disable the kexec_load syscall, while still allowing the
> > > kexec_file_load to operate so long as the target kernel image is signed.
> > > 
> > > However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is
> > > enabled and there is an appraise rule set, then the target kernel would
> > > have to be verifiable by both IMA and the architecture specific kernel
> > > verification procedure.
> > > 
> > > This patch adds a new func= for IMA appraisal specifically for the original
> > > kexec_load syscall. Therefore, the kexec_load syscall can be effectively
> > > disabled via IMA policy, leaving the kexec_file_load syscall able to do its
> > > own signature verification, and not require it to be signed via IMA. To
> > > retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is
> > > unchanged, and thus enables appraisal for both kexec syscalls.
> > 
> > This seems like a roundabout way to disallow the kexec_load syscall.
> > Wouldn't it make more sense to simply disallow kexec_load any time
> > CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that
> > option impotent? Or has that idea already been rejected?
> 
> Agreed!  We can modify the "case LOADING_KEXEC_IMAGE" in
> ima_load_data() to prevent the kexec_load based on
> CONFIG_KEXEC_VERIFY_SIG.
> 
> The architecture specific policy would only include the IMA appraise
> rule if CONFIG_KEXEC_VERIFY_SIG was not defined.

After looking at this some more I'm having second thoughts about my
suggestion. As a distro we produce a kernel that needs to be flexible
enough for a variety of scenarios, and if we completely close off the
ability to load an unsigned kernel for kexec that's almost certainly
going to end up breaking some use cases.

So I think it is necessary to make this a run-time decision rather than
a compile-time decision. The patch as provided does this based on
whether or not the kernel was booted under secure boot. That might be
reasonable, though I still find this mechanism kind of awkward. It seems
like ideally there would instead be some logic that would accept the
image if the KEXEC_VERIFY_SIG verification had passed, and otherwise
require IMA signature verification.

Thanks,
Seth

Re: [PATCH 3/4] ima: add support for KEXEC_ORIG_KERNEL_CHECK

[Mimi Zohar]

On Fri, 2018-08-03 at 11:16 -0500, Seth Forshee wrote:
> On Fri, Aug 03, 2018 at 10:54:59AM -0400, Mimi Zohar wrote:
> > On Fri, 2018-08-03 at 08:11 -0500, Seth Forshee wrote:
> > > On Wed, Jul 25, 2018 at 06:31:59PM -0500, Eric Richter wrote:
> > > > IMA can verify the signature of kernel images loaded with kexec_file_load,
> > > > but can not verify images loaded with the regular kexec_load syscall.
> > > > Therefore, the appraisal will automatically fail during kexec_load when an
> > > > appraise policy rule is set for func=KEXEC_KERNEL_CHECK. This can be used
> > > > to effectively disable the kexec_load syscall, while still allowing the
> > > > kexec_file_load to operate so long as the target kernel image is signed.
> > > > 
> > > > However, this conflicts with CONFIG_KEXEC_VERIFY_SIG. If that option is
> > > > enabled and there is an appraise rule set, then the target kernel would
> > > > have to be verifiable by both IMA and the architecture specific kernel
> > > > verification procedure.
> > > > 
> > > > This patch adds a new func= for IMA appraisal specifically for the original
> > > > kexec_load syscall. Therefore, the kexec_load syscall can be effectively
> > > > disabled via IMA policy, leaving the kexec_file_load syscall able to do its
> > > > own signature verification, and not require it to be signed via IMA. To
> > > > retain compatibility, the existing func=KEXEC_KERNEL_CHECK flag is
> > > > unchanged, and thus enables appraisal for both kexec syscalls.
> > > 
> > > This seems like a roundabout way to disallow the kexec_load syscall.
> > > Wouldn't it make more sense to simply disallow kexec_load any time
> > > CONFIG_KEXEC_VERIFY_SIG is enabled, since it effectively renders that
> > > option impotent? Or has that idea already been rejected?
> > 
> > Agreed!  We can modify the "case LOADING_KEXEC_IMAGE" in
> > ima_load_data() to prevent the kexec_load based on
> > CONFIG_KEXEC_VERIFY_SIG.
> > 
> > The architecture specific policy would only include the IMA appraise
> > rule if CONFIG_KEXEC_VERIFY_SIG was not defined.
> 
> After looking at this some more I'm having second thoughts about my
> suggestion. As a distro we produce a kernel that needs to be flexible
> enough for a variety of scenarios, and if we completely close off the
> ability to load an unsigned kernel for kexec that's almost certainly
> going to end up breaking some use cases.
> 
> So I think it is necessary to make this a run-time decision rather than
> a compile-time decision. The patch as provided does this based on
> whether or not the kernel was booted under secure boot. That might be
> reasonable, though I still find this mechanism kind of awkward.

Right, the above change is almost right.  Instead of preventing the
kexec_load syscall based on CONFIG_KEXEC_VERIFY_SIG it should be based
on a runtime secure boot flag.  Only if there is an arch specific
secure boot function and the secure boot flag is enabled, would the
kexec_load be disabled.

Without an architecture specific secure boot function, the existing
IMA code would fail the kexec_load syscall.

>  It seems
> like ideally there would instead be some logic that would accept the
> image if the KEXEC_VERIFY_SIG verification had passed, and otherwise
> require IMA signature verification.

True, but for now to coordinate between the two signature verification
methods, only if CONFIG_KEXEC_VERIFY_SIG is not enabled would an IMA
architecture specific rule be defined.

Mimi