[PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

This adds a CONFIG to trigger BUG()s when the kernel encounters
unexpected data structure integrity as currently detected with
CONFIG_DEBUG_LIST.

Specifically list operations have been a target for widening flaws to gain
"write anywhere" primitives for attackers, so this also consolidates the
debug checking to avoid code and check duplication (e.g. RCU list debug
was missing a check that got added to regular list debug). It also stops
manipulations when corruption is detected, since worsening the corruption
makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
since the checks are so inexpensive.)

This is mostly a refactoring of similar code from PaX and Grsecurity,
along with MSM kernel changes by Stephen Boyd.

Along with the patches is a new lkdtm test to validate that setting
CONFIG_DEBUG_LIST actually does what is desired.

Thanks,

-Kees

[kernel-hardening] [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

This adds a CONFIG to trigger BUG()s when the kernel encounters
unexpected data structure integrity as currently detected with
CONFIG_DEBUG_LIST.

Specifically list operations have been a target for widening flaws to gain
"write anywhere" primitives for attackers, so this also consolidates the
debug checking to avoid code and check duplication (e.g. RCU list debug
was missing a check that got added to regular list debug). It also stops
manipulations when corruption is detected, since worsening the corruption
makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
since the checks are so inexpensive.)

This is mostly a refactoring of similar code from PaX and Grsecurity,
along with MSM kernel changes by Stephen Boyd.

Along with the patches is a new lkdtm test to validate that setting
CONFIG_DEBUG_LIST actually does what is desired.

Thanks,

-Kees

[PATCH v2 1/5] list: Split list_add() debug checking into separate function

[Kees Cook]

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 <keescook@chromium.org>
---
 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 <linux/export.h>
@@ -13,33 +12,32 @@
 #include <linux/rculist.h>
 
 /*
- * 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;
+	}
+	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)
 {
-- 
2.7.4

[kernel-hardening] [PATCH v2 1/5] list: Split list_add() debug checking into separate function

[Kees Cook]

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 <keescook@chromium.org>
---
 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 <linux/export.h>
@@ -13,33 +12,32 @@
 #include <linux/rculist.h>
 
 /*
- * 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;
+	}
+	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)
 {
-- 
2.7.4

[PATCH v2 2/5] rculist: Consolidate DEBUG_LIST for list_add_rcu()

[Kees Cook]

Consolidates the debug checking for list_add_rcu() into the new single
debug function. Notably, this fixes the sanity check that was added in
 commit 17a801f4bfeb ("list_debug: WARN for adding something already in
the list"). Before, it wasn't being checked for RCU lists.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/rculist.h |  8 +++-----
 lib/list_debug.c        | 19 -------------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 8beb98dcf14f..4f7a9561b8c4 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -45,19 +45,17 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
  * This is only for internal list manipulation where we know
  * the prev/next entries already!
  */
-#ifndef CONFIG_DEBUG_LIST
 static inline void __list_add_rcu(struct list_head *new,
 		struct list_head *prev, struct list_head *next)
 {
+	if (!__list_add_valid(new, prev, next))
+		return;
+
 	new->next = next;
 	new->prev = prev;
 	rcu_assign_pointer(list_next_rcu(prev), new);
 	next->prev = new;
 }
-#else
-void __list_add_rcu(struct list_head *new,
-		    struct list_head *prev, struct list_head *next);
-#endif
 
 /**
  * list_add_rcu - add a new entry to rcu-protected list
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);
-- 
2.7.4

[kernel-hardening] [PATCH v2 2/5] rculist: Consolidate DEBUG_LIST for list_add_rcu()

[Kees Cook]

Consolidates the debug checking for list_add_rcu() into the new single
debug function. Notably, this fixes the sanity check that was added in
 commit 17a801f4bfeb ("list_debug: WARN for adding something already in
the list"). Before, it wasn't being checked for RCU lists.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/rculist.h |  8 +++-----
 lib/list_debug.c        | 19 -------------------
 2 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/include/linux/rculist.h b/include/linux/rculist.h
index 8beb98dcf14f..4f7a9561b8c4 100644
--- a/include/linux/rculist.h
+++ b/include/linux/rculist.h
@@ -45,19 +45,17 @@ static inline void INIT_LIST_HEAD_RCU(struct list_head *list)
  * This is only for internal list manipulation where we know
  * the prev/next entries already!
  */
-#ifndef CONFIG_DEBUG_LIST
 static inline void __list_add_rcu(struct list_head *new,
 		struct list_head *prev, struct list_head *next)
 {
+	if (!__list_add_valid(new, prev, next))
+		return;
+
 	new->next = next;
 	new->prev = prev;
 	rcu_assign_pointer(list_next_rcu(prev), new);
 	next->prev = new;
 }
-#else
-void __list_add_rcu(struct list_head *new,
-		    struct list_head *prev, struct list_head *next);
-#endif
 
 /**
  * list_add_rcu - add a new entry to rcu-protected list
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);
-- 
2.7.4

[PATCH v2 3/5] list: Split list_del() debug checking into separate function

[Kees Cook]

Similar to the list_add() debug consolidation, this consolidates the
debug checking performed during CONFIG_DEBUG_LIST, and stops list
updates when corruption is found.

Refactored from same hardening in PaX and Grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/list.h | 15 +++++++++------
 lib/list_debug.c     | 53 +++++++++++++++++++++++-----------------------------
 2 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index 0ed58591538e..569c1cf80c64 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -32,6 +32,7 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
 extern bool __list_add_valid(struct list_head *new,
 			      struct list_head *prev,
 			      struct list_head *next);
+extern bool __list_del_entry_valid(struct list_head *entry);
 #else
 static inline bool __list_add_valid(struct list_head *new,
 				struct list_head *prev,
@@ -39,6 +40,10 @@ static inline bool __list_add_valid(struct list_head *new,
 {
 	return true;
 }
+static inline bool __list_del_entry_valid(struct list_head *entry)
+{
+	return true;
+}
 #endif
 
 /*
@@ -106,22 +111,20 @@ static inline void __list_del(struct list_head * prev, struct list_head * next)
  * Note: list_empty() on entry does not return true after this, the entry is
  * in an undefined state.
  */
-#ifndef CONFIG_DEBUG_LIST
 static inline void __list_del_entry(struct list_head *entry)
 {
+	if (!__list_del_entry_valid(entry))
+		return;
+
 	__list_del(entry->prev, entry->next);
 }
 
 static inline void list_del(struct list_head *entry)
 {
-	__list_del(entry->prev, entry->next);
+	__list_del_entry(entry);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;
 }
-#else
-extern void __list_del_entry(struct list_head *entry);
-extern void list_del(struct list_head *entry);
-#endif
 
 /**
  * list_replace - replace old entry by new one
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);
-- 
2.7.4

[kernel-hardening] [PATCH v2 3/5] list: Split list_del() debug checking into separate function

[Kees Cook]

Similar to the list_add() debug consolidation, this consolidates the
debug checking performed during CONFIG_DEBUG_LIST, and stops list
updates when corruption is found.

Refactored from same hardening in PaX and Grsecurity.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/list.h | 15 +++++++++------
 lib/list_debug.c     | 53 +++++++++++++++++++++++-----------------------------
 2 files changed, 32 insertions(+), 36 deletions(-)

diff --git a/include/linux/list.h b/include/linux/list.h
index 0ed58591538e..569c1cf80c64 100644
--- a/include/linux/list.h
+++ b/include/linux/list.h
@@ -32,6 +32,7 @@ static inline void INIT_LIST_HEAD(struct list_head *list)
 extern bool __list_add_valid(struct list_head *new,
 			      struct list_head *prev,
 			      struct list_head *next);
+extern bool __list_del_entry_valid(struct list_head *entry);
 #else
 static inline bool __list_add_valid(struct list_head *new,
 				struct list_head *prev,
@@ -39,6 +40,10 @@ static inline bool __list_add_valid(struct list_head *new,
 {
 	return true;
 }
+static inline bool __list_del_entry_valid(struct list_head *entry)
+{
+	return true;
+}
 #endif
 
 /*
@@ -106,22 +111,20 @@ static inline void __list_del(struct list_head * prev, struct list_head * next)
  * Note: list_empty() on entry does not return true after this, the entry is
  * in an undefined state.
  */
-#ifndef CONFIG_DEBUG_LIST
 static inline void __list_del_entry(struct list_head *entry)
 {
+	if (!__list_del_entry_valid(entry))
+		return;
+
 	__list_del(entry->prev, entry->next);
 }
 
 static inline void list_del(struct list_head *entry)
 {
-	__list_del(entry->prev, entry->next);
+	__list_del_entry(entry);
 	entry->next = LIST_POISON1;
 	entry->prev = LIST_POISON2;
 }
-#else
-extern void __list_del_entry(struct list_head *entry);
-extern void list_del(struct list_head *entry);
-#endif
 
 /**
  * list_replace - replace old entry by new one
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);
-- 
2.7.4

[PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

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 Stephen Boyd 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()").

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 17 ++++++++++++++++
 lib/Kconfig.debug   | 10 ++++++++++
 lib/list_debug.c    | 57 +++++++++++++++++++++--------------------------------
 3 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index e51b0709e78d..011e8e95aa0e 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 }
 
 #endif	/* CONFIG_GENERIC_BUG */
+
+/*
+ * Since detected data corruption should stop operation on the affected
+ * structures, this returns false if the corruption condition is found.
+ */
+#define CHECK_DATA_CORRUPTION(condition, format...)			 \
+	do {								 \
+		if (unlikely(condition)) {				 \
+			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
+				printk(KERN_ERR format);		 \
+				BUG();					 \
+			} else						 \
+				WARN(1, format);			 \
+			return false;					 \
+		}							 \
+	} while (0)
+
 #endif	/* _LINUX_BUG_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2307d7c89dac..58d358a4c7f3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1987,6 +1987,16 @@ config TEST_STATIC_KEYS
 
 	  If unsure, say N.
 
+config BUG_ON_DATA_CORRUPTION
+	bool "Trigger a BUG when data corruption is detected"
+	select CONFIG_DEBUG_LIST
+	help
+	  Select this option if the kernel should BUG when it encounters
+	  data corruption in kernel memory structures when they get checked
+	  for validity.
+
+	  If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
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;
 
 }
-- 
2.7.4

[kernel-hardening] [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

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 Stephen Boyd 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()").

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 17 ++++++++++++++++
 lib/Kconfig.debug   | 10 ++++++++++
 lib/list_debug.c    | 57 +++++++++++++++++++++--------------------------------
 3 files changed, 49 insertions(+), 35 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index e51b0709e78d..011e8e95aa0e 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 }
 
 #endif	/* CONFIG_GENERIC_BUG */
+
+/*
+ * Since detected data corruption should stop operation on the affected
+ * structures, this returns false if the corruption condition is found.
+ */
+#define CHECK_DATA_CORRUPTION(condition, format...)			 \
+	do {								 \
+		if (unlikely(condition)) {				 \
+			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
+				printk(KERN_ERR format);		 \
+				BUG();					 \
+			} else						 \
+				WARN(1, format);			 \
+			return false;					 \
+		}							 \
+	} while (0)
+
 #endif	/* _LINUX_BUG_H */
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2307d7c89dac..58d358a4c7f3 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1987,6 +1987,16 @@ config TEST_STATIC_KEYS
 
 	  If unsure, say N.
 
+config BUG_ON_DATA_CORRUPTION
+	bool "Trigger a BUG when data corruption is detected"
+	select CONFIG_DEBUG_LIST
+	help
+	  Select this option if the kernel should BUG when it encounters
+	  data corruption in kernel memory structures when they get checked
+	  for validity.
+
+	  If unsure, say N.
+
 source "samples/Kconfig"
 
 source "lib/Kconfig.kgdb"
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;
 
 }
-- 
2.7.4

[PATCH v2 5/5] lkdtm: Add tests for struct list corruption

[Kees Cook]

When building under CONFIG_DEBUG_LIST, list addition and removal will be
sanity-checked. This validates that the check is working as expected by
setting up classic corruption attacks against list manipulations, available
with the new lkdtm tests CORRUPT_LIST_ADD and CORRUPT_LIST_DEL.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.h      |  2 ++
 drivers/misc/lkdtm_bugs.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/lkdtm_core.c |  2 ++
 3 files changed, 72 insertions(+)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index fdf954c2107f..cfa1039c62e7 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -21,6 +21,8 @@ void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
 void lkdtm_ATOMIC_UNDERFLOW(void);
 void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_CORRUPT_LIST_ADD(void);
+void lkdtm_CORRUPT_LIST_DEL(void);
 
 /* lkdtm_heap.c */
 void lkdtm_OVERWRITE_ALLOCATION(void);
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index 182ae1894b32..f336206d4b1f 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -5,8 +5,13 @@
  * test source files.
  */
 #include "lkdtm.h"
+#include <linux/list.h>
 #include <linux/sched.h>
 
+struct lkdtm_list {
+	struct list_head node;
+};
+
 /*
  * Make sure our attempts to over run the kernel stack doesn't trigger
  * a compiler warning when CONFIG_FRAME_WARN is set. Then make sure we
@@ -146,3 +151,66 @@ void lkdtm_ATOMIC_OVERFLOW(void)
 	pr_info("attempting bad atomic overflow\n");
 	atomic_inc(&over);
 }
+
+void lkdtm_CORRUPT_LIST_ADD(void)
+{
+	/*
+	 * Initially, an empty list via LIST_HEAD:
+	 *	test_head.next = &test_head
+	 *	test_head.prev = &test_head
+	 */
+	LIST_HEAD(test_head);
+	struct lkdtm_list good, bad;
+	void *target[2] = { };
+	void *redirection = &target;
+
+	pr_info("attempting good list addition\n");
+
+	/*
+	 * Adding to the list performs these actions:
+	 *	test_head.next->prev = &good.node
+	 *	good.node.next = test_head.next
+	 *	good.node.prev = test_head
+	 *	test_head.next = good.node
+	 */
+	list_add(&good.node, &test_head);
+
+	pr_info("attempting corrupted list addition\n");
+	/*
+	 * In simulating this "write what where" primitive, the "what" is
+	 * the address of &bad.node, and the "where" is the address held
+	 * by "redirection".
+	 */
+	test_head.next = redirection;
+	list_add(&bad.node, &test_head);
+
+	if (target[0] == NULL && target[1] == NULL)
+		pr_err("Overwrite did not happen, but no BUG?!\n");
+	else
+		pr_err("list_add() corruption not detected!\n");
+}
+
+void lkdtm_CORRUPT_LIST_DEL(void)
+{
+	LIST_HEAD(test_head);
+	struct lkdtm_list item;
+	void *target[2] = { };
+	void *redirection = &target;
+
+	list_add(&item.node, &test_head);
+
+	pr_info("attempting good list removal\n");
+	list_del(&item.node);
+
+	pr_info("attempting corrupted list removal\n");
+	list_add(&item.node, &test_head);
+
+	/* As with the list_add() test above, this corrupts "next". */
+	item.node.next = redirection;
+	list_del(&item.node);
+
+	if (target[0] == NULL && target[1] == NULL)
+		pr_err("Overwrite did not happen, but no BUG?!\n");
+	else
+		pr_err("list_del() corruption not detected!\n");
+}
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index f9154b8d67f6..7eeb71a75549 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -197,6 +197,8 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(EXCEPTION),
 	CRASHTYPE(LOOP),
 	CRASHTYPE(OVERFLOW),
+	CRASHTYPE(CORRUPT_LIST_ADD),
+	CRASHTYPE(CORRUPT_LIST_DEL),
 	CRASHTYPE(CORRUPT_STACK),
 	CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
 	CRASHTYPE(OVERWRITE_ALLOCATION),
-- 
2.7.4

[kernel-hardening] [PATCH v2 5/5] lkdtm: Add tests for struct list corruption

[Kees Cook]

When building under CONFIG_DEBUG_LIST, list addition and removal will be
sanity-checked. This validates that the check is working as expected by
setting up classic corruption attacks against list manipulations, available
with the new lkdtm tests CORRUPT_LIST_ADD and CORRUPT_LIST_DEL.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.h      |  2 ++
 drivers/misc/lkdtm_bugs.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/lkdtm_core.c |  2 ++
 3 files changed, 72 insertions(+)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index fdf954c2107f..cfa1039c62e7 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -21,6 +21,8 @@ void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
 void lkdtm_ATOMIC_UNDERFLOW(void);
 void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_CORRUPT_LIST_ADD(void);
+void lkdtm_CORRUPT_LIST_DEL(void);
 
 /* lkdtm_heap.c */
 void lkdtm_OVERWRITE_ALLOCATION(void);
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index 182ae1894b32..f336206d4b1f 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -5,8 +5,13 @@
  * test source files.
  */
 #include "lkdtm.h"
+#include <linux/list.h>
 #include <linux/sched.h>
 
+struct lkdtm_list {
+	struct list_head node;
+};
+
 /*
  * Make sure our attempts to over run the kernel stack doesn't trigger
  * a compiler warning when CONFIG_FRAME_WARN is set. Then make sure we
@@ -146,3 +151,66 @@ void lkdtm_ATOMIC_OVERFLOW(void)
 	pr_info("attempting bad atomic overflow\n");
 	atomic_inc(&over);
 }
+
+void lkdtm_CORRUPT_LIST_ADD(void)
+{
+	/*
+	 * Initially, an empty list via LIST_HEAD:
+	 *	test_head.next = &test_head
+	 *	test_head.prev = &test_head
+	 */
+	LIST_HEAD(test_head);
+	struct lkdtm_list good, bad;
+	void *target[2] = { };
+	void *redirection = &target;
+
+	pr_info("attempting good list addition\n");
+
+	/*
+	 * Adding to the list performs these actions:
+	 *	test_head.next->prev = &good.node
+	 *	good.node.next = test_head.next
+	 *	good.node.prev = test_head
+	 *	test_head.next = good.node
+	 */
+	list_add(&good.node, &test_head);
+
+	pr_info("attempting corrupted list addition\n");
+	/*
+	 * In simulating this "write what where" primitive, the "what" is
+	 * the address of &bad.node, and the "where" is the address held
+	 * by "redirection".
+	 */
+	test_head.next = redirection;
+	list_add(&bad.node, &test_head);
+
+	if (target[0] == NULL && target[1] == NULL)
+		pr_err("Overwrite did not happen, but no BUG?!\n");
+	else
+		pr_err("list_add() corruption not detected!\n");
+}
+
+void lkdtm_CORRUPT_LIST_DEL(void)
+{
+	LIST_HEAD(test_head);
+	struct lkdtm_list item;
+	void *target[2] = { };
+	void *redirection = &target;
+
+	list_add(&item.node, &test_head);
+
+	pr_info("attempting good list removal\n");
+	list_del(&item.node);
+
+	pr_info("attempting corrupted list removal\n");
+	list_add(&item.node, &test_head);
+
+	/* As with the list_add() test above, this corrupts "next". */
+	item.node.next = redirection;
+	list_del(&item.node);
+
+	if (target[0] == NULL && target[1] == NULL)
+		pr_err("Overwrite did not happen, but no BUG?!\n");
+	else
+		pr_err("list_del() corruption not detected!\n");
+}
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index f9154b8d67f6..7eeb71a75549 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -197,6 +197,8 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(EXCEPTION),
 	CRASHTYPE(LOOP),
 	CRASHTYPE(OVERFLOW),
+	CRASHTYPE(CORRUPT_LIST_ADD),
+	CRASHTYPE(CORRUPT_LIST_DEL),
 	CRASHTYPE(CORRUPT_STACK),
 	CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
 	CRASHTYPE(OVERWRITE_ALLOCATION),
-- 
2.7.4

Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Joe Perches]

On Tue, 2016-08-16 at 17:20 -0700, Kees Cook wrote:
> 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.
[]
> diff --git a/include/linux/bug.h b/include/linux/bug.h
[]
> @@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
>  }
>  
>  #endif	/* CONFIG_GENERIC_BUG */
> +
> +/*
> + * Since detected data corruption should stop operation on the affected
> + * structures, this returns false if the corruption condition is found.
> + */
> +#define CHECK_DATA_CORRUPTION(condition, format...)			 \

My preference would be to use (condition, fmt, ...)

> +	do {								 \
> +		if (unlikely(condition)) {				 \
> +			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> +				printk(KERN_ERR format);		 \

and
				pr_err(fmt, ##__VA_ARGS__);

so that any use would also get any local pr_fmt applied as well.

> +				BUG();					 \
> +			} else						 \
> +				WARN(1, format);			 \
> +			return false;					 \
> +		}							 \
> +	} while (0)
> +
>  #endif	/* _LINUX_BUG_H */

[kernel-hardening] Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Joe Perches]

On Tue, 2016-08-16 at 17:20 -0700, Kees Cook wrote:
> 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.
[]
> diff --git a/include/linux/bug.h b/include/linux/bug.h
[]
> @@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
>  }
>  
>  #endif	/* CONFIG_GENERIC_BUG */
> +
> +/*
> + * Since detected data corruption should stop operation on the affected
> + * structures, this returns false if the corruption condition is found.
> + */
> +#define CHECK_DATA_CORRUPTION(condition, format...)			 \

My preference would be to use (condition, fmt, ...)

> +	do {								 \
> +		if (unlikely(condition)) {				 \
> +			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> +				printk(KERN_ERR format);		 \

and
				pr_err(fmt, ##__VA_ARGS__);

so that any use would also get any local pr_fmt applied as well.

> +				BUG();					 \
> +			} else						 \
> +				WARN(1, format);			 \
> +			return false;					 \
> +		}							 \
> +	} while (0)
> +
>  #endif	/* _LINUX_BUG_H */

Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Henrique de Moraes Holschuh]

On Tue, 16 Aug 2016, Kees Cook wrote:
> This adds a CONFIG to trigger BUG()s when the kernel encounters
> unexpected data structure integrity as currently detected with
> CONFIG_DEBUG_LIST.
> 
> Specifically list operations have been a target for widening flaws to gain
> "write anywhere" primitives for attackers, so this also consolidates the
> debug checking to avoid code and check duplication (e.g. RCU list debug
> was missing a check that got added to regular list debug). It also stops
> manipulations when corruption is detected, since worsening the corruption
> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
> since the checks are so inexpensive.)

Well, maybe it wants a name that it looks like something that should be
enabled by default on production kernels?

I.e. CONFIG_DETECT_LIST_CORRUPTION or somesuch?

-- 
  Henrique Holschuh

[kernel-hardening] Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Henrique de Moraes Holschuh]

On Tue, 16 Aug 2016, Kees Cook wrote:
> This adds a CONFIG to trigger BUG()s when the kernel encounters
> unexpected data structure integrity as currently detected with
> CONFIG_DEBUG_LIST.
> 
> Specifically list operations have been a target for widening flaws to gain
> "write anywhere" primitives for attackers, so this also consolidates the
> debug checking to avoid code and check duplication (e.g. RCU list debug
> was missing a check that got added to regular list debug). It also stops
> manipulations when corruption is detected, since worsening the corruption
> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
> since the checks are so inexpensive.)

Well, maybe it wants a name that it looks like something that should be
enabled by default on production kernels?

I.e. CONFIG_DETECT_LIST_CORRUPTION or somesuch?

-- 
  Henrique Holschuh

Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Tue, Aug 16, 2016 at 5:55 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 16 Aug 2016, Kees Cook wrote:
>> This adds a CONFIG to trigger BUG()s when the kernel encounters
>> unexpected data structure integrity as currently detected with
>> CONFIG_DEBUG_LIST.
>>
>> Specifically list operations have been a target for widening flaws to gain
>> "write anywhere" primitives for attackers, so this also consolidates the
>> debug checking to avoid code and check duplication (e.g. RCU list debug
>> was missing a check that got added to regular list debug). It also stops
>> manipulations when corruption is detected, since worsening the corruption
>> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
>> since the checks are so inexpensive.)
>
> Well, maybe it wants a name that it looks like something that should be
> enabled by default on production kernels?
>
> I.e. CONFIG_DETECT_LIST_CORRUPTION or somesuch?

Yeah, that very well be true. I'd currently like to avoid CONFIG name
churn, but I've added it to my list of CONFIGs to rename (along with
CONFIG_DEBUG_RODATA). :)

-Kees

-- 
Kees Cook
Nexus Security

[kernel-hardening] Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Tue, Aug 16, 2016 at 5:55 PM, Henrique de Moraes Holschuh
<hmh@hmh.eng.br> wrote:
> On Tue, 16 Aug 2016, Kees Cook wrote:
>> This adds a CONFIG to trigger BUG()s when the kernel encounters
>> unexpected data structure integrity as currently detected with
>> CONFIG_DEBUG_LIST.
>>
>> Specifically list operations have been a target for widening flaws to gain
>> "write anywhere" primitives for attackers, so this also consolidates the
>> debug checking to avoid code and check duplication (e.g. RCU list debug
>> was missing a check that got added to regular list debug). It also stops
>> manipulations when corruption is detected, since worsening the corruption
>> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
>> since the checks are so inexpensive.)
>
> Well, maybe it wants a name that it looks like something that should be
> enabled by default on production kernels?
>
> I.e. CONFIG_DETECT_LIST_CORRUPTION or somesuch?

Yeah, that very well be true. I'd currently like to avoid CONFIG name
churn, but I've added it to my list of CONFIGs to rename (along with
CONFIG_DEBUG_RODATA). :)

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Tue, Aug 16, 2016 at 5:26 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-08-16 at 17:20 -0700, Kees Cook wrote:
>> 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.
> []
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
> []
>> @@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
>>  }
>>
>>  #endif       /* CONFIG_GENERIC_BUG */
>> +
>> +/*
>> + * Since detected data corruption should stop operation on the affected
>> + * structures, this returns false if the corruption condition is found.
>> + */
>> +#define CHECK_DATA_CORRUPTION(condition, format...)                   \
>
> My preference would be to use (condition, fmt, ...)
>
>> +     do {                                                             \
>> +             if (unlikely(condition)) {                               \
>> +                     if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
>> +                             printk(KERN_ERR format);                 \
>
> and
>                                 pr_err(fmt, ##__VA_ARGS__);
>
> so that any use would also get any local pr_fmt applied as well.
>
>> +                             BUG();                                   \
>> +                     } else                                           \
>> +                             WARN(1, format);                         \
>> +                     return false;                                    \
>> +             }                                                        \
>> +     } while (0)
>> +
>>  #endif       /* _LINUX_BUG_H */
>

Ah yes, excellent point. I'll convert this for my v3. Thanks!

-Kees

-- 
Kees Cook
Nexus Security

[kernel-hardening] Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Tue, Aug 16, 2016 at 5:26 PM, Joe Perches <joe@perches.com> wrote:
> On Tue, 2016-08-16 at 17:20 -0700, Kees Cook wrote:
>> 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.
> []
>> diff --git a/include/linux/bug.h b/include/linux/bug.h
> []
>> @@ -118,4 +118,21 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
>>  }
>>
>>  #endif       /* CONFIG_GENERIC_BUG */
>> +
>> +/*
>> + * Since detected data corruption should stop operation on the affected
>> + * structures, this returns false if the corruption condition is found.
>> + */
>> +#define CHECK_DATA_CORRUPTION(condition, format...)                   \
>
> My preference would be to use (condition, fmt, ...)
>
>> +     do {                                                             \
>> +             if (unlikely(condition)) {                               \
>> +                     if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
>> +                             printk(KERN_ERR format);                 \
>
> and
>                                 pr_err(fmt, ##__VA_ARGS__);
>
> so that any use would also get any local pr_fmt applied as well.
>
>> +                             BUG();                                   \
>> +                     } else                                           \
>> +                             WARN(1, format);                         \
>> +                     return false;                                    \
>> +             }                                                        \
>> +     } while (0)
>> +
>>  #endif       /* _LINUX_BUG_H */
>

Ah yes, excellent point. I'll convert this for my v3. Thanks!

-Kees

-- 
Kees Cook
Nexus Security

Re: [PATCH v2 1/5] list: Split list_add() debug checking into separate function

[Steven Rostedt]

On Tue, 16 Aug 2016 17:20:25 -0700
Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org>
> ---
>  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 <linux/export.h>
> @@ -13,33 +12,32 @@
>  #include <linux/rculist.h>
>  
>  /*
> - * 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)
>  {

[kernel-hardening] Re: [PATCH v2 1/5] list: Split list_add() debug checking into separate function

[Steven Rostedt]

On Tue, 16 Aug 2016 17:20:25 -0700
Kees Cook <keescook@chromium.org> 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 <keescook@chromium.org>
> ---
>  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 <linux/export.h>
> @@ -13,33 +12,32 @@
>  #include <linux/rculist.h>
>  
>  /*
> - * 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)
>  {

Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Steven Rostedt]

On Tue, 16 Aug 2016 17:20:28 -0700
Kees Cook <keescook@chromium.org> wrote:


>  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);

OK, you totally rewrote the WARN() section anyway, thus ignore my
comment on the previous email.

-- Steve

>  	return true;
>  
>  }

[kernel-hardening] Re: [PATCH v2 4/5] bug: Provide toggle for BUG on data corruption

[Steven Rostedt]

On Tue, 16 Aug 2016 17:20:28 -0700
Kees Cook <keescook@chromium.org> wrote:


>  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);

OK, you totally rewrote the WARN() section anyway, thus ignore my
comment on the previous email.

-- Steve

>  	return true;
>  
>  }

Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Stephen Boyd]

On 08/16, Kees Cook wrote:
> This adds a CONFIG to trigger BUG()s when the kernel encounters
> unexpected data structure integrity as currently detected with
> CONFIG_DEBUG_LIST.
> 
> Specifically list operations have been a target for widening flaws to gain
> "write anywhere" primitives for attackers, so this also consolidates the
> debug checking to avoid code and check duplication (e.g. RCU list debug
> was missing a check that got added to regular list debug). It also stops
> manipulations when corruption is detected, since worsening the corruption
> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
> since the checks are so inexpensive.)
> 
> This is mostly a refactoring of similar code from PaX and Grsecurity,
> along with MSM kernel changes by Stephen Boyd.

Which commit in the MSM kernel from me? I wonder if perhaps
you're thinking of a patch from Syed Rameez Mustafa like commit
1c014f321e6d67f47 in the msm-3.4 kernel.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[kernel-hardening] Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Stephen Boyd]

On 08/16, Kees Cook wrote:
> This adds a CONFIG to trigger BUG()s when the kernel encounters
> unexpected data structure integrity as currently detected with
> CONFIG_DEBUG_LIST.
> 
> Specifically list operations have been a target for widening flaws to gain
> "write anywhere" primitives for attackers, so this also consolidates the
> debug checking to avoid code and check duplication (e.g. RCU list debug
> was missing a check that got added to regular list debug). It also stops
> manipulations when corruption is detected, since worsening the corruption
> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
> since the checks are so inexpensive.)
> 
> This is mostly a refactoring of similar code from PaX and Grsecurity,
> along with MSM kernel changes by Stephen Boyd.

Which commit in the MSM kernel from me? I wonder if perhaps
you're thinking of a patch from Syed Rameez Mustafa like commit
1c014f321e6d67f47 in the msm-3.4 kernel.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Wed, Aug 17, 2016 at 1:17 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/16, Kees Cook wrote:
>> This adds a CONFIG to trigger BUG()s when the kernel encounters
>> unexpected data structure integrity as currently detected with
>> CONFIG_DEBUG_LIST.
>>
>> Specifically list operations have been a target for widening flaws to gain
>> "write anywhere" primitives for attackers, so this also consolidates the
>> debug checking to avoid code and check duplication (e.g. RCU list debug
>> was missing a check that got added to regular list debug). It also stops
>> manipulations when corruption is detected, since worsening the corruption
>> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
>> since the checks are so inexpensive.)
>>
>> This is mostly a refactoring of similar code from PaX and Grsecurity,
>> along with MSM kernel changes by Stephen Boyd.
>
> Which commit in the MSM kernel from me? I wonder if perhaps
> you're thinking of a patch from Syed Rameez Mustafa like commit
> 1c014f321e6d67f47 in the msm-3.4 kernel.

Ooof. I can't read. Yes, you were the committer, not the author! Whoops!

https://android.googlesource.com/kernel/msm/+/7b49b86d3aa3d0c6400454a346bad1bbdf0cc78f%5E%21/#F0

I will adjust the changelog. Thanks!

-Kees

-- 
Kees Cook
Nexus Security

[kernel-hardening] Re: [PATCH v2 0/5] bug: Provide toggle for BUG on data corruption

[Kees Cook]

On Wed, Aug 17, 2016 at 1:17 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 08/16, Kees Cook wrote:
>> This adds a CONFIG to trigger BUG()s when the kernel encounters
>> unexpected data structure integrity as currently detected with
>> CONFIG_DEBUG_LIST.
>>
>> Specifically list operations have been a target for widening flaws to gain
>> "write anywhere" primitives for attackers, so this also consolidates the
>> debug checking to avoid code and check duplication (e.g. RCU list debug
>> was missing a check that got added to regular list debug). It also stops
>> manipulations when corruption is detected, since worsening the corruption
>> makes no sense. (Really, everyone should build with CONFIG_DEBUG_LIST
>> since the checks are so inexpensive.)
>>
>> This is mostly a refactoring of similar code from PaX and Grsecurity,
>> along with MSM kernel changes by Stephen Boyd.
>
> Which commit in the MSM kernel from me? I wonder if perhaps
> you're thinking of a patch from Syed Rameez Mustafa like commit
> 1c014f321e6d67f47 in the msm-3.4 kernel.

Ooof. I can't read. Yes, you were the committer, not the author! Whoops!

https://android.googlesource.com/kernel/msm/+/7b49b86d3aa3d0c6400454a346bad1bbdf0cc78f%5E%21/#F0

I will adjust the changelog. Thanks!

-Kees

-- 
Kees Cook
Nexus Security