[kernel-hardening] [PATCH v2] bug: further enhance use of CHECK_DATA_CORRUPTION

[kernel-hardening] [PATCH v2] bug: further enhance use of CHECK_DATA_CORRUPTION

[Kees Cook]

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

The series depends on the changes in -next made to lib/refcount.c,
so it might be easiest if this goes through the locking tree...

v2 is a rebase to -next and adjusts to using WARN_ONCE() instead of WARN().

-Kees

v1 was here: https://lkml.org/lkml/2017/3/6/720

[kernel-hardening] [PATCH v2 1/7] bug: Clarify help text for BUG_ON_DATA_CORRUPTION

[Kees Cook]

This expands on the Kconfig help text for CONFIG_BUG_ON_DATA_CORRUPTION to
more clearly explain the rationale of BUG vs WARN. Additionally fixes a
grammar glitch in the macro comment.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 3 ++-
 lib/Kconfig.debug   | 9 ++++++---
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..db1e41c69bac 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -124,7 +124,8 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 
 /*
  * Since detected data corruption should stop operation on the affected
- * structures. Return value must be checked and sanely acted on by caller.
+ * structures, require that the condition is checked and sanely acted on
+ * by the caller.
  */
 static inline __must_check bool check_data_corruption(bool v) { return v; }
 #define CHECK_DATA_CORRUPTION(condition, fmt, ...)			 \
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 77fadface4f9..5ac4d1148385 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1998,9 +1998,12 @@ 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 the offending process is killed. Additionally, the system
+	  owner can furhter configure the system for immediate reboots
+	  (via panic_on_oops sysctl) or crash dumps.
 
 	  If unsure, say N.
 
-- 
2.7.4

Re: [kernel-hardening] [PATCH v2 1/7] bug: Clarify help text for BUG_ON_DATA_CORRUPTION

[Ian Campbell]

> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 77fadface4f9..5ac4d1148385 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1998,9 +1998,12 @@ 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

First it says it enables some checks, but here it says it upgrades them
to BUGs which seems inconsistent.

> +	  that the offending process is killed. Additionally, the system
> +	  owner can furhter configure the system for immediate reboots

                   "further"

> +	  (via panic_on_oops sysctl) or crash dumps.
>  
> >  	  If unsure, say N.
>  

Re: [kernel-hardening] [PATCH v2 1/7] bug: Clarify help text for BUG_ON_DATA_CORRUPTION

[Kees Cook]

On Tue, Apr 4, 2017 at 10:47 PM, Ian Campbell <ijc@hellion.org.uk> wrote:
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 77fadface4f9..5ac4d1148385 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -1998,9 +1998,12 @@ 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
>
> First it says it enables some checks, but here it says it upgrades them
> to BUGs which seems inconsistent.

Right, it does both. It uses Kconfig "select" to enable checks, and
raises checks from WARN to BUG.

>
>> +       that the offending process is killed. Additionally, the system
>> +       owner can furhter configure the system for immediate reboots
>
>                    "further"

Ah, thanks!

>
>> +       (via panic_on_oops sysctl) or crash dumps.
>>
>> >       If unsure, say N.
>>

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] [PATCH v2 2/7] bug: Improve unlikely() in data corruption check

[Kees Cook]

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 db1e41c69bac..b6cfcb7f778f 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -130,15 +130,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

[kernel-hardening] [PATCH v2 3/7] bug: Use WARN_ONCE() for CHECK_DATA_CORRUPTION()

[Kees Cook]

Since users of CHECK_DATA_CORRUPTION() should be failing safe, the
condition that triggers a WARN() may recur. This would mean a logging
DoS of the system, so switch to WARN_ONCE() instead. (Those wanting
per-instance notifications should already be building with
CONFIG_BUG_ON_DATA_CORRUPTION so there is no change in that case.)

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index b6cfcb7f778f..c011438a7c6c 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -136,7 +136,7 @@ static inline __must_check bool check_data_corruption(bool v) { return v; }
 				pr_err(fmt, ##__VA_ARGS__);		 \
 				BUG();					 \
 			} else						 \
