LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
@ 2018-05-29 18:01 Mimi Zohar
  2018-05-29 18:01 ` [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data Mimi Zohar
                   ` (8 more replies)
  0 siblings, 9 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel

Instead of adding the security_kernel_read_file LSM hook - or defining a
wrapper for security_kernel_read_file LSM hook and adding it, or
renaming the existing hook to security_kernel_read_data() and adding it
- in places where the kernel isn't reading a file, this version of the
patch set defines a new LSM hook named security_kernel_load_data().

The new LSM hook does not replace the existing security_kernel_read_file
LSM hook, which is still needed, but defines a new LSM hook allowing
LSMs and IMA-appraisal the opportunity to fail loading userspace
provided file/data.

The only difference between the two LSM hooks is the LSM hook name and a
file descriptor.  Whether this is cause enough for requiring a new LSM
hook, is left to the security community.

---

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.

In addition, this patch set includes 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 and persists after
loading a custom kernel policy.

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

Changelog v4:
- Define a new LSM hook named security_kernel_load_data().
- Define kernel_load_data_id enumeration.
- Replace the existing LSM hook in init_module syscall.

Changelog v3:
Based on James' feedback:
- Renamed security_kernel_read_file() to security_kernel_read_data().
- Defined new kernel_load_data_id enumeration.
- Cleaned up ima_read_data(), replacing if's with switch.

Changelog v2:
- 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.

Mimi Zohar (8):
  security: define new LSM hook named security_kernel_load_data
  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)
  module: replace the existing LSM hook in init_module

 drivers/base/firmware_loader/fallback.c |  7 +++
 include/linux/ima.h                     |  7 +++
 include/linux/lsm_hooks.h               |  6 +++
 include/linux/security.h                | 33 ++++++++++++++
 kernel/kexec.c                          |  8 ++++
 kernel/module.c                         |  2 +-
 security/integrity/ima/Kconfig          | 58 ++++++++++++++++++++++++
 security/integrity/ima/ima.h            |  1 +
 security/integrity/ima/ima_main.c       | 79 ++++++++++++++++++++++++---------
 security/integrity/ima/ima_policy.c     | 48 ++++++++++++++++++--
 security/security.c                     | 10 +++++
 security/selinux/hooks.c                | 26 ++++++++---
 12 files changed, 255 insertions(+), 30 deletions(-)
-- 
2.7.5


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

