From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752703AbcHQNRA (ORCPT ); Wed, 17 Aug 2016 09:17:00 -0400 Received: from smtprelay0164.hostedemail.com ([216.40.44.164]:50287 "EHLO smtprelay.hostedemail.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751550AbcHQNQ7 (ORCPT ); Wed, 17 Aug 2016 09:16:59 -0400 X-Session-Marker: 726F737465647440676F6F646D69732E6F7267 X-Spam-Summary: 2,0,0,,d41d8cd98f00b204,rostedt@goodmis.org,:::::::::::::::::::::::::::::::::::::::::::::::::,RULES_HIT:41:69:355:379:541:599:800:960:973:988:989:1260:1277:1311:1313:1314:1345:1359:1437:1515:1516:1518:1535:1544:1593:1594:1605:1711:1730:1747:1777:1792:2393:2538:2553:2559:2562:2693:3138:3139:3140:3141:3142:3622:3865:3866:3867:3868:3870:3871:3873:3874:4117:4321:4605:5007:6261:6742:6743:7807:7875:9010:9012:9040:10004:10848:10967:11026:11232:11473:11658:11914:12043:12291:12294:12296:12517:12519:12555:12683:12740:13439:14181:14659:14721:21080:21451:30012:30054:30064:30067:30069:30070:30090:30091,0,RBL:none,CacheIP:none,Bayesian:0.5,0.5,0.5,Netcheck:none,DomainCache:0,MSF:not bulk,SPF:fn,MSBL:0,DNSBL:none,Custom_rules:0:0:0,LFtime:4,LUA_SUMMARY:none X-HE-Tag: frog13_767fff9b11b48 X-Filterd-Recvd-Size: 6694 Date: Wed, 17 Aug 2016 09:16:52 -0400 From: Steven Rostedt To: Kees Cook Cc: "Paul E . McKenney" , Laura Abbott , Stephen Boyd , Daniel Micay , Joe Perches , Arnd Bergmann , Greg Kroah-Hartman , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , "Aneesh Kumar K.V" , "Kirill A. Shutemov" , Michael Ellerman , Dan Williams , Andrew Morton , Ingo Molnar , Thomas Gleixner , Josef Bacik , Andrey Ryabinin , Tejun Heo , Nikolay Aleksandrov , Dmitry Vyukov , linux-kernel@vger.kernel.org, kernel-hardening@lists.openwall.com Subject: Re: [PATCH v2 1/5] list: Split list_add() debug checking into separate function Message-ID: <20160817091652.59fa2668@gandalf.local.home> In-Reply-To: <1471393229-27182-2-git-send-email-keescook@chromium.org> References: <1471393229-27182-1-git-send-email-keescook@chromium.org> <1471393229-27182-2-git-send-email-keescook@chromium.org> X-Mailer: Claws Mail 3.13.2 (GTK+ 2.24.30; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 16 Aug 2016 17:20:25 -0700 Kees Cook wrote: > Right now, __list_add() code is repeated either in list.h or in > list_debug.c, but only the debug checks are the different part. This > extracts the checking into a separate function and consolidates > __list_add(). Additionally this __list_add_debug() will stop list > manipulations if a corruption is detected, instead of allowing for further > corruption that may lead to even worse conditions. > > This is slight refactoring of the same hardening done in PaX and Grsecurity. > > Signed-off-by: Kees Cook > --- > include/linux/list.h | 22 ++++++++++++++++------ > lib/list_debug.c | 48 +++++++++++++++++++++++------------------------- > 2 files changed, 39 insertions(+), 31 deletions(-) > > diff --git a/include/linux/list.h b/include/linux/list.h > index 5183138aa932..0ed58591538e 100644 > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -28,27 +28,37 @@ static inline void INIT_LIST_HEAD(struct list_head *list) > list->prev = list; > } > > +#ifdef CONFIG_DEBUG_LIST > +extern bool __list_add_valid(struct list_head *new, > + struct list_head *prev, > + struct list_head *next); > +#else > +static inline bool __list_add_valid(struct list_head *new, > + struct list_head *prev, > + struct list_head *next) > +{ > + return true; > +} > +#endif > + > /* > * Insert a new entry between two known consecutive entries. > * > * This is only for internal list manipulation where we know > * the prev/next entries already! > */ > -#ifndef CONFIG_DEBUG_LIST > static inline void __list_add(struct list_head *new, > struct list_head *prev, > struct list_head *next) > { > + if (!__list_add_valid(new, prev, next)) > + return; > + > next->prev = new; > new->next = next; > new->prev = prev; > WRITE_ONCE(prev->next, new); > } > -#else > -extern void __list_add(struct list_head *new, > - struct list_head *prev, > - struct list_head *next); > -#endif > > /** > * list_add - add a new entry > diff --git a/lib/list_debug.c b/lib/list_debug.c > index 3859bf63561c..149dd57b583b 100644 > --- a/lib/list_debug.c > +++ b/lib/list_debug.c > @@ -2,8 +2,7 @@ > * Copyright 2006, Red Hat, Inc., Dave Jones > * Released under the General Public License (GPL). > * > - * This file contains the linked list implementations for > - * DEBUG_LIST. > + * This file contains the linked list validation for DEBUG_LIST. > */ > > #include > @@ -13,33 +12,32 @@ > #include > > /* > - * Insert a new entry between two known consecutive entries. > - * > - * This is only for internal list manipulation where we know > - * the prev/next entries already! > + * Check that the data structures for the list manipulations are reasonably > + * valid. Failures here indicate memory corruption (and possibly an exploit > + * attempt). > */ > > -void __list_add(struct list_head *new, > - struct list_head *prev, > - struct list_head *next) > +bool __list_add_valid(struct list_head *new, struct list_head *prev, > + struct list_head *next) > { > - WARN(next->prev != prev, > - "list_add corruption. next->prev should be " > - "prev (%p), but was %p. (next=%p).\n", > - prev, next->prev, next); > - WARN(prev->next != next, > - "list_add corruption. prev->next should be " > - "next (%p), but was %p. (prev=%p).\n", > - next, prev->next, prev); > - WARN(new == prev || new == next, > - "list_add double add: new=%p, prev=%p, next=%p.\n", > - new, prev, next); > - next->prev = new; > - new->next = next; > - new->prev = prev; > - WRITE_ONCE(prev->next, new); > + if (unlikely(next->prev != prev)) { > + WARN(1, "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", > + prev, next->prev, next); > + return false; BTW, WARN() does return the result, thus you could have just wrapped the if () around them: if (WARN(next->prev != prev, "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", prev, next->prev, next)) return; Just FYI. -- Steve > + } > + if (unlikely(prev->next != next)) { > + WARN(1, "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", > + next, prev->next, prev); > + return false; > + } > + if (unlikely(new == prev || new == next)) { > + WARN(1, "list_add double add: new=%p, prev=%p, next=%p.\n", > + new, prev, next); > + return false; > + } > + return true; > } > -EXPORT_SYMBOL(__list_add); > +EXPORT_SYMBOL(__list_add_valid); > > void __list_del_entry(struct list_head *entry) > { From mboxrd@z Thu Jan 1 00:00:00 1970 Reply-To: kernel-hardening@lists.openwall.com Date: Wed, 17 Aug 2016 09:16:52 -0400 From: Steven Rostedt Message-ID: <20160817091652.59fa2668@gandalf.local.home> In-Reply-To: <1471393229-27182-2-git-send-email-keescook@chromium.org> References: <1471393229-27182-1-git-send-email-keescook@chromium.org> <1471393229-27182-2-git-send-email-keescook@chromium.org> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: [kernel-hardening] Re: [PATCH v2 1/5] list: Split list_add() debug checking into separate function To: Kees Cook Cc: "Paul E . McKenney" , Laura Abbott , Stephen Boyd , Daniel Micay , Joe Perches , Arnd Bergmann , Greg Kroah-Hartman , Josh Triplett , Mathieu Desnoyers , Lai Jiangshan , "Aneesh Kumar K.V" , "Kirill A. Shutemov" , Michael Ellerman , Dan Williams , Andrew Morton , Ingo Molnar , Thomas Gleixner , Josef Bacik , Andrey Ryabinin , Tejun Heo , Nikolay Aleksandrov , Dmitry Vyukov , linux-kernel@vger.kernel.org, kernel-hardening@lists.openwall.com List-ID: On Tue, 16 Aug 2016 17:20:25 -0700 Kees Cook wrote: > Right now, __list_add() code is repeated either in list.h or in > list_debug.c, but only the debug checks are the different part. This > extracts the checking into a separate function and consolidates > __list_add(). Additionally this __list_add_debug() will stop list > manipulations if a corruption is detected, instead of allowing for further > corruption that may lead to even worse conditions. > > This is slight refactoring of the same hardening done in PaX and Grsecurity. > > Signed-off-by: Kees Cook > --- > include/linux/list.h | 22 ++++++++++++++++------ > lib/list_debug.c | 48 +++++++++++++++++++++++------------------------- > 2 files changed, 39 insertions(+), 31 deletions(-) > > diff --git a/include/linux/list.h b/include/linux/list.h > index 5183138aa932..0ed58591538e 100644 > --- a/include/linux/list.h > +++ b/include/linux/list.h > @@ -28,27 +28,37 @@ static inline void INIT_LIST_HEAD(struct list_head *list) > list->prev = list; > } > > +#ifdef CONFIG_DEBUG_LIST > +extern bool __list_add_valid(struct list_head *new, > + struct list_head *prev, > + struct list_head *next); > +#else > +static inline bool __list_add_valid(struct list_head *new, > + struct list_head *prev, > + struct list_head *next) > +{ > + return true; > +} > +#endif > + > /* > * Insert a new entry between two known consecutive entries. > * > * This is only for internal list manipulation where we know > * the prev/next entries already! > */ > -#ifndef CONFIG_DEBUG_LIST > static inline void __list_add(struct list_head *new, > struct list_head *prev, > struct list_head *next) > { > + if (!__list_add_valid(new, prev, next)) > + return; > + > next->prev = new; > new->next = next; > new->prev = prev; > WRITE_ONCE(prev->next, new); > } > -#else > -extern void __list_add(struct list_head *new, > - struct list_head *prev, > - struct list_head *next); > -#endif > > /** > * list_add - add a new entry > diff --git a/lib/list_debug.c b/lib/list_debug.c > index 3859bf63561c..149dd57b583b 100644 > --- a/lib/list_debug.c > +++ b/lib/list_debug.c > @@ -2,8 +2,7 @@ > * Copyright 2006, Red Hat, Inc., Dave Jones > * Released under the General Public License (GPL). > * > - * This file contains the linked list implementations for > - * DEBUG_LIST. > + * This file contains the linked list validation for DEBUG_LIST. > */ > > #include > @@ -13,33 +12,32 @@ > #include > > /* > - * Insert a new entry between two known consecutive entries. > - * > - * This is only for internal list manipulation where we know > - * the prev/next entries already! > + * Check that the data structures for the list manipulations are reasonably > + * valid. Failures here indicate memory corruption (and possibly an exploit > + * attempt). > */ > > -void __list_add(struct list_head *new, > - struct list_head *prev, > - struct list_head *next) > +bool __list_add_valid(struct list_head *new, struct list_head *prev, > + struct list_head *next) > { > - WARN(next->prev != prev, > - "list_add corruption. next->prev should be " > - "prev (%p), but was %p. (next=%p).\n", > - prev, next->prev, next); > - WARN(prev->next != next, > - "list_add corruption. prev->next should be " > - "next (%p), but was %p. (prev=%p).\n", > - next, prev->next, prev); > - WARN(new == prev || new == next, > - "list_add double add: new=%p, prev=%p, next=%p.\n", > - new, prev, next); > - next->prev = new; > - new->next = next; > - new->prev = prev; > - WRITE_ONCE(prev->next, new); > + if (unlikely(next->prev != prev)) { > + WARN(1, "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", > + prev, next->prev, next); > + return false; BTW, WARN() does return the result, thus you could have just wrapped the if () around them: if (WARN(next->prev != prev, "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", prev, next->prev, next)) return; Just FYI. -- Steve > + } > + if (unlikely(prev->next != next)) { > + WARN(1, "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", > + next, prev->next, prev); > + return false; > + } > + if (unlikely(new == prev || new == next)) { > + WARN(1, "list_add double add: new=%p, prev=%p, next=%p.\n", > + new, prev, next); > + return false; > + } > + return true; > } > -EXPORT_SYMBOL(__list_add); > +EXPORT_SYMBOL(__list_add_valid); > > void __list_del_entry(struct list_head *entry) > {