From f43922162184f2bd54d87c1b5e97cf72d0dd1290 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:35 +0000 Subject: module: Take const arg in validate_section_offset `validate_section_offset` doesn't modify the info passed in. Make this clear by adjusting the type signature. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 49b9bca9de12..1a2dd52147ba 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1645,7 +1645,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; -- cgit v1.2.3-70-g09d2 From 90f8f312db720dbabec7c6258ef580b50129cc21 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:36 +0000 Subject: module: Factor out elf_validity_ehdr Factor out verification of the ELF header and document what is checked. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 70 +++++++++++++++++++++++++++++++++++----------------- 1 file changed, 47 insertions(+), 23 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 1a2dd52147ba..59c977acfb44 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1664,6 +1664,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. @@ -1693,30 +1737,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; -- cgit v1.2.3-70-g09d2 From c92aab819d56d51631f0484ed7af11d9d8ff4cb0 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:37 +0000 Subject: module: Factor out elf_validity_cache_sechdrs 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 Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 125 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 82 insertions(+), 43 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 59c977acfb44..1f3a07ee59c6 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1708,6 +1708,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. @@ -1737,29 +1818,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. */ @@ -1772,11 +1834,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 @@ -1793,18 +1850,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) { @@ -1823,12 +1868,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++; -- cgit v1.2.3-70-g09d2 From 3c5700aeabd87e81d9153a7666b28d0e405c6c88 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:38 +0000 Subject: module: Factor out elf_validity_cache_secstrings 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 Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 106 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 69 insertions(+), 37 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 1f3a07ee59c6..6a9159afca02 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1789,6 +1789,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. @@ -1812,7 +1877,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; @@ -1821,34 +1886,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]; @@ -1877,14 +1917,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; } } -- cgit v1.2.3-70-g09d2 From fbc0e4e482aac7f2b0d20f0a11f6d5eeda346fda Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:39 +0000 Subject: module: Factor out elf_validity_cache_index_info Centralize .modinfo detection and property validation. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 82 +++++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 68 insertions(+), 14 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 6a9159afca02..511d645ac577 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) { @@ -1854,6 +1886,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. @@ -1880,13 +1945,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; @@ -1912,24 +1979,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)"); -- cgit v1.2.3-70-g09d2 From 0be41a9367d1fbb16b4b57d81082341af114bad7 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:40 +0000 Subject: module: Factor out elf_validity_cache_index_mod Centralize .gnu.linkonce.this_module detection and property validation. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 129 ++++++++++++++++++++++++++------------------------- 1 file changed, 67 insertions(+), 62 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 511d645ac577..ec638187ffcf 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1919,6 +1919,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. @@ -1944,7 +2006,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); @@ -1954,16 +2015,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", @@ -1973,14 +2033,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; } } @@ -1996,55 +2048,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 -- cgit v1.2.3-70-g09d2 From 9bd4982cf7d65f4c9e0793d5a8fda6ad838e8554 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:41 +0000 Subject: module: Factor out elf_validity_cache_index_sym Centralize symbol table detection and property validation. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 73 +++++++++++++++++++++++++++++++--------------------- 1 file changed, 44 insertions(+), 29 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index ec638187ffcf..6be58b0a6468 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -1980,6 +1980,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 @@ -2003,10 +2036,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) @@ -2018,34 +2049,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. */ @@ -2066,9 +2084,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) -- cgit v1.2.3-70-g09d2 From 0a9395334496d3be8bde491e46087540cb8f141d Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:42 +0000 Subject: module: Factor out elf_validity_cache_index_str 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 Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 37 ++++++++++++++++++++++++++++--------- 1 file changed, 28 insertions(+), 9 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 6be58b0a6468..43140475aac0 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2014,6 +2014,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. @@ -2037,7 +2062,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) @@ -2054,16 +2078,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. */ -- cgit v1.2.3-70-g09d2 From f3f561218bb60afd6d3e3b26add39ff46de89c83 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:43 +0000 Subject: module: Group section index calculations together Group all the index detection together to make the parent function easier to read. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 68 +++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 51 insertions(+), 17 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index 43140475aac0..e04a228c694a 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2039,6 +2039,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. @@ -2069,16 +2119,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; @@ -2095,13 +2136,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; } -- cgit v1.2.3-70-g09d2 From 837031e052af32c747906238fb1feb87778e4fe0 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:44 +0000 Subject: module: Factor out elf_validity_cache_strtab 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 Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 23 ++++++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index e04a228c694a..c082d5d41a8d 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2089,6 +2089,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. @@ -2122,9 +2139,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; -- cgit v1.2.3-70-g09d2 From d979e3dffa93c9284f244ef64f7a68042c2f8b80 Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:45 +0000 Subject: module: Additional validation in elf_validity_cache_strtab 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 Reviewed-by: Sami Tolvanen Signed-off-by: Luis Chamberlain --- kernel/module/main.c | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) (limited to 'kernel') diff --git a/kernel/module/main.c b/kernel/module/main.c index c082d5d41a8d..b40b632f00a6 100644 --- a/kernel/module/main.c +++ b/kernel/module/main.c @@ -2090,17 +2090,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; -- cgit v1.2.3-70-g09d2 From 2295cf87ed5a6da4564034e4f8ebcce0a0a021ed Mon Sep 17 00:00:00 2001 From: Matthew Maurer Date: Tue, 15 Oct 2024 23:16:46 +0000 Subject: module: Reformat struct for code style Using commas to declare struct members makes adding new members to this struct not as nice with patch management. Test results linux-modules-kpd succeed [0]. Signed-off-by: Matthew Maurer Reviewed-by: Sami Tolvanen [mcgrof: add automated test results from kdevops using KPD ] Link: https://github.com/linux-kdevops/linux-modules-kpd/actions/runs/11420095343 # [0] Signed-off-by: Luis Chamberlain --- kernel/module/internal.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'kernel') 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; }; -- cgit v1.2.3-70-g09d2