| Message ID | 20230914112739.112729-2-alessandro.carminati@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enhancing Boot Speed and Security with Delayed Module Signature Verification | expand |
On Thu, Sep 14, 2023 at 11:27:38AM +0000, Alessandro Carminati (Red Hat) wrote: > This commit introduces a novel boot argument parameter that provides an > advanced level of control over the verification of module signatures > during the initial stages of booting. With this enhancement, we gain the > capability to postpone the verification of module signatures to after > intrd stage is finished. > > Given that bootloader-provided artifacts are commonly employed > post-verification, Is such a thing expressed with a kernel config? If so then shouldn't this be default for those uses cases? > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com> > --- > include/linux/module.h | 4 +++ > kernel/module/main.c | 14 ++++++----- > kernel/module/signing.c | 56 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 68 insertions(+), 6 deletions(-) > > diff --git a/include/linux/module.h b/include/linux/module.h > index a98e188cf37b..9899aeac43b0 100644 > --- a/include/linux/module.h > +++ b/include/linux/module.h > @@ -34,6 +34,10 @@ > > #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN > > +#ifdef CONFIG_MODULE_SIG > +extern int module_sig_check_wait; > +#endif Please add under is_module_sig_enforced. That's one new line Vs 3 new ones. I see the code which skips module signature verification and the knobs but I don't see the code which complete the promise to do the actual signature verification post initrd / initramfs state. What gives? Luis
Hello Luis, Thanks a lot for sharing your thoughts about this topic. Il giorno gio 16 nov 2023 alle ore 18:35 Luis Chamberlain <mcgrof@kernel.org> ha scritto: > > On Thu, Sep 14, 2023 at 11:27:38AM +0000, Alessandro Carminati (Red Hat) wrote: > > This commit introduces a novel boot argument parameter that provides an > > advanced level of control over the verification of module signatures > > during the initial stages of booting. With this enhancement, we gain the > > capability to postpone the verification of module signatures to after > > intrd stage is finished. > > > > Given that bootloader-provided artifacts are commonly employed > > post-verification, > > Is such a thing expressed with a kernel config? If so then shouldn't > this be default for those uses cases? > I've hesitated to propose this as the default behavior for a few reasons: The current patch doesn’t include a check for the secure boot chain being enabled. From what I've learned, confirming secure boot mode on UEFI systems, especially on non x86 targets, lacks a standardized method. Considering this, I've chosen to leave the decision to users who can specify a command-line argument during kernel boot. Implementing a new feature as default in the kernel might face resistance and seem intrusive. To avoid potential conflicts, I lean towards letting users choose to opt into this feature. Additionally, It’s essential to acknowledge that not all kernels involve an initrd phase. Embedded systems often differ in their use of an initrd compared to traditional Linux distributions. Enforcing this feature by default might not align with the preferences of the embedded systems community. I'm open to investing time in proposing conditions that might pave the way for this feature to become default if certain conditions are met. > > Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com> > > --- > > include/linux/module.h | 4 +++ > > kernel/module/main.c | 14 ++++++----- > > kernel/module/signing.c | 56 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 68 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/module.h b/include/linux/module.h > > index a98e188cf37b..9899aeac43b0 100644 > > --- a/include/linux/module.h > > +++ b/include/linux/module.h > > @@ -34,6 +34,10 @@ > > > > #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN > > > > +#ifdef CONFIG_MODULE_SIG > > +extern int module_sig_check_wait; > > +#endif > > Please add under is_module_sig_enforced. That's one new line Vs 3 new ones. > > I see the code which skips module signature verification and the knobs > but I don't see the code which complete the promise to do the actual > signature verification post initrd / initramfs state. What gives? My initial intention wasn't centered around providing an automated solution. Instead, I envisioned a design where users could manually restore module verification during a specific point in their init scripts. It might be plausible to restore module verification when the rootfs is remounted. However, this seems limiting rather than advantageous. To offer users maximum flexibility, I considered a solution involving a sysfs file as a control mechanism. The objective of the RFC I sent out is to gather opinions on this feature. Understanding what people think about it is very welcome. In this context, the question arises: would it be better to have an automated solution that restores the module signature verification automatically? > > Luis
On Fri, Nov 17, 2023 at 02:56:53PM +0100, Alessandro Carminati wrote: > Il giorno gio 16 nov 2023 alle ore 18:35 Luis Chamberlain > <mcgrof@kernel.org> ha scritto: > > > > I see the code which skips module signature verification and the knobs > > but I don't see the code which complete the promise to do the actual > > signature verification post initrd / initramfs state. What gives? > > My initial intention wasn't centered around providing an automated solution. It is not even an automated solution, it's *any* solution. So to be clear your patch simply disables module verification, it has no solution. > Instead, I envisioned a design where users could manually restore module > verification during a specific point in their init scripts. > > It might be plausible to restore module verification when the rootfs is > remounted. However, this seems limiting rather than advantageous. The patch as-is describes a lofty world and does nothing other than disables module verification. If a patch disables module verification it should just do that and describe that. Nothing else. The subject of the patch tends to suggest some flexibility it provided but does nothing of being flexible, it outright disables module signature verification. The commit log and the patch subject description are describing something completely different than what the code actually does, and it gives me to the concern, to the point that if you didn't have a few commit logs in the kernel I would have thought your intent was test kernel developers with some AI type of code that does something stupid and very carefully crafted commit log. Nacked-by: Luis Chamberlain <mcgrof@kernel.org> Luis
On Fri, 2023-11-17 at 10:33 -0800, Luis Chamberlain wrote: > On Fri, Nov 17, 2023 at 02:56:53PM +0100, Alessandro Carminati wrote: > > Il giorno gio 16 nov 2023 alle ore 18:35 Luis Chamberlain > > <mcgrof@kernel.org> ha scritto: > > > > > > I see the code which skips module signature verification and the knobs > > > but I don't see the code which complete the promise to do the actual > > > signature verification post initrd / initramfs state. What gives? > > > > My initial intention wasn't centered around providing an automated solution. > > It is not even an automated solution, it's *any* solution. So to be > clear your patch simply disables module verification, it has no > solution. > > > Instead, I envisioned a design where users could manually restore module > > verification during a specific point in their init scripts. > > > > It might be plausible to restore module verification when the rootfs is > > remounted. However, this seems limiting rather than advantageous. > > The patch as-is describes a lofty world and does nothing other than > disables module verification. If a patch disables module verification > it should just do that and describe that. Nothing else. The subject > of the patch tends to suggest some flexibility it provided but does > nothing of being flexible, it outright disables module signature > verification. The commit log and the patch subject description are > describing something completely different than what the code actually > does, and it gives me to the concern, to the point that if you didn't > have a few commit logs in the kernel I would have thought your intent > was test kernel developers with some AI type of code that does something > stupid and very carefully crafted commit log. > > Nacked-by: Luis Chamberlain <mcgrof@kernel.org> > > Luis Hi, On top of that, this change would go counter to the security posture that is established by the Lockdown LSM, especially when UEFI Secure Boot is used. While it would be great if initrds were only ever read- only, signed, and verified, the reality is that on all general purpose distributions in use today the initrd is writable by root, and the kernel command line is also writable by root. E.g.: on Debian, one just has to edit /etc/default/grub, run a command and at the next reboot arbitrary kernel command line parameters will be applied, including this one, and combined with adding an arbitrary kernel module to the appropriate path this becomes, de-facto, a uid0- to-ring0 privilege escalation avenue, which is something the secure boot + shim + lockdown workflow tries very hard to avoid. While there might be room for such features (e.g.: https://docs.kernel.org/admin-guide/LSM/LoadPin.html ) in very specific and defined environment or use cases, at the very least there needs to be: - the mechanism to lock it down again very early at boot as indicated in the cover letter, which is missing as pointed out by Luis - a kconfig governing the availability of the new kernel cmdline option, disabled by default - ideally a check or hook into the Lockdown LSM, so that it can stop the new kernel command line option, if enabled at build time and if specified, from actually working when secure boot is enabled, or at the very, very minimum taint it Then users wanting to use such a performance optimization will be able to do so in their own special purpose builds, without negatively affecting the security story of generalist distributions.
diff --git a/include/linux/module.h b/include/linux/module.h index a98e188cf37b..9899aeac43b0 100644 --- a/include/linux/module.h +++ b/include/linux/module.h @@ -34,6 +34,10 @@ #define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN +#ifdef CONFIG_MODULE_SIG +extern int module_sig_check_wait; +#endif + struct modversion_info { unsigned long crc; char name[MODULE_NAME_LEN]; diff --git a/kernel/module/main.c b/kernel/module/main.c index 59b1d067e528..d24dd1f728f2 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2033,12 +2033,14 @@ static void module_augment_kernel_taints(struct module *mod, struct load_info *i add_taint_module(mod, TAINT_TEST, LOCKDEP_STILL_OK); } #ifdef CONFIG_MODULE_SIG - mod->sig_ok = info->sig_ok; - if (!mod->sig_ok) { - pr_notice_once("%s: module verification failed: signature " - "and/or required key missing - tainting " - "kernel\n", mod->name); - add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK); + if (!module_sig_check_wait) { + mod->sig_ok = info->sig_ok; + if (!mod->sig_ok) { + pr_notice_once("%s: module verification failed: signature " + "and/or required key missing - tainting " + "kernel\n", mod->name); + add_taint_module(mod, TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK); + } } #endif diff --git a/kernel/module/signing.c b/kernel/module/signing.c index a2ff4242e623..2580ceed3cdb 100644 --- a/kernel/module/signing.c +++ b/kernel/module/signing.c @@ -19,6 +19,8 @@ #undef MODULE_PARAM_PREFIX #define MODULE_PARAM_PREFIX "module." +int module_sig_check_wait; + static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE); module_param(sig_enforce, bool_enable_only, 0644); @@ -37,6 +39,58 @@ void set_module_sig_enforced(void) sig_enforce = true; } +/* + * test thing to enable sig enforcing later in boot sequence + */ +static int __init module_sig_check_wait_arg(char *str) +{ + return kstrtoint(str, 0, &module_sig_check_wait); +} +__setup("module_sig_check_wait=", module_sig_check_wait_arg); + +/* + * securityfs entry to disable module_sig_check_wait, and start enforcing modules signature check + */ +static ssize_t module_sig_check_wait_read(struct file *file, char __user *buf, size_t count, + loff_t *ppos) +{ + return simple_read_from_buffer(buf, count, ppos, + module_sig_check_wait == 1 ? "1\n" : "0\n", 2); +} + +static ssize_t module_sig_check_wait_write(struct file *file, const char __user *buf, + size_t n, loff_t *ppos) +{ + int tmp; + + if (kstrtoint_from_user(buf, n, 10, &tmp)) + return -EINVAL; + if (tmp != 0) { + pr_info("module_sig_check_wait can be only disabled!\n"); + return -EINVAL; + } + pr_info("module_sig_check_wait disabled!\n"); + module_sig_check_wait = tmp; + + return n; +} + +static const struct file_operations module_sig_check_wait_ops = { + .read = module_sig_check_wait_read, + .write = module_sig_check_wait_write, +}; + +static int __init module_sig_check_wait_secfs_init(void) +{ + struct dentry *dentry; + + dentry = securityfs_create_file("module_sig_check_wait", 0644, NULL, NULL, + &module_sig_check_wait_ops); + return PTR_ERR_OR_ZERO(dentry); +} + +core_initcall(module_sig_check_wait_secfs_init); + /* * Verify the signature on a module. */ @@ -69,6 +123,8 @@ int mod_verify_sig(const void *mod, struct load_info *info) int module_sig_check(struct load_info *info, int flags) { + if (module_sig_check_wait) + return 0; int err = -ENODATA; const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1; const char *reason;
This commit introduces a novel boot argument parameter that provides an advanced level of control over the verification of module signatures during the initial stages of booting. With this enhancement, we gain the capability to postpone the verification of module signatures to after intrd stage is finished. Given that bootloader-provided artifacts are commonly employed post-verification, we can effectively capitalize on this verification process and exempt the verification of modules within the initrd. This strategic adjustment reduces the time needed in this stage to load modules by temporarily enabling unsigned modules. Before terminating the stage modules signature check can be re-enabled keeping the system security level. To activate this functionality, a fresh boot argument parameter has been incorporated to disable verification during early boot. Furthermore, a supplementary method is necessary to reactivate verification subsequently. This is executed by leveraging a switch within a /proc file, inspired by the foundational principles of the Lockdown LSM levels. While this mechanism permits the activation of the functionaliity, and it ensures that the switch remains unalterable once engaged. example usage: ``` $ qemu-system-aarch64 -M virt -cpu cortex-a53 -machine gic-version=3 \ -m 512 -nographic -smp 2 -kernel arch/arm64/boot/Image \ -initrd aarch64br.initrd \ -append "rootwait init=/sbin/init module_sig_check_wait=1" [ 0.000000] Booting Linux on physical CPU 0x0000000000 [0x410fd034] [...] [ 0.689441] Freeing unused kernel memory: 8576K [ 0.690536] Run /init as init process Starting syslogd: OK Starting klogd: OK Running sysctl: OK Saving 256 bits of non-creditable seed for next boot Starting network: OK Welcome to Buildroot buildroot login: root ~ # cat /sys/module/module/parameters/sig_enforce Y ~ # mount -t securityfs none /sys/kernel/security ~ # cat /sys/kernel/security/module_sig_check_wait 1 ~ # insmod usbserial.ko ~ # lsmod Module Size Used by Not tainted usbserial 36864 0 ~ # rmmod usbserial ~ # echo 0 >/sys/kernel/security/module_sig_check_wait [ 226.114569] module_sig_check_wait disabled! ~ # insmod usbserial.ko [ 230.646980] Loading of unsigned module is rejected [ 230.648608] Loading of unsigned module is rejected insmod: can't insert 'usbserial.ko': Key was rejected by service ~ # echo 1 > /sys/kernel/security/module_sig_check_wait [ 248.036518] module_sig_check_wait can be only disabled! ~ # ``` Signed-off-by: Alessandro Carminati (Red Hat) <alessandro.carminati@gmail.com> --- include/linux/module.h | 4 +++ kernel/module/main.c | 14 ++++++----- kernel/module/signing.c | 56 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 6 deletions(-)