2022-02-19 07:03:54

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v6 00/13] module: core code clean up

Hi Luis,

As per your suggestion [1], this is an attempt to refactor and split
optional code out of core module support code into separate components.
This version is based on Linus' commit 7993e65fdd0f ("Merge tag
'mtd/fixes-for-5.17-rc5' of
git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux").
Please let me know your thoughts.

Changes since v5 [2]:

- Updated MAINTAINERS to include the entire kernel/module/ directory
(Christophe Leroy)
- Reintroduce commit a97ac8cb24a3 ("module: fix signature check failures
when using in-kernel decompression") (Michal Suchánek)
- Refactored code to address some (i.e.
--ignore=MULTIPLE_ASSIGNMENTS,ASSIGN_IN_IF was used) style violations
e.g. "Alignment should match open parenthesis", reported by
scripts/checkpatch.pl --strict (Christophe Leroy)
- Used PAGE_ALIGN() and PAGE_ALIGNED() instead (Christophe Leroy)
- Removed sig_enforce from include/linux/module.h as it is only
used in kernel/module/signing.c (Christophe Leroy)
- Added static keyword for anything not used outside a source file
(Christophe Leroy)
- Moved mod_sysfs_teardown() to kernel/module/sysfs.c (Christophe Leroy)
- Removed kdb_modules from kernel/debug/kdb/kdb_private.h
(Christophe Leroy)

Changes since v4 [3]:

- Moved is_livepatch_module() and set_livepatch_module() to
kernel/module/livepatch.c
- Addressed minor compiler warning concerning
kernel/module/internal.h (0-day)
- Resolved style violations reported by scripts/checkpatch.pl
- Dropped patch 5 [4] so external patch [5] can be applied at
a later date post merge into module-next (Christophe Leroy)

Changes since v3 [6]:

- Refactored both is_livepatch_module() and set_livepatch_module(),
respectively, to use IS_ENABLED(CONFIG_LIVEPATCH) (Joe Perches)
- Addressed various compiler warnings e.g., no previous prototype (0-day)

Changes since v2 [7]:

- Moved module decompress support to a separate file
- Made check_modinfo_livepatch() generic (Petr Mladek)
- Removed filename from each newly created file (Luis Chamberlain)
- Addressed some (i.e. --ignore=ASSIGN_IN_IF,AVOID_BUG was used)
minor scripts/checkpatch.pl concerns e.g., use strscpy over
strlcpy and missing a blank line after declarations (Allen)

Changes since v1 [8]:

- Moved module version support code into a new file

[1]: https://lore.kernel.org/lkml/[email protected]/
[2]: https://lore.kernel.org/lkml/[email protected]/
[3]: https://lore.kernel.org/lkml/[email protected]/
[4]: https://lore.kernel.org/lkml/[email protected]/
[5]: https://lore.kernel.org/lkml/203348805c9ac9851d8939d15cb9802ef047b5e2.1643919758.git.christophe.leroy@csgroup.eu/
[6]: https://lore.kernel.org/lkml/[email protected]/
[7]: https://lore.kernel.org/lkml/[email protected]/
[8]: https://lore.kernel.org/lkml/[email protected]/


Aaron Tomlin (13):
module: Move all into module/
module: Simple refactor in preparation for split
module: Make internal.h and decompress.c more compliant
module: Move livepatch support to a separate file
module: Move latched RB-tree support to a separate file
module: Move strict rwx support to a separate file
module: Move extra signature support out of core code
module: Move kmemleak support to a separate file
module: Move kallsyms support into a separate file
module: Move procfs support into a separate file
module: Move sysfs support into a separate file
module: Move kdb_modules list out of core code
module: Move version support into a separate file

MAINTAINERS | 2 +-
include/linux/module.h | 9 +-
kernel/Makefile | 5 +-
kernel/debug/kdb/kdb_main.c | 5 +
kernel/debug/kdb/kdb_private.h | 4 -
kernel/module-internal.h | 50 -
kernel/module/Makefile | 19 +
kernel/module/debug_kmemleak.c | 30 +
.../decompress.c} | 5 +-
kernel/module/internal.h | 284 +++
kernel/module/kallsyms.c | 502 +++++
kernel/module/livepatch.c | 74 +
kernel/{module.c => module/main.c} | 1856 +----------------
kernel/module/procfs.c | 142 ++
.../signature.c} | 0
kernel/module/signing.c | 122 ++
kernel/module/strict_rwx.c | 84 +
kernel/module/sysfs.c | 436 ++++
kernel/module/tree_lookup.c | 109 +
kernel/module/version.c | 109 +
kernel/module_signing.c | 45 -
21 files changed, 2010 insertions(+), 1882 deletions(-)
delete mode 100644 kernel/module-internal.h
create mode 100644 kernel/module/Makefile
create mode 100644 kernel/module/debug_kmemleak.c
rename kernel/{module_decompress.c => module/decompress.c} (99%)
create mode 100644 kernel/module/internal.h
create mode 100644 kernel/module/kallsyms.c
create mode 100644 kernel/module/livepatch.c
rename kernel/{module.c => module/main.c} (64%)
create mode 100644 kernel/module/procfs.c
rename kernel/{module_signature.c => module/signature.c} (100%)
create mode 100644 kernel/module/signing.c
create mode 100644 kernel/module/strict_rwx.c
create mode 100644 kernel/module/sysfs.c
create mode 100644 kernel/module/tree_lookup.c
create mode 100644 kernel/module/version.c
delete mode 100644 kernel/module_signing.c


base-commit: 7993e65fdd0fe07beb9f36f998f9bbef2c0ee391
--
2.34.1


2022-02-19 09:25:00

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v6 06/13] module: Move strict rwx support to a separate file

No functional change.

