linux-kernel - Re: [PATCH] ARM: module: Add all unwind tables when load module

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: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YkVQXPYkae41B7Z2@shell.armlinux.org.uk>
Date:   Thu, 31 Mar 2022 07:55:24 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Chen Zhongjin <chenzhongjin@...wei.com>
Cc:     alexander.sverdlin@...ia.com, ardb@...nel.org,
        linus.walleij@...aro.org, nico@...xnic.net,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ARM: module: Add all unwind tables when load module

Hi,

On Fri, Mar 25, 2022 at 09:12:52AM +0800, Chen Zhongjin wrote:
> For EABI stack unwinding, when loading .ko module
> the EXIDX sections will be added to a unwind_table list.
> 
> However not all EXIDX sections are added because EXIDX
> sections are searched by hardcoded section names.
> 
> For functions in other sections such as .ref.text
> or .kprobes.text, gcc generates seprated EXIDX sections
> (such as .ARM.exidx.ref.text or .ARM.exidx.kprobes.text).
> 
> These extra EXIDX sections are not loaded, so when unwinding
> functions in these sections, we will failed with:
> 
> 	unwind: Index not found xxx
> 
> To fix that, I refactor the code for searching and adding
> EXIDX sections:
> 
> - Check section type to search EXIDX tables (0x70000001)
> instead of strcmp() the hardcoded names. Then find the
> corresponding text sections by their section names.
> 
> - Add a unwind_table list in module->arch to save their own
> unwind_table instead of the fixed-lenth array.
> 
> - Save .ARM.exidx.init.text section ptr, because it should
> be cleaned after module init.
> 
> Now all EXIDX sections of .ko can be added correctly.
> 
> Signed-off-by: Chen Zhongjin <chenzhongjin@...wei.com>
> ---
>  arch/arm/include/asm/module.h | 17 ++------
>  arch/arm/include/asm/unwind.h |  1 +
>  arch/arm/kernel/module.c      | 73 +++++++++++++++++------------------
>  3 files changed, 40 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/arm/include/asm/module.h b/arch/arm/include/asm/module.h
> index cfffae67c04e..8139b6a33a22 100644
> --- a/arch/arm/include/asm/module.h
> +++ b/arch/arm/include/asm/module.h
> @@ -3,20 +3,10 @@
>  #define _ASM_ARM_MODULE_H
>  
>  #include <asm-generic/module.h>
> -
> -struct unwind_table;
> +#include <asm/unwind.h>
>  
>  #ifdef CONFIG_ARM_UNWIND
> -enum {
> -	ARM_SEC_INIT,
> -	ARM_SEC_DEVINIT,
> -	ARM_SEC_CORE,
> -	ARM_SEC_EXIT,
> -	ARM_SEC_DEVEXIT,
> -	ARM_SEC_HOT,
> -	ARM_SEC_UNLIKELY,
> -	ARM_SEC_MAX,
> -};
> +#define ELF_SECTION_UNWIND 0x70000001
>  #endif
>  
>  #define PLT_ENT_STRIDE		L1_CACHE_BYTES
> @@ -36,7 +26,8 @@ struct mod_plt_sec {
>  
>  struct mod_arch_specific {
>  #ifdef CONFIG_ARM_UNWIND
> -	struct unwind_table *unwind[ARM_SEC_MAX];
> +	struct unwind_table unwind_list;
> +	struct unwind_table *init_table;
>  #endif
>  #ifdef CONFIG_ARM_MODULE_PLTS
>  	struct mod_plt_sec	core;
> diff --git a/arch/arm/include/asm/unwind.h b/arch/arm/include/asm/unwind.h
> index 0f8a3439902d..b51f85417f58 100644
> --- a/arch/arm/include/asm/unwind.h
> +++ b/arch/arm/include/asm/unwind.h
> @@ -24,6 +24,7 @@ struct unwind_idx {
>  
>  struct unwind_table {
>  	struct list_head list;
> +	struct list_head mod_list;
>  	const struct unwind_idx *start;
>  	const struct unwind_idx *origin;
>  	const struct unwind_idx *stop;
> diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
> index 549abcedf795..272f9bdeb1ed 100644
> --- a/arch/arm/kernel/module.c
> +++ b/arch/arm/kernel/module.c
> @@ -459,46 +459,36 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
>  #ifdef CONFIG_ARM_UNWIND
>  	const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
>  	const Elf_Shdr *sechdrs_end = sechdrs + hdr->e_shnum;
> -	struct mod_unwind_map maps[ARM_SEC_MAX];
> -	int i;
> +	struct unwind_table *table_list = &mod->arch.unwind_list;

Please rename "table_list" to "unwind_list" so we use a consistent name
for this.

>  
> -	memset(maps, 0, sizeof(maps));
> +	INIT_LIST_HEAD(&table_list->mod_list);
> +	mod->arch.init_table = NULL;
>  
>  	for (s = sechdrs; s < sechdrs_end; s++) {
> -		const char *secname = secstrs + s->sh_name;
> +		const unsigned int sectype = s->sh_type;

Please loose this local variable.

>  
>  		if (!(s->sh_flags & SHF_ALLOC))
>  			continue;
>  
> -		if (strcmp(".ARM.exidx.init.text", secname) == 0)
> -			maps[ARM_SEC_INIT].unw_sec = s;
> -		else if (strcmp(".ARM.exidx", secname) == 0)
> -			maps[ARM_SEC_CORE].unw_sec = s;
> -		else if (strcmp(".ARM.exidx.exit.text", secname) == 0)
> -			maps[ARM_SEC_EXIT].unw_sec = s;
> -		else if (strcmp(".ARM.exidx.text.unlikely", secname) == 0)
> -			maps[ARM_SEC_UNLIKELY].unw_sec = s;
> -		else if (strcmp(".ARM.exidx.text.hot", secname) == 0)
> -			maps[ARM_SEC_HOT].unw_sec = s;
> -		else if (strcmp(".init.text", secname) == 0)
> -			maps[ARM_SEC_INIT].txt_sec = s;
> -		else if (strcmp(".text", secname) == 0)
> -			maps[ARM_SEC_CORE].txt_sec = s;
> -		else if (strcmp(".exit.text", secname) == 0)
> -			maps[ARM_SEC_EXIT].txt_sec = s;
> -		else if (strcmp(".text.unlikely", secname) == 0)
> -			maps[ARM_SEC_UNLIKELY].txt_sec = s;
> -		else if (strcmp(".text.hot", secname) == 0)
> -			maps[ARM_SEC_HOT].txt_sec = s;
> +		if (sectype == ELF_SECTION_UNWIND) {

I think given the amouont of indentation this causes, that:

		if (!(s->sh_flags & SHF_ALLOC)) ||
		    s->sh_type != ELF_SECTION_UNWIND)
			continue;

would be preferred here.

Other than that, patch looks fine, thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!