[PATCH v2 0/9] kexec/firmware: support system wide policy requiring signatures

[PATCH v2 0/9] kexec/firmware: support system wide policy requiring signatures

[Mimi Zohar]

IMA-appraisal is mostly being used in the embedded or single purpose
closed system environments.  In these environments, both the Kconfig
options and the userspace tools can be modified appropriately to limit
syscalls.  For stock kernels, userspace applications need to continue to
work with older kernels as well as with newer kernels.

In this environment, the customer needs the ability to define a system
wide IMA policy, such as requiring all kexec'ed images, firmware, kernel
modules to be signed, without being dependent on either the Kconfig
options or the userspace tools.[1]

This patch set allows the customer to define a policy which requires
the kexec'ed kernel images, firmware, and/or kernel modules to be
signed.

New to this patch set is the ability to configure a build time IMA policy,
which is automatically loaded at run time without needing to specify it
on the boot command line.  The build time policy rules persist after
loading a custom kernel policy.

[1] kexec-tools suupports the new syscall based on a flag (-s).

Changelog v3:
- combined "kexec: limit kexec_load syscall" and "firmware: kernel
signature verification" patch sets.
- add support for build time policy.
- defined generic security_kernel_read_blob() wrapper for
  security_kernel_read_file(). Suggested by Luis.
- removed the CONFIG_CFG80211_REQUIRE_SIGNED_REGDB ifdef.  If both REGDB
  and an IMA-appraisal policy require signed firmware, for now require
  both signatures.  Subsequent patches might change this.
- Still unclear if the pre-allocated firmware buffer can be accessed
  prior to the signature verification completes.

Mimi Zohar (9):
  ima: based on policy verify firmware signatures (pre-allocated buffer)
  ima: fix updating the ima_appraise flag
  security: define security_kernel_read_blob() wrapper
  kexec: add call to LSM hook in original kexec_load syscall
  ima: based on policy require signed kexec kernel images
  firmware: add call to LSM hook before firmware sysfs fallback
  ima: based on policy require signed firmware (sysfs fallback)
  ima: add build time policy
  ima: based on policy prevent loading firmware (pre-allocated buffer)

 drivers/base/firmware_loader/fallback.c |  7 +++
 include/linux/fs.h                      |  1 +
 include/linux/security.h                |  6 +++
 kernel/kexec.c                          | 11 +++++
 security/integrity/ima/Kconfig          | 58 +++++++++++++++++++++++++
 security/integrity/ima/ima.h            |  1 +
 security/integrity/ima/ima_main.c       | 29 +++++++++++++
 security/integrity/ima/ima_policy.c     | 76 +++++++++++++++++++++++++++------
 security/security.c                     |  6 +++
 9 files changed, 183 insertions(+), 12 deletions(-)

-- 
2.7.5

[PATCH v2 1/9] ima: based on policy verify firmware signatures (pre-allocated buffer)

[Mimi Zohar]

Don't differentiate between kernel_read_file_id READING_FIRMWARE and
READING_FIRMWARE_PREALLOC_BUFFER enumerations.

Fixes: a098ecd firmware: support loading into a pre-allocated buffer (since 4.8)
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Stephen Boyd <stephen.boyd@linaro.org>
---
 security/integrity/ima/ima_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index e771b736aa03..83f84928ad76 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -447,6 +447,7 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 
 static int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
+	[READING_FIRMWARE_PREALLOC_BUFFER] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
 	[READING_KEXEC_IMAGE] = KEXEC_KERNEL_CHECK,
 	[READING_KEXEC_INITRAMFS] = KEXEC_INITRAMFS_CHECK,
-- 
2.7.5

[PATCH v2 2/9] ima: fix updating the ima_appraise flag

[Mimi Zohar]

As IMA policy rules are added, a mask of the type of rule (eg. kexec
kernel image, firmware, IMA policy) is updated.  Based on this mask,
integrity decisions can be made quickly.

Unlike custom IMA policy rules, which replace the original builtin
policy rules and update the mask, the builtin "secure_boot" policy
rules were loaded, but did not update the mask.

This patch refactors the code to load custom policies, defining a new
function named ima_appraise_flag().  The new function is called either
when loading the builtin "secure_boot" or custom policies.

Fixes: 503ceaef8e2e ("ima: define a set of appraisal rules requiring file signatures")
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/ima_policy.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 03cbba423e59..8bbc18eb07eb 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -440,6 +440,17 @@ void ima_update_policy_flag(void)
 		ima_policy_flag &= ~IMA_APPRAISE;
 }
 
+static int ima_appraise_flag(enum ima_hooks func)
+{
+	if (func == MODULE_CHECK)
+		return IMA_APPRAISE_MODULES;
+	else if (func == FIRMWARE_CHECK)
+		return IMA_APPRAISE_FIRMWARE;
+	else if (func == POLICY_CHECK)
+		return IMA_APPRAISE_POLICY;
+	return 0;
+}
+
 /**
  * ima_init_policy - initialize the default measure rules.
  *
@@ -478,9 +489,11 @@ void __init ima_init_policy(void)
 	 * Insert the appraise rules requiring file signatures, prior to
 	 * any other appraise rules.
 	 */
-	for (i = 0; i < secure_boot_entries; i++)
-		list_add_tail(&secure_boot_rules[i].list,
-			      &ima_default_rules);
+	for (i = 0; i < secure_boot_entries; i++) {
+		list_add_tail(&secure_boot_rules[i].list, &ima_default_rules);
+		temp_ima_appraise |=
+		    ima_appraise_flag(secure_boot_rules[i].func);
+	}
 
 	for (i = 0; i < appraise_entries; i++) {
 		list_add_tail(&default_appraise_rules[i].list,
@@ -934,12 +947,9 @@ static int ima_parse_rule(char *rule, struct ima_rule_entry *entry)
 	}
 	if (!result && (entry->action == UNKNOWN))
 		result = -EINVAL;
-	else if (entry->func == MODULE_CHECK)
-		temp_ima_appraise |= IMA_APPRAISE_MODULES;
-	else if (entry->func == FIRMWARE_CHECK)
-		temp_ima_appraise |= IMA_APPRAISE_FIRMWARE;
-	else if (entry->func == POLICY_CHECK)
-		temp_ima_appraise |= IMA_APPRAISE_POLICY;
+	else if (entry->action == APPRAISE)
+		temp_ima_appraise |= ima_appraise_flag(entry->func);
+
 	audit_log_format(ab, "res=%d", !result);
 	audit_log_end(ab);
 	return result;
-- 
2.7.5

[PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Mimi Zohar]

In order for LSMs and IMA-appraisal to differentiate between the original
and new syscalls (eg. kexec, kernel modules, firmware), both the original
and new syscalls must call an LSM hook.

Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
introduced calling security_kernel_module_from_file() in both the original
and new syscalls.  Commit a1db74209483 ("module: replace
copy_module_from_fd with kernel version") replaced these LSM calls with
security_kernel_read_file().

Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
with kernel version") and commit b804defe4297  ("kexec: replace call to
copy_file_from_fd() with kernel version") replaced their own version of
reading a file from the kernel with the generic
kernel_read_file_from_path/fd() versions, which call the pre and post
security_kernel_read_file LSM hooks.

Missing are LSM calls in the original kexec syscall and firmware sysfs
fallback method.  From a technical perspective there is no justification
for defining a new LSM hook, as the existing security_kernel_read_file()
works just fine.  The original syscalls, however, do not read a file, so
the security hook name is inappropriate.  Instead of defining a new LSM
hook, this patch defines security_kernel_read_blob() as a wrapper for
the existing LSM security_kernel_file_read() hook.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Howells <dhowells@redhat.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>

Changelog v2:
- Define a generic wrapper named security_kernel_read_blob() for
security_kernel_read_file().

Changelog v1:
- Define and call security_kexec_load(), a wrapper for
security_kernel_read_file().
---
 include/linux/security.h | 6 ++++++
 security/security.c      | 6 ++++++
 2 files changed, 12 insertions(+)

diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..4db1967a688b 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name);
 int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id);
+int security_kernel_read_blob(enum kernel_read_file_id id);
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
 			     int flags);
 int security_task_setpgid(struct task_struct *p, pid_t pgid);
@@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct file *file,
 	return 0;
 }
 
+static inline int security_kernel_read_blob(enum kernel_read_file_id id)
+{
+	return 0;
+}
+
 static inline int security_task_fix_setuid(struct cred *new,
 					   const struct cred *old,
 					   int flags)
diff --git a/security/security.c b/security/security.c
index 68f46d849abe..8f199b2bf4a2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
 }
 EXPORT_SYMBOL_GPL(security_kernel_read_file);
 
+int security_kernel_read_blob(enum kernel_read_file_id id)
+{
+	return security_kernel_read_file(NULL, id);
+}
+EXPORT_SYMBOL_GPL(security_kernel_read_blob);
+
 int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 				   enum kernel_read_file_id id)
 {
-- 
2.7.5

[PATCH v2 4/9] kexec: add call to LSM hook in original kexec_load syscall

[Mimi Zohar]

In order for LSMs and IMA-appraisal to differentiate between the
original and new syscalls, both the original and new syscalls must call
an LSM hook.  This patch adds a call to security_kernel_read_blob() in
the original kexec syscall.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Howells <dhowells@redhat.com>
---
 kernel/kexec.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..64cebe92a690 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -11,6 +11,7 @@
 #include <linux/capability.h>
 #include <linux/mm.h>
 #include <linux/file.h>
+#include <linux/security.h>
 #include <linux/kexec.h>
 #include <linux/mutex.h>
 #include <linux/list.h>
@@ -195,11 +196,21 @@ static int do_kexec_load(unsigned long entry, unsigned long nr_segments,
 static inline int kexec_load_check(unsigned long nr_segments,
 				   unsigned long flags)
 {
+	int result;
+
 	/* We only trust the superuser with rebooting the system. */
 	if (!capable(CAP_SYS_BOOT) || kexec_load_disabled)
 		return -EPERM;
 
 	/*
+	 * Allow LSMs and IMA to differentiate between kexec_load and
+	 * kexec_file_load syscalls.
+	 */
+	result = security_kernel_read_blob(READING_KEXEC_IMAGE);
+	if (result < 0)
+		return result;
+
+	/*
 	 * Verify we have a legal set of flags
 	 * This leaves us room for future extensions.
 	 */
-- 
2.7.5

[PATCH v2 5/9] ima: based on policy require signed kexec kernel images

[Mimi Zohar]

The original kexec_load syscall can not verify file signatures.  This
patch differentiates between the kexec_load and kexec_file_load
syscalls.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Eric Biederman <ebiederm@xmission.com>
Cc: Luis R. Rodriguez <mcgrof@kernel.org>
Cc: Kees Cook <keescook@chromium.org>
Cc: David Howells <dhowells@redhat.com>
---
 security/integrity/ima/ima.h        | 1 +
 security/integrity/ima/ima_main.c   | 9 +++++++++
 security/integrity/ima/ima_policy.c | 2 ++
 3 files changed, 12 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 354bb5716ce3..78c15264b17b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -232,6 +232,7 @@ int ima_policy_show(struct seq_file *m, void *v);
 #define IMA_APPRAISE_MODULES	0x08
 #define IMA_APPRAISE_FIRMWARE	0x10
 #define IMA_APPRAISE_POLICY	0x20
+#define IMA_APPRAISE_KEXEC	0x40
 
 #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 83f84928ad76..7e1a127f18fe 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -442,6 +442,15 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		}
 		return 0;	/* We rely on module signature checking */
 	}
+
+	if (!file && read_id == READING_KEXEC_IMAGE) {
+		if ((ima_appraise & IMA_APPRAISE_KEXEC) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file syscall.\n");
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
+		return 0;
+	}
 	return 0;
 }
 
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 8bbc18eb07eb..c27f6993b07a 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -448,6 +448,8 @@ static int ima_appraise_flag(enum ima_hooks func)
 		return IMA_APPRAISE_FIRMWARE;
 	else if (func == POLICY_CHECK)
 		return IMA_APPRAISE_POLICY;
+	else if (func == KEXEC_KERNEL_CHECK)
+		return IMA_APPRAISE_KEXEC;
 	return 0;
 }
 
-- 
2.7.5

[PATCH v2 6/9] firmware: add call to LSM hook before firmware sysfs fallback

[Mimi Zohar]

Add an LSM hook prior to allowing firmware sysfs fallback loading.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>

Changelog:
- call security_kernel_read_blob()
- rename the READING_FIRMWARE_FALLBACK kernel_read_file_id enumeration to
READING_FIRMWARE_FALLBACK_SYSFS.
---
 drivers/base/firmware_loader/fallback.c | 7 +++++++
 include/linux/fs.h                      | 1 +
 2 files changed, 8 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 358354148dec..6ec97e19c017 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -651,6 +651,8 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+	int ret;
+
 	if (fw_fallback_config.ignore_sysfs_fallback) {
 		pr_info_once("Ignoring firmware sysfs fallback due to sysctl knob\n");
 		return false;
@@ -659,6 +661,11 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 	if ((opt_flags & FW_OPT_NOFALLBACK))
 		return false;
 
+	/* Also permit LSMs and IMA to fail firmware sysfs fallback */
+	ret = security_kernel_read_blob(READING_FIRMWARE_FALLBACK_SYSFS);
+	if (ret < 0)
+		return ret;
+
 	return fw_force_sysfs_fallback(opt_flags);
 }
 
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 760d8da1b6c7..6e31d9207435 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2810,6 +2810,7 @@ extern int do_pipe_flags(int *, int);
 	id(UNKNOWN, unknown)		\
 	id(FIRMWARE, firmware)		\
 	id(FIRMWARE_PREALLOC_BUFFER, firmware)	\
+	id(FIRMWARE_FALLBACK_SYSFS, firmware)	\
 	id(MODULE, kernel-module)		\
 	id(KEXEC_IMAGE, kexec-image)		\
 	id(KEXEC_INITRAMFS, kexec-initramfs)	\
-- 
2.7.5

[PATCH v2 7/9] ima: based on policy require signed firmware (sysfs fallback)

[Mimi Zohar]

With an IMA policy requiring signed firmware, this patch prevents
the sysfs fallback method of loading firmware.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Matthew Garrett <mjg59@google.com>
---
 security/integrity/ima/ima_main.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 7e1a127f18fe..29d1a929af5c 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -451,7 +451,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		}
 		return 0;
 	}
+
+	if (read_id == READING_FIRMWARE_FALLBACK_SYSFS) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("Prevent firmware sysfs fallback loading.\n");
+			return -EACCES;
+		}
+		return 0;
+	}
 	return 0;
+
 }
 
 static int read_idmap[READING_MAX_ID] = {
-- 
2.7.5

[PATCH v2 8/9] ima: add build time policy

[Mimi Zohar]

IMA by default does not measure, appraise or audit files, but can be
enabled at runtime by specifying a builtin policy on the boot command line
or by loading a custom policy.

This patch defines a build time policy, which verifies kernel modules,
firmware, kexec image, and/or the IMA policy signatures.  This build time
policy is automatically enabled at runtime.  The build time policy rules
persist after loading a custom policy.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 security/integrity/ima/Kconfig      | 58 +++++++++++++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c | 46 +++++++++++++++++++++++++++--
 2 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/Kconfig b/security/integrity/ima/Kconfig
index 6a8f67714c83..004919d9bf09 100644
--- a/security/integrity/ima/Kconfig
+++ b/security/integrity/ima/Kconfig
@@ -156,6 +156,64 @@ config IMA_APPRAISE
 	  <http://linux-ima.sourceforge.net>
 	  If unsure, say N.
 
+config IMA_APPRAISE_BUILD_POLICY
+	bool "IMA build time configured policy rules"
+	depends on IMA_APPRAISE && INTEGRITY_ASYMMETRIC_KEYS
+	default n
+	help
+	  This option defines an IMA appraisal policy at build time, which
+	  is enforced at run time without having to specify a builtin
+	  policy name on the boot command line.  The build time appraisal
+	  policy rules persist after loading a custom policy.
+
+	  Depending on the rules configured, this policy may require kernel
+	  modules, firmware, the kexec kernel image, and/or the IMA policy
+	  to be signed.  Unsigned files might prevent the system from
+	  booting or applications from working properly.
+
+config IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+	bool "Appraise firmware signatures"
+	depends on IMA_APPRAISE_BUILD_POLICY
+	default n
+	help
+	  This option defines a policy requiring all firmware to be signed,
+	  including the regulatory.db.  If both this option and
+	  CFG80211_REQUIRE_SIGNED_REGDB are enabled, then both signature
+	  verification methods are necessary.
+
+config IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+	bool "Appraise kexec kernel image signatures"
+	depends on IMA_APPRAISE_BUILD_POLICY
+	default n
+	help
+	  Enabling this rule will require all kexec'ed kernel images to
+	  be signed and verified by a public key on the trusted IMA
+	  keyring.
+
+	  Kernel image signatures can not be verified by the original
+	  kexec_load syscall.  Enabling this rule will prevent its
+	  usage.
+
+config IMA_APPRAISE_REQUIRE_MODULE_SIGS
+	bool "Appraise kernel modules signatures"
+	depends on IMA_APPRAISE_BUILD_POLICY
+	default n
+	help
+	  Enabling this rule will require all kernel modules to be signed
+	  and verified by a public key on the trusted IMA keyring.
+
+	  Kernel module signatures can only be verified by IMA-appraisal,
+	  via the finit_module syscall. Enabling this rule will prevent
+	  the usage of the init_module syscall.
+
+config IMA_APPRAISE_REQUIRE_POLICY_SIGS
+	bool "Appraise IMA policy signature"
+	depends on IMA_APPRAISE_BUILD_POLICY
+	default n
+	help
+	  Enabling this rule will require the IMA policy to be signed and
+	  and verified by a key on the trusted IMA keyring.
+
 config IMA_APPRAISE_BOOTPARAM
 	bool "ima_appraise boot parameter"
 	depends on IMA_APPRAISE
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c27f6993b07a..3c0bc8a1a88e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -49,6 +49,7 @@
 
 int ima_policy_flag;
 static int temp_ima_appraise;
+static int build_ima_appraise __ro_after_init;
 
 #define MAX_LSM_RULES 6
 enum lsm_rule_types { LSM_OBJ_USER, LSM_OBJ_ROLE, LSM_OBJ_TYPE,
@@ -162,6 +163,25 @@ static struct ima_rule_entry default_appraise_rules[] __ro_after_init = {
 #endif
 };
 
+static struct ima_rule_entry build_appraise_rules[] __ro_after_init = {
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_MODULE_SIGS
+	{.action = APPRAISE, .func = MODULE_CHECK,
+	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_FIRMWARE_SIGS
+	{.action = APPRAISE, .func = FIRMWARE_CHECK,
+	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_KEXEC_SIGS
+	{.action = APPRAISE, .func = KEXEC_KERNEL_CHECK,
+	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+#ifdef CONFIG_IMA_APPRAISE_REQUIRE_POLICY_SIGS
+	{.action = APPRAISE, .func = POLICY_CHECK,
+	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
+#endif
+};
+
 static struct ima_rule_entry secure_boot_rules[] __ro_after_init = {
 	{.action = APPRAISE, .func = MODULE_CHECK,
 	 .flags = IMA_FUNC | IMA_DIGSIG_REQUIRED},
@@ -435,7 +455,7 @@ void ima_update_policy_flag(void)
 			ima_policy_flag |= entry->action;
 	}
 
-	ima_appraise |= temp_ima_appraise;
+	ima_appraise |= (build_ima_appraise | temp_ima_appraise);
 	if (!ima_appraise)
 		ima_policy_flag &= ~IMA_APPRAISE;
 }
@@ -488,8 +508,8 @@ void __init ima_init_policy(void)
 	}
 
 	/*
-	 * Insert the appraise rules requiring file signatures, prior to
-	 * any other appraise rules.
+	 * Insert the builtin "secure_boot" policy rules requiring file
+	 * signatures, prior to any other appraise rules.
 	 */
 	for (i = 0; i < secure_boot_entries; i++) {
 		list_add_tail(&secure_boot_rules[i].list, &ima_default_rules);
@@ -497,6 +517,26 @@ void __init ima_init_policy(void)
 		    ima_appraise_flag(secure_boot_rules[i].func);
 	}
 
+	/*
+	 * Insert the build time appraise rules requiring file signatures
+	 * for both the initial and custom policies, prior to other appraise
+	 * rules.
+	 */
+	for (i = 0; i < ARRAY_SIZE(build_appraise_rules); i++) {
+		struct ima_rule_entry *entry;
+
+		if (!secure_boot_entries)
+			list_add_tail(&build_appraise_rules[i].list,
+				      &ima_default_rules);
+
+		entry = kmemdup(&build_appraise_rules[i], sizeof(*entry),
+				GFP_KERNEL);
+		if (entry)
+			list_add_tail(&entry->list, &ima_policy_rules);
+		build_ima_appraise |=
+			ima_appraise_flag(build_appraise_rules[i].func);
+	}
+
 	for (i = 0; i < appraise_entries; i++) {
 		list_add_tail(&default_appraise_rules[i].list,
 			      &ima_default_rules);
-- 
2.7.5

[PATCH v2 9/9] ima: based on policy prevent loading firmware (pre-allocated buffer)

[Mimi Zohar]

Question: can the device access the pre-allocated buffer at any time?

By allowing devices to request firmware be loaded directly into a
pre-allocated buffer, will this allow the device access to the firmware
before the kernel has verified the firmware signature?

Is it dependent on the type of buffer allocated (eg. DMA)?  For example,
qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent().

With an IMA policy requiring signed firmware, this patch would prevent
loading firmware into a pre-allocated buffer.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: David Howells <dhowells@redhat.com>
Cc: Kees Cook <keescook@chromium.org>
Cc: Serge E. Hallyn <serge@hallyn.com>
Cc: Stephen Boyd <stephen.boyd@linaro.org>
---
 security/integrity/ima/ima_main.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 29d1a929af5c..6224468845e6 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -452,6 +452,15 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		return 0;
 	}
 
+	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("Prevent device from accessing firmware prior to verifying the firmware signature.\n");
+			return -EACCES;
+		}
+		return 0;
+	}
+
 	if (read_id == READING_FIRMWARE_FALLBACK_SYSFS) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
-- 
2.7.5

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Casey Schaufler]

On 5/17/2018 7:48 AM, Mimi Zohar wrote:
> In order for LSMs and IMA-appraisal to differentiate between the original
> and new syscalls (eg. kexec, kernel modules, firmware), both the original
> and new syscalls must call an LSM hook.
>
> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
> introduced calling security_kernel_module_from_file() in both the original
> and new syscalls.  Commit a1db74209483 ("module: replace
> copy_module_from_fd with kernel version") replaced these LSM calls with
> security_kernel_read_file().
>
> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
> with kernel version") and commit b804defe4297  ("kexec: replace call to
> copy_file_from_fd() with kernel version") replaced their own version of
> reading a file from the kernel with the generic
> kernel_read_file_from_path/fd() versions, which call the pre and post
> security_kernel_read_file LSM hooks.
>
> Missing are LSM calls in the original kexec syscall and firmware sysfs
> fallback method.  From a technical perspective there is no justification
> for defining a new LSM hook, as the existing security_kernel_read_file()
> works just fine.  The original syscalls, however, do not read a file, so
> the security hook name is inappropriate.  Instead of defining a new LSM
> hook, this patch defines security_kernel_read_blob() as a wrapper for
> the existing LSM security_kernel_file_read() hook.

What a marvelous opportunity to bikeshed!

I really dislike adding another security_ interface just because
the name isn't quite right. Especially a wrapper, which is just
code and execution overhead. Why not change security_kernel_read_file()
to security_kernel_read_blob() everywhere and be done?

>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Eric Biederman <ebiederm@xmission.com>
> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: David Howells <dhowells@redhat.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
>
> Changelog v2:
> - Define a generic wrapper named security_kernel_read_blob() for
> security_kernel_read_file().
>
> Changelog v1:
> - Define and call security_kexec_load(), a wrapper for
> security_kernel_read_file().
> ---
>  include/linux/security.h | 6 ++++++
>  security/security.c      | 6 ++++++
>  2 files changed, 12 insertions(+)
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 63030c85ee19..4db1967a688b 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -323,6 +323,7 @@ int security_kernel_module_request(char *kmod_name);
>  int security_kernel_read_file(struct file *file, enum kernel_read_file_id id);
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  				   enum kernel_read_file_id id);
> +int security_kernel_read_blob(enum kernel_read_file_id id);
>  int security_task_fix_setuid(struct cred *new, const struct cred *old,
>  			     int flags);
>  int security_task_setpgid(struct task_struct *p, pid_t pgid);
> @@ -922,6 +923,11 @@ static inline int security_kernel_post_read_file(struct file *file,
>  	return 0;
>  }
>  
> +static inline int security_kernel_read_blob(enum kernel_read_file_id id)
> +{
> +	return 0;
> +}
> +
>  static inline int security_task_fix_setuid(struct cred *new,
>  					   const struct cred *old,
>  					   int flags)
> diff --git a/security/security.c b/security/security.c
> index 68f46d849abe..8f199b2bf4a2 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1044,6 +1044,12 @@ int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
>  }
>  EXPORT_SYMBOL_GPL(security_kernel_read_file);
>  
> +int security_kernel_read_blob(enum kernel_read_file_id id)
> +{
> +	return security_kernel_read_file(NULL, id);
> +}
> +EXPORT_SYMBOL_GPL(security_kernel_read_blob);
> +
>  int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  				   enum kernel_read_file_id id)
>  {

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Eric W. Biederman]