This patch migrates code that makes module text
and rodata memory read-only and non-text memory
non-executable from core module code into
kernel/module/strict_rwx.c.

Signed-off-by: Aaron Tomlin <[email protected]>
---
kernel/module/Makefile | 1 +
kernel/module/internal.h | 38 +++++++++++++++
kernel/module/main.c | 99 +-------------------------------------
kernel/module/strict_rwx.c | 84 ++++++++++++++++++++++++++++++++
4 files changed, 125 insertions(+), 97 deletions(-)
create mode 100644 kernel/module/strict_rwx.c

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index 6fb21ebe1aa3..3f48343636ff 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -10,4 +10,5 @@ obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
obj-$(CONFIG_LIVEPATCH) += livepatch.o
ifdef CONFIG_MODULES
obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
+obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 57a715454c9e..f4b7e123d625 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -20,6 +20,17 @@
/* Maximum number of characters written by module_flags() */
#define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)

+/*
+ * Modules' sections will be aligned on page boundaries
+ * to ensure complete separation of code and data, but
+ * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
+ */
+#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
+# define debug_align(X) PAGE_ALIGN(X)
+#else
+# define debug_align(X) (X)
+#endif
+
extern struct mutex module_mutex;
extern struct list_head modules;

@@ -126,3 +137,30 @@ static inline struct module *mod_find(unsigned long addr)
return NULL;
}
#endif /* CONFIG_MODULES_TREE_LOOKUP */
+
+#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
+void frob_text(const struct module_layout *layout, int (*set_memory)(unsigned long start,
+ int num_pages));
+#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
+
+#ifdef CONFIG_STRICT_MODULE_RWX
+void frob_rodata(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages));
+void frob_ro_after_init(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages));
+void frob_writable_data(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages));
+void module_enable_ro(const struct module *mod, bool after_init);
+void module_enable_nx(const struct module *mod);
+int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+ char *secstrings, struct module *mod);
+
+#else /* !CONFIG_STRICT_MODULE_RWX */
+static void module_enable_nx(const struct module *mod) { }
+static void module_enable_ro(const struct module *mod, bool after_init) {}
+static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+ char *secstrings, struct module *mod)
+{
+ return 0;
+}
+#endif /* CONFIG_STRICT_MODULE_RWX */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 76b53880ad91..5cd63f14b1ef 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -63,17 +63,6 @@
#define CREATE_TRACE_POINTS
#include <trace/events/module.h>

-/*
- * Modules' sections will be aligned on page boundaries
- * to ensure complete separation of code and data, but
- * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
- */
-#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
-# define debug_align(X) ALIGN(X, PAGE_SIZE)
-#else
-# define debug_align(X) (X)
-#endif
-
/*
* Mutex protects:
* 1) List of modules (also safely readable with preempt_disable),
@@ -1819,8 +1808,8 @@ static void mod_sysfs_teardown(struct module *mod)
* whether we are strict.
*/
#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
-static void frob_text(const struct module_layout *layout,
- int (*set_memory)(unsigned long start, int num_pages))
+void frob_text(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages))
{
BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
@@ -1837,90 +1826,6 @@ static void module_enable_x(const struct module *mod)
static void module_enable_x(const struct module *mod) { }
#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */

-#ifdef CONFIG_STRICT_MODULE_RWX
-static void frob_rodata(const struct module_layout *layout,
- int (*set_memory)(unsigned long start, int num_pages))
-{
- BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->text_size & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
- set_memory((unsigned long)layout->base + layout->text_size,
- (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
-}
-
-static void frob_ro_after_init(const struct module_layout *layout,
- int (*set_memory)(unsigned long start, int num_pages))
-{
- BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->ro_size & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
- set_memory((unsigned long)layout->base + layout->ro_size,
- (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
-}
-
-static void frob_writable_data(const struct module_layout *layout,
- int (*set_memory)(unsigned long start, int num_pages))
-{
- BUG_ON((unsigned long)layout->base & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->ro_after_init_size & (PAGE_SIZE-1));
- BUG_ON((unsigned long)layout->size & (PAGE_SIZE-1));
- set_memory((unsigned long)layout->base + layout->ro_after_init_size,
- (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
-}
-
-static void module_enable_ro(const struct module *mod, bool after_init)
-{
- if (!rodata_enabled)
- return;
-
- set_vm_flush_reset_perms(mod->core_layout.base);
- set_vm_flush_reset_perms(mod->init_layout.base);
- frob_text(&mod->core_layout, set_memory_ro);
-
- frob_rodata(&mod->core_layout, set_memory_ro);
- frob_text(&mod->init_layout, set_memory_ro);
- frob_rodata(&mod->init_layout, set_memory_ro);
-
- if (after_init)
- frob_ro_after_init(&mod->core_layout, set_memory_ro);
-}
-
-static void module_enable_nx(const struct module *mod)
-{
- frob_rodata(&mod->core_layout, set_memory_nx);
- frob_ro_after_init(&mod->core_layout, set_memory_nx);
- frob_writable_data(&mod->core_layout, set_memory_nx);
- frob_rodata(&mod->init_layout, set_memory_nx);
- frob_writable_data(&mod->init_layout, set_memory_nx);
-}
-
-static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
- char *secstrings, struct module *mod)
-{
- const unsigned long shf_wx = SHF_WRITE|SHF_EXECINSTR;
- int i;
-
- for (i = 0; i < hdr->e_shnum; i++) {
- if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
- pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
- mod->name, secstrings + sechdrs[i].sh_name, i);
- return -ENOEXEC;
- }
- }
-
- return 0;
-}
-
-#else /* !CONFIG_STRICT_MODULE_RWX */
-static void module_enable_nx(const struct module *mod) { }
-static void module_enable_ro(const struct module *mod, bool after_init) {}
-static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
- char *secstrings, struct module *mod)
-{
- return 0;
-}
-#endif /* CONFIG_STRICT_MODULE_RWX */
-
void __weak module_memfree(void *module_region)
{
/*
diff --git a/kernel/module/strict_rwx.c b/kernel/module/strict_rwx.c
new file mode 100644
index 000000000000..c642889f8e77
--- /dev/null
+++ b/kernel/module/strict_rwx.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Module strict rwx
+ *
+ * Copyright (C) 2015 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/vmalloc.h>
+#include <linux/set_memory.h>
+#include "internal.h"
+
+void frob_rodata(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages))
+{
+ BUG_ON(!PAGE_ALIGNED(layout->base));
+ BUG_ON(!PAGE_ALIGNED(layout->text_size));
+ BUG_ON(!PAGE_ALIGNED(layout->ro_size));
+ set_memory((unsigned long)layout->base + layout->text_size,
+ (layout->ro_size - layout->text_size) >> PAGE_SHIFT);
+}
+
+void frob_ro_after_init(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages))
+{
+ BUG_ON(!PAGE_ALIGNED(layout->base));
+ BUG_ON(!PAGE_ALIGNED(layout->ro_size));
+ BUG_ON(!PAGE_ALIGNED(layout->ro_after_init_size));
+ set_memory((unsigned long)layout->base + layout->ro_size,
+ (layout->ro_after_init_size - layout->ro_size) >> PAGE_SHIFT);
+}
+
+void frob_writable_data(const struct module_layout *layout,
+ int (*set_memory)(unsigned long start, int num_pages))
+{
+ BUG_ON(!PAGE_ALIGNED(layout->base));
+ BUG_ON(!PAGE_ALIGNED(layout->ro_after_init_size));
+ BUG_ON(!PAGE_ALIGNED(layout->size));
+ set_memory((unsigned long)layout->base + layout->ro_after_init_size,
+ (layout->size - layout->ro_after_init_size) >> PAGE_SHIFT);
+}
+
+void module_enable_ro(const struct module *mod, bool after_init)
+{
+ if (!rodata_enabled)
+ return;
+
+ set_vm_flush_reset_perms(mod->core_layout.base);
+ set_vm_flush_reset_perms(mod->init_layout.base);
+ frob_text(&mod->core_layout, set_memory_ro);
+
+ frob_rodata(&mod->core_layout, set_memory_ro);
+ frob_text(&mod->init_layout, set_memory_ro);
+ frob_rodata(&mod->init_layout, set_memory_ro);
+
+ if (after_init)
+ frob_ro_after_init(&mod->core_layout, set_memory_ro);
+}
+
+void module_enable_nx(const struct module *mod)
+{
+ frob_rodata(&mod->core_layout, set_memory_nx);
+ frob_ro_after_init(&mod->core_layout, set_memory_nx);
+ frob_writable_data(&mod->core_layout, set_memory_nx);
+ frob_rodata(&mod->init_layout, set_memory_nx);
+ frob_writable_data(&mod->init_layout, set_memory_nx);
+}
+
+int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
+ char *secstrings, struct module *mod)
+{
+ const unsigned long shf_wx = SHF_WRITE | SHF_EXECINSTR;
+ int i;
+
+ for (i = 0; i < hdr->e_shnum; i++) {
+ if ((sechdrs[i].sh_flags & shf_wx) == shf_wx) {
+ pr_err("%s: section %s (index %d) has invalid WRITE|EXEC flags\n",
+ mod->name, secstrings + sechdrs[i].sh_name, i);
+ return -ENOEXEC;
+ }
+ }
+
+ return 0;
+}
--
2.34.1

2022-02-19 09:47:27

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v6 03/13] module: Make internal.h and decompress.c more compliant

This patch will address the following warning and style violations
generated by ./scripts/checkpatch.pl in strict mode:

WARNING: Use #include <linux/module.h> instead of <asm/module.h>
#10: FILE: kernel/module/internal.h:10:
+#include <asm/module.h>

CHECK: spaces preferred around that '-' (ctx:VxV)
#18: FILE: kernel/module/internal.h:18:
+#define INIT_OFFSET_MASK (1UL << (BITS_PER_LONG-1))

CHECK: Please use a blank line after function/struct/union/enum declarations
#69: FILE: kernel/module/internal.h:69:
+}
+static inline void module_decompress_cleanup(struct load_info *info)
^
CHECK: extern prototypes should be avoided in .h files
#84: FILE: kernel/module/internal.h:84:
+extern int mod_verify_sig(const void *mod, struct load_info *info);

WARNING: Missing a blank line after declarations
#116: FILE: kernel/module/decompress.c:116:
+ struct page *page = module_get_next_page(info);
+ if (!page) {

WARNING: Missing a blank line after declarations
#174: FILE: kernel/module/decompress.c:174:
+ struct page *page = module_get_next_page(info);
+ if (!page) {

CHECK: Please use a blank line after function/struct/union/enum declarations
#258: FILE: kernel/module/decompress.c:258:
+}
+static struct kobj_attribute module_compression_attr = __ATTR_RO(compression);

Note: Fortunately, the multiple-include optimisation found in
include/linux/module.h will prevent duplication/or inclusion more than
once.

Fixes: f314dfea16a ("modsign: log module name in the event of an error")
Reviewed-by: Christophe Leroy <[email protected]>
Signed-off-by: Aaron Tomlin <[email protected]>
---
kernel/module/decompress.c | 3 +++
kernel/module/internal.h | 6 ++++--
2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/module/decompress.c b/kernel/module/decompress.c
index d14d6443225a..2fc7081dd7c1 100644
--- a/kernel/module/decompress.c
+++ b/kernel/module/decompress.c
@@ -113,6 +113,7 @@ static ssize_t module_gzip_decompress(struct load_info *info,

do {
struct page *page = module_get_next_page(info);
+
if (!page) {
retval = -ENOMEM;
goto out_inflate_end;
@@ -171,6 +172,7 @@ static ssize_t module_xz_decompress(struct load_info *info,

do {
struct page *page = module_get_next_page(info);
+
if (!page) {
retval = -ENOMEM;
goto out;
@@ -256,6 +258,7 @@ static ssize_t compression_show(struct kobject *kobj,
{
return sysfs_emit(buf, "%s\n", __stringify(MODULE_COMPRESSION));
}
+
static struct kobj_attribute module_compression_attr = __ATTR_RO(compression);

static int __init module_decompress_sysfs_init(void)
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index ea8c4c02614c..e0775e66bcf7 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -6,7 +6,8 @@
*/

#include <linux/elf.h>
-#include <asm/module.h>
+#include <linux/compiler.h>
+#include <linux/module.h>
#include <linux/mutex.h>

#ifndef ARCH_SHF_SMALL
@@ -54,7 +55,7 @@ struct load_info {
} index;
};

-extern int mod_verify_sig(const void *mod, struct load_info *info);
+int mod_verify_sig(const void *mod, struct load_info *info);

#ifdef CONFIG_MODULE_DECOMPRESS
int module_decompress(struct load_info *info, const void *buf, size_t size);
@@ -65,6 +66,7 @@ static inline int module_decompress(struct load_info *info,
{
return -EOPNOTSUPP;
}
+
static inline void module_decompress_cleanup(struct load_info *info)
{
}
--
2.34.1

2022-02-19 13:06:27

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v6 07/13] module: Move extra signature support out of core code

No functional change.

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

Signed-off-by: Aaron Tomlin <[email protected]>
---
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/internal.h b/kernel/module/internal.h
index f4b7e123d625..0fdd9c0cd77d 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -164,3 +164,12 @@ static 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);
+}
--
2.34.1

2022-02-19 18:37:11

by Luis Chamberlain

[permalink] [raw]
Subject: Re: [PATCH v6 00/13] module: core code clean up

On Fri, Feb 18, 2022 at 09:24:58PM +0000, Aaron Tomlin wrote:
> Hi Luis,
>
> As per your suggestion [1], this is an attempt to refactor and split
> optional code out of core module support code into separate components.
> This version is based on Linus' commit 7993e65fdd0f ("Merge tag
> 'mtd/fixes-for-5.17-rc5' of
> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux").
> Please let me know your thoughts.

Fantastic, thanks for doing all this work, I've pushed this out to
modules-next so that the testing can start as this will be in linux-next
soon. I'll obviously wait for more reviews, we have a long time before this
gets merged to Linus, so just want to start getting testing done now rather
than later. And other folks are depending on your changes to start
getting their own code up too.

Thanks!

Luis

2022-02-20 04:22:58

by Aaron Tomlin

[permalink] [raw]
Subject: [PATCH v6 05/13] module: Move latched RB-tree support to a separate file

No functional change.

This patch migrates module latched RB-tree support
(e.g. see __module_address()) from core module code
into kernel/module/tree_lookup.c.

Signed-off-by: Aaron Tomlin <[email protected]>
---
kernel/module/Makefile | 3 +
kernel/module/internal.h | 33 +++++++++
kernel/module/main.c | 130 ++----------------------------------
kernel/module/tree_lookup.c | 109 ++++++++++++++++++++++++++++++
4 files changed, 149 insertions(+), 126 deletions(-)
create mode 100644 kernel/module/tree_lookup.c

diff --git a/kernel/module/Makefile b/kernel/module/Makefile
index ba3ebdb7055b..6fb21ebe1aa3 100644
--- a/kernel/module/Makefile
+++ b/kernel/module/Makefile
@@ -8,3 +8,6 @@ obj-$(CONFIG_MODULE_DECOMPRESS) += decompress.o
obj-$(CONFIG_MODULE_SIG) += signing.o
obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
obj-$(CONFIG_LIVEPATCH) += livepatch.o
+ifdef CONFIG_MODULES
+obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
+endif
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index ad7a444253ed..57a715454c9e 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -9,6 +9,7 @@
#include <linux/compiler.h>
#include <linux/module.h>
#include <linux/mutex.h>
+#include <linux/rculist.h>

#ifndef ARCH_SHF_SMALL
#define ARCH_SHF_SMALL 0
@@ -93,3 +94,35 @@ static inline void module_decompress_cleanup(struct load_info *info)
{
}
#endif
+
+#ifdef CONFIG_MODULES_TREE_LOOKUP
+struct mod_tree_root {
+ struct latch_tree_root root;
+ unsigned long addr_min;
+ unsigned long addr_max;
+};
+
+extern struct mod_tree_root mod_tree;
+
+void mod_tree_insert(struct module *mod);
+void mod_tree_remove_init(struct module *mod);
+void mod_tree_remove(struct module *mod);
+struct module *mod_find(unsigned long addr);
+#else /* !CONFIG_MODULES_TREE_LOOKUP */
+
+static void mod_tree_insert(struct module *mod) { }
+static void mod_tree_remove_init(struct module *mod) { }
+static void mod_tree_remove(struct module *mod) { }
+static inline struct module *mod_find(unsigned long addr)
+{
+ struct module *mod;
+
+ list_for_each_entry_rcu(mod, &modules, list,
+ lockdep_is_held(&module_mutex)) {
+ if (within_module(addr, mod))
+ return mod;
+ }
+
+ return NULL;
+}
+#endif /* CONFIG_MODULES_TREE_LOOKUP */
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 3596ebf3a6c3..76b53880ad91 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -90,138 +90,16 @@ static DECLARE_WORK(init_free_wq, do_free_init);
static LLIST_HEAD(init_free_list);

#ifdef CONFIG_MODULES_TREE_LOOKUP
-
-/*
- * Use a latched RB-tree for __module_address(); this allows us to use
- * RCU-sched lookups of the address from any context.
- *
- * This is conditional on PERF_EVENTS || TRACING because those can really hit
- * __module_address() hard by doing a lot of stack unwinding; potentially from
- * NMI context.
- */
-
-static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
-{
- struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
-
- return (unsigned long)layout->base;
-}
-
-static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
-{
- struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
-
- return (unsigned long)layout->size;
-}
-
-static __always_inline bool
-mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
-{
- return __mod_tree_val(a) < __mod_tree_val(b);
-}
-
-static __always_inline int
-mod_tree_comp(void *key, struct latch_tree_node *n)
-{
- unsigned long val = (unsigned long)key;
- unsigned long start, end;
-
- start = __mod_tree_val(n);
- if (val < start)
- return -1;
-
- end = start + __mod_tree_size(n);
- if (val >= end)
- return 1;
-
- return 0;
-}
-
-static const struct latch_tree_ops mod_tree_ops = {
- .less = mod_tree_less,
- .comp = mod_tree_comp,
-};
-
-static struct mod_tree_root {
- struct latch_tree_root root;
- unsigned long addr_min;
- unsigned long addr_max;
-} mod_tree __cacheline_aligned = {
+struct mod_tree_root mod_tree __cacheline_aligned = {
.addr_min = -1UL,
};

#define module_addr_min mod_tree.addr_min
#define module_addr_max mod_tree.addr_max

-static noinline void __mod_tree_insert(struct mod_tree_node *node)
-{
- latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
-}
-
-static void __mod_tree_remove(struct mod_tree_node *node)
-{
- latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
-}
-
-/*
- * These modifications: insert, remove_init and remove; are serialized by the
- * module_mutex.
- */
-static void mod_tree_insert(struct module *mod)
-{
- mod->core_layout.mtn.mod = mod;
- mod->init_layout.mtn.mod = mod;
-
- __mod_tree_insert(&mod->core_layout.mtn);
- if (mod->init_layout.size)
- __mod_tree_insert(&mod->init_layout.mtn);
-}
-
-static void mod_tree_remove_init(struct module *mod)
-{
- if (mod->init_layout.size)
- __mod_tree_remove(&mod->init_layout.mtn);
-}
-
-static void mod_tree_remove(struct module *mod)
-{
- __mod_tree_remove(&mod->core_layout.mtn);
- mod_tree_remove_init(mod);
-}
-
-static struct module *mod_find(unsigned long addr)
-{
- struct latch_tree_node *ltn;
-
- ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
- if (!ltn)
- return NULL;
-
- return container_of(ltn, struct mod_tree_node, node)->mod;
-}
-
-#else /* MODULES_TREE_LOOKUP */
-
-static unsigned long module_addr_min = -1UL, module_addr_max = 0;
-
-static void mod_tree_insert(struct module *mod) { }
-static void mod_tree_remove_init(struct module *mod) { }
-static void mod_tree_remove(struct module *mod) { }
-
-static struct module *mod_find(unsigned long addr)
-{
- struct module *mod;
-
- list_for_each_entry_rcu(mod, &modules, list,
- lockdep_is_held(&module_mutex)) {
- if (within_module(addr, mod))
- return mod;
- }
-
- return NULL;
-}
-
-#endif /* MODULES_TREE_LOOKUP */
+#else /* !CONFIG_MODULES_TREE_LOOKUP */
+static unsigned long module_addr_min = -1UL, module_addr_max;
+#endif /* CONFIG_MODULES_TREE_LOOKUP */

/*
* Bounds of module text, for speeding up __module_address.
diff --git a/kernel/module/tree_lookup.c b/kernel/module/tree_lookup.c
new file mode 100644
index 000000000000..0bc4ec3b22ce
--- /dev/null
+++ b/kernel/module/tree_lookup.c
@@ -0,0 +1,109 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Modules tree lookup
+ *
+ * Copyright (C) 2015 Peter Zijlstra
+ * Copyright (C) 2015 Rusty Russell
+ */
+
+#include <linux/module.h>
+#include <linux/rbtree_latch.h>
+#include "internal.h"
+
+/*
+ * Use a latched RB-tree for __module_address(); this allows us to use
+ * RCU-sched lookups of the address from any context.
+ *
+ * This is conditional on PERF_EVENTS || TRACING because those can really hit
+ * __module_address() hard by doing a lot of stack unwinding; potentially from
+ * NMI context.
+ */
+
+static __always_inline unsigned long __mod_tree_val(struct latch_tree_node *n)
+{
+ struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
+
+ return (unsigned long)layout->base;
+}
+
+static __always_inline unsigned long __mod_tree_size(struct latch_tree_node *n)
+{
+ struct module_layout *layout = container_of(n, struct module_layout, mtn.node);
+
+ return (unsigned long)layout->size;
+}
+
+static __always_inline bool
+mod_tree_less(struct latch_tree_node *a, struct latch_tree_node *b)
+{
+ return __mod_tree_val(a) < __mod_tree_val(b);
+}
+
+static __always_inline int
+mod_tree_comp(void *key, struct latch_tree_node *n)
+{
+ unsigned long val = (unsigned long)key;
+ unsigned long start, end;
+
+ start = __mod_tree_val(n);
+ if (val < start)
+ return -1;
+
+ end = start + __mod_tree_size(n);
+ if (val >= end)
+ return 1;
+
+ return 0;
+}
+
+static const struct latch_tree_ops mod_tree_ops = {
+ .less = mod_tree_less,
+ .comp = mod_tree_comp,
+};
+
+static noinline void __mod_tree_insert(struct mod_tree_node *node)
+{
+ latch_tree_insert(&node->node, &mod_tree.root, &mod_tree_ops);
+}
+
+static void __mod_tree_remove(struct mod_tree_node *node)
+{
+ latch_tree_erase(&node->node, &mod_tree.root, &mod_tree_ops);
+}
+
+/*
+ * These modifications: insert, remove_init and remove; are serialized by the
+ * module_mutex.
+ */
+void mod_tree_insert(struct module *mod)
+{
+ mod->core_layout.mtn.mod = mod;
+ mod->init_layout.mtn.mod = mod;
+
+ __mod_tree_insert(&mod->core_layout.mtn);
+ if (mod->init_layout.size)
+ __mod_tree_insert(&mod->init_layout.mtn);
+}
+
+void mod_tree_remove_init(struct module *mod)
+{
+ if (mod->init_layout.size)
+ __mod_tree_remove(&mod->init_layout.mtn);
+}
+
+void mod_tree_remove(struct module *mod)
+{
+ __mod_tree_remove(&mod->core_layout.mtn);
+ mod_tree_remove_init(mod);
+}
+
+struct module *mod_find(unsigned long addr)
+{
+ struct latch_tree_node *ltn;
+
+ ltn = latch_tree_find((void *)addr, &mod_tree.root, &mod_tree_ops);
+ if (!ltn)
+ return NULL;
+
+ return container_of(ltn, struct mod_tree_node, node)->mod;
+}
--
2.34.1

2022-02-21 09:06:38

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v6 06/13] module: Move strict rwx support to a separate file



Le 18/02/2022 à 22:25, Aaron Tomlin a écrit :
> No functional change.
>
> This patch migrates code that makes module text
> and rodata memory read-only and non-text memory
> non-executable from core module code into
> kernel/module/strict_rwx.c.
>
> Signed-off-by: Aaron Tomlin <[email protected]>
> ---
> kernel/module/Makefile | 1 +
> kernel/module/internal.h | 38 +++++++++++++++
> kernel/module/main.c | 99 +-------------------------------------
> kernel/module/strict_rwx.c | 84 ++++++++++++++++++++++++++++++++
> 4 files changed, 125 insertions(+), 97 deletions(-)
> create mode 100644 kernel/module/strict_rwx.c

CC kernel/module/strict_rwx.o
In file included from ./include/linux/build_bug.h:5,
from ./include/linux/container_of.h:5,
from ./include/linux/list.h:5,
from ./include/linux/module.h:12,
from kernel/module/strict_rwx.c:8:
kernel/module/strict_rwx.c: In function 'frob_rodata':
kernel/module/strict_rwx.c:16:17: error: implicit declaration of
function 'PAGE_ALIGNED'; did you mean 'IS_ALIGNED'?
[-Werror=implicit-function-declaration]
16 | BUG_ON(!PAGE_ALIGNED(layout->base));
| ^~~~~~~~~~~~
./include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
78 | # define unlikely(x) __builtin_expect(!!(x), 0)
| ^
kernel/module/strict_rwx.c:16:9: note: in expansion of macro 'BUG_ON'
16 | BUG_ON(!PAGE_ALIGNED(layout->base));
| ^~~~~~
cc1: some warnings being treated as errors
make[2]: *** [scripts/Makefile.build:288 : kernel/module/strict_rwx.o]
Erreur 1



You have to include <linux/mm.h>

Christophe

2022-02-21 14:52:50

by Aaron Tomlin

[permalink] [raw]
Subject: Re: [PATCH v6 06/13] module: Move strict rwx support to a separate file

On Mon 2022-02-21 06:31 +0000, Christophe Leroy wrote:
> CC kernel/module/strict_rwx.o
> In file included from ./include/linux/build_bug.h:5,
> from ./include/linux/container_of.h:5,
> from ./include/linux/list.h:5,
> from ./include/linux/module.h:12,
> from kernel/module/strict_rwx.c:8:
> kernel/module/strict_rwx.c: In function 'frob_rodata':
> kernel/module/strict_rwx.c:16:17: error: implicit declaration of
> function 'PAGE_ALIGNED'; did you mean 'IS_ALIGNED'?
> [-Werror=implicit-function-declaration]
> 16 | BUG_ON(!PAGE_ALIGNED(layout->base));
> | ^~~~~~~~~~~~
> ./include/linux/compiler.h:78:45: note: in definition of macro 'unlikely'
> 78 | # define unlikely(x) __builtin_expect(!!(x), 0)
> | ^
> kernel/module/strict_rwx.c:16:9: note: in expansion of macro 'BUG_ON'
> 16 | BUG_ON(!PAGE_ALIGNED(layout->base));
> | ^~~~~~
> cc1: some warnings being treated as errors
> make[2]: *** [scripts/Makefile.build:288 : kernel/module/strict_rwx.o]
> Erreur 1
>
>
>
> You have to include <linux/mm.h>

Christophe,

Strange, I have not seen this before. Locally, looking at
kernel/module/strict_rwx.i it is eventually included. Anyhow, you are
right. Also, it will not hurt due to the multiple-include optimisation
found in include/linux/mm.h.


Kind regards,

--
Aaron Tomlin

2022-02-21 16:25:37

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v6 05/13] module: Move latched RB-tree support to a separate file



Le 18/02/2022 à 22:25, Aaron Tomlin a écrit :
> No functional change.
>
> This patch migrates module latched RB-tree support
> (e.g. see __module_address()) from core module code
> into kernel/module/tree_lookup.c.
>
> Signed-off-by: Aaron Tomlin <[email protected]>
> ---
> kernel/module/Makefile | 3 +
> kernel/module/internal.h | 33 +++++++++
> kernel/module/main.c | 130 ++----------------------------------
> kernel/module/tree_lookup.c | 109 ++++++++++++++++++++++++++++++
> 4 files changed, 149 insertions(+), 126 deletions(-)
> create mode 100644 kernel/module/tree_lookup.c


CC kernel/module/main.o
kernel/module/main.c:3723:6: warning: no previous prototype for
'module_layout' [-Wmissing-prototypes]
3723 | void module_layout(struct module *mod,
| ^~~~~~~~~~~~~
CC kernel/module/strict_rwx.o
In file included from kernel/module/strict_rwx.c:12:
kernel/module/internal.h:140:13: warning: 'mod_tree_remove' defined but
not used [-Wunused-function]
140 | static void mod_tree_remove(struct module *mod) { }
| ^~~~~~~~~~~~~~~
kernel/module/internal.h:139:13: warning: 'mod_tree_remove_init' defined
but not used [-Wunused-function]
139 | static void mod_tree_remove_init(struct module *mod) { }
| ^~~~~~~~~~~~~~~~~~~~
kernel/module/internal.h:138:13: warning: 'mod_tree_insert' defined but
not used [-Wunused-function]
138 | static void mod_tree_insert(struct module *mod) { }
| ^~~~~~~~~~~~~~~
CC kernel/module/kallsyms.o
In file included from kernel/module/kallsyms.c:12:
kernel/module/internal.h:140:13: warning: 'mod_tree_remove' defined but
not used [-Wunused-function]
140 | static void mod_tree_remove(struct module *mod) { }
| ^~~~~~~~~~~~~~~
kernel/module/internal.h:139:13: warning: 'mod_tree_remove_init' defined
but not used [-Wunused-function]
139 | static void mod_tree_remove_init(struct module *mod) { }
| ^~~~~~~~~~~~~~~~~~~~
kernel/module/internal.h:138:13: warning: 'mod_tree_insert' defined but
not used [-Wunused-function]
138 | static void mod_tree_insert(struct module *mod) { }
| ^~~~~~~~~~~~~~~
CC kernel/module/procfs.o
In file included from kernel/module/procfs.c:13:
kernel/module/internal.h:140:13: warning: 'mod_tree_remove' defined but
not used [-Wunused-function]
140 | static void mod_tree_remove(struct module *mod) { }
| ^~~~~~~~~~~~~~~
kernel/module/internal.h:139:13: warning: 'mod_tree_remove_init' defined
but not used [-Wunused-function]
139 | static void mod_tree_remove_init(struct module *mod) { }
| ^~~~~~~~~~~~~~~~~~~~
kernel/module/internal.h:138:13: warning: 'mod_tree_insert' defined but
not used [-Wunused-function]
138 | static void mod_tree_insert(struct module *mod) { }
| ^~~~~~~~~~~~~~~


Christophe

2022-02-21 18:51:42

by Miroslav Benes

[permalink] [raw]
Subject: Re: [PATCH v6 00/13] module: core code clean up

On Fri, 18 Feb 2022, Luis Chamberlain wrote:

> On Fri, Feb 18, 2022 at 09:24:58PM +0000, Aaron Tomlin wrote:
> > Hi Luis,
> >
> > As per your suggestion [1], this is an attempt to refactor and split
> > optional code out of core module support code into separate components.
> > This version is based on Linus' commit 7993e65fdd0f ("Merge tag
> > 'mtd/fixes-for-5.17-rc5' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux").
> > Please let me know your thoughts.
>
> Fantastic, thanks for doing all this work, I've pushed this out to
> modules-next so that the testing can start as this will be in linux-next
> soon. I'll obviously wait for more reviews, we have a long time before this
> gets merged to Linus, so just want to start getting testing done now rather
> than later. And other folks are depending on your changes to start
> getting their own code up too.

Aaron's series is unfortunately split. Could you also push out the
remaining 3 patches ([email protected]), please?

Miroslav

2022-02-21 21:19:34

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v6 06/13] module: Move strict rwx support to a separate file



