Re: [PATCH v2] module: Implement sig_unenforce parameter

[Jessica Yu <jeyu@kernel.org>]

+++ 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@intel.com>
>---
>
>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