[PATCH v5 0/2] powerpc module arch checks

[PATCH v5 0/2] powerpc module arch checks

[Nicholas Piggin]

These slipped through the cracks. Picking them up again...

The story so far is that Jessica gave us the new scheme in patch 1,
now slightly rebased. Patch 2 implements the additional check
that powerpc wants which originally came from Michael, and has been
updated to the new approach.

This was previously attached to the ELFv2 build option for big-endian
kernels, but it can go ahead of that option.

Just checking everybody is still okay with the code and their SOBs,
and Luis if you would be okay for patch 1 to be merged via powerpc or
prefer to take it in the module tree (or maybe you object to the
code in the first place).

Thanks,
Nick

Nicholas Piggin (2):
  module: add module_elf_check_arch for module-specific checks
  powerpc/64: Add module check for ELF ABI version

 arch/powerpc/kernel/module.c | 17 +++++++++++++++++
 include/linux/moduleloader.h |  3 +++
 kernel/module/main.c         | 10 ++++++++++
 3 files changed, 30 insertions(+)

-- 
2.37.2

[PATCH v5 1/2] module: add module_elf_check_arch for module-specific checks

[Nicholas Piggin]

The elf_check_arch() function is also used to test compatibility of
usermode binaries. Kernel modules may have more specific requirements,
for example powerpc would like to test for ABI version compatibility.

Add a weak module_elf_check_arch() that defaults to true, and call it
from elf_validity_check().

Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Jessica Yu <jeyu@kernel.org>
[np: added changelog, adjust name, rebase]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 include/linux/moduleloader.h |  3 +++
 kernel/module/main.c         | 10 ++++++++++
 2 files changed, 13 insertions(+)

diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 9e09d11ffe5b..7b4587a19189 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -13,6 +13,9 @@
  * must be implemented by each architecture.
  */
 
+/* arch may override to do additional checking of ELF header architecture */
+bool module_elf_check_arch(Elf_Ehdr *hdr);
+
 /* Adjust arch-specific sections.  Return 0 on success.  */
 int module_frob_arch_sections(Elf_Ehdr *hdr,
 			      Elf_Shdr *sechdrs,
diff --git a/kernel/module/main.c b/kernel/module/main.c
index d02d39c7174e..7b3f6fb0d428 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1674,6 +1674,11 @@ static int elf_validity_check(struct load_info *info)
 		       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;
@@ -2247,6 +2252,11 @@ static void flush_module_icache(const struct module *mod)
 			   (unsigned long)mod->core_layout.base + mod->core_layout.size);
 }
 
+bool __weak module_elf_check_arch(Elf_Ehdr *hdr)
+{
+	return true;
+}
+
 int __weak module_frob_arch_sections(Elf_Ehdr *hdr,
 				     Elf_Shdr *sechdrs,
 				     char *secstrings,
-- 
2.37.2

[PATCH v5 2/2] powerpc/64: Add module check for ELF ABI version

[Nicholas Piggin]

Override the generic module ELF check to provide a check for the ELF ABI
version. This becomes important if we allow big-endian ELF ABI V2 builds
but it doesn't hurt to check now.

Cc: Jessica Yu <jeyu@kernel.org>
Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
[np: split patch, added changelog, adjust to Jessica's proposal]
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
 arch/powerpc/kernel/module.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
index f6d6ae0a1692..d46bf9bfda26 100644
--- a/arch/powerpc/kernel/module.c
+++ b/arch/powerpc/kernel/module.c
@@ -19,6 +19,23 @@
 
 static LIST_HEAD(module_bug_list);
 
+#ifdef CONFIG_PPC64
+bool module_elf_check_arch(Elf_Ehdr *hdr)
+{
+	unsigned long abi_level = hdr->e_flags & 0x3;
+
+	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2)) {
+		if (abi_level != 2)
+			return false;
+	} else {
+		if (abi_level >= 2)
+			return false;
+	}
+
+	return true;
+}
+#endif
+
 static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
 				    const Elf_Shdr *sechdrs,
 				    const char *name)
-- 
2.37.2

Re: [PATCH v5 1/2] module: add module_elf_check_arch for module-specific checks

[Luis Chamberlain]

On Mon, Oct 31, 2022 at 10:07:32PM +1000, Nicholas Piggin wrote:
> The elf_check_arch() function is also used to test compatibility of
> usermode binaries. Kernel modules may have more specific requirements,
> for example powerpc would like to test for ABI version compatibility.
> 
> Add a weak module_elf_check_arch() that defaults to true, and call it
> from elf_validity_check().
> 
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Jessica Yu <jeyu@kernel.org>
> [np: added changelog, adjust name, rebase]
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>

Acked-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [PATCH v5 0/2] powerpc module arch checks