Le 18/02/2022 à 22:25, Aaron Tomlin a écrit :
> No functional change.
>
> This patch migrates code that makes module text
> and rodata memory read-only and non-text memory
> non-executable from core module code into
> kernel/module/strict_rwx.c.
>
> Signed-off-by: Aaron Tomlin <[email protected]>
> ---
> kernel/module/Makefile | 1 +
> kernel/module/internal.h | 38 +++++++++++++++
> kernel/module/main.c | 99 +-------------------------------------
> kernel/module/strict_rwx.c | 84 ++++++++++++++++++++++++++++++++
> 4 files changed, 125 insertions(+), 97 deletions(-)
> create mode 100644 kernel/module/strict_rwx.c


kernel/module/internal.h:182:12: error: 'module_enforce_rwx_sections'
defined but not used [-Werror=unused-function]
kernel/module/internal.h:181:13: error: 'module_enable_ro' defined but
not used [-Werror=unused-function]
kernel/module/internal.h:180:13: error: 'module_enable_nx' defined but
not used [-Werror=unused-function]
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:288: kernel/module/signing.o] Error 1

2022-02-22 04:48:43

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v6 06/13] module: Move strict rwx support to a separate file



Le 18/02/2022 à 22:25, Aaron Tomlin a écrit :
> No functional change.
>
> This patch migrates code that makes module text
> and rodata memory read-only and non-text memory
> non-executable from core module code into
> kernel/module/strict_rwx.c.
>
> Signed-off-by: Aaron Tomlin <[email protected]>
> ---
> kernel/module/Makefile | 1 +
> kernel/module/internal.h | 38 +++++++++++++++
> kernel/module/main.c | 99 +-------------------------------------
> kernel/module/strict_rwx.c | 84 ++++++++++++++++++++++++++++++++
> 4 files changed, 125 insertions(+), 97 deletions(-)
> create mode 100644 kernel/module/strict_rwx.c
>
> diff --git a/kernel/module/Makefile b/kernel/module/Makefile
> index 6fb21ebe1aa3..3f48343636ff 100644
> --- a/kernel/module/Makefile
> +++ b/kernel/module/Makefile
> @@ -10,4 +10,5 @@ obj-$(CONFIG_MODULE_SIG_FORMAT) += signature.o
> obj-$(CONFIG_LIVEPATCH) += livepatch.o
> ifdef CONFIG_MODULES
> obj-$(CONFIG_MODULES_TREE_LOOKUP) += tree_lookup.o
> +obj-$(CONFIG_STRICT_MODULE_RWX) += strict_rwx.o
> endif
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 57a715454c9e..f4b7e123d625 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -20,6 +20,17 @@
> /* Maximum number of characters written by module_flags() */
> #define MODULE_FLAGS_BUF_SIZE (TAINT_FLAGS_COUNT + 4)
>
> +/*
> + * Modules' sections will be aligned on page boundaries
> + * to ensure complete separation of code and data, but
> + * only when CONFIG_ARCH_HAS_STRICT_MODULE_RWX=y
> + */
> +#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
> +# define debug_align(X) PAGE_ALIGN(X)
> +#else
> +# define debug_align(X) (X)
> +#endif
> +
> extern struct mutex module_mutex;
> extern struct list_head modules;
>
> @@ -126,3 +137,30 @@ static inline struct module *mod_find(unsigned long addr)
> return NULL;
> }
> #endif /* CONFIG_MODULES_TREE_LOOKUP */
> +
> +#ifdef CONFIG_ARCH_HAS_STRICT_MODULE_RWX
> +void frob_text(const struct module_layout *layout, int (*set_memory)(unsigned long start,
> + int num_pages));
> +#endif /* CONFIG_ARCH_HAS_STRICT_MODULE_RWX */
> +
> +#ifdef CONFIG_STRICT_MODULE_RWX
> +void frob_rodata(const struct module_layout *layout,
> + int (*set_memory)(unsigned long start, int num_pages));
> +void frob_ro_after_init(const struct module_layout *layout,
> + int (*set_memory)(unsigned long start, int num_pages));
> +void frob_writable_data(const struct module_layout *layout,
> + int (*set_memory)(unsigned long start, int num_pages));

Those three frob_() functions are only used in strict_rwx.c, they should
not appear in internal.h and should be static in strict_rwx.c

> +void module_enable_ro(const struct module *mod, bool after_init);
> +void module_enable_nx(const struct module *mod);
> +int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> + char *secstrings, struct module *mod);
> +
> +#else /* !CONFIG_STRICT_MODULE_RWX */
> +static void module_enable_nx(const struct module *mod) { }
> +static void module_enable_ro(const struct module *mod, bool after_init) {}
> +static int module_enforce_rwx_sections(Elf_Ehdr *hdr, Elf_Shdr *sechdrs,
> + char *secstrings, struct module *mod)

Those three must be static inline

> +{
> + return 0;
> +}
> +#endif /* CONFIG_STRICT_MODULE_RWX */


Christophe

2022-02-22 11:24:44

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v6 00/13] module: core code clean up



Le 19/02/2022 à 03:12, Luis Chamberlain a écrit :
> On Fri, Feb 18, 2022 at 09:24:58PM +0000, Aaron Tomlin wrote:
>> Hi Luis,
>>
>> As per your suggestion [1], this is an attempt to refactor and split
>> optional code out of core module support code into separate components.
>> This version is based on Linus' commit 7993e65fdd0f ("Merge tag
>> 'mtd/fixes-for-5.17-rc5' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux").
>> Please let me know your thoughts.
>
> Fantastic, thanks for doing all this work, I've pushed this out to
> modules-next so that the testing can start as this will be in linux-next
> soon. I'll obviously wait for more reviews, we have a long time before this
> gets merged to Linus, so just want to start getting testing done now rather
> than later. And other folks are depending on your changes to start
> getting their own code up too.
>

Hi Luis,

modules-next misses the 3 last patches from Aaron.

Aaron's series as build problems, I sent 4 fixups which allows it to
build, then I rebased my series on top of that.

And I added on top of it some misc cleanups.

All this has undergone kisskb build test at
http://kisskb.ellerman.id.au/kisskb/head/d8ee36c58284baf315e8aa3532a8d97abae8026d/
This only build failure (indeed a final link failure) is unrelated to my
series.

Everything is also available on https://github.com/chleroy/linux.git in
branch module-v4

Looking forward to getting everything into modules-next hence into
linux-next.

Christophe

2022-02-22 14:14:10

by Aaron Tomlin

[permalink] [raw]
Subject: Re: [PATCH v6 00/13] module: core code clean up

On Fri 2022-02-18 18:12 -0800, Luis Chamberlain wrote:
> On Fri, Feb 18, 2022 at 09:24:58PM +0000, Aaron Tomlin wrote:
> > Hi Luis,
> >
> > As per your suggestion [1], this is an attempt to refactor and split
> > optional code out of core module support code into separate components.
> > This version is based on Linus' commit 7993e65fdd0f ("Merge tag
> > 'mtd/fixes-for-5.17-rc5' of
> > git://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux").
> > Please let me know your thoughts.
>
> Fantastic, thanks for doing all this work, I've pushed this out to
> modules-next so that the testing can start as this will be in linux-next
> soon. I'll obviously wait for more reviews, we have a long time before this
> gets merged to Linus, so just want to start getting testing done now rather
> than later. And other folks are depending on your changes to start
> getting their own code up too.

Hi Luis,

No problem. Unfortunately, this was not the complete series. I can send a
v7 later today with the suggested improvements made by Christophe.


Kind regards,

--
Aaron Tomlin