From 68e051d4a782767639205e447c083d08c51bc028 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 17 Aug 2016 14:42:08 -0700 Subject: BACKPORT: list: Split list_add() debug checking into separate function (cherry-picked from d7c816733d501b59dbdc2483f2cc8e4431fd9160) Right now, __list_add() code is repeated either in list.h or in list_debug.c, but the only differences between the two versions are the debug checks. This commit therefore extracts these debug checks into a separate __list_add_valid() function and consolidates __list_add(). Additionally this new __list_add_valid() function 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. Change-Id: I9a9c9a58857cf837bec7abdb2ee4970cd1242a5e Signed-off-by: Kees Cook Acked-by: Steven Rostedt Signed-off-by: Paul E. McKenney Acked-by: Rik van Riel Signed-off-by: Satya Tangirala --- lib/list_debug.c | 48 +++++++++++++++++++++++------------------------- 1 file changed, 23 insertions(+), 25 deletions(-) (limited to 'lib/list_debug.c') diff --git a/lib/list_debug.c b/lib/list_debug.c index c24c2f7e296f..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; - 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; + } + 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) { -- cgit v1.2.3 From f0b68d746af4aa4988463b24c6f5d0f97f0aaa82 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 17 Aug 2016 14:42:09 -0700 Subject: UPSTREAM: rculist: Consolidate DEBUG_LIST for list_add_rcu() (cherry-picked from 54acd4397d7e7a725c94101180cd9f38ef701acc) This commit consolidates the debug checking for list_add_rcu() into the new single __list_add_valid() debug function. Notably, this commit fixes the sanity check that was added in commit 17a801f4bfeb ("list_debug: WARN for adding something already in the list"), which wasn't checking RCU-protected lists. Change-Id: I1f7e169d4dc45bbc9938087a171c5df747344414 Signed-off-by: Kees Cook Acked-by: Steven Rostedt Signed-off-by: Paul E. McKenney Acked-by: Rik van Riel Signed-off-by: Satya Tangirala --- lib/list_debug.c | 19 ------------------- 1 file changed, 19 deletions(-) (limited to 'lib/list_debug.c') diff --git a/lib/list_debug.c b/lib/list_debug.c index 149dd57b583b..d0b89b9d0736 100644 --- a/lib/list_debug.c +++ b/lib/list_debug.c @@ -77,22 +77,3 @@ void list_del(struct list_head *entry) entry->prev = LIST_POISON2; } EXPORT_SYMBOL(list_del); - -/* - * RCU variants. - */ -void __list_add_rcu(struct list_head *new, - struct list_head *prev, struct list_head *next) -{ - WARN(next->prev != prev, - "list_add_rcu corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", - prev, next->prev, next); - WARN(prev->next != next, - "list_add_rcu corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", - next, prev->next, prev); - new->next = next; - new->prev = prev; - rcu_assign_pointer(list_next_rcu(prev), new); - next->prev = new; -} -EXPORT_SYMBOL(__list_add_rcu); -- cgit v1.2.3 From 10c8c8a765f1335c46817d6dbf3e288dd45884df Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 17 Aug 2016 14:42:10 -0700 Subject: UPSTREAM: list: Split list_del() debug checking into separate function (cherry-picked from 0cd340dcb05c4a43742fe156f36737bb2a321bfd) Similar to the list_add() debug consolidation, this commit consolidates the debug checking performed during CONFIG_DEBUG_LIST into a new __list_del_entry_valid() function, and stops list updates when corruption is found. Refactored from same hardening in PaX and Grsecurity. Change-Id: I9e3b8654ab25f3a196e3336fc4882b73010873e7 Signed-off-by: Kees Cook Acked-by: Steven Rostedt Signed-off-by: Paul E. McKenney Acked-by: Rik van Riel Signed-off-by: Satya Tangirala --- lib/list_debug.c | 53 +++++++++++++++++++++++------------------------------ 1 file changed, 23 insertions(+), 30 deletions(-) (limited to 'lib/list_debug.c') diff --git a/lib/list_debug.c b/lib/list_debug.c index d0b89b9d0736..276565fca2a6 100644 --- a/lib/list_debug.c +++ b/lib/list_debug.c @@ -39,41 +39,34 @@ bool __list_add_valid(struct list_head *new, struct list_head *prev, } EXPORT_SYMBOL(__list_add_valid); -void __list_del_entry(struct list_head *entry) +bool __list_del_entry_valid(struct list_head *entry) { struct list_head *prev, *next; prev = entry->prev; next = entry->next; - if (WARN(next == LIST_POISON1, - "list_del corruption, %p->next is LIST_POISON1 (%p)\n", - entry, LIST_POISON1) || - WARN(prev == LIST_POISON2, - "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", - entry, LIST_POISON2) || - WARN(prev->next != entry, - "list_del corruption. prev->next should be %p, " - "but was %p\n", entry, prev->next) || - WARN(next->prev != entry, - "list_del corruption. next->prev should be %p, " - "but was %p\n", entry, next->prev)) - return; - - __list_del(prev, next); -} -EXPORT_SYMBOL(__list_del_entry); + if (unlikely(next == LIST_POISON1)) { + WARN(1, "list_del corruption, %p->next is LIST_POISON1 (%p)\n", + entry, LIST_POISON1); + return false; + } + if (unlikely(prev == LIST_POISON2)) { + WARN(1, "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", + entry, LIST_POISON2); + return false; + } + if (unlikely(prev->next != entry)) { + WARN(1, "list_del corruption. prev->next should be %p, but was %p\n", + entry, prev->next); + return false; + } + if (unlikely(next->prev != entry)) { + WARN(1, "list_del corruption. next->prev should be %p, but was %p\n", + entry, next->prev); + return false; + } + return true; -/** - * list_del - deletes entry from list. - * @entry: the element to delete from the list. - * Note: list_empty on entry does not return true after this, the entry is - * in an undefined state. - */ -void list_del(struct list_head *entry) -{ - __list_del_entry(entry); - entry->next = LIST_POISON1; - entry->prev = LIST_POISON2; } -EXPORT_SYMBOL(list_del); +EXPORT_SYMBOL(__list_del_entry_valid); -- cgit v1.2.3 From 7514f97280fdf4373f99ca28e573925cfb828aa7 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Wed, 17 Aug 2016 14:42:11 -0700 Subject: UPSTREAM: bug: Provide toggle for BUG on data corruption (cherry-picked from de54ebbe26bb371a6f1fbc0593372232f04e3107) The kernel checks for cases of data structure corruption under some CONFIGs (e.g. CONFIG_DEBUG_LIST). When corruption is detected, some systems may want to BUG() immediately instead of letting the system run with known corruption. Usually these kinds of manipulation primitives can be used by security flaws to gain arbitrary memory write control. This provides a new config CONFIG_BUG_ON_DATA_CORRUPTION and a corresponding macro CHECK_DATA_CORRUPTION for handling these situations. Notably, even if not BUGing, the kernel should not continue processing the corrupted structure. This is inspired by similar hardening by Syed Rameez Mustafa in MSM kernels, and in PaX and Grsecurity, which is likely in response to earlier removal of the BUG calls in commit 924d9addb9b1 ("list debugging: use WARN() instead of BUG()"). Change-Id: I4cdfa9fbebe32a990a111d051e4ec4e421f77a09 Signed-off-by: Kees Cook Acked-by: Steven Rostedt Signed-off-by: Paul E. McKenney Acked-by: Rik van Riel Signed-off-by: Satya Tangirala --- lib/list_debug.c | 57 ++++++++++++++++++++++---------------------------------- 1 file changed, 22 insertions(+), 35 deletions(-) (limited to 'lib/list_debug.c') diff --git a/lib/list_debug.c b/lib/list_debug.c index 276565fca2a6..7f7bfa55eb6d 100644 --- a/lib/list_debug.c +++ b/lib/list_debug.c @@ -20,21 +20,16 @@ bool __list_add_valid(struct list_head *new, struct list_head *prev, struct list_head *next) { - 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; - } - 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; - } + CHECK_DATA_CORRUPTION(next->prev != prev, + "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", + prev, next->prev, next); + CHECK_DATA_CORRUPTION(prev->next != next, + "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", + next, prev->next, prev); + CHECK_DATA_CORRUPTION(new == prev || new == next, + "list_add double add: new=%p, prev=%p, next=%p.\n", + new, prev, next); + return true; } EXPORT_SYMBOL(__list_add_valid); @@ -46,26 +41,18 @@ bool __list_del_entry_valid(struct list_head *entry) prev = entry->prev; next = entry->next; - if (unlikely(next == LIST_POISON1)) { - WARN(1, "list_del corruption, %p->next is LIST_POISON1 (%p)\n", - entry, LIST_POISON1); - return false; - } - if (unlikely(prev == LIST_POISON2)) { - WARN(1, "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", - entry, LIST_POISON2); - return false; - } - if (unlikely(prev->next != entry)) { - WARN(1, "list_del corruption. prev->next should be %p, but was %p\n", - entry, prev->next); - return false; - } - if (unlikely(next->prev != entry)) { - WARN(1, "list_del corruption. next->prev should be %p, but was %p\n", - entry, next->prev); - return false; - } + CHECK_DATA_CORRUPTION(next == LIST_POISON1, + "list_del corruption, %p->next is LIST_POISON1 (%p)\n", + entry, LIST_POISON1); + CHECK_DATA_CORRUPTION(prev == LIST_POISON2, + "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", + entry, LIST_POISON2); + CHECK_DATA_CORRUPTION(prev->next != entry, + "list_del corruption. prev->next should be %p, but was %p\n", + entry, prev->next); + CHECK_DATA_CORRUPTION(next->prev != entry, + "list_del corruption. next->prev should be %p, but was %p\n", + entry, next->prev); return true; } -- cgit v1.2.3 From a3772a806a04164f2fc9a956bb997c4b8c8179e9 Mon Sep 17 00:00:00 2001 From: Kees Cook Date: Fri, 24 Feb 2017 15:00:38 -0800 Subject: UPSTREAM: bug: switch data corruption check to __must_check (cherry-picked from 85caa95b9f19bb3a26d7e025d1134760b69e0c40) The CHECK_DATA_CORRUPTION() macro was designed to have callers do something meaningful/protective on failure. However, using "return false" in the macro too strictly limits the design patterns of callers. Instead, let callers handle the logic test directly, but make sure that the result IS checked by forcing __must_check (which appears to not be able to be used directly on macro expressions). Change-Id: I635dc2f39959104ea8b475d2d5018af3502f33ba Link: http://lkml.kernel.org/r/20170206204547.GA125312@beast Signed-off-by: Kees Cook Suggested-by: Arnd Bergmann Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds Signed-off-by: Satya Tangirala --- lib/list_debug.c | 45 ++++++++++++++++++++++++--------------------- 1 file changed, 24 insertions(+), 21 deletions(-) (limited to 'lib/list_debug.c') diff --git a/lib/list_debug.c b/lib/list_debug.c index 7f7bfa55eb6d..a34db8d27667 100644 --- a/lib/list_debug.c +++ b/lib/list_debug.c @@ -20,15 +20,16 @@ bool __list_add_valid(struct list_head *new, struct list_head *prev, struct list_head *next) { - CHECK_DATA_CORRUPTION(next->prev != prev, - "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", - prev, next->prev, next); - CHECK_DATA_CORRUPTION(prev->next != next, - "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", - next, prev->next, prev); - CHECK_DATA_CORRUPTION(new == prev || new == next, - "list_add double add: new=%p, prev=%p, next=%p.\n", - new, prev, next); + if (CHECK_DATA_CORRUPTION(next->prev != prev, + "list_add corruption. next->prev should be prev (%p), but was %p. (next=%p).\n", + prev, next->prev, next) || + CHECK_DATA_CORRUPTION(prev->next != next, + "list_add corruption. prev->next should be next (%p), but was %p. (prev=%p).\n", + next, prev->next, prev) || + CHECK_DATA_CORRUPTION(new == prev || new == next, + "list_add double add: new=%p, prev=%p, next=%p.\n", + new, prev, next)) + return false; return true; } @@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry) prev = entry->prev; next = entry->next; - CHECK_DATA_CORRUPTION(next == LIST_POISON1, - "list_del corruption, %p->next is LIST_POISON1 (%p)\n", - entry, LIST_POISON1); - CHECK_DATA_CORRUPTION(prev == LIST_POISON2, - "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", - entry, LIST_POISON2); - CHECK_DATA_CORRUPTION(prev->next != entry, - "list_del corruption. prev->next should be %p, but was %p\n", - entry, prev->next); - CHECK_DATA_CORRUPTION(next->prev != entry, - "list_del corruption. next->prev should be %p, but was %p\n", - entry, next->prev); + if (CHECK_DATA_CORRUPTION(next == LIST_POISON1, + "list_del corruption, %p->next is LIST_POISON1 (%p)\n", + entry, LIST_POISON1) || + CHECK_DATA_CORRUPTION(prev == LIST_POISON2, + "list_del corruption, %p->prev is LIST_POISON2 (%p)\n", + entry, LIST_POISON2) || + CHECK_DATA_CORRUPTION(prev->next != entry, + "list_del corruption. prev->next should be %p, but was %p\n", + entry, prev->next) || + CHECK_DATA_CORRUPTION(next->prev != entry, + "list_del corruption. next->prev should be %p, but was %p\n", + entry, next->prev)) + return false; + return true; } -- cgit v1.2.3