[PATCH v2 0/5] LSM: LoadPin for kernel file loading restrictions

[PATCH v2 0/5] LSM: LoadPin for kernel file loading restrictions

[Kees Cook]

This provides the mini-LSM "loadpin" that intercepts the now consolidated
kernel_file_read LSM hook so that a system can keep all loads coming from
a single trusted filesystem. This is what Chrome OS uses to pin kernel
module and firmware loading to the read-only crypto-verified dm-verity
partition so that kernel module signing is not needed.

-Kees

v2:
- break out utility helpers into separate functions
- have Yama use new helpers too

[PATCH v2 1/5] string_helpers: add kstrdup_quotable

[Kees Cook]

Handle allocating and escaping a string safe for logging.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/string_helpers.h |  2 ++
 lib/string_helpers.c           | 28 ++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index dabe643eb5fa..10f535bc9bb5 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -68,4 +68,6 @@ static inline int string_escape_str_any_np(const char *src, char *dst,
 	return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, only);
 }
 
+char *kstrdup_quotable(char *src);
+
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 5c88204b6f1f..9b07c696d709 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -10,6 +10,7 @@
 #include <linux/export.h>
 #include <linux/ctype.h>
 #include <linux/errno.h>
+#include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/string_helpers.h>
 
@@ -534,3 +535,30 @@ int string_escape_mem(const char *src, size_t isz, char *dst, size_t osz,
 	return p - dst;
 }
 EXPORT_SYMBOL(string_escape_mem);
+
+/*
+ * Return an allocated string that has been escaped of special characters
+ * and double quotes, making it safe to log in quotes.
+ */
+char *kstrdup_quotable(char *src)
+{
+	size_t slen, dlen;
+	char *dst;
+	const int flags = ESCAPE_HEX;
+	const char esc[] = "\f\n\r\t\v\a\e\\\"";
+
+	if (!src)
+		return NULL;
+	slen = strlen(src);
+
+	dlen = string_escape_mem(src, slen, NULL, 0, flags, esc);
+	dst = kmalloc(dlen + 1, GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	BUG_ON(string_escape_mem(src, slen, dst, dlen, flags, esc) != dlen);
+	dst[dlen] = '\0';
+
+	return dst;
+}
+EXPORT_SYMBOL_GPL(kstrdup_quotable);
-- 
2.6.3

[PATCH v2 2/5] string_helpers: add kstrdup_quotable_cmdline

[Kees Cook]

Provide an escaped (but readable: no inter-argument NULLs) commandline
safe for logging.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/string_helpers.h |  1 +
 lib/string_helpers.c           | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index 10f535bc9bb5..ee1e8f701eb1 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -69,5 +69,6 @@ static inline int string_escape_str_any_np(const char *src, char *dst,
 }
 
 char *kstrdup_quotable(char *src);
+char *kstrdup_quotable_cmdline(struct task_struct *task);
 
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 9b07c696d709..142bed739fed 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -10,6 +10,7 @@
 #include <linux/export.h>
 #include <linux/ctype.h>
 #include <linux/errno.h>
+#include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/string_helpers.h>
@@ -562,3 +563,37 @@ char *kstrdup_quotable(char *src)
 	return dst;
 }
 EXPORT_SYMBOL_GPL(kstrdup_quotable);
+
+/*
+ * Returns allocated NULL-terminated string containing process
+ * command line, with inter-argument NULLs replaced with spaces,
+ * and other special characters escaped.
+ */
+char *kstrdup_quotable_cmdline(struct task_struct *task)
+{
+	char *buffer, *quoted;
+	int i, res;
+
+	buffer = kmalloc(PAGE_SIZE, GFP_TEMPORARY);
+	if (!buffer)
+		return NULL;
+
+	res = get_cmdline(task, buffer, PAGE_SIZE - 1);
+	buffer[res] = '\0';
+
+	/* Collapse trailing NULLs. */
+	for (; res > 0; res--)
+		if (buffer[res-1] != '\0')
+			break;
+
+	/* Replace inter-argument NULLs. */
+	for (i = 0; i < res; i++)
+		if (buffer[i] == '\0')
+			buffer[i] = ' ';
+
+	/* Make sure result is printable. */
+	quoted = kstrdup_quotable(buffer);
+	kfree(buffer);
+	return quoted;
+}
+EXPORT_SYMBOL_GPL(kstrdup_quotable_cmdline);
-- 
2.6.3

[PATCH v2 3/5] string_helpers: add kstrdup_quotable_file

[Kees Cook]

Allocate a NULL-terminated file path with special characters escaped,
safe for logging.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/string_helpers.h |  3 +++
 lib/string_helpers.c           | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
index ee1e8f701eb1..63f06745c36d 100644
--- a/include/linux/string_helpers.h
+++ b/include/linux/string_helpers.h
@@ -3,6 +3,8 @@
 
 #include <linux/types.h>
 
+struct file;
+
 /* Descriptions of the types of units to
  * print in */
 enum string_size_units {
@@ -70,5 +72,6 @@ static inline int string_escape_str_any_np(const char *src, char *dst,
 
 char *kstrdup_quotable(char *src);
 char *kstrdup_quotable_cmdline(struct task_struct *task);
+char *kstrdup_quotable_file(struct file *file);
 
 #endif
diff --git a/lib/string_helpers.c b/lib/string_helpers.c
index 142bed739fed..981e6655a9d4 100644
--- a/lib/string_helpers.c
+++ b/lib/string_helpers.c
@@ -10,6 +10,8 @@
 #include <linux/export.h>
 #include <linux/ctype.h>
 #include <linux/errno.h>
+#include <linux/fs.h>
+#include <linux/limits.h>
 #include <linux/mm.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -597,3 +599,31 @@ char *kstrdup_quotable_cmdline(struct task_struct *task)
 	return quoted;
 }
 EXPORT_SYMBOL_GPL(kstrdup_quotable_cmdline);
+
+/*
+ * Returns allocated NULL-terminated string containing pathname,
+ * with special characters escaped, able to be safely logged. If
+ * there is an error, the leading character will be "<".
+ */
+char *kstrdup_quotable_file(struct file *file)
+{
+	char *temp, *pathname;
+
+	if (!file)
+		return kstrdup("<unknown>", GFP_KERNEL);
+
+	/* We add 11 spaces for ' (deleted)' to be appended */
+	temp = kmalloc(PATH_MAX + 11, GFP_TEMPORARY);
+	if (!temp)
+		return kstrdup("<no_memory>", GFP_KERNEL);
+
+	pathname = file_path(file, temp, PATH_MAX + 11);
+	if (IS_ERR(pathname))
+		pathname = kstrdup("<too_long>", GFP_KERNEL);
+	else
+		pathname = kstrdup_quotable(pathname);
+
+	kfree(temp);
+	return pathname;
+}
+EXPORT_SYMBOL_GPL(kstrdup_quotable_file);
-- 
2.6.3

[PATCH v2 4/5] Yama: consolidate error reporting

[Kees Cook]

Use a common error reporting function for Yama violation reports, and give
more detail into the process command lines.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 security/yama/yama_lsm.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/security/yama/yama_lsm.c b/security/yama/yama_lsm.c
index cb6ed10816d4..fbd8ce90b068 100644
--- a/security/yama/yama_lsm.c
+++ b/security/yama/yama_lsm.c
@@ -18,6 +18,7 @@
 #include <linux/prctl.h>
 #include <linux/ratelimit.h>
 #include <linux/workqueue.h>
+#include <linux/string_helpers.h>
 
 #define YAMA_SCOPE_DISABLED	0
 #define YAMA_SCOPE_RELATIONAL	1
@@ -41,6 +42,22 @@ static DEFINE_SPINLOCK(ptracer_relations_lock);
 static void yama_relation_cleanup(struct work_struct *work);
 static DECLARE_WORK(yama_relation_work, yama_relation_cleanup);
 
+static void report_access(const char *access, struct task_struct *target,
+			  struct task_struct *agent)
+{
+	char *target_cmd, *agent_cmd;
+
+	target_cmd = kstrdup_quotable_cmdline(target);
+	agent_cmd = kstrdup_quotable_cmdline(agent);
+
+	pr_notice_ratelimited(
+		"ptrace %s of \"%s\"[%d] was attempted by \"%s\"[%d]\n",
+		access, target_cmd, target->pid, agent_cmd, agent->pid);
+
+	kfree(agent_cmd);
+	kfree(target_cmd);
+}
+
 /**
  * yama_relation_cleanup - remove invalid entries from the relation list
  *
@@ -307,11 +324,8 @@ static int yama_ptrace_access_check(struct task_struct *child,
 		}
 	}
 
-	if (rc && (mode & PTRACE_MODE_NOAUDIT) == 0) {
-		printk_ratelimited(KERN_NOTICE
-			"ptrace of pid %d was attempted by: %s (pid %d)\n",
-			child->pid, current->comm, current->pid);
-	}
+	if (rc && (mode & PTRACE_MODE_NOAUDIT) == 0)
+		report_access("attach", child, current);
 
 	return rc;
 }
@@ -337,11 +351,8 @@ int yama_ptrace_traceme(struct task_struct *parent)
 		break;
 	}
 
-	if (rc) {
-		printk_ratelimited(KERN_NOTICE
-			"ptraceme of pid %d was attempted by: %s (pid %d)\n",
-			current->pid, parent->comm, parent->pid);
-	}
+	if (rc)
+		report_access("traceme", current, parent);
 
 	return rc;
 }
-- 
2.6.3

[PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Kees Cook]

This LSM enforces that kernel-loaded files (modules, firmware, etc)
must all come from the same filesystem, with the expectation that
such a filesystem is backed by a read-only device such as dm-verity
or CDROM. This allows systems that have a verified and/or unchangeable
filesystem to enforce module and firmware loading restrictions without
needing to sign the files individually.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 MAINTAINERS                |   6 ++
 include/linux/lsm_hooks.h  |   5 ++
 security/Kconfig           |   1 +
 security/Makefile          |   2 +
 security/loadpin/Kconfig   |  10 +++
 security/loadpin/Makefile  |   1 +
 security/loadpin/loadpin.c | 206 +++++++++++++++++++++++++++++++++++++++++++++
 security/security.c        |   1 +
 8 files changed, 232 insertions(+)
 create mode 100644 security/loadpin/Kconfig
 create mode 100644 security/loadpin/Makefile
 create mode 100644 security/loadpin/loadpin.c

diff --git a/MAINTAINERS b/MAINTAINERS
index f07a174bbc81..06ab115a2066 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9937,6 +9937,12 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/jj/apparmor-dev.git
 S:	Supported
 F:	security/apparmor/
 
+LOADPIN SECURITY MODULE
+M:	Kees Cook <keescook@chromium.org>
+T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git lsm/loadpin
+S:	Supported
+F:	security/loadpin/
+
 YAMA SECURITY MODULE
 M:	Kees Cook <keescook@chromium.org>
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/kees/linux.git yama/tip
diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
index cdee11cbcdf1..f3402aab1927 100644
--- a/include/linux/lsm_hooks.h
+++ b/include/linux/lsm_hooks.h
@@ -1893,5 +1893,10 @@ extern void __init yama_add_hooks(void);
 #else
 static inline void __init yama_add_hooks(void) { }
 #endif
+#ifdef CONFIG_SECURITY_LOADPIN
+void __init loadpin_add_hooks(void);
+#else
+static inline void loadpin_add_hooks(void) { };
+#endif
 
 #endif /* ! __LINUX_LSM_HOOKS_H */
diff --git a/security/Kconfig b/security/Kconfig
index e45237897b43..176758cdfa57 100644
--- a/security/Kconfig
+++ b/security/Kconfig
@@ -122,6 +122,7 @@ source security/selinux/Kconfig
 source security/smack/Kconfig
 source security/tomoyo/Kconfig
 source security/apparmor/Kconfig
+source security/loadpin/Kconfig
 source security/yama/Kconfig
 
 source security/integrity/Kconfig
diff --git a/security/Makefile b/security/Makefile
index c9bfbc84ff50..f2d71cdb8e19 100644
--- a/security/Makefile
+++ b/security/Makefile
@@ -8,6 +8,7 @@ subdir-$(CONFIG_SECURITY_SMACK)		+= smack
 subdir-$(CONFIG_SECURITY_TOMOYO)        += tomoyo
 subdir-$(CONFIG_SECURITY_APPARMOR)	+= apparmor
 subdir-$(CONFIG_SECURITY_YAMA)		+= yama
+subdir-$(CONFIG_SECURITY_LOADPIN)	+= loadpin
 
 # always enable default capabilities
 obj-y					+= commoncap.o
@@ -22,6 +23,7 @@ obj-$(CONFIG_AUDIT)			+= lsm_audit.o
 obj-$(CONFIG_SECURITY_TOMOYO)		+= tomoyo/
 obj-$(CONFIG_SECURITY_APPARMOR)		+= apparmor/
 obj-$(CONFIG_SECURITY_YAMA)		+= yama/
+obj-$(CONFIG_SECURITY_LOADPIN)		+= loadpin/
 obj-$(CONFIG_CGROUP_DEVICE)		+= device_cgroup.o
 
 # Object integrity file lists
diff --git a/security/loadpin/Kconfig b/security/loadpin/Kconfig
new file mode 100644
index 000000000000..c668ac4eda65
--- /dev/null
+++ b/security/loadpin/Kconfig
@@ -0,0 +1,10 @@
+config SECURITY_LOADPIN
+	bool "Pin load of kernel files (modules, fw, etc) to one filesystem"
+	depends on SECURITY && BLOCK
+	help
+	  Any files read through the kernel file reading interface
+	  (kernel modules, firmware, kexec images, security policy) will
+	  be pinned to the first filesystem used for loading. Any files
+	  that come from other filesystems will be rejected. This is best
+	  used on systems without an initrd that have a root filesystem
+	  backed by a read-only device such as dm-verity or a CDROM.
diff --git a/security/loadpin/Makefile b/security/loadpin/Makefile
new file mode 100644
index 000000000000..c2d77f83037b
--- /dev/null
+++ b/security/loadpin/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_SECURITY_LOADPIN) += loadpin.o
diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
new file mode 100644
index 000000000000..04aa8ee41610
--- /dev/null
+++ b/security/loadpin/loadpin.c
@@ -0,0 +1,206 @@
+/*
+ * Module and Firmware Pinning Security Module
+ *
+ * Copyright 2011-2016 Google Inc.
+ *
+ * Author: Kees Cook <keescook@chromium.org>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#define pr_fmt(fmt) "LoadPin: " fmt
+
+#include <linux/module.h>
+#include <linux/fs.h>
+#include <linux/fs_struct.h>
+#include <linux/lsm_hooks.h>
+#include <linux/mount.h>
+#include <linux/path.h>
+#include <linux/sched.h>	/* current */
+#include <linux/string_helpers.h>
+
+static void report_load(const char *origin, struct file *file, char *operation)
+{
+	char *cmdline, *pathname;
+
+	pathname = kstrdup_quotable_file(file);
+	cmdline = kstrdup_quotable_cmdline(current);
+
+	pr_notice("%s %s obj=%s%s%s pid=%d cmdline=%s%s%s\n",
+		  origin, operation,
+		  (pathname && pathname[0] != '<') ? "\"" : "",
+		  pathname,
+		  (pathname && pathname[0] != '<') ? "\"" : "",
+		  task_pid_nr(current),
+		  cmdline ? "\"" : "", cmdline, cmdline ? "\"" : "");
+
+	kfree(cmdline);
+	kfree(pathname);
+}
+
+static int load_pinning = 1;
+static struct super_block *pinned_root;
+static DEFINE_SPINLOCK(pinned_root_spinlock);
+
+#ifdef CONFIG_SYSCTL
+static int zero;
+static int one = 1;
+
+static struct ctl_path loadpin_sysctl_path[] = {
+	{ .procname = "kernel", },
+	{ }
+};
+
+static struct ctl_table loadpin_sysctl_table[] = {
+	{
+		.procname       = "load_pinning",
+		.data           = &load_pinning,
+		.maxlen         = sizeof(int),
+		.mode           = 0644,
+		.proc_handler   = proc_dointvec_minmax,
+		.extra1         = &zero,
+		.extra2         = &one,
+	},
+	{ }
+};
+
+/*
+ * This must be called after early kernel init, since then the rootdev
+ * is available.
+ */
+static void check_pinning_enforcement(struct super_block *mnt_sb)
+{
+	bool ro = false;
+
+	/*
+	 * If load pinning is not enforced via a read-only block
+	 * device, allow sysctl to change modes for testing.
+	 */
+	if (mnt_sb->s_bdev) {
+		ro = bdev_read_only(mnt_sb->s_bdev);
+		pr_info("dev(%u,%u): %s\n",
+			MAJOR(mnt_sb->s_bdev->bd_dev),
+			MINOR(mnt_sb->s_bdev->bd_dev),
+			ro ? "read-only" : "writable");
+	} else
+		pr_info("mnt_sb lacks block device, treating as: writable\n");
+
+	if (!ro) {
+		if (!register_sysctl_paths(loadpin_sysctl_path,
+					   loadpin_sysctl_table))
+			pr_notice("sysctl registration failed!\n");
+		else
+			pr_info("load pinning can be disabled.\n");
+	} else
+		pr_info("load pinning engaged.\n");
+}
+#else
+static void check_pinning_enforcement(struct super_block *mnt_sb)
+{
+	pr_info("load pinning engaged.\n");
+}
+#endif
+
+static void loadpin_sb_free_security(struct super_block *mnt_sb)
+{
+	/*
+	 * When unmounting the filesystem we were using for load
+	 * pinning, we acknowledge the superblock release, but make sure
+	 * no other modules or firmware can be loaded.
+	 */
+	if (!IS_ERR_OR_NULL(pinned_root) && mnt_sb == pinned_root) {
+		pinned_root = ERR_PTR(-EIO);
+		pr_info("umount pinned fs: refusing further loads\n");
+	}
+}
+
+static const char *id_str[READING_MAX_ID] = {
+	[READING_FIRMWARE] = "firmware",
+	[READING_MODULE] = "kernel module",
+	[READING_KEXEC_IMAGE] = "kexec image",
+	[READING_KEXEC_INITRAMFS] = "kexec initramfs",
+	[READING_POLICY] = "security policy",
+};
+
+static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
+{
+	struct super_block *load_root;
+	const char *origin;
+
+	if (id < 0 || id >= READING_MAX_ID)
+		return -EINVAL;
+
+	origin = id_str[id];
+	if (!origin) {
+		origin = "unknown";
+		pr_warn_once("unknown kernel_read_file_id %d\n", id);
+	}
+
+	/* This handles the older init_module API that has a NULL file. */
+	if (!file) {
+		if (!load_pinning) {
+			report_load(origin, NULL, "old-api-pinning-ignored");
+			return 0;
+		}
+
+		report_load(origin, NULL, "old-api-denied");
+		return -EPERM;
+	}
+
+	load_root = file->f_path.mnt->mnt_sb;
+
+	/* First loaded module/firmware defines the root for all others. */
+	spin_lock(&pinned_root_spinlock);
+	/*
+	 * pinned_root is only NULL at startup. Otherwise, it is either
+	 * a valid reference, or an ERR_PTR.
+	 */
+	if (!pinned_root) {
+		pinned_root = load_root;
+		/*
+		 * Unlock now since it's only pinned_root we care about.
+		 * In the worst case, we will (correctly) report pinning
+		 * failures before we have announced that pinning is
+		 * enabled. This would be purely cosmetic.
+		 */
+		spin_unlock(&pinned_root_spinlock);
+		check_pinning_enforcement(pinned_root);
+		report_load(origin, file, "pinned");
+	} else {
+		spin_unlock(&pinned_root_spinlock);
+	}
+
+	if (IS_ERR_OR_NULL(pinned_root) || load_root != pinned_root) {
+		if (unlikely(!load_pinning)) {
+			report_load(origin, file, "pinning-ignored");
+			return 0;
+		}
+
+		report_load(origin, file, "denied");
+		return -EPERM;
+	}
+
+	return 0;
+}
+
+static struct security_hook_list loadpin_hooks[] = {
+	LSM_HOOK_INIT(sb_free_security, loadpin_sb_free_security),
+	LSM_HOOK_INIT(kernel_read_file, loadpin_read_file),
+};
+
+void __init loadpin_add_hooks(void)
+{
+	pr_info("preparing to pin");
+	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks));
+}
+
+/* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
+module_param(load_pinning, int, 0);
+MODULE_PARM_DESC(load_pinning, "Pin module/firmware loading (default: true)");
diff --git a/security/security.c b/security/security.c
index 3644b0344d29..ce02178c892f 100644
--- a/security/security.c
+++ b/security/security.c
@@ -60,6 +60,7 @@ int __init security_init(void)
 	 */
 	capability_add_hooks();
 	yama_add_hooks();
