Re: [PATCH v2] module: Implement sig_unenforce parameter

From: Jessica Yu
Date: Wed Jun 13 2018 - 09:15:29 EST


+++ 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