lkml.org 
[lkml]   [2019]   [Apr]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH v3 3/9] livepatch: Add klp-convert tool
    On Wed, Apr 10, 2019 at 11:50:52AM -0400, Joe Lawrence wrote:
    >
    > [ ... snip ... ]
    >
    > +static bool convert_rela(struct section *oldsec, struct rela *r,
    > + struct sympos *sp, struct elf *klp_elf)
    > +{
    > + struct section *sec;
    > + struct rela *r1, *r2;
    > +
    > + sec = get_or_create_klp_rela_section(oldsec, sp, klp_elf);
    > + if (!sec) {
    > + WARN("Can't create or access klp.rela section (%s.%s)\n",
    > + sp->object_name, sp->symbol_name);
    > + return false;
    > + }
    > +
    > + if (!convert_klp_symbol(r->sym, sp)) {
    > + WARN("Unable to convert symbol name (%s.%s)\n", sec->name,
    > + r->sym->name);
    > + return false;
    > + }
    > +
    > + /* Move the converted rela to klp rela section */
    > + list_for_each_entry_safe(r1, r2, &oldsec->relas, list) {
    > + if (r1->sym->name == r->sym->name) {
    > + list_del(&r1->list);
    > + list_add(&r1->list, &sec->relas);
    > + }
    > + }
    > + return true;
    > +}

    This one took a while to find and debug, but I believe that
    convert_rela()'s list removal is not as safe as it thinks it is.

    Start with its calling context from main() below:

    > + list_for_each_entry_safe(sec, aux, &klp_elf->sections, list) {
    > + if (!is_rela_section(sec))
    > + continue;
    > +
    > + list_for_each_entry_safe(rela, tmprela, &sec->relas, list) {
    > + if (!must_convert(rela->sym))
    > + continue;
    > +
    > + if (!find_missing_position(rela->sym, &sp)) {
    > + WARN("Unable to find missing symbol: %s",
    > + rela->sym->name);
    > + return -1;
    > + }
    > + if (!convert_rela(sec, rela, &sp, klp_elf)) {
    > + WARN("Unable to convert relocation: %s",
    > + rela->sym->name);
    > + return -1;
    > + }
    > + }
    > + }

    AFAIK the *_safe list traversals, they cache the ->next value at the
    beginning of each iteration, so that one could blow the current element
    in position away. The cached ->next value is then assigned when moving
    to the next iteration.

    But notice how convert_rela() looks through the entire list of
    relocations, moving each rela with a matching symbol?

    Consider a slight tweak to samples/livepatch-annotated.c:


    static int livepatch_cmdline_proc_show(struct seq_file *m, void *v)
    {
    + if (saved_command_line)
    + saved_command_line[0] = '\0';
    +
    seq_printf(m, "%s livepatch=1\n", saved_command_line);
    return 0;
    }


    On my system, this generates relocations like this:

    % eu-readelf --relocs samples/livepatch/livepatch-annotated-sample.o
    Relocation section [ 2] '.rela.text' for section [ 1] '.text' at offset 0x98 contains 9 entries:
    Offset Type Value Addend Name
    0x0000000000000001 X86_64_PC32 000000000000000000 -4 __fentry__
    0x0000000000000008 X86_64_PC32 000000000000000000 -4 saved_command_line
    0x0000000000000017 X86_64_PC32 000000000000000000 -4 saved_command_line
    0x000000000000001e X86_64_32S 000000000000000000 +0 .rodata.str1.1
    0x0000000000000023 X86_64_PC32 000000000000000000 -4 seq_printf
    0x0000000000000031 X86_64_PC32 000000000000000000 -4 __fentry__
    0x0000000000000038 X86_64_32S 000000000000000000 +0 .data
    0x0000000000000051 X86_64_PC32 000000000000000000 -4 __fentry__
    0x000000000000003d X86_64_PC32 000000000000000000 -4 klp_enable_patch

    We now have back-to-back rela's with sym->name = "saved_command_line".
    When the first is converted, convert_rela() will move both of them to
    the klp rela section. The linked list values may be consistent, but the
    cached ->next value will be bogus and the in-flight-traversal will run
    off the rails.


    I think we can work around it with a combination of 1) only moving a
    single rela symbol at a time in convert_rela and 2) processing the
    second (third, etc.) a little bit more so that they are moved
    individually.

    I hacked this together and it works against the livepatch-annotate.c
    test above so far...

    -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

    diff --git a/scripts/livepatch/klp-convert.c b/scripts/livepatch/klp-convert.c
    index 82c27d219372..126395f1c0cd 100644
    --- a/scripts/livepatch/klp-convert.c
    +++ b/scripts/livepatch/klp-convert.c
    @@ -517,6 +517,7 @@ static bool convert_rela(struct section *oldsec, struct rela *r,
    if (r1->sym->name == r->sym->name) {
    list_del(&r1->list);
    list_add(&r1->list, &sec->relas);
    + break;
    }
    }
    return true;
    @@ -549,8 +550,8 @@ static bool is_converted(char *sname)
    }

    /*
    - * Checks if symbol must be converted (conditions):
    - * not resolved, not already converted or isn't an exported symbol
    + * Checks if symbol must be or was already converted (conditions):
    + * not resolved or isn't an exported symbol
    */
    static bool must_convert(struct symbol *sym)
    {
    @@ -566,7 +567,7 @@ static bool must_convert(struct symbol *sym)
    if (strcmp(sym->name, ".TOC.") == 0)
    return false;

    - return (!(is_converted(sym->name) || is_exported(sym->name)));
    + return (!is_exported(sym->name));
    }

    /* Checks if a section is a klp rela section */
    @@ -640,7 +641,8 @@ int main(int argc, const char **argv)
    if (!must_convert(rela->sym))
    continue;

    - if (!find_missing_position(rela->sym, &sp)) {
    + if (!is_converted(rela->sym->name) &&
    + !find_missing_position(rela->sym, &sp)) {
    WARN("Unable to find missing symbol: %s",
    rela->sym->name);
    return -1;
    --
    2.20.1
    \
     
     \ /
      Last update: 2019-04-23 22:36    [W:4.037 / U:0.012 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site