linux-kernel - Re: [PATCH 2/2] arch: arm64: Reject modules with internal alternative callbacks

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMj1kXGaT5Kf9=v0rSxW5TB__vDN=iPK7=+gDNBO=vTPrd_4Zg@mail.gmail.com>
Date: Fri, 19 Sep 2025 17:01:09 +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>, 
	Fanqin Cui <cuifq1@...natelecom.cn>
Subject: Re: [PATCH 2/2] arch: arm64: Reject modules with internal alternative callbacks

Hi Adrian,

On Fri, 19 Sept 2025 at 14:23, Adrian Barnaś <abarnas@...gle.com> wrote:
>
> During module loading, check if there is a callback function used by the
> alternatives specified in the '.altinstruction' ELF section and block
> loading the module if such a function is present.
>

Why?

AIUI, the issue being addressed is the fact that we cannot yet execute
code from the module itself when alternatives are being applied, and
so the callback must live in the core kernel, or in another module.

So this is a really big hammer, given that it disallows all callback
alternatives, including ones that we could easily support.

I don't object to the approach per se, but please explain why this
solution is reasonable (after you've explained the problem)



> Reported-by: Fanqin Cui <cuifq1@...natelecom.cn>
> Closes: https://lore.kernel.org/all/20250807072700.348514-1-fanqincui@163.com/
> Signed-off-by: Adrian Barnaś <abarnas@...gle.com>
> ---
>  arch/arm64/include/asm/alternative.h |  4 ++--
>  arch/arm64/kernel/alternative.c      | 15 ++++++++++-----
>  arch/arm64/kernel/module.c           |  8 ++++++--
>  3 files changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
> index 00d97b8a757f..6eab2517c809 100644
> --- a/arch/arm64/include/asm/alternative.h
> +++ b/arch/arm64/include/asm/alternative.h
> @@ -26,9 +26,9 @@ 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) { }
>  #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..69fae94c843a 100644
> --- a/arch/arm64/kernel/alternative.c
> +++ b/arch/arm64/kernel/alternative.c
> @@ -139,15 +139,18 @@ 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;
>         alternative_cb_t alt_cb;
>
>         for (alt = region->begin; alt < region->end; alt++) {
> +               if (ALT_HAS_CB(alt) && is_module)
> +                       return -EPERM;

Nit: is EPERM the appropriate error code here?

> +
>                 int nr_inst;
>                 int cap = ALT_CAP(alt);
>
> @@ -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 5d6d228c6156..aeefc50229e3 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -478,8 +478,12 @@ 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
>