+	loadpin_add_hooks();
 
 	/*
 	 * Load all the remaining security modules.
-- 
2.6.3

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Andrew Morton]

On Mon, 28 Mar 2016 14:14:22 -0700 Kees Cook <keescook@chromium.org> wrote:

> This LSM enforces that kernel-loaded files (modules, firmware, etc)
> must all come from the same filesystem, with the expectation that
> such a filesystem is backed by a read-only device such as dm-verity
> or CDROM. This allows systems that have a verified and/or unchangeable
> filesystem to enforce module and firmware loading restrictions without
> needing to sign the files individually.

Patchset generally looks good to me.  It's regrettable that a load of
stuff was added to lib/ for one obscure LSM but hopefully (doubtfully)
someone else will find a use for some of it.

I'll assume that James is handling all of this.

> --- /dev/null
> +++ b/security/loadpin/loadpin.c
> @@ -0,0 +1,206 @@
> +/*
> + * Module and Firmware Pinning Security Module
> + *
> + * Copyright 2011-2016 Google Inc.
> + *
> + * Author: Kees Cook <keescook@chromium.org>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#define pr_fmt(fmt) "LoadPin: " fmt
> +
> +#include <linux/module.h>
> +#include <linux/fs.h>
> +#include <linux/fs_struct.h>
> +#include <linux/lsm_hooks.h>
> +#include <linux/mount.h>
> +#include <linux/path.h>
> +#include <linux/sched.h>	/* current */
> +#include <linux/string_helpers.h>
> +
> +static void report_load(const char *origin, struct file *file, char *operation)
> +{
> +	char *cmdline, *pathname;
> +
> +	pathname = kstrdup_quotable_file(file);
> +	cmdline = kstrdup_quotable_cmdline(current);
> +
> +	pr_notice("%s %s obj=%s%s%s pid=%d cmdline=%s%s%s\n",
> +		  origin, operation,
> +		  (pathname && pathname[0] != '<') ? "\"" : "",
> +		  pathname,
> +		  (pathname && pathname[0] != '<') ? "\"" : "",
> +		  task_pid_nr(current),
> +		  cmdline ? "\"" : "", cmdline, cmdline ? "\"" : "");
> +
> +	kfree(cmdline);
> +	kfree(pathname);
> +}
> +
> +static int load_pinning = 1;
> +static struct super_block *pinned_root;
> +static DEFINE_SPINLOCK(pinned_root_spinlock);
> +
> +#ifdef CONFIG_SYSCTL
> +static int zero;
> +static int one = 1;
> +
> +static struct ctl_path loadpin_sysctl_path[] = {
> +	{ .procname = "kernel", },
> +	{ }
> +};
> +
> +static struct ctl_table loadpin_sysctl_table[] = {
> +	{
> +		.procname       = "load_pinning",
> +		.data           = &load_pinning,
> +		.maxlen         = sizeof(int),
> +		.mode           = 0644,
> +		.proc_handler   = proc_dointvec_minmax,
> +		.extra1         = &zero,
> +		.extra2         = &one,
> +	},
> +	{ }
> +};

