[lkml]   [2012]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    SubjectRe: [PATCH v2 1/2] uprobes: Pass probed vaddr to arch_uprobe_analyze_insn()
    On 06/14, Srikar Dronamraju wrote:
    > * Oleg Nesterov <> [2012-06-13 21:15:19]:
    > > For example. Suppose there is some instruction in /lib64/ which
    > > is valid for 64-bit, but not for 32-bit.
    > >
    > > Suppose that a 32-bit application does mmap("/lib64/", PROT_EXEC).
    > >
    > How correct is it to have a 32 bit binary link to a 64 bit binary/library?

    No, I didn't mean this. I guess you misunderstood my point, see below.

    > > Now. If vma_prio_tree_foreach() finds this 32-bit mm first, uprobe_register()
    > > fails even if there are other 64-bit applications which could be traced.
    > >
    > > Or. uprobe_register() succeeds because it finds a 64-bit mm first, and
    > > then that 32-bit application actually executes the invalid insn.
    > >
    > > We can move arch_uprobe_analyze_insn() outside of !UPROBE_COPY_INSN block.
    > >
    > > Or, perhaps, validate_insn_bits() should call both
    > > validate_insn_32bits() and validate_insn_64bits(), and set the
    > > UPROBE_VALID_IF_32 / UPROBE_VALID_IF_64 flags. install_breakpoint()
    > > should do the additinal check before set_swbp() and verify that
    > > .ia32_compat matches UPROBE_VALID_IF_*.
    > >
    > > What do you think?
    > >
    > Lets say we do find a 32 bit app and 64 bit app using the same library
    > and the underlying instruction is valid for tracing in 64 bit and not 32
    > bit. So when we are registering, and failed to insert a breakpoint for
    > the 32 bit app, should we just bail out or should we return a failure?

    I do not really know, I tend to think we should not fail. But this is
    another story...

    Look. Suppose that a 32-bit app starts after uprobe_register() succeeds.
    In this case we have no option, uprobe_mmap()->install_breakpoint()
    should "silently" fail. Currently it doesn't, this is one of the reasons
    why I think the validation logic is wrong.

    And. if install_breakpoint() can fail later anyway (in this case), then
    I think uprobe_register() should not fail.

    But probably this needs more discussion.

    > I would probably prefer to read the underlying file something similar to
    > what exec does and based on the magic decipher if we should verify for
    > 32 bit instructions or 64 bit instructions.

    But this can't protect from the malicious user who does
    mmap(64-bit-code, PROT_EXEC) from a 32-bit app, and this can confuse
    uprobes even if that 32-bit app never tries to actually execute that

    That is why I think we need the additional (and arch-dependant) check
    before every set_swbp(), but arch_uprobe_analyze_insn/etc should not
    depend on task/mm/vaddr/whatever.


     \ /
      Last update: 2012-06-14 21:01    [W:4.051 / U:0.008 seconds]
    ©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site