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

["Adrian Barnaś" <abarnas@google.com>]

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@google.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@google.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