Message-ID: <B7FB2BE6-DF1C-414A-B4C2-0C15FD1CBF75@chromium.org>
Date:   Fri, 13 May 2022 15:36:26 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Matthias Kaehlcke <mka@...omium.org>,
        Alasdair Kergon <agk@...hat.com>,
        Mike Snitzer <snitzer@...nel.org>,
        James Morris <jmorris@...ei.org>,
        "Serge E . Hallyn" <serge@...lyn.com>
CC:     dm-devel@...hat.com, linux-kernel@...r.kernel.org,
        linux-raid@...r.kernel.org, Song Liu <song@...nel.org>,
        Douglas Anderson <dianders@...omium.org>,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity devices



On May 4, 2022 12:54:18 PM PDT, Matthias Kaehlcke <mka@...omium.org> wrote:
>Extend LoadPin to allow loading of kernel files from trusted dm-verity [1]
>devices.
>
>This change adds the concept of trusted verity devices to LoadPin. LoadPin
>maintains a list of root digests of verity devices it considers trusted.
>Userspace can populate this list through an ioctl on the new LoadPin
>securityfs entry 'dm-verity'. The ioctl receives a file descriptor of
>a file with verity digests as parameter. Verity reads the digests from
>this file after confirming that the file is located on the pinned root.
>The list of trusted digests can only be set up once, which is typically
>done at boot time.
>
>When a kernel file is read LoadPin first checks (as usual) whether the file
>is located on the pinned root, if so the file can be loaded. Otherwise, if
>the verity extension is enabled, LoadPin determines whether the file is
>located on a verity backed device and whether the root digest of that

I think this should be "... on an already trusted device ..."

