|
Message-ID: <CA+rthh95rW95woiWvyeCcoJT-qRM18KCDGHGtH=bDHZdExmVmg@mail.gmail.com> Date: Fri, 7 Apr 2017 10:34:32 +0200 From: Mathias Krause <minipli@...glemail.com> To: Andy Lutomirski <luto@...capital.net>, Kees Cook <keescook@...omium.org> Cc: Andy Lutomirski <luto@...nel.org>, "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, Mark Rutland <mark.rutland@....com>, Hoeun Ryu <hoeun.ryu@...il.com>, PaX Team <pageexec@...email.hu>, Emese Revfy <re.emese@...il.com>, Russell King <linux@...linux.org.uk>, X86 ML <x86@...nel.org>, "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org> Subject: Re: Re: [RFC v2][PATCH 04/11] x86: Implement __arch_rare_write_begin/unmap() On 6 April 2017 at 17:59, Andy Lutomirski <luto@...capital.net> wrote: > On Wed, Apr 5, 2017 at 5:14 PM, Kees Cook <keescook@...omium.org> wrote: >> On Wed, Apr 5, 2017 at 4:57 PM, Andy Lutomirski <luto@...nel.org> wrote: >>> On Wed, Mar 29, 2017 at 6:41 PM, Kees Cook <keescook@...omium.org> wrote: >>>> On Wed, Mar 29, 2017 at 3:38 PM, Andy Lutomirski <luto@...capital.net> wrote: >>>>> On Wed, Mar 29, 2017 at 11:15 AM, Kees Cook <keescook@...omium.org> wrote: >>>>>> Based on PaX's x86 pax_{open,close}_kernel() implementation, this >>>>>> allows HAVE_ARCH_RARE_WRITE to work on x86. >>>>>> >>>>>> + >>>>>> +static __always_inline unsigned long __arch_rare_write_begin(void) >>>>>> +{ >>>>>> + unsigned long cr0; >>>>>> + >>>>>> + preempt_disable(); >>>>> >>>>> This looks wrong. DEBUG_LOCKS_WARN_ON(!irqs_disabled()) would work, >>>>> as would local_irq_disable(). There's no way that just disabling >>>>> preemption is enough. >>>>> >>>>> (Also, how does this interact with perf nmis?) >>>> >>>> Do you mean preempt_disable() isn't strong enough here? I'm open to >>>> suggestions. The goal would be to make sure nothing between _begin and >>>> _end would get executed without interruption... >>>> >>> >>> Sorry for the very slow response. >>> >>> preempt_disable() isn't strong enough to prevent interrupts, and an >>> interrupt here would run with WP off, causing unknown havoc. I tend >>> to think that the caller should be responsible for turning off >>> interrupts. >> >> So, something like: >> >> Top-level functions: >> >> static __always_inline rare_write_begin(void) >> { >> preempt_disable(); >> local_irq_disable(); >> barrier(); >> __arch_rare_write_begin(); >> barrier(); >> } > > Looks good, except you don't need preempt_disable(). > local_irq_disable() also disables preemption. You might need to use > local_irq_save(), though, depending on whether any callers already > have IRQs off. Well, doesn't look good to me. NMIs will still be able to interrupt this code and will run with CR0.WP = 0. Shouldn't you instead question yourself why PaX can do it "just" with preempt_disable() instead?! Cheers, Mathias
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.