-				WARN(1, fmt, ##__VA_ARGS__);		 \
+				WARN_ONCE(1, fmt, ##__VA_ARGS__);	 \
 		}							 \
 		unlikely(corruption);					 \
 	}))
-- 
2.7.4

[kernel-hardening] [PATCH v2 4/7] bug: Enable DEBUG_CREDENTIALS under BUG_ON_DATA_CORRUPTION

[Kees Cook]

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 5ac4d1148385..9a1b6b56cef4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1288,7 +1288,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
@@ -1996,6 +1996,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

[kernel-hardening] [PATCH v2 5/7] bug: Enable DEBUG_SG under BUG_ON_DATA_CORRUPTION

[Kees Cook]

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 9a1b6b56cef4..45bfc0be38fc 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1268,7 +1268,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
@@ -1998,6 +1998,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

[kernel-hardening] [PATCH v2 6/7] notifiers: Use CHECK_DATA_CORRUPTION() on checks

[Kees Cook]

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 45bfc0be38fc..0dc8f6065be8 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1278,7 +1278,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
@@ -1998,6 +1998,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

[kernel-hardening] [PATCH v2 7/7] refcount: Check bad states with CHECK_DATA_CORRUPTION()

[Kees Cook]

This converts from WARN_ONCE() 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 f42124ccf295..88289210d9fd 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))
+
 /**
  * refcount_add_not_zero - add a value to a refcount unless it is 0
  * @i: the value to add to the refcount
@@ -72,7 +79,8 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 
 	} while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
 
-	WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	REFCOUNT_CHECK(new == UINT_MAX,
+			"refcount_t: add saturated; leaking memory.\n");
 
 	return true;
 }
@@ -96,7 +104,8 @@ EXPORT_SYMBOL_GPL(refcount_add_not_zero);
  */
 void refcount_add(unsigned int i, refcount_t *r)
 {
-	WARN_ONCE(!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);
 
@@ -127,7 +136,8 @@ bool refcount_inc_not_zero(refcount_t *r)
 
 	} while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
 
-	WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	REFCOUNT_CHECK(new == UINT_MAX,
+			"refcount_t: inc saturated; leaking memory.\n");
 
 	return true;
 }
@@ -147,7 +157,8 @@ EXPORT_SYMBOL_GPL(refcount_inc_not_zero);
  */
 void refcount_inc(refcount_t *r)
 {
-	WARN_ONCE(!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);
 
@@ -180,10 +191,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 			return false;
 
 		new = val - i;
-		if (new > val) {
-			WARN_ONCE(new > val, "refcount_t: underflow; use-after-free.\n");
+		if (REFCOUNT_CHECK(new > val,
+				"refcount_t: sub underflow; use-after-free.\n"))
 			return false;
-		}
 
 	} while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
 
@@ -222,7 +232,8 @@ EXPORT_SYMBOL_GPL(refcount_dec_and_test);
  */
 void refcount_dec(refcount_t *r)
 {
-	WARN_ONCE(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);
 
@@ -273,10 +284,9 @@ bool refcount_dec_not_one(refcount_t *r)
 			return false;
 
 		new = val - 1;
-		if (new > val) {
-			WARN_ONCE(new > val, "refcount_t: underflow; use-after-free.\n");
+		if (REFCOUNT_CHECK(new > val,
+				"refcount_t: dec underflow; use-after-free.\n"))
 			return true;
-		}
 
 	} while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
 
@@ -345,4 +355,3 @@ bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
 	return true;
 }
 EXPORT_SYMBOL_GPL(refcount_dec_and_lock);
-
-- 
2.7.4

Re: [PATCH v2] bug: further enhance use of CHECK_DATA_CORRUPTION

[Josh Poimboeuf]

On Tue, Apr 04, 2017 at 03:12:11PM -0700, Kees Cook wrote:
> 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.)
> 
> The series depends on the changes in -next made to lib/refcount.c,
> so it might be easiest if this goes through the locking tree...
> 
> v2 is a rebase to -next and adjusts to using WARN_ONCE() instead of WARN().
> 
> -Kees
> 
> v1 was here: https://lkml.org/lkml/2017/3/6/720

Ping?  Just wondering what ever happened to this 3+ year old series...

-- 
Josh