Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1593613 > unrolled thread

[PATCH 0/6] bug: further enhance use of BUG_ON_DATA_CORRUPTION

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] bug: further enhance use of BUG_ON_DATA_CORRUPTION Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 4/6] bug: Enable DEBUG_SG under BUG_ON_DATA_CORRUPTION Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 5/6] notifiers: Use CHECK_DATA_CORRUPTION() on checks Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 3/6] bug: Enable DEBUG_CREDENTIALS under BUG_ON_DATA_CORRUPTION Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 1/6] bug: Clarify help text for BUG_ON_DATA_CORRUPTION Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 2/6] bug: Improve unlikely() in data corruption check Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100
    [PATCH 6/6] refcount: Check bad states with CHECK_DATA_CORRUPTION Kees Cook <keescook@chromium.org> - 2017-03-06 20:20 +0100

#1593613 — [PATCH 0/6] bug: further enhance use of BUG_ON_DATA_CORRUPTION

This continues in applying the CHECK_DATA_CORRUPTION tests where
appropriate, and pulling similar CONFIGs under the same check. Most
notably, this adds the checks to refcount_t so that system builders can
Oops their kernels when encountering a potential refcounter attack. (And
so now the LKDTM tests for refcount issues pass correctly.)

I'd love to get people's Acks in case this needs to go through the
kspp tree instead of something else, like maybe -mm if that seems better.

Thanks,

-Kees

[toc] | [next] | [standalone]

#1593614 — [PATCH 4/6] bug: Enable DEBUG_SG under BUG_ON_DATA_CORRUPTION

Similar to CONFIG_DEBUG_CREDENTIALS, CONFIG_DEBUG_SG already handles
calling BUG, and performs inexpensive checks. This enables it under
CONFIG_BUG_ON_DATA_CORRUPTION.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.debug | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 009d6f8c7e5a..42c61cfe7d19 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1267,7 +1267,7 @@ config DEBUG_PI_LIST
 
 config DEBUG_SG
 	bool "Debug SG table operations"
-	depends on DEBUG_KERNEL
+	depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
 	help
 	  Enable this to turn on checks on scatter-gather tables. This can
 	  help find problems with drivers that do not properly initialize
@@ -1995,6 +1995,7 @@ config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
 	select DEBUG_CREDENTIALS
 	select DEBUG_LIST
+	select DEBUG_SG
 	help
 	  This option enables several inexpensive data corruption checks.
 	  Most of these checks normally just WARN and try to further avoid
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1593615 — [PATCH 5/6] notifiers: Use CHECK_DATA_CORRUPTION() on checks

When performing notifier function pointer sanity checking, allow
CONFIG_BUG_ON_DATA_CORRUPTION to upgrade from a WARN to a BUG.
Additionally enables CONFIG_DEBUG_NOTIFIERS when selecting
CONFIG_BUG_ON_DATA_CORRUPTION.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 kernel/notifier.c | 5 +++--
 lib/Kconfig.debug | 3 ++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/notifier.c b/kernel/notifier.c