Casey Schaufler <casey@schaufler-ca.com> writes:

> On 5/17/2018 7:48 AM, Mimi Zohar wrote:
>> In order for LSMs and IMA-appraisal to differentiate between the original
>> and new syscalls (eg. kexec, kernel modules, firmware), both the original
>> and new syscalls must call an LSM hook.
>>
>> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
>> introduced calling security_kernel_module_from_file() in both the original
>> and new syscalls.  Commit a1db74209483 ("module: replace
>> copy_module_from_fd with kernel version") replaced these LSM calls with
>> security_kernel_read_file().
>>
>> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
>> with kernel version") and commit b804defe4297  ("kexec: replace call to
>> copy_file_from_fd() with kernel version") replaced their own version of
>> reading a file from the kernel with the generic
>> kernel_read_file_from_path/fd() versions, which call the pre and post
>> security_kernel_read_file LSM hooks.
>>
>> Missing are LSM calls in the original kexec syscall and firmware sysfs
>> fallback method.  From a technical perspective there is no justification
>> for defining a new LSM hook, as the existing security_kernel_read_file()
>> works just fine.  The original syscalls, however, do not read a file, so
>> the security hook name is inappropriate.  Instead of defining a new LSM
>> hook, this patch defines security_kernel_read_blob() as a wrapper for
>> the existing LSM security_kernel_file_read() hook.
>
> What a marvelous opportunity to bikeshed!
>
> I really dislike adding another security_ interface just because
> the name isn't quite right. Especially a wrapper, which is just
> code and execution overhead. Why not change security_kernel_read_file()
> to security_kernel_read_blob() everywhere and be done?

Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>

Nack on this sharing nonsense.  These two interfaces do not share any
code in their implementations other than the if statement to distinguish
between the two cases.

Casey you are wrong.  We need something different here.

Mimi a wrapper does not cut it.   The code is not shared.  Despite using
a single function call today.

If we want comprehensible and maintainable code in the security modules
we need to split these two pieces of functionality apart.

Eric

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Mimi Zohar]

