Skip to contentSkip to conversation

RCU list updates for 4.9

PATCH New Paul E. McKenney 1 participant 8 messages Aug 22, 2016

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

cover RCU list updates for 4.9

Hello!

This series provides updates to RCU-protected linked lists:

1. Expand list_first_entry_or_null() to allow better compiler
optimizations, courtesy of Chris Wilson.

2-6. Allow security-conscious builds to specify BUG rather than WARN
for linked-list corruption, courtesy of Kees Cook.

7. Fix a bogus CONFIG_-prefixed Kconfig-option reference in #5
above, courtesy of Valentin Rothberg.

Thanx, Paul

#1 lore raw

PATCH 1/7 list: Expand list_first_entry_or_null()

From: Chris Wilson <[email protected]>

Due to the use of READ_ONCE() in list_empty() the compiler cannot optimise !list_empty() ? list_first_entry() : NULL very well. By manually expanding list_first_entry_or_null() we can take advantage of the READ_ONCE() to avoid the list element changing under the test while the compiler can generate smaller code.

Cc "Paul E. McKenney" <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Dan Williams <[email protected]>
Cc Jan Kara <[email protected]>
Cc Josef Bacik <[email protected]>
---
 include/linux/list.h | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)
include/linux/list.h +5 -2
index 5183138aa932..5809e9a2de5b 100644
@@ -381,8 +381,11 @@ static inline void list_splice_tail_init(struct list_head *list,
  *
  * Note that if the list is empty, it returns NULL.
  */
-#define list_first_entry_or_null(ptr, type, member) \
-	(!list_empty(ptr) ? list_first_entry(ptr, type, member) : NULL)
+#define list_first_entry_or_null(ptr, type, member) ({ \
+	struct list_head *head__ = (ptr); \
+	struct list_head *pos__ = READ_ONCE(head__->next); \
+	pos__ != head__ ? list_entry(pos__, type, member) : NULL; \
+})
 
 /**
  * list_next_entry - get the next element in list
Signature · 1 line
2.5.2
#2 lore raw

PATCH 2/7 list: Split list_add() debug checking into separate function

In reply to Paul E. McKenney

From: Kees Cook <[email protected]>

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.

---
 include/linux/list.h | 22 ++++++++++++++++------
 lib/list_debug.c     | 48 +++++++++++++++++++++++-------------------------
 2 files changed, 39 insertions(+), 31 deletions(-)
include/linux/list.h +16 -6
index 5809e9a2de5b..b6da9b1dce4d 100644
@@ -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
lib/list_debug.c +23 -25

64 lines not on this page · raw message

Signature · 1 line
2.5.2
#3 lore raw

PATCH 3/7 rculist: Consolidate DEBUG_LIST for list_add_rcu()

In reply to Paul E. McKenney

From: Kees Cook <[email protected]>

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.

---
 include/linux/rculist.h |  8 +++-----
 lib/list_debug.c        | 19 -------------------
 2 files changed, 3 insertions(+), 24 deletions(-)
include/linux/rculist.h +3 -5
index 8beb98dcf14f..4f7a9561b8c4 100644
@@ -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
lib/list_debug.c +0 -19
index 149dd57b583b..d0b89b9d0736 100644
@@ -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);
Signature · 1 line
2.5.2
#4 lore raw

PATCH 4/7 list: Split list_del() debug checking into separate function

In reply to Paul E. McKenney

From: Kees Cook <[email protected]>

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.

---
 include/linux/list.h | 15 +++++++++------
 lib/list_debug.c     | 53 +++++++++++++++++++++++-----------------------------
 2 files changed, 32 insertions(+), 36 deletions(-)
include/linux/list.h +9 -6
index b6da9b1dce4d..d1039ecaf94f 100644
@@ -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
lib/list_debug.c +23 -30

64 lines not on this page · raw message

Signature · 1 line
2.5.2
#5 lore raw

PATCH 5/7 bug: Provide toggle for BUG on data corruption

In reply to Paul E. McKenney

From: Kees Cook <[email protected]>

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

---
 include/linux/bug.h | 17 ++++++++++++++++
 lib/Kconfig.debug   | 10 ++++++++++
 lib/list_debug.c    | 57 +++++++++++++++++++++--------------------------------
 3 files changed, 49 insertions(+), 35 deletions(-)
include/linux/bug.h +17 -0
index e51b0709e78d..51a486f4eb4c 100644
@@ -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 */
lib/Kconfig.debug +10 -0
index 2307d7c89dac..58d358a4c7f3 100644
@@ -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"
lib/list_debug.c +22 -35

69 lines not on this page · raw message

Signature · 1 line
2.5.2
#6 lore raw

PATCH 6/7 lkdtm: Add tests for struct list corruption

In reply to Paul E. McKenney

From: Kees Cook <[email protected]>

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.

---
 drivers/misc/lkdtm.h      |  2 ++
 drivers/misc/lkdtm_bugs.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++
 drivers/misc/lkdtm_core.c |  2 ++
 3 files changed, 72 insertions(+)
drivers/misc/lkdtm.h +2 -0
index fdf954c2107f..cfa1039c62e7 100644
@@ -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);
drivers/misc/lkdtm_bugs.c +68 -0

79 lines not on this page · raw message

drivers/misc/lkdtm_core.c +2 -0
index f9154b8d67f6..7eeb71a75549 100644
@@ -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),
Signature · 1 line
2.5.2
#7 lore raw

PATCH 7/7 lib/Kconfig.debug: Fix typo in select statement

In reply to Paul E. McKenney

From: Valentin Rothberg <[email protected]>

Commit 484f29c7430b3 ("bug: Provide toggle for BUG on data corruption") added a Kconfig select statement on CONFIG_DEBUG_LIST, but the CONFIG_ prefix is only used in Make and C(PP) syntax. Remove the CONFIG_ prefix to correctly select the Kconfig option DEBUG_LIST.

---
 lib/Kconfig.debug | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
lib/Kconfig.debug +1 -1
index 58d358a4c7f3..56f359e6c197 100644
@@ -1989,7 +1989,7 @@ config TEST_STATIC_KEYS
 
 config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
-	select CONFIG_DEBUG_LIST
+	select DEBUG_LIST
 	help
 	  Select this option if the kernel should BUG when it encounters
 	  data corruption in kernel memory structures when they get checked
Signature · 1 line
2.5.2