[Luis Chamberlain]

On Mon, Oct 31, 2022 at 10:07:31PM +1000, Nicholas Piggin wrote:
> Luis if you would be okay for patch 1 to be merged via powerpc or
> prefer to take it in the module tree (or maybe you object to the
> code in the first place).

Looks good to me, and nothing on my radar which would cause a conflict
so happy for you to take it via powerpc or I can take it and apply it
right away to tricke / get tested on linux-next by tomorrow.

Let me know.

  Luis

Re: [PATCH v5 2/2] powerpc/64: Add module check for ELF ABI version

[Christophe Leroy]

Le 31/10/2022 à 13:07, Nicholas Piggin a écrit :
> Override the generic module ELF check to provide a check for the ELF ABI
> version. This becomes important if we allow big-endian ELF ABI V2 builds
> but it doesn't hurt to check now.
> 
> Cc: Jessica Yu <jeyu@kernel.org>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> [np: split patch, added changelog, adjust to Jessica's proposal]
> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> ---
>   arch/powerpc/kernel/module.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
> 
> diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
> index f6d6ae0a1692..d46bf9bfda26 100644
> --- a/arch/powerpc/kernel/module.c
> +++ b/arch/powerpc/kernel/module.c
> @@ -19,6 +19,23 @@
>   
>   static LIST_HEAD(module_bug_list);
>   
> +#ifdef CONFIG_PPC64

Can it go in arch/powerpc/kernel/module_64.c instead ?

> +bool module_elf_check_arch(Elf_Ehdr *hdr)
> +{
> +	unsigned long abi_level = hdr->e_flags & 0x3;
> +
> +	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2)) {
> +		if (abi_level != 2)
> +			return false;
> +	} else {
> +		if (abi_level >= 2)
> +			return false;
> +	}
> +
> +	return true;

Can be simpler:

	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
		return abi_level == 2;
	else
		return abi_level < 2;


> +}
> +#endif
> +
>   static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
>   				    const Elf_Shdr *sechdrs,
>   				    const char *name)

Re: [PATCH v5 0/2] powerpc module arch checks

[Michael Ellerman]

Luis Chamberlain <mcgrof@kernel.org> writes:
> On Mon, Oct 31, 2022 at 10:07:31PM +1000, Nicholas Piggin wrote:
>> Luis if you would be okay for patch 1 to be merged via powerpc or
>> prefer to take it in the module tree (or maybe you object to the
>> code in the first place).
>
> Looks good to me, and nothing on my radar which would cause a conflict
> so happy for you to take it via powerpc or I can take it and apply it
> right away to tricke / get tested on linux-next by tomorrow.
>
> Let me know.

Thanks. I guess it's mostly a nop on other architectures, so probably
makes sense for me to take it.

cheers

Re: [PATCH v5 2/2] powerpc/64: Add module check for ELF ABI version

[Nicholas Piggin]

On Thu Nov 3, 2022 at 6:35 PM AEST, Christophe Leroy wrote:
>
>
> Le 31/10/2022 à 13:07, Nicholas Piggin a écrit :
> > Override the generic module ELF check to provide a check for the ELF ABI
> > version. This becomes important if we allow big-endian ELF ABI V2 builds
> > but it doesn't hurt to check now.
> > 
> > Cc: Jessica Yu <jeyu@kernel.org>
> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
> > [np: split patch, added changelog, adjust to Jessica's proposal]
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >   arch/powerpc/kernel/module.c | 17 +++++++++++++++++
> >   1 file changed, 17 insertions(+)
> > 
> > diff --git a/arch/powerpc/kernel/module.c b/arch/powerpc/kernel/module.c
> > index f6d6ae0a1692..d46bf9bfda26 100644
> > --- a/arch/powerpc/kernel/module.c
> > +++ b/arch/powerpc/kernel/module.c
> > @@ -19,6 +19,23 @@
> >   
> >   static LIST_HEAD(module_bug_list);
> >   
> > +#ifdef CONFIG_PPC64
>
> Can it go in arch/powerpc/kernel/module_64.c instead ?
>
> > +bool module_elf_check_arch(Elf_Ehdr *hdr)
> > +{
> > +	unsigned long abi_level = hdr->e_flags & 0x3;
> > +
> > +	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2)) {
> > +		if (abi_level != 2)
> > +			return false;
> > +	} else {
> > +		if (abi_level >= 2)
> > +			return false;
> > +	}
> > +
> > +	return true;
>
> Can be simpler:
>
> 	if (IS_ENABLED(CONFIG_PPC64_ELF_ABI_V2))
> 		return abi_level == 2;
> 	else
> 		return abi_level < 2;

Yes I think both of those can be done. Good suggestions.

Thanks,
Nick