Message-ID: <87po1k2304.fsf@xmission.com>
Date:   Thu, 24 May 2018 15:49:15 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Mimi Zohar <zohar@...ux.vnet.ibm.com>
Cc:     linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org,
        linux-kernel@...r.kernel.org, David Howells <dhowells@...hat.com>,
        "Luis R . Rodriguez" <mcgrof@...nel.org>,
        kexec@...ts.infradead.org, Andres Rodriguez <andresx7@...il.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Kees Cook <keescook@...omium.org>,
        Casey Schaufler <casey@...aufler-ca.com>
Subject: Re: [PATCH v3 1/7] security: rename security_kernel_read_file() hook


I already nacked this approach because the two cases don't
share a bit of code.  When I looked closer it was even crazier.

The way ima uses this hook and the post_load hook today is a travesty.

The way the security_kernel_file_read and security_kernel_file_post_read
are called today and are used by ima don't make the least little bit of
sense.

Abusing security_kernel_file_read in the module loader and then abusing
security_kernel_file_post_read in the firmware loader is insane.  The
loadpin lsm could not even figure this out and so it failed to work
because of these shenanighans.

Only implementing kernel_file_read to handle the !file case is pretty
much insane.   There is no way this should be expanded to cover kexec
until the code actually makes sense.  We need a maintainable kernel.

Below is where I suggest you start on sorting out these security hooks.
- Adding a security_kernel_arg to catch when you want to allow/deny the
  use of an argument to a syscall.  What security_kernel_file_read and
  security_kernel_file_post_read have been abused for.

- Removing ima_file_read because it is completely subsumed by the new
  call.

- Please note with adding this new hook there is no code shared between
  the cases, and the lsm code becomes simpler shorter when it can assume
  security_kernel_file_post_read always takes a struct file.  (Even with
  the addition of a new security hook).

Eric

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 358354148dec..04536ff81bd2 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -294,9 +294,7 @@ static ssize_t firmware_loading_store(struct device *dev,
 				dev_err(dev, "%s: map pages failed\n",
 					__func__);
 			else
-				rc = security_kernel_post_read_file(NULL,
-						fw_priv->data, fw_priv->size,
-						READING_FIRMWARE);
+				rc = security_kernel_arg(KARG_FIRMWARE);
 
 			/*
 			 * Same logic as fw_load_abort, only the DONE bit
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 0e4647e0eb60..9fb42736ba29 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,7 +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_read_file(struct file *file, enum kernel_read_file_id id);
+extern int ima_kernel_arg(enum kernel_arg_id id);
 extern int ima_post_read_file(struct file *file, void *buf, loff_t size,
 			      enum kernel_read_file_id id);
 extern void ima_post_path_mknod(struct dentry *dentry);
@@ -49,7 +50,7 @@ static inline int ima_file_mmap(struct file *file, unsigned long prot)
 	return 0;
 }
 
-static inline int ima_read_file(struct file *file, enum kernel_read_file_id id)
+static inline int ima_kernel_arg(enum kernel_arg_id id)
 {
 	return 0;
 }
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index 9d0b286f3dba..7f8bc3030784 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_arg:
+ *	Use a syscall argument
+ *	@id kernel argument 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
@@ -1577,6 +1581,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_arg)(enum kernel_arg_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);
@@ -1866,6 +1871,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_arg;
 	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 200920f521a1..6cf1bd87f041 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -159,6 +159,32 @@ 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_arg_id(id) \
+	id(UNKNOWN, unknown)		\
+	id(FIRMWARE, firmware)		\
+	id(MODULE, kernel-module)	\
+	id(MAX_ID, )
+
+#define __karg_enumify(ENUM, dummy) KARG_ ## ENUM,
+#define __karg_stringify(dummy, str) #str,
+
+enum kernel_arg_id {
+	__kernel_arg_id(__karg_enumify)
+};
+
+static const char * const kernel_arg_str[] = {
+	__kernel_arg_id(__karg_stringify)
+};
+
+static inline const char *kernel_arg_id_str(enum kernel_arg_id id)
+{
+	if ((unsigned)id >= KARG_MAX_ID)
+		return kernel_arg_str[KARG_UNKNOWN];
+
+	return kernel_arg_str[id];
+}
+
+
 #ifdef CONFIG_SECURITY
 
 struct security_mnt_opts {
@@ -326,6 +352,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_arg(enum kernel_arg_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);
@@ -923,6 +950,11 @@ static inline int security_kernel_module_request(char *kmod_name)
 	return 0;
 }
 
+static inline int security_kernel_arg(enum kernel_arg_id id)
+{
+	return 0;
+}
+
 static inline int security_kernel_read_file(struct file *file,
 					    enum kernel_read_file_id id)
 {
diff --git a/kernel/module.c b/kernel/module.c
index ce8066b88178..03a1dd21ad4a 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_arg(KARG_MODULE);
 	if (err)
 		return err;
 
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 74d0bd7e76d7..d51a8ca97238 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -421,32 +421,6 @@ void ima_post_path_mknod(struct dentry *dentry)
 		iint->flags |= IMA_NEW_FILE;
 }
 
-/**
- * ima_read_file - pre-measure/appraise hook decision based on policy
- * @file: pointer to the file to be measured/appraised/audit
- * @read_id: caller identifier
- *
- * Permit reading a file based on policy. The policy rules are written
- * in terms of the policy identifier.  Appraising the integrity of
- * a file requires a file descriptor.
- *
- * For permission return 0, otherwise return -EACCES.
- */
-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 */
-	}
-	return 0;
-}
-
 static int read_idmap[READING_MAX_ID] = {
 	[READING_FIRMWARE] = FIRMWARE_CHECK,
 	[READING_MODULE] = MODULE_CHECK,
@@ -474,21 +448,7 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 	enum ima_hooks func;
 	u32 secid;
 
-	if (!file && read_id == READING_FIRMWARE) {
-		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
-		    (ima_appraise & IMA_APPRAISE_ENFORCE))
-			return -EACCES;	/* INTEGRITY_UNKNOWN */
-		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;
-
-	if (!file || !buf || size == 0) { /* should never happen */
+	if (!buf || size == 0) { /* should never happen */
 		if (ima_appraise & IMA_APPRAISE_ENFORCE)
 			return -EACCES;
 		return 0;
@@ -500,6 +460,40 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
 				   MAY_READ, func, 0);
 }
 
+/**
+ * ima_kernel_arg - pre-measure/appraise hook decision based on policy
+ * @id: caller identifier
+ *
+ * Permit using an argument to a syscall based on policy. The policy
+ * rules are written in terms of the policy identifier.
+ *
+ * For permission return 0, otherwise return -EACCES.
+ */
+int ima_kernel_arg(enum kernel_arg_id id)
+{
+	if (id == KARG_MODULE) {
+		bool sig_enforce = is_module_sig_enforced();
+		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 */
+	}
+	else if (id == KARG_FIRMWARE) {
+		if ((ima_appraise & IMA_APPRAISE_FIRMWARE) &&
+		    (ima_appraise & IMA_APPRAISE_ENFORCE))
+			return -EACCES;	/* INTEGRITY_UNKNOWN */
+		return 0;
+	}
+	else {
+		if (ima_appraise & IMA_APPRAISE_ENFORCE)
+			return -EACCES;
+		return 0;
+	}
+	return 0;
+}
+
 static int __init init_ima(void)
 {
 	int error;
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
index 5fa191252c8f..f5333e5abac9 100644
--- a/security/loadpin/loadpin.c
+++ b/security/loadpin/loadpin.c
@@ -121,23 +121,24 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb)
 	}
 }
 
-static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+static int loadpin_read_data(enum kernel_read_data_id id)
 {
-	struct super_block *load_root;
-	const char *origin = kernel_read_file_id_str(id);
+	const char *origin = kernel_arg_id_str(id);
 
 	/* This handles the older init_module API that has a NULL file. */
-	if (!file) {
-		if (!enabled) {
-			report_load(origin, NULL, "old-api-pinning-ignored");
-			return 0;
-		}
-
-		report_load(origin, NULL, "old-api-denied");
-		return -EPERM;
+	if (!enabled) {
+		report_load(origin, NULL, "old-api-pinning-ignored");
+		return 0;
 	}
 
-	load_root = file->f_path.mnt->mnt_sb;
+	report_load(origin, NULL, "old-api-denied");
+	return -EPERM;
+}
+
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+{
+	struct super_block *load_root;
+	const char *origin = kernel_read_file_id_str(id);
 
 	/* First loaded module/firmware defines the root for all others. */
 	spin_lock(&pinned_root_spinlock);
@@ -175,6 +176,7 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
 
 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_data, loadpin_read_data),
 	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
 };
 
diff --git a/security/security.c b/security/security.c
index 7bc2fde023a7..9b5f43c24ee2 100644
--- a/security/security.c
+++ b/security/security.c
@@ -1033,14 +1033,19 @@ int security_kernel_module_request(char *kmod_name)
 	return call_int_hook(kernel_module_request, 0, kmod_name);
 }
 
-int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
+int security_kernel_arg(enum kernel_arg_id id)
 {
 	int ret;
 
-	ret = call_int_hook(kernel_read_file, 0, file, id);
-	if (ret)
-		return ret;
-	return ima_read_file(file, id);
+	ret = call_int_hook(kernel_arg, 0, id);
+	if (!ret)
+		ret = ima_kernel_arg(id);
+	return ret;
+}
+
+int security_kernel_read_file(struct file *file, enum kernel_read_file_id id)
+{
+	return call_int_hook(kernel_read_file, 0, file, id);
 }
 EXPORT_SYMBOL_GPL(security_kernel_read_file);
 
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 4cafe6a19167..76843099fed6 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -4010,6 +4010,15 @@ static int selinux_kernel_module_request(char *kmod_name)
 			    SYSTEM__MODULE_REQUEST, &ad);
 }
 
+static int selinux_kernel_module_arg(void)
+{
+	/* init_module */
+	u32 sid = current_sid();
+	return avc_has_perm(&selinux_state,
+			    sid, sid, SECCLASS_SYSTEM,
+			    SYSTEM__MODULE_LOAD, NULL);
+}
+
 static int selinux_kernel_module_from_file(struct file *file)
 {
 	struct common_audit_data ad;
@@ -4018,12 +4027,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 +4046,20 @@ static int selinux_kernel_module_from_file(struct file *file)
 				SYSTEM__MODULE_LOAD, &ad);
 }
 
+static int selinux_kernel_arg(enum kernel_arg_id id)
+{
+	int rc = 0;
+
+	switch (id) {
+	case KARG_MODULE:
+		rc = selinux_kernel_module_arg();
+		break;
+	default:
+		break;
+	}
+	return rc;
+}
+
 static int selinux_kernel_read_file(struct file *file,
 				    enum kernel_read_file_id id)
 {
@@ -6938,6 +6955,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_arg, selinux_kernel_arg),
 	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),

Powered by blists - more mailing lists