RE: [PATCH v2] module: Implement sig_unenforce parameter

[Warden, Brett T]

> -----Original Message-----
> From: Jessica Yu [mailto:jeyu@xxxxxxxxxx]
> Sent: Wednesday, June 13, 2018 6:15 AM
> To: Warden, Brett T <brett.t.warden@xxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] module: Implement sig_unenforce parameter
> 
> +++ Brett T. Warden [06/06/18 12:44 -0700]:
> >When CONFIG_MODULE_SIG_FORCE is enabled, also provide a boot-time-only
> >parameter, module.sig_unenforce, to disable signature enforcement. This
> >allows distributions to ship with signature verification enforcement
> >enabled by default, but for users to elect to disable it without
> >recompiling, to support building and loading out-of-tree modules.
> >
> >Signed-off-by: Brett T. Warden <brett.t.warden@xxxxxxxxx>
> >---
> >
> >Added CONFIG_X86 guards around use of boot_params since this structure
> >is arch-specific.
> >
> > .../admin-guide/kernel-parameters.txt         |  4 +++
> > kernel/module.c                               | 31 +++++++++++++++++--
> > 2 files changed, 33 insertions(+), 2 deletions(-)
> >
> >diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> >index 1beb30d8d7fc..87909e021558 100644
> >--- a/Documentation/admin-guide/kernel-parameters.txt
> >+++ b/Documentation/admin-guide/kernel-parameters.txt
> >@@ -2380,6 +2380,10 @@
> > 			Note that if CONFIG_MODULE_SIG_FORCE is set, that
> > 			is always true, so this option does nothing.
> >
> >+	module.sig_unenforce
> >+			[KNL] This parameter allows modules without signatures
> >+			to be loaded, overriding CONFIG_MODULE_SIG_FORCE.
> >+
> > 	module_blacklist=  [KNL] Do not load a comma-separated list of
> > 			modules.  Useful for debugging problem modules.
> >
> >diff --git a/kernel/module.c b/kernel/module.c
> >index c9bea7f2b43e..27f23d85e1ba 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -64,6 +64,7 @@
> > #include <linux/bsearch.h>
> > #include <linux/dynamic_debug.h>
> > #include <linux/audit.h>
> >+#include <linux/efi.h>
> > #include <uapi/linux/module.h>
> > #include "module-internal.h"
> >
> >@@ -274,9 +275,13 @@ static void module_assert_mutex_or_preempt(void)
> > }
> >
> > static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> >-#ifndef CONFIG_MODULE_SIG_FORCE
> >+#ifdef CONFIG_MODULE_SIG_FORCE
> >+/* Allow disabling module signature requirement by adding boot param */
> >+static bool sig_unenforce;
> >+module_param(sig_unenforce, bool_enable_only, 0444);
> >+#else /* !CONFIG_MODULE_SIG_FORCE */
> > module_param(sig_enforce, bool_enable_only, 0644);
> >-#endif /* !CONFIG_MODULE_SIG_FORCE */
> >+#endif /* CONFIG_MODULE_SIG_FORCE */
> >
> > /*
> >  * Export sig_enforce kernel cmdline parameter to allow other subsystems
> rely
> >@@ -415,6 +420,10 @@ extern const s32 __start___kcrctab_unused[];
> > extern const s32 __start___kcrctab_unused_gpl[];
> > #endif
> >
> >+#ifdef CONFIG_X86
> >+extern struct boot_params boot_params;
> >+#endif
> >+
> > #ifndef CONFIG_MODVERSIONS
> > #define symversion(base, idx) NULL
> > #else
> >@@ -4243,6 +4252,24 @@ static const struct file_operations
> proc_modules_operations = {
> > static int __init proc_modules_init(void)
> > {
> > 	proc_create("modules", 0, NULL, &proc_modules_operations);
> >+
> >+#ifdef CONFIG_MODULE_SIG_FORCE
> >+#ifdef CONFIG_X86
> >+	switch (boot_params.secure_boot) {
> >+	case efi_secureboot_mode_unset:
> >+	case efi_secureboot_mode_unknown:
> >+	case efi_secureboot_mode_disabled:
> >+		/*
> >+		 * sig_unenforce is only applied if SecureBoot is not
> >+		 * enabled.
> >+		 */
> >+		sig_enforce = !sig_unenforce;
> >+	}
> >+#else
> >+	sig_enforce = !sig_unenforce;
> >+#endif
> >+#endif
> 
> I don't want to rope in Secure Boot specifics and arch-specific code
> (other than what's in arch/) into the module loader. I also don't want
> to change the current semantics of CONFIG_MODULE_SIG_FORCE, for any
> existing users who just want to set a build-time policy and already
> assume that unsigned modules can't be loaded, period. Lastly, having
> both sig_enforce and sig_unenforce parameters is really confusing.
> Might as well just make it one parameter - sig_enforce - and make that
> toggleable under a certain configuration.
> 
> It sounds like you want to ship with signature enforcement enabled by
> default, but stil want to make this toggleable for users. So maybe
> something in between CONFIG_MODULE_SIG and CONFIG_MODULE_SIG_FORCE.
> Half baked suggestion: Would a new config option that enforces module
> signatures by default but makes sig_enforce toggleable for users suit
> your use case? This would be less strict than CONFIG_MODULE_SIG_FORCE.
> 
> Thanks,
> 
> Jessica

What if I make a sub-option (default n) of CONFIG_MODULE_SIG_FORCE that
would allow toggling sig_enforce (eliminating sig_unenforce)?
CONFIG_MODULE_SIG_FORCE_ALLOW_OVERRIDE?



Our internal security review produced the requirement to check for
SecureBoot before disabling sig_enforce. I'll have to do some research
to figure out a cleaner way to accomplish that without X86 code in
module.c.


Thanks for the feedback,

-- Brett