[PATCH 0/3] Module signing and version info

[PATCH 0/3] Module signing and version info

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

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(-)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 1/3] module: Invalidate signatures on force-loaded modules

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1862 bytes --]

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 <ben@decadent.org.uk>
Cc: stable@vger.kernel.org
---
 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;
 


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 2/3] Documentation/module-signing.txt: Note need for version info if reusing a key

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]

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 <ben@decadent.org.uk>
Cc: stable@vger.kernel.org
---
 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.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[PATCH 3/3] module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

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 <ben@decadent.org.uk>
---
 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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules

[Rusty Russell]

Ben Hutchings <ben@decadent.org.uk> 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 <ben@decadent.org.uk>
> Cc: stable@vger.kernel.org
> ---
>  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.

Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1879 bytes --]

On Tue, 2016-04-26 at 20:07 +0930, Rusty Russell wrote:
> Ben Hutchings <ben@decadent.org.uk> 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 <ben@decadent.org.uk>
> > Cc: stable@vger.kernel.org
> > ---
> >  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

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules

[Rusty Russell]

Ben Hutchings <ben@decadent.org.uk> writes:
> On Tue, 2016-04-26 at 20:07 +0930, Rusty Russell wrote:
>> Ben Hutchings <ben@decadent.org.uk> 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.