diff mbox series

[v9,07/14] module: Move extra signature support out of core code

Message ID 20220228234322.2073104-8-atomlin@redhat.com (mailing list archive)
State Superseded
Headers show
Series module: core code clean up | expand

Commit Message

Aaron Tomlin Feb. 28, 2022, 11:43 p.m. UTC 
No functional change.

This patch migrates additional module signature check
code from core module code into kernel/module/signing.c.

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
---
 kernel/module/internal.h |  9 +++++
 kernel/module/main.c     | 87 ----------------------------------------
 kernel/module/signing.c  | 77 +++++++++++++++++++++++++++++++++++
 3 files changed, 86 insertions(+), 87 deletions(-)

Comments

Christophe Leroy March 2, 2022, 8:08 a.m. UTC | #1 
Le 01/03/2022 à 00:43, Aaron Tomlin a écrit :
> No functional change.
> 
> This patch migrates additional module signature check
> code from core module code into kernel/module/signing.c.
> 
> Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Signed-off-by: Aaron Tomlin <atomlin@redhat.com>
> ---
>   kernel/module/internal.h |  9 +++++
>   kernel/module/main.c     | 87 ----------------------------------------
>   kernel/module/signing.c  | 77 +++++++++++++++++++++++++++++++++++
>   3 files changed, 86 insertions(+), 87 deletions(-)
> 

> diff --git a/kernel/module/signing.c b/kernel/module/signing.c
> index 8aeb6d2ee94b..85c8999dfecf 100644
> --- a/kernel/module/signing.c
> +++ b/kernel/module/signing.c
> @@ -11,9 +11,29 @@
>   #include <linux/module_signature.h>
>   #include <linux/string.h>
>   #include <linux/verification.h>
> +#include <linux/security.h>
>   #include <crypto/public_key.h>
> +#include <uapi/linux/module.h>
>   #include "internal.h"
>   
> +static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
> +module_param(sig_enforce, bool_enable_only, 0644);
> +
> +/*
> + * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
> + * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
> + */
> +bool is_module_sig_enforced(void)
> +{
> +	return sig_enforce;
> +}
> +EXPORT_SYMBOL(is_module_sig_enforced);

As reported by the test robot, that's not enough.

When it was in main.c, is_module_sig_enforced() was build as soon as 
CONFIG_MODULES was set.

Now it is only built when CONFIG_MODULE_SIG is selected, so you have to 
modify include/linux/modules.h and have the stub 
is_module_sig_enforced() when CONFIG_MODULE_SIG is not selected and not 
only when CONFIG_MODULES is not selected.


> +
> +void set_module_sig_enforced(void)
> +{
> +	sig_enforce = true;
> +}
> +
>   /*
>    * Verify the signature on a module.
>    */

Christophe
Aaron Tomlin March 2, 2022, 1:33 p.m. UTC | #2 
On Wed 2022-03-02 08:08 +0000, Christophe Leroy wrote:
> > +bool is_module_sig_enforced(void)
> > +{
> > +	return sig_enforce;
> > +}
> > +EXPORT_SYMBOL(is_module_sig_enforced);
> 
> As reported by the test robot, that's not enough.

Hi Christophe,

Thanks for testing this.

> When it was in main.c, is_module_sig_enforced() was build as soon as 
> CONFIG_MODULES was set.
> Now it is only built when CONFIG_MODULE_SIG is selected,

Agreed.

> so you have to modify include/linux/modules.h and have the stub
> is_module_sig_enforced() when CONFIG_MODULE_SIG is not selected and not
> only when CONFIG_MODULES is not selected.

Sure: when Kconfig CONFIG_MODULE_SIG is not selected.

Luis,

I can see that the latest series is in mcgrof/modules-testing.
Should I address the above as a separate patch with "Fixes:" or
provide a whole new series, with the fix within the same patch?
In my opinion, another iteration would be cleaner.


