Re: [PATCH v2 2/5] modules: Refactor + kdoc elf_validity_cached_copy

2023-11-18 Thread Greg KH
On Sat, Nov 18, 2023 at 02:54:43AM +, Matthew Maurer wrote:
> Functionality is almost identical, just structured for better
> documentation and readability. Changes:
> 
> * Section names are checked for *all* non-SHT_NULL sections, not just
>   SHF_ALLOC sections. We have code that accesses section names of
>   non-SHF_ALLOC sections (see find_any_sec for example)
> * The section name check occurs *before* strcmping on section names.
>   Previously, it was possible to use an out-of-bounds offset to strcmp
>   against ".modinfo" or ".gnu.linkonce.this_module"
> * strtab is checked for NUL lead+termination and nonzero size
> * The symbol table is swept to ensure offsets are inbounds of strtab
> 
> While some of these oversights would normally be worrying, all of the
> potentially unverified accesses occur after signature check, and only in
> response to a user who can load a kernel module.
> 
> Signed-off-by: Matthew Maurer 
> ---
>  kernel/module/internal.h |   7 +-
>  kernel/module/main.c | 585 +--
>  2 files changed, 444 insertions(+), 148 deletions(-)

Again, this needs to be broken into much smaller pieces before we can
even review it.  Would you want to review this?

thanks,

greg "think of the reviewers" k-h


[PATCH v2 2/5] modules: Refactor + kdoc elf_validity_cached_copy

2023-11-17 Thread Matthew Maurer
Functionality is almost identical, just structured for better
documentation and readability. Changes:

* Section names are checked for *all* non-SHT_NULL sections, not just
  SHF_ALLOC sections. We have code that accesses section names of
  non-SHF_ALLOC sections (see find_any_sec for example)
* The section name check occurs *before* strcmping on section names.
  Previously, it was possible to use an out-of-bounds offset to strcmp
  against ".modinfo" or ".gnu.linkonce.this_module"
* strtab is checked for NUL lead+termination and nonzero size
* The symbol table is swept to ensure offsets are inbounds of strtab

While some of these oversights would normally be worrying, all of the
potentially unverified accesses occur after signature check, and only in
response to a user who can load a kernel module.

Signed-off-by: Matthew Maurer 
---
 kernel/module/internal.h |   7 +-
 kernel/module/main.c | 585 +--
 2 files changed, 444 insertions(+), 148 deletions(-)

diff --git a/kernel/module/internal.h b/kernel/module/internal.h
index c8b7b4dcf782..d8dc52eb9c82 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;
 };
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 98fedfdb8db5..8b2848b3183a 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -193,6 +193,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)
 {
@@ -1627,7 +1659,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;
@@ -1646,62 +1678,80 @@ static int validate_section_offset(struct load_info 
*info, Elf_Shdr *shdr)
return 0;
 }
 
-/*
- * Check userspace passed ELF module against our expectations, and cache
- * useful variables for further processing as we go.
- *
- * This does basic validity checks against section offsets and sizes, the
- * section name string table, and the indices used for it (sh_name).
+/**
+ * elf_validity_ehdr() - Checks an ELF header for module validity
+ * @info: Load info containing the ELF header to check
  *
- * As a last step, since we're already checking the ELF sections we cache
- * useful variables which will be used later for our convenience:
+ * Checks whether an ELF header could belong to a valid module. Checks:
  *
- * o pointers to section headers
- * o cache the modinfo symbol section
- * o cache the string symbol section
- * o cache the module section
+ * * 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.
  *
- * As a last step we set info->mod to the temporary copy of the module in
- * info->hdr. The final one will be allocated in move_module(). Any
- * modifications we make to our copy of the module will be carried over
- * to the final minted module.
+ * Return: %0 if valid, %-ENOEXEC on failure.
  */
-static int elf_validity_cache_copy(struct load_info *info, int flags)
+static int elf_validity_ehdr(const struct load_info