From: Luis Chamberlain <mcgrof@kernel.org>
To: linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org,
pmladek@suse.com, david@redhat.com, petr.pavlu@suse.com,
prarit@redhat.com
Cc: christophe.leroy@csgroup.eu, song@kernel.org, mcgrof@kernel.org
Subject: [PATCH 1/5] module: add sanity check for ELF module section
Date: Sun, 19 Mar 2023 14:35:38 -0700 [thread overview]
Message-ID: <20230319213542.1790479-2-mcgrof@kernel.org> (raw)
In-Reply-To: <20230319213542.1790479-1-mcgrof@kernel.org>
The ELF ".gnu.linkonce.this_module" section is special, it is what we
use to construct the struct module __this_module, which THIS_MODULE
points to. When userspace loads a module we always deal first with a
copy of the userspace buffer, and twiddle with the userspace copy's
version of the struct module. Eventually we allocate memory to do a
memcpy() of that struct module, under the assumption that the module
size is right. But we have no validity checks against the size or
the requirements for the section.
Add some validity checks for the special module section early and while
at it, cache the module section index early, so we don't have to do that
later.
While at it, just move over the assigment of the info->mod to make the
code clearer. The validity checker also adds an explicit size check to
ensure the module section size matches the kernel's run time size for
sizeof(struct module). This should prevent sloppy loads of modules
which are built today *without* actually increasing the size of
the struct module. A developer today can for example expand the size
of struct module, rebuild a directoroy 'make fs/xfs/' for example and
then try to insmode the driver there. That module would in effect have
an incorrect size. This new size check would put a stop gap against such
mistakes.
This also makes the entire goal of ".gnu.linkonce.this_module" pretty
clear. Before this patch verification of the goal / intent required some
Indian Jones whips, torches and cleaning up big old spider webs.
Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
---
kernel/module/main.c | 62 +++++++++++++++++++++++++++++++++++++-------
1 file changed, 53 insertions(+), 9 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index cf097ffe6a4a..e1a9dd51c036 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2,6 +2,7 @@
/*
* Copyright (C) 2002 Richard Henderson
* Copyright (C) 2001 Rusty Russell, 2002, 2010 Rusty Russell IBM.
+ * Copyright (C) 2023 Luis Chamberlain <mcgrof@kernel.org>
*/
#define INCLUDE_VERMAGIC
@@ -1656,6 +1657,7 @@ static int elf_validity_check(struct load_info *info)
unsigned int i;
Elf_Shdr *shdr, *strhdr;
int err;
+ unsigned int num_mod_secs = 0, mod_idx;
if (info->len < sizeof(*(info->hdr))) {
pr_err("Invalid ELF header len %lu\n", info->len);
@@ -1767,6 +1769,11 @@ static int elf_validity_check(struct load_info *info)
i, shdr->sh_type);
return err;
}
+ if (strcmp(info->secstrings + shdr->sh_name,
+ ".gnu.linkonce.this_module") == 0) {
+ num_mod_secs++;
+ mod_idx = i;
+ }
if (shdr->sh_flags & SHF_ALLOC) {
if (shdr->sh_name >= strhdr->sh_size) {
@@ -1779,6 +1786,52 @@ static int elf_validity_check(struct load_info *info)
}
}
+ /*
+ * The ".gnu.linkonce.this_module" ELF section is special. It is
+ * what modpost uses to refer to __this_module and let's use rely
+ * on THIS_MODULE to point to &__this_module properly. The kernel's
+ * modpost declares it on each modules's *.mod.c file. If the struct
+ * module of the kernel changes a full kernel rebuild is required.
+ *
+ * We have a few expectaions for this special section, the following
+ * code validates all this for us:
+ *
+ * o Only one section must exist
+ * o We expect the kernel to always have to allocate it: SHF_ALLOC
+ * o The section size must match the kernel's run time's struct module
+ * size
+ */
+ if (num_mod_secs != 1) {
+ pr_err("Only one .gnu.linkonce.this_module section must exist.\n");
+ goto no_exec;
+ }
+
+ shdr = &info->sechdrs[mod_idx];
+
+ /*
+ * This is already implied on the switch above, however let's be
+ * pedantic about it.
+ */
+ if (shdr->sh_type == SHT_NOBITS) {
+ pr_err(".gnu.linkonce.this_module section must have a size set\n");
+ goto no_exec;
+ }
+
+ if (!(shdr->sh_flags & SHF_ALLOC)) {
+ pr_err(".gnu.linkonce.this_module must occupy memory during process execution\n");
+ goto no_exec;
+ }
+
+ if (shdr->sh_size != sizeof(struct module)) {
+ pr_err(".gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n");
+ goto no_exec;
+ }
+
+ info->index.mod = mod_idx;
+
+ /* This is temporary: point mod into copy of data. */
+ info->mod = (void *)info->hdr + shdr->sh_offset;
+
return 0;
no_exec:
@@ -1925,15 +1978,6 @@ static int setup_load_info(struct load_info *info, int flags)
return -ENOEXEC;
}
- info->index.mod = find_sec(info, ".gnu.linkonce.this_module");
- if (!info->index.mod) {
- pr_warn("%s: No module found in object\n",
- info->name ?: "(missing .modinfo section or name field)");
- return -ENOEXEC;
- }
- /* This is temporary: point mod into copy of data. */
- info->mod = (void *)info->hdr + info->sechdrs[info->index.mod].sh_offset;
-
/*
* If we didn't load the .modinfo 'name' field earlier, fall back to
* on-disk struct mod 'name' field.
--
2.39.1
next prev parent reply other threads:[~2023-03-19 21:36 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-19 21:35 [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
2023-03-19 21:35 ` Luis Chamberlain [this message]
2023-03-19 21:35 ` [PATCH 2/5] module: add stop-grap sanity check on module memcpy() Luis Chamberlain
2023-03-19 21:35 ` [PATCH 3/5] module: move more elf validity checks to elf_validity_check() Luis Chamberlain
2023-03-19 21:35 ` [PATCH 4/5] module: merge remnants of setup_load_info() to elf validation Luis Chamberlain
2023-03-19 21:35 ` [PATCH 5/5] module: fold usermode helper kmod into modules directory Luis Chamberlain
2023-03-22 23:43 ` [PATCH 0/5] module: ELF validation enhancement and cleanups Luis Chamberlain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20230319213542.1790479-2-mcgrof@kernel.org \
--to=mcgrof@kernel.org \
--cc=christophe.leroy@csgroup.eu \
--cc=david@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-modules@vger.kernel.org \
--cc=petr.pavlu@suse.com \
--cc=pmladek@suse.com \
--cc=prarit@redhat.com \
--cc=song@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox