From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756517AbdDFAOU (ORCPT ); Wed, 5 Apr 2017 20:14:20 -0400 Received: from mail-it0-f50.google.com ([209.85.214.50]:36910 "EHLO mail-it0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752318AbdDFAOL (ORCPT ); Wed, 5 Apr 2017 20:14:11 -0400 MIME-Version: 1.0 In-Reply-To: References: <1490811363-93944-1-git-send-email-keescook@chromium.org> <1490811363-93944-5-git-send-email-keescook@chromium.org> From: Kees Cook Date: Wed, 5 Apr 2017 17:14:09 -0700 X-Google-Sender-Auth: pIOQnfh37ke4fA9VmoCp4f3NfKs Message-ID: Subject: Re: [RFC v2][PATCH 04/11] x86: Implement __arch_rare_write_begin/unmap() To: Andy Lutomirski Cc: "kernel-hardening@lists.openwall.com" , Mark Rutland , Hoeun Ryu , PaX Team , Emese Revfy , Russell King , X86 ML , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 5, 2017 at 4:57 PM, Andy Lutomirski wrote: > On Wed, Mar 29, 2017 at 6:41 PM, Kees Cook wrote: >> On Wed, Mar 29, 2017 at 3:38 PM, Andy Lutomirski wrote: >>> On Wed, Mar 29, 2017 at 11:15 AM, Kees Cook 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(); } static __always_inline rare_write_end(void) { barrier(); __arch_rare_write_end(); barrier(); local_irq_enable(); preempt_enable_no_resched(); } x86-specific helpers: static __always_inline unsigned long __arch_rare_write_begin(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(cr0 & X86_CR0_WP); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } static __always_inline unsigned long __arch_rare_write_end(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(!(cr0 & X86_CR0_WP)); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } I can give it a spin... -Kees -- Kees Cook Pixel Security From mboxrd@z Thu Jan 1 00:00:00 1970 From: keescook@chromium.org (Kees Cook) Date: Wed, 5 Apr 2017 17:14:09 -0700 Subject: [RFC v2][PATCH 04/11] x86: Implement __arch_rare_write_begin/unmap() In-Reply-To: References: <1490811363-93944-1-git-send-email-keescook@chromium.org> <1490811363-93944-5-git-send-email-keescook@chromium.org> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Apr 5, 2017 at 4:57 PM, Andy Lutomirski wrote: > On Wed, Mar 29, 2017 at 6:41 PM, Kees Cook wrote: >> On Wed, Mar 29, 2017 at 3:38 PM, Andy Lutomirski wrote: >>> On Wed, Mar 29, 2017 at 11:15 AM, Kees Cook 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(); } static __always_inline rare_write_end(void) { barrier(); __arch_rare_write_end(); barrier(); local_irq_enable(); preempt_enable_no_resched(); } x86-specific helpers: static __always_inline unsigned long __arch_rare_write_begin(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(cr0 & X86_CR0_WP); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } static __always_inline unsigned long __arch_rare_write_end(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(!(cr0 & X86_CR0_WP)); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } I can give it a spin... -Kees -- Kees Cook Pixel Security From mboxrd@z Thu Jan 1 00:00:00 1970 MIME-Version: 1.0 Sender: keescook@google.com In-Reply-To: References: <1490811363-93944-1-git-send-email-keescook@chromium.org> <1490811363-93944-5-git-send-email-keescook@chromium.org> From: Kees Cook Date: Wed, 5 Apr 2017 17:14:09 -0700 Message-ID: Content-Type: text/plain; charset=UTF-8 Subject: [kernel-hardening] Re: [RFC v2][PATCH 04/11] x86: Implement __arch_rare_write_begin/unmap() To: Andy Lutomirski Cc: "kernel-hardening@lists.openwall.com" , Mark Rutland , Hoeun Ryu , PaX Team , Emese Revfy , Russell King , X86 ML , "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" List-ID: On Wed, Apr 5, 2017 at 4:57 PM, Andy Lutomirski wrote: > On Wed, Mar 29, 2017 at 6:41 PM, Kees Cook wrote: >> On Wed, Mar 29, 2017 at 3:38 PM, Andy Lutomirski wrote: >>> On Wed, Mar 29, 2017 at 11:15 AM, Kees Cook 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(); } static __always_inline rare_write_end(void) { barrier(); __arch_rare_write_end(); barrier(); local_irq_enable(); preempt_enable_no_resched(); } x86-specific helpers: static __always_inline unsigned long __arch_rare_write_begin(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(cr0 & X86_CR0_WP); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } static __always_inline unsigned long __arch_rare_write_end(void) { unsigned long cr0; cr0 = read_cr0() ^ X86_CR0_WP; BUG_ON(!(cr0 & X86_CR0_WP)); write_cr0(cr0); return cr0 ^ X86_CR0_WP; } I can give it a spin... -Kees -- Kees Cook Pixel Security