On May 4, 2022 12:54:18 PM PDT, Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> >--- > >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 -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