index 6196af8a8223..58cc14958d92 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -84,8 +84,9 @@ static int notifier_call_chain(struct notifier_block **nl,
 		next_nb = rcu_dereference_raw(nb->next);
 
 #ifdef CONFIG_DEBUG_NOTIFIERS
-		if (unlikely(!func_ptr_is_kernel_text(nb->notifier_call))) {
-			WARN(1, "Invalid notifier called!");
+		if (CHECK_DATA_CORRUPTION(
+				!func_ptr_is_kernel_text(nb->notifier_call),
+				"Invalid notifier called!")) {
 			nb = next_nb;
 			continue;
 		}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 42c61cfe7d19..70e9f2c1bb30 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1277,7 +1277,7 @@ config DEBUG_SG
 
 config DEBUG_NOTIFIERS
 	bool "Debug notifier call chains"
-	depends on DEBUG_KERNEL
+	depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
 	help
 	  Enable this to turn on sanity checking for notifier call chains.
 	  This is most useful for kernel developers to make sure that
@@ -1995,6 +1995,7 @@ config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
 	select DEBUG_CREDENTIALS
 	select DEBUG_LIST
+	select DEBUG_NOTIFIERS
 	select DEBUG_SG
 	help
 	  This option enables several inexpensive data corruption checks.
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1593616 — [PATCH 3/6] bug: Enable DEBUG_CREDENTIALS under BUG_ON_DATA_CORRUPTION

Since CONFIG_DEBUG_CREDENTIALS already handles reporting and issuing a
BUG when it encounters corruption, add this to the list of corruption
test CONFIGs that are enabled under CONFIG_BUG_ON_DATA_CORRUPTION.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.debug | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 4a73d46711fb..009d6f8c7e5a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1287,7 +1287,7 @@ config DEBUG_NOTIFIERS
 
 config DEBUG_CREDENTIALS
 	bool "Debug credential management"
-	depends on DEBUG_KERNEL
+	depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
 	help
 	  Enable this to turn on some debug checking for credential
 	  management.  The additional code keeps track of the number of
@@ -1993,6 +1993,7 @@ config TEST_STATIC_KEYS
 
 config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
+	select DEBUG_CREDENTIALS
 	select DEBUG_LIST
 	help
 	  This option enables several inexpensive data corruption checks.
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1593617 — [PATCH 1/6] bug: Clarify help text for BUG_ON_DATA_CORRUPTION

This expands on the Kconfig help text for CONFIG_BUG_ON_DATA_CORRUPTION.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/Kconfig.debug | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 97d62c2da6c2..4a73d46711fb 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1995,9 +1995,11 @@ config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
 	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
-	  for validity.
+	  This option enables several inexpensive data corruption checks.
+	  Most of these checks normally just WARN and try to further avoid
+	  the corruption. Selecting this option upgrades these to BUGs, so
+	  that a system owner can furhter configure the system for immediate
+	  reboots or crash dumps.
 
 	  If unsure, say N.
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1593618 — [PATCH 2/6] bug: Improve unlikely() in data corruption check

This improves the compiler branch-hinting used in CHECK_DATA_CORRUPTION(),
similar to how it is done in WARN_ON() and friends.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 5828489309bb..5ef65dc2ed8b 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -129,15 +129,15 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 static inline __must_check bool check_data_corruption(bool v) { return v; }
 #define CHECK_DATA_CORRUPTION(condition, fmt, ...)			 \
 	check_data_corruption(({					 \
-		bool corruption = unlikely(condition);			 \
-		if (corruption) {					 \
+		bool corruption = !!(condition);			 \
+		if (unlikely(corruption)) {				 \
 			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
 				pr_err(fmt, ##__VA_ARGS__);		 \
 				BUG();					 \
 			} else						 \
 				WARN(1, fmt, ##__VA_ARGS__);		 \
 		}							 \
-		corruption;						 \
+		unlikely(corruption);					 \
 	}))
 
 #endif	/* _LINUX_BUG_H */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1593619 — [PATCH 6/6] refcount: Check bad states with CHECK_DATA_CORRUPTION

This converts from WARN() to CHECK_DATA_CORRUPTION() (so that system
builders can choose between WARN and BUG). Additionally moves refcount_t
sanity-check conditionals into regular function flow.

Now when built with CONFIG_BUG_ON_DATA_CORRUPTION, the LKDTM REFCOUNT_*
tests correctly kill offending processes.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 lib/refcount.c | 33 +++++++++++++++++++++------------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/lib/refcount.c b/lib/refcount.c
index 1d33366189d1..54aff1e0582f 100644
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -37,6 +37,13 @@
 #include <linux/refcount.h>
 #include <linux/bug.h>
 
+/*
+ * CHECK_DATA_CORRUPTION() is defined with __must_check, but we have a
+ * couple places where we want to report a condition that has already
+ * been checked, so this lets us cheat __must_check.
+ */
+#define REFCOUNT_CHECK(cond, str) unlikely(CHECK_DATA_CORRUPTION(cond, str))
+
 bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 {
 	unsigned int old, new, val = atomic_read(&r->refs);
@@ -58,7 +65,8 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 		val = old;
 	}
 
-	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	REFCOUNT_CHECK(new == UINT_MAX,
+			"refcount_t: add saturated; leaking memory.\n");
 
 	return true;
 }
@@ -66,7 +74,8 @@ EXPORT_SYMBOL_GPL(refcount_add_not_zero);
 
 void refcount_add(unsigned int i, refcount_t *r)
 {
-	WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+	REFCOUNT_CHECK(!refcount_add_not_zero(i, r),
+			"refcount_t: addition on 0; use-after-free.\n");
 }
 EXPORT_SYMBOL_GPL(refcount_add);
 
@@ -97,7 +106,8 @@ bool refcount_inc_not_zero(refcount_t *r)
 		val = old;
 	}
 
-	WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	REFCOUNT_CHECK(new == UINT_MAX,
+			"refcount_t: inc saturated; leaking memory.\n");
 
 	return true;
 }
@@ -111,7 +121,8 @@ EXPORT_SYMBOL_GPL(refcount_inc_not_zero);
  */
 void refcount_inc(refcount_t *r)
 {
-	WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+	REFCOUNT_CHECK(!refcount_inc_not_zero(r),
+			"refcount_t: increment on 0; use-after-free.\n");
 }
 EXPORT_SYMBOL_GPL(refcount_inc);
 
@@ -124,10 +135,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 			return false;
 
 		new = val - i;
-		if (new > val) {
-			WARN(new > val, "refcount_t: underflow; use-after-free.\n");
+		if (REFCOUNT_CHECK(new > val,
+				"refcount_t: sub underflow; use-after-free.\n"))
 			return false;
-		}
 
 		old = atomic_cmpxchg_release(&r->refs, val, new);
 		if (old == val)
@@ -164,7 +174,8 @@ EXPORT_SYMBOL_GPL(refcount_dec_and_test);
 
 void refcount_dec(refcount_t *r)
 {
-	WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+	REFCOUNT_CHECK(refcount_dec_and_test(r),
+			"refcount_t: decrement hit 0; leaking memory.\n");
 }
 EXPORT_SYMBOL_GPL(refcount_dec);
 
@@ -203,10 +214,9 @@ bool refcount_dec_not_one(refcount_t *r)
 			return false;
 
 		new = val - 1;
-		if (new > val) {
-			WARN(new > val, "refcount_t: underflow; use-after-free.\n");
+		if (REFCOUNT_CHECK(new > val,
+				"refcount_t: dec underflow; use-after-free.\n"))
 			return true;
-		}
 
 		old = atomic_cmpxchg_release(&r->refs, val, new);
 		if (old == val)
@@ -264,4 +274,3 @@ bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
 	return true;
 }
 EXPORT_SYMBOL_GPL(refcount_dec_and_lock);
-
-- 
2.7.4

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web