| 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: <20181102135322.GA5289@linux-8ccs>
Date: Fri, 2 Nov 2018 14:53:22 +0100
From: Jessica Yu <jeyu@...nel.org>
To: Vincent Whitchurch <vincent.whitchurch@...s.com>
Cc: linux@...linux.org.uk, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org, live-patching@...r.kernel.org
Subject: Re: [PATCH v2] ARM: module: Fix function kallsyms on Thumb-2
+++ 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
Powered by blists - more mailing lists