Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1385701 > unrolled thread

[PATCH 0/3] Module signing and version info

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] Module signing and version info Ben Hutchings <ben@decadent.org.uk> - 2016-04-23 20:50 +0200
    [PATCH 3/3] module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE  is enabled Ben Hutchings <ben@decadent.org.uk> - 2016-04-23 20:50 +0200
    [PATCH 2/3] Documentation/module-signing.txt: Note need for version  info if reusing a key Ben Hutchings <ben@decadent.org.uk> - 2016-04-23 20:50 +0200
    [PATCH 1/3] module: Invalidate signatures on force-loaded modules Ben Hutchings <ben@decadent.org.uk> - 2016-04-23 20:50 +0200
      Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules Rusty Russell <rusty@rustcorp.com.au> - 2016-04-26 22:40 +0200
        Re: [PATCH 1/3] module: Invalidate signatures on force-loaded  modules Ben Hutchings <ben@decadent.org.uk> - 2016-04-26 23:10 +0200
          Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules Rusty Russell <rusty@rustcorp.com.au> - 2016-04-28 02:30 +0200

#1385701 — [PATCH 0/3] Module signing and version info

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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;
 

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web