From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933688AbcKVR2Z (ORCPT ); Tue, 22 Nov 2016 12:28:25 -0500 Received: from mail-vk0-f50.google.com ([209.85.213.50]:33648 "EHLO mail-vk0-f50.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933653AbcKVR2W (ORCPT ); Tue, 22 Nov 2016 12:28:22 -0500 MIME-Version: 1.0 In-Reply-To: <20161122095715.GN3092@twins.programming.kicks-ass.net> References: <20161122095715.GN3092@twins.programming.kicks-ass.net> From: Andy Lutomirski Date: Tue, 22 Nov 2016 09:28:01 -0800 Message-ID: Subject: Re: [RFC][PATCH] x86: Verify access_ok() context To: Peter Zijlstra Cc: Linus Torvalds , Ingo Molnar , Thomas Gleixner , "H. Peter Anvin" , "linux-kernel@vger.kernel.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 Tue, Nov 22, 2016 at 1:57 AM, Peter Zijlstra wrote: > > I recently encountered wreckage because access_ok() was used where it > should not be, add an explicit WARN when access_ok() is used wrongly. > > Signed-off-by: Peter Zijlstra (Intel) > --- > arch/x86/include/asm/uaccess.h | 7 +++++-- > include/linux/preempt.h | 21 +++++++++++++-------- > 2 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h > index faf3687f1035..b139c46ba122 100644 > --- a/arch/x86/include/asm/uaccess.h > +++ b/arch/x86/include/asm/uaccess.h > @@ -88,8 +88,11 @@ static inline bool __chk_range_not_ok(unsigned long addr, unsigned long size, un > * checks that the pointer is in the user space range - after calling > * this function, memory access functions may still return -EFAULT. > */ > -#define access_ok(type, addr, size) \ > - likely(!__range_not_ok(addr, size, user_addr_max())) > +#define access_ok(type, addr, size) \ > +({ \ > + WARN_ON_ONCE(!in_task()); \ Should this be guarded by some debug option? This may hurt performance on production systems quite a bit. > diff --git a/include/linux/preempt.h b/include/linux/preempt.h > index 75e4e30677f1..7eeceac52dea 100644 > --- a/include/linux/preempt.h > +++ b/include/linux/preempt.h > @@ -65,19 +65,24 @@ > > /* > * Are we doing bottom half or hardware interrupt processing? > - * Are we in a softirq context? Interrupt context? > - * in_softirq - Are we currently processing softirq or have bh disabled? > - * in_serving_softirq - Are we currently processing softirq? > + * > + * in_irq() - We're in (hard) IRQ context > + * in_softirq() - We have BH disabled, or are processing softirqs > + * in_interrupt() - We're in NMI,IRQ,SoftIRQ context or have BH disabled > + * in_serving_softirq() - We're in softirq context > + * in_nmi() - We're in NMI context > + * in_task() - We're in task context > + * > + * Note: due to the BH disabled confusion: in_softirq(),in_interrupt() really > + * should not be used in new code. > */ > #define in_irq() (hardirq_count()) > #define in_softirq() (softirq_count()) > #define in_interrupt() (irq_count()) > #define in_serving_softirq() (softirq_count() & SOFTIRQ_OFFSET) > - > -/* > - * Are we in NMI context? > - */ > -#define in_nmi() (preempt_count() & NMI_MASK) > +#define in_nmi() (preempt_count() & NMI_MASK) > +#define in_task() (!(preempt_count() & \ > + (NMI_MASK | HARDIRQ_MASK | SOFTIRQ_OFFSET))) LGTM. For what it's worth, I think ARM recently started saving the address limit and resetting it to USER_DS on NMI entry. --Andy