* [PATCH 01/12] module: Take const arg in validate_section_offset
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 02/12] module: Factor out elf_validity_ehdr Matthew Maurer
` (12 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
`validate_section_offset` doesn't modify the info passed in. Make this
clear by adjusting the type signature.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index ef54733bd7d2..1ed1d1bf1416 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1678,7 +1678,7 @@ bool __weak module_exit_section(const char *name)
return strstarts(name, ".exit");
}
-static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
+static int validate_section_offset(const struct load_info *info, Elf_Shdr *shdr)
{
#if defined(CONFIG_64BIT)
unsigned long long secend;
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 02/12] module: Factor out elf_validity_ehdr
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
2024-10-15 23:16 ` [PATCH 01/12] module: Take const arg in validate_section_offset Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 03/12] module: Factor out elf_validity_cache_sechdrs Matthew Maurer
` (11 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Factor out verification of the ELF header and document what is checked.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 70 +++++++++++++++++++++++++++++---------------
1 file changed, 47 insertions(+), 23 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 1ed1d1bf1416..c836354928f0 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1697,6 +1697,50 @@ static int validate_section_offset(const struct load_info *info, Elf_Shdr *shdr)
return 0;
}
+/**
+ * elf_validity_ehdr() - Checks an ELF header for module validity
+ * @info: Load info containing the ELF header to check
+ *
+ * Checks whether an ELF header could belong to a valid module. Checks:
+ *
+ * * ELF header is within the data the user provided
+ * * ELF magic is present
+ * * It is relocatable (not final linked, not core file, etc.)
+ * * The header's machine type matches what the architecture expects.
+ * * Optional arch-specific hook for other properties
+ * - module_elf_check_arch() is currently only used by PPC to check
+ * ELF ABI version, but may be used by others in the future.
+ *
+ * Return: %0 if valid, %-ENOEXEC on failure.
+ */
+static int elf_validity_ehdr(const struct load_info *info)
+{
+ if (info->len < sizeof(*(info->hdr))) {
+ pr_err("Invalid ELF header len %lu\n", info->len);
+ return -ENOEXEC;
+ }
+ if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0) {
+ pr_err("Invalid ELF header magic: != %s\n", ELFMAG);
+ return -ENOEXEC;
+ }
+ if (info->hdr->e_type != ET_REL) {
+ pr_err("Invalid ELF header type: %u != %u\n",
+ info->hdr->e_type, ET_REL);
+ return -ENOEXEC;
+ }
+ if (!elf_check_arch(info->hdr)) {
+ pr_err("Invalid architecture in ELF header: %u\n",
+ info->hdr->e_machine);
+ return -ENOEXEC;
+ }
+ if (!module_elf_check_arch(info->hdr)) {
+ pr_err("Invalid module architecture in ELF header: %u\n",
+ info->hdr->e_machine);
+ return -ENOEXEC;
+ }
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -1726,30 +1770,10 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
unsigned int num_info_secs = 0, info_idx;
unsigned int num_sym_secs = 0, sym_idx;
- if (info->len < sizeof(*(info->hdr))) {
- pr_err("Invalid ELF header len %lu\n", info->len);
- goto no_exec;
- }
+ err = elf_validity_ehdr(info);
+ if (err < 0)
+ return err;
- if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0) {
- pr_err("Invalid ELF header magic: != %s\n", ELFMAG);
- goto no_exec;
- }
- if (info->hdr->e_type != ET_REL) {
- pr_err("Invalid ELF header type: %u != %u\n",
- info->hdr->e_type, ET_REL);
- goto no_exec;
- }
- if (!elf_check_arch(info->hdr)) {
- pr_err("Invalid architecture in ELF header: %u\n",
- info->hdr->e_machine);
- goto no_exec;
- }
- if (!module_elf_check_arch(info->hdr)) {
- pr_err("Invalid module architecture in ELF header: %u\n",
- info->hdr->e_machine);
- goto no_exec;
- }
if (info->hdr->e_shentsize != sizeof(Elf_Shdr)) {
pr_err("Invalid ELF section header size\n");
goto no_exec;
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 03/12] module: Factor out elf_validity_cache_sechdrs
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
2024-10-15 23:16 ` [PATCH 01/12] module: Take const arg in validate_section_offset Matthew Maurer
2024-10-15 23:16 ` [PATCH 02/12] module: Factor out elf_validity_ehdr Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 04/12] module: Factor out elf_validity_cache_secstrings Matthew Maurer
` (10 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Factor out and document the validation of section headers.
Because we now validate all section offsets and lengths before accessing
them, we can remove the ad-hoc checks.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 125 ++++++++++++++++++++++++++++---------------
1 file changed, 82 insertions(+), 43 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c836354928f0..467e35f0232a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1741,6 +1741,87 @@ static int elf_validity_ehdr(const struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_sechdrs() - Cache section headers if valid
+ * @info: Load info to compute section headers from
+ *
+ * Checks:
+ *
+ * * ELF header is valid (see elf_validity_ehdr())
+ * * Section headers are the size we expect
+ * * Section array fits in the user provided data
+ * * Section index 0 is NULL
+ * * Section contents are inbounds
+ *
+ * Then updates @info with a &load_info->sechdrs pointer if valid.
+ *
+ * Return: %0 if valid, negative error code if validation failed.
+ */
+static int elf_validity_cache_sechdrs(struct load_info *info)
+{
+ Elf_Shdr *sechdrs;
+ Elf_Shdr *shdr;
+ int i;
+ int err;
+
+ err = elf_validity_ehdr(info);
+ if (err < 0)
+ return err;
+
+ if (info->hdr->e_shentsize != sizeof(Elf_Shdr)) {
+ pr_err("Invalid ELF section header size\n");
+ return -ENOEXEC;
+ }
+
+ /*
+ * e_shnum is 16 bits, and sizeof(Elf_Shdr) is
+ * known and small. So e_shnum * sizeof(Elf_Shdr)
+ * will not overflow unsigned long on any platform.
+ */
+ if (info->hdr->e_shoff >= info->len
+ || (info->hdr->e_shnum * sizeof(Elf_Shdr) >
+ info->len - info->hdr->e_shoff)) {
+ pr_err("Invalid ELF section header overflow\n");
+ return -ENOEXEC;
+ }
+
+ sechdrs = (void *)info->hdr + info->hdr->e_shoff;
+
+ /*
+ * The code assumes that section 0 has a length of zero and
+ * an addr of zero, so check for it.
+ */
+ if (sechdrs[0].sh_type != SHT_NULL
+ || sechdrs[0].sh_size != 0
+ || sechdrs[0].sh_addr != 0) {
+ pr_err("ELF Spec violation: section 0 type(%d)!=SH_NULL or non-zero len or addr\n",
+ sechdrs[0].sh_type);
+ return -ENOEXEC;
+ }
+
+ /* Validate contents are inbounds */
+ for (i = 1; i < info->hdr->e_shnum; i++) {
+ shdr = &sechdrs[i];
+ switch (shdr->sh_type) {
+ case SHT_NULL:
+ case SHT_NOBITS:
+ /* No contents, offset/size don't mean anything */
+ continue;
+ default:
+ err = validate_section_offset(info, shdr);
+ if (err < 0) {
+ pr_err("Invalid ELF section in module (section %u type %u)\n",
+ i, shdr->sh_type);
+ return err;
+ }
+ }
+ }
+
+ info->sechdrs = sechdrs;
+
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -1770,29 +1851,10 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
unsigned int num_info_secs = 0, info_idx;
unsigned int num_sym_secs = 0, sym_idx;
- err = elf_validity_ehdr(info);
+ err = elf_validity_cache_sechdrs(info);
if (err < 0)
return err;
- if (info->hdr->e_shentsize != sizeof(Elf_Shdr)) {
- pr_err("Invalid ELF section header size\n");
- goto no_exec;
- }
-
- /*
- * e_shnum is 16 bits, and sizeof(Elf_Shdr) is
- * known and small. So e_shnum * sizeof(Elf_Shdr)
- * will not overflow unsigned long on any platform.
- */
- if (info->hdr->e_shoff >= info->len
- || (info->hdr->e_shnum * sizeof(Elf_Shdr) >
- info->len - info->hdr->e_shoff)) {
- pr_err("Invalid ELF section header overflow\n");
- goto no_exec;
- }
-
- info->sechdrs = (void *)info->hdr + info->hdr->e_shoff;
-
/*
* Verify if the section name table index is valid.
*/
@@ -1805,11 +1867,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
}
strhdr = &info->sechdrs[info->hdr->e_shstrndx];
- err = validate_section_offset(info, strhdr);
- if (err < 0) {
- pr_err("Invalid ELF section hdr(type %u)\n", strhdr->sh_type);
- return err;
- }
/*
* The section name table must be NUL-terminated, as required
@@ -1826,18 +1883,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
goto no_exec;
}
- /*
- * The code assumes that section 0 has a length of zero and
- * an addr of zero, so check for it.
- */
- if (info->sechdrs[0].sh_type != SHT_NULL
- || info->sechdrs[0].sh_size != 0
- || info->sechdrs[0].sh_addr != 0) {
- pr_err("ELF Spec violation: section 0 type(%d)!=SH_NULL or non-zero len or addr\n",
- info->sechdrs[0].sh_type);
- goto no_exec;
- }
-
for (i = 1; i < info->hdr->e_shnum; i++) {
shdr = &info->sechdrs[i];
switch (shdr->sh_type) {
@@ -1856,12 +1901,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
sym_idx = i;
fallthrough;
default:
- err = validate_section_offset(info, shdr);
- if (err < 0) {
- pr_err("Invalid ELF section in module (section %u type %u)\n",
- i, shdr->sh_type);
- return err;
- }
if (strcmp(info->secstrings + shdr->sh_name,
".gnu.linkonce.this_module") == 0) {
num_mod_secs++;
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 04/12] module: Factor out elf_validity_cache_secstrings
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (2 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 03/12] module: Factor out elf_validity_cache_sechdrs Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 05/12] module: Factor out elf_validity_cache_index_info Matthew Maurer
` (9 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Factor out the validation of section names.
There are two behavioral changes:
1. Previously, we did not validate non-SHF_ALLOC sections.
This may have once been safe, as find_sec skips non-SHF_ALLOC
sections, but find_any_sec, which will be used to load BTF if that is
enabled, ignores the SHF_ALLOC flag. Since there's no need to support
invalid section names, validate all of them, not just SHF_ALLOC
sections.
2. Section names were validated *after* accessing them for the purposes
of detecting ".modinfo" and ".gnu.linkonce.this_module". They are now
checked prior to the access, which could avoid bad accesses with
malformed modules.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 106 ++++++++++++++++++++++++++++---------------
1 file changed, 69 insertions(+), 37 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 467e35f0232a..473f1fb25de2 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1822,6 +1822,71 @@ static int elf_validity_cache_sechdrs(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_secstrings() - Caches section names if valid
+ * @info: Load info to cache section names from. Must have valid sechdrs.
+ *
+ * Specifically checks:
+ *
+ * * Section name table index is inbounds of section headers
+ * * Section name table is not empty
+ * * Section name table is NUL terminated
+ * * All section name offsets are inbounds of the section
+ *
+ * Then updates @info with a &load_info->secstrings pointer if valid.
+ *
+ * Return: %0 if valid, negative error code if validation failed.
+ */
+static int elf_validity_cache_secstrings(struct load_info *info)
+{
+ Elf_Shdr *strhdr, *shdr;
+ char *secstrings;
+ int i;
+
+ /*
+ * Verify if the section name table index is valid.
+ */
+ if (info->hdr->e_shstrndx == SHN_UNDEF
+ || info->hdr->e_shstrndx >= info->hdr->e_shnum) {
+ pr_err("Invalid ELF section name index: %d || e_shstrndx (%d) >= e_shnum (%d)\n",
+ info->hdr->e_shstrndx, info->hdr->e_shstrndx,
+ info->hdr->e_shnum);
+ return -ENOEXEC;
+ }
+
+ strhdr = &info->sechdrs[info->hdr->e_shstrndx];
+
+ /*
+ * The section name table must be NUL-terminated, as required
+ * by the spec. This makes strcmp and pr_* calls that access
+ * strings in the section safe.
+ */
+ secstrings = (void *)info->hdr + strhdr->sh_offset;
+ if (strhdr->sh_size == 0) {
+ pr_err("empty section name table\n");
+ return -ENOEXEC;
+ }
+ if (secstrings[strhdr->sh_size - 1] != '\0') {
+ pr_err("ELF Spec violation: section name table isn't null terminated\n");
+ return -ENOEXEC;
+ }
+
+ for (i = 0; i < info->hdr->e_shnum; i++) {
+ shdr = &info->sechdrs[i];
+ /* SHT_NULL means sh_name has an undefined value */
+ if (shdr->sh_type == SHT_NULL)
+ continue;
+ if (shdr->sh_name >= strhdr->sh_size) {
+ pr_err("Invalid ELF section name in module (section %u type %u)\n",
+ i, shdr->sh_type);
+ return -ENOEXEC;
+ }
+ }
+
+ info->secstrings = secstrings;
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -1845,7 +1910,7 @@ static int elf_validity_cache_sechdrs(struct load_info *info)
static int elf_validity_cache_copy(struct load_info *info, int flags)
{
unsigned int i;
- Elf_Shdr *shdr, *strhdr;
+ Elf_Shdr *shdr;
int err;
unsigned int num_mod_secs = 0, mod_idx;
unsigned int num_info_secs = 0, info_idx;
@@ -1854,34 +1919,9 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
err = elf_validity_cache_sechdrs(info);
if (err < 0)
return err;
-
- /*
- * Verify if the section name table index is valid.
- */
- if (info->hdr->e_shstrndx == SHN_UNDEF
- || info->hdr->e_shstrndx >= info->hdr->e_shnum) {
- pr_err("Invalid ELF section name index: %d || e_shstrndx (%d) >= e_shnum (%d)\n",
- info->hdr->e_shstrndx, info->hdr->e_shstrndx,
- info->hdr->e_shnum);
- goto no_exec;
- }
-
- strhdr = &info->sechdrs[info->hdr->e_shstrndx];
-
- /*
- * The section name table must be NUL-terminated, as required
- * by the spec. This makes strcmp and pr_* calls that access
- * strings in the section safe.
- */
- info->secstrings = (void *)info->hdr + strhdr->sh_offset;
- if (strhdr->sh_size == 0) {
- pr_err("empty section name table\n");
- goto no_exec;
- }
- if (info->secstrings[strhdr->sh_size - 1] != '\0') {
- pr_err("ELF Spec violation: section name table isn't null terminated\n");
- goto no_exec;
- }
+ err = elf_validity_cache_secstrings(info);
+ if (err < 0)
+ return err;
for (i = 1; i < info->hdr->e_shnum; i++) {
shdr = &info->sechdrs[i];
@@ -1910,14 +1950,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
num_info_secs++;
info_idx = i;
}
-
- if (shdr->sh_flags & SHF_ALLOC) {
- if (shdr->sh_name >= strhdr->sh_size) {
- pr_err("Invalid ELF section name in module (section %u type %u)\n",
- i, shdr->sh_type);
- return -ENOEXEC;
- }
- }
break;
}
}
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 05/12] module: Factor out elf_validity_cache_index_info
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (3 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 04/12] module: Factor out elf_validity_cache_secstrings Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 06/12] module: Factor out elf_validity_cache_index_mod Matthew Maurer
` (8 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Centralize .modinfo detection and property validation.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 82 ++++++++++++++++++++++++++++++++++++--------
1 file changed, 68 insertions(+), 14 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 473f1fb25de2..6747cbc774b0 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -195,6 +195,38 @@ static unsigned int find_sec(const struct load_info *info, const char *name)
return 0;
}
+/**
+ * find_any_unique_sec() - Find a unique section index by name
+ * @info: Load info for the module to scan
+ * @name: Name of the section we're looking for
+ *
+ * Locates a unique section by name. Ignores SHF_ALLOC.
+ *
+ * Return: Section index if found uniquely, zero if absent, negative count
+ * of total instances if multiple were found.
+ */
+static int find_any_unique_sec(const struct load_info *info, const char *name)
+{
+ unsigned int idx;
+ unsigned int count = 0;
+ int i;
+
+ for (i = 1; i < info->hdr->e_shnum; i++) {
+ if (strcmp(info->secstrings + info->sechdrs[i].sh_name,
+ name) == 0) {
+ count++;
+ idx = i;
+ }
+ }
+ if (count == 1) {
+ return idx;
+ } else if (count == 0) {
+ return 0;
+ } else {
+ return -count;
+ }
+}
+
/* Find a module section, or NULL. */
static void *section_addr(const struct load_info *info, const char *name)
{
@@ -1887,6 +1919,39 @@ static int elf_validity_cache_secstrings(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_index_info() - Validate and cache modinfo section
+ * @info: Load info to populate the modinfo index on.
+ * Must have &load_info->sechdrs and &load_info->secstrings populated
+ *
+ * Checks that if there is a .modinfo section, it is unique.
+ * Then, it caches its index in &load_info->index.info.
+ * Finally, it tries to populate the name to improve error messages.
+ *
+ * Return: %0 if valid, %-ENOEXEC if multiple modinfo sections were found.
+ */
+static int elf_validity_cache_index_info(struct load_info *info)
+{
+ int info_idx;
+
+ info_idx = find_any_unique_sec(info, ".modinfo");
+
+ if (info_idx == 0)
+ /* Early return, no .modinfo */
+ return 0;
+
+ if (info_idx < 0) {
+ pr_err("Only one .modinfo section must exist.\n");
+ return -ENOEXEC;
+ }
+
+ info->index.info = info_idx;
+ /* Try to find a name early so we can log errors with a module name */
+ info->name = get_modinfo(info, "name");
+
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -1913,13 +1978,15 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
Elf_Shdr *shdr;
int err;
unsigned int num_mod_secs = 0, mod_idx;
- unsigned int num_info_secs = 0, info_idx;
unsigned int num_sym_secs = 0, sym_idx;
err = elf_validity_cache_sechdrs(info);
if (err < 0)
return err;
err = elf_validity_cache_secstrings(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_info(info);
if (err < 0)
return err;
@@ -1945,24 +2012,11 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
".gnu.linkonce.this_module") == 0) {
num_mod_secs++;
mod_idx = i;
- } else if (strcmp(info->secstrings + shdr->sh_name,
- ".modinfo") == 0) {
- num_info_secs++;
- info_idx = i;
}
break;
}
}
- if (num_info_secs > 1) {
- pr_err("Only one .modinfo section must exist.\n");
- goto no_exec;
- } else if (num_info_secs == 1) {
- /* Try to find a name early so we can log errors with a module name */
- info->index.info = info_idx;
- info->name = get_modinfo(info, "name");
- }
-
if (num_sym_secs != 1) {
pr_warn("%s: module has no symbols (stripped?)\n",
info->name ?: "(missing .modinfo section or name field)");
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 06/12] module: Factor out elf_validity_cache_index_mod
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (4 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 05/12] module: Factor out elf_validity_cache_index_info Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 07/12] module: Factor out elf_validity_cache_index_sym Matthew Maurer
` (7 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Centralize .gnu.linkonce.this_module detection and property validation.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 129 ++++++++++++++++++++++---------------------
1 file changed, 67 insertions(+), 62 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 6747cbc774b0..b633347d5d98 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1952,6 +1952,68 @@ static int elf_validity_cache_index_info(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_index_mod() - Validates and caches this_module section
+ * @info: Load info to cache this_module on.
+ * Must have &load_info->sechdrs and &load_info->secstrings populated
+ *
+ * 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 expectations for this special section, this function
+ * validates all this for us:
+ *
+ * * The section has contents
+ * * The section is unique
+ * * We expect the kernel to always have to allocate it: SHF_ALLOC
+ * * The section size must match the kernel's run time's struct module
+ * size
+ *
+ * If all checks pass, the index will be cached in &load_info->index.mod
+ *
+ * Return: %0 on validation success, %-ENOEXEC on failure
+ */
+static int elf_validity_cache_index_mod(struct load_info *info)
+{
+ Elf_Shdr *shdr;
+ int mod_idx;
+
+ mod_idx = find_any_unique_sec(info, ".gnu.linkonce.this_module");
+ if (mod_idx <= 0) {
+ pr_err("module %s: Exactly one .gnu.linkonce.this_module section must exist.\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
+ shdr = &info->sechdrs[mod_idx];
+
+ if (shdr->sh_type == SHT_NOBITS) {
+ pr_err("module %s: .gnu.linkonce.this_module section must have a size set\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
+ if (!(shdr->sh_flags & SHF_ALLOC)) {
+ pr_err("module %s: .gnu.linkonce.this_module must occupy memory during process execution\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
+ if (shdr->sh_size != sizeof(struct module)) {
+ pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
+ info->index.mod = mod_idx;
+
+ return 0;
+}
+
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -1977,7 +2039,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
unsigned int i;
Elf_Shdr *shdr;
int err;
- unsigned int num_mod_secs = 0, mod_idx;
unsigned int num_sym_secs = 0, sym_idx;
err = elf_validity_cache_sechdrs(info);
@@ -1987,16 +2048,15 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
if (err < 0)
return err;
err = elf_validity_cache_index_info(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_mod(info);
if (err < 0)
return err;
for (i = 1; i < info->hdr->e_shnum; i++) {
shdr = &info->sechdrs[i];
- switch (shdr->sh_type) {
- case SHT_NULL:
- case SHT_NOBITS:
- continue;
- case SHT_SYMTAB:
+ if (shdr->sh_type == SHT_SYMTAB) {
if (shdr->sh_link == SHN_UNDEF
|| shdr->sh_link >= info->hdr->e_shnum) {
pr_err("Invalid ELF sh_link!=SHN_UNDEF(%d) or (sh_link(%d) >= hdr->e_shnum(%d)\n",
@@ -2006,14 +2066,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
}
num_sym_secs++;
sym_idx = i;
- fallthrough;
- default:
- if (strcmp(info->secstrings + shdr->sh_name,
- ".gnu.linkonce.this_module") == 0) {
- num_mod_secs++;
- mod_idx = i;
- }
- break;
}
}
@@ -2029,55 +2081,8 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
info->index.str = shdr->sh_link;
info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;
- /*
- * 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("module %s: Only one .gnu.linkonce.this_module section must exist.\n",
- info->name ?: "(missing .modinfo section or name field)");
- 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("module %s: .gnu.linkonce.this_module section must have a size set\n",
- info->name ?: "(missing .modinfo section or name field)");
- goto no_exec;
- }
-
- if (!(shdr->sh_flags & SHF_ALLOC)) {
- pr_err("module %s: .gnu.linkonce.this_module must occupy memory during process execution\n",
- info->name ?: "(missing .modinfo section or name field)");
- goto no_exec;
- }
-
- if (shdr->sh_size != sizeof(struct module)) {
- pr_err("module %s: .gnu.linkonce.this_module section size must match the kernel's built struct module size at run time\n",
- info->name ?: "(missing .modinfo section or name field)");
- 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;
+ 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
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 07/12] module: Factor out elf_validity_cache_index_sym
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (5 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 06/12] module: Factor out elf_validity_cache_index_mod Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 08/12] module: Factor out elf_validity_cache_index_str Matthew Maurer
` (6 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Centralize symbol table detection and property validation.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 73 ++++++++++++++++++++++++++------------------
1 file changed, 44 insertions(+), 29 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index b633347d5d98..955746649f37 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2013,6 +2013,39 @@ static int elf_validity_cache_index_mod(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_index_sym() - Validate and cache symtab index
+ * @info: Load info to cache symtab index in.
+ * Must have &load_info->sechdrs and &load_info->secstrings populated.
+ *
+ * Checks that there is exactly one symbol table, then caches its index in
+ * &load_info->index.sym.
+ *
+ * Return: %0 if valid, %-ENOEXEC on failure.
+ */
+static int elf_validity_cache_index_sym(struct load_info *info)
+{
+ unsigned int sym_idx;
+ unsigned int num_sym_secs = 0;
+ int i;
+
+ for (i = 1; i < info->hdr->e_shnum; i++) {
+ if (info->sechdrs[i].sh_type == SHT_SYMTAB) {
+ num_sym_secs++;
+ sym_idx = i;
+ }
+ }
+
+ if (num_sym_secs != 1) {
+ pr_warn("%s: module has no symbols (stripped?)\n",
+ info->name ?: "(missing .modinfo section or name field)");
+ return -ENOEXEC;
+ }
+
+ info->index.sym = sym_idx;
+
+ return 0;
+}
/*
* Check userspace passed ELF module against our expectations, and cache
@@ -2036,10 +2069,8 @@ static int elf_validity_cache_index_mod(struct load_info *info)
*/
static int elf_validity_cache_copy(struct load_info *info, int flags)
{
- unsigned int i;
- Elf_Shdr *shdr;
int err;
- unsigned int num_sym_secs = 0, sym_idx;
+ int str_idx;
err = elf_validity_cache_sechdrs(info);
if (err < 0)
@@ -2051,34 +2082,21 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
if (err < 0)
return err;
err = elf_validity_cache_index_mod(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_sym(info);
if (err < 0)
return err;
- for (i = 1; i < info->hdr->e_shnum; i++) {
- shdr = &info->sechdrs[i];
- if (shdr->sh_type == SHT_SYMTAB) {
- if (shdr->sh_link == SHN_UNDEF
- || shdr->sh_link >= info->hdr->e_shnum) {
- pr_err("Invalid ELF sh_link!=SHN_UNDEF(%d) or (sh_link(%d) >= hdr->e_shnum(%d)\n",
- shdr->sh_link, shdr->sh_link,
- info->hdr->e_shnum);
- goto no_exec;
- }
- num_sym_secs++;
- sym_idx = i;
- }
- }
-
- if (num_sym_secs != 1) {
- pr_warn("%s: module has no symbols (stripped?)\n",
- info->name ?: "(missing .modinfo section or name field)");
- goto no_exec;
+ str_idx = info->sechdrs[info->index.sym].sh_link;
+ if (str_idx == SHN_UNDEF || str_idx >= info->hdr->e_shnum) {
+ pr_err("Invalid ELF sh_link!=SHN_UNDEF(%d) or (sh_link(%d) >= hdr->e_shnum(%d)\n",
+ str_idx, str_idx, info->hdr->e_shnum);
+ return -ENOEXEC;
}
- /* Sets internal symbols and strings. */
- info->index.sym = sym_idx;
- shdr = &info->sechdrs[sym_idx];
- info->index.str = shdr->sh_link;
+ /* Sets internal strings. */
+ info->index.str = str_idx;
info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;
/* This is temporary: point mod into copy of data. */
@@ -2099,9 +2117,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
info->index.pcpu = find_pcpusec(info);
return 0;
-
-no_exec:
- return -ENOEXEC;
}
#define COPY_CHUNK_SIZE (16*PAGE_SIZE)
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 08/12] module: Factor out elf_validity_cache_index_str
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (6 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 07/12] module: Factor out elf_validity_cache_index_sym Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 09/12] module: Group section index calculations together Matthew Maurer
` (5 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Pull out index validation for the symbol string section.
Note that this does not validate the *contents* of the string table,
only shape and presence of the section.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 37 ++++++++++++++++++++++++++++---------
1 file changed, 28 insertions(+), 9 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 955746649f37..a6bed293d97b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2047,6 +2047,31 @@ static int elf_validity_cache_index_sym(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_index_str() - Validate and cache strtab index
+ * @info: Load info to cache strtab index in.
+ * Must have &load_info->sechdrs and &load_info->secstrings populated.
+ * Must have &load_info->index.sym populated.
+ *
+ * Looks at the symbol table's associated string table, makes sure it is
+ * in-bounds, and caches it.
+ *
+ * Return: %0 if valid, %-ENOEXEC on failure.
+ */
+static int elf_validity_cache_index_str(struct load_info *info)
+{
+ unsigned int str_idx = info->sechdrs[info->index.sym].sh_link;
+
+ if (str_idx == SHN_UNDEF || str_idx >= info->hdr->e_shnum) {
+ pr_err("Invalid ELF sh_link!=SHN_UNDEF(%d) or (sh_link(%d) >= hdr->e_shnum(%d)\n",
+ str_idx, str_idx, info->hdr->e_shnum);
+ return -ENOEXEC;
+ }
+
+ info->index.str = str_idx;
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -2070,7 +2095,6 @@ static int elf_validity_cache_index_sym(struct load_info *info)
static int elf_validity_cache_copy(struct load_info *info, int flags)
{
int err;
- int str_idx;
err = elf_validity_cache_sechdrs(info);
if (err < 0)
@@ -2087,16 +2111,11 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
err = elf_validity_cache_index_sym(info);
if (err < 0)
return err;
-
- str_idx = info->sechdrs[info->index.sym].sh_link;
- if (str_idx == SHN_UNDEF || str_idx >= info->hdr->e_shnum) {
- pr_err("Invalid ELF sh_link!=SHN_UNDEF(%d) or (sh_link(%d) >= hdr->e_shnum(%d)\n",
- str_idx, str_idx, info->hdr->e_shnum);
- return -ENOEXEC;
- }
+ err = elf_validity_cache_index_str(info);
+ if (err < 0)
+ return err;
/* Sets internal strings. */
- info->index.str = str_idx;
info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;
/* This is temporary: point mod into copy of data. */
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 09/12] module: Group section index calculations together
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (7 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 08/12] module: Factor out elf_validity_cache_index_str Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 10/12] module: Factor out elf_validity_cache_strtab Matthew Maurer
` (4 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Group all the index detection together to make the parent function
easier to read.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 68 +++++++++++++++++++++++++++++++++-----------
1 file changed, 51 insertions(+), 17 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index a6bed293d97b..f352c73b6f40 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2072,6 +2072,56 @@ static int elf_validity_cache_index_str(struct load_info *info)
return 0;
}
+/**
+ * elf_validity_cache_index() - Resolve, validate, cache section indices
+ * @info: Load info to read from and update.
+ * &load_info->sechdrs and &load_info->secstrings must be populated.
+ * @flags: Load flags, relevant to suppress version loading, see
+ * uapi/linux/module.h
+ *
+ * Populates &load_info->index, validating as it goes.
+ * See child functions for per-field validation:
+ *
+ * * elf_validity_cache_index_info()
+ * * elf_validity_cache_index_mod()
+ * * elf_validity_cache_index_sym()
+ * * elf_validity_cache_index_str()
+ *
+ * If versioning is not suppressed via flags, load the version index from
+ * a section called "__versions" with no validation.
+ *
+ * If CONFIG_SMP is enabled, load the percpu section by name with no
+ * validation.
+ *
+ * Return: 0 on success, negative error code if an index failed validation.
+ */
+static int elf_validity_cache_index(struct load_info *info, int flags)
+{
+ int err;
+
+ err = elf_validity_cache_index_info(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_mod(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_sym(info);
+ if (err < 0)
+ return err;
+ err = elf_validity_cache_index_str(info);
+ if (err < 0)
+ return err;
+
+ if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
+ info->index.vers = 0; /* Pretend no __versions section! */
+ else
+ info->index.vers = find_sec(info, "__versions");
+
+ info->index.pcpu = find_pcpusec(info);
+
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -2102,16 +2152,7 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
err = elf_validity_cache_secstrings(info);
if (err < 0)
return err;
- err = elf_validity_cache_index_info(info);
- if (err < 0)
- return err;
- err = elf_validity_cache_index_mod(info);
- if (err < 0)
- return err;
- err = elf_validity_cache_index_sym(info);
- if (err < 0)
- return err;
- err = elf_validity_cache_index_str(info);
+ err = elf_validity_cache_index(info, flags);
if (err < 0)
return err;
@@ -2128,13 +2169,6 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
if (!info->name)
info->name = info->mod->name;
- if (flags & MODULE_INIT_IGNORE_MODVERSIONS)
- info->index.vers = 0; /* Pretend no __versions section! */
- else
- info->index.vers = find_sec(info, "__versions");
-
- info->index.pcpu = find_pcpusec(info);
-
return 0;
}
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 10/12] module: Factor out elf_validity_cache_strtab
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (8 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 09/12] module: Group section index calculations together Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 11/12] module: Additional validation in elf_validity_cache_strtab Matthew Maurer
` (3 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
This patch only moves the existing strtab population to a function.
Validation comes in a following patch, this is split out to make the new
validation checks more clearly separated.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 23 ++++++++++++++++++++---
1 file changed, 20 insertions(+), 3 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index f352c73b6f40..22aa5eb4e4f4 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2122,6 +2122,23 @@ static int elf_validity_cache_index(struct load_info *info, int flags)
return 0;
}
+/**
+ * elf_validity_cache_strtab() - Cache symbol string table
+ * @info: Load info to read from and update.
+ * Must have &load_info->sechdrs and &load_info->secstrings populated.
+ * Must have &load_info->index populated.
+ *
+ * Return: 0 on success, negative error code if a check failed.
+ */
+static int elf_validity_cache_strtab(struct load_info *info)
+{
+ Elf_Shdr *str_shdr = &info->sechdrs[info->index.str];
+ char *strtab = (char *)info->hdr + str_shdr->sh_offset;
+
+ info->strtab = strtab;
+ return 0;
+}
+
/*
* Check userspace passed ELF module against our expectations, and cache
* useful variables for further processing as we go.
@@ -2155,9 +2172,9 @@ static int elf_validity_cache_copy(struct load_info *info, int flags)
err = elf_validity_cache_index(info, flags);
if (err < 0)
return err;
-
- /* Sets internal strings. */
- info->strtab = (char *)info->hdr + info->sechdrs[info->index.str].sh_offset;
+ err = elf_validity_cache_strtab(info);
+ if (err < 0)
+ return err;
/* This is temporary: point mod into copy of data. */
info->mod = (void *)info->hdr + info->sechdrs[info->index.mod].sh_offset;
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 11/12] module: Additional validation in elf_validity_cache_strtab
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (9 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 10/12] module: Factor out elf_validity_cache_strtab Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-15 23:16 ` [PATCH 12/12] module: Reformat struct for code style Matthew Maurer
` (2 subsequent siblings)
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Validate properties of the strtab that are depended on elsewhere, but
were previously unchecked:
* String table nonempty (offset 0 is valid)
* String table has a leading NUL (offset 0 corresponds to "")
* String table is NUL terminated (strfoo functions won't run out of the
table while reading).
* All symbols names are inbounds of the string table.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/main.c | 38 +++++++++++++++++++++++++++++++++++++-
1 file changed, 37 insertions(+), 1 deletion(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 22aa5eb4e4f4..3db9ff544c09 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -2123,17 +2123,53 @@ static int elf_validity_cache_index(struct load_info *info, int flags)
}
/**
- * elf_validity_cache_strtab() - Cache symbol string table
+ * elf_validity_cache_strtab() - Validate and cache symbol string table
* @info: Load info to read from and update.
* Must have &load_info->sechdrs and &load_info->secstrings populated.
* Must have &load_info->index populated.
*
+ * Checks:
+ *
+ * * The string table is not empty.
+ * * The string table starts and ends with NUL (required by ELF spec).
+ * * Every &Elf_Sym->st_name offset in the symbol table is inbounds of the
+ * string table.
+ *
+ * And caches the pointer as &load_info->strtab in @info.
+ *
* Return: 0 on success, negative error code if a check failed.
*/
static int elf_validity_cache_strtab(struct load_info *info)
{
Elf_Shdr *str_shdr = &info->sechdrs[info->index.str];
+ Elf_Shdr *sym_shdr = &info->sechdrs[info->index.sym];
char *strtab = (char *)info->hdr + str_shdr->sh_offset;
+ Elf_Sym *syms = (void *)info->hdr + sym_shdr->sh_offset;
+ int i;
+
+ if (str_shdr->sh_size == 0) {
+ pr_err("empty symbol string table\n");
+ return -ENOEXEC;
+ }
+ if (strtab[0] != '\0') {
+ pr_err("symbol string table missing leading NUL\n");
+ return -ENOEXEC;
+ }
+ if (strtab[str_shdr->sh_size - 1] != '\0') {
+ pr_err("symbol string table isn't NUL terminated\n");
+ return -ENOEXEC;
+ }
+
+ /*
+ * Now that we know strtab is correctly structured, check symbol
+ * starts are inbounds before they're used later.
+ */
+ for (i = 0; i < sym_shdr->sh_size / sizeof(*syms); i++) {
+ if (syms[i].st_name >= str_shdr->sh_size) {
+ pr_err("symbol name out of bounds in string table");
+ return -ENOEXEC;
+ }
+ }
info->strtab = strtab;
return 0;
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* [PATCH 12/12] module: Reformat struct for code style
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (10 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 11/12] module: Additional validation in elf_validity_cache_strtab Matthew Maurer
@ 2024-10-15 23:16 ` Matthew Maurer
2024-10-17 0:44 ` [PATCH 00/12] Module Validation Refactor Sami Tolvanen
2024-10-19 22:10 ` Luis Chamberlain
13 siblings, 0 replies; 15+ messages in thread
From: Matthew Maurer @ 2024-10-15 23:16 UTC (permalink / raw)
To: mcgrof
Cc: linux-kernel, linux-modules, samitolvanen, Matthew Maurer,
Petr Pavlu, Daniel Gomez
Using commas to declare struct members makes adding new members to this
struct not as nice with patch management.
Signed-off-by: Matthew Maurer <mmaurer@google.com>
---
kernel/module/internal.h | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index 2ebece8a789f..daef2be83902 100644
--- a/kernel/module/internal.h
+++ b/kernel/module/internal.h
@@ -80,7 +80,12 @@ struct load_info {
unsigned int used_pages;
#endif
struct {
- unsigned int sym, str, mod, vers, info, pcpu;
+ unsigned int sym;
+ unsigned int str;
+ unsigned int mod;
+ unsigned int vers;
+ unsigned int info;
+ unsigned int pcpu;
} index;
};
--
2.47.0.rc1.288.g06298d1525-goog
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 00/12] Module Validation Refactor
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (11 preceding siblings ...)
2024-10-15 23:16 ` [PATCH 12/12] module: Reformat struct for code style Matthew Maurer
@ 2024-10-17 0:44 ` Sami Tolvanen
2024-10-19 22:10 ` Luis Chamberlain
13 siblings, 0 replies; 15+ messages in thread
From: Sami Tolvanen @ 2024-10-17 0:44 UTC (permalink / raw)
To: Matthew Maurer; +Cc: mcgrof, linux-kernel, linux-modules
Hi,
On Tue, Oct 15, 2024 at 4:16 PM Matthew Maurer <mmaurer@google.com> wrote:
>
> Split out from Extended MODVERSIONS Support [1]
>
> This series refactors module validation during loading to ensure that
> everything is checked on its way in. This is intended to make the code
> robust enough that we can more confidently add new pieces like extended
> MODVERSIONS.
>
> [1] https://lore.kernel.org/all/20240925233854.90072-1-mmaurer@google.com/
>
> Matthew Maurer (12):
> module: Take const arg in validate_section_offset
> module: Factor out elf_validity_ehdr
> module: Factor out elf_validity_cache_sechdrs
> module: Factor out elf_validity_cache_secstrings
> module: Factor out elf_validity_cache_index_info
> module: Factor out elf_validity_cache_index_mod
> module: Factor out elf_validity_cache_index_sym
> module: Factor out elf_validity_cache_index_str
> module: Group section index calculations together
> module: Factor out elf_validity_cache_strtab
> module: Additional validation in elf_validity_cache_strtab
> module: Reformat struct for code style
>
> kernel/module/internal.h | 7 +-
> kernel/module/main.c | 569 +++++++++++++++++++++++++++++----------
> 2 files changed, 428 insertions(+), 148 deletions(-)
Looks like these patches are unchanged from the pre-split version[1].
For the series:
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
[1] https://lore.kernel.org/linux-modules/20240925233854.90072-1-mmaurer@google.com/
Sami
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 00/12] Module Validation Refactor
2024-10-15 23:16 [PATCH 00/12] Module Validation Refactor Matthew Maurer
` (12 preceding siblings ...)
2024-10-17 0:44 ` [PATCH 00/12] Module Validation Refactor Sami Tolvanen
@ 2024-10-19 22:10 ` Luis Chamberlain
13 siblings, 0 replies; 15+ messages in thread
From: Luis Chamberlain @ 2024-10-19 22:10 UTC (permalink / raw)
To: Matthew Maurer, Song Liu
Cc: linux-kernel, linux-modules, samitolvanen, petr.pavlu, da.gomez
On Tue, Oct 15, 2024 at 11:16:34PM +0000, Matthew Maurer wrote:
> Split out from Extended MODVERSIONS Support [1]
>
> This series refactors module validation during loading to ensure that
> everything is checked on its way in. This is intended to make the code
> robust enough that we can more confidently add new pieces like extended
> MODVERSIONS.
>
> [1] https://lore.kernel.org/all/20240925233854.90072-1-mmaurer@google.com/
Thanks! KPD [0] picked this up and ran automated tests for us using kdevops,
and the tests passed [1], I've included that into the last commit log and
pushed to modules-next [2].
[0] https://github.com/facebookincubator/kernel-patches-daemon
[1] https://github.com/linux-kdevops/linux-modules-kpd/actions/runs/11420095343
[2] git://git.kernel.org/pub/scm/linux/kernel/git/modules/linux.git
Luis
^ permalink raw reply [flat|nested] 15+ messages in thread