DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B934F21C98
Date: Thu, 10 Aug 2017 22:43:28 +0200
From: Jessica Yu <jeyu@kernel.org>
To: Matthew Garrett <mjg59@google.com>
Cc: linux-kernel@vger.kernel.org, rusty@rustcorp.com.au
Subject: Re: Allow automatic kernel taint on unsigned module load to be
 disabled
Message-ID: <20170810204328.kk4lbj4hvednmofw@redbean>
References: <20170804180751.14896-1-mjg59@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20170804180751.14896-1-mjg59@google.com>
User-Agent: NeoMutt/20170714 (1.8.3)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3485
Lines: 87

+++ Matthew Garrett [04/08/17 11:07 -0700]:
>Distributions may wish to provide kernels that permit loading of
>unsigned modules based on certain policy decisions. Right now that
>results in the kernel being tainted whenever an unsigned module is
>loaded, which may not be desirable. Add a config option to disable that.

Hi Matthew!

I think I'm missing some context here. Could you provide some more
background and help me understand why we want to go into all this
trouble just to avoid a taint?  Was there a recent bug report, mailing
list discussion, etc. that spurred you to write this patch? I'm not
understanding why this particular taint is undesirable.

I still think there is informational value in providing the unsigned
module taint on a kernel that supports module signatures (CONFIG_MODULE_SIG).
When debugging or trawling through crash dumps, module taints are
useful for developers to immediately identify which modules were
out-of-tree, which were unsigned and therefore not originally shipped
by the distro etc, which often applies to e.g. 3rd party/dkms modules.
And if a user for example locally compiles a module without signing it
why would the unsigned module taint bother them more than the
out-of-tree one (because that module would get both taints)?

If it is the "module verification failed" message that is actually
scaring users, we could perhaps "soften" it to say something like
"loading unsigned module X".

Jessica

>Signed-off-by: Matthew Garrett <mjg59@google.com>
>---
> init/Kconfig    | 13 ++++++++++++-
> kernel/module.c |  2 ++
> 2 files changed, 14 insertions(+), 1 deletion(-)
>
>diff --git a/init/Kconfig b/init/Kconfig
>index 8514b25db21c..196860c5d1e5 100644
>--- a/init/Kconfig
>+++ b/init/Kconfig
>@@ -1749,12 +1749,23 @@ config MODULE_SIG
> 	  debuginfo strip done by some packagers (such as rpmbuild) and
> 	  inclusion into an initramfs that wants the module size reduced.
>
>+config MODULE_UNSIGNED_TAINT
>+	bool "Taint the kernel if unsigned modules are loaded"
>+	default y
>+	depends on MODULE_SIG
>+	help
>+	  Taint the kernel if an unsigned kernel module is loaded. If this
>+	  option is enabled, the kernel will be tainted on an attempt to load
>+	  an unsigned module or signed modules for which we don't have a key
>+	  even if signature enforcement is disabled.
>+
> config MODULE_SIG_FORCE
> 	bool "Require modules to be validly signed"
> 	depends on MODULE_SIG
> 	help
> 	  Reject unsigned modules or signed modules for which we don't have a
>-	  key.  Without this, such modules will simply taint the kernel.
>+	  key. Without this, such modules will be loaded successfully but will
>+	  (if MODULE_UNSIGNED_TAINT is set) taint the kernel.
>
> config MODULE_SIG_ALL
> 	bool "Automatically sign all modules"
>diff --git a/kernel/module.c b/kernel/module.c
>index 40f983cbea81..71f80c8816f2 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -3660,12 +3660,14 @@ static int load_module(struct load_info *info, const char __user *uargs,
>
> #ifdef CONFIG_MODULE_SIG
> 	mod->sig_ok = info->sig_ok;
>+#ifdef CONFIG_MODULE_UNSIGNED_TAINT
> 	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
> #endif
>
> 	/* To avoid stressing percpu allocator, do this once we're unique. */
>-- 
>2.14.0.rc1.383.gd1ce394fe2-goog
>