[PATCH v2 0/2] arm64: modules: Reject loading of malformed modules

[PATCH v2 0/2] arm64: modules: Reject loading of malformed modules

[Adrian Barnaś]

Hi all,

Here is version two of the patches I previously posted here:

  v1: https://lore.kernel.org/all/20250919122321.946462-1-abarnas@google.com/

Changes:
  * Renamed the parameter `is_module` to `skip_dry_run` in scs_patch()
  * Moved comments to module_finalize() and improve justification
  * Instead of rejecting all modules with callback, reject those with cb
    pointing outside core kernel text
  * Replace -EPERM to -ENOEXEC when rejecting modules with incorrect cb
  * Fix missing return in apply_alternatives_module() placeholder

Best regards
Adrian

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: Dylan Hatch <dylanbhatch@google.com>
Cc: Mark Rutland <mark.rutland@arm.com>

Adrian Barnaś (2):
  arch: arm64: Fail module loading if dynamic SCS patching fails
  arch: arm64: Reject modules with internal alternative callbacks

 arch/arm64/include/asm/alternative.h |  7 +++++--
 arch/arm64/include/asm/scs.h         |  2 +-
 arch/arm64/kernel/alternative.c      | 19 ++++++++++++-------
 arch/arm64/kernel/module.c           | 21 +++++++++++++++++----
 arch/arm64/kernel/pi/map_kernel.c    |  2 +-
 arch/arm64/kernel/pi/patch-scs.c     | 10 ++++++----
 arch/arm64/kernel/pi/pi.h            |  2 +-
 7 files changed, 43 insertions(+), 20 deletions(-)

-- 
2.51.0.534.gc79095c0ca-goog

[PATCH v2 1/2] arch: arm64: Fail module loading if dynamic SCS patching fails

[Adrian Barnaś]

Disallow a module to load if SCS dynamic patching fails for its code. For
module loading, instead of running a dry-run to check for patching errors,
try to run patching in the first run and propagate any errors so module
loading will fail.

Signed-off-by: Adrian Barnaś <abarnas@google.com>
---
 arch/arm64/include/asm/scs.h      |  2 +-
 arch/arm64/kernel/module.c        | 12 ++++++++++--
 arch/arm64/kernel/pi/map_kernel.c |  2 +-
 arch/arm64/kernel/pi/patch-scs.c  | 10 ++++++----
 arch/arm64/kernel/pi/pi.h         |  2 +-
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
index a76f9b387a26..c59f6324f2bb 100644
--- a/arch/arm64/include/asm/scs.h
+++ b/arch/arm64/include/asm/scs.h
@@ -53,7 +53,7 @@ enum {
 	EDYNSCS_INVALID_CFA_OPCODE		= 4,
 };
 
-int __pi_scs_patch(const u8 eh_frame[], int size);
+int __pi_scs_patch(const u8 eh_frame[], int size, bool skip_dry_run);
 
 #endif /* __ASSEMBLY __ */
 
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index 40148d2725ce..d32ab7dd86a8 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -484,10 +484,18 @@ int module_finalize(const Elf_Ehdr *hdr,
 	if (scs_is_dynamic()) {
 		s = find_section(hdr, sechdrs, ".init.eh_frame");
 		if (s) {
-			ret = __pi_scs_patch((void *)s->sh_addr, s->sh_size);
-			if (ret)
+			/*
+			 * Because we can reject modules that are malformed
+			 * so SCS patching fails, skip dry run and try to patch
+			 * it in place. If patching fails, the module would not
+			 * be loaded anyway.
+			 */
+			ret = __pi_scs_patch((void *)s->sh_addr, s->sh_size, true);
+			if (ret) {
 				pr_err("module %s: error occurred during dynamic SCS patching (%d)\n",
 				       me->name, ret);
+				return -ENOEXEC;
+			}
 		}
 	}
 
diff --git a/arch/arm64/kernel/pi/map_kernel.c b/arch/arm64/kernel/pi/map_kernel.c
index 0f4bd7771859..7187eda9e8a5 100644
--- a/arch/arm64/kernel/pi/map_kernel.c
+++ b/arch/arm64/kernel/pi/map_kernel.c
@@ -98,7 +98,7 @@ static void __init map_kernel(u64 kaslr_offset, u64 va_offset, int root_level)
 
 		if (enable_scs) {
 			scs_patch(__eh_frame_start + va_offset,
-				  __eh_frame_end - __eh_frame_start);
+				  __eh_frame_end - __eh_frame_start, false);
 			asm("ic ialluis");
 
 			dynamic_scs_is_enabled = true;
diff --git a/arch/arm64/kernel/pi/patch-scs.c b/arch/arm64/kernel/pi/patch-scs.c
index 55d0cd64ef71..bbe7d30ed12b 100644
--- a/arch/arm64/kernel/pi/patch-scs.c
+++ b/arch/arm64/kernel/pi/patch-scs.c
@@ -225,7 +225,7 @@ static int scs_handle_fde_frame(const struct eh_frame *frame,
 	return 0;
 }
 
-int scs_patch(const u8 eh_frame[], int size)
+int scs_patch(const u8 eh_frame[], int size, bool skip_dry_run)
 {
 	int code_alignment_factor = 1;
 	bool fde_use_sdata8 = false;
@@ -277,11 +277,13 @@ int scs_patch(const u8 eh_frame[], int size)
 			}
 		} else {
 			ret = scs_handle_fde_frame(frame, code_alignment_factor,
-						   fde_use_sdata8, true);
+						   fde_use_sdata8, !skip_dry_run);
 			if (ret)
 				return ret;
-			scs_handle_fde_frame(frame, code_alignment_factor,
-					     fde_use_sdata8, false);
+
+			if (!skip_dry_run)
+				scs_handle_fde_frame(frame, code_alignment_factor,
+						     fde_use_sdata8, false);
 		}
 
 		p += sizeof(frame->size) + frame->size;
diff --git a/arch/arm64/kernel/pi/pi.h b/arch/arm64/kernel/pi/pi.h
index 46cafee7829f..220da972cce1 100644
--- a/arch/arm64/kernel/pi/pi.h
+++ b/arch/arm64/kernel/pi/pi.h
@@ -27,7 +27,7 @@ extern pgd_t init_pg_dir[], init_pg_end[];
 void init_feature_override(u64 boot_status, const void *fdt, int chosen);
 u64 kaslr_early_init(void *fdt, int chosen);
 void relocate_kernel(u64 offset);
-int scs_patch(const u8 eh_frame[], int size);
+int scs_patch(const u8 eh_frame[], int size, bool skip_dry_run);
 
 void map_range(u64 *pgd, u64 start, u64 end, u64 pa, pgprot_t prot,
 	       int level, pte_t *tbl, bool may_use_cont, u64 va_offset);
-- 
2.51.0.534.gc79095c0ca-goog

[PATCH v2 2/2] arch: arm64: Reject modules with internal alternative callbacks

[Adrian Barnaś]

During module loading, check if a callback function used by the
alternatives specified in the '.altinstruction' ELF section (if present)
is located in core kernel .text. If not fail module loading before
callback is called.

Reported-by: Fanqin Cui <cuifq1@chinatelecom.cn>
Closes: https://lore.kernel.org/all/20250807072700.348514-1-fanqincui@163.com/
Signed-off-by: Adrian Barnaś <abarnas@google.com>
---
 arch/arm64/include/asm/alternative.h |  7 +++++--
 arch/arm64/kernel/alternative.c      | 19 ++++++++++++-------
 arch/arm64/kernel/module.c           |  9 +++++++--
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
index 00d97b8a757f..51746005239b 100644
--- a/arch/arm64/include/asm/alternative.h
+++ b/arch/arm64/include/asm/alternative.h
@@ -26,9 +26,12 @@ void __init apply_alternatives_all(void);
 bool alternative_is_applied(u16 cpucap);
 
 #ifdef CONFIG_MODULES
-void apply_alternatives_module(void *start, size_t length);
+int apply_alternatives_module(void *start, size_t length);
 #else
-static inline void apply_alternatives_module(void *start, size_t length) { }
+static inline int apply_alternatives_module(void *start, size_t length)
+{
+	return 0;
+}
 #endif
 
 void alt_cb_patch_nops(struct alt_instr *alt, __le32 *origptr,
diff --git a/arch/arm64/kernel/alternative.c b/arch/arm64/kernel/alternative.c
index 8ff6610af496..11893a0360ad 100644
--- a/arch/arm64/kernel/alternative.c
+++ b/arch/arm64/kernel/alternative.c
@@ -139,9 +139,9 @@ static noinstr void clean_dcache_range_nopatch(u64 start, u64 end)
 	} while (cur += d_size, cur < end);
 }
 
-static void __apply_alternatives(const struct alt_region *region,
-				 bool is_module,
-				 unsigned long *cpucap_mask)
+static int __apply_alternatives(const struct alt_region *region,
+				bool is_module,
+				unsigned long *cpucap_mask)
 {
 	struct alt_instr *alt;
 	__le32 *origptr, *updptr;
@@ -166,10 +166,13 @@ static void __apply_alternatives(const struct alt_region *region,
 		updptr = is_module ? origptr : lm_alias(origptr);
 		nr_inst = alt->orig_len / AARCH64_INSN_SIZE;
 
-		if (ALT_HAS_CB(alt))
+		if (ALT_HAS_CB(alt)) {
 			alt_cb  = ALT_REPL_PTR(alt);
-		else
+			if (!core_kernel_text((unsigned long)alt_cb))
+				return -ENOEXEC;
+		} else {
 			alt_cb = patch_alternative;
+		}
 
 		alt_cb(alt, origptr, updptr, nr_inst);
 
@@ -193,6 +196,8 @@ static void __apply_alternatives(const struct alt_region *region,
 		bitmap_and(applied_alternatives, applied_alternatives,
 			   system_cpucaps, ARM64_NCAPS);
 	}
+
+	return 0;
 }
 
 static void __init apply_alternatives_vdso(void)
@@ -277,7 +282,7 @@ void __init apply_boot_alternatives(void)
 }
 
 #ifdef CONFIG_MODULES
-void apply_alternatives_module(void *start, size_t length)
+int apply_alternatives_module(void *start, size_t length)
 {
 	struct alt_region region = {
 		.begin	= start,
@@ -287,7 +292,7 @@ void apply_alternatives_module(void *start, size_t length)
 
 	bitmap_fill(all_capabilities, ARM64_NCAPS);
 
-	__apply_alternatives(&region, true, &all_capabilities[0]);
+	return __apply_alternatives(&region, true, &all_capabilities[0]);
 }
 #endif
 
diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
index d32ab7dd86a8..6e5b488a219e 100644
--- a/arch/arm64/kernel/module.c
+++ b/arch/arm64/kernel/module.c
@@ -478,8 +478,13 @@ int module_finalize(const Elf_Ehdr *hdr,
 	int ret;
 
 	s = find_section(hdr, sechdrs, ".altinstructions");
-	if (s)
-		apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+	if (s) {
+		ret = apply_alternatives_module((void *)s->sh_addr, s->sh_size);
+		if (ret < 0) {
+			pr_err("module %s: error occurred when applying alternatives\n", me->name);
+			return ret;
+		}
+	}
 
 	if (scs_is_dynamic()) {
 		s = find_section(hdr, sechdrs, ".init.eh_frame");
-- 
2.51.0.534.gc79095c0ca-goog

Re: [PATCH v2 0/2] arm64: modules: Reject loading of malformed modules

[Ard Biesheuvel]

On Mon, 22 Sept 2025 at 15:04, Adrian Barnaś <abarnas@google.com> wrote:
>
> Hi all,
>
> Here is version two of the patches I previously posted here:
>
>   v1: https://lore.kernel.org/all/20250919122321.946462-1-abarnas@google.com/
>
> Changes:
>   * Renamed the parameter `is_module` to `skip_dry_run` in scs_patch()
>   * Moved comments to module_finalize() and improve justification
>   * Instead of rejecting all modules with callback, reject those with cb
>     pointing outside core kernel text
>   * Replace -EPERM to -ENOEXEC when rejecting modules with incorrect cb
>   * Fix missing return in apply_alternatives_module() placeholder
>
> Best regards
> Adrian
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Dylan Hatch <dylanbhatch@google.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
>
> Adrian Barnaś (2):
>   arch: arm64: Fail module loading if dynamic SCS patching fails
>   arch: arm64: Reject modules with internal alternative callbacks
>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Re: [PATCH v2 2/2] arch: arm64: Reject modules with internal alternative callbacks

[Mark Rutland]

On Mon, Sep 22, 2025 at 01:04:27PM +0000, Adrian Barnaś wrote:
> During module loading, check if a callback function used by the
> alternatives specified in the '.altinstruction' ELF section (if present)
> is located in core kernel .text. If not fail module loading before
> callback is called.
> 
> Reported-by: Fanqin Cui <cuifq1@chinatelecom.cn>
> Closes: https://lore.kernel.org/all/20250807072700.348514-1-fanqincui@163.com/
> Signed-off-by: Adrian Barnaś <abarnas@google.com>
> ---
>  arch/arm64/include/asm/alternative.h |  7 +++++--
>  arch/arm64/kernel/alternative.c      | 19 ++++++++++++-------
>  arch/arm64/kernel/module.c           |  9 +++++++--
>  3 files changed, 24 insertions(+), 11 deletions(-)

[...]

> @@ -166,10 +166,13 @@ static void __apply_alternatives(const struct alt_region *region,
>  		updptr = is_module ? origptr : lm_alias(origptr);
>  		nr_inst = alt->orig_len / AARCH64_INSN_SIZE;
>  
> -		if (ALT_HAS_CB(alt))
> +		if (ALT_HAS_CB(alt)) {
>  			alt_cb  = ALT_REPL_PTR(alt);
> -		else
> +			if (!core_kernel_text((unsigned long)alt_cb))
> +				return -ENOEXEC;
> +		} else {
>  			alt_cb = patch_alternative;
> +		}

There's an existing noinstr safety issue there that this makes a bit
worse.

The core_kernel_text() function is instrumentable, and so for
(non-module) alternatives, calling that in the middle of patching isn't
safe (as it could lead to calling arbitrary C code mid-patching).

That said, __apply_alternatives() aren't marked as noinstr, and cleaning
that up properly is going to require some major rework. I don't think we
want to block this patch on that.

I think we can bodge around the worst of that for now with:

	if (is_module && !core_kernel_text((unsigned long)alt_cb))
		return -ENOEXEC;

... which'll avoid calling out to other instrumentable code during the
patching sequence, and minimize the risk of that blowing up during boot.

I'll see about prioritizing a follow-up to fix the extant issues more
thoroughly.

Will, are you happy to add the 'is module &&' part to the condition?

Mark.

Re: [PATCH v2 0/2] arm64: modules: Reject loading of malformed modules

[Will Deacon]

On Mon, 22 Sep 2025 13:04:25 +0000, Adrian Barnaś wrote:
> Here is version two of the patches I previously posted here:
> 
>   v1: https://lore.kernel.org/all/20250919122321.946462-1-abarnas@google.com/
> 
> Changes:
>   * Renamed the parameter `is_module` to `skip_dry_run` in scs_patch()
>   * Moved comments to module_finalize() and improve justification
>   * Instead of rejecting all modules with callback, reject those with cb
>     pointing outside core kernel text
>   * Replace -EPERM to -ENOEXEC when rejecting modules with incorrect cb
>   * Fix missing return in apply_alternatives_module() placeholder
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/2] arm64: Fail module loading if dynamic SCS patching fails
      https://git.kernel.org/arm64/c/6d4a0fbd34a4
[2/2] arm64: Reject modules with internal alternative callbacks
      https://git.kernel.org/arm64/c/8e8ae788964a

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev