[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

Thumb-2 functions have the lowest bit set in the symbol value in the
symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
generated from the output of nm, and nm clears the lowest bit.

 $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
  95947: 8015dc89   686 FUNC    GLOBAL DEFAULT    2 show_interrupts
 $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
 8015dc88 T show_interrupts
 $ cat /proc/kallsyms | grep show_interrupts
 8015dc88 T show_interrupts

However, for modules, the kallsyms uses the values in the symbol table
without modification, so for functions in modules, the lowest bit is set
in kallsyms.

 $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
    268: 000000e1    44 FUNC    GLOBAL DEFAULT    2 tun_get_socket
 $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
 000000e0 T tun_get_socket
 $ cat /proc/kallsyms | grep tun_get_socket
 7fcd30e1 t tun_get_socket  [tun]

Because of this, the offset of the crashing instruction shown in oopses
is incorrect when the crash is in a module.  For example, given a
tun_get_socket which starts like this,

 000000e0 <tun_get_socket>:
       e0:       b500            push    {lr}
       e2:       f7ff fffe       bl      0 <__gnu_mcount_nc>
       e6:       4b08            ldr     r3, [pc, #32]
       e8:       6942            ldr     r2, [r0, #20]
       ea:       429a            cmp     r2, r3
       ec:       d002            beq.n   f4 <tun_get_socket+0x14>

a crash when tun_get_socket is called with NULL results in:

 PC is at tun_get_socket+0x7/0x2c [tun]
 pc : [<7fcdb0e8>]

which can result in the incorrect line being reported by gdb if this
symbol+offset is used there.  If the crash is on the first instruction
of a function, the "PC is at" line would also report the symbol name of
the preceding function.

To solve this, fix up these symbols like nm does.  For this, we need a
new hook in the generic module loading code, before the symbols' st_info
is overwritten by add_kallsyms().  After the fix:

 $ cat /proc/kallsyms | grep tun_get_socket
 7fcd30e0 t tun_get_socket  [tun]

 PC is at tun_get_socket+0x8/0x2c [tun]
 pc : [<7fcdb0e8>]

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
v2: Fix build warning with !MODULES

 arch/arm/kernel/module.c     | 14 ++++++++++++++
 include/linux/moduleloader.h |  3 +++
 kernel/module.c              |  6 ++++++
 3 files changed, 23 insertions(+)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..771f86318d84 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -399,6 +399,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	return 0;
 }
 
+#if defined(CONFIG_THUMB2_KERNEL) && defined(CONFIG_KALLSYMS)
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
+{
+	int i;
+
+	for (i = 0; i < kallsyms->num_symtab; i++) {
+		Elf_Sym *sym = &kallsyms->symtab[i];
+
+		if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+			sym->st_value &= ~1;
+	}
+}
+#endif
+
 void
 module_arch_cleanup(struct module *mod)
 {
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..b1ac5584eaa5 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+struct mod_kallsyms;
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..ded4f4b49824 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2659,6 +2659,10 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 	mod->init_layout.size = debug_align(mod->init_layout.size);
 }
 
+void __weak module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
+{
+}
+
 /*
  * We use the full symtab and strtab which layout_symtab arranged to
  * be appended to the init section.  Later we switch to the cut-down
@@ -2680,6 +2684,8 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	/* Make sure we get permanent strtab: don't use info->strtab. */
 	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
 
+	module_fixup_kallsyms(mod->kallsyms);
+
 	/* Set types up while we still have access to sections. */
 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
 		mod->kallsyms->symtab[i].st_info
-- 
2.11.0

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

Thumb-2 functions have the lowest bit set in the symbol value in the
symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
generated from the output of nm, and nm clears the lowest bit.

 $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
  95947: 8015dc89   686 FUNC    GLOBAL DEFAULT    2 show_interrupts
 $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
 8015dc88 T show_interrupts
 $ cat /proc/kallsyms | grep show_interrupts
 8015dc88 T show_interrupts

However, for modules, the kallsyms uses the values in the symbol table
without modification, so for functions in modules, the lowest bit is set
in kallsyms.

 $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
    268: 000000e1    44 FUNC    GLOBAL DEFAULT    2 tun_get_socket
 $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
 000000e0 T tun_get_socket
 $ cat /proc/kallsyms | grep tun_get_socket
 7fcd30e1 t tun_get_socket  [tun]

Because of this, the offset of the crashing instruction shown in oopses
is incorrect when the crash is in a module.  For example, given a
tun_get_socket which starts like this,

 000000e0 <tun_get_socket>:
       e0:       b500            push    {lr}
       e2:       f7ff fffe       bl      0 <__gnu_mcount_nc>
       e6:       4b08            ldr     r3, [pc, #32]
       e8:       6942            ldr     r2, [r0, #20]
       ea:       429a            cmp     r2, r3
       ec:       d002            beq.n   f4 <tun_get_socket+0x14>

a crash when tun_get_socket is called with NULL results in:

 PC is at tun_get_socket+0x7/0x2c [tun]
 pc : [<7fcdb0e8>]

which can result in the incorrect line being reported by gdb if this
symbol+offset is used there.  If the crash is on the first instruction
of a function, the "PC is at" line would also report the symbol name of
the preceding function.

To solve this, fix up these symbols like nm does.  For this, we need a
new hook in the generic module loading code, before the symbols' st_info
is overwritten by add_kallsyms().  After the fix:

 $ cat /proc/kallsyms | grep tun_get_socket
 7fcd30e0 t tun_get_socket  [tun]

 PC is at tun_get_socket+0x8/0x2c [tun]
 pc : [<7fcdb0e8>]

Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
---
v2: Fix build warning with !MODULES

 arch/arm/kernel/module.c     | 14 ++++++++++++++
 include/linux/moduleloader.h |  3 +++
 kernel/module.c              |  6 ++++++
 3 files changed, 23 insertions(+)

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..771f86318d84 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -399,6 +399,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 	return 0;
 }
 
+#if defined(CONFIG_THUMB2_KERNEL) && defined(CONFIG_KALLSYMS)
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
+{
+	int i;
+
+	for (i = 0; i < kallsyms->num_symtab; i++) {
+		Elf_Sym *sym = &kallsyms->symtab[i];
+
+		if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+			sym->st_value &= ~1;
+	}
+}
+#endif
+
 void
 module_arch_cleanup(struct module *mod)
 {
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..b1ac5584eaa5 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+struct mod_kallsyms;
+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..ded4f4b49824 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2659,6 +2659,10 @@ static void layout_symtab(struct module *mod, struct load_info *info)
 	mod->init_layout.size = debug_align(mod->init_layout.size);
 }
 
+void __weak module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
+{
+}
+
 /*
  * We use the full symtab and strtab which layout_symtab arranged to
  * be appended to the init section.  Later we switch to the cut-down
@@ -2680,6 +2684,8 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 	/* Make sure we get permanent strtab: don't use info->strtab. */
 	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
 
+	module_fixup_kallsyms(mod->kallsyms);
+
 	/* Set types up while we still have access to sections. */
 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
 		mod->kallsyms->symtab[i].st_info
-- 
2.11.0

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [31/10/18 09:42 +0100]:
>Thumb-2 functions have the lowest bit set in the symbol value in the
>symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
>generated from the output of nm, and nm clears the lowest bit.
>
> $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
>  95947: 8015dc89   686 FUNC    GLOBAL DEFAULT    2 show_interrupts
> $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
> 8015dc88 T show_interrupts
> $ cat /proc/kallsyms | grep show_interrupts
> 8015dc88 T show_interrupts
>
>However, for modules, the kallsyms uses the values in the symbol table
>without modification, so for functions in modules, the lowest bit is set
>in kallsyms.
>
> $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
>    268: 000000e1    44 FUNC    GLOBAL DEFAULT    2 tun_get_socket
> $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
> 000000e0 T tun_get_socket
> $ cat /proc/kallsyms | grep tun_get_socket
> 7fcd30e1 t tun_get_socket  [tun]
>
>Because of this, the offset of the crashing instruction shown in oopses
>is incorrect when the crash is in a module.  For example, given a
>tun_get_socket which starts like this,
>
> 000000e0 <tun_get_socket>:
>       e0:       b500            push    {lr}
>       e2:       f7ff fffe       bl      0 <__gnu_mcount_nc>
>       e6:       4b08            ldr     r3, [pc, #32]
>       e8:       6942            ldr     r2, [r0, #20]
>       ea:       429a            cmp     r2, r3
>       ec:       d002            beq.n   f4 <tun_get_socket+0x14>
>
>a crash when tun_get_socket is called with NULL results in:
>
> PC is at tun_get_socket+0x7/0x2c [tun]
> pc : [<7fcdb0e8>]
>
>which can result in the incorrect line being reported by gdb if this
>symbol+offset is used there.  If the crash is on the first instruction
>of a function, the "PC is at" line would also report the symbol name of
>the preceding function.
>
>To solve this, fix up these symbols like nm does.  For this, we need a
>new hook in the generic module loading code, before the symbols' st_info
>is overwritten by add_kallsyms().  After the fix:
>
> $ cat /proc/kallsyms | grep tun_get_socket
> 7fcd30e0 t tun_get_socket  [tun]
>
> PC is at tun_get_socket+0x8/0x2c [tun]
> pc : [<7fcdb0e8>]
>
>Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>---
>v2: Fix build warning with !MODULES


Hi Vincent,

Just a few questions -

Could this be done in modpost? I'm guessing the answer is no as some
relocations may rely on that bit being set in st_value, right?
Therefore we can only clear the bit _after_ relocations to the module
are applied at runtime, correct?

I'm not against having an arch-specific kallsyms fixup function, my
only concern would be if this would interfere with the delayed
relocations livepatching does, if there are plans in the future to
have livepatching on arm (IIRC there was an attempt at a port in
2016). If there exists some Thumb-2 relocation types that rely on that
lowest bit in st_value being set in order to be applied, and we clear
it permanently from the symtab, then livepatching wouldn't be able to
apply those types of relocations anymore. If this is a legitimate
concern, then perhaps an alternative solution would be to have an
arch-specific kallsyms symbol-value-fixup function for accesses to
sym.st_value, without modifying the module symtab.


Thanks,

Jessica

> arch/arm/kernel/module.c     | 14 ++++++++++++++
> include/linux/moduleloader.h |  3 +++
> kernel/module.c              |  6 ++++++
> 3 files changed, 23 insertions(+)
>
>diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
>index 3ff571c2c71c..771f86318d84 100644
>--- a/arch/arm/kernel/module.c
>+++ b/arch/arm/kernel/module.c
>@@ -399,6 +399,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
> 	return 0;
> }
>
>+#if defined(CONFIG_THUMB2_KERNEL) && defined(CONFIG_KALLSYMS)
>+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
>+{
>+	int i;
>+
>+	for (i = 0; i < kallsyms->num_symtab; i++) {
>+		Elf_Sym *sym = &kallsyms->symtab[i];
>+
>+		if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
>+			sym->st_value &= ~1;
>+	}
>+}
>+#endif
>+
> void
> module_arch_cleanup(struct module *mod)
> {
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 31013c2effd3..b1ac5584eaa5 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
> /* Any cleanup before freeing mod->module_init */
> void module_arch_freeing_init(struct module *mod);
>
>+struct mod_kallsyms;
>+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms);
>+
> #ifdef CONFIG_KASAN
> #include <linux/kasan.h>
> #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
>diff --git a/kernel/module.c b/kernel/module.c
>index 49a405891587..ded4f4b49824 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2659,6 +2659,10 @@ static void layout_symtab(struct module *mod, struct load_info *info)
> 	mod->init_layout.size = debug_align(mod->init_layout.size);
> }
>
>+void __weak module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
>+{
>+}
>+
> /*
>  * We use the full symtab and strtab which layout_symtab arranged to
>  * be appended to the init section.  Later we switch to the cut-down
>@@ -2680,6 +2684,8 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> 	/* Make sure we get permanent strtab: don't use info->strtab. */
> 	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>
>+	module_fixup_kallsyms(mod->kallsyms);
>+
> 	/* Set types up while we still have access to sections. */
> 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
> 		mod->kallsyms->symtab[i].st_info
>-- 
>2.11.0
>

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [31/10/18 09:42 +0100]:
>Thumb-2 functions have the lowest bit set in the symbol value in the
>symtab.  When kallsyms are generated for the vmlinux, the kallsyms are
>generated from the output of nm, and nm clears the lowest bit.
>
> $ arm-linux-gnueabihf-readelf -a vmlinux | grep show_interrupts
>  95947: 8015dc89   686 FUNC    GLOBAL DEFAULT    2 show_interrupts
> $ arm-linux-gnueabihf-nm vmlinux | grep show_interrupts
> 8015dc88 T show_interrupts
> $ cat /proc/kallsyms | grep show_interrupts
> 8015dc88 T show_interrupts
>
>However, for modules, the kallsyms uses the values in the symbol table
>without modification, so for functions in modules, the lowest bit is set
>in kallsyms.
>
> $ arm-linux-gnueabihf-readelf -a drivers/net/tun.ko | grep tun_get_socket
>    268: 000000e1    44 FUNC    GLOBAL DEFAULT    2 tun_get_socket
> $ arm-linux-gnueabihf-nm drivers/net/tun.ko | grep tun_get_socket
> 000000e0 T tun_get_socket
> $ cat /proc/kallsyms | grep tun_get_socket
> 7fcd30e1 t tun_get_socket  [tun]
>
>Because of this, the offset of the crashing instruction shown in oopses
>is incorrect when the crash is in a module.  For example, given a
>tun_get_socket which starts like this,
>
> 000000e0 <tun_get_socket>:
>       e0:       b500            push    {lr}
>       e2:       f7ff fffe       bl      0 <__gnu_mcount_nc>
>       e6:       4b08            ldr     r3, [pc, #32]
>       e8:       6942            ldr     r2, [r0, #20]
>       ea:       429a            cmp     r2, r3
>       ec:       d002            beq.n   f4 <tun_get_socket+0x14>
>
>a crash when tun_get_socket is called with NULL results in:
>
> PC is at tun_get_socket+0x7/0x2c [tun]
> pc : [<7fcdb0e8>]
>
>which can result in the incorrect line being reported by gdb if this
>symbol+offset is used there.  If the crash is on the first instruction
>of a function, the "PC is at" line would also report the symbol name of
>the preceding function.
>
>To solve this, fix up these symbols like nm does.  For this, we need a
>new hook in the generic module loading code, before the symbols' st_info
>is overwritten by add_kallsyms().  After the fix:
>
> $ cat /proc/kallsyms | grep tun_get_socket
> 7fcd30e0 t tun_get_socket  [tun]
>
> PC is at tun_get_socket+0x8/0x2c [tun]
> pc : [<7fcdb0e8>]
>
>Signed-off-by: Vincent Whitchurch <vincent.whitchurch@axis.com>
>---
>v2: Fix build warning with !MODULES


Hi Vincent,

Just a few questions -

Could this be done in modpost? I'm guessing the answer is no as some
relocations may rely on that bit being set in st_value, right?
Therefore we can only clear the bit _after_ relocations to the module
are applied at runtime, correct?

I'm not against having an arch-specific kallsyms fixup function, my
only concern would be if this would interfere with the delayed
relocations livepatching does, if there are plans in the future to
have livepatching on arm (IIRC there was an attempt at a port in
2016). If there exists some Thumb-2 relocation types that rely on that
lowest bit in st_value being set in order to be applied, and we clear
it permanently from the symtab, then livepatching wouldn't be able to
apply those types of relocations anymore. If this is a legitimate
concern, then perhaps an alternative solution would be to have an
arch-specific kallsyms symbol-value-fixup function for accesses to
sym.st_value, without modifying the module symtab.


Thanks,

Jessica

> arch/arm/kernel/module.c     | 14 ++++++++++++++
> include/linux/moduleloader.h |  3 +++
> kernel/module.c              |  6 ++++++
> 3 files changed, 23 insertions(+)
>
>diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
>index 3ff571c2c71c..771f86318d84 100644
>--- a/arch/arm/kernel/module.c
>+++ b/arch/arm/kernel/module.c
>@@ -399,6 +399,20 @@ int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
> 	return 0;
> }
>
>+#if defined(CONFIG_THUMB2_KERNEL) && defined(CONFIG_KALLSYMS)
>+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
>+{
>+	int i;
>+
>+	for (i = 0; i < kallsyms->num_symtab; i++) {
>+		Elf_Sym *sym = &kallsyms->symtab[i];
>+
>+		if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
>+			sym->st_value &= ~1;
>+	}
>+}
>+#endif
>+
> void
> module_arch_cleanup(struct module *mod)
> {
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 31013c2effd3..b1ac5584eaa5 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -86,6 +86,9 @@ void module_arch_cleanup(struct module *mod);
> /* Any cleanup before freeing mod->module_init */
> void module_arch_freeing_init(struct module *mod);
>
>+struct mod_kallsyms;
>+void module_fixup_kallsyms(struct mod_kallsyms *kallsyms);
>+
> #ifdef CONFIG_KASAN
> #include <linux/kasan.h>
> #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
>diff --git a/kernel/module.c b/kernel/module.c
>index 49a405891587..ded4f4b49824 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2659,6 +2659,10 @@ static void layout_symtab(struct module *mod, struct load_info *info)
> 	mod->init_layout.size = debug_align(mod->init_layout.size);
> }
>
>+void __weak module_fixup_kallsyms(struct mod_kallsyms *kallsyms)
>+{
>+}
>+
> /*
>  * We use the full symtab and strtab which layout_symtab arranged to
>  * be appended to the init section.  Later we switch to the cut-down
>@@ -2680,6 +2684,8 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
> 	/* Make sure we get permanent strtab: don't use info->strtab. */
> 	mod->kallsyms->strtab = (void *)info->sechdrs[info->index.str].sh_addr;
>
>+	module_fixup_kallsyms(mod->kallsyms);
>+
> 	/* Set types up while we still have access to sections. */
> 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
> 		mod->kallsyms->symtab[i].st_info
>-- 
>2.11.0
>

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> Could this be done in modpost? I'm guessing the answer is no as some
> relocations may rely on that bit being set in st_value, right?
> Therefore we can only clear the bit _after_ relocations to the module
> are applied at runtime, correct?

Yes, that's correct, it needs to be done after the relocations.

> I'm not against having an arch-specific kallsyms fixup function, my
> only concern would be if this would interfere with the delayed
> relocations livepatching does, if there are plans in the future to
> have livepatching on arm (IIRC there was an attempt at a port in
> 2016). If there exists some Thumb-2 relocation types that rely on that
> lowest bit in st_value being set in order to be applied, and we clear
> it permanently from the symtab, then livepatching wouldn't be able to
> apply those types of relocations anymore. If this is a legitimate
> concern, then perhaps an alternative solution would be to have an
> arch-specific kallsyms symbol-value-fixup function for accesses to
> sym.st_value, without modifying the module symtab.

I'm not familiar with livepatching, but yes, if it needs to do
relocations later I guess we should preserve the original value.

I gave the alternative solution a go and it seems to work.
add_kallsyms() currently overwrites st_info so I had to move the
elf_type to the unused st_other field instead to preserve st_info:

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..f443d0ccd1a0 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -336,6 +336,16 @@ static const Elf_Shdr *find_mod_section(const Elf32_Ehdr *hdr,
 extern void fixup_pv_table(const void *, unsigned long);
 extern void fixup_smp(const void *, unsigned long);
 
+#ifdef CONFIG_THUMB2_KERNEL
+unsigned long symbol_value(Elf_Sym *sym)
+{
+	if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+		return sym->st_value & ~1;
+
+	return sym->st_value;
+}
+#endif
+
 int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 		    struct module *mod)
 {
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..6bf6118db37f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,8 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+unsigned long symbol_value(Elf_Sym *sym);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..871bf4450e9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 
 	/* Set types up while we still have access to sections. */
 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
-		mod->kallsyms->symtab[i].st_info
+		mod->kallsyms->symtab[i].st_other
 			= elf_type(&mod->kallsyms->symtab[i], info);
 
 	/* Now populate the cut down core kallsyms for after init. */
@@ -3916,6 +3916,11 @@ static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
 	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
 }
 
+unsigned long __weak symbol_value(Elf_Sym *sym)
+{
+	return sym->st_value;
+}
+
 static const char *get_ksymbol(struct module *mod,
 			       unsigned long addr,
 			       unsigned long *size,
@@ -3934,6 +3939,9 @@ static const char *get_ksymbol(struct module *mod,
 	/* Scan for closest preceding symbol, and next symbol. (ELF
 	   starts real symbols at 1). */
 	for (i = 1; i < kallsyms->num_symtab; i++) {
+		unsigned long thisval = symbol_value(&kallsyms->symtab[i]);
+		unsigned long bestval = symbol_value(&kallsyms->symtab[best]);
+
 		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
 			continue;
 
@@ -3943,21 +3951,21 @@ static const char *get_ksymbol(struct module *mod,
 		    || is_arm_mapping_symbol(symname(kallsyms, i)))
 			continue;
 
-		if (kallsyms->symtab[i].st_value <= addr
-		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
+		if (thisval <= addr
+		    && thisval > bestval)
 			best = i;
-		if (kallsyms->symtab[i].st_value > addr
-		    && kallsyms->symtab[i].st_value < nextval)
-			nextval = kallsyms->symtab[i].st_value;
+		if (thisval > addr
+		    && thisval < nextval)
+			nextval = thisval;
 	}
 
 	if (!best)
 		return NULL;
 
 	if (size)
-		*size = nextval - kallsyms->symtab[best].st_value;
+		*size = nextval - symbol_value(&kallsyms->symtab[best]);
 	if (offset)
-		*offset = addr - kallsyms->symtab[best].st_value;
+		*offset = addr - symbol_value(&kallsyms->symtab[best]);
 	return symname(kallsyms, best);
 }
 
@@ -4060,8 +4068,8 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 			continue;
 		kallsyms = rcu_dereference_sched(mod->kallsyms);
 		if (symnum < kallsyms->num_symtab) {
-			*value = kallsyms->symtab[symnum].st_value;
-			*type = kallsyms->symtab[symnum].st_info;
+			*value = symbol_value(&kallsyms->symtab[symnum]);
+			*type = kallsyms->symtab[symnum].st_other;
 			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
@@ -4082,7 +4090,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
 	for (i = 0; i < kallsyms->num_symtab; i++)
 		if (strcmp(name, symname(kallsyms, i)) == 0 &&
 		    kallsyms->symtab[i].st_shndx != SHN_UNDEF)
-			return kallsyms->symtab[i].st_value;
+			return symbol_value(&kallsyms->symtab[i]);
 	return 0;
 }
 
@@ -4132,7 +4140,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 				continue;
 
 			ret = fn(data, symname(kallsyms, i),
-				 mod, kallsyms->symtab[i].st_value);
+				 mod, symbol_value(&kallsyms->symtab[i]));
 			if (ret != 0)
 				return ret;
 		}

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> Could this be done in modpost? I'm guessing the answer is no as some
> relocations may rely on that bit being set in st_value, right?
> Therefore we can only clear the bit _after_ relocations to the module
> are applied at runtime, correct?

Yes, that's correct, it needs to be done after the relocations.

> I'm not against having an arch-specific kallsyms fixup function, my
> only concern would be if this would interfere with the delayed
> relocations livepatching does, if there are plans in the future to
> have livepatching on arm (IIRC there was an attempt at a port in
> 2016). If there exists some Thumb-2 relocation types that rely on that
> lowest bit in st_value being set in order to be applied, and we clear
> it permanently from the symtab, then livepatching wouldn't be able to
> apply those types of relocations anymore. If this is a legitimate
> concern, then perhaps an alternative solution would be to have an
> arch-specific kallsyms symbol-value-fixup function for accesses to
> sym.st_value, without modifying the module symtab.

I'm not familiar with livepatching, but yes, if it needs to do
relocations later I guess we should preserve the original value.

I gave the alternative solution a go and it seems to work.
add_kallsyms() currently overwrites st_info so I had to move the
elf_type to the unused st_other field instead to preserve st_info:

diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
index 3ff571c2c71c..f443d0ccd1a0 100644
--- a/arch/arm/kernel/module.c
+++ b/arch/arm/kernel/module.c
@@ -336,6 +336,16 @@ static const Elf_Shdr *find_mod_section(const Elf32_Ehdr *hdr,
 extern void fixup_pv_table(const void *, unsigned long);
 extern void fixup_smp(const void *, unsigned long);
 
+#ifdef CONFIG_THUMB2_KERNEL
+unsigned long symbol_value(Elf_Sym *sym)
+{
+	if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
+		return sym->st_value & ~1;
+
+	return sym->st_value;
+}
+#endif
+
 int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
 		    struct module *mod)
 {
diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
index 31013c2effd3..6bf6118db37f 100644
--- a/include/linux/moduleloader.h
+++ b/include/linux/moduleloader.h
@@ -86,6 +86,8 @@ void module_arch_cleanup(struct module *mod);
 /* Any cleanup before freeing mod->module_init */
 void module_arch_freeing_init(struct module *mod);
 
+unsigned long symbol_value(Elf_Sym *sym);
+
 #ifdef CONFIG_KASAN
 #include <linux/kasan.h>
 #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
diff --git a/kernel/module.c b/kernel/module.c
index 49a405891587..871bf4450e9d 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
 
 	/* Set types up while we still have access to sections. */
 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
-		mod->kallsyms->symtab[i].st_info
+		mod->kallsyms->symtab[i].st_other
 			= elf_type(&mod->kallsyms->symtab[i], info);
 
 	/* Now populate the cut down core kallsyms for after init. */
@@ -3916,6 +3916,11 @@ static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
 	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
 }
 
+unsigned long __weak symbol_value(Elf_Sym *sym)
+{
+	return sym->st_value;
+}
+
 static const char *get_ksymbol(struct module *mod,
 			       unsigned long addr,
 			       unsigned long *size,
@@ -3934,6 +3939,9 @@ static const char *get_ksymbol(struct module *mod,
 	/* Scan for closest preceding symbol, and next symbol. (ELF
 	   starts real symbols at 1). */
 	for (i = 1; i < kallsyms->num_symtab; i++) {
+		unsigned long thisval = symbol_value(&kallsyms->symtab[i]);
+		unsigned long bestval = symbol_value(&kallsyms->symtab[best]);
+
 		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
 			continue;
 
@@ -3943,21 +3951,21 @@ static const char *get_ksymbol(struct module *mod,
 		    || is_arm_mapping_symbol(symname(kallsyms, i)))
 			continue;
 
-		if (kallsyms->symtab[i].st_value <= addr
-		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
+		if (thisval <= addr
+		    && thisval > bestval)
 			best = i;
-		if (kallsyms->symtab[i].st_value > addr
-		    && kallsyms->symtab[i].st_value < nextval)
-			nextval = kallsyms->symtab[i].st_value;
+		if (thisval > addr
+		    && thisval < nextval)
+			nextval = thisval;
 	}
 
 	if (!best)
 		return NULL;
 
 	if (size)
-		*size = nextval - kallsyms->symtab[best].st_value;
+		*size = nextval - symbol_value(&kallsyms->symtab[best]);
 	if (offset)
-		*offset = addr - kallsyms->symtab[best].st_value;
+		*offset = addr - symbol_value(&kallsyms->symtab[best]);
 	return symname(kallsyms, best);
 }
 
@@ -4060,8 +4068,8 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
 			continue;
 		kallsyms = rcu_dereference_sched(mod->kallsyms);
 		if (symnum < kallsyms->num_symtab) {
-			*value = kallsyms->symtab[symnum].st_value;
-			*type = kallsyms->symtab[symnum].st_info;
+			*value = symbol_value(&kallsyms->symtab[symnum]);
+			*type = kallsyms->symtab[symnum].st_other;
 			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
 			*exported = is_exported(name, *value, mod);
@@ -4082,7 +4090,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
 	for (i = 0; i < kallsyms->num_symtab; i++)
 		if (strcmp(name, symname(kallsyms, i)) == 0 &&
 		    kallsyms->symtab[i].st_shndx != SHN_UNDEF)
-			return kallsyms->symtab[i].st_value;
+			return symbol_value(&kallsyms->symtab[i]);
 	return 0;
 }
 
@@ -4132,7 +4140,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
 				continue;
 
 			ret = fn(data, symname(kallsyms, i),
-				 mod, kallsyms->symtab[i].st_value);
+				 mod, symbol_value(&kallsyms->symtab[i]));
 			if (ret != 0)
 				return ret;
 		}

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [01/11/18 16:29 +0100]:
>On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
>> Could this be done in modpost? I'm guessing the answer is no as some
>> relocations may rely on that bit being set in st_value, right?
>> Therefore we can only clear the bit _after_ relocations to the module
>> are applied at runtime, correct?
>
>Yes, that's correct, it needs to be done after the relocations.
>
>> I'm not against having an arch-specific kallsyms fixup function, my
>> only concern would be if this would interfere with the delayed
>> relocations livepatching does, if there are plans in the future to
>> have livepatching on arm (IIRC there was an attempt at a port in
>> 2016). If there exists some Thumb-2 relocation types that rely on that
>> lowest bit in st_value being set in order to be applied, and we clear
>> it permanently from the symtab, then livepatching wouldn't be able to
>> apply those types of relocations anymore. If this is a legitimate
>> concern, then perhaps an alternative solution would be to have an
>> arch-specific kallsyms symbol-value-fixup function for accesses to
>> sym.st_value, without modifying the module symtab.
>
>I'm not familiar with livepatching, but yes, if it needs to do
>relocations later I guess we should preserve the original value.

Yeah, I think the symtab needs to remain unmodified for livepatch to
be able to do delayed relocations after module load.

>I gave the alternative solution a go and it seems to work.
>add_kallsyms() currently overwrites st_info so I had to move the
>elf_type to the unused st_other field instead to preserve st_info:

I think that's fine, I think the only usages of st_other I've found
are during relocations, and the field is big enough for a char, so it
should be fine to reuse it afterwards.

>diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
>index 3ff571c2c71c..f443d0ccd1a0 100644
>--- a/arch/arm/kernel/module.c
>+++ b/arch/arm/kernel/module.c
>@@ -336,6 +336,16 @@ static const Elf_Shdr *find_mod_section(const Elf32_Ehdr *hdr,
> extern void fixup_pv_table(const void *, unsigned long);
> extern void fixup_smp(const void *, unsigned long);
>
>+#ifdef CONFIG_THUMB2_KERNEL
>+unsigned long symbol_value(Elf_Sym *sym)
>+{
>+	if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
>+		return sym->st_value & ~1;
>+
>+	return sym->st_value;
>+}
>+#endif
>+
> int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
> 		    struct module *mod)
> {
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 31013c2effd3..6bf6118db37f 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -86,6 +86,8 @@ void module_arch_cleanup(struct module *mod);
> /* Any cleanup before freeing mod->module_init */
> void module_arch_freeing_init(struct module *mod);
>
>+unsigned long symbol_value(Elf_Sym *sym);
>+
> #ifdef CONFIG_KASAN
> #include <linux/kasan.h>
> #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
>diff --git a/kernel/module.c b/kernel/module.c
>index 49a405891587..871bf4450e9d 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>
> 	/* Set types up while we still have access to sections. */
> 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
>-		mod->kallsyms->symtab[i].st_info
>+		mod->kallsyms->symtab[i].st_other
> 			= elf_type(&mod->kallsyms->symtab[i], info);
>
> 	/* Now populate the cut down core kallsyms for after init. */
>@@ -3916,6 +3916,11 @@ static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
> 	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> }
>
>+unsigned long __weak symbol_value(Elf_Sym *sym)
>+{
>+	return sym->st_value;
>+}
>+
> static const char *get_ksymbol(struct module *mod,
> 			       unsigned long addr,
> 			       unsigned long *size,
>@@ -3934,6 +3939,9 @@ static const char *get_ksymbol(struct module *mod,
> 	/* Scan for closest preceding symbol, and next symbol. (ELF
> 	   starts real symbols at 1). */
> 	for (i = 1; i < kallsyms->num_symtab; i++) {
>+		unsigned long thisval = symbol_value(&kallsyms->symtab[i]);
>+		unsigned long bestval = symbol_value(&kallsyms->symtab[best]);
>+
> 		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
> 			continue;
>
>@@ -3943,21 +3951,21 @@ static const char *get_ksymbol(struct module *mod,
> 		    || is_arm_mapping_symbol(symname(kallsyms, i)))
> 			continue;
>
>-		if (kallsyms->symtab[i].st_value <= addr
>-		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
>+		if (thisval <= addr
>+		    && thisval > bestval)
> 			best = i;
>-		if (kallsyms->symtab[i].st_value > addr
>-		    && kallsyms->symtab[i].st_value < nextval)
>-			nextval = kallsyms->symtab[i].st_value;
>+		if (thisval > addr
>+		    && thisval < nextval)
>+			nextval = thisval;
> 	}
>
> 	if (!best)
> 		return NULL;
>
> 	if (size)
>-		*size = nextval - kallsyms->symtab[best].st_value;
>+		*size = nextval - symbol_value(&kallsyms->symtab[best]);
> 	if (offset)
>-		*offset = addr - kallsyms->symtab[best].st_value;
>+		*offset = addr - symbol_value(&kallsyms->symtab[best]);
> 	return symname(kallsyms, best);
> }
>
>@@ -4060,8 +4068,8 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 			continue;
> 		kallsyms = rcu_dereference_sched(mod->kallsyms);
> 		if (symnum < kallsyms->num_symtab) {
>-			*value = kallsyms->symtab[symnum].st_value;
>-			*type = kallsyms->symtab[symnum].st_info;
>+			*value = symbol_value(&kallsyms->symtab[symnum]);
>+			*type = kallsyms->symtab[symnum].st_other;
> 			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
> 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
> 			*exported = is_exported(name, *value, mod);
>@@ -4082,7 +4090,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
> 	for (i = 0; i < kallsyms->num_symtab; i++)
> 		if (strcmp(name, symname(kallsyms, i)) == 0 &&
> 		    kallsyms->symtab[i].st_shndx != SHN_UNDEF)
>-			return kallsyms->symtab[i].st_value;
>+			return symbol_value(&kallsyms->symtab[i]);
> 	return 0;
> }
>
>@@ -4132,7 +4140,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> 				continue;
>
> 			ret = fn(data, symname(kallsyms, i),
>-				 mod, kallsyms->symtab[i].st_value);
>+				 mod, symbol_value(&kallsyms->symtab[i]));
> 			if (ret != 0)
> 				return ret;
> 		}

This looks pretty good, could you submit it as a real patch? My only
suggestion is to maybe rename symbol_value() to something more
specific, so we could distinguish it from kernel_symbol_value() and
kernel_symbol_name(), which are used for exported syms that operate on
struct kernel_symbol. Maybe kallsyms_symbol_value() or
module_kallsyms_symbol_value()? (That reminds me, I really should
clean up all the kallsyms/exported symbol code in module.c, the naming
scheme is a confusing mess :-/)

Thanks!

Jessica

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [01/11/18 16:29 +0100]:
>On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
>> Could this be done in modpost? I'm guessing the answer is no as some
>> relocations may rely on that bit being set in st_value, right?
>> Therefore we can only clear the bit _after_ relocations to the module
>> are applied at runtime, correct?
>
>Yes, that's correct, it needs to be done after the relocations.
>
>> I'm not against having an arch-specific kallsyms fixup function, my
>> only concern would be if this would interfere with the delayed
>> relocations livepatching does, if there are plans in the future to
>> have livepatching on arm (IIRC there was an attempt at a port in
>> 2016). If there exists some Thumb-2 relocation types that rely on that
>> lowest bit in st_value being set in order to be applied, and we clear
>> it permanently from the symtab, then livepatching wouldn't be able to
>> apply those types of relocations anymore. If this is a legitimate
>> concern, then perhaps an alternative solution would be to have an
>> arch-specific kallsyms symbol-value-fixup function for accesses to
>> sym.st_value, without modifying the module symtab.
>
>I'm not familiar with livepatching, but yes, if it needs to do
>relocations later I guess we should preserve the original value.

Yeah, I think the symtab needs to remain unmodified for livepatch to
be able to do delayed relocations after module load.

>I gave the alternative solution a go and it seems to work.
>add_kallsyms() currently overwrites st_info so I had to move the
>elf_type to the unused st_other field instead to preserve st_info:

I think that's fine, I think the only usages of st_other I've found
are during relocations, and the field is big enough for a char, so it
should be fine to reuse it afterwards.

>diff --git a/arch/arm/kernel/module.c b/arch/arm/kernel/module.c
>index 3ff571c2c71c..f443d0ccd1a0 100644
>--- a/arch/arm/kernel/module.c
>+++ b/arch/arm/kernel/module.c
>@@ -336,6 +336,16 @@ static const Elf_Shdr *find_mod_section(const Elf32_Ehdr *hdr,
> extern void fixup_pv_table(const void *, unsigned long);
> extern void fixup_smp(const void *, unsigned long);
>
>+#ifdef CONFIG_THUMB2_KERNEL
>+unsigned long symbol_value(Elf_Sym *sym)
>+{
>+	if (ELF_ST_TYPE(sym->st_info) == STT_FUNC)
>+		return sym->st_value & ~1;
>+
>+	return sym->st_value;
>+}
>+#endif
>+
> int module_finalize(const Elf32_Ehdr *hdr, const Elf_Shdr *sechdrs,
> 		    struct module *mod)
> {
>diff --git a/include/linux/moduleloader.h b/include/linux/moduleloader.h
>index 31013c2effd3..6bf6118db37f 100644
>--- a/include/linux/moduleloader.h
>+++ b/include/linux/moduleloader.h
>@@ -86,6 +86,8 @@ void module_arch_cleanup(struct module *mod);
> /* Any cleanup before freeing mod->module_init */
> void module_arch_freeing_init(struct module *mod);
>
>+unsigned long symbol_value(Elf_Sym *sym);
>+
> #ifdef CONFIG_KASAN
> #include <linux/kasan.h>
> #define MODULE_ALIGN (PAGE_SIZE << KASAN_SHADOW_SCALE_SHIFT)
>diff --git a/kernel/module.c b/kernel/module.c
>index 49a405891587..871bf4450e9d 100644
>--- a/kernel/module.c
>+++ b/kernel/module.c
>@@ -2682,7 +2682,7 @@ static void add_kallsyms(struct module *mod, const struct load_info *info)
>
> 	/* Set types up while we still have access to sections. */
> 	for (i = 0; i < mod->kallsyms->num_symtab; i++)
>-		mod->kallsyms->symtab[i].st_info
>+		mod->kallsyms->symtab[i].st_other
> 			= elf_type(&mod->kallsyms->symtab[i], info);
>
> 	/* Now populate the cut down core kallsyms for after init. */
>@@ -3916,6 +3916,11 @@ static const char *symname(struct mod_kallsyms *kallsyms, unsigned int symnum)
> 	return kallsyms->strtab + kallsyms->symtab[symnum].st_name;
> }
>
>+unsigned long __weak symbol_value(Elf_Sym *sym)
>+{
>+	return sym->st_value;
>+}
>+
> static const char *get_ksymbol(struct module *mod,
> 			       unsigned long addr,
> 			       unsigned long *size,
>@@ -3934,6 +3939,9 @@ static const char *get_ksymbol(struct module *mod,
> 	/* Scan for closest preceding symbol, and next symbol. (ELF
> 	   starts real symbols at 1). */
> 	for (i = 1; i < kallsyms->num_symtab; i++) {
>+		unsigned long thisval = symbol_value(&kallsyms->symtab[i]);
>+		unsigned long bestval = symbol_value(&kallsyms->symtab[best]);
>+
> 		if (kallsyms->symtab[i].st_shndx == SHN_UNDEF)
> 			continue;
>
>@@ -3943,21 +3951,21 @@ static const char *get_ksymbol(struct module *mod,
> 		    || is_arm_mapping_symbol(symname(kallsyms, i)))
> 			continue;
>
>-		if (kallsyms->symtab[i].st_value <= addr
>-		    && kallsyms->symtab[i].st_value > kallsyms->symtab[best].st_value)
>+		if (thisval <= addr
>+		    && thisval > bestval)
> 			best = i;
>-		if (kallsyms->symtab[i].st_value > addr
>-		    && kallsyms->symtab[i].st_value < nextval)
>-			nextval = kallsyms->symtab[i].st_value;
>+		if (thisval > addr
>+		    && thisval < nextval)
>+			nextval = thisval;
> 	}
>
> 	if (!best)
> 		return NULL;
>
> 	if (size)
>-		*size = nextval - kallsyms->symtab[best].st_value;
>+		*size = nextval - symbol_value(&kallsyms->symtab[best]);
> 	if (offset)
>-		*offset = addr - kallsyms->symtab[best].st_value;
>+		*offset = addr - symbol_value(&kallsyms->symtab[best]);
> 	return symname(kallsyms, best);
> }
>
>@@ -4060,8 +4068,8 @@ int module_get_kallsym(unsigned int symnum, unsigned long *value, char *type,
> 			continue;
> 		kallsyms = rcu_dereference_sched(mod->kallsyms);
> 		if (symnum < kallsyms->num_symtab) {
>-			*value = kallsyms->symtab[symnum].st_value;
>-			*type = kallsyms->symtab[symnum].st_info;
>+			*value = symbol_value(&kallsyms->symtab[symnum]);
>+			*type = kallsyms->symtab[symnum].st_other;
> 			strlcpy(name, symname(kallsyms, symnum), KSYM_NAME_LEN);
> 			strlcpy(module_name, mod->name, MODULE_NAME_LEN);
> 			*exported = is_exported(name, *value, mod);
>@@ -4082,7 +4090,7 @@ static unsigned long mod_find_symname(struct module *mod, const char *name)
> 	for (i = 0; i < kallsyms->num_symtab; i++)
> 		if (strcmp(name, symname(kallsyms, i)) == 0 &&
> 		    kallsyms->symtab[i].st_shndx != SHN_UNDEF)
>-			return kallsyms->symtab[i].st_value;
>+			return symbol_value(&kallsyms->symtab[i]);
> 	return 0;
> }
>
>@@ -4132,7 +4140,7 @@ int module_kallsyms_on_each_symbol(int (*fn)(void *, const char *,
> 				continue;
>
> 			ret = fn(data, symname(kallsyms, i),
>-				 mod, kallsyms->symtab[i].st_value);
>+				 mod, symbol_value(&kallsyms->symtab[i]));
> 			if (ret != 0)
> 				return ret;
> 		}

This looks pretty good, could you submit it as a real patch? My only
suggestion is to maybe rename symbol_value() to something more
specific, so we could distinguish it from kernel_symbol_value() and
kernel_symbol_name(), which are used for exported syms that operate on
struct kernel_symbol. Maybe kallsyms_symbol_value() or
module_kallsyms_symbol_value()? (That reminds me, I really should
clean up all the kallsyms/exported symbol code in module.c, the naming
scheme is a confusing mess :-/)

Thanks!

Jessica

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:
> +++ Vincent Whitchurch [01/11/18 16:29 +0100]:
> > On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> > > Could this be done in modpost? I'm guessing the answer is no as some
> > > relocations may rely on that bit being set in st_value, right?
> > > Therefore we can only clear the bit _after_ relocations to the module
> > > are applied at runtime, correct?
> > 
> > Yes, that's correct, it needs to be done after the relocations.
> > 
> > > I'm not against having an arch-specific kallsyms fixup function, my
> > > only concern would be if this would interfere with the delayed
> > > relocations livepatching does, if there are plans in the future to
> > > have livepatching on arm (IIRC there was an attempt at a port in
> > > 2016). If there exists some Thumb-2 relocation types that rely on that
> > > lowest bit in st_value being set in order to be applied, and we clear
> > > it permanently from the symtab, then livepatching wouldn't be able to
> > > apply those types of relocations anymore. If this is a legitimate
> > > concern, then perhaps an alternative solution would be to have an
> > > arch-specific kallsyms symbol-value-fixup function for accesses to
> > > sym.st_value, without modifying the module symtab.
> > 
> > I'm not familiar with livepatching, but yes, if it needs to do
> > relocations later I guess we should preserve the original value.
> 
> Yeah, I think the symtab needs to remain unmodified for livepatch to
> be able to do delayed relocations after module load.
> 
> > I gave the alternative solution a go and it seems to work.
> > add_kallsyms() currently overwrites st_info so I had to move the
> > elf_type to the unused st_other field instead to preserve st_info:
> 
> I think that's fine, I think the only usages of st_other I've found
> are during relocations, and the field is big enough for a char, so it
> should be fine to reuse it afterwards.

If livepatch can do relocations later, doesn't that mean that we _can't_
reuse st_other for storing the elf_type?  OTOH, the current code
overwrites st_info, and I see that used from various archs' relocation
functions too, so perhaps I'm missing something.

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Vincent Whitchurch]

On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:
> +++ Vincent Whitchurch [01/11/18 16:29 +0100]:
> > On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
> > > Could this be done in modpost? I'm guessing the answer is no as some
> > > relocations may rely on that bit being set in st_value, right?
> > > Therefore we can only clear the bit _after_ relocations to the module
> > > are applied at runtime, correct?
> > 
> > Yes, that's correct, it needs to be done after the relocations.
> > 
> > > I'm not against having an arch-specific kallsyms fixup function, my
> > > only concern would be if this would interfere with the delayed
> > > relocations livepatching does, if there are plans in the future to
> > > have livepatching on arm (IIRC there was an attempt at a port in
> > > 2016). If there exists some Thumb-2 relocation types that rely on that
> > > lowest bit in st_value being set in order to be applied, and we clear
> > > it permanently from the symtab, then livepatching wouldn't be able to
> > > apply those types of relocations anymore. If this is a legitimate
> > > concern, then perhaps an alternative solution would be to have an
> > > arch-specific kallsyms symbol-value-fixup function for accesses to
> > > sym.st_value, without modifying the module symtab.
> > 
> > I'm not familiar with livepatching, but yes, if it needs to do
> > relocations later I guess we should preserve the original value.
> 
> Yeah, I think the symtab needs to remain unmodified for livepatch to
> be able to do delayed relocations after module load.
> 
> > I gave the alternative solution a go and it seems to work.
> > add_kallsyms() currently overwrites st_info so I had to move the
> > elf_type to the unused st_other field instead to preserve st_info:
> 
> I think that's fine, I think the only usages of st_other I've found
> are during relocations, and the field is big enough for a char, so it
> should be fine to reuse it afterwards.

If livepatch can do relocations later, doesn't that mean that we _can't_
reuse st_other for storing the elf_type?  OTOH, the current code
overwrites st_info, and I see that used from various archs' relocation
functions too, so perhaps I'm missing something.

Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [09/11/18 14:53 +0100]:
>On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:
>> +++ Vincent Whitchurch [01/11/18 16:29 +0100]:
>> > On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
>> > > Could this be done in modpost? I'm guessing the answer is no as some
>> > > relocations may rely on that bit being set in st_value, right?
>> > > Therefore we can only clear the bit _after_ relocations to the module
>> > > are applied at runtime, correct?
>> >
>> > Yes, that's correct, it needs to be done after the relocations.
>> >
>> > > I'm not against having an arch-specific kallsyms fixup function, my
>> > > only concern would be if this would interfere with the delayed
>> > > relocations livepatching does, if there are plans in the future to
>> > > have livepatching on arm (IIRC there was an attempt at a port in
>> > > 2016). If there exists some Thumb-2 relocation types that rely on that
>> > > lowest bit in st_value being set in order to be applied, and we clear
>> > > it permanently from the symtab, then livepatching wouldn't be able to
>> > > apply those types of relocations anymore. If this is a legitimate
>> > > concern, then perhaps an alternative solution would be to have an
>> > > arch-specific kallsyms symbol-value-fixup function for accesses to
>> > > sym.st_value, without modifying the module symtab.
>> >
>> > I'm not familiar with livepatching, but yes, if it needs to do
>> > relocations later I guess we should preserve the original value.
>>
>> Yeah, I think the symtab needs to remain unmodified for livepatch to
>> be able to do delayed relocations after module load.
>>
>> > I gave the alternative solution a go and it seems to work.
>> > add_kallsyms() currently overwrites st_info so I had to move the
>> > elf_type to the unused st_other field instead to preserve st_info:
>>
>> I think that's fine, I think the only usages of st_other I've found
>> are during relocations, and the field is big enough for a char, so it
>> should be fine to reuse it afterwards.
>
>If livepatch can do relocations later, doesn't that mean that we _can't_
>reuse st_other for storing the elf_type?  OTOH, the current code
>overwrites st_info, and I see that used from various archs' relocation
>functions too, so perhaps I'm missing something.

D'oh, I just contradicted myself. Yes, you are correct. AFAIK the only
reason we haven't run into issues yet in livepatch is because the only
arches it currently supports (x86, s390, powerpc) don't depend on
st_info for module relocations. But yes, that's an outstanding problem
that will need to be fixed if livepatch gains support on more arches,
we shouldn't be overwriting that field. And after grepping around I do
see that st_other is used in powerpc, alpha, sh for module
relocations. I suggested in my other reply to your v3 patch that
reusing st_size instead of st_info might work, since it's not used in
the module loader/kallsyms, and arches don't seem to use it for module
relocs. Maybe that would work?

Jessica

[PATCH v2] ARM: module: Fix function kallsyms on Thumb-2

[Jessica Yu]

+++ Vincent Whitchurch [09/11/18 14:53 +0100]:
>On Fri, Nov 02, 2018 at 02:53:22PM +0100, Jessica Yu wrote:
>> +++ Vincent Whitchurch [01/11/18 16:29 +0100]:
>> > On Wed, Oct 31, 2018 at 04:53:41PM +0100, Jessica Yu wrote:
>> > > Could this be done in modpost? I'm guessing the answer is no as some
>> > > relocations may rely on that bit being set in st_value, right?
>> > > Therefore we can only clear the bit _after_ relocations to the module
>> > > are applied at runtime, correct?
>> >
>> > Yes, that's correct, it needs to be done after the relocations.
>> >
>> > > I'm not against having an arch-specific kallsyms fixup function, my
>> > > only concern would be if this would interfere with the delayed
>> > > relocations livepatching does, if there are plans in the future to
>> > > have livepatching on arm (IIRC there was an attempt at a port in
>> > > 2016). If there exists some Thumb-2 relocation types that rely on that
>> > > lowest bit in st_value being set in order to be applied, and we clear
>> > > it permanently from the symtab, then livepatching wouldn't be able to
>> > > apply those types of relocations anymore. If this is a legitimate
>> > > concern, then perhaps an alternative solution would be to have an
>> > > arch-specific kallsyms symbol-value-fixup function for accesses to
>> > > sym.st_value, without modifying the module symtab.
>> >
>> > I'm not familiar with livepatching, but yes, if it needs to do
>> > relocations later I guess we should preserve the original value.
>>
>> Yeah, I think the symtab needs to remain unmodified for livepatch to
>> be able to do delayed relocations after module load.
>>
>> > I gave the alternative solution a go and it seems to work.
>> > add_kallsyms() currently overwrites st_info so I had to move the
>> > elf_type to the unused st_other field instead to preserve st_info:
>>
>> I think that's fine, I think the only usages of st_other I've found
>> are during relocations, and the field is big enough for a char, so it
>> should be fine to reuse it afterwards.
>
>If livepatch can do relocations later, doesn't that mean that we _can't_
>reuse st_other for storing the elf_type?  OTOH, the current code
>overwrites st_info, and I see that used from various archs' relocation
>functions too, so perhaps I'm missing something.

D'oh, I just contradicted myself. Yes, you are correct. AFAIK the only
reason we haven't run into issues yet in livepatch is because the only
arches it currently supports (x86, s390, powerpc) don't depend on
st_info for module relocations. But yes, that's an outstanding problem
that will need to be fixed if livepatch gains support on more arches,
we shouldn't be overwriting that field. And after grepping around I do
see that st_other is used in powerpc, alpha, sh for module
relocations. I suggested in my other reply to your v3 patch that
reusing st_size instead of st_info might work, since it's not used in
the module loader/kallsyms, and arches don't seem to use it for module
relocs. Maybe that would work?

Jessica