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

[PATCH 0/6] bug: further enhance use of BUG_ON_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.)

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

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

[Kees Cook]

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

[PATCH 2/6] 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 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

[PATCH 3/6] 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 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

[PATCH 4/6] 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 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

[PATCH 5/6] 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 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

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

[Kees Cook]

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

Re: [PATCH 5/6] notifiers: Use CHECK_DATA_CORRUPTION() on checks

[Kees Cook]

On Mon, Mar 6, 2017 at 11:09 AM, Kees Cook <keescook@chromium.org> wrote:
> 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.

Any feedback on this change? By default, this retains the existing
WARN behavior...

-Kees

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



-- 
Kees Cook
Pixel Security

Re: [PATCH 6/6] refcount: Check bad states with CHECK_DATA_CORRUPTION

[Kees Cook]

On Mon, Mar 6, 2017 at 11:09 AM, Kees Cook <keescook@chromium.org> wrote:
> 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.

Any feedback on this change? I'd like to get this and the prior
patches into -next soon for more testing.

-Kees

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



-- 
Kees Cook
Pixel Security

Re: [PATCH 5/6] notifiers: Use CHECK_DATA_CORRUPTION() on checks

[Arjan van de Ven]

On 3/22/2017 12:29 PM, Kees Cook wrote:
>> 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.

> Any feedback on this change? By default, this retains the existing
> WARN behavior...

if you're upgrading, is the end point really a panic() ?
e.g. do you assume people to also set panic-on-oops?

Re: [PATCH 5/6] notifiers: Use CHECK_DATA_CORRUPTION() on checks

[Kees Cook]

On Wed, Mar 22, 2017 at 12:32 PM, Arjan van de Ven
<arjan@linux.intel.com> wrote:
> On 3/22/2017 12:29 PM, Kees Cook wrote:
>>>
>>> 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.
>
>
>> Any feedback on this change? By default, this retains the existing
>> WARN behavior...
>
>
> if you're upgrading, is the end point really a panic() ?
> e.g. do you assume people to also set panic-on-oops?

That's one option, yes. With the BUG, the process associated is killed
(which is the first level of defense upgrade), and if a system is also
set to panic-on-oops, the entire system will panic (and usually such
systems also retain their crash consoles in some fashion for later
analysis, etc).

-Kees

-- 
Kees Cook
Pixel Security