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

Groups > linux.kernel > #1464825 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/5] bug: Provide toggle for BUG on data corruption Kees Cook <keescook@chromium.org> - 2016-08-17 23:50 +0200
    [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption Kees Cook <keescook@chromium.org> - 2016-08-17 23:50 +0200
      Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption Arnd Bergmann <arnd@arndb.de> - 2016-08-22 15:20 +0200
        Re: [PATCH v3 4/5] bug: Provide toggle for BUG on data corruption "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-22 20:00 +0200
          Re: [kernel-hardening] Re: [PATCH v3 4/5] bug: Provide toggle for BUG  on data corruption Kees Cook <keescook@chromium.org> - 2016-08-23 00:40 +0200
    Re: [PATCH v3 0/5] bug: Provide toggle for BUG on data corruption Steven Rostedt <rostedt@goodmis.org> - 2016-08-18 15:50 +0200
      Re: [PATCH v3 0/5] bug: Provide toggle for BUG on data corruption "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-19 03:10 +0200
    Re: [kernel-hardening] [PATCH v3 0/5] bug: Provide toggle for BUG  on data corruption Rik van Riel <riel@redhat.com> - 2016-08-19 03:00 +0200
      Re: [kernel-hardening] [PATCH v3 0/5] bug: Provide toggle for BUG on  data corruption "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-19 03:30 +0200
        Re: [kernel-hardening] [PATCH v3 0/5] bug: Provide toggle for BUG  on data corruption Rik van Riel <riel@redhat.com> - 2016-08-19 05:00 +0200
          Re: [kernel-hardening] [PATCH v3 0/5] bug: Provide toggle for BUG on  data corruption "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2016-08-19 19:50 +0200

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

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

[toc] | [next] | [standalone]

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

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

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

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

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

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

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

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

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

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

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

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

#1465372

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

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

#1465711

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
> 

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

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

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.

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

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

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

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

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

[Multipart message — attachments visible in raw view] — view raw

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.

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web