* [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-06-04 19:59   ` Serge E. Hallyn
  2018-05-29 18:01 ` [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall Mimi Zohar
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook, Casey Schaufler

Differentiate between the kernel reading a file from the kernel loading
data provided by userspace.  This patch defines a new LSM hook named
security_kernel_load_data.

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 v4:
- Define new LSM hook named security_kernel_load_data.

Changelog v3:
- Rename security_kernel_read_file to security_kernel_read_data().

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/lsm_hooks.h |  6 ++++++
 include/linux/security.h  | 33 +++++++++++++++++++++++++++++++++
 security/security.c       |  5 +++++
 3 files changed, 44 insertions(+)

diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 8f1131c8dd54..a08bc2587b96 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -576,6 +576,10 @@
  *	userspace to load a kernel module with the given name.
  *	@kmod_name name of the module requested by the kernel
  *	Return 0 if successful.
+ * @kernel_load_data:
+ *	Load data provided by userspace.
+ *	@id kernel load data identifier
+ *	Return 0 if permission is granted.
  * @kernel_read_file:
  *	Read a file specified by userspace.
  *	@file contains the file structure pointing to the file being read
@@ -1582,6 +1586,7 @@ union security_list_options {
 	int (*kernel_act_as)(struct cred *new, u32 secid);
 	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
 	int (*kernel_module_request)(char *kmod_name);
+	int (*kernel_load_data)(enum kernel_load_data_id id);
 	int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
 	int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
 				     enum kernel_read_file_id id);
@@ -1872,6 +1877,7 @@ struct security_hook_heads {
 	struct hlist_head cred_getsecid;
 	struct hlist_head kernel_act_as;
 	struct hlist_head kernel_create_files_as;
+	struct hlist_head kernel_load_data;
 	struct hlist_head kernel_read_file;
 	struct hlist_head kernel_post_read_file;
 	struct hlist_head kernel_module_request;
diff --git a/include/linux/security.h b/include/linux/security.h
index 63030c85ee19..3d54d5945755 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -159,6 +159,33 @@ extern int mmap_min_addr_handler(struct ctl_table *table, int write,
 typedef int (*initxattrs) (struct inode *inode,
 			   const struct xattr *xattr_array, void *fs_data);
 
+
+#define __kernel_load_data_id(id) \
+	id(UNKNOWN, unknown)		\
+	id(FIRMWARE, firmware)		\
+	id(MODULE, kernel-module)		\
+	id(KEXEC_IMAGE, kexec-image)		\
+	id(MAX_ID, )
+
+#define __data_id_enumify(ENUM, dummy) LOADING_ ## ENUM,
+#define __data_id_stringify(dummy, str) #str,
+
+enum kernel_load_data_id {
+	__kernel_load_data_id(__data_id_enumify)
+};
+
+static const char * const kernel_load_data_str[] = {
+	__kernel_load_data_id(__data_id_stringify)
+};
+
+static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
+{
+	if ((unsigned)id >= LOADING_MAX_ID)
+		return kernel_load_data_str[LOADING_UNKNOWN];
+
+	return kernel_load_data_str[id];
+}
+
 #ifdef CONFIG_SECURITY
 
 struct security_mnt_opts {
@@ -320,6 +347,7 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
 int security_kernel_act_as(struct cred *new, u32 secid);
 int security_kernel_create_files_as(struct cred *new, struct inode *inode);
 int security_kernel_module_request(char *kmod_name);
+int security_kernel_load_data(enum kernel_load_data_id id);
 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);
@@ -909,6 +937,11 @@ static inline int security_kernel_module_request(char *kmod_name)
 	return 0;
 }
 
+static inline int security_kernel_load_data(enum kernel_load_data_id id)
+{
+	return 0;
+}
+
 static inline int security_kernel_read_file(struct file *file,
 					    enum kernel_read_file_id id)
 {
diff --git a/security/security.c b/security/security.c
index 68f46d849abe..c2de2f134854 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1056,6 +1056,11 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
 }
 EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
 
+int security_kernel_load_data(enum kernel_load_data_id id)
+{
+	return call_int_hook(kernel_load_data, 0, id);
+}
+
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
 			     int flags)
 {
-- 
2.7.5

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

* [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
  2018-05-29 18:01 ` [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-06-04 20:00   ` Serge E. Hallyn
  2018-05-29 18:01 ` [PATCH v4 3/8] ima: based on policy require signed kexec kernel images Mimi Zohar
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook

In order for LSMs and IMA-appraisal to differentiate between kexec_load
and kexec_file_load syscalls, both the original and new syscalls must
call an LSM hook.  This patch adds a call to security_kernel_load_data()
in the original kexec_load 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 | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/kernel/kexec.c b/kernel/kexec.c
index aed8fb2564b3..68559808fdfa 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,10 +196,17 @@ 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;
 
+	/* Permit LSMs and IMA to fail the kexec */
+	result = security_kernel_load_data(LOADING_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

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

* [PATCH v4 3/8] ima: based on policy require signed kexec kernel images
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
  2018-05-29 18:01 ` [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data Mimi Zohar
  2018-05-29 18:01 ` [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-05-29 18:01 ` [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback Mimi Zohar
                   ` (5 subsequent siblings)
  8 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook

The original kexec_load syscall can not verify file signatures, nor can
the kexec image be measured.  Based on policy, deny the kexec_load
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>

Changelog v3:
- use switch/case
---
 include/linux/ima.h                 |  7 +++++++
 security/integrity/ima/ima.h        |  1 +
 security/integrity/ima/ima_main.c   | 27 +++++++++++++++++++++++++++
 security/integrity/ima/ima_policy.c |  2 ++
 security/security.c                 |  7 ++++++-
 5 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..84806b54b50a 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -11,6 +11,7 @@
 #define _LINUX_IMA_H
 
 #include <linux/fs.h>
+#include <linux/security.h>
 #include <linux/kexec.h>
 struct linux_binprm;
 
@@ -19,6 +20,7 @@ extern int ima_bprm_check(struct linux_binprm *bprm);
 extern int ima_file_check(struct file *file, int mask, int opened);
 extern void ima_file_free(struct file *file);
 extern int ima_file_mmap(struct file *file, unsigned long prot);
+extern int ima_load_data(enum kernel_load_data_id id);
 extern int ima_read_file(struct file *file, enum kernel_read_file_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
@@ -49,6 +51,11 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
 	return 0;
 }
 
+static inline int ima_load_data(enum kernel_load_data_id id)
+{
+	return 0;
+}
+
 static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
 {
 	return 0;
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..a565d46084c2 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -499,6 +499,33 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 				   MAY_READ, func, 0);
 }
 
+/**
+ * ima_load_data - appraise decision based on policy
+ * @id: kernel load data caller identifier
+ *
+ * Callers of this LSM hook can not measure, appraise, or audit the
+ * data provided by userspace.  Enforce policy rules requring a file
+ * signature (eg. kexec'ed kernel image).
+ *
+ * For permission return 0, otherwise return -EACCES.
+ */
+int ima_load_data(enum kernel_load_data_id id)
+{
+	if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
+		return 0;
+
+	switch (id) {
+	case LOADING_KEXEC_IMAGE:
+		if (ima_appraise & 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 */
+		}
+	default:
+		break;
+	}
+	return 0;
+}
+
 static int __init init_ima(void)
 {
 	int error;
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;
 }
 
diff --git a/security/security.c b/security/security.c
index c2de2f134854..4927e7cc7d96 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1058,7 +1058,12 @@ EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
 
 int security_kernel_load_data(enum kernel_load_data_id id)
 {
-	return call_int_hook(kernel_load_data, 0, id);
+	int ret;
+
+	ret = call_int_hook(kernel_load_data, 0, id);
+	if (ret)
+		return ret;
+	return ima_load_data(id);
 }
 
 int security_task_fix_setuid(struct cred *new, const struct cred *old,
-- 
2.7.5


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

* [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (2 preceding siblings ...)
  2018-05-29 18:01 ` [PATCH v4 3/8] ima: based on policy require signed kexec kernel images Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-06-01 18:19   ` Luis R. Rodriguez
  2018-05-29 18:01 ` [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback) Mimi Zohar
                   ` (4 subsequent siblings)
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Luis R . Rodriguez,
	Kees Cook

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 v4:
- call new LSM security_kernel_arg hook

Changelog v2:
- 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 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 358354148dec..2443bda81631 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_load_data(LOADING_FIRMWARE);
+	if (ret < 0)
+		return ret;
+
 	return fw_force_sysfs_fallback(opt_flags);
 }
 
-- 
2.7.5


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

* [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (3 preceding siblings ...)
  2018-05-29 18:01 ` [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-06-01 18:21   ` Luis R. Rodriguez
  2018-05-29 18:01 ` [PATCH v4 6/8] ima: add build time policy Mimi Zohar
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Luis R . Rodriguez,
	Matthew Garrett

Luis, is the security_kernel_post_read_file LSM hook in
firmware_loading_store() still needed after this patch?  Should it be
calling security_kernel_load_data() instead?

---

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, 9 insertions(+), 1 deletion(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index a565d46084c2..4a87f78098c8 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -475,8 +475,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 
 	if (!file && read_id == READING_FIRMWARE) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE))
+		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
+			pr_err("Prevent firmware loading_store.\n");
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
 		return 0;
 	}
 
@@ -520,6 +522,12 @@ int ima_load_data(enum kernel_load_data_id id)
 			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
 		}
+		break;
+	case LOADING_FIRMWARE:
+		if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
+			pr_err("Prevent firmware sysfs fallback loading.\n");
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
 	default:
 		break;
 	}
-- 
2.7.5


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

* [PATCH v4 6/8] ima: add build time policy
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (4 preceding siblings ...)
  2018-05-29 18:01 ` [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback) Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-05-29 18:01 ` [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer) Mimi Zohar
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel

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 and persists 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


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

* [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (5 preceding siblings ...)
  2018-05-29 18:01 ` [PATCH v4 6/8] ima: add build time policy Mimi Zohar
@ 2018-05-29 18:01 ` Mimi Zohar
  2018-06-01 19:15   ` Luis R. Rodriguez
  2018-05-29 18:02 ` [PATCH v4 8/8] module: replace the existing LSM hook in init_module Mimi Zohar
  2018-06-04 14:03 ` [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:01 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Luis R . Rodriguez,
	Kees Cook, Serge E . Hallyn, Stephen Boyd

Some systems are memory constrained but they need to load very large
firmwares.  The firmware subsystem allows drivers to request this
firmware be loaded from the filesystem, but this requires that the
entire firmware be loaded into kernel memory first before it's provided
to the driver.  This can lead to a situation where we map the firmware
twice, once to load the firmware into kernel memory and once to copy the
firmware into the final resting place.

To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
into a pre-allocated buffer") introduced request_firmware_into_buf() API
that allows drivers to request firmware be loaded directly into a
pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
allocate this buffer.  According to Documentation/DMA-API.txt,

     Consistent memory is memory for which a write by either the
     device or the processor can immediately be read by the processor
     or device without having to worry about caching effects.  (You
     may however need to make sure to flush the processor's write
     buffers before telling devices to read that memory.)

Devices using pre-allocated DMA memory run the risk of the firmware
being accessible by the device prior to the kernel's firmware signature
verification has completed.

Loading firmware already calls the security_kernel_read_file LSM hook.
With an IMA policy requiring signed firmware, this patch prevents
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 <sboyd@kernel.org>
---
 security/integrity/ima/ima_main.c | 26 +++++++++++++++++---------
 1 file changed, 17 insertions(+), 9 deletions(-)

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 4a87f78098c8..3dae605a1604 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -419,6 +419,15 @@ void ima_post_path_mknod(struct dentry *dentry)
 		iint->flags |= IMA_NEW_FILE;
 }
 
+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,
+	[READING_POLICY] = POLICY_CHECK
+};
+
 /**
  * ima_read_file - pre-measure/appraise hook decision based on policy
  * @file: pointer to the file to be measured/appraised/audit
@@ -442,18 +451,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 		}
 		return 0;	/* We rely on module signature checking */
 	}
+
+	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;
 }
 
-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,
-	[READING_POLICY] = POLICY_CHECK
-};
-
 /**
  * ima_post_read_file - in memory collect/appraise/audit measurement
  * @file: pointer to the file to be measured/appraised/audit
-- 
2.7.5

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

* [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (6 preceding siblings ...)
  2018-05-29 18:01 ` [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer) Mimi Zohar
@ 2018-05-29 18:02 ` Mimi Zohar
  2018-05-29 22:39   ` Paul Moore
  2018-06-04 14:03 ` [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
  8 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 18:02 UTC (permalink / raw)
  To: linux-integrity
  Cc: Mimi Zohar, linux-security-module, linux-kernel, David Howells,
	Luis R . Rodriguez, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Jeff Vander Stoep,
	Paul Moore, Casey Schaufler

Both the init_module and finit_module syscalls call either directly
or indirectly the security_kernel_read_file LSM hook.  This patch
replaces the direct call in init_module with a call to the new
security_kernel_load_data hook and makes the corresponding changes in
SELinux and IMA.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
---
 kernel/module.c                   |  2 +-
 security/integrity/ima/ima_main.c | 24 ++++++++++--------------
 security/selinux/hooks.c          | 26 ++++++++++++++++++++------
 3 files changed, 31 insertions(+), 21 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index ce8066b88178..b97c642b5b4d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 	if (info->len < sizeof(*(info->hdr)))
 		return -ENOEXEC;
 
-	err = security_kernel_read_file(NULL, READING_MODULE);
+	err = security_kernel_load_data(LOADING_MODULE);
 	if (err)
 		return err;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 3dae605a1604..0ff1d8152f6e 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -441,17 +441,6 @@ static int read_idmap[READING_MAX_ID] = {
  */
 int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 {
-	bool sig_enforce = is_module_sig_enforced();
-
-	if (!file && read_id == READING_MODULE) {
-		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
-			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
-			return -EACCES;	/* INTEGRITY_UNKNOWN */
-		}
-		return 0;	/* We rely on module signature checking */
-	}
-
 	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
@@ -490,9 +479,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 		return 0;
 	}
 
-	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
-		return 0;
-
 	/* permit signed certs */
 	if (!file && read_id == READING_X509_CERTIFICATE)
 		return 0;
@@ -521,6 +507,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
  */
 int ima_load_data(enum kernel_load_data_id id)
 {
+	bool sig_enforce;
+
 	if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
 		return 0;
 
@@ -536,6 +524,14 @@ int ima_load_data(enum kernel_load_data_id id)
 			pr_err("Prevent firmware sysfs fallback loading.\n");
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
 		}
+		break;
+	case LOADING_MODULE:
+		sig_enforce = is_module_sig_enforced();
+
+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
+			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
 	default:
 		break;
 	}
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 02ebd1585eaf..e02186470fc5 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
 	u32 sid = current_sid();
 	int rc;
 
-	/* init_module */
-	if (file == NULL)
-		return avc_has_perm(&selinux_state,
-				    sid, sid, SECCLASS_SYSTEM,
-					SYSTEM__MODULE_LOAD, NULL);
-
 	/* finit_module */
 
 	ad.type = LSM_AUDIT_DATA_FILE;
@@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
 				SYSTEM__MODULE_LOAD, &ad);
 }
 
+static int selinux_kernel_load_data(enum kernel_load_data_id id)
+{
+	u32 sid;
+	int rc = 0;
+
+	switch (id) {
+	case LOADING_MODULE:
+		sid = current_sid();
+
+		/* init_module */
+		return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
+				    SYSTEM__MODULE_LOAD, NULL);
+	default:
+		break;
+	}
+
+	return rc;
+}
+
 static int selinux_kernel_read_file(struct file *file,
 				    enum kernel_read_file_id id)
 {
@@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
+	LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
 	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
 	LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
 	LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
-- 
2.7.5


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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 18:02 ` [PATCH v4 8/8] module: replace the existing LSM hook in init_module Mimi Zohar
@ 2018-05-29 22:39   ` Paul Moore
  2018-05-29 23:14     ` Mimi Zohar
                       ` (2 more replies)
  0 siblings, 3 replies; 43+ messages in thread
From: Paul Moore @ 2018-05-29 22:39 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Both the init_module and finit_module syscalls call either directly
> or indirectly the security_kernel_read_file LSM hook.  This patch
> replaces the direct call in init_module with a call to the new
> security_kernel_load_data hook and makes the corresponding changes in
> SELinux and IMA.
>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> ---
>  kernel/module.c                   |  2 +-
>  security/integrity/ima/ima_main.c | 24 ++++++++++--------------
>  security/selinux/hooks.c          | 26 ++++++++++++++++++++------
>  3 files changed, 31 insertions(+), 21 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index ce8066b88178..b97c642b5b4d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>         if (info->len < sizeof(*(info->hdr)))
>                 return -ENOEXEC;
>
> -       err = security_kernel_read_file(NULL, READING_MODULE);
> +       err = security_kernel_load_data(LOADING_MODULE);
>         if (err)
>                 return err;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 3dae605a1604..0ff1d8152f6e 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -441,17 +441,6 @@ static int read_idmap[READING_MAX_ID] = {
>   */
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  {
> -       bool sig_enforce = is_module_sig_enforced();
> -
> -       if (!file && read_id == READING_MODULE) {
> -               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> -                   (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> -                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> -                       return -EACCES; /* INTEGRITY_UNKNOWN */
> -               }
> -               return 0;       /* We rely on module signature checking */
> -       }
> -
>         if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
>                 if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
>                     (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> @@ -490,9 +479,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>                 return 0;
>         }
>
> -       if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
> -               return 0;
> -
>         /* permit signed certs */
>         if (!file && read_id == READING_X509_CERTIFICATE)
>                 return 0;
> @@ -521,6 +507,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>   */
>  int ima_load_data(enum kernel_load_data_id id)
>  {
> +       bool sig_enforce;
> +
>         if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
>                 return 0;
>
> @@ -536,6 +524,14 @@ int ima_load_data(enum kernel_load_data_id id)
>                         pr_err("Prevent firmware sysfs fallback loading.\n");
>                         return -EACCES; /* INTEGRITY_UNKNOWN */
>                 }
> +               break;
> +       case LOADING_MODULE:
> +               sig_enforce = is_module_sig_enforced();
> +
> +               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
> +                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> +                       return -EACCES; /* INTEGRITY_UNKNOWN */
> +               }
>         default:
>                 break;
>         }
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 02ebd1585eaf..e02186470fc5 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
>         u32 sid = current_sid();
>         int rc;
>
> -       /* init_module */
> -       if (file == NULL)
> -               return avc_has_perm(&selinux_state,
> -                                   sid, sid, SECCLASS_SYSTEM,
> -                                       SYSTEM__MODULE_LOAD, NULL);
> -
>         /* finit_module */
>
>         ad.type = LSM_AUDIT_DATA_FILE;
> @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
>                                 SYSTEM__MODULE_LOAD, &ad);
>  }
>
> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> +{
> +       u32 sid;
> +       int rc = 0;
> +
> +       switch (id) {
> +       case LOADING_MODULE:
> +               sid = current_sid();
> +
> +               /* init_module */
> +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
> +                                   SYSTEM__MODULE_LOAD, NULL);
> +       default:
> +               break;
> +       }
> +
> +       return rc;
> +}

I'm not a fan of the duplication here.  If we must have a new LSM hook
for this, can we at least have it call
selinux_kernel_module_from_file() so we have all the kernel module
loading logic/controls in one function?  Yes, I understand there are
differences between init_module() and finit_module() but I like
handling them both in one function as we do today.

>  static int selinux_kernel_read_file(struct file *file,
>                                     enum kernel_read_file_id id)
>  {
> @@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
>         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
>         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
>         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> --
> 2.7.5
>

-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 22:39   ` Paul Moore
@ 2018-05-29 23:14     ` Mimi Zohar
  2018-05-30 21:00       ` Paul Moore
  2018-05-29 23:25     ` [PATCH v4 " Mimi Zohar
  2018-05-30  2:25     ` Eric W. Biederman
  2 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 23:14 UTC (permalink / raw)
  To: Paul Moore
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote:
[...]
> > @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
> >                                 SYSTEM__MODULE_LOAD, &ad);
> >  }
> >
> > +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> > +{
> > +       u32 sid;
> > +       int rc = 0;
> > +
> > +       switch (id) {
> > +       case LOADING_MODULE:
> > +               sid = current_sid();
> > +
> > +               /* init_module */
> > +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
> > +                                   SYSTEM__MODULE_LOAD, NULL);
> > +       default:
> > +               break;
> > +       }
> > +
> > +       return rc;
> > +}
> 
> I'm not a fan of the duplication here.  If we must have a new LSM hook
> for this, can we at least have it call
> selinux_kernel_module_from_file() so we have all the kernel module
> loading logic/controls in one function?  Yes, I understand there are
> differences between init_module() and finit_module() but I like
> handling them both in one function as we do today.

There's some disagreement as to whether we really need two LSM hooks.
 This sounds like you would prefer a single LSM hook, not the two that
this patch set introduces.

We need to come to some consensus.  (Comments appreciated in 0/8.)

Mimi

> 
> >  static int selinux_kernel_read_file(struct file *file,
> >                                     enum kernel_read_file_id id)
> >  {
> > @@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> >         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
> >         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> > +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
> >         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> >         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
> >         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> > --
> > 2.7.5
> >
> 


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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 22:39   ` Paul Moore
  2018-05-29 23:14     ` Mimi Zohar
@ 2018-05-29 23:25     ` Mimi Zohar
  2018-05-30  2:25     ` Eric W. Biederman
  2 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-05-29 23:25 UTC (permalink / raw)
  To: Paul Moore, Kees Cook
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

Hi Kees,

Missing from this patch are the loadpin changes.  Before including
them in the next version of this patch, do you prefer separating the
init_module from the finit_module support in loadpin_read_file() or
keeping it as one function, like Paul for SELinux?

Mimi

On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote:
> On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > Both the init_module and finit_module syscalls call either directly
> > or indirectly the security_kernel_read_file LSM hook.  This patch
> > replaces the direct call in init_module with a call to the new
> > security_kernel_load_data hook and makes the corresponding changes in
> > SELinux and IMA.
> >
> > Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> > Cc: Jeff Vander Stoep <jeffv@google.com>
> > Cc: Paul Moore <paul@paul-moore.com>
> > Cc: Casey Schaufler <casey@schaufler-ca.com>
> > ---
> >  kernel/module.c                   |  2 +-
> >  security/integrity/ima/ima_main.c | 24 ++++++++++--------------
> >  security/selinux/hooks.c          | 26 ++++++++++++++++++++------
> >  3 files changed, 31 insertions(+), 21 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index ce8066b88178..b97c642b5b4d 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> >         if (info->len < sizeof(*(info->hdr)))
> >                 return -ENOEXEC;
> >
> > -       err = security_kernel_read_file(NULL, READING_MODULE);
> > +       err = security_kernel_load_data(LOADING_MODULE);
> >         if (err)
> >                 return err;
> >
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 3dae605a1604..0ff1d8152f6e 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -441,17 +441,6 @@ static int read_idmap[READING_MAX_ID] = {
> >   */
> >  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> >  {
> > -       bool sig_enforce = is_module_sig_enforced();
> > -
> > -       if (!file && read_id == READING_MODULE) {
> > -               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> > -                   (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > -                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> > -                       return -EACCES; /* INTEGRITY_UNKNOWN */
> > -               }
> > -               return 0;       /* We rely on module signature checking */
> > -       }
> > -
> >         if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> >                 if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> >                     (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > @@ -490,9 +479,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> >                 return 0;
> >         }
> >
> > -       if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
> > -               return 0;
> > -
> >         /* permit signed certs */
> >         if (!file && read_id == READING_X509_CERTIFICATE)
> >                 return 0;
> > @@ -521,6 +507,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> >   */
> >  int ima_load_data(enum kernel_load_data_id id)
> >  {
> > +       bool sig_enforce;
> > +
> >         if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
> >                 return 0;
> >
> > @@ -536,6 +524,14 @@ int ima_load_data(enum kernel_load_data_id id)
> >                         pr_err("Prevent firmware sysfs fallback loading.\n");
> >                         return -EACCES; /* INTEGRITY_UNKNOWN */
> >                 }
> > +               break;
> > +       case LOADING_MODULE:
> > +               sig_enforce = is_module_sig_enforced();
> > +
> > +               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
> > +                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> > +                       return -EACCES; /* INTEGRITY_UNKNOWN */
> > +               }
> >         default:
> >                 break;
> >         }
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index 02ebd1585eaf..e02186470fc5 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
> >         u32 sid = current_sid();
> >         int rc;
> >
> > -       /* init_module */
> > -       if (file == NULL)
> > -               return avc_has_perm(&selinux_state,
> > -                                   sid, sid, SECCLASS_SYSTEM,
> > -                                       SYSTEM__MODULE_LOAD, NULL);
> > -
> >         /* finit_module */
> >
> >         ad.type = LSM_AUDIT_DATA_FILE;
> > @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
> >                                 SYSTEM__MODULE_LOAD, &ad);
> >  }
> >
> > +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> > +{
> > +       u32 sid;
> > +       int rc = 0;
> > +
> > +       switch (id) {
> > +       case LOADING_MODULE:
> > +               sid = current_sid();
> > +
> > +               /* init_module */
> > +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
> > +                                   SYSTEM__MODULE_LOAD, NULL);
> > +       default:
> > +               break;
> > +       }
> > +
> > +       return rc;
> > +}
> 
> I'm not a fan of the duplication here.  If we must have a new LSM hook
> for this, can we at least have it call
> selinux_kernel_module_from_file() so we have all the kernel module
> loading logic/controls in one function?  Yes, I understand there are
> differences between init_module() and finit_module() but I like
> handling them both in one function as we do today.
> 
> >  static int selinux_kernel_read_file(struct file *file,
> >                                     enum kernel_read_file_id id)
> >  {
> > @@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> >         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> >         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
> >         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> > +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
> >         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> >         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
> >         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> > --
> > 2.7.5
> >
> 


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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 22:39   ` Paul Moore
  2018-05-29 23:14     ` Mimi Zohar
  2018-05-29 23:25     ` [PATCH v4 " Mimi Zohar
@ 2018-05-30  2:25     ` Eric W. Biederman
  2018-05-30 21:09       ` Paul Moore
  2 siblings, 1 reply; 43+ messages in thread
From: Eric W. Biederman @ 2018-05-30  2:25 UTC (permalink / raw)
  To: Paul Moore
  Cc: Mimi Zohar, linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Jeff Vander Stoep,
	Casey Schaufler

Paul Moore <paul@paul-moore.com> writes:

> On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>> Both the init_module and finit_module syscalls call either directly
>> or indirectly the security_kernel_read_file LSM hook.  This patch
>> replaces the direct call in init_module with a call to the new
>> security_kernel_load_data hook and makes the corresponding changes in
>> SELinux and IMA.
>>
>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>> Cc: Jeff Vander Stoep <jeffv@google.com>
>> Cc: Paul Moore <paul@paul-moore.com>
>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>> ---
>>  kernel/module.c                   |  2 +-
>>  security/integrity/ima/ima_main.c | 24 ++++++++++--------------
>>  security/selinux/hooks.c          | 26 ++++++++++++++++++++------
>>  3 files changed, 31 insertions(+), 21 deletions(-)
>>
>> diff --git a/kernel/module.c b/kernel/module.c
>> index ce8066b88178..b97c642b5b4d 100644
>> --- a/kernel/module.c
>> +++ b/kernel/module.c
>> @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>>         if (info->len < sizeof(*(info->hdr)))
>>                 return -ENOEXEC;
>>
>> -       err = security_kernel_read_file(NULL, READING_MODULE);
>> +       err = security_kernel_load_data(LOADING_MODULE);
>>         if (err)
>>                 return err;
>>
>> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>> index 3dae605a1604..0ff1d8152f6e 100644
>> --- a/security/integrity/ima/ima_main.c
>> +++ b/security/integrity/ima/ima_main.c
>> @@ -441,17 +441,6 @@ static int read_idmap[READING_MAX_ID] = {
>>   */
>>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>>  {
>> -       bool sig_enforce = is_module_sig_enforced();
>> -
>> -       if (!file && read_id == READING_MODULE) {
>> -               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
>> -                   (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> -                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
>> -                       return -EACCES; /* INTEGRITY_UNKNOWN */
>> -               }
>> -               return 0;       /* We rely on module signature checking */
>> -       }
>> -
>>         if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
>>                 if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
>>                     (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>> @@ -490,9 +479,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>>                 return 0;
>>         }
>>
>> -       if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
>> -               return 0;
>> -
>>         /* permit signed certs */
>>         if (!file && read_id == READING_X509_CERTIFICATE)
>>                 return 0;
>> @@ -521,6 +507,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>>   */
>>  int ima_load_data(enum kernel_load_data_id id)
>>  {
>> +       bool sig_enforce;
>> +
>>         if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
>>                 return 0;
>>
>> @@ -536,6 +524,14 @@ int ima_load_data(enum kernel_load_data_id id)
>>                         pr_err("Prevent firmware sysfs fallback loading.\n");
>>                         return -EACCES; /* INTEGRITY_UNKNOWN */
>>                 }
>> +               break;
>> +       case LOADING_MODULE:
>> +               sig_enforce = is_module_sig_enforced();
>> +
>> +               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
>> +                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
>> +                       return -EACCES; /* INTEGRITY_UNKNOWN */
>> +               }
>>         default:
>>                 break;
>>         }
>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> index 02ebd1585eaf..e02186470fc5 100644
>> --- a/security/selinux/hooks.c
>> +++ b/security/selinux/hooks.c
>> @@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
>>         u32 sid = current_sid();
>>         int rc;
>>
>> -       /* init_module */
>> -       if (file == NULL)
>> -               return avc_has_perm(&selinux_state,
>> -                                   sid, sid, SECCLASS_SYSTEM,
>> -                                       SYSTEM__MODULE_LOAD, NULL);
>> -
>>         /* finit_module */
>>
>>         ad.type = LSM_AUDIT_DATA_FILE;
>> @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
>>                                 SYSTEM__MODULE_LOAD, &ad);
>>  }
>>
>> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
>> +{
>> +       u32 sid;
>> +       int rc = 0;
>> +
>> +       switch (id) {
>> +       case LOADING_MODULE:
>> +               sid = current_sid();
>> +
>> +               /* init_module */
>> +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
>> +                                   SYSTEM__MODULE_LOAD, NULL);
>> +       default:
>> +               break;
>> +       }
>> +
>> +       return rc;
>> +}
>
> I'm not a fan of the duplication here.

There are a couple of fundamental and strong differences here.

selinux_kernel_load_data only has the current_sid to work with.

selinux_module_data_from_file is all about the logic of how
to get fsec or isec from the file and from the inode.

For selinux and for every other lsm that uses the hooks that difference
of whether or not you have a file leads to different logic and different
code.  There is no meaningful sharing between the two cases.

In selinux all of the meaningful sharing happens with calls to
avc_has_perm(... SYSTEM__MODULE_LOAD, ...);

So as far as I can see talking about duplication is unfounded there is
none.

> If we must have a new LSM hook
> for this, can we at least have it call
> selinux_kernel_module_from_file() so we have all the kernel module
> loading logic/controls in one function?  Yes, I understand there are
> differences between init_module() and finit_module() but I like
> handling them both in one function as we do today.

Except even today the actual logic is not shared in a single function.
The only thing that happens in a single function is a switch statement
that calls different functions.

So what is the point of having a ``common'' function?

Eric

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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-29 23:14     ` Mimi Zohar
@ 2018-05-30 21:00       ` Paul Moore
  2018-05-31 15:23         ` [PATCH v4a " Mimi Zohar
  0 siblings, 1 reply; 43+ messages in thread
From: Paul Moore @ 2018-05-30 21:00 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

On Tue, May 29, 2018 at 7:14 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-05-29 at 18:39 -0400, Paul Moore wrote:
> [...]
>> > @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
>> >                                 SYSTEM__MODULE_LOAD, &ad);
>> >  }
>> >
>> > +static int selinux_kernel_load_data(enum kernel_load_data_id id)
>> > +{
>> > +       u32 sid;
>> > +       int rc = 0;
>> > +
>> > +       switch (id) {
>> > +       case LOADING_MODULE:
>> > +               sid = current_sid();
>> > +
>> > +               /* init_module */
>> > +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
>> > +                                   SYSTEM__MODULE_LOAD, NULL);
>> > +       default:
>> > +               break;
>> > +       }
>> > +
>> > +       return rc;
>> > +}
>>
>> I'm not a fan of the duplication here.  If we must have a new LSM hook
>> for this, can we at least have it call
>> selinux_kernel_module_from_file() so we have all the kernel module
>> loading logic/controls in one function?  Yes, I understand there are
>> differences between init_module() and finit_module() but I like
>> handling them both in one function as we do today.
>
> There's some disagreement as to whether we really need two LSM hooks.
>  This sounds like you would prefer a single LSM hook, not the two that
> this patch set introduces.
>
> We need to come to some consensus.  (Comments appreciated in 0/8.)

My comments were intentionally made on the SELinux specific code and
not the LSM generic code/hooks.  As the LSM hook code must not make
any assumptions about the underlying LSM implementations, it may make
sense to have two different hooks.  However as far as the SELinux code
is concerned I would rather keep the access controls in one function
as mentioned above.  From a purely selfish SELinux perspective I would
prefer a single hook, but if others feel two hooks are better, that's
fine with me too.

>> >  static int selinux_kernel_read_file(struct file *file,
>> >                                     enum kernel_read_file_id id)
>> >  {
>> > @@ -6950,6 +6963,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>> >         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>> >         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>> >         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
>> > +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
>> >         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
>> >         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
>> >         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
>> > --
>> > 2.7.5

-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH v4 8/8] module: replace the existing LSM hook in init_module
  2018-05-30  2:25     ` Eric W. Biederman
@ 2018-05-30 21:09       ` Paul Moore
  0 siblings, 0 replies; 43+ messages in thread
From: Paul Moore @ 2018-05-30 21:09 UTC (permalink / raw)
  To: Eric W. Biederman
  Cc: Mimi Zohar, linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Jeff Vander Stoep,
	Casey Schaufler

On Tue, May 29, 2018 at 10:25 PM, Eric W. Biederman
<ebiederm@xmission.com> wrote:
> Paul Moore <paul@paul-moore.com> writes:
>> On Tue, May 29, 2018 at 2:02 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
>>> Both the init_module and finit_module syscalls call either directly
>>> or indirectly the security_kernel_read_file LSM hook.  This patch
>>> replaces the direct call in init_module with a call to the new
>>> security_kernel_load_data hook and makes the corresponding changes in
>>> SELinux and IMA.
>>>
>>> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>>> Cc: Jeff Vander Stoep <jeffv@google.com>
>>> Cc: Paul Moore <paul@paul-moore.com>
>>> Cc: Casey Schaufler <casey@schaufler-ca.com>
>>> ---
>>>  kernel/module.c                   |  2 +-
>>>  security/integrity/ima/ima_main.c | 24 ++++++++++--------------
>>>  security/selinux/hooks.c          | 26 ++++++++++++++++++++------
>>>  3 files changed, 31 insertions(+), 21 deletions(-)

...

>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>> index 02ebd1585eaf..e02186470fc5 100644
>>> --- a/security/selinux/hooks.c
>>> +++ b/security/selinux/hooks.c
>>> @@ -4018,12 +4018,6 @@ static int selinux_kernel_module_from_file(struct file *file)
>>>         u32 sid = current_sid();
>>>         int rc;
>>>
>>> -       /* init_module */
>>> -       if (file == NULL)
>>> -               return avc_has_perm(&selinux_state,
>>> -                                   sid, sid, SECCLASS_SYSTEM,
>>> -                                       SYSTEM__MODULE_LOAD, NULL);
>>> -
>>>         /* finit_module */
>>>
>>>         ad.type = LSM_AUDIT_DATA_FILE;
>>> @@ -4043,6 +4037,25 @@ static int selinux_kernel_module_from_file(struct file *file)
>>>                                 SYSTEM__MODULE_LOAD, &ad);
>>>  }
>>>
>>> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
>>> +{
>>> +       u32 sid;
>>> +       int rc = 0;
>>> +
>>> +       switch (id) {
>>> +       case LOADING_MODULE:
>>> +               sid = current_sid();
>>> +
>>> +               /* init_module */
>>> +               return avc_has_perm(&selinux_state, sid, sid, SECCLASS_SYSTEM,
>>> +                                   SYSTEM__MODULE_LOAD, NULL);
>>> +       default:
>>> +               break;
>>> +       }
>>> +
>>> +       return rc;
>>> +}
>>
>> I'm not a fan of the duplication here.
>
> There are a couple of fundamental and strong differences here.
>
> selinux_kernel_load_data only has the current_sid to work with.
>
> selinux_module_data_from_file is all about the logic of how
> to get fsec or isec from the file and from the inode.
>
> For selinux and for every other lsm that uses the hooks that difference
> of whether or not you have a file leads to different logic and different
> code.  There is no meaningful sharing between the two cases.
>
> In selinux all of the meaningful sharing happens with calls to
> avc_has_perm(... SYSTEM__MODULE_LOAD, ...);
>
> So as far as I can see talking about duplication is unfounded there is
> none.

The duplication is the system:module_load access check (look at the
avc_has_perm() calls in selinux_kernel_module_from_file().  I believe
there is a readability/maintainability advantage to keeping them in
one function, and I would like it to stay that way.  If these
particular LSM hook(s) ever became performance critical (e.g. many
invocations a second) then I could see the value in separating them
out to eliminating some conditionals/branching, but as far as I can
tell that is not a problem at present.

I'm guessing your concern is more about the LSM hooks themselves and
not the individual implementations, in which case please see my last
response to Mimi.  I'm not opposed to two separate LSM hooks, I just
want to avoid moving the system:module_load checks out of
selinux_kernel_module_from_file(); which is independent of the number
of LSM hooks.

-- 
paul moore
www.paul-moore.com

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

* [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-05-30 21:00       ` Paul Moore
@ 2018-05-31 15:23         ` Mimi Zohar
  2018-06-01 22:28           ` Paul Moore
                             ` (2 more replies)
  0 siblings, 3 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-05-31 15:23 UTC (permalink / raw)
  To: Paul Moore, Kees Cook
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

Both the init_module and finit_module syscalls call either directly
or indirectly the security_kernel_read_file LSM hook.  This patch
replaces the direct call in init_module with a call to the new
security_kernel_load_data hook and makes the corresponding changes
in SELinux, LoadPin, and IMA.

Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Cc: Jeff Vander Stoep <jeffv@google.com>
Cc: Paul Moore <paul@paul-moore.com>
Cc: Casey Schaufler <casey@schaufler-ca.com>
Cc: Kees Cook <keescook@chromium.org>

---
Changelog:
- For SELinux, have both the security_kernel_read_file and
security_kernel_load_data LSM hooks call selinux_kernel_read_file().
- LoadPin: replace existing init_module LSM hook support with
new security_kernel_load_data hook.

 kernel/module.c                   |  2 +-
 security/integrity/ima/ima_main.c | 24 ++++++++++--------------
 security/loadpin/loadpin.c        | 15 +++++++++++++++
 security/selinux/hooks.c          | 15 +++++++++++++++
 4 files changed, 41 insertions(+), 15 deletions(-)

diff --git a/kernel/module.c b/kernel/module.c
index ce8066b88178..b97c642b5b4d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
 	if (info->len < sizeof(*(info->hdr)))
 		return -ENOEXEC;
 
-	err = security_kernel_read_file(NULL, READING_MODULE);
+	err = security_kernel_load_data(LOADING_MODULE);
 	if (err)
 		return err;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 5a7696152982..cd33a2eff496 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -438,17 +438,6 @@ static int read_idmap[READING_MAX_ID] = {
  */
 int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 {
-	bool sig_enforce = is_module_sig_enforced();
-
-	if (!file && read_id == READING_MODULE) {
-		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
-			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
-			return -EACCES;	/* INTEGRITY_UNKNOWN */
-		}
-		return 0;	/* We rely on module signature checking */
-	}
-
 	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
 		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
@@ -487,9 +476,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 		return 0;
 	}
 
-	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
-		return 0;
-
 	/* permit signed certs */
 	if (!file && read_id == READING_X509_CERTIFICATE)
 		return 0;
@@ -518,6 +504,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
  */
 int ima_load_data(enum kernel_load_data_id id)
 {
+	bool sig_enforce;
+
 	if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
 		return 0;
 
@@ -533,6 +521,14 @@ int ima_load_data(enum kernel_load_data_id id)
 			pr_err("Prevent firmware sysfs fallback loading.\n");
 			return -EACCES;	/* INTEGRITY_UNKNOWN */
 		}
+		break;
+	case LOADING_MODULE:
+		sig_enforce = is_module_sig_enforced();
+
+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
+			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		}
 	default:
 		break;
 	}
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 5fa191252c8f..a9c07bfbc338 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -173,9 +173,24 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 	return 0;
 }
 
+static int loadpin_load_data(enum kernel_load_data_id id)
+{
+	int rc = 0;
+
+	switch (id) {
+	case LOADING_MODULE:
+		rc = loadpin_read_file(NULL, READING_MODULE);
+	default:
+		break;
+	}
+
+	return rc;
+}
+
 static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
+	LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
 };
 
 void __init loadpin_add_hooks(void)
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 02ebd1585eaf..475aed9ee2c7 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4059,6 +4059,20 @@ static int selinux_kernel_read_file(struct file *file,
 	return rc;
 }
 
+static int selinux_kernel_load_data(enum kernel_load_data_id id)
+{
+	int rc = 0;
+
+	switch (id) {
+	case LOADING_MODULE:
+		rc = selinux_kernel_module_from_file(NULL);
+	default:
+		break;
+	}
+
+	return rc;
+}
+
 static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
 {
 	return avc_has_perm(&selinux_state,
@@ -6950,6 +6964,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
+	LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
 	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
 	LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
 	LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
-- 
2.7.5


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

* Re: [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback
  2018-05-29 18:01 ` [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback Mimi Zohar
@ 2018-06-01 18:19   ` Luis R. Rodriguez
  0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-01 18:19 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook

On Tue, May 29, 2018 at 02:01:56PM -0400, Mimi Zohar wrote:
> Add an LSM hook prior to allowing firmware sysfs fallback loading.

Acked-by: Luis R. Rodriguez <mcgrof@kernel.org>

> 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 v4:
> - call new LSM security_kernel_arg hook
> 
> Changelog v2:
> - 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 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
> index 358354148dec..2443bda81631 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_load_data(LOADING_FIRMWARE);
> +	if (ret < 0)
> +		return ret;
> +
>  	return fw_force_sysfs_fallback(opt_flags);
>  }
>  
> -- 
> 2.7.5
> 
> 

-- 
Do not panic

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

* Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
  2018-05-29 18:01 ` [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback) Mimi Zohar
@ 2018-06-01 18:21   ` Luis R. Rodriguez
  2018-06-01 22:39     ` Mimi Zohar
  0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-01 18:21 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Matthew Garrett

On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> Luis, is the security_kernel_post_read_file LSM hook in
> firmware_loading_store() still needed after this patch?  Should it be
> calling security_kernel_load_data() instead?

That's up to Kees to decide as he added that hook, and knows
what LSMs may be doing with it. From my perspective it is confusing
to have that hook there so I think it could be removed now.

Kees?

  Luis

> 
> ---
> 
> 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, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index a565d46084c2..4a87f78098c8 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -475,8 +475,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  
>  	if (!file && read_id == READING_FIRMWARE) {
>  		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> -		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> +			pr_err("Prevent firmware loading_store.\n");
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> +		}
>  		return 0;
>  	}
>  
> @@ -520,6 +522,12 @@ int ima_load_data(enum kernel_load_data_id id)
>  			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
>  			return -EACCES;	/* INTEGRITY_UNKNOWN */
>  		}
> +		break;
> +	case LOADING_FIRMWARE:
> +		if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
> +			pr_err("Prevent firmware sysfs fallback loading.\n");
> +			return -EACCES;	/* INTEGRITY_UNKNOWN */
> +		}
>  	default:
>  		break;
>  	}
> -- 
> 2.7.5
> 
> 

-- 
Do not panic

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

* Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-05-29 18:01 ` [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer) Mimi Zohar
@ 2018-06-01 19:15   ` Luis R. Rodriguez
  2018-06-01 19:25     ` Luis R. Rodriguez
  0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-01 19:15 UTC (permalink / raw)
  To: Mimi Zohar, Matthew Garrett
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook,
	Serge E . Hallyn, Stephen Boyd

On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> Some systems are memory constrained but they need to load very large
> firmwares.  The firmware subsystem allows drivers to request this
> firmware be loaded from the filesystem, but this requires that the
> entire firmware be loaded into kernel memory first before it's provided
> to the driver.  This can lead to a situation where we map the firmware
> twice, once to load the firmware into kernel memory and once to copy the
> firmware into the final resting place.
> 
> To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> into a pre-allocated buffer") introduced request_firmware_into_buf() API
> that allows drivers to request firmware be loaded directly into a
> pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> allocate this buffer.  According to Documentation/DMA-API.txt,
> 
>      Consistent memory is memory for which a write by either the
>      device or the processor can immediately be read by the processor
>      or device without having to worry about caching effects.  (You
>      may however need to make sure to flush the processor's write
>      buffers before telling devices to read that memory.)
> 
> Devices using pre-allocated DMA memory run the risk of the firmware
> being accessible by the device prior to the kernel's firmware signature
> verification has completed.

Indeed. And since its DMA memory we have *no idea* what can happen in
terms of consumption of this firmware from hardware, when it would start
consuming it in particular.

If the device has its own hardware firmware verification mechanism this is
completely obscure to us, but it may however suffice certain security policies.

The problem here lies in the conflicting security policies of the kernel wanting
to not give away firmware until its complete and the current inability to enable
us to have platforms suggest they trust hardware won't do something stupid.
This becomes an issue since the semantics of the firmware API preallocated
buffer do not require currently allow the kernel to inform LSMs of the fact
that a buffer is DMA memory or not, and a way for certain platforms then
to say that such use is fine for specific devices.

Given a pointer can we determine if a piece of memory is DMA or not? Seems
hacky to use such inferences if we had them anyway... but worth asking...

I would suggest we augment the prealloc buffer firmware API to pass a
flags argument which helps describe the preallocated buffer, and for now
allow us to enable callers to describe if the buffer is kernel memory or
DMA memory, and then have the LSMs decide based on this information as well.
The qualcomm driver would change to use the DMA flag, and IMA would in turn
deny such uses. Once and if platforms want to trust the DMA flag they can
later add infrastructure for specifying this somehow.

> Loading firmware already calls the security_kernel_read_file LSM hook.
> With an IMA policy requiring signed firmware, this patch prevents
> 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 <sboyd@kernel.org>
> ---
>  security/integrity/ima/ima_main.c | 26 +++++++++++++++++---------
>  1 file changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 4a87f78098c8..3dae605a1604 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -419,6 +419,15 @@ void ima_post_path_mknod(struct dentry *dentry)
>  		iint->flags |= IMA_NEW_FILE;
>  }
>  
> +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,
> +	[READING_POLICY] = POLICY_CHECK
> +};
> +
>  /**
>   * ima_read_file - pre-measure/appraise hook decision based on policy
>   * @file: pointer to the file to be measured/appraised/audit
> @@ -442,18 +451,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  		}
>  		return 0;	/* We rely on module signature checking */
>  	}
> +
> +	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;
>  }
>  
> -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,
> -	[READING_POLICY] = POLICY_CHECK
> -};
> -
>  /**
>   * ima_post_read_file - in memory collect/appraise/audit measurement
>   * @file: pointer to the file to be measured/appraised/audit
> -- 
> 2.7.5
> 
> 

-- 
Do not panic

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

* Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-06-01 19:15   ` Luis R. Rodriguez
@ 2018-06-01 19:25     ` Luis R. Rodriguez
  2018-06-05 22:37       ` Kees Cook
  0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-01 19:25 UTC (permalink / raw)
  To: Luis R. Rodriguez, Vlastimil Babka
  Cc: Mimi Zohar, Matthew Garrett, linux-integrity,
	linux-security-module, linux-kernel, David Howells,
	Eric Biederman, kexec, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Kees Cook, Serge E . Hallyn, Stephen Boyd

On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> > Some systems are memory constrained but they need to load very large
> > firmwares.  The firmware subsystem allows drivers to request this
> > firmware be loaded from the filesystem, but this requires that the
> > entire firmware be loaded into kernel memory first before it's provided
> > to the driver.  This can lead to a situation where we map the firmware
> > twice, once to load the firmware into kernel memory and once to copy the
> > firmware into the final resting place.
> > 
> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> > that allows drivers to request firmware be loaded directly into a
> > pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> > allocate this buffer.  According to Documentation/DMA-API.txt,
> > 
> >      Consistent memory is memory for which a write by either the
> >      device or the processor can immediately be read by the processor
> >      or device without having to worry about caching effects.  (You
> >      may however need to make sure to flush the processor's write
> >      buffers before telling devices to read that memory.)
> > 
> > Devices using pre-allocated DMA memory run the risk of the firmware
> > being accessible by the device prior to the kernel's firmware signature
> > verification has completed.
> 
> Indeed. And since its DMA memory we have *no idea* what can happen in
> terms of consumption of this firmware from hardware, when it would start
> consuming it in particular.
> 
> If the device has its own hardware firmware verification mechanism this is
> completely obscure to us, but it may however suffice certain security policies.
> 
> The problem here lies in the conflicting security policies of the kernel wanting
> to not give away firmware until its complete and the current inability to enable
> us to have platforms suggest they trust hardware won't do something stupid.
> This becomes an issue since the semantics of the firmware API preallocated
> buffer do not require currently allow the kernel to inform LSMs of the fact
> that a buffer is DMA memory or not, and a way for certain platforms then
> to say that such use is fine for specific devices.
> 
> Given a pointer can we determine if a piece of memory is DMA or not?

FWIW

Vlastimil suggests page_zone() or virt_to_page() may be able to.

  Luis

> Seems
> hacky to use such inferences if we had them anyway... but worth asking...
> 
> I would suggest we augment the prealloc buffer firmware API to pass a
> flags argument which helps describe the preallocated buffer, and for now
> allow us to enable callers to describe if the buffer is kernel memory or
> DMA memory, and then have the LSMs decide based on this information as well.
> The qualcomm driver would change to use the DMA flag, and IMA would in turn
> deny such uses. Once and if platforms want to trust the DMA flag they can
> later add infrastructure for specifying this somehow.
> 
> > Loading firmware already calls the security_kernel_read_file LSM hook.
> > With an IMA policy requiring signed firmware, this patch prevents
> > 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 <sboyd@kernel.org>
> > ---
> >  security/integrity/ima/ima_main.c | 26 +++++++++++++++++---------
> >  1 file changed, 17 insertions(+), 9 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index 4a87f78098c8..3dae605a1604 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -419,6 +419,15 @@ void ima_post_path_mknod(struct dentry *dentry)
> >  		iint->flags |= IMA_NEW_FILE;
> >  }
> >  
> > +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,
> > +	[READING_POLICY] = POLICY_CHECK
> > +};
> > +
> >  /**
> >   * ima_read_file - pre-measure/appraise hook decision based on policy
> >   * @file: pointer to the file to be measured/appraised/audit
> > @@ -442,18 +451,17 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> >  		}
> >  		return 0;	/* We rely on module signature checking */
> >  	}
> > +
> > +	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;
> >  }
> >  
> > -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,
> > -	[READING_POLICY] = POLICY_CHECK
> > -};
> > -
> >  /**
> >   * ima_post_read_file - in memory collect/appraise/audit measurement
> >   * @file: pointer to the file to be measured/appraised/audit
> > -- 
> > 2.7.5
> > 
> > 
> 
> -- 
> Do not panic
> 

-- 
Do not panic

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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-05-31 15:23         ` [PATCH v4a " Mimi Zohar
@ 2018-06-01 22:28           ` Paul Moore
  2018-06-04  9:19           ` Jessica Yu
  2018-06-05 19:45           ` Kees Cook
  2 siblings, 0 replies; 43+ messages in thread
From: Paul Moore @ 2018-06-01 22:28 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Kees Cook, linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

On Thu, May 31, 2018 at 11:23 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> Both the init_module and finit_module syscalls call either directly
> or indirectly the security_kernel_read_file LSM hook.  This patch
> replaces the direct call in init_module with a call to the new
> security_kernel_load_data hook and makes the corresponding changes
> in SELinux, LoadPin, and IMA.
>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Jeff Vander Stoep <jeffv@google.com>
> Cc: Paul Moore <paul@paul-moore.com>
> Cc: Casey Schaufler <casey@schaufler-ca.com>
> Cc: Kees Cook <keescook@chromium.org>
>
> ---
> Changelog:
> - For SELinux, have both the security_kernel_read_file and
> security_kernel_load_data LSM hooks call selinux_kernel_read_file().
> - LoadPin: replace existing init_module LSM hook support with
> new security_kernel_load_data hook.
>
>  kernel/module.c                   |  2 +-
>  security/integrity/ima/ima_main.c | 24 ++++++++++--------------
>  security/loadpin/loadpin.c        | 15 +++++++++++++++
>  security/selinux/hooks.c          | 15 +++++++++++++++
>  4 files changed, 41 insertions(+), 15 deletions(-)

As mentioned in the previous iteration, I have no strong opinion on
the question of the LSM hooks, but the SELinux bits look okay to me.

Acked-by: Paul Moore <paul@paul-moore.com>

> diff --git a/kernel/module.c b/kernel/module.c
> index ce8066b88178..b97c642b5b4d 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
>         if (info->len < sizeof(*(info->hdr)))
>                 return -ENOEXEC;
>
> -       err = security_kernel_read_file(NULL, READING_MODULE);
> +       err = security_kernel_load_data(LOADING_MODULE);
>         if (err)
>                 return err;
>
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index 5a7696152982..cd33a2eff496 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -438,17 +438,6 @@ static int read_idmap[READING_MAX_ID] = {
>   */
>  int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
>  {
> -       bool sig_enforce = is_module_sig_enforced();
> -
> -       if (!file && read_id == READING_MODULE) {
> -               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
> -                   (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> -                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> -                       return -EACCES; /* INTEGRITY_UNKNOWN */
> -               }
> -               return 0;       /* We rely on module signature checking */
> -       }
> -
>         if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
>                 if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
>                     (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> @@ -487,9 +476,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>                 return 0;
>         }
>
> -       if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
> -               return 0;
> -
>         /* permit signed certs */
>         if (!file && read_id == READING_X509_CERTIFICATE)
>                 return 0;
> @@ -518,6 +504,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>   */
>  int ima_load_data(enum kernel_load_data_id id)
>  {
> +       bool sig_enforce;
> +
>         if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
>                 return 0;
>
> @@ -533,6 +521,14 @@ int ima_load_data(enum kernel_load_data_id id)
>                         pr_err("Prevent firmware sysfs fallback loading.\n");
>                         return -EACCES; /* INTEGRITY_UNKNOWN */
>                 }
> +               break;
> +       case LOADING_MODULE:
> +               sig_enforce = is_module_sig_enforced();
> +
> +               if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
> +                       pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
> +                       return -EACCES; /* INTEGRITY_UNKNOWN */
> +               }
>         default:
>                 break;
>         }
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 5fa191252c8f..a9c07bfbc338 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -173,9 +173,24 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>         return 0;
>  }
>
> +static int loadpin_load_data(enum kernel_load_data_id id)
> +{
> +       int rc = 0;
> +
> +       switch (id) {
> +       case LOADING_MODULE:
> +               rc = loadpin_read_file(NULL, READING_MODULE);
> +       default:
> +               break;
> +       }
> +
> +       return rc;
> +}
> +
>  static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>         LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> +       LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
>  };
>
>  void __init loadpin_add_hooks(void)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 02ebd1585eaf..475aed9ee2c7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4059,6 +4059,20 @@ static int selinux_kernel_read_file(struct file *file,
>         return rc;
>  }
>
> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> +{
> +       int rc = 0;
> +
> +       switch (id) {
> +       case LOADING_MODULE:
> +               rc = selinux_kernel_module_from_file(NULL);
> +       default:
> +               break;
> +       }
> +
> +       return rc;
> +}
> +
>  static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
>  {
>         return avc_has_perm(&selinux_state,
> @@ -6950,6 +6964,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
>         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
>         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
>         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> --
> 2.7.5
>



-- 
paul moore
www.paul-moore.com

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

* Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
  2018-06-01 18:21   ` Luis R. Rodriguez
@ 2018-06-01 22:39     ` Mimi Zohar
  2018-06-01 22:46       ` Luis R. Rodriguez
  0 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-06-01 22:39 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Matthew Garrett

On Fri, 2018-06-01 at 20:21 +0200, Luis R. Rodriguez wrote:
> On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> > Luis, is the security_kernel_post_read_file LSM hook in
> > firmware_loading_store() still needed after this patch?  Should it be
> > calling security_kernel_load_data() instead?
> 
> That's up to Kees to decide as he added that hook, and knows
> what LSMs may be doing with it. From my perspective it is confusing
> to have that hook there so I think it could be removed now.
> 
> Kees?

Commit 6593d92 ("firmware_class: perform new LSM checks") references
two methods of loading firmware -  filesystem-found firmware and
demand-loaded blobs.  I assume this call in firmware_loading_store()
is the demand-loaded blobs.  Does that method still exist?  Is it
still being used?

> 
>   Luis
> 
> > 
> > ---
> > 
> > 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, 9 insertions(+), 1 deletion(-)
> > 
> > diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> > index a565d46084c2..4a87f78098c8 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -475,8 +475,10 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> >  
> >  	if (!file && read_id == READING_FIRMWARE) {
> >  		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> > -		    (ima_appraise & IMA_APPRAISE_ENFORCE))
> > +		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
> > +			pr_err("Prevent firmware loading_store.\n");
> >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > +		}
> >  		return 0;
> >  	}
> >  
> > @@ -520,6 +522,12 @@ int ima_load_data(enum kernel_load_data_id id)
> >  			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
> >  			return -EACCES;	/* INTEGRITY_UNKNOWN */
> >  		}
> > +		break;
> > +	case LOADING_FIRMWARE:
> > +		if (ima_appraise & IMA_APPRAISE_FIRMWARE) {
> > +			pr_err("Prevent firmware sysfs fallback loading.\n");
> > +			return -EACCES;	/* INTEGRITY_UNKNOWN */
> > +		}
> >  	default:
> >  		break;
> >  	}
> > -- 
> > 2.7.5
> > 
> > 
> 

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

* Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
  2018-06-01 22:39     ` Mimi Zohar
@ 2018-06-01 22:46       ` Luis R. Rodriguez
  2018-06-01 23:04         ` Mimi Zohar
  0 siblings, 1 reply; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-01 22:46 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Luis R. Rodriguez, linux-integrity, linux-security-module,
	linux-kernel, David Howells, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Matthew Garrett

On Fri, Jun 01, 2018 at 06:39:55PM -0400, Mimi Zohar wrote:
> On Fri, 2018-06-01 at 20:21 +0200, Luis R. Rodriguez wrote:
> > On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> > > Luis, is the security_kernel_post_read_file LSM hook in
> > > firmware_loading_store() still needed after this patch?  Should it be
> > > calling security_kernel_load_data() instead?
> > 
> > That's up to Kees to decide as he added that hook, and knows
> > what LSMs may be doing with it. From my perspective it is confusing
> > to have that hook there so I think it could be removed now.
> > 
> > Kees?
> 
> Commit 6593d92 ("firmware_class: perform new LSM checks") references
> two methods of loading firmware -  filesystem-found firmware and
> demand-loaded blobs.  I assume this call in firmware_loading_store()
> is the demand-loaded blobs.  Does that method still exist?  Is it
> still being used?

Yeah its the stupid sysfs interface. So likely loadpin needs porting
as you IMA as you did.

  Luis

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

* Re: [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback)
  2018-06-01 22:46       ` Luis R. Rodriguez
@ 2018-06-01 23:04         ` Mimi Zohar
  0 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-06-01 23:04 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Eric Biederman, kexec, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Matthew Garrett

On Sat, 2018-06-02 at 00:46 +0200, Luis R. Rodriguez wrote:
> On Fri, Jun 01, 2018 at 06:39:55PM -0400, Mimi Zohar wrote:
> > On Fri, 2018-06-01 at 20:21 +0200, Luis R. Rodriguez wrote:
> > > On Tue, May 29, 2018 at 02:01:57PM -0400, Mimi Zohar wrote:
> > > > Luis, is the security_kernel_post_read_file LSM hook in
> > > > firmware_loading_store() still needed after this patch?  Should it be
> > > > calling security_kernel_load_data() instead?
> > > 
> > > That's up to Kees to decide as he added that hook, and knows
> > > what LSMs may be doing with it. From my perspective it is confusing
> > > to have that hook there so I think it could be removed now.
> > > 
> > > Kees?
> > 
> > Commit 6593d92 ("firmware_class: perform new LSM checks") references
> > two methods of loading firmware -  filesystem-found firmware and
> > demand-loaded blobs.  I assume this call in firmware_loading_store()
> > is the demand-loaded blobs.  Does that method still exist?  Is it
> > still being used?
> 
> Yeah its the stupid sysfs interface. So likely loadpin needs porting
> as you IMA as you did.

In this case, it doesn't look like the call to
security_kernel_post_read_file() should be changed, which means that
all the LSMs and IMA still need to support !file.
 
Mimi


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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-05-31 15:23         ` [PATCH v4a " Mimi Zohar
  2018-06-01 22:28           ` Paul Moore
@ 2018-06-04  9:19           ` Jessica Yu
  2018-06-05 19:45           ` Kees Cook
  2 siblings, 0 replies; 43+ messages in thread
From: Jessica Yu @ 2018-06-04  9:19 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Paul Moore, Kees Cook, linux-integrity, linux-security-module,
	linux-kernel, David Howells, Luis R . Rodriguez, Eric Biederman,
	kexec, Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jeff Vander Stoep, Casey Schaufler

+++ Mimi Zohar [31/05/18 11:23 -0400]:
>Both the init_module and finit_module syscalls call either directly
>or indirectly the security_kernel_read_file LSM hook.  This patch
>replaces the direct call in init_module with a call to the new
>security_kernel_load_data hook and makes the corresponding changes
>in SELinux, LoadPin, and IMA.
>
>Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
>Cc: Jeff Vander Stoep <jeffv@google.com>
>Cc: Paul Moore <paul@paul-moore.com>
>Cc: Casey Schaufler <casey@schaufler-ca.com>
>Cc: Kees Cook <keescook@chromium.org>

For the module.c parts:

Acked-by: Jessica Yu <jeyu@kernel.org>

>Changelog:
>- For SELinux, have both the security_kernel_read_file and
>security_kernel_load_data LSM hooks call selinux_kernel_read_file().
>- LoadPin: replace existing init_module LSM hook support with
>new security_kernel_load_data hook.
>
> kernel/module.c                   |  2 +-
> security/integrity/ima/ima_main.c | 24 ++++++++++--------------
> security/loadpin/loadpin.c        | 15 +++++++++++++++
> security/selinux/hooks.c          | 15 +++++++++++++++
> 4 files changed, 41 insertions(+), 15 deletions(-)
>
>diff --git a/kernel/module.c b/kernel/module.c
>index ce8066b88178..b97c642b5b4d 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2879,7 +2879,7 @@ static int copy_module_from_user(const void __user *umod, unsigned long len,
> 	if (info->len < sizeof(*(info->hdr)))
> 		return -ENOEXEC;
>
>-	err = security_kernel_read_file(NULL, READING_MODULE);
>+	err = security_kernel_load_data(LOADING_MODULE);
> 	if (err)
> 		return err;
>
>diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
>index 5a7696152982..cd33a2eff496 100644
>--- a/security/integrity/ima/ima_main.c
>+++ b/security/integrity/ima/ima_main.c
>@@ -438,17 +438,6 @@ static int read_idmap[READING_MAX_ID] = {
>  */
> int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
> {
>-	bool sig_enforce = is_module_sig_enforced();
>-
>-	if (!file && read_id == READING_MODULE) {
>-		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES) &&
>-		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>-			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
>-			return -EACCES;	/* INTEGRITY_UNKNOWN */
>-		}
>-		return 0;	/* We rely on module signature checking */
>-	}
>-
> 	if (read_id == READING_FIRMWARE_PREALLOC_BUFFER) {
> 		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
> 		    (ima_appraise & IMA_APPRAISE_ENFORCE)) {
>@@ -487,9 +476,6 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
> 		return 0;
> 	}
>
>-	if (!file && read_id == READING_MODULE) /* MODULE_SIG_FORCE enabled */
>-		return 0;
>-
> 	/* permit signed certs */
> 	if (!file && read_id == READING_X509_CERTIFICATE)
> 		return 0;
>@@ -518,6 +504,8 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
>  */
> int ima_load_data(enum kernel_load_data_id id)
> {
>+	bool sig_enforce;
>+
> 	if ((ima_appraise & IMA_APPRAISE_ENFORCE) != IMA_APPRAISE_ENFORCE)
> 		return 0;
>
>@@ -533,6 +521,14 @@ int ima_load_data(enum kernel_load_data_id id)
> 			pr_err("Prevent firmware sysfs fallback loading.\n");
> 			return -EACCES;	/* INTEGRITY_UNKNOWN */
> 		}
>+		break;
>+	case LOADING_MODULE:
>+		sig_enforce = is_module_sig_enforced();
>+
>+		if (!sig_enforce && (ima_appraise & IMA_APPRAISE_MODULES)) {
>+			pr_err("impossible to appraise a module without a file descriptor. sig_enforce kernel parameter might help\n");
>+			return -EACCES;	/* INTEGRITY_UNKNOWN */
>+		}
> 	default:
> 		break;
> 	}
>diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
>index 5fa191252c8f..a9c07bfbc338 100644
>--- a/security/loadpin/loadpin.c
>+++ b/security/loadpin/loadpin.c
>@@ -173,9 +173,24 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> 	return 0;
> }
>
>+static int loadpin_load_data(enum kernel_load_data_id id)
>+{
>+	int rc = 0;
>+
>+	switch (id) {
>+	case LOADING_MODULE:
>+		rc = loadpin_read_file(NULL, READING_MODULE);
>+	default:
>+		break;
>+	}
>+
>+	return rc;
>+}
>+
> static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
> 	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
> 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
>+	LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
> };
>
> void __init loadpin_add_hooks(void)
>diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>index 02ebd1585eaf..475aed9ee2c7 100644
>--- a/security/selinux/hooks.c
>+++ b/security/selinux/hooks.c
>@@ -4059,6 +4059,20 @@ static int selinux_kernel_read_file(struct file *file,
> 	return rc;
> }
>
>+static int selinux_kernel_load_data(enum kernel_load_data_id id)
>+{
>+	int rc = 0;
>+
>+	switch (id) {
>+	case LOADING_MODULE:
>+		rc = selinux_kernel_module_from_file(NULL);
>+	default:
>+		break;
>+	}
>+
>+	return rc;
>+}
>+
> static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
> {
> 	return avc_has_perm(&selinux_state,
>@@ -6950,6 +6964,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
> 	LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
> 	LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
> 	LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
>+	LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
> 	LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
> 	LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
> 	LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
>-- 
>2.7.5
>

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
                   ` (7 preceding siblings ...)
  2018-05-29 18:02 ` [PATCH v4 8/8] module: replace the existing LSM hook in init_module Mimi Zohar
@ 2018-06-04 14:03 ` Mimi Zohar
  2018-06-04 19:32   ` Serge E. Hallyn
  2018-06-04 22:03   ` Kees Cook
  8 siblings, 2 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-06-04 14:03 UTC (permalink / raw)
  To: Casey Schaufler, James Morris, Kees Cook, Paul Moore, Serge E. Hallyn
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu

On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote:
> Instead of adding the security_kernel_read_file LSM hook - or defining a
> wrapper for security_kernel_read_file LSM hook and adding it, or
> renaming the existing hook to security_kernel_read_data() and adding it
> - in places where the kernel isn't reading a file, this version of the
> patch set defines a new LSM hook named security_kernel_load_data().
> 
> The new LSM hook does not replace the existing security_kernel_read_file
> LSM hook, which is still needed, but defines a new LSM hook allowing
> LSMs and IMA-appraisal the opportunity to fail loading userspace
> provided file/data.
> 
> The only difference between the two LSM hooks is the LSM hook name and a
> file descriptor.  Whether this is cause enough for requiring a new LSM
> hook, is left to the security community.

Paul does not have a preference as to adding a new LSM hook or calling
the existing hook.  Either way is fine, as long as both the new and
existing hooks call the existing function.

Casey didn't like the idea of a wrapper.
James suggested renaming the LSM hook.

The maintainers for the callers of the LSM hook prefer a meaningful
LSM hook name.  The "null" argument is not as much of a concern.  Only
Eric seems to be asking for a separate, new LSM hook, without the
"null" argument.

Unless someone really objects, to accommodate Eric we'll define a new
LSM hook named security_kernel_load_data.  Eric, are you planning on
Ack'ing patches 1 & 2?

Mimi


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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-04 14:03 ` [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
@ 2018-06-04 19:32   ` Serge E. Hallyn
  2018-06-04 19:53     ` Mimi Zohar
  2018-06-04 22:03   ` Kees Cook
  1 sibling, 1 reply; 43+ messages in thread
From: Serge E. Hallyn @ 2018-06-04 19:32 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Casey Schaufler, James Morris, Kees Cook, Paul Moore,
	Serge E. Hallyn, linux-integrity, linux-security-module,
	linux-kernel, David Howells, Luis R . Rodriguez, Eric Biederman,
	kexec, Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel,
	Jessica Yu

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote:
> > Instead of adding the security_kernel_read_file LSM hook - or defining a
> > wrapper for security_kernel_read_file LSM hook and adding it, or
> > renaming the existing hook to security_kernel_read_data() and adding it
> > - in places where the kernel isn't reading a file, this version of the
> > patch set defines a new LSM hook named security_kernel_load_data().
> > 
> > The new LSM hook does not replace the existing security_kernel_read_file
> > LSM hook, which is still needed, but defines a new LSM hook allowing
> > LSMs and IMA-appraisal the opportunity to fail loading userspace
> > provided file/data.
> > 
> > The only difference between the two LSM hooks is the LSM hook name and a
> > file descriptor.  Whether this is cause enough for requiring a new LSM
> > hook, is left to the security community.
> 
> Paul does not have a preference as to adding a new LSM hook or calling
> the existing hook.  Either way is fine, as long as both the new and
> existing hooks call the existing function.
> 
> Casey didn't like the idea of a wrapper.
> James suggested renaming the LSM hook.
> 
> The maintainers for the callers of the LSM hook prefer a meaningful
> LSM hook name.  The "null" argument is not as much of a concern.  Only
> Eric seems to be asking for a separate, new LSM hook, without the
> "null" argument.
> 
> Unless someone really objects, to accommodate Eric we'll define a new
> LSM hook named security_kernel_load_data.  Eric, are you planning on

I'm confused - isn't that what this patchset did? :)

> Ack'ing patches 1 & 2?
> 
> Mimi

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-04 19:32   ` Serge E. Hallyn
@ 2018-06-04 19:53     ` Mimi Zohar
  0 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-06-04 19:53 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Casey Schaufler, James Morris, Kees Cook, Paul Moore,
	linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu

On Mon, 2018-06-04 at 14:32 -0500, Serge E. Hallyn wrote:
> Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> > On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote:
> > > Instead of adding the security_kernel_read_file LSM hook - or defining a
> > > wrapper for security_kernel_read_file LSM hook and adding it, or
> > > renaming the existing hook to security_kernel_read_data() and adding it
> > > - in places where the kernel isn't reading a file, this version of the
> > > patch set defines a new LSM hook named security_kernel_load_data().
> > > 
> > > The new LSM hook does not replace the existing security_kernel_read_file
> > > LSM hook, which is still needed, but defines a new LSM hook allowing
> > > LSMs and IMA-appraisal the opportunity to fail loading userspace
> > > provided file/data.
> > > 
> > > The only difference between the two LSM hooks is the LSM hook name and a
> > > file descriptor.  Whether this is cause enough for requiring a new LSM
> > > hook, is left to the security community.
> > 
> > Paul does not have a preference as to adding a new LSM hook or calling
> > the existing hook.  Either way is fine, as long as both the new and
> > existing hooks call the existing function.
> > 
> > Casey didn't like the idea of a wrapper.
> > James suggested renaming the LSM hook.
> > 
> > The maintainers for the callers of the LSM hook prefer a meaningful
> > LSM hook name.  The "null" argument is not as much of a concern.  Only
> > Eric seems to be asking for a separate, new LSM hook, without the
> > "null" argument.
> > 
> > Unless someone really objects, to accommodate Eric we'll define a new
> > LSM hook named security_kernel_load_data.  Eric, are you planning on
> 
> I'm confused - isn't that what this patchset did? :)

Right.  I'm trying to get consensus whether it is needed.

> 
> > Ack'ing patches 1 & 2?
> > 
> > Mimi
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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

* Re: [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data
  2018-05-29 18:01 ` [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data Mimi Zohar
@ 2018-06-04 19:59   ` Serge E. Hallyn
  0 siblings, 0 replies; 43+ messages in thread
From: Serge E. Hallyn @ 2018-06-04 19:59 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook,
	Casey Schaufler

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> Differentiate between the kernel reading a file from the kernel loading
> data provided by userspace.  This patch defines a new LSM hook named
> security_kernel_load_data.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

Acked-by: Serge Hallyn <serge@hallyn.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 v4:
> - Define new LSM hook named security_kernel_load_data.
> 
> Changelog v3:
> - Rename security_kernel_read_file to security_kernel_read_data().
> 
> 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/lsm_hooks.h |  6 ++++++
>  include/linux/security.h  | 33 +++++++++++++++++++++++++++++++++
>  security/security.c       |  5 +++++
>  3 files changed, 44 insertions(+)
> 
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 8f1131c8dd54..a08bc2587b96 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -576,6 +576,10 @@
>   *	userspace to load a kernel module with the given name.
>   *	@kmod_name name of the module requested by the kernel
>   *	Return 0 if successful.
> + * @kernel_load_data:
> + *	Load data provided by userspace.
> + *	@id kernel load data identifier
> + *	Return 0 if permission is granted.
>   * @kernel_read_file:
>   *	Read a file specified by userspace.
>   *	@file contains the file structure pointing to the file being read
> @@ -1582,6 +1586,7 @@ union security_list_options {
>  	int (*kernel_act_as)(struct cred *new, u32 secid);
>  	int (*kernel_create_files_as)(struct cred *new, struct inode *inode);
>  	int (*kernel_module_request)(char *kmod_name);
> +	int (*kernel_load_data)(enum kernel_load_data_id id);
>  	int (*kernel_read_file)(struct file *file, enum kernel_read_file_id id);
>  	int (*kernel_post_read_file)(struct file *file, char *buf, loff_t size,
>  				     enum kernel_read_file_id id);
> @@ -1872,6 +1877,7 @@ struct security_hook_heads {
>  	struct hlist_head cred_getsecid;
>  	struct hlist_head kernel_act_as;
>  	struct hlist_head kernel_create_files_as;
> +	struct hlist_head kernel_load_data;
>  	struct hlist_head kernel_read_file;
>  	struct hlist_head kernel_post_read_file;
>  	struct hlist_head kernel_module_request;
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 63030c85ee19..3d54d5945755 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -159,6 +159,33 @@ extern int mmap_min_addr_handler(struct ctl_table *table, int write,
>  typedef int (*initxattrs) (struct inode *inode,
>  			   const struct xattr *xattr_array, void *fs_data);
>  
> +
> +#define __kernel_load_data_id(id) \
> +	id(UNKNOWN, unknown)		\
> +	id(FIRMWARE, firmware)		\
> +	id(MODULE, kernel-module)		\
> +	id(KEXEC_IMAGE, kexec-image)		\
> +	id(MAX_ID, )
> +
> +#define __data_id_enumify(ENUM, dummy) LOADING_ ## ENUM,
> +#define __data_id_stringify(dummy, str) #str,
> +
> +enum kernel_load_data_id {
> +	__kernel_load_data_id(__data_id_enumify)
> +};
> +
> +static const char * const kernel_load_data_str[] = {
> +	__kernel_load_data_id(__data_id_stringify)
> +};
> +
> +static inline const char *kernel_load_data_id_str(enum kernel_load_data_id id)
> +{
> +	if ((unsigned)id >= LOADING_MAX_ID)
> +		return kernel_load_data_str[LOADING_UNKNOWN];
> +
> +	return kernel_load_data_str[id];
> +}
> +
>  #ifdef CONFIG_SECURITY
>  
>  struct security_mnt_opts {
> @@ -320,6 +347,7 @@ void security_cred_getsecid(const struct cred *c, u32 *secid);
>  int security_kernel_act_as(struct cred *new, u32 secid);
>  int security_kernel_create_files_as(struct cred *new, struct inode *inode);
>  int security_kernel_module_request(char *kmod_name);
> +int security_kernel_load_data(enum kernel_load_data_id id);
>  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);
> @@ -909,6 +937,11 @@ static inline int security_kernel_module_request(char *kmod_name)
>  	return 0;
>  }
>  
> +static inline int security_kernel_load_data(enum kernel_load_data_id id)
> +{
> +	return 0;
> +}
> +
>  static inline int security_kernel_read_file(struct file *file,
>  					    enum kernel_read_file_id id)
>  {
> diff --git a/security/security.c b/security/security.c
> index 68f46d849abe..c2de2f134854 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1056,6 +1056,11 @@ int security_kernel_post_read_file(struct file *file, char *buf, loff_t size,
>  }
>  EXPORT_SYMBOL_GPL(security_kernel_post_read_file);
>  
> +int security_kernel_load_data(enum kernel_load_data_id id)
> +{
> +	return call_int_hook(kernel_load_data, 0, id);
> +}
> +
>  int security_task_fix_setuid(struct cred *new, const struct cred *old,
>  			     int flags)
>  {
> -- 
> 2.7.5

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

* Re: [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall
  2018-05-29 18:01 ` [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall Mimi Zohar
@ 2018-06-04 20:00   ` Serge E. Hallyn
  0 siblings, 0 replies; 43+ messages in thread
From: Serge E. Hallyn @ 2018-06-04 20:00 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: linux-integrity, linux-security-module, linux-kernel,
	David Howells, Luis R . Rodriguez, Eric Biederman, kexec,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Kees Cook

Quoting Mimi Zohar (zohar@linux.vnet.ibm.com):
> In order for LSMs and IMA-appraisal to differentiate between kexec_load
> and kexec_file_load syscalls, both the original and new syscalls must
> call an LSM hook.  This patch adds a call to security_kernel_load_data()
> in the original kexec_load syscall.
> 
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> Cc: Eric Biederman <ebiederm@xmission.com>

Acked-by: Serge Hallyn <serge@hallyn.com>

> Cc: Luis R. Rodriguez <mcgrof@kernel.org>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: David Howells <dhowells@redhat.com>
> ---
>  kernel/kexec.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index aed8fb2564b3..68559808fdfa 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,10 +196,17 @@ 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;
>  
> +	/* Permit LSMs and IMA to fail the kexec */
> +	result = security_kernel_load_data(LOADING_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

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-04 14:03 ` [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
  2018-06-04 19:32   ` Serge E. Hallyn
@ 2018-06-04 22:03   ` Kees Cook
  2018-06-05  4:09     ` Serge E. Hallyn
  1 sibling, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-04 22:03 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Casey Schaufler, James Morris, Paul Moore, Serge E. Hallyn,
	linux-integrity, linux-security-module, LKML, David Howells,
	Luis R . Rodriguez, Eric Biederman, Kexec Mailing List,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu

On Mon, Jun 4, 2018 at 7:03 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote:
>> Instead of adding the security_kernel_read_file LSM hook - or defining a
>> wrapper for security_kernel_read_file LSM hook and adding it, or
>> renaming the existing hook to security_kernel_read_data() and adding it
>> - in places where the kernel isn't reading a file, this version of the
>> patch set defines a new LSM hook named security_kernel_load_data().
>>
>> The new LSM hook does not replace the existing security_kernel_read_file
>> LSM hook, which is still needed, but defines a new LSM hook allowing
>> LSMs and IMA-appraisal the opportunity to fail loading userspace
>> provided file/data.
>>
>> The only difference between the two LSM hooks is the LSM hook name and a
>> file descriptor.  Whether this is cause enough for requiring a new LSM
>> hook, is left to the security community.
>
> Paul does not have a preference as to adding a new LSM hook or calling
> the existing hook.  Either way is fine, as long as both the new and
> existing hooks call the existing function.
>
> Casey didn't like the idea of a wrapper.
> James suggested renaming the LSM hook.
>
> The maintainers for the callers of the LSM hook prefer a meaningful
> LSM hook name.  The "null" argument is not as much of a concern.  Only
> Eric seems to be asking for a separate, new LSM hook, without the
> "null" argument.
>
> Unless someone really objects, to accommodate Eric we'll define a new
> LSM hook named security_kernel_load_data.  Eric, are you planning on
> Ack'ing patches 1 & 2?

I'm sorry I'm late to review this series. Reading through what you
have, it seems like the existing hook is fine. If the name has
slipped, we can rename it, but I think adding another hook for the
same logical action (loading something into the kernel) is confusing.

It seems that only patches needed are 2 & 4 (new hook callsites), 5, 6
& 7 (IMA coverage and policy). 1 and 8 seem needless to me. If the
objection is that isn't use on non-file objects, sure, rename it. But
I don't see a _logical_ difference between the proposed and existing
callsites. enum kernel_read_file_id covers the "type" already....

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-04 22:03   ` Kees Cook
@ 2018-06-05  4:09     ` Serge E. Hallyn
  2018-06-05 12:19       ` Kees Cook
  0 siblings, 1 reply; 43+ messages in thread
From: Serge E. Hallyn @ 2018-06-05  4:09 UTC (permalink / raw)
  To: Kees Cook
  Cc: Mimi Zohar, Casey Schaufler, James Morris, Paul Moore,
	Serge E. Hallyn, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jessica Yu

Quoting Kees Cook (keescook@chromium.org):
> On Mon, Jun 4, 2018 at 7:03 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2018-05-29 at 14:01 -0400, Mimi Zohar wrote:
> >> Instead of adding the security_kernel_read_file LSM hook - or defining a
> >> wrapper for security_kernel_read_file LSM hook and adding it, or
> >> renaming the existing hook to security_kernel_read_data() and adding it
> >> - in places where the kernel isn't reading a file, this version of the
> >> patch set defines a new LSM hook named security_kernel_load_data().
> >>
> >> The new LSM hook does not replace the existing security_kernel_read_file
> >> LSM hook, which is still needed, but defines a new LSM hook allowing
> >> LSMs and IMA-appraisal the opportunity to fail loading userspace
> >> provided file/data.
> >>
> >> The only difference between the two LSM hooks is the LSM hook name and a
> >> file descriptor.  Whether this is cause enough for requiring a new LSM
> >> hook, is left to the security community.
> >
> > Paul does not have a preference as to adding a new LSM hook or calling
> > the existing hook.  Either way is fine, as long as both the new and
> > existing hooks call the existing function.
> >
> > Casey didn't like the idea of a wrapper.
> > James suggested renaming the LSM hook.
> >
> > The maintainers for the callers of the LSM hook prefer a meaningful
> > LSM hook name.  The "null" argument is not as much of a concern.  Only
> > Eric seems to be asking for a separate, new LSM hook, without the
> > "null" argument.
> >
> > Unless someone really objects, to accommodate Eric we'll define a new
> > LSM hook named security_kernel_load_data.  Eric, are you planning on
> > Ack'ing patches 1 & 2?
> 
> I'm sorry I'm late to review this series. Reading through what you
> have, it seems like the existing hook is fine. If the name has
> slipped, we can rename it, but I think adding another hook for the
> same logical action (loading something into the kernel) is confusing.

Personally I agree with Eric and prefer a new hook.  I don't feel strongly
enough about it to keep bikeshedding, but since this set already exists,
it seems like the way to go.

> It seems that only patches needed are 2 & 4 (new hook callsites), 5, 6
> & 7 (IMA coverage and policy). 1 and 8 seem needless to me. If the
> objection is that isn't use on non-file objects, sure, rename it. But
> I don't see a _logical_ difference between the proposed and existing
> callsites. enum kernel_read_file_id covers the "type" already....
> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-05  4:09     ` Serge E. Hallyn
@ 2018-06-05 12:19       ` Kees Cook
  2018-06-05 13:25         ` Serge E. Hallyn
  0 siblings, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-05 12:19 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Mimi Zohar, Casey Schaufler, James Morris, Paul Moore,
	linux-integrity, linux-security-module, LKML, David Howells,
	Luis R . Rodriguez, Eric Biederman, Kexec Mailing List,
	Andres Rodriguez, Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu

On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Personally I agree with Eric and prefer a new hook.  I don't feel strongly
> enough about it to keep bikeshedding, but since this set already exists,
> it seems like the way to go.

And the new hook is "load stuff without a file descriptor"?

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-05 12:19       ` Kees Cook
@ 2018-06-05 13:25         ` Serge E. Hallyn
  2018-06-05 13:43           ` Kees Cook
  0 siblings, 1 reply; 43+ messages in thread
From: Serge E. Hallyn @ 2018-06-05 13:25 UTC (permalink / raw)
  To: Kees Cook
  Cc: Serge E. Hallyn, Mimi Zohar, Casey Schaufler, James Morris,
	Paul Moore, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jessica Yu

Quoting Kees Cook (keescook@chromium.org):
> On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Personally I agree with Eric and prefer a new hook.  I don't feel strongly
> > enough about it to keep bikeshedding, but since this set already exists,
> > it seems like the way to go.
> 
> And the new hook is "load stuff without a file descriptor"?

Yes.  Load stuff based on my own credentials not those attached
to a file.

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-05 13:25         ` Serge E. Hallyn
@ 2018-06-05 13:43           ` Kees Cook
  2018-06-05 14:05             ` Mimi Zohar
  0 siblings, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-05 13:43 UTC (permalink / raw)
  To: Serge E. Hallyn
  Cc: Mimi Zohar, Casey Schaufler, Paul Moore, linux-integrity,
	linux-security-module, LKML, David Howells, Luis R . Rodriguez,
	Eric Biederman, Kexec Mailing List, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu, James Morris

On Tue, Jun 5, 2018 at 6:25 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> Quoting Kees Cook (keescook@chromium.org):
>> On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
>> > Personally I agree with Eric and prefer a new hook.  I don't feel strongly
>> > enough about it to keep bikeshedding, but since this set already exists,
>> > it seems like the way to go.
>>
>> And the new hook is "load stuff without a file descriptor"?
>
> Yes.  Load stuff based on my own credentials not those attached
> to a file.

Okay, I can live with that. :)

-- 
Kees Cook
Pixel Security

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

* Re: [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures
  2018-06-05 13:43           ` Kees Cook
@ 2018-06-05 14:05             ` Mimi Zohar
  0 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-06-05 14:05 UTC (permalink / raw)
  To: Kees Cook, Serge E. Hallyn
  Cc: Casey Schaufler, Paul Moore, linux-integrity,
	linux-security-module, LKML, David Howells, Luis R . Rodriguez,
	Eric Biederman, Kexec Mailing List, Andres Rodriguez,
	Greg Kroah-Hartman, Ard Biesheuvel, Jessica Yu, James Morris

On Tue, 2018-06-05 at 06:43 -0700, Kees Cook wrote:
> On Tue, Jun 5, 2018 at 6:25 AM, Serge E. Hallyn <serge@hallyn.com> wrote:
> > Quoting Kees Cook (keescook@chromium.org):
> >> On Mon, Jun 4, 2018 at 9:09 PM, Serge E. Hallyn <serge@hallyn.com> wrote:
> >> > Personally I agree with Eric and prefer a new hook.  I don't feel strongly
> >> > enough about it to keep bikeshedding, but since this set already exists,
> >> > it seems like the way to go.
> >>
> >> And the new hook is "load stuff without a file descriptor"?
> >
> > Yes.  Load stuff based on my own credentials not those attached
> > to a file.
> 
> Okay, I can live with that. :)

Can I get your Ack on the loadpin changes in v4a patch 8/8?

Mimi


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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-05-31 15:23         ` [PATCH v4a " Mimi Zohar
  2018-06-01 22:28           ` Paul Moore
  2018-06-04  9:19           ` Jessica Yu
@ 2018-06-05 19:45           ` Kees Cook
  2018-06-05 21:35             ` Mimi Zohar
  2 siblings, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-05 19:45 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Paul Moore, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jeff Vander Stoep, Casey Schaufler

On Thu, May 31, 2018 at 8:23 AM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
> index 5fa191252c8f..a9c07bfbc338 100644
> --- a/security/loadpin/loadpin.c
> +++ b/security/loadpin/loadpin.c
> @@ -173,9 +173,24 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>         return 0;
>  }
>
> +static int loadpin_load_data(enum kernel_load_data_id id)
> +{
> +       int rc = 0;
> +
> +       switch (id) {
> +       case LOADING_MODULE:
> +               rc = loadpin_read_file(NULL, READING_MODULE);
> +       default:
> +               break;
> +       }
> +
> +       return rc;
> +}

Is it worth keeping the same enum between the two hooks? That would
simplify this a bit since it could just pass the id without remapping.

And if you must have a separate enum, please change this to fail
closed instead of open (and mark the fall-through):

int rc = -EPERM;

switch (id) {
case LOADING_MODULE:
    rc = loadpin_read_file(NULL, READING_MODULE);
    /* Fall-through */
default:
    break;
}

Thanks!

-Kees

> +
>  static struct security_hook_list loadpin_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
>         LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
> +       LSM_HOOK_INIT(kernel_load_data, loadpin_load_data),
>  };
>
>  void __init loadpin_add_hooks(void)
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 02ebd1585eaf..475aed9ee2c7 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4059,6 +4059,20 @@ static int selinux_kernel_read_file(struct file *file,
>         return rc;
>  }
>
> +static int selinux_kernel_load_data(enum kernel_load_data_id id)
> +{
> +       int rc = 0;
> +
> +       switch (id) {
> +       case LOADING_MODULE:
> +               rc = selinux_kernel_module_from_file(NULL);
> +       default:
> +               break;
> +       }
> +
> +       return rc;
> +}
> +
>  static int selinux_task_setpgid(struct task_struct *p, pid_t pgid)
>  {
>         return avc_has_perm(&selinux_state,
> @@ -6950,6 +6964,7 @@ static struct security_hook_list selinux_hooks[] __lsm_ro_after_init = {
>         LSM_HOOK_INIT(kernel_act_as, selinux_kernel_act_as),
>         LSM_HOOK_INIT(kernel_create_files_as, selinux_kernel_create_files_as),
>         LSM_HOOK_INIT(kernel_module_request, selinux_kernel_module_request),
> +       LSM_HOOK_INIT(kernel_load_data, selinux_kernel_load_data),
>         LSM_HOOK_INIT(kernel_read_file, selinux_kernel_read_file),
>         LSM_HOOK_INIT(task_setpgid, selinux_task_setpgid),
>         LSM_HOOK_INIT(task_getpgid, selinux_task_getpgid),
> --
> 2.7.5
>



-- 
Kees Cook
Pixel Security

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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-06-05 19:45           ` Kees Cook
@ 2018-06-05 21:35             ` Mimi Zohar
  2018-06-05 22:26               ` Kees Cook
  0 siblings, 1 reply; 43+ messages in thread
From: Mimi Zohar @ 2018-06-05 21:35 UTC (permalink / raw)
  To: Kees Cook
  Cc: Paul Moore, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jeff Vander Stoep, Casey Schaufler

On Tue, 2018-06-05 at 12:45 -0700, Kees Cook wrote:

> And if you must have a separate enum, please change this to fail
> closed instead of open (and mark the fall-through):
> 
> int rc = -EPERM;
> 
> switch (id) {
> case LOADING_MODULE:
>     rc = loadpin_read_file(NULL, READING_MODULE);
>     /* Fall-through */
> default:
>     break;
> }

This will fail the sysfs firmware fallback loading and the kexec_load
syscall without any message, as you have for init_module.  Is that
what you want?

Mimi

 

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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-06-05 21:35             ` Mimi Zohar
@ 2018-06-05 22:26               ` Kees Cook
  2018-06-05 22:40                 ` Mimi Zohar
  0 siblings, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-05 22:26 UTC (permalink / raw)
  To: Mimi Zohar
  Cc: Paul Moore, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jeff Vander Stoep, Casey Schaufler, James Morris

On Tue, Jun 5, 2018 at 2:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Tue, 2018-06-05 at 12:45 -0700, Kees Cook wrote:
>
>> And if you must have a separate enum, please change this to fail
>> closed instead of open (and mark the fall-through):
>>
>> int rc = -EPERM;
>>
>> switch (id) {
>> case LOADING_MODULE:
>>     rc = loadpin_read_file(NULL, READING_MODULE);
>>     /* Fall-through */
>> default:
>>     break;
>> }
>
> This will fail the sysfs firmware fallback loading and the kexec_load
> syscall without any message, as you have for init_module.  Is that
> what you want?

I'd prefer there be a full mapping of the enums so that everything
gets passed into loadpin_read_file() :)

Can the enum be shared or is that nonsensical?

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-06-01 19:25     ` Luis R. Rodriguez
@ 2018-06-05 22:37       ` Kees Cook
  2018-06-06  6:20         ` Ard Biesheuvel
  0 siblings, 1 reply; 43+ messages in thread
From: Kees Cook @ 2018-06-05 22:37 UTC (permalink / raw)
  To: Mimi Zohar, Luis R. Rodriguez
  Cc: Vlastimil Babka, Matthew Garrett, linux-integrity,
	linux-security-module, LKML, David Howells, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Serge E . Hallyn, Stephen Boyd, Laura Abbott,
	Rik van Riel

On Fri, Jun 1, 2018 at 12:25 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
>> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
>> > Some systems are memory constrained but they need to load very large
>> > firmwares.  The firmware subsystem allows drivers to request this
>> > firmware be loaded from the filesystem, but this requires that the
>> > entire firmware be loaded into kernel memory first before it's provided
>> > to the driver.  This can lead to a situation where we map the firmware
>> > twice, once to load the firmware into kernel memory and once to copy the
>> > firmware into the final resting place.
>> >
>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
>> > that allows drivers to request firmware be loaded directly into a
>> > pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
>> > allocate this buffer.  According to Documentation/DMA-API.txt,
>> >
>> >      Consistent memory is memory for which a write by either the
>> >      device or the processor can immediately be read by the processor
>> >      or device without having to worry about caching effects.  (You
>> >      may however need to make sure to flush the processor's write
>> >      buffers before telling devices to read that memory.)
>> >
>> > Devices using pre-allocated DMA memory run the risk of the firmware
>> > being accessible by the device prior to the kernel's firmware signature
>> > verification has completed.
>>
>> Indeed. And since its DMA memory we have *no idea* what can happen in
>> terms of consumption of this firmware from hardware, when it would start
>> consuming it in particular.
>>
>> If the device has its own hardware firmware verification mechanism this is
>> completely obscure to us, but it may however suffice certain security policies.
>>
>> The problem here lies in the conflicting security policies of the kernel wanting
>> to not give away firmware until its complete and the current inability to enable
>> us to have platforms suggest they trust hardware won't do something stupid.
>> This becomes an issue since the semantics of the firmware API preallocated
>> buffer do not require currently allow the kernel to inform LSMs of the fact
>> that a buffer is DMA memory or not, and a way for certain platforms then
>> to say that such use is fine for specific devices.
>>
>> Given a pointer can we determine if a piece of memory is DMA or not?
>
> FWIW
>
> Vlastimil suggests page_zone() or virt_to_page() may be able to.

I don't see a PAGEFLAG for DMA, but I do see ZONE_DMA for
page_zone()... So maybe something like

struct page *page;

page = virt_to_page(address);
if (!page)
   fail closed...
if (page_zone(page) == ZONE_DMA)
    handle dma case...
else
    non-dma

But I've CCed Laura and Rik, who I always lean on when I have these
kinds of page questions...

-Kees

-- 
Kees Cook
Pixel Security

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

* Re: [PATCH v4a 8/8] module: replace the existing LSM hook in init_module
  2018-06-05 22:26               ` Kees Cook
@ 2018-06-05 22:40                 ` Mimi Zohar
  0 siblings, 0 replies; 43+ messages in thread
From: Mimi Zohar @ 2018-06-05 22:40 UTC (permalink / raw)
  To: Kees Cook
  Cc: Paul Moore, linux-integrity, linux-security-module, LKML,
	David Howells, Luis R . Rodriguez, Eric Biederman,
	Kexec Mailing List, Andres Rodriguez, Greg Kroah-Hartman,
	Ard Biesheuvel, Jeff Vander Stoep, Casey Schaufler, James Morris

On Tue, 2018-06-05 at 15:26 -0700, Kees Cook wrote:
> On Tue, Jun 5, 2018 at 2:35 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Tue, 2018-06-05 at 12:45 -0700, Kees Cook wrote:
> >
> >> And if you must have a separate enum, please change this to fail
> >> closed instead of open (and mark the fall-through):
> >>
> >> int rc = -EPERM;
> >>
> >> switch (id) {
> >> case LOADING_MODULE:
> >>     rc = loadpin_read_file(NULL, READING_MODULE);
> >>     /* Fall-through */
> >> default:
> >>     break;
> >> }
> >
> > This will fail the sysfs firmware fallback loading and the kexec_load
> > syscall without any message, as you have for init_module.  Is that
> > what you want?
> 
> I'd prefer there be a full mapping of the enums so that everything
> gets passed into loadpin_read_file() :)
> 
> Can the enum be shared or is that nonsensical?

Considering this is v4 of the patch set, it's pretty obvious I did
everything possible not to define a new LSM hook.  Even if we can't
re-use the existing enum, we could define the new enum in terms
of __kernel_read_file_id.

enum kernel_load_data_id {
        __kernel_read_file_id(__data_id_enumify)
};

static const char * const kernel_load_data_str[] = {
        __kernel_read_file_id(__data_id_stringify)
};

Eric, Serge, would using either the existing __kernel_read_file_id
enum or the above definitions be acceptable?

Mimi

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

* Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-06-05 22:37       ` Kees Cook
@ 2018-06-06  6:20         ` Ard Biesheuvel
  2018-06-06 22:06           ` Luis R. Rodriguez
  0 siblings, 1 reply; 43+ messages in thread
From: Ard Biesheuvel @ 2018-06-06  6:20 UTC (permalink / raw)
  To: Kees Cook
  Cc: Mimi Zohar, Luis R. Rodriguez, Vlastimil Babka, Matthew Garrett,
	linux-integrity, linux-security-module, LKML, David Howells,
	Eric Biederman, Kexec Mailing List, Andres Rodriguez,
	Greg Kroah-Hartman, Serge E . Hallyn, Stephen Boyd, Laura Abbott,
	Rik van Riel

On 6 June 2018 at 00:37, Kees Cook <keescook@chromium.org> wrote:
> On Fri, Jun 1, 2018 at 12:25 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
>> On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
>>> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
>>> > Some systems are memory constrained but they need to load very large
>>> > firmwares.  The firmware subsystem allows drivers to request this
>>> > firmware be loaded from the filesystem, but this requires that the
>>> > entire firmware be loaded into kernel memory first before it's provided
>>> > to the driver.  This can lead to a situation where we map the firmware
>>> > twice, once to load the firmware into kernel memory and once to copy the
>>> > firmware into the final resting place.
>>> >
>>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
>>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
>>> > that allows drivers to request firmware be loaded directly into a
>>> > pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
>>> > allocate this buffer.  According to Documentation/DMA-API.txt,
>>> >
>>> >      Consistent memory is memory for which a write by either the
>>> >      device or the processor can immediately be read by the processor
>>> >      or device without having to worry about caching effects.  (You
>>> >      may however need to make sure to flush the processor's write
>>> >      buffers before telling devices to read that memory.)
>>> >
>>> > Devices using pre-allocated DMA memory run the risk of the firmware
>>> > being accessible by the device prior to the kernel's firmware signature
>>> > verification has completed.
>>>
>>> Indeed. And since its DMA memory we have *no idea* what can happen in
>>> terms of consumption of this firmware from hardware, when it would start
>>> consuming it in particular.
>>>
>>> If the device has its own hardware firmware verification mechanism this is
>>> completely obscure to us, but it may however suffice certain security policies.
>>>
>>> The problem here lies in the conflicting security policies of the kernel wanting
>>> to not give away firmware until its complete and the current inability to enable
>>> us to have platforms suggest they trust hardware won't do something stupid.
>>> This becomes an issue since the semantics of the firmware API preallocated
>>> buffer do not require currently allow the kernel to inform LSMs of the fact
>>> that a buffer is DMA memory or not, and a way for certain platforms then
>>> to say that such use is fine for specific devices.
>>>
>>> Given a pointer can we determine if a piece of memory is DMA or not?
>>
>> FWIW
>>
>> Vlastimil suggests page_zone() or virt_to_page() may be able to.
>
> I don't see a PAGEFLAG for DMA, but I do see ZONE_DMA for
> page_zone()... So maybe something like
>
> struct page *page;
>
> page = virt_to_page(address);
> if (!page)
>    fail closed...
> if (page_zone(page) == ZONE_DMA)
>     handle dma case...
> else
>     non-dma
>
> But I've CCed Laura and Rik, who I always lean on when I have these
> kinds of page questions...
>

That is not going to help. In general, DMA can access any memory in
the system (unless a IOMMU is actively preventing that).

The streaming DMA API allows you to map()/unmap() arbitrary pieces of
memory for DMA, regardless of how they were allocated. (Some drivers
were even doing DMA from the stack at some point, but this broke
vmapped stacks so most of these cases have been fixed) Uploading
firmware to a device does not require a coherent (as opposed to
streaming) mapping for DMA, and so it is perfectly reasonable for a
driver to use the streaming API to map the firmware image (wherever it
is in memory) and map it.

However, the DMA API does impose some ordering. Mapping memory for DMA
gives you a DMA address (which may be different from the physical
address [depending on the platform]), and this DMA address is what
gets programmed into the device, not the virtual or physical address.
That means you can be reasonably confident that the device will not be
able to consume what is in this memory before it has been mapped for
DMA. Also, the DMA api explicitly forbids touching memory mapped for
streaming DMA: the device owns it at this point, and so the CPU should
refrain from accessing it.

So the question is, why is QCOM_MDT_LOADER using a coherent DMA
mapping? That does not make any sense purely for moving firmware into
the device, and it is indeed a security hazard if we are trying to
perform a signature check before the device is cleared for reading it.

Note that qcom_scm_pas_init_image() is documented as

        /*
         * During the scm call memory protection will be enabled for the meta
         * data blob, so make sure it's physically contiguous, 4K aligned and
         * non-cachable to avoid XPU violations.
         */

and dma_alloc_coherent() happens to give them that. Whether the DMA
mapping is actually used is a different matter: the code is a bit
complex, but it calls into the secure world to set up the region.

If this is the only counterexample, I wouldn't worry about it too much
(QCOM have elaborate SoC management layers in the secure world), and
simply mandate that only streaming DMA be used for firmware loading,
and that the firmware signature verification is performed before the
memory is mapped for DMA.

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

* Re: [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer)
  2018-06-06  6:20         ` Ard Biesheuvel
@ 2018-06-06 22:06           ` Luis R. Rodriguez
  0 siblings, 0 replies; 43+ messages in thread
From: Luis R. Rodriguez @ 2018-06-06 22:06 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: Kees Cook, Mimi Zohar, Luis R. Rodriguez, Vlastimil Babka,
	Matthew Garrett, linux-integrity, linux-security-module, LKML,
	David Howells, Eric Biederman, Kexec Mailing List,
	Andres Rodriguez, Greg Kroah-Hartman, Serge E . Hallyn,
	Stephen Boyd, Laura Abbott, Rik van Riel

On Wed, Jun 06, 2018 at 08:20:17AM +0200, Ard Biesheuvel wrote:
> On 6 June 2018 at 00:37, Kees Cook <keescook@chromium.org> wrote:
> > On Fri, Jun 1, 2018 at 12:25 PM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> >> On Fri, Jun 01, 2018 at 09:15:45PM +0200, Luis R. Rodriguez wrote:
> >>> On Tue, May 29, 2018 at 02:01:59PM -0400, Mimi Zohar wrote:
> >>> > Some systems are memory constrained but they need to load very large
> >>> > firmwares.  The firmware subsystem allows drivers to request this
> >>> > firmware be loaded from the filesystem, but this requires that the
> >>> > entire firmware be loaded into kernel memory first before it's provided
> >>> > to the driver.  This can lead to a situation where we map the firmware
> >>> > twice, once to load the firmware into kernel memory and once to copy the
> >>> > firmware into the final resting place.
> >>> >
> >>> > To resolve this problem, commit a098ecd2fa7d ("firmware: support loading
> >>> > into a pre-allocated buffer") introduced request_firmware_into_buf() API
> >>> > that allows drivers to request firmware be loaded directly into a
> >>> > pre-allocated buffer.  The QCOM_MDT_LOADER calls dma_alloc_coherent() to
> >>> > allocate this buffer.  According to Documentation/DMA-API.txt,
> >>> >
> >>> >      Consistent memory is memory for which a write by either the
> >>> >      device or the processor can immediately be read by the processor
> >>> >      or device without having to worry about caching effects.  (You
> >>> >      may however need to make sure to flush the processor's write
> >>> >      buffers before telling devices to read that memory.)
> >>> >
> >>> > Devices using pre-allocated DMA memory run the risk of the firmware
> >>> > being accessible by the device prior to the kernel's firmware signature
> >>> > verification has completed.
> >>>
> >>> Indeed. And since its DMA memory we have *no idea* what can happen in
> >>> terms of consumption of this firmware from hardware, when it would start
> >>> consuming it in particular.
> >>>
> >>> If the device has its own hardware firmware verification mechanism this is
> >>> completely obscure to us, but it may however suffice certain security policies.
> >>>
> >>> The problem here lies in the conflicting security policies of the kernel wanting
> >>> to not give away firmware until its complete and the current inability to enable
> >>> us to have platforms suggest they trust hardware won't do something stupid.
> >>> This becomes an issue since the semantics of the firmware API preallocated
> >>> buffer do not require currently allow the kernel to inform LSMs of the fact
> >>> that a buffer is DMA memory or not, and a way for certain platforms then
> >>> to say that such use is fine for specific devices.
> >>>
> >>> Given a pointer can we determine if a piece of memory is DMA or not?
> >>
> >> FWIW
> >>
> >> Vlastimil suggests page_zone() or virt_to_page() may be able to.
> >
> > I don't see a PAGEFLAG for DMA, but I do see ZONE_DMA for
> > page_zone()... So maybe something like
> >
> > struct page *page;
> >
> > page = virt_to_page(address);
> > if (!page)
> >    fail closed...
> > if (page_zone(page) == ZONE_DMA)
> >     handle dma case...
> > else
> >     non-dma
> >
> > But I've CCed Laura and Rik, who I always lean on when I have these
> > kinds of page questions...
> >
> 
> That is not going to help. In general, DMA can access any memory in
> the system (unless a IOMMU is actively preventing that).
> 
> The streaming DMA API allows you to map()/unmap() arbitrary pieces of
> memory for DMA, regardless of how they were allocated. (Some drivers
> were even doing DMA from the stack at some point, but this broke
> vmapped stacks so most of these cases have been fixed) Uploading
> firmware to a device does not require a coherent (as opposed to
> streaming) mapping for DMA, and so it is perfectly reasonable for a
> driver to use the streaming API to map the firmware image (wherever it
> is in memory) and map it.

This is useful thanks!

But let's keep in mind that this isn't about whether or not this should be
done. This is about informing security layers to make a choice to decide
whether or not what a solution is doing is banana crazy or not.

> However, the DMA API does impose some ordering. Mapping memory for DMA
> gives you a DMA address (which may be different from the physical
> address [depending on the platform]), and this DMA address is what
> gets programmed into the device, not the virtual or physical address.
> That means you can be reasonably confident that the device will not be
> able to consume what is in this memory before it has been mapped for
> DMA.

Again, not for coherent DMA. By *definition* the device *and* processor has
immediate access to data written *immediately* when dma_alloc_coherent() is
used. And that is what was used on the qcom drivers last we checked. The call
sequence is:

qcom_mdt_load() -> qcom_scm_pas_init_image() -> dma_alloc_coherent()

> Also, the DMA api explicitly forbids touching memory mapped for
> streaming DMA: the device owns it at this point, and so the CPU should
> refrain from accessing it.

Wonderful. I note you said *should*. And again, its up to LSMs to decide
if that is not good enough.

> So the question is, why is QCOM_MDT_LOADER using a coherent DMA
> mapping?

Right and the qcom driver makes it very difficult to decipher and verify.
*And* when folks were poked about this during patch review it was clearly
stated that the READING ID should annotate DMA if it was DMA so LSMs
could be informed.

> That does not make any sense purely for moving firmware into
> the device, and it is indeed a security hazard if we are trying to
> perform a signature check before the device is cleared for reading it.

:)

> Note that qcom_scm_pas_init_image() is documented as
> 
>         /*
>          * During the scm call memory protection will be enabled for the meta
>          * data blob, so make sure it's physically contiguous, 4K aligned and
>          * non-cachable to avoid XPU violations.
>          */
> 
> and dma_alloc_coherent() happens to give them that. Whether the DMA
> mapping is actually used is a different matter: the code is a bit
> complex, but it calls into the secure world to set up the region.

Again, it doesn't matter whether they think its OK, that's fine, it is just
up to the LSMs to then decide on their own if this is pure crap.

> If this is the only counterexample,

The problem is this is the *only* user of request_firmware_into_buf()!

> I wouldn't worry about it too much
> (QCOM have elaborate SoC management layers in the secure world), and
> simply mandate that only streaming DMA be used for firmware loading,
> and that the firmware signature verification is performed before the
> memory is mapped for DMA.

Again, it doesn't matter what they *think*. LSMs want the ability to
decide as well and its fair for them to want to differentiate coherent
DMA and say this is not reasonably trustworthy.

  Luis

-- 
Do not panic

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

end of thread, other threads:[~2018-06-06 22:06 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-29 18:01 [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
2018-05-29 18:01 ` [PATCH v4 1/8] security: define new LSM hook named security_kernel_load_data Mimi Zohar
2018-06-04 19:59   ` Serge E. Hallyn
2018-05-29 18:01 ` [PATCH v4 2/8] kexec: add call to LSM hook in original kexec_load syscall Mimi Zohar
2018-06-04 20:00   ` Serge E. Hallyn
2018-05-29 18:01 ` [PATCH v4 3/8] ima: based on policy require signed kexec kernel images Mimi Zohar
2018-05-29 18:01 ` [PATCH v4 4/8] firmware: add call to LSM hook before firmware sysfs fallback Mimi Zohar
2018-06-01 18:19   ` Luis R. Rodriguez
2018-05-29 18:01 ` [PATCH v4 5/8] ima: based on policy require signed firmware (sysfs fallback) Mimi Zohar
2018-06-01 18:21   ` Luis R. Rodriguez
2018-06-01 22:39     ` Mimi Zohar
2018-06-01 22:46       ` Luis R. Rodriguez
2018-06-01 23:04         ` Mimi Zohar
2018-05-29 18:01 ` [PATCH v4 6/8] ima: add build time policy Mimi Zohar
2018-05-29 18:01 ` [RFC PATCH v4 7/8] ima: based on policy prevent loading firmware (pre-allocated buffer) Mimi Zohar
2018-06-01 19:15   ` Luis R. Rodriguez
2018-06-01 19:25     ` Luis R. Rodriguez
2018-06-05 22:37       ` Kees Cook
2018-06-06  6:20         ` Ard Biesheuvel
2018-06-06 22:06           ` Luis R. Rodriguez
2018-05-29 18:02 ` [PATCH v4 8/8] module: replace the existing LSM hook in init_module Mimi Zohar
2018-05-29 22:39   ` Paul Moore
2018-05-29 23:14     ` Mimi Zohar
2018-05-30 21:00       ` Paul Moore
2018-05-31 15:23         ` [PATCH v4a " Mimi Zohar
2018-06-01 22:28           ` Paul Moore
2018-06-04  9:19           ` Jessica Yu
2018-06-05 19:45           ` Kees Cook
2018-06-05 21:35             ` Mimi Zohar
2018-06-05 22:26               ` Kees Cook
2018-06-05 22:40                 ` Mimi Zohar
2018-05-29 23:25     ` [PATCH v4 " Mimi Zohar
2018-05-30  2:25     ` Eric W. Biederman
2018-05-30 21:09       ` Paul Moore
2018-06-04 14:03 ` [PATCH v4 0/8] kexec/firmware: support system wide policy requiring signatures Mimi Zohar
2018-06-04 19:32   ` Serge E. Hallyn
2018-06-04 19:53     ` Mimi Zohar
2018-06-04 22:03   ` Kees Cook
2018-06-05  4:09     ` Serge E. Hallyn
2018-06-05 12:19       ` Kees Cook
2018-06-05 13:25         ` Serge E. Hallyn
2018-06-05 13:43           ` Kees Cook
2018-06-05 14:05             ` Mimi Zohar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).