| 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: <aM1s-I0gM_rAf5LU@google.com>
Date: Fri, 19 Sep 2025 14:47:20 +0000
From: Adrian Barnaś <abarnas@...gle.com>
To: Ard Biesheuvel <ardb@...nel.org>
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, Sep 19, 2025 at 03:59:21PM +0200, Ard Biesheuvel wrote:
>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.
Agreed.
>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.
Maybe `bool skip_dry_run`?
>
>> #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.
>
Will do.
>> 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
>>
Thank you for a review,
Adrian
Powered by blists - more mailing lists