Re: [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity devices
Kees Cook
keescook at chromium.org
Fri May 13 22:36:26 UTC 2022
On May 4, 2022 12:54:18 PM PDT, Matthias Kaehlcke <mka at chromium.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 at chromium.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
More information about the Linux-security-module-archive
mailing list