Kind regards,
Christophe Leroy March 2, 2022, 1:41 p.m. UTC | #3 
Le 02/03/2022 à 14:33, Aaron Tomlin a écrit :
> On Wed 2022-03-02 08:08 +0000, Christophe Leroy wrote:
>>> +bool is_module_sig_enforced(void)
>>> +{
>>> +	return sig_enforce;
>>> +}
>>> +EXPORT_SYMBOL(is_module_sig_enforced);
>>
>> As reported by the test robot, that's not enough.
> 
> Hi Christophe,
> 
> Thanks for testing this.
> 
>> When it was in main.c, is_module_sig_enforced() was build as soon as
>> CONFIG_MODULES was set.
>> Now it is only built when CONFIG_MODULE_SIG is selected,
> 
> Agreed.
> 
>> so you have to modify include/linux/modules.h and have the stub
>> is_module_sig_enforced() when CONFIG_MODULE_SIG is not selected and not
>> only when CONFIG_MODULES is not selected.
> 
> Sure: when Kconfig CONFIG_MODULE_SIG is not selected.
> 
> Luis,
> 
> I can see that the latest series is in mcgrof/modules-testing.
> Should I address the above as a separate patch with "Fixes:" or
> provide a whole new series, with the fix within the same patch?
> In my opinion, another iteration would be cleaner.
> 
> 

On the powerpc list, usually for this kind of stuff, if the fixup 
doesn't impact the other commits of the series, we provide an 
incremental fixup and Michael squashes it in the faulty commit while 
rebasing.

That way you get the advantage of a new iteration without the disadvantages.

Up to Luis to tell what he prefers.

Christophe
Aaron Tomlin March 5, 2022, 8:37 p.m. UTC | #4 
On Wed 2022-03-02 08:08 +0000, Christophe Leroy wrote:
> When it was in main.c, is_module_sig_enforced() was build as soon as 
> CONFIG_MODULES was set.
> 
> Now it is only built when CONFIG_MODULE_SIG is selected, so you have to 
> modify include/linux/modules.h and have the stub 
> is_module_sig_enforced() when CONFIG_MODULE_SIG is not selected and not 
> only when CONFIG_MODULES is not selected.

Hi Christophe,

Looking at this again, perhaps I'm missing something. If I understand
correctly, Kconfig CONFIG_MODULE_SIG cannot be selected without
CONFIG_MODULES; also CONFIG_MODULE_SIG depends on CONFIG_MODULES, no?
So, what is present is enough right i.e. the stub when CONFIG_MODULES is
not enabled?


Kind regards,
Christophe Leroy March 6, 2022, 5:46 p.m. UTC | #5 
Le 05/03/2022 à 21:37, Aaron Tomlin a écrit :
> On Wed 2022-03-02 08:08 +0000, Christophe Leroy wrote:
>> When it was in main.c, is_module_sig_enforced() was build as soon as
>> CONFIG_MODULES was set.
>>
>> Now it is only built when CONFIG_MODULE_SIG is selected, so you have to
>> modify include/linux/modules.h and have the stub
>> is_module_sig_enforced() when CONFIG_MODULE_SIG is not selected and not
>> only when CONFIG_MODULES is not selected.
> 
> Hi Christophe,
> 
> Looking at this again, perhaps I'm missing something. If I understand
> correctly, Kconfig CONFIG_MODULE_SIG cannot be selected without
> CONFIG_MODULES; also CONFIG_MODULE_SIG depends on CONFIG_MODULES, no?
> So, what is present is enough right i.e. the stub when CONFIG_MODULES is
> not enabled?
> 

There are three possibilities:
1/ CONFIG_MODULES=n, CONFIG_MODULE_SIG=n
2/ CONFIG_MODULES=y, CONFIG_MODULE_SIG=n
3/ CONFIG_MODULES=y, CONFIG_MODULE_SIG=y

Case 1/, is_module_sig_enforced() is a static inline stub in 
linux/modules.h returning always false

Case 3/, is_module_sig_enforced() is in kernel/module/signing.c

Case 2/, is_module_sig_enforced() is nowhere.

