Re: [PATCH v2] module: Implement sig_unenforce parameter

[Jessica Yu]

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