On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote:
> Casey Schaufler <casey@schaufler-ca.com> writes:
> 
> > On 5/17/2018 7:48 AM, Mimi Zohar wrote:
> >> In order for LSMs and IMA-appraisal to differentiate between the original
> >> and new syscalls (eg. kexec, kernel modules, firmware), both the original
> >> and new syscalls must call an LSM hook.
> >>
> >> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
> >> introduced calling security_kernel_module_from_file() in both the original
> >> and new syscalls.  Commit a1db74209483 ("module: replace
> >> copy_module_from_fd with kernel version") replaced these LSM calls with
> >> security_kernel_read_file().
> >>
> >> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
> >> with kernel version") and commit b804defe4297  ("kexec: replace call to
> >> copy_file_from_fd() with kernel version") replaced their own version of
> >> reading a file from the kernel with the generic
> >> kernel_read_file_from_path/fd() versions, which call the pre and post
> >> security_kernel_read_file LSM hooks.
> >>
> >> Missing are LSM calls in the original kexec syscall and firmware sysfs
> >> fallback method.  From a technical perspective there is no justification
> >> for defining a new LSM hook, as the existing security_kernel_read_file()
> >> works just fine.  The original syscalls, however, do not read a file, so
> >> the security hook name is inappropriate.  Instead of defining a new LSM
> >> hook, this patch defines security_kernel_read_blob() as a wrapper for
> >> the existing LSM security_kernel_file_read() hook.
> >
> > What a marvelous opportunity to bikeshed!
> >
> > I really dislike adding another security_ interface just because
> > the name isn't quite right. Especially a wrapper, which is just
> > code and execution overhead. Why not change security_kernel_read_file()
> > to security_kernel_read_blob() everywhere and be done?
> 
> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Nack on this sharing nonsense.  These two interfaces do not share any
> code in their implementations other than the if statement to distinguish
> between the two cases.
> 
> Casey you are wrong.  We need something different here.
> 
> Mimi a wrapper does not cut it.   The code is not shared.  Despite using
> a single function call today.
> 
> If we want comprehensible and maintainable code in the security modules
> we need to split these two pieces of functionality apart.

kernel_read_file() is a common, generic method of reading a file from
the kernel, which is being called from a number of places.  The
kernel_read_file_id enumeration is needed to differentiate between the
callers.  The purpose of the new security_kernel_read_file() calls is
not for the kernel to read a file, but as a method of identifying the
original buffer based methods containing a file.

Having to define a separate LSM hook for each of the original, non
kernel_read_file(), buffer based method callers, kind of makes sense,
as the callers themselves are specific, but is it really necessary?
Could we define a new, generic LSM hook named
security_kernel_buffer_data() for this purpose?

Mimi

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Casey Schaufler]

On 5/18/2018 4:30 AM, Mimi Zohar wrote:
> On Thu, 2018-05-17 at 22:37 -0500, Eric W. Biederman wrote:
>> Casey Schaufler <casey@schaufler-ca.com> writes:
>>
>>> On 5/17/2018 7:48 AM, Mimi Zohar wrote:
>>>> In order for LSMs and IMA-appraisal to differentiate between the original
>>>> and new syscalls (eg. kexec, kernel modules, firmware), both the original
>>>> and new syscalls must call an LSM hook.
>>>>
>>>> Commit 2e72d51b4ac3 ("security: introduce kernel_module_from_file hook")
>>>> introduced calling security_kernel_module_from_file() in both the original
>>>> and new syscalls.  Commit a1db74209483 ("module: replace
>>>> copy_module_from_fd with kernel version") replaced these LSM calls with
>>>> security_kernel_read_file().
>>>>
>>>> Commit e40ba6d56b41 ("firmware: replace call to fw_read_file_contents()
>>>> with kernel version") and commit b804defe4297  ("kexec: replace call to
>>>> copy_file_from_fd() with kernel version") replaced their own version of
>>>> reading a file from the kernel with the generic
>>>> kernel_read_file_from_path/fd() versions, which call the pre and post
>>>> security_kernel_read_file LSM hooks.
>>>>
>>>> Missing are LSM calls in the original kexec syscall and firmware sysfs
>>>> fallback method.  From a technical perspective there is no justification
>>>> for defining a new LSM hook, as the existing security_kernel_read_file()
>>>> works just fine.  The original syscalls, however, do not read a file, so
>>>> the security hook name is inappropriate.  Instead of defining a new LSM
>>>> hook, this patch defines security_kernel_read_blob() as a wrapper for
>>>> the existing LSM security_kernel_file_read() hook.
>>> What a marvelous opportunity to bikeshed!
>>>
>>> I really dislike adding another security_ interface just because
>>> the name isn't quite right. Especially a wrapper, which is just
>>> code and execution overhead. Why not change security_kernel_read_file()
>>> to security_kernel_read_blob() everywhere and be done?
>> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> Nack on this sharing nonsense.  These two interfaces do not share any
>> code in their implementations other than the if statement to distinguish
>> between the two cases.
>>
>> Casey you are wrong.  We need something different here.
>>
>> Mimi a wrapper does not cut it.   The code is not shared.  Despite using
>> a single function call today.
>>
>> If we want comprehensible and maintainable code in the security modules
>> we need to split these two pieces of functionality apart.
> kernel_read_file() is a common, generic method of reading a file from
> the kernel, which is being called from a number of places.  The
> kernel_read_file_id enumeration is needed to differentiate between the
> callers.  The purpose of the new security_kernel_read_file() calls is
> not for the kernel to read a file, but as a method of identifying the
> original buffer based methods containing a file.
>
> Having to define a separate LSM hook for each of the original, non
> kernel_read_file(), buffer based method callers, kind of makes sense,
> as the callers themselves are specific, but is it really necessary?
> Could we define a new, generic LSM hook named
> security_kernel_buffer_data() for this purpose?

If there are two disparate behaviors wrapped into kernel_read_file()
Eric (bless him) is right. It should be broken into two hooks. I think
that if we look (too) carefully we'll find other places where hooks
should get broken up, or combined*. My question is just how important
is it that this gets changed?

---
* calling security_inode_secid() and then immediately
  security_secid_to_secctx() grates on my sensibilities.

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Mimi Zohar]

On Fri, 2018-05-18 at 07:58 -0700, Casey Schaufler wrote:
> On 5/18/2018 4:30 AM, Mimi Zohar wrote:

> > Having to define a separate LSM hook for each of the original, non
> > kernel_read_file(), buffer based method callers, kind of makes sense,
> > as the callers themselves are specific, but is it really necessary?
> > Could we define a new, generic LSM hook named
> > security_kernel_buffer_data() for this purpose?
> 
> If there are two disparate behaviors wrapped into kernel_read_file()
> Eric (bless him) is right. It should be broken into two hooks. I think
> that if we look (too) carefully we'll find other places where hooks
> should get broken up, or combined*. My question is just how important
> is it that this gets changed?

Other than the LSM call in copy_module_from_user(), this patch set is
adding the LSM call in kexec_load() and firmware_fallback_sysfs().

Eric, the question remains whether we need distinct LSM hooks in each
of these places or can we have a single, generic LSM hook named
security_kernel_buffer_data()?

Mimi

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[James Morris]

On Thu, 17 May 2018, Eric W. Biederman wrote:

> Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> 
> Nack on this sharing nonsense.  These two interfaces do not share any
> code in their implementations other than the if statement to distinguish
> between the two cases.

Hmm, it's not even doing that.

There's already an if(!file && read_id == X) { } check and this is another 
one being added.

> If we want comprehensible and maintainable code in the security modules
> we need to split these two pieces of functionality apart.

All ima_read is doing in both the old and new case is checking if there's 
no file then if it's a certain operation, returning an error.

To echo Eric and Casey's suggestions, how about changing the name of the 
hook to security_kernel_read_data() ?

Then ima_read_file() can be changed to ima_read_data(), and then instead 
of two if (!file && read_id == X) checks, have:

	if (!file) {
		switch (read_id) {
		}
	}




-- 
James Morris
<jmorris@namei.org>

Re: [PATCH v2 3/9] security: define security_kernel_read_blob() wrapper

[Mimi Zohar]

On Sat, 2018-05-19 at 03:13 +1000, James Morris wrote:
> On Thu, 17 May 2018, Eric W. Biederman wrote:
> 
> > Nacked-by: "Eric W. Biederman" <ebiederm@xmission.com>
> > 
> > Nack on this sharing nonsense.  These two interfaces do not share any
> > code in their implementations other than the if statement to distinguish
> > between the two cases.
> 
> Hmm, it's not even doing that.
> 
> There's already an if(!file && read_id == X) { } check and this is another 
> one being added.
> 
> > If we want comprehensible and maintainable code in the security modules
> > we need to split these two pieces of functionality apart.
> 
> All ima_read is doing in both the old and new case is checking if there's 
> no file then if it's a certain operation, returning an error.
> 
> To echo Eric and Casey's suggestions, how about changing the name of the 
> hook to security_kernel_read_data() ?

Thanks, James.  Somehow I missed this option.  Renaming the existing
hook, would be the easiest solution.  Eric, are you in agreement with
James' naming suggestion/solution?

> Then ima_read_file() can be changed to ima_read_data(), and then instead 
> of two if (!file && read_id == X) checks, have:
> 
> 	if (!file) {
> 		switch (read_id) {
> 		}
> 	}
> 
> 
> 
>