In linux/modules.h, you have:

#ifdef CONFIG_MODULES
....
a lot of stuff
...
bool is_module_sig_enforced(void);
void set_module_sig_enforced(void);

#else
....
a lot of stuff
...
static inline bool is_module_sig_enforced(void)
{
	return false;
}

static inline void set_module_sig_enforced(void)
{
}
...
some more stuff
#endif



You must take is_module_sig_enforced() out of that #ifdef CONFIG_MODULES 
/ #else / #endif and add after the #endif /* CONFIG_MODULES */:

#ifdef CONFIG_MODULE_SIG
bool is_module_sig_enforced(void);
#else
static inline bool is_module_sig_enforced(void)
{
	return false;
}
#endif

Christophe
Aaron Tomlin March 7, 2022, 9:38 a.m. UTC | #6 
On Sun 2022-03-06 17:46 +0000, Christophe Leroy wrote:
> 2/ CONFIG_MODULES=y, CONFIG_MODULE_SIG=n

Christophe,

I see, I did not notice that the above is even possible.

> Case 2/, is_module_sig_enforced() is nowhere.

Understood.


Luis,

I'll send a v10 later today.


Kind regards,
diff mbox series

Patch

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index a6895bb5598a..d6f646a5da41 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -158,3 +158,12 @@  static inline int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
 	return 0;
 }
 #endif /* CONFIG_STRICT_MODULE_RWX */
+
+#ifdef CONFIG_MODULE_SIG
+int module_sig_check(struct load_info *info, int flags);
+#else /* !CONFIG_MODULE_SIG */
+static inline int module_sig_check(struct load_info *info, int flags)
+{
+	return 0;
+}
+#endif /* !CONFIG_MODULE_SIG */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 5cd63f14b1ef..c63e10c61694 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -23,7 +23,6 @@ 
 #include <linux/vmalloc.h>
 #include <linux/elf.h>
 #include <linux/proc_fs.h>
-#include <linux/security.h>
 #include <linux/seq_file.h>
 #include <linux/syscalls.h>
 #include <linux/fcntl.h>
@@ -127,28 +126,6 @@  static void module_assert_mutex_or_preempt(void)
 #endif
 }
 
-#ifdef CONFIG_MODULE_SIG
-static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
-module_param(sig_enforce, bool_enable_only, 0644);
-
-void set_module_sig_enforced(void)
-{
-	sig_enforce = true;
-}
-#else
-#define sig_enforce false
-#endif
-
-/*
- * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
- * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
- */
-bool is_module_sig_enforced(void)
-{
-	return sig_enforce;
-}
-EXPORT_SYMBOL(is_module_sig_enforced);
-
 /* Block module loading/unloading? */
 int modules_disabled = 0;
 core_param(nomodule, modules_disabled, bint, 0);
@@ -2569,70 +2546,6 @@  static inline void kmemleak_load_module(const struct module *mod,
 }
 #endif
 
