If a module signing key is used for multiple kernel builds, it is
critical that the modules for each build can be distinguished.
This series makes force-loading invalidate module signatures and
documents the importance of module version info when reusing a key
for multiple builds.
Ben.
Ben Hutchings (3):
module: Invalidate signatures on force-loaded modules
Documentation/module-signing.txt: Note need for version info if
reusing a key
module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled
Documentation/module-signing.txt | 6 ++++++
init/Kconfig | 1 +
kernel/module.c | 13 +++++++++----
3 files changed, 16 insertions(+), 4 deletions(-)
Signing a module should only make it trusted by the specific kernel it
was built for, not anything else. Loading a signed module meant for a
kernel with a different ABI could have interesting effects.
Therefore, treat all signatures as invalid when a module is
force-loaded.
Signed-off-by: Ben Hutchings <[email protected]>
Cc: [email protected]
---
kernel/module.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/kernel/module.c b/kernel/module.c
index 66426f743c29..649b1827ed15 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2599,13 +2599,18 @@ static inline void kmemleak_load_module(const struct module *mod,
#endif
#ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info)
+static int module_sig_check(struct load_info *info, int flags)
{
int err = -ENOKEY;
const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
const void *mod = info->hdr;
- if (info->len > markerlen &&
+ /*
+ * Require flags == 0, as a module with version information
+ * removed is no longer the module that was signed
+ */
+ if (flags == 0 &&
+ info->len > markerlen &&
memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
/* We truncate the module to discard the signature */
info->len -= markerlen;
@@ -2624,7 +2629,7 @@ static int module_sig_check(struct load_info *info)
return err;
}
#else /* !CONFIG_MODULE_SIG */
-static int module_sig_check(struct load_info *info)
+static int module_sig_check(struct load_info *info, int flags)
{
return 0;
}
@@ -3386,7 +3391,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
long err;
char *after_dashes;
- err = module_sig_check(info);
+ err = module_sig_check(info, flags);
if (err)
goto free_copy;
Signing a module should only make it trusted by the specific kernel it
was built for, not anything else. If a module signing key is used for
multiple ABI-incompatible kernels, the modules need to include enough
version information to distinguish them.
Signed-off-by: Ben Hutchings <[email protected]>
Cc: [email protected]
---
Documentation/module-signing.txt | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/module-signing.txt b/Documentation/module-signing.txt
index 696d5caf4fd8..f0e3361db20c 100644
--- a/Documentation/module-signing.txt
+++ b/Documentation/module-signing.txt
@@ -271,3 +271,9 @@ Since the private key is used to sign modules, viruses and malware could use
the private key to sign modules and compromise the operating system. The
private key must be either destroyed or moved to a secure location and not kept
in the root node of the kernel source tree.
+
+If you use the same private key to sign modules for multiple kernel
+configurations, you must ensure that the module version information is
+sufficient to prevent loading a module into a different kernel. Either
+set CONFIG_MODVERSIONS=y or ensure that each configuration has a different
+kernel release string by changing EXTRAVERSION or CONFIG_LOCALVERSION.
Force-loading now fails if signature enforcement is enabled, so if
signature enforcement is statically enabled then we may as well
disable it completely.
Signed-off-by: Ben Hutchings <[email protected]>
---
init/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/init/Kconfig b/init/Kconfig
index e0d26162432e..269533088a1b 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -1853,6 +1853,7 @@ if MODULES
config MODULE_FORCE_LOAD
bool "Forced module loading"
default n
+ depends on !MODULE_SIG_FORCE
help
Allow loading of modules without version information (ie. modprobe
--force). Forced module loading sets the 'F' (forced) taint flag and
Ben Hutchings <[email protected]> writes:
> Signing a module should only make it trusted by the specific kernel it
> was built for, not anything else. Loading a signed module meant for a
> kernel with a different ABI could have interesting effects.
> Therefore, treat all signatures as invalid when a module is
> force-loaded.
>
> Signed-off-by: Ben Hutchings <[email protected]>
> Cc: [email protected]
> ---
> kernel/module.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/module.c b/kernel/module.c
> index 66426f743c29..649b1827ed15 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2599,13 +2599,18 @@ static inline void kmemleak_load_module(const struct module *mod,
> #endif
>
> #ifdef CONFIG_MODULE_SIG
> -static int module_sig_check(struct load_info *info)
> +static int module_sig_check(struct load_info *info, int flags)
> {
> int err = -ENOKEY;
> const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> const void *mod = info->hdr;
>
> - if (info->len > markerlen &&
> + /*
> + * Require flags == 0, as a module with version information
> + * removed is no longer the module that was signed
> + */
> + if (flags == 0 &&
This check is a bit lazy. We could have other flags in future,
so this should really be !(flags &
(MODULE_INIT_IGNORE_MODVERSIONS|MODULE_INIT_IGNORE_VERMAGIC) right?
Cheers,
Rusty.
On Tue, 2016-04-26 at 20:07 +0930, Rusty Russell wrote:
> Ben Hutchings <[email protected]> writes:
> >
> > Signing a module should only make it trusted by the specific kernel it
> > was built for, not anything else. Loading a signed module meant for a
> > kernel with a different ABI could have interesting effects.
> > Therefore, treat all signatures as invalid when a module is
> > force-loaded.
> >
> > Signed-off-by: Ben Hutchings <[email protected]>
> > Cc: [email protected]
> > ---
> > kernel/module.c | 13 +++++++++----
> > 1 file changed, 9 insertions(+), 4 deletions(-)
> >
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 66426f743c29..649b1827ed15 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -2599,13 +2599,18 @@ static inline void kmemleak_load_module(const struct module *mod,
> > #endif
> >
> > #ifdef CONFIG_MODULE_SIG
> > -static int module_sig_check(struct load_info *info)
> > +static int module_sig_check(struct load_info *info, int flags)
> > {
> > int err = -ENOKEY;
> > const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
> > const void *mod = info->hdr;
> >
> > - if (info->len > markerlen &&
> > + /*
> > + * Require flags == 0, as a module with version information
> > + * removed is no longer the module that was signed
> > + */
> > + if (flags == 0 &&
> This check is a bit lazy. We could have other flags in future,
> so this should really be !(flags &
> (MODULE_INIT_IGNORE_MODVERSIONS|MODULE_INIT_IGNORE_VERMAGIC) right?
Yes we could, but I'd prefer this to fail-safe in case no-one thinks
about whether it should be updated then.
Ben.
--
Ben Hutchings
The generation of random numbers is too important to be left to chance.
- Robert Coveyou
Ben Hutchings <[email protected]> writes:
> On Tue, 2016-04-26 at 20:07 +0930, Rusty Russell wrote:
>> Ben Hutchings <[email protected]> writes:
>> > - if (info->len > markerlen &&
>> > + /*
>> > + * Require flags == 0, as a module with version information
>> > + * removed is no longer the module that was signed
>> > + */
>> > + if (flags == 0 &&
>> This check is a bit lazy. We could have other flags in future,
>> so this should really be !(flags &
>> (MODULE_INIT_IGNORE_MODVERSIONS|MODULE_INIT_IGNORE_VERMAGIC) right?
>
> Yes we could, but I'd prefer this to fail-safe in case no-one thinks
> about whether it should be updated then.
Yeah, line ball. We could screw up either way, and I can't think of
an reasonable new flag off the top of my head to give a concrete
example.
I've applied all three, thanks!
Rusty.