Re: [PATCH v4 1/2] module: add elf_check_module_arch for module specific elf arch checks

[Nicholas Piggin]

Excerpts from Jessica Yu's message of June 15, 2021 10:17 pm:
> +++ Nicholas Piggin [15/06/21 12:05 +1000]:
>>Excerpts from Jessica Yu's message of June 14, 2021 10:06 pm:
>>> +++ Nicholas Piggin [11/06/21 19:39 +1000]:
>>>>The elf_check_arch() function is used to test usermode binaries, but
>>>>kernel modules may have more specific requirements. powerpc would like
>>>>to test for ABI version compatibility.
>>>>
>>>>Add an arch-overridable function elf_check_module_arch() that defaults
>>>>to elf_check_arch() and use it in elf_validity_check().
>>>>
>>>>Signed-off-by: Michael Ellerman <mpe@xxxxxxxxxxxxxx>
>>>>[np: split patch, added changelog]
>>>>Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
>>>>---
>>>> include/linux/moduleloader.h | 5 +++++
>>>> kernel/module.c              | 2 +-
>>>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>>>>index 9e09d11ffe5b..fdc042a84562 100644
>>>>--- a/include/linux/moduleloader.h
>>>>+++ b/include/linux/moduleloader.h
>>>>@@ -13,6 +13,11 @@
>>>>  * must be implemented by each architecture.
>>>>  */
>>>>
>>>>+// Allow arch to optionally do additional checking of module ELF header
>>>>+#ifndef elf_check_module_arch
>>>>+#define elf_check_module_arch elf_check_arch
>>>>+#endif
>>>
>>> Hi Nicholas,
>>>
>>> Why not make elf_check_module_arch() consistent with the other
>>> arch-specific functions? Please see module_frob_arch_sections(),
>>> module_{init,exit}_section(), etc in moduleloader.h. That is, they are
>>> all __weak functions that are overridable by arches. We can maybe make
>>> elf_check_module_arch() a weak symbol, available for arches to
>>> override if they want to perform additional elf checks. Then we don't
>>> have to have this one-off #define.
>>
>>
>>Like this? I like it. Good idea.
> 
> Yeah! Also, maybe we can alternatively make elf_check_module_arch() a
> separate check entirely so that the powerpc implementation doesn't
> have to include that extra elf_check_arch() call. Something like this maybe?

Yeah we can do that. Would you be okay if it goes via powerpc tree? If 
yes, then we should get your Ack (or SOB because it seems to be entirely 
your patch now :D)

Thanks,
Nick

> 
> diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
> index 9e09d11ffe5b..2f9ebd593b4f 100644
> --- a/include/linux/moduleloader.h
> +++ b/include/linux/moduleloader.h
> @@ -39,6 +39,9 @@ bool module_init_section(const char *name);
>    */
>   bool module_exit_section(const char *name);
> 
> +/* Arch may override to do additional checking of ELF header architecture */
> +int elf_check_module_arch(Elf_Ehdr *hdr);
> +
>   /*
>    * Apply the given relocation to the (simplified) ELF.  Return -error
>    * or 0.
> diff --git a/kernel/module.c b/kernel/module.c
> index fdd6047728df..9963a979ed54 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2923,6 +2923,11 @@ static int validate_section_offset(struct load_info *info, Elf_Shdr *shdr)
>          return 0;
>   }
> 
> +int __weak elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> +       return 1;
> +}
> +
>   /*
>    * Sanity checks against invalid binaries, wrong arch, weird elf version.
>    *
> @@ -2941,6 +2946,7 @@ static int elf_validity_check(struct load_info *info)
>          if (memcmp(info->hdr->e_ident, ELFMAG, SELFMAG) != 0
>              || info->hdr->e_type != ET_REL
>              || !elf_check_arch(info->hdr)
> +           || !elf_check_module_arch(info->hdr)
>              || info->hdr->e_shentsize != sizeof(Elf_Shdr))
>                  return -ENOEXEC;
>   
> 
>