| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMj1kXEVThBdH7H7NX+Ma8j0_85GevzczFgvvozK-TsqT2geFw@mail.gmail.com>
Date: Fri, 19 Sep 2025 15:59:21 +0200
From: Ard Biesheuvel <ardb@...nel.org>
To: Adrian Barnaś <abarnas@...gle.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will@...nel.org>,
Dylan Hatch <dylanbhatch@...gle.com>, Mark Rutland <mark.rutland@....com>
Subject: Re: [PATCH 1/2] arch: arm64: Fail module loading if dynamic SCS
patching fails
Hi,
On Fri, 19 Sept 2025 at 14:23, Adrian Barnaś <abarnas@...gle.com> wrote:
>
> 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@...gle.com>
> ---
> arch/arm64/include/asm/scs.h | 2 +-
> arch/arm64/kernel/module.c | 6 ++++--
> arch/arm64/kernel/pi/map_kernel.c | 2 +-
> arch/arm64/kernel/pi/patch-scs.c | 15 +++++++++++----
> arch/arm64/kernel/pi/pi.h | 2 +-
> 5 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/scs.h b/arch/arm64/include/asm/scs.h
> index a76f9b387a26..ffcfcda87f10 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 is_module);
>
Calling the parameter 'is_module' puts the policy in the SCS patching
code, which now has to reason about how patching a module differs from
patching other code.
So I'd prefer to call this 'two_pass' or 'dry_run' or whatever, where
setting it guarantees that when an error is returned, no function will
be left in an inconsistent state.
> #endif /* __ASSEMBLY __ */
>
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 40148d2725ce..5d6d228c6156 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -484,10 +484,12 @@ 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)
> + 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..78266fb1fa61 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 is_module)
> {
> int code_alignment_factor = 1;
> bool fde_use_sdata8 = false;
> @@ -276,12 +276,19 @@ int scs_patch(const u8 eh_frame[], int size)
> return EDYNSCS_INVALID_CIE_SDATA_SIZE;
> }
> } else {
> + /*
> + * For loadable module instead of running a dry run try
> + * to patch scs instruction in place and trigger error
> + * if failed, to prevent module loading.
> + */
Move this comment to the module loader, and explain why the two pass
approach is not needed in this case.
> ret = scs_handle_fde_frame(frame, code_alignment_factor,
> - fde_use_sdata8, true);
> + fde_use_sdata8, !is_module);
> if (ret)
> return ret;
> - scs_handle_fde_frame(frame, code_alignment_factor,
> - fde_use_sdata8, false);
> +
> + if (!is_module)
> + 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..4ccbba24fadc 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 is_module);
>
> 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
>
Powered by blists - more mailing lists