On Wed, May 05, 2021 at 10:07:29PM +1000, Michael Ellerman wrote:
> Michal Suchánek <msucha...@suse.de> writes:
> > On Mon, May 03, 2021 at 01:37:57PM +0200, Andreas Schwab wrote:
> >> Should this add a tag to the module vermagic?
> >
> > Would the modues link even if the vermagic was not changed?
>
> Most modules will require some symbols from the kernel, and those will
> be dot symbols, which won't resolve.
>
> But there are a few small modules that don't rely on any kernel symbols,
> which can load.
>
> > I suppose something like this might do it.
>
> It would, but I feel like we should be handling this at the ELF level.
> ie. we don't allow loading modules with a different ELF machine type, so
> neither should we allow loading a module with the wrong ELF ABI.
>
> And you can build the kernel without MODVERSIONS, so relying on
> MODVERSIONS still leaves a small exposure (same kernel version
> with/without ELFv2).
>
> I don't see an existing hook that would do what we want. There's
> elf_check_arch(), but that also applies to userspace binaries, which is
> not what we want.
>
> Maybe something like below.
The below patch works for me.
Tested-by: Michal Suchánek <msucha...@suse.de>
Built a Hello World module for both v1 and v2 ABI, and kernels built
with v1 and v2 ABI rejected module with the other ABI.
[ 100.602943] Module has invalid ELF structures
insmod: ERROR: could not insert module moin_v1.ko: Invalid module format
Thanks
Michal
>
> cheers
>
>
> diff --git a/arch/powerpc/include/asm/module.h
> b/arch/powerpc/include/asm/module.h
> index 857d9ff24295..d0e9368982d8 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -83,5 +83,28 @@ static inline int module_finalize_ftrace(struct module
> *mod, const Elf_Shdr *sec
> }
> #endif
>
> +#ifdef CONFIG_PPC64
> +static inline bool elf_check_module_arch(Elf_Ehdr *hdr)
> +{
> + unsigned long flags;
> +
> + if (!elf_check_arch(hdr))
> + return false;
> +
> + flags = hdr->e_flags & 0x3;
> +
> +#ifdef CONFIG_PPC64_BUILD_ELF_V2_ABI
> + if (flags == 2)
> + return true;
> +#else
> + if (flags < 2)
> + return true;
> +#endif
> + return false;
> +}
> +
> +#define elf_check_module_arch elf_check_module_arch
> +#endif /* CONFIG_PPC64 */
> +
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_MODULE_H */
> 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
> +
> /* Adjust arch-specific sections. Return 0 on success. */
> int module_frob_arch_sections(Elf_Ehdr *hdr,
> Elf_Shdr *sechdrs,
> diff --git a/kernel/module.c b/kernel/module.c
> index b5dd92e35b02..c71889107226 100644
> --- a/kernel/module.c
> +++ b/kernel/module.c
> @@ -2941,7 +2941,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;
>
