Module signing and version info
cover Module signing and version info
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(-)
PATCH 1/3 module: Invalidate signatures on force-loaded modules
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.
--- kernel/module.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
kernel/module.c +9 -4
@@ -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;
Re: [PATCH 1/3] module: Invalidate signatures on force-loaded modules
Quoted 23 lines · Ben Hutchings
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]>--- kernel/module.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)kernel/module.c +1
-1@@ -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) {
kernel/module.c static inline void kmemleak_load_module(const struct module *mod, · line 2612
Quoted 10 lines
kernel/module.c +5
-1int 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
Quoted 30 lines · Rusty Russell
Quoted 29 lines · Ben Hutchings
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]>---
kernel/module.c | 13 +++++++++----1 file changed, 9 insertions(+), 4 deletions(-)kernel/module.c +0
-0@@ -2599,13 +2599,18 @@ static inline void kmemleak_load_module(const struct module *mod, #endif
#ifdefCONFIG_MODULE_SIGkernel/module.c +1
-1-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;kernel/module.c +0
-1- if (info->len > markerlen &&
Quoted 8 lines
kernel/module.c static inline void kmemleak_load_module(const struct module *mod, · line 2612
Quoted 5 lines
kernel/module.c +5
-0+/* +* 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.
Signature · 3 lines
Ben Hutchings
The generation of random numbers is too important to be left to chance.
- Robert CoveyouRusty Russell
AppliedRe: [PATCH 1/3] module: Invalidate signatures on force-loaded modules
Quoted 12 lines · Ben Hutchings
Quoted 10 lines · Rusty Russell
kernel/module.c static inline void kmemleak_load_module(const struct module *mod, · line 2612
Quoted 6 lines · Ben Hutchings
kernel/module.c +5
-1- 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?
Quoted 2 lines
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.
PATCH 2/3 Documentation/module-signing.txt: Note need for version info if reusing a key
In reply to Ben HutchingsSigning 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.
--- Documentation/module-signing.txt | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/module-signing.txt +6 -0
@@ -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.
PATCH 3/3 module: Disable MODULE_FORCE_LOAD when MODULE_SIG_FORCE is enabled
In reply to Ben HutchingsForce-loading now fails if signature enforcement is enabled, so if signature enforcement is statically enabled then we may as well disable it completely.
--- init/Kconfig | 1 + 1 file changed, 1 insertion(+)
init/Kconfig +1 -0
@@ -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