[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Mon, 27 Aug 2018 12:25:04 -0700
From: Andy Lutomirski <luto@...nel.org>
To: Jann Horn <jannh@...gle.com>
Cc: Andy Lutomirski <luto@...nel.org>, Masami Hiramatsu <mhiramat@...nel.org>,
"Naveen N . Rao" <naveen.n.rao@...ux.vnet.ibm.com>,
Anil S Keshavamurthy <anil.s.keshavamurthy@...el.com>, "David S. Miller" <davem@...emloft.net>,
Kees Cook <keescook@...omium.org>, Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, "the arch/x86 maintainers" <x86@...nel.org>,
Kernel Hardening <kernel-hardening@...ts.openwall.com>, Tony Luck <tony.luck@...el.com>,
Borislav Petkov <bp@...en8.de>, kernel list <linux-kernel@...r.kernel.org>,
Dmitry Vyukov <dvyukov@...gle.com>, linux-edac <linux-edac@...r.kernel.org>
Subject: Re: [PATCH v2 3/7] x86: stop calling fixup_exception() from kprobe_fault_handler()
On Mon, Aug 27, 2018 at 12:22 PM, Jann Horn <jannh@...gle.com> wrote:
> On Mon, Aug 27, 2018 at 9:02 PM Andy Lutomirski <luto@...nel.org> wrote:
>>
>> On Mon, Aug 27, 2018 at 11:56 AM, Jann Horn <jannh@...gle.com> wrote:
>> > This removes the call into exception fixup that was added in
>> > commit c28f896634f2 ("[PATCH] kprobes: fix broken fault handling for
>> > x86_64").
>> >
>> > On X86, kprobe_fault_handler() is called from two places:
>> > do_general_protection() (for #GP) and kprobes_fault() (for #PF).
>> > In both paths, the fixup_exception() call in the kprobe fault handler is
>> > redundant.
>> >
>> > For #GP, fixup_exception() is called immediately before
>> > kprobe_fault_handler() is invoked - if someone wanted to fix up our #GP,
>> > they've already done so, no need to try again. (This assumes that the
>> > kprobe's fault handler isn't going to do something crazy like changing RIP
>> > so that it suddenly points to an instruction that does userspace access.)
>>
>> This needs review by someone who understands kprobes better than I do.
>> What happens if someone puts a kprobe on a uaccess instruction and the
>> uaccess subsequently faults?
>
> Ugh, good point. I'll admit to not having thought about that properly.
>
> I think that's the "if (unlikely(regs->ip == (unsigned
> long)cur->ainsn.insn))" branch in kprobe_fault_handler(), which I'm
> not touching.
>
> For #PF, both without and with my patch, stuff should get fixed up by
> the normal pagefault handler, since the fixup happens after the kprobe
> handler has fiddled with the exception state.
>
> For #GP, we're already past the fixup call, and I think both without
> and with my patch, nothing will catch it - so I think that's a bug,
> but I don't think it's one I'm introducing.
Fair enough. If there is indeed a problem, it'll be easier to fix
sanely with your patch applied :)
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