-#ifdef CONFIG_MODULE_SIG
-static int module_sig_check(struct load_info *info, int flags)
-{
-	int err = -ENODATA;
-	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
-	const char *reason;
-	const void *mod = info->hdr;
-	bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
-				       MODULE_INIT_IGNORE_VERMAGIC);
-	/*
-	 * Do not allow mangled modules as a module with version information
-	 * removed is no longer the module that was signed.
-	 */
-	if (!mangled_module &&
-	    info->len > markerlen &&
-	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
-		/* We truncate the module to discard the signature */
-		info->len -= markerlen;
-		err = mod_verify_sig(mod, info);
-		if (!err) {
-			info->sig_ok = true;
-			return 0;
-		}
-	}
-
-	/*
-	 * We don't permit modules to be loaded into the trusted kernels
-	 * without a valid signature on them, but if we're not enforcing,
-	 * certain errors are non-fatal.
-	 */
-	switch (err) {
-	case -ENODATA:
-		reason = "unsigned module";
-		break;
-	case -ENOPKG:
-		reason = "module with unsupported crypto";
-		break;
-	case -ENOKEY:
-		reason = "module with unavailable key";
-		break;
-
-	default:
-		/*
-		 * All other errors are fatal, including lack of memory,
-		 * unparseable signatures, and signature check failures --
-		 * even if signatures aren't required.
-		 */
-		return err;
-	}
-
-	if (is_module_sig_enforced()) {
-		pr_notice("Loading of %s is rejected\n", reason);
-		return -EKEYREJECTED;
-	}
-
-	return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
-}
-#else /* !CONFIG_MODULE_SIG */
-static int module_sig_check(struct load_info *info, int flags)
-{
-	return 0;
-}
-#endif /* !CONFIG_MODULE_SIG */
-
 static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
 {
 #if defined(CONFIG_64BIT)
diff --git a/kernel/module/signing.c b/kernel/module/signing.c
index 8aeb6d2ee94b..85c8999dfecf 100644
--- a/kernel/module/signing.c
+++ b/kernel/module/signing.c
@@ -11,9 +11,29 @@ 
 #include <linux/module_signature.h>
 #include <linux/string.h>
 #include <linux/verification.h>
+#include <linux/security.h>
 #include <crypto/public_key.h>
+#include <uapi/linux/module.h>
 #include "internal.h"
 
+static bool sig_enforce = IS_ENABLED(CONFIG_MODULE_SIG_FORCE);
+module_param(sig_enforce, bool_enable_only, 0644);
+
+/*
+ * Export sig_enforce kernel cmdline parameter to allow other subsystems rely
+ * on that instead of directly to CONFIG_MODULE_SIG_FORCE config.
+ */
+bool is_module_sig_enforced(void)
+{
+	return sig_enforce;
+}
+EXPORT_SYMBOL(is_module_sig_enforced);
+
+void set_module_sig_enforced(void)
+{
+	sig_enforce = true;
+}
+
 /*
  * Verify the signature on a module.
  */
@@ -43,3 +63,60 @@  int mod_verify_sig(const void *mod, struct load_info *info)
 				      VERIFYING_MODULE_SIGNATURE,
 				      NULL, NULL);
 }
+
+int module_sig_check(struct load_info *info, int flags)
+{
+	int err = -ENODATA;
+	const unsigned long markerlen = sizeof(MODULE_SIG_STRING) - 1;
+	const char *reason;
+	const void *mod = info->hdr;
+	bool mangled_module = flags & (MODULE_INIT_IGNORE_MODVERSIONS |
+				       MODULE_INIT_IGNORE_VERMAGIC);
+	/*
+	 * Do not allow mangled modules as a module with version information
+	 * removed is no longer the module that was signed.
+	 */
+	if (!mangled_module &&
+	    info->len > markerlen &&
+	    memcmp(mod + info->len - markerlen, MODULE_SIG_STRING, markerlen) == 0) {
+		/* We truncate the module to discard the signature */
+		info->len -= markerlen;
+		err = mod_verify_sig(mod, info);
+		if (!err) {
+			info->sig_ok = true;
+			return 0;
+		}
+	}
+
+	/*
+	 * We don't permit modules to be loaded into the trusted kernels
+	 * without a valid signature on them, but if we're not enforcing,
+	 * certain errors are non-fatal.
+	 */
+	switch (err) {
+	case -ENODATA:
+		reason = "unsigned module";
+		break;
+	case -ENOPKG:
+		reason = "module with unsupported crypto";
+		break;
+	case -ENOKEY:
+		reason = "module with unavailable key";
+		break;
+
+	default:
+		/*
+		 * All other errors are fatal, including lack of memory,
+		 * unparseable signatures, and signature check failures --
+		 * even if signatures aren't required.
+		 */
+		return err;
+	}
+
+	if (is_module_sig_enforced()) {
+		pr_notice("Loading of %s is rejected\n", reason);
+		return -EKEYREJECTED;
+	}
+
+	return security_locked_down(LOCKDOWN_MODULE_SIGNATURE);
+}