Re: [PATCH v3] powerpc/64: Option to use ELFv2 ABI for big-endian kernels

Should this add a tag to the module vermagic?

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

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?

I suppose something like this might do it.

Thanks

Michal

diff --git a/arch/powerpc/include/asm/vermagic.h b/arch/powerpc/include/asm/vermagic.h
index b054a8576e5d..3fdaacd7a743 100644
--- a/arch/powerpc/include/asm/vermagic.h
+++ b/arch/powerpc/include/asm/vermagic.h

@@ -14,7 +14,14 @@
 #define MODULE_ARCH_VERMAGIC_RELOCATABLE	""
 #endif
 
+
+#ifdef CONFIG_PPC64_BUILD_BIG_ENDIAN_ELF_V2_ABI
+#define MODULE_ARCH_VERMAGIC_ELF_V2_ABI	"abi-elfv2 "
+#else
+#define MODULE_ARCH_VERMAGIC_ELF_V2_ABI	""
+#endif
+
 #define MODULE_ARCH_VERMAGIC \
-		MODULE_ARCH_VERMAGIC_FTRACE MODULE_ARCH_VERMAGIC_RELOCATABLE
+		MODULE_ARCH_VERMAGIC_FTRACE MODULE_ARCH_VERMAGIC_RELOCATABLE MODULE_ARCH_VERMAGIC_ELF_V2_ABI
 
 #endif /* _ASM_VERMAGIC_H */

Excerpts from Michal Suchánek's message of May 4, 2021 12:38 am:

On Mon, May 03, 2021 at 01:37:57PM +0200, Andreas Schwab wrote:

quoted

Should this add a tag to the module vermagic?

Good question.

Would the modues link even if the vermagic was not changed?

I suppose something like this might do it.

Yeah I don't know how robust the loader would be to a mismatch. We could 
add this patch.

Thanks,
Nick

Michal Suchánek [off-list ref] writes:

On Mon, May 03, 2021 at 01:37:57PM +0200, Andreas Schwab wrote:

quoted

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.

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;
 

On Wed, May 05, 2021 at 10:07:29PM +1000, Michael Ellerman wrote:

Michal Suchánek [off-list ref] writes:

quoted

On Mon, May 03, 2021 at 01:37:57PM +0200, Andreas Schwab wrote:

quoted

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.

quoted

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.

Yes, that looks better.

Thanks

Michal

On Wed, May 05, 2021 at 10:07:29PM +1000, Michael Ellerman wrote:

Michal Suchánek [off-list ref] writes:

quoted

On Mon, May 03, 2021 at 01:37:57PM +0200, Andreas Schwab wrote:

quoted

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.

quoted

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 <redacted>

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