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

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

[Kees Cook]

This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.

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

Thanks,

-Kees

v3:
- fix MSM attribution, sboyd
- use pr_err, joe

v2:
- consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
- drop non-list BUGs, labbott

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

[Kees Cook]

This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.

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

Thanks,

-Kees

v3:
- fix MSM attribution, sboyd
- use pr_err, joe

v2:
- consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
- drop non-list BUGs, labbott

[PATCH v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 v3 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 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()").

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..51a486f4eb4c 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, fmt, ...)			 \
+	do {								 \
+		if (unlikely(condition)) {				 \
+			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
+				pr_err(fmt, ##__VA_ARGS__);		 \
+				BUG();					 \
+			} else						 \
+				WARN(1, fmt, ##__VA_ARGS__);		 \
+			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 v3 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 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()").

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..51a486f4eb4c 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, fmt, ...)			 \
+	do {								 \
+		if (unlikely(condition)) {				 \
+			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
+				pr_err(fmt, ##__VA_ARGS__);		 \
+				BUG();					 \
+			} else						 \
+				WARN(1, fmt, ##__VA_ARGS__);		 \
+			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 v3 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 v3 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 v3 0/5] bug: Provide toggle for BUG on data corruption

[Steven Rostedt]

On Wed, 17 Aug 2016 14:42:07 -0700
Kees Cook <keescook@chromium.org> wrote:

> This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> 
> Along with the patches is a new lkdtm test to validate that setting
> CONFIG_DEBUG_LIST actually does what is desired.
> 

The series looks fine by me.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> Thanks,
> 
> -Kees
> 
> v3:
> - fix MSM attribution, sboyd
> - use pr_err, joe
> 
> v2:
> - consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
> - drop non-list BUGs, labbott

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

[Steven Rostedt]

On Wed, 17 Aug 2016 14:42:07 -0700
Kees Cook <keescook@chromium.org> wrote:

> This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> 
> Along with the patches is a new lkdtm test to validate that setting
> CONFIG_DEBUG_LIST actually does what is desired.
> 

The series looks fine by me.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> Thanks,
> 
> -Kees
> 
> v3:
> - fix MSM attribution, sboyd
> - use pr_err, joe
> 
> v2:
> - consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
> - drop non-list BUGs, labbott

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

[Paul E. McKenney]

On Thu, Aug 18, 2016 at 09:46:57AM -0400, Steven Rostedt wrote:
> On Wed, 17 Aug 2016 14:42:07 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> > 
> > Along with the patches is a new lkdtm test to validate that setting
> > CONFIG_DEBUG_LIST actually does what is desired.
> > 
> 
> The series looks fine by me.
> 
> Acked-by: Steven Rostedt <rostedt@goodmis.org>

Queued for further review and testing with Steven's Acked-by, thank
you, Kees!

							Thanx, Paul

> -- Steve
> 
> > Thanks,
> > 
> > -Kees
> > 
> > v3:
> > - fix MSM attribution, sboyd
> > - use pr_err, joe
> > 
> > v2:
> > - consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
> > - drop non-list BUGs, labbott
> 

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

[Paul E. McKenney]

On Thu, Aug 18, 2016 at 09:46:57AM -0400, Steven Rostedt wrote:
> On Wed, 17 Aug 2016 14:42:07 -0700
> Kees Cook <keescook@chromium.org> wrote:
> 
> > This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> > 
> > Along with the patches is a new lkdtm test to validate that setting
> > CONFIG_DEBUG_LIST actually does what is desired.
> > 
> 
> The series looks fine by me.
> 
> Acked-by: Steven Rostedt <rostedt@goodmis.org>

Queued for further review and testing with Steven's Acked-by, thank
you, Kees!

							Thanx, Paul

> -- Steve
> 
> > Thanks,
> > 
> > -Kees
> > 
> > v3:
> > - fix MSM attribution, sboyd
> > - use pr_err, joe
> > 
> > v2:
> > - consolidate printk/WARN/BUG/return logic into a CONFIG-specific macro
> > - drop non-list BUGs, labbott
> 

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

[Rik van Riel]

On Wed, 2016-08-17 at 14:42 -0700, Kees Cook wrote:
> This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> 
> Along with the patches is a new lkdtm test to validate that setting
> CONFIG_DEBUG_LIST actually does what is desired.
> 

Series looks good to me, too.

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

[Paul E. McKenney]

On Thu, Aug 18, 2016 at 01:42:55PM -0400, Rik van Riel wrote:
> On Wed, 2016-08-17 at 14:42 -0700, Kees Cook wrote:
> > This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> > 
> > Along with the patches is a new lkdtm test to validate that setting
> > CONFIG_DEBUG_LIST actually does what is desired.
> 
> Series looks good to me, too.

Reviewed-by?  Acked-by?  Ephemeral accolades?  ;-)

							Thanx, Paul

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

[Rik van Riel]

[-- Attachment #1: Type: text/plain, Size: 1578 bytes --]

On Thu, 2016-08-18 at 13:57 -0700, Paul E. McKenney wrote:
> On Thu, Aug 18, 2016 at 01:42:55PM -0400, Rik van Riel wrote:
> > On Wed, 2016-08-17 at 14:42 -0700, Kees Cook wrote:
> > > This adds CONFIG_BUG_ON_DATA_CORRUPTION 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 Syed Rameez Mustafa.
> > > 
> > > Along with the patches is a new lkdtm test to validate that
> > > setting
> > > CONFIG_DEBUG_LIST actually does what is desired.
> > 
> > Series looks good to me, too.
> 
> Reviewed-by?  Acked-by?  Ephemeral accolades?  ;-)

Acked-by: Rik van Riel <riel@redhat.com>

works, but I saw you already committed the series to
your tree, and was not sure you would add more reviews :)

-- 

All Rights Reversed.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Paul E. McKenney]

On Thu, Aug 18, 2016 at 10:53:18PM -0400, Rik van Riel wrote:
> On Thu, 2016-08-18 at 13:57 -0700, Paul E. McKenney wrote:
> > On Thu, Aug 18, 2016 at 01:42:55PM -0400, Rik van Riel wrote:
> > > On Wed, 2016-08-17 at 14:42 -0700, Kees Cook wrote:
> > > > This adds CONFIG_BUG_ON_DATA_CORRUPTION to trigger BUG()s when
> > > > the
> > > > kernel
> > > > encounters unexpected data structure integrity as currently
> > > > detected
> > > > with CONFIG_DEBUG_LIST.
> > > >=20
> > > > 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.)
> > > >=20
> > > > This is mostly a refactoring of similar code from PaX and
> > > > Grsecurity,
> > > > along with MSM kernel changes by Syed Rameez Mustafa.
> > > >=20
> > > > Along with the patches is a new lkdtm test to validate that
> > > > setting
> > > > CONFIG_DEBUG_LIST actually does what is desired.
> > >=20
> > > Series looks good to me, too.
> >=20
> > Reviewed-by?=C2=A0=C2=A0Acked-by?=C2=A0=C2=A0Ephemeral accolades?=C2=A0=
> =C2=A0;-)
> 
> Acked-by: Rik van Riel <riel@redhat.com>
> 
> works, but I saw you already committed the series to
> your tree, and was not sure you would add more reviews :)

Well, there is at least one more rebase for topic branches.  ;-)

							Thanx, Paul

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

[Arnd Bergmann]

On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
> +
> +/*
> + * 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, fmt, ...)                      \
> +       do {                                                             \
> +               if (unlikely(condition)) {                               \
> +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> +                               pr_err(fmt, ##__VA_ARGS__);              \
> +                               BUG();                                   \
> +                       } else                                           \
> +                               WARN(1, fmt, ##__VA_ARGS__);             \
> +                       return false;                                    \
> +               }                                                        \
> +       } while (0)
> +

I think the "return false" inside of the macro makes it easy to misread
what is actually going on.

How about making it a macro that returns the condition argument?

#define CHECK_DATA_CORRUPTION(condition, fmt, ...)	\
({	\
	bool _condition = unlikely(condition);	\
	if (_condition) {	\
		...
	}	\
	_condition;	\
})

	Arnd

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

[Arnd Bergmann]

On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
> +
> +/*
> + * 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, fmt, ...)                      \
> +       do {                                                             \
> +               if (unlikely(condition)) {                               \
> +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> +                               pr_err(fmt, ##__VA_ARGS__);              \
> +                               BUG();                                   \
> +                       } else                                           \
> +                               WARN(1, fmt, ##__VA_ARGS__);             \
> +                       return false;                                    \
> +               }                                                        \
> +       } while (0)
> +

I think the "return false" inside of the macro makes it easy to misread
what is actually going on.

How about making it a macro that returns the condition argument?

#define CHECK_DATA_CORRUPTION(condition, fmt, ...)	\
({	\
	bool _condition = unlikely(condition);	\
	if (_condition) {	\
		...
	}	\
	_condition;	\
})

	Arnd

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

[Paul E. McKenney]

On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote:
> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
> > +
> > +/*
> > + * 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, fmt, ...)                      \
> > +       do {                                                             \
> > +               if (unlikely(condition)) {                               \
> > +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> > +                               pr_err(fmt, ##__VA_ARGS__);              \
> > +                               BUG();                                   \
> > +                       } else                                           \
> > +                               WARN(1, fmt, ##__VA_ARGS__);             \
> > +                       return false;                                    \
> > +               }                                                        \
> > +       } while (0)
> > +
> 
> I think the "return false" inside of the macro makes it easy to misread
> what is actually going on.
> 
> How about making it a macro that returns the condition argument?
> 
> #define CHECK_DATA_CORRUPTION(condition, fmt, ...)	\
> ({	\
> 	bool _condition = unlikely(condition);	\
> 	if (_condition) {	\
> 		...
> 	}	\
> 	_condition;	\
> })

That does look better, now that you mention it.  Kees, any objections?

							Thanx, Paul

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

[Paul E. McKenney]

On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote:
> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
> > +
> > +/*
> > + * 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, fmt, ...)                      \
> > +       do {                                                             \
> > +               if (unlikely(condition)) {                               \
> > +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
> > +                               pr_err(fmt, ##__VA_ARGS__);              \
> > +                               BUG();                                   \
> > +                       } else                                           \
> > +                               WARN(1, fmt, ##__VA_ARGS__);             \
> > +                       return false;                                    \
> > +               }                                                        \
> > +       } while (0)
> > +
> 
> I think the "return false" inside of the macro makes it easy to misread
> what is actually going on.
> 
> How about making it a macro that returns the condition argument?
> 
> #define CHECK_DATA_CORRUPTION(condition, fmt, ...)	\
> ({	\
> 	bool _condition = unlikely(condition);	\
> 	if (_condition) {	\
> 		...
> 	}	\
> 	_condition;	\
> })

That does look better, now that you mention it.  Kees, any objections?

							Thanx, Paul

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

[Kees Cook]

On Mon, Aug 22, 2016 at 10:53 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote:
>> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
>> > +
>> > +/*
>> > + * 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, fmt, ...)                      \
>> > +       do {                                                             \
>> > +               if (unlikely(condition)) {                               \
>> > +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
>> > +                               pr_err(fmt, ##__VA_ARGS__);              \
>> > +                               BUG();                                   \
>> > +                       } else                                           \
>> > +                               WARN(1, fmt, ##__VA_ARGS__);             \
>> > +                       return false;                                    \
>> > +               }                                                        \
>> > +       } while (0)
>> > +
>>
>> I think the "return false" inside of the macro makes it easy to misread
>> what is actually going on.
>>
>> How about making it a macro that returns the condition argument?
>>
>> #define CHECK_DATA_CORRUPTION(condition, fmt, ...)    \
>> ({    \
>>       bool _condition = unlikely(condition);  \
>>       if (_condition) {       \
>>               ...
>>       }       \
>>       _condition;     \
>> })
>
> That does look better, now that you mention it.  Kees, any objections?

That's fine with me; it'll require changing the callers of the macros
to test their results, but that should be clean change.

-Kees

-- 
Kees Cook
Nexus Security

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

[Kees Cook]

On Mon, Aug 22, 2016 at 10:53 AM, Paul E. McKenney
<paulmck@linux.vnet.ibm.com> wrote:
> On Mon, Aug 22, 2016 at 03:15:35PM +0200, Arnd Bergmann wrote:
>> On Wednesday, August 17, 2016 2:42:11 PM CEST Kees Cook wrote:
>> > +
>> > +/*
>> > + * 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, fmt, ...)                      \
>> > +       do {                                                             \
>> > +               if (unlikely(condition)) {                               \
>> > +                       if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
>> > +                               pr_err(fmt, ##__VA_ARGS__);              \
>> > +                               BUG();                                   \
>> > +                       } else                                           \
>> > +                               WARN(1, fmt, ##__VA_ARGS__);             \
>> > +                       return false;                                    \
>> > +               }                                                        \
>> > +       } while (0)
>> > +
>>
>> I think the "return false" inside of the macro makes it easy to misread
>> what is actually going on.
>>
>> How about making it a macro that returns the condition argument?
>>
>> #define CHECK_DATA_CORRUPTION(condition, fmt, ...)    \
>> ({    \
>>       bool _condition = unlikely(condition);  \
>>       if (_condition) {       \
>>               ...
>>       }       \
>>       _condition;     \
>> })
>
> That does look better, now that you mention it.  Kees, any objections?

That's fine with me; it'll require changing the callers of the macros
to test their results, but that should be clean change.

-Kees

-- 
Kees Cook
Nexus Security