There should be somewhere to document the new sysctl?

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Kees Cook]

On Mon, Mar 28, 2016 at 2:38 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 28 Mar 2016 14:14:22 -0700 Kees Cook <keescook@chromium.org> wrote:
>
>> This LSM enforces that kernel-loaded files (modules, firmware, etc)
>> must all come from the same filesystem, with the expectation that
>> such a filesystem is backed by a read-only device such as dm-verity
>> or CDROM. This allows systems that have a verified and/or unchangeable
>> filesystem to enforce module and firmware loading restrictions without
>> needing to sign the files individually.
>
> Patchset generally looks good to me.  It's regrettable that a load of
> stuff was added to lib/ for one obscure LSM but hopefully (doubtfully)
> someone else will find a use for some of it.
>
> I'll assume that James is handling all of this.
>
>> --- /dev/null
>> +++ b/security/loadpin/loadpin.c
>> @@ -0,0 +1,206 @@
>> +/*
>> + * Module and Firmware Pinning Security Module
>> + *
>> + * Copyright 2011-2016 Google Inc.
>> + *
>> + * Author: Kees Cook <keescook@chromium.org>
>> + *
>> + * This software is licensed under the terms of the GNU General Public
>> + * License version 2, as published by the Free Software Foundation, and
>> + * may be copied, distributed, and modified under those terms.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#define pr_fmt(fmt) "LoadPin: " fmt
>> +
>> +#include <linux/module.h>
>> +#include <linux/fs.h>
>> +#include <linux/fs_struct.h>
>> +#include <linux/lsm_hooks.h>
>> +#include <linux/mount.h>
>> +#include <linux/path.h>
>> +#include <linux/sched.h>     /* current */
>> +#include <linux/string_helpers.h>
>> +
>> +static void report_load(const char *origin, struct file *file, char *operation)
>> +{
>> +     char *cmdline, *pathname;
>> +
>> +     pathname = kstrdup_quotable_file(file);
>> +     cmdline = kstrdup_quotable_cmdline(current);
>> +
>> +     pr_notice("%s %s obj=%s%s%s pid=%d cmdline=%s%s%s\n",
>> +               origin, operation,
>> +               (pathname && pathname[0] != '<') ? "\"" : "",
>> +               pathname,
>> +               (pathname && pathname[0] != '<') ? "\"" : "",
>> +               task_pid_nr(current),
>> +               cmdline ? "\"" : "", cmdline, cmdline ? "\"" : "");
>> +
>> +     kfree(cmdline);
>> +     kfree(pathname);
>> +}
>> +
>> +static int load_pinning = 1;
>> +static struct super_block *pinned_root;
>> +static DEFINE_SPINLOCK(pinned_root_spinlock);
>> +
>> +#ifdef CONFIG_SYSCTL
>> +static int zero;
>> +static int one = 1;
>> +
>> +static struct ctl_path loadpin_sysctl_path[] = {
>> +     { .procname = "kernel", },
>> +     { }
>> +};
>> +
>> +static struct ctl_table loadpin_sysctl_table[] = {
>> +     {
>> +             .procname       = "load_pinning",
>> +             .data           = &load_pinning,
>> +             .maxlen         = sizeof(int),
>> +             .mode           = 0644,
>> +             .proc_handler   = proc_dointvec_minmax,
>> +             .extra1         = &zero,
>> +             .extra2         = &one,
>> +     },
>> +     { }
>> +};
>
> There should be somewhere to document the new sysctl?