>device is in the list of trusted digests. The file can be loaded if the
>verity device has a trusted root digest.
>
>Background:
>
>As of now LoadPin restricts loading of kernel files to a single pinned
>filesystem, typically the rootfs. This works for many systems, however it
>can result in a bloated rootfs (and OTA updates) on platforms where
>multiple boards with different hardware configurations use the same rootfs
>image. Especially when 'optional' files are large it may be preferable to
>download/install them only when they are actually needed by a given board.
>Chrome OS uses Downloadable Content (DLC) [2] to deploy certain 'packages'
>at runtime. As an example a DLC package could contain firmware for a
>peripheral that is not present on all boards. DLCs use dm-verity to verify
>the integrity of the DLC content.
>
>[1] https://www.kernel.org/doc/html/latest/admin-guide/device-mapper/verity.html
>[2] https://chromium.googlesource.com/chromiumos/platform2/+/HEAD/dlcservice/docs/developer.md
>
>Signed-off-by: Matthias Kaehlcke <mka@...omium.org>
>---
>
>Changes in v3:
>- added securityfs for LoadPin (currently only populated when
>  CONFIG_SECURITY_LOADPIN_VERITY=y)
>- added uapi include for LoadPin
>- changed the interface for setting up the list of trusted
>  digests from sysctl to ioctl on securityfs entry
>- added stub for loadpin_is_fs_trusted() to be used
>  CONFIG_SECURITY_LOADPIN_VERITY is not select
>- depend on CONFIG_SECURITYFS instead of CONFIG_SYSTCL
>- updated Kconfig help
>- minor changes in read_trusted_verity_root_digests()
>- updated commit message
>
>Changes in v2:
>- userspace now passes the path of the file with the verity digests
>  via systcl, instead of the digests themselves
>- renamed sysctl file to 'trusted_verity_root_digests_path'
>- have CONFIG_SECURITY_LOADPIN_VERITY depend on CONFIG_SYSCTL
>- updated Kconfig doc
>- updated commit message
>
> include/uapi/linux/loadpin.h |  19 ++++
> security/loadpin/Kconfig     |  16 +++
> security/loadpin/loadpin.c   | 184 ++++++++++++++++++++++++++++++++++-
> 3 files changed, 218 insertions(+), 1 deletion(-)
> create mode 100644 include/uapi/linux/loadpin.h
>
>diff --git a/include/uapi/linux/loadpin.h b/include/uapi/linux/loadpin.h
>new file mode 100644
>index 000000000000..d303a582209b
>--- /dev/null
>+++ b/include/uapi/linux/loadpin.h
>@@ -0,0 +1,19 @@
>+/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>+/*
>+ * Copyright (c) 2022, Google LLC
>+ */
>+
>+#ifndef _UAPI_LINUX_LOOP_LOADPIN_H
>+#define _UAPI_LINUX_LOOP_LOADPIN_H
>+
>+#define LOADPIN_IOC_MAGIC	'L'
>+
>+/**
>+ * LOADPIN_IOC_SET_TRUSTED_VERITY_DIGESTS - Set up the root digests of verity devices
>+ *                                          that loadpin should trust.
>+ *
>+ * Takes a file descriptor from which to read the root digests of trusted verity devices.

Maybe add to the comment the securityfs node path here as a helpful hint to the reader, and mention the format (comma separated?)

>+ */
>+#define LOADPIN_IOC_SET_TRUSTED_VERITY_DIGESTS _IOW(LOADPIN_IOC_MAGIC, 0x00, unsigned int)
>+
>+#endif /* _UAPI_LINUX_LOOP_LOADPIN_H */
>diff --git a/security/loadpin/Kconfig b/security/loadpin/Kconfig
>index 91be65dec2ab..e319ca8e3f3d 100644
>--- a/security/loadpin/Kconfig
>+++ b/security/loadpin/Kconfig
>@@ -18,3 +18,19 @@ config SECURITY_LOADPIN_ENFORCE
> 	  If selected, LoadPin will enforce pinning at boot. If not
> 	  selected, it can be enabled at boot with the kernel parameter
> 	  "loadpin.enforce=1".
>+
>+config SECURITY_LOADPIN_VERITY
>+	bool "Allow reading files from certain other filesystems that use dm-verity"
>+	depends on DM_VERITY=y && SECURITYFS
>+	help
>+	  If selected LoadPin can allow reading files from filesystems
>+	  that use dm-verity. LoadPin maintains a list of verity root
>+	  digests it considers trusted. A verity backed filesystem is
>+	  considered trusted if its root digest is found in the list
>+	  of trusted digests.
>+
>+	  The list of trusted verity can be populated through an ioctl
>+	  on the LoadPin securityfs entry 'dm-verity'. The ioctl
>+	  expects a file descriptor of a file with verity digests as
>+	  parameter. The file must be located on the pinned root and
>+	  contain a comma separated list of digests.
>diff --git a/security/loadpin/loadpin.c b/security/loadpin/loadpin.c
>index b12f7d986b1e..c29ce562a366 100644
>--- a/security/loadpin/loadpin.c
>+++ b/security/loadpin/loadpin.c
>@@ -18,6 +18,9 @@
> #include <linux/path.h>
> #include <linux/sched.h>	/* current */
> #include <linux/string_helpers.h>
>+#include <linux/device-mapper.h>
>+#include <linux/dm-verity-loadpin.h>
>+#include <uapi/linux/loadpin.h>
> 
> static void report_load(const char *origin, struct file *file, char *operation)
> {
>@@ -43,6 +46,9 @@ static char *exclude_read_files[READING_MAX_ID];
> static int ignore_read_file_id[READING_MAX_ID] __ro_after_init;
> static struct super_block *pinned_root;
> static DEFINE_SPINLOCK(pinned_root_spinlock);
>+#ifdef CONFIG_SECURITY_LOADPIN_VERITY
>+static LIST_HEAD(trusted_verity_root_digests);
>+#endif
> 
> #ifdef CONFIG_SYSCTL
> 
>@@ -118,6 +124,24 @@ static void loadpin_sb_free_security(struct super_block *mnt_sb)
> 	}
> }
> 
>+#ifdef CONFIG_SECURITY_LOADPIN_VERITY
>+static bool loadpin_is_fs_trusted(struct super_block *sb)
>+{
>+	struct mapped_device *md = dm_get_md(sb->s_bdev->bd_dev);
>+	bool trusted;
>+
>+	if (!md)
>+		return false;
>+
>+	trusted = dm_verity_loadpin_is_md_trusted(md);
>+	dm_put(md);
>+
>+	return trusted;
>+}
>+#else
>+static inline bool loadpin_is_fs_trusted(struct super_block *sb) { return false; };
>+#endif
>+
> static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
> 			     bool contents)
> {
>@@ -174,7 +198,8 @@ static int loadpin_read_file(struct file *file, enum kernel_read_file_id id,
> 		spin_unlock(&pinned_root_spinlock);
> 	}
> 
>-	if (IS_ERR_OR_NULL(pinned_root) || load_root != pinned_root) {
>+	if (IS_ERR_OR_NULL(pinned_root) ||
>+	    ((load_root != pinned_root) && !loadpin_is_fs_trusted(load_root))) {
> 		if (unlikely(!enforce)) {
> 			report_load(origin, file, "pinning-ignored");
> 			return 0;
>@@ -240,6 +265,7 @@ static int __init loadpin_init(void)
> 		enforce ? "" : "not ");
> 	parse_exclude();
> 	security_add_hooks(loadpin_hooks, ARRAY_SIZE(loadpin_hooks), "loadpin");
>+
> 	return 0;
> }
> 
>@@ -248,6 +274,162 @@ DEFINE_LSM(loadpin) = {
> 	.init = loadpin_init,
> };
> 
>+#ifdef CONFIG_SECURITY_LOADPIN_VERITY
>+
>+enum loadpin_securityfs_interface_index {
>+	LOADPIN_DM_VERITY,
>+};
>+
>+static int read_trusted_verity_root_digests(unsigned int fd)
>+{
>+	struct fd f;
>+	void *data;

Probably easier if this is u8 *?

>+	int rc;
>+	char *p, *d;
>+
>+	/* The list of trusted root digests can only be set up once */
>+	if (!list_empty(&trusted_verity_root_digests))
>+		return -EPERM;
>+
>+	f = fdget(fd);
>+	if (!f.file)
>+		return -EINVAL;
>+
>+	data = kzalloc(SZ_4K, GFP_KERNEL);
>+	if (!data) {
>+		rc = -ENOMEM;
>+		goto err;
>+	}
>+
>+	rc = kernel_read_file(f.file, 0, &data, SZ_4K - 1, NULL, READING_POLICY);
>+	if (rc < 0)
>+		goto err;
>+
>+	((char *)data)[rc] = '\0';
>+
>+	p = strim(data);
>+	while ((d = strsep(&p, ",")) != NULL) {

Maybe be flexible and add newline as a separator too?

>+		int len = strlen(d);
>+		struct trusted_root_digest *trd;
>+
>+		if (len % 2) {
>+			rc = -EPROTO;
>+			goto err;
>+		}
>+
>+		len /= 2;
>+
>+		trd = kzalloc(sizeof(*trd), GFP_KERNEL);

With the struct change, this could be:

kzalloc(struct_size(trd, data, len), ...)

>+		if (!trd) {
>+			rc = -ENOMEM;
>+			goto err;
>+		}
>+
>+		trd->data = kzalloc(len, GFP_KERNEL);
>+		if (!trd->data) {
>+			kfree(trd);
>+			rc = -ENOMEM;
>+			goto err;
>+		}
>+
>+		if (hex2bin(trd->data, d, len)) {
>+			kfree(trd);
>+			kfree(trd->data);
>+			rc = -EPROTO;
>+			goto err;
>+		}
>+
>+		trd->len = len;
>+
>+		list_add_tail(&trd->node, &trusted_verity_root_digests);
>+	}
>+
>+	kfree(data);
>+	fdput(f);
>+
>+	if (!list_empty(&trusted_verity_root_digests))
>+		dm_verity_loadpin_set_trusted_root_digests(&trusted_verity_root_digests);
>+
>+	return 0;
>+
>+err:
>+	kfree(data);
>+

Maybe add a comment that any load failure will invalidate the entire list?

>+	{
>+		struct trusted_root_digest *trd, *tmp;
>+
>+		list_for_each_entry_safe(trd, tmp, &trusted_verity_root_digests, node) {
>+			kfree(trd->data);
>+			list_del(&trd->node);
>+			kfree(trd);
>+		}
>+	}
>+
>+	fdput(f);
>+
>+	return rc;
>+}
>+
>+/******************************** securityfs ********************************/
>+
>+static long dm_verity_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
>+{
>+	void __user *uarg = (void __user *)arg;
>+	unsigned int fd;
>+	int rc;
>+
>+	switch (cmd) {
>+	case LOADPIN_IOC_SET_TRUSTED_VERITY_DIGESTS:
>+		rc = copy_from_user(&fd, uarg, sizeof(fd));
>+		if (rc)
>+			return rc;
>+
>+		return read_trusted_verity_root_digests(fd);
>+
>+	default:
>+		return -EINVAL;
>+	}
>+}
>+
>+static const struct file_operations loadpin_dm_verity_ops = {
>+	.unlocked_ioctl = dm_verity_ioctl,
>+	.compat_ioctl = compat_ptr_ioctl,
>+};
>+
>+/**
>+ * init_loadpin_securityfs - create the securityfs directory for LoadPin
>+ *
>+ * We can not put this method normally under the loadpin_init() code path since
>+ * the security subsystem gets initialized before the vfs caches.
>+ *
>+ * Returns 0 if the securityfs directory creation was successful.
>+ */
>+static int __init init_loadpin_securityfs(void)
>+{
>+	struct dentry *loadpin_dir, *dentry;
>+
>+	loadpin_dir = securityfs_create_dir("loadpin", NULL);
>+	if (IS_ERR(loadpin_dir)) {
>+		pr_err("LoadPin: could not create securityfs dir: %d\n",
>+		       PTR_ERR(loadpin_dir));
>+		return PTR_ERR(loadpin_dir);
>+	}
>+
>+	dentry = securityfs_create_file("dm-verity", 0600, loadpin_dir,
>+					(void *)LOADPIN_DM_VERITY, &loadpin_dm_verity_ops);
>+	if (IS_ERR(dentry)) {
>+		pr_err("LoadPin: could not create securityfs entry 'dm-verity': %d\n",
>+		       PTR_ERR(dentry));
>+		return PTR_ERR(dentry);
>+	}
>+
>+	return 0;
>+}
>+
>+fs_initcall(init_loadpin_securityfs);
>+
>+#endif /* CONFIG_SECURITY_LOADPIN_VERITY */
>+
> /* Should not be mutable after boot, so not listed in sysfs (perm == 0). */
> module_param(enforce, int, 0);
> MODULE_PARM_DESC(enforce, "Enforce module/firmware pinning");

Otherwise looks good! The only other thing I can think of is pondering more about more carefully failing closed. E.g. instead of just throwing away all the other hashes on a file load failure, maybe lock out future attempts to set it too? I'm not sure this is actually useful, though. :P it shouldn't be possible to corrupt the file, etc. But in the universe where things like DirtyCOW happens, I've gotten even more paranoid. ;)

-- 
Kees Cook

Powered by blists - more mailing lists