lkml.org 
[lkml]   [2022]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 10/18] objtool: Extricate ibt from stack validation
    On Thu, Apr 14, 2022 at 09:53:18AM +0200, Peter Zijlstra wrote:
    > On Wed, Apr 13, 2022 at 04:19:45PM -0700, Josh Poimboeuf wrote:
    > > Extricate ibt from validate_branch() in preparation for making stack
    > > validation optional.
    >
    > It does a bit more..

    Indeed.

    > > - /* already done in validate_branch() */
    > > - if (sec->sh.sh_flags & SHF_EXECINSTR)
    > > - continue;
    > >
    > > - if (!sec->reloc)
    > > continue;
    > >
    > > - if (!strncmp(sec->name, ".orc", 4))
    > > - continue;
    > >
    > > - if (!strncmp(sec->name, ".discard", 8))
    > > continue;
    > >
    > > - if (!strncmp(sec->name, ".debug", 6))
    > > continue;
    > >
    > > - if (!strcmp(sec->name, "_error_injection_whitelist"))
    > > continue;
    > >
    > > - if (!strcmp(sec->name, "_kprobe_blacklist"))
    > > continue;
    > >
    > > - is_data = strstr(sec->name, ".data") || strstr(sec->name, ".rodata");
    > >
    > > - list_for_each_entry(reloc, &sec->reloc->reloc_list, list) {
    > > - struct instruction *dest;
    > >
    > > - dest = validate_ibt_reloc(file, reloc);
    > > - if (is_data && dest && !dest->noendbr)
    > > - warn_noendbr("data ", sec, reloc->offset, dest);
    > > - }
    >
    > So this iterates all sections and excludes a bunch, and only reports
    > fail for .data/.rodata.

    Oops.

    > > +static int validate_ibt(struct objtool_file *file)
    > > +{
    > > + struct section *sec;
    > > + struct reloc *reloc;
    > > + struct instruction *insn;
    > > + int warnings = 0;
    > > +
    > > + for_each_insn(file, insn)
    > > + warnings += validate_ibt_insn(file, insn);
    >
    > So I specifically didn't do this because I wanted to reduce the amount
    > of loops we do over those instructions. But yeah, if you really want to
    > allow --ibt without --stack-validate (but why?) then I suppose so.
    >
    > Esp. for the vmlinux.o case, iterating all insn can quickly add up to
    > significant time.

    I didn't look at the performance, but if it's a problem then we can
    eventually look at combining several of the for_each_insn() features
    into a single loop: retpolines, ibt, reachability check, maybe even some
    of decode_sections().

    >
    > > + for_each_sec(file, sec) {
    > > +
    > > + if (!strstr(sec->name, ".data") && !strstr(sec->name, ".rodata"))
    > > + continue;
    >
    > But this only iterates .data/.rodata.
    >
    > That's not the same, specifically, it'll not iterate stuff like ksymtab
    > that contains the EXPORT_SYMBOL* crud. The result being that we can now
    > seal EXPORT'ed symbols, which will make modules really sad.
    >
    > There's also the .initcall sections, sealing initcalls typcally ends really
    > badly.
    >
    > And there might be a few others I forgot about.

    Ok. That was subtle, it needs a comment or two. I had the distinct
    feeling I was introducing a bug, then I got distracted ;-)

    Doesn't the compiler give those special cases ENDBR anyway? Just
    wondering why we avoid the warning for those.

    --
    Josh

    \
     
     \ /
      Last update: 2022-04-14 18:08    [W:3.257 / U:0.412 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site