Good call. I'll send a follow-up with a Documentation patch.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Joe Perches]

On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
> This LSM enforces that kernel-loaded files (modules, firmware, etc)
> must all come from the same filesystem, with the expectation that
> such a filesystem is backed by a read-only device such as dm-verity
> or CDROM. This allows systems that have a verified and/or unchangeable
> filesystem to enforce module and firmware loading restrictions without
> needing to sign the files individually.

trivia:

> diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
[]
> +#define pr_fmt(fmt) "LoadPin: " fmt

Using

#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

would be a lot more common.

Is there some reason the logging messages should be
prefixed with "LoadPin: " instead of "loadpin: "?

Re: [PATCH v2 1/5] string_helpers: add kstrdup_quotable

[Joe Perches]

On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
> Handle allocating and escaping a string safe for logging.
[]
> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
[]
> @@ -68,4 +68,6 @@ static inline int string_escape_str_any_np(const char *src, char *dst,
>  	return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, only);
>  }
>  
> +char *kstrdup_quotable(char *src);

char *kstrdup_quotable(const char *src)

but shouldn't this really take a gfp_t like all
the other kstr<string> functions?

> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
[]
> +char *kstrdup_quotable(char *src)
[]
> +	BUG_ON(string_escape_mem(src, slen, dst, dlen, flags, esc) != dlen);

BUG_ON?  why?

Re: [PATCH v2 2/5] string_helpers: add kstrdup_quotable_cmdline

[Andy Shevchenko]

On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
> Provide an escaped (but readable: no inter-argument NULLs)
> commandline
> safe for logging.

> +/*
> + * Returns allocated NULL-terminated string containing process
> + * command line, with inter-argument NULLs replaced with spaces,
> + * and other special characters escaped.
> + */
> +char *kstrdup_quotable_cmdline(struct task_struct *task)
> +{
> +	char *buffer, *quoted;
> +	int i, res;
> +
> +	buffer = kmalloc(PAGE_SIZE, GFP_TEMPORARY);
> +	if (!buffer)
> +		return NULL;
> +
> +	res = get_cmdline(task, buffer, PAGE_SIZE - 1);
> +	buffer[res] = '\0';
> +
> +	/* Collapse trailing NULLs. */
> +	for (; res > 0; res--)
> +		if (buffer[res-1] != '\0')
> +			break;

/* buffer[res] is '\0', so, predecrement is safe here */
while (buffer[--res] == '\0')
 /* nothing */;

?

> +
> +	/* Replace inter-argument NULLs. */
> +	for (i = 0; i < res; i++)
> +		if (buffer[i] == '\0')
> +			buffer[i] = ' ';
> +
> +	/* Make sure result is printable. */
> +	quoted = kstrdup_quotable(buffer);
> +	kfree(buffer);
> +	return quoted;
> +}
> +EXPORT_SYMBOL_GPL(kstrdup_quotable_cmdline);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 2/5] string_helpers: add kstrdup_quotable_cmdline

[Andy Shevchenko]

On Wed, 2016-03-30 at 14:07 +0300, Andy Shevchenko wrote:
> On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:


> > +	res = get_cmdline(task, buffer, PAGE_SIZE - 1);
> > +	buffer[res] = '\0';
> > +
> > +	/* Collapse trailing NULLs. */
> > +	for (; res > 0; res--)
> > +		if (buffer[res-1] != '\0')
> > +			break;
> /* buffer[res] is '\0', so, predecrement is safe here */
> while (buffer[--res] == '\0')
>  /* nothing */;
> 
> ?
> 

Oops, no, the following should be better

while (--res >= 0 && buffer[res] == '\0') ;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Mimi Zohar]

On Mon, 2016-03-28 at 14:38 -0700, Andrew Morton wrote:
> On Mon, 28 Mar 2016 14:14:22 -0700 Kees Cook <keescook@chromium.org> wrote:
> 
> > This LSM enforces that kernel-loaded files (modules, firmware, etc)
> > must all come from the same filesystem, with the expectation that
> > such a filesystem is backed by a read-only device such as dm-verity
> > or CDROM. This allows systems that have a verified and/or unchangeable
> > filesystem to enforce module and firmware loading restrictions without
> > needing to sign the files individually.
> 
> Patchset generally looks good to me.  It's regrettable that a load of
> stuff was added to lib/ for one obscure LSM but hopefully (doubtfully)
> someone else will find a use for some of it.

I'm planning on adding support for measuring buffers, like the boot
command line, which will need to be string safe.

Mimi

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Mimi Zohar]

On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:

> +static const char *id_str[READING_MAX_ID] = {
> +	[READING_FIRMWARE] = "firmware",
> +	[READING_MODULE] = "kernel module",
> +	[READING_KEXEC_IMAGE] = "kexec image",
> +	[READING_KEXEC_INITRAMFS] = "kexec initramfs",
> +	[READING_POLICY] = "security policy",
> +};
> +
> +static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
> +{
> +	struct super_block *load_root;
> +	const char *origin;
> +
> +	if (id < 0 || id >= READING_MAX_ID)
> +		return -EINVAL;
> +

The kernel_read_file_id enumeration starts at 1.

> +	origin = id_str[id];
> +	if (!origin) {
> +		origin = "unknown";

Right, all the entries in the kernel_read_file_id enumeration are
currently listed in id_str.  Checking origin is needed for when id_str[]
isn't kept in sync with the enumeration.

Looks good!

Mimi

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Kees Cook]

On Thu, Mar 31, 2016 at 2:24 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
>
>> +static const char *id_str[READING_MAX_ID] = {
>> +     [READING_FIRMWARE] = "firmware",
>> +     [READING_MODULE] = "kernel module",
>> +     [READING_KEXEC_IMAGE] = "kexec image",
>> +     [READING_KEXEC_INITRAMFS] = "kexec initramfs",
>> +     [READING_POLICY] = "security policy",
>> +};
>> +
>> +static int loadpin_read_file(struct file *file, enum kernel_read_file_id id)
>> +{
>> +     struct super_block *load_root;
>> +     const char *origin;
>> +
>> +     if (id < 0 || id >= READING_MAX_ID)
>> +             return -EINVAL;
>> +
>
> The kernel_read_file_id enumeration starts at 1.

Yeah, I noticed that but decided that since I didn't have a named
value to mark the start, and negative value enums are extremely
unliked, I'd just check against "0" since missing id_str entries will
be correctly identified as "unknown" in the case where id_str and
READING_* enum is out of sync.

>> +     origin = id_str[id];
>> +     if (!origin) {
>> +             origin = "unknown";
>
> Right, all the entries in the kernel_read_file_id enumeration are
> currently listed in id_str.  Checking origin is needed for when id_str[]
> isn't kept in sync with the enumeration.

I wonder if there should be a function that returns a const string for
each kernel_read_file_id enum so users of the enum don't need to do
it?

> Looks good!

Thanks! I'll be sending an updated version shortly...

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 5/5] LSM: LoadPin for kernel file loading restrictions

[Mimi Zohar]

On Mon, 2016-04-04 at 12:31 -0700, Kees Cook wrote:
> On Thu, Mar 31, 2016 at 2:24 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> > On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
> >
> >> +static const char *id_str[READING_MAX_ID] = {
> >> +     [READING_FIRMWARE] = "firmware",
> >> +     [READING_MODULE] = "kernel module",
> >> +     [READING_KEXEC_IMAGE] = "kexec image",
> >> +     [READING_KEXEC_INITRAMFS] = "kexec initramfs",
> >> +     [READING_POLICY] = "security policy",
> >> +};
> >> +

> I wonder if there should be a function that returns a const string for
> each kernel_read_file_id enum so users of the enum don't need to do
> it?

Right, having a single, corresponding, string array would be good.  Some
of the strings in id_str[] have blanks, which might be problematic for
the audit subsystem, and would need to be replaced with a hyphen or
underscore.

Mimi

Re: [PATCH v2 2/5] string_helpers: add kstrdup_quotable_cmdline

[Kees Cook]

On Wed, Mar 30, 2016 at 4:11 AM, Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Wed, 2016-03-30 at 14:07 +0300, Andy Shevchenko wrote:
>> On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
>
>
>> > +   res = get_cmdline(task, buffer, PAGE_SIZE - 1);
>> > +   buffer[res] = '\0';
>> > +
>> > +   /* Collapse trailing NULLs. */
>> > +   for (; res > 0; res--)
>> > +           if (buffer[res-1] != '\0')
>> > +                   break;
>> /* buffer[res] is '\0', so, predecrement is safe here */
>> while (buffer[--res] == '\0')
>>  /* nothing */;
>>
>> ?
>>
>
> Oops, no, the following should be better
>
> while (--res >= 0 && buffer[res] == '\0') ;

Sure, I'll change it. Though I'll need to adjust the next loop's use
of res (since it effectively changes from "number of bytes" counter to
"location of last non-null byte" (i <= res).

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: [PATCH v2 1/5] string_helpers: add kstrdup_quotable

[Kees Cook]

On Mon, Mar 28, 2016 at 4:30 PM, Joe Perches <joe@perches.com> wrote:
> On Mon, 2016-03-28 at 14:14 -0700, Kees Cook wrote:
>> Handle allocating and escaping a string safe for logging.
> []
>> diff --git a/include/linux/string_helpers.h b/include/linux/string_helpers.h
> []
>> @@ -68,4 +68,6 @@ static inline int string_escape_str_any_np(const char *src, char *dst,
>>       return string_escape_str(src, dst, sz, ESCAPE_ANY_NP, only);
>>  }
>>
>> +char *kstrdup_quotable(char *src);
>
> char *kstrdup_quotable(const char *src)
>
> but shouldn't this really take a gfp_t like all
> the other kstr<string> functions?

Ah, good idea, yes. I'll add this.

>> diff --git a/lib/string_helpers.c b/lib/string_helpers.c
> []
>> +char *kstrdup_quotable(char *src)
> []
>> +     BUG_ON(string_escape_mem(src, slen, dst, dlen, flags, esc) != dlen);
>
> BUG_ON?  why?

I was thinking that if the string_escape_mem output length changes
between calls, something has gone terribly wrong internally, and we
should stop. But, since we're limited to dlen, it'll be a truncation
at worst. I will change this to WARN_ON, which is what kasprintf does.

Thanks!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security