| 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: <bcedf943-6c94-5d23-a637-4f29daf21eda@suse.de>
Date: Wed, 24 Apr 2019 16:13:59 -0300
From: Joao Moreira <jmoreira@...e.de>
To: Miroslav Benes <mbenes@...e.cz>,
Joe Lawrence <joe.lawrence@...hat.com>
Cc: linux-kernel@...r.kernel.org, live-patching@...r.kernel.org,
linux-kbuild@...r.kernel.org, Jessica Yu <jeyu@...nel.org>,
Jiri Kosina <jikos@...nel.org>,
Josh Poimboeuf <jpoimboe@...hat.com>,
Konstantin Khlebnikov <khlebnikov@...dex-team.ru>,
Masahiro Yamada <yamada.masahiro@...ionext.com>,
Michael Matz <matz@...e.de>, Nicolai Stange <nstange@...e.de>,
Petr Mladek <pmladek@...e.com>
Subject: Re: [PATCH v3 0/9] klp-convert livepatch build tooling
On 4/24/19 3:19 PM, Miroslav Benes wrote:
> [...]
>
>> Result: a small tweak to sympos_sanity_check() to relax its symbol
>> uniqueness verification: allow for duplicate <object, name, position>
>> instances. Now it will only complain when a supplied symbol references
>> the same <object, name> but a different <position>.
>>
>> diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
>> index 82c27d219372..713835dfc9ec 100644
>> --- a/scripts/livepatch/klp-convert.c
>> +++ b/scripts/livepatch/klp-convert.c
>> @@ -194,7 +194,10 @@ static void clear_sympos_annontations(struct elf *klp_elf)
>> }
>> }
>>
>> -/* Checks if two or more elements in usr_symbols have the same name */
>> +/*
>> + * Checks if two or more elements in usr_symbols have the same
>> + * object and name, but different symbol position
>> + */
>> static bool sympos_sanity_check(void)
>> {
>> bool sane = true;
>> @@ -203,7 +206,9 @@ static bool sympos_sanity_check(void)
>> list_for_each_entry(sp, &usr_symbols, list) {
>> aux = list_next_entry(sp, list);
>> list_for_each_entry_from(aux, &usr_symbols, list) {
>> - if (strcmp(sp->symbol_name, aux->symbol_name) == 0) {
>> + if (sp->pos != aux->pos &&
>> + strcmp(sp->object_name, aux->object_name) == 0 &&
>> + strcmp(sp->symbol_name, aux->symbol_name) == 0)
>> WARN("Conflicting KLP_SYMPOS definition: %s.%s,%d vs. %s.%s,%d.",
>> sp->object_name, sp->symbol_name, sp->pos,
>> aux->object_name, aux->symbol_name, aux->pos);
>
> Looks good and I'd definitely include it in v4.
>
>> Unique sympos
>> -------------
>>
>> But even with the above workaround, specifying unique symbol positions
>> will (and should) result in a klp-convert complaint.
>>
>> When modifying the test module with unique symbol position annotation
>> values (1 and 2 respectively):
>>
>> test_klp_convert_multi_objs_a.c:
>>
>> extern void *state_show;
>> ...
>> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
>> KLP_SYMPOS(state_show, 1)
>> };
>>
>> test_klp_convert_multi_objs_b.c:
>>
>> extern void *state_show;
>> ...
>> KLP_MODULE_RELOC(vmlinux) vmlinux_relocs_b[] = {
>> KLP_SYMPOS(state_show, 2)
>> };
>>
>>
>> Each object file's symbol table contains a "state_show" instance:
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_a.o | grep '\<state_show\>'
>> 30: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs_b.o | grep '\<state_show\>'
>> 20: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> and the intermediate test_klp_convert_multi_objs.klp.o file contains a
>> combined .klp.module_relocs.vmlinux relocation section with two
>> KLP_SYMPOS structures, each with a unique <sympos> value:
>>
>> % objcopy -O binary --only-section=.klp.module_relocs.vmlinux \
>> lib/livepatch/test_klp_convert_multi_objs.klp.o >(hexdump)
>>
>> 0000000 0000 0000 0000 0000 0001 0000 0000 0000
>> 0000010 0000 0000 0002 0000
>>
>> but the symbol tables were merged, sorted and non-unique symbols
>> removed, leaving a *single* unresolved "state_show" instance:
>>
>> % eu-readelf --symbols lib/livepatch/test_klp_convert_multi_objs.klp.o | grep '\<state_show\>'
>> 53: 0000000000000000 0 NOTYPE GLOBAL DEFAULT UNDEF state_show
>>
>> which means that even though we have separate relocations for each
>> "state_show" instance:
>>
>> Relocation section [ 6] '.rela.text.unlikely' for section [ 5] '.text.unlikely' at offset 0x44510 contains 11 entries:
>> Offset Type Value Addend Name
>> 0x0000000000000003 X86_64_32S 000000000000000000 +0 state_show
>> ...
>> 0x0000000000000034 X86_64_32S 000000000000000000 +0 state_show
>> ...
>>
>> they share the same rela->sym and there is no way to distinguish which
>> one correlates to which KLP_SYMPOS structure.
>>
>>
>> Future Work
>> -----------
>>
>> I don't see an easy way to support multiple homonym <object, name>
>> symbols with unique <position> values in the current livepatch module
>> Elf format. The only solutions that come to mind right now include
>> renaming homonym symbols somehow to retain the relocation->symbol
>> relationship when separate object files are combined. Perhaps an
>> intermediate linker step could make annotated symbols unique in some way
>> to achieve this. /thinking out loud
>
> I'd set this aside for now and we can return to it later. I think it could
> be quite rare in practice.
>
> I was thinking about renaming the symbol too. We can extend the symbol
> naming convention we have now and deal with it in klp_resolve_symbols(),
> but maybe Josh will come up with something clever and cleaner.
I think this could work well, but (sorry if I understood Joe's idea
wrongly) not as a linker step. Instead of modifying the linker, I think
we could create another tool and plug it into the kbuild pipeline prior
to the livepatch module linking. This way, we would parse the .o elf
files, check for homonyms and rename them based on a convention that is
later understood by klp-convert, as suggested.
If I am not missing something, this would fix the case where we have
homonyms pointing to the same or different positions, without additional
user intervention other then adding the SYMPOS annotations.
If you consider this to be useful I can start experiencing.
>
> Miroslav
>
Powered by blists - more mailing lists