[PATCH 0/4] refcount_t followups...

[PATCH 0/4] refcount_t followups...

[Kees Cook]

Hi,

After doing some testing on the new refcount_t series, here are some
changes to improve refcount_t for the paranoid case. ;)

Since the new LKDTM tests depend on the new API, please add them to your
series; it doesn't make sense for it to go through drivers-misc.

The first patch (Kconfig fix) could be trivially folded into the initial
refcount_t patch, too.

Thanks!

-Kees

[kernel-hardening] [PATCH 0/4] refcount_t followups...

[Kees Cook]

Hi,

After doing some testing on the new refcount_t series, here are some
changes to improve refcount_t for the paranoid case. ;)

Since the new LKDTM tests depend on the new API, please add them to your
series; it doesn't make sense for it to go through drivers-misc.

The first patch (Kconfig fix) could be trivially folded into the initial
refcount_t patch, too.

Thanks!

-Kees

[PATCH 1/4] refcount_t: fix Kconfig help

[Kees Cook]

Minor fix to build refcount_t series.

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

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 6f61d32ee536..20fde8d4523a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -731,7 +731,7 @@ source "lib/Kconfig.kasan"
 
 config DEBUG_REFCOUNT
 	bool "Verbose refcount checks"
-	--help--
+	help
 	  Say Y here if you want reference counters (refcount_t and kref) to
 	  generate WARNs on dubious usage. Without this refcount_t will still
 	  be a saturating counter and avoid Use-After-Free by turning it into
-- 
2.7.4

[kernel-hardening] [PATCH 1/4] refcount_t: fix Kconfig help

[Kees Cook]

Minor fix to build refcount_t series.

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

diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 6f61d32ee536..20fde8d4523a 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -731,7 +731,7 @@ source "lib/Kconfig.kasan"
 
 config DEBUG_REFCOUNT
 	bool "Verbose refcount checks"
-	--help--
+	help
 	  Say Y here if you want reference counters (refcount_t and kref) to
 	  generate WARNs on dubious usage. Without this refcount_t will still
 	  be a saturating counter and avoid Use-After-Free by turning it into
-- 
2.7.4

[PATCH 2/4] lkdtm: convert to refcount_t testing

[Kees Cook]

Since we'll be using refcount_t instead of atomic_t for refcounting,
change the LKDTM tests to reflect the new interface and test conditions.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.h      |  8 +++--
 drivers/misc/lkdtm_bugs.c | 87 +++++++++++++++++++++++++++++++++++++++--------
 drivers/misc/lkdtm_core.c |  8 +++--
 3 files changed, 85 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index cfa1039c62e7..67d27be60405 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -19,8 +19,12 @@ void lkdtm_SOFTLOCKUP(void);
 void lkdtm_HARDLOCKUP(void);
 void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
-void lkdtm_ATOMIC_UNDERFLOW(void);
-void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_REFCOUNT_SATURATE_INC(void);
+void lkdtm_REFCOUNT_SATURATE_ADD(void);
+void lkdtm_REFCOUNT_ZERO_DEC(void);
+void lkdtm_REFCOUNT_ZERO_INC(void);
+void lkdtm_REFCOUNT_ZERO_SUB(void);
+void lkdtm_REFCOUNT_ZERO_ADD(void);
 void lkdtm_CORRUPT_LIST_ADD(void);
 void lkdtm_CORRUPT_LIST_DEL(void);
 
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index bb3bb8ef5f44..e3f4cd8876b5 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -6,6 +6,7 @@
  */
 #include "lkdtm.h"
 #include <linux/list.h>
+#include <linux/refcount.h>
 #include <linux/sched.h>
 
 struct lkdtm_list {
@@ -134,28 +135,86 @@ void lkdtm_HUNG_TASK(void)
 	schedule();
 }
 
-void lkdtm_ATOMIC_UNDERFLOW(void)
+void lkdtm_REFCOUNT_SATURATE_INC(void)
 {
-	atomic_t under = ATOMIC_INIT(INT_MIN);
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
 
-	pr_info("attempting good atomic increment\n");
-	atomic_inc(&under);
-	atomic_dec(&under);
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
 
-	pr_info("attempting bad atomic underflow\n");
-	atomic_dec(&under);
+	pr_info("attempting bad refcount inc overflow\n");
+	refcount_inc(&over);
+	refcount_inc(&over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_SATURATE_ADD(void)
+{
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
+
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
+
+	pr_info("attempting bad refcount add overflow\n");
+	refcount_add(2, &over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_DEC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount decrement to zero\n");
+	refcount_dec(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went crazy\n");
 }
 
-void lkdtm_ATOMIC_OVERFLOW(void)
+void lkdtm_REFCOUNT_ZERO_SUB(void)
 {
-	atomic_t over = ATOMIC_INIT(INT_MAX);
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount subtract past zero\n");
+	if (!refcount_sub_and_test(2, &zero))
+		pr_info("wrap attempt was noticed\n");
+	if (refcount_read(&zero) == 1)
+		pr_err("Correctly stayed above 0, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
 
-	pr_info("attempting good atomic decrement\n");
-	atomic_dec(&over);
-	atomic_inc(&over);
+void lkdtm_REFCOUNT_ZERO_INC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
 
-	pr_info("attempting bad atomic overflow\n");
-	atomic_inc(&over);
+	pr_info("attempting bad refcount increment from zero\n");
+	refcount_inc(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_ADD(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount addition from zero\n");
+	refcount_add(2, &zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
 }
 
 void lkdtm_CORRUPT_LIST_ADD(void)
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index 4d44084071d8..b9a4cd4a9b68 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -220,8 +220,12 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(WRITE_RO),
 	CRASHTYPE(WRITE_RO_AFTER_INIT),
 	CRASHTYPE(WRITE_KERN),
-	CRASHTYPE(ATOMIC_UNDERFLOW),
-	CRASHTYPE(ATOMIC_OVERFLOW),
+	CRASHTYPE(REFCOUNT_SATURATE_INC),
+	CRASHTYPE(REFCOUNT_SATURATE_ADD),
+	CRASHTYPE(REFCOUNT_ZERO_DEC),
+	CRASHTYPE(REFCOUNT_ZERO_INC),
+	CRASHTYPE(REFCOUNT_ZERO_SUB),
+	CRASHTYPE(REFCOUNT_ZERO_ADD),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
 	CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
-- 
2.7.4

[kernel-hardening] [PATCH 2/4] lkdtm: convert to refcount_t testing

[Kees Cook]

Since we'll be using refcount_t instead of atomic_t for refcounting,
change the LKDTM tests to reflect the new interface and test conditions.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm.h      |  8 +++--
 drivers/misc/lkdtm_bugs.c | 87 +++++++++++++++++++++++++++++++++++++++--------
 drivers/misc/lkdtm_core.c |  8 +++--
 3 files changed, 85 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index cfa1039c62e7..67d27be60405 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -19,8 +19,12 @@ void lkdtm_SOFTLOCKUP(void);
 void lkdtm_HARDLOCKUP(void);
 void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
-void lkdtm_ATOMIC_UNDERFLOW(void);
-void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_REFCOUNT_SATURATE_INC(void);
+void lkdtm_REFCOUNT_SATURATE_ADD(void);
+void lkdtm_REFCOUNT_ZERO_DEC(void);
+void lkdtm_REFCOUNT_ZERO_INC(void);
+void lkdtm_REFCOUNT_ZERO_SUB(void);
+void lkdtm_REFCOUNT_ZERO_ADD(void);
 void lkdtm_CORRUPT_LIST_ADD(void);
 void lkdtm_CORRUPT_LIST_DEL(void);
 
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index bb3bb8ef5f44..e3f4cd8876b5 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -6,6 +6,7 @@
  */
 #include "lkdtm.h"
 #include <linux/list.h>
+#include <linux/refcount.h>
 #include <linux/sched.h>
 
 struct lkdtm_list {
@@ -134,28 +135,86 @@ void lkdtm_HUNG_TASK(void)
 	schedule();
 }
 
-void lkdtm_ATOMIC_UNDERFLOW(void)
+void lkdtm_REFCOUNT_SATURATE_INC(void)
 {
-	atomic_t under = ATOMIC_INIT(INT_MIN);
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
 
-	pr_info("attempting good atomic increment\n");
-	atomic_inc(&under);
-	atomic_dec(&under);
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
 
-	pr_info("attempting bad atomic underflow\n");
-	atomic_dec(&under);
+	pr_info("attempting bad refcount inc overflow\n");
+	refcount_inc(&over);
+	refcount_inc(&over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_SATURATE_ADD(void)
+{
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
+
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
+
+	pr_info("attempting bad refcount add overflow\n");
+	refcount_add(2, &over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_DEC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount decrement to zero\n");
+	refcount_dec(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went crazy\n");
 }
 
-void lkdtm_ATOMIC_OVERFLOW(void)
+void lkdtm_REFCOUNT_ZERO_SUB(void)
 {
-	atomic_t over = ATOMIC_INIT(INT_MAX);
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount subtract past zero\n");
+	if (!refcount_sub_and_test(2, &zero))
+		pr_info("wrap attempt was noticed\n");
+	if (refcount_read(&zero) == 1)
+		pr_err("Correctly stayed above 0, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
 
-	pr_info("attempting good atomic decrement\n");
-	atomic_dec(&over);
-	atomic_inc(&over);
+void lkdtm_REFCOUNT_ZERO_INC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
 
-	pr_info("attempting bad atomic overflow\n");
-	atomic_inc(&over);
+	pr_info("attempting bad refcount increment from zero\n");
+	refcount_inc(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_ADD(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount addition from zero\n");
+	refcount_add(2, &zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
 }
 
 void lkdtm_CORRUPT_LIST_ADD(void)
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index 4d44084071d8..b9a4cd4a9b68 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -220,8 +220,12 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(WRITE_RO),
 	CRASHTYPE(WRITE_RO_AFTER_INIT),
 	CRASHTYPE(WRITE_KERN),
-	CRASHTYPE(ATOMIC_UNDERFLOW),
-	CRASHTYPE(ATOMIC_OVERFLOW),
+	CRASHTYPE(REFCOUNT_SATURATE_INC),
+	CRASHTYPE(REFCOUNT_SATURATE_ADD),
+	CRASHTYPE(REFCOUNT_ZERO_DEC),
+	CRASHTYPE(REFCOUNT_ZERO_INC),
+	CRASHTYPE(REFCOUNT_ZERO_SUB),
+	CRASHTYPE(REFCOUNT_ZERO_ADD),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
 	CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
-- 
2.7.4

[tip:locking/core] lkdtm: Convert to refcount_t testing

[tip-bot for Kees Cook]

Commit-ID:  ff86b30010eee8249dc244ce1868b886bbee6449
Gitweb:     http://git.kernel.org/tip/ff86b30010eee8249dc244ce1868b886bbee6449
Author:     Kees Cook <keescook@chromium.org>
AuthorDate: Fri, 3 Feb 2017 15:26:50 -0800
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 10 Feb 2017 09:04:20 +0100

lkdtm: Convert to refcount_t testing

Since we'll be using refcount_t instead of atomic_t for refcounting,
change the LKDTM tests to reflect the new interface and test conditions.

Signed-off-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Hans Liljestrand <ishkamiel@gmail.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: arnd@arndb.de
Cc: dhowells@redhat.com
Cc: dwindsor@gmail.com
Cc: elena.reshetova@intel.com
Cc: gregkh@linuxfoundation.org
Cc: h.peter.anvin@intel.com
Cc: kernel-hardening@lists.openwall.com
Cc: will.deacon@arm.com
Link: http://lkml.kernel.org/r/1486164412-7338-3-git-send-email-keescook@chromium.org
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 drivers/misc/lkdtm.h      |  8 +++--
 drivers/misc/lkdtm_bugs.c | 87 +++++++++++++++++++++++++++++++++++++++--------
 drivers/misc/lkdtm_core.c |  8 +++--
 3 files changed, 85 insertions(+), 18 deletions(-)

diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index cfa1039..67d27be 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -19,8 +19,12 @@ void lkdtm_SOFTLOCKUP(void);
 void lkdtm_HARDLOCKUP(void);
 void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(void);
-void lkdtm_ATOMIC_UNDERFLOW(void);
-void lkdtm_ATOMIC_OVERFLOW(void);
+void lkdtm_REFCOUNT_SATURATE_INC(void);
+void lkdtm_REFCOUNT_SATURATE_ADD(void);
+void lkdtm_REFCOUNT_ZERO_DEC(void);
+void lkdtm_REFCOUNT_ZERO_INC(void);
+void lkdtm_REFCOUNT_ZERO_SUB(void);
+void lkdtm_REFCOUNT_ZERO_ADD(void);
 void lkdtm_CORRUPT_LIST_ADD(void);
 void lkdtm_CORRUPT_LIST_DEL(void);
 
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index 91edd0b..cba0837 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -6,6 +6,7 @@
  */
 #include "lkdtm.h"
 #include <linux/list.h>
+#include <linux/refcount.h>
 #include <linux/sched.h>
 
 struct lkdtm_list {
@@ -129,28 +130,86 @@ void lkdtm_HUNG_TASK(void)
 	schedule();
 }
 
-void lkdtm_ATOMIC_UNDERFLOW(void)
+void lkdtm_REFCOUNT_SATURATE_INC(void)
 {
-	atomic_t under = ATOMIC_INIT(INT_MIN);
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
 
-	pr_info("attempting good atomic increment\n");
-	atomic_inc(&under);
-	atomic_dec(&under);
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
 
-	pr_info("attempting bad atomic underflow\n");
-	atomic_dec(&under);
+	pr_info("attempting bad refcount inc overflow\n");
+	refcount_inc(&over);
+	refcount_inc(&over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_SATURATE_ADD(void)
+{
+	refcount_t over = REFCOUNT_INIT(UINT_MAX - 1);
+
+	pr_info("attempting good refcount decrement\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
+
+	pr_info("attempting bad refcount add overflow\n");
+	refcount_add(2, &over);
+	if (refcount_read(&over) == UINT_MAX)
+		pr_err("Correctly stayed saturated, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_DEC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount decrement to zero\n");
+	refcount_dec(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went crazy\n");
 }
 
-void lkdtm_ATOMIC_OVERFLOW(void)
+void lkdtm_REFCOUNT_ZERO_SUB(void)
 {
-	atomic_t over = ATOMIC_INIT(INT_MAX);
+	refcount_t zero = REFCOUNT_INIT(1);
+
+	pr_info("attempting bad refcount subtract past zero\n");
+	if (!refcount_sub_and_test(2, &zero))
+		pr_info("wrap attempt was noticed\n");
+	if (refcount_read(&zero) == 1)
+		pr_err("Correctly stayed above 0, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount wrapped\n");
+}
 
-	pr_info("attempting good atomic decrement\n");
-	atomic_dec(&over);
-	atomic_inc(&over);
+void lkdtm_REFCOUNT_ZERO_INC(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
 
-	pr_info("attempting bad atomic overflow\n");
-	atomic_inc(&over);
+	pr_info("attempting bad refcount increment from zero\n");
+	refcount_inc(&zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
+}
+
+void lkdtm_REFCOUNT_ZERO_ADD(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount addition from zero\n");
+	refcount_add(2, &zero);
+	if (refcount_read(&zero) == 0)
+		pr_err("Stayed at zero, but no BUG?!\n");
+	else
+		pr_err("Fail: refcount went past zero\n");
 }
 
 void lkdtm_CORRUPT_LIST_ADD(void)
diff --git a/drivers/misc/lkdtm_core.c b/drivers/misc/lkdtm_core.c
index 7eeb71a..16e4cf1 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -220,8 +220,12 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(WRITE_RO),
 	CRASHTYPE(WRITE_RO_AFTER_INIT),
 	CRASHTYPE(WRITE_KERN),
-	CRASHTYPE(ATOMIC_UNDERFLOW),
-	CRASHTYPE(ATOMIC_OVERFLOW),
+	CRASHTYPE(REFCOUNT_SATURATE_INC),
+	CRASHTYPE(REFCOUNT_SATURATE_ADD),
+	CRASHTYPE(REFCOUNT_ZERO_DEC),
+	CRASHTYPE(REFCOUNT_ZERO_INC),
+	CRASHTYPE(REFCOUNT_ZERO_SUB),
+	CRASHTYPE(REFCOUNT_ZERO_ADD),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
 	CRASHTYPE(USERCOPY_HEAP_FLAG_TO),

[PATCH 3/4] bug: Switch data corruption check to __must_check

[Kees Cook]

The CHECK_DATA_CORRUPTION() macro was designed to have callers do
something meaningful/protective on failure. However, using "return false"
in the macro too strictly limits the design patterns of callers. Instead,
let callers handle the logic test directly, but make sure that the result
IS checked by forcing __must_check (which appears to not be able to be
used directly on macro expressions).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 12 +++++++-----
 lib/list_debug.c    | 45 ++++++++++++++++++++++++---------------------
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index baff2e8fc8a8..5828489309bb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -124,18 +124,20 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 
 /*
  * Since detected data corruption should stop operation on the affected
- * structures, this returns false if the corruption condition is found.
+ * structures. Return value must be checked and sanely acted on by caller.
  */
+static inline __must_check bool check_data_corruption(bool v) { return v; }
 #define CHECK_DATA_CORRUPTION(condition, fmt, ...)			 \
-	do {								 \
-		if (unlikely(condition)) {				 \
+	check_data_corruption(({					 \
+		bool corruption = unlikely(condition);			 \
+		if (corruption) {					 \
 			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
 				pr_err(fmt, ##__VA_ARGS__);		 \
 				BUG();					 \
 			} else						 \
 				WARN(1, fmt, ##__VA_ARGS__);		 \
-			return false;					 \
 		}							 \
-	} while (0)
+		corruption;						 \
+	}))
 
 #endif	/* _LINUX_BUG_H */
diff --git a/lib/list_debug.c b/lib/list_debug.c
index 7f7bfa55eb6d..a34db8d27667 100644
--- a/lib/list_debug.c
+++ b/lib/list_debug.c
@@ -20,15 +20,16 @@
 bool __list_add_valid(struct list_head *new, struct list_head *prev,
 		      struct list_head *next)
 {
-	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);
+	if (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 false;
 
 	return true;
 }
@@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry)
 	prev = entry->prev;
 	next = entry->next;
 
-	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);
+	if (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 false;
+
 	return true;
 
 }
-- 
2.7.4

[kernel-hardening] [PATCH 3/4] bug: Switch data corruption check to __must_check

[Kees Cook]

The CHECK_DATA_CORRUPTION() macro was designed to have callers do
something meaningful/protective on failure. However, using "return false"
in the macro too strictly limits the design patterns of callers. Instead,
let callers handle the logic test directly, but make sure that the result
IS checked by forcing __must_check (which appears to not be able to be
used directly on macro expressions).

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/bug.h | 12 +++++++-----
 lib/list_debug.c    | 45 ++++++++++++++++++++++++---------------------
 2 files changed, 31 insertions(+), 26 deletions(-)

diff --git a/include/linux/bug.h b/include/linux/bug.h
index baff2e8fc8a8..5828489309bb 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -124,18 +124,20 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
 
 /*
  * Since detected data corruption should stop operation on the affected
- * structures, this returns false if the corruption condition is found.
+ * structures. Return value must be checked and sanely acted on by caller.
  */
+static inline __must_check bool check_data_corruption(bool v) { return v; }
 #define CHECK_DATA_CORRUPTION(condition, fmt, ...)			 \
-	do {								 \
-		if (unlikely(condition)) {				 \
+	check_data_corruption(({					 \
+		bool corruption = unlikely(condition);			 \
+		if (corruption) {					 \
 			if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
 				pr_err(fmt, ##__VA_ARGS__);		 \
 				BUG();					 \
 			} else						 \
 				WARN(1, fmt, ##__VA_ARGS__);		 \
-			return false;					 \
 		}							 \
-	} while (0)
+		corruption;						 \
+	}))
 
 #endif	/* _LINUX_BUG_H */
diff --git a/lib/list_debug.c b/lib/list_debug.c
index 7f7bfa55eb6d..a34db8d27667 100644
--- a/lib/list_debug.c
+++ b/lib/list_debug.c
@@ -20,15 +20,16 @@
 bool __list_add_valid(struct list_head *new, struct list_head *prev,
 		      struct list_head *next)
 {
-	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);
+	if (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 false;
 
 	return true;
 }
@@ -41,18 +42,20 @@ bool __list_del_entry_valid(struct list_head *entry)
 	prev = entry->prev;
 	next = entry->next;
 
-	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);
+	if (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 false;
+
 	return true;
 
 }
-- 
2.7.4

[PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
is marked __much_check, we override few cases where the failure has
already been handled but we want to explicitly report it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
 lib/Kconfig.debug        |  2 ++
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index 5b89cad62237..ef32910c7dd8 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -43,10 +43,10 @@
 #include <linux/spinlock.h>
 
 #if CONFIG_DEBUG_REFCOUNT
-#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
+#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
 #define __refcount_check	__must_check
 #else
-#define REFCOUNT_WARN(cond, str) (void)(cond)
+#define REFCOUNT_CHECK(cond, str) (!!(cond))
 #define __refcount_check
 #endif
 
@@ -86,14 +86,18 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 			break;
 	}
 
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	val = REFCOUNT_CHECK(new == UINT_MAX,
+			     "refcount_t: add saturated; leaking memory.\n");
 
 	return true;
 }
 
 static inline void refcount_add(unsigned int i, refcount_t *r)
 {
-	REFCOUNT_WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(!refcount_add_not_zero(i, r),
+			   "refcount_t: addition on 0; use-after-free.\n");
 }
 
 /*
@@ -121,7 +125,8 @@ bool refcount_inc_not_zero(refcount_t *r)
 			break;
 	}
 
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	val = REFCOUNT_CHECK(new == UINT_MAX,
+			     "refcount_t: inc saturated; leaking memory.\n");
 
 	return true;
 }
@@ -134,7 +139,10 @@ bool refcount_inc_not_zero(refcount_t *r)
  */
 static inline void refcount_inc(refcount_t *r)
 {
-	REFCOUNT_WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(!refcount_inc_not_zero(r),
+			   "refcount_t: increment on 0; use-after-free.\n");
 }
 
 /*
@@ -155,10 +163,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 			return false;
 
 		new = val - i;
-		if (new > val) {
-			REFCOUNT_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;
-		}
 
 		if (atomic_try_cmpxchg_release(&r->refs, &val, new))
 			break;
@@ -183,7 +190,10 @@ bool refcount_dec_and_test(refcount_t *r)
 static inline
 void refcount_dec(refcount_t *r)
 {
-	REFCOUNT_WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(refcount_dec_and_test(r),
+			   "refcount_t: decrement hit 0; leaking memory.\n");
 }
 
 /*
@@ -224,10 +234,9 @@ bool refcount_dec_not_one(refcount_t *r)
 			return false;
 
 		new = val - 1;
-		if (new > val) {
-			REFCOUNT_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;
-		}
 
 		if (atomic_try_cmpxchg_release(&r->refs, &val, new))
 			break;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 20fde8d4523a..01e7aa578456 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -731,6 +731,7 @@ source "lib/Kconfig.kasan"
 
 config DEBUG_REFCOUNT
 	bool "Verbose refcount checks"
+	depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
 	help
 	  Say Y here if you want reference counters (refcount_t and kref) to
 	  generate WARNs on dubious usage. Without this refcount_t will still
@@ -2011,6 +2012,7 @@ config TEST_STATIC_KEYS
 config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
 	select DEBUG_LIST
+	select DEBUG_REFCOUNT
 	help
 	  Select this option if the kernel should BUG when it encounters
 	  data corruption in kernel memory structures when they get checked
-- 
2.7.4

[kernel-hardening] [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
is marked __much_check, we override few cases where the failure has
already been handled but we want to explicitly report it.

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
 lib/Kconfig.debug        |  2 ++
 2 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/include/linux/refcount.h b/include/linux/refcount.h
index 5b89cad62237..ef32910c7dd8 100644
--- a/include/linux/refcount.h
+++ b/include/linux/refcount.h
@@ -43,10 +43,10 @@
 #include <linux/spinlock.h>
 
 #if CONFIG_DEBUG_REFCOUNT
-#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
+#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
 #define __refcount_check	__must_check
 #else
-#define REFCOUNT_WARN(cond, str) (void)(cond)
+#define REFCOUNT_CHECK(cond, str) (!!(cond))
 #define __refcount_check
 #endif
 
@@ -86,14 +86,18 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
 			break;
 	}
 
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	val = REFCOUNT_CHECK(new == UINT_MAX,
+			     "refcount_t: add saturated; leaking memory.\n");
 
 	return true;
 }
 
 static inline void refcount_add(unsigned int i, refcount_t *r)
 {
-	REFCOUNT_WARN(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(!refcount_add_not_zero(i, r),
+			   "refcount_t: addition on 0; use-after-free.\n");
 }
 
 /*
@@ -121,7 +125,8 @@ bool refcount_inc_not_zero(refcount_t *r)
 			break;
 	}
 
-	REFCOUNT_WARN(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+	val = REFCOUNT_CHECK(new == UINT_MAX,
+			     "refcount_t: inc saturated; leaking memory.\n");
 
 	return true;
 }
@@ -134,7 +139,10 @@ bool refcount_inc_not_zero(refcount_t *r)
  */
 static inline void refcount_inc(refcount_t *r)
 {
-	REFCOUNT_WARN(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(!refcount_inc_not_zero(r),
+			   "refcount_t: increment on 0; use-after-free.\n");
 }
 
 /*
@@ -155,10 +163,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
 			return false;
 
 		new = val - i;
-		if (new > val) {
-			REFCOUNT_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;
-		}
 
 		if (atomic_try_cmpxchg_release(&r->refs, &val, new))
 			break;
@@ -183,7 +190,10 @@ bool refcount_dec_and_test(refcount_t *r)
 static inline
 void refcount_dec(refcount_t *r)
 {
-	REFCOUNT_WARN(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+	bool __always_unused b;
+
+	b = REFCOUNT_CHECK(refcount_dec_and_test(r),
+			   "refcount_t: decrement hit 0; leaking memory.\n");
 }
 
 /*
@@ -224,10 +234,9 @@ bool refcount_dec_not_one(refcount_t *r)
 			return false;
 
 		new = val - 1;
-		if (new > val) {
-			REFCOUNT_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;
-		}
 
 		if (atomic_try_cmpxchg_release(&r->refs, &val, new))
 			break;
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 20fde8d4523a..01e7aa578456 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -731,6 +731,7 @@ source "lib/Kconfig.kasan"
 
 config DEBUG_REFCOUNT
 	bool "Verbose refcount checks"
+	depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
 	help
 	  Say Y here if you want reference counters (refcount_t and kref) to
 	  generate WARNs on dubious usage. Without this refcount_t will still
@@ -2011,6 +2012,7 @@ config TEST_STATIC_KEYS
 config BUG_ON_DATA_CORRUPTION
 	bool "Trigger a BUG when data corruption is detected"
 	select DEBUG_LIST
+	select DEBUG_REFCOUNT
 	help
 	  Select this option if the kernel should BUG when it encounters
 	  data corruption in kernel memory structures when they get checked
-- 
2.7.4

Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
> is marked __much_check, we override few cases where the failure has
> already been handled but we want to explicitly report it.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>  lib/Kconfig.debug        |  2 ++
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index 5b89cad62237..ef32910c7dd8 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -43,10 +43,10 @@
>  #include <linux/spinlock.h>
>  
>  #if CONFIG_DEBUG_REFCOUNT
> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)

OK, so that goes back to a full WARN() which will make the generated
code gigantic due to the whole printk() trainwreck :/

[kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
> is marked __much_check, we override few cases where the failure has
> already been handled but we want to explicitly report it.
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>  lib/Kconfig.debug        |  2 ++
>  2 files changed, 24 insertions(+), 13 deletions(-)
> 
> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> index 5b89cad62237..ef32910c7dd8 100644
> --- a/include/linux/refcount.h
> +++ b/include/linux/refcount.h
> @@ -43,10 +43,10 @@
>  #include <linux/spinlock.h>
>  
>  #if CONFIG_DEBUG_REFCOUNT
> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)

OK, so that goes back to a full WARN() which will make the generated
code gigantic due to the whole printk() trainwreck :/

Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
>> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
>> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
>> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
>> is marked __much_check, we override few cases where the failure has
>> already been handled but we want to explicitly report it.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>>  lib/Kconfig.debug        |  2 ++
>>  2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
>> index 5b89cad62237..ef32910c7dd8 100644
>> --- a/include/linux/refcount.h
>> +++ b/include/linux/refcount.h
>> @@ -43,10 +43,10 @@
>>  #include <linux/spinlock.h>
>>
>>  #if CONFIG_DEBUG_REFCOUNT
>> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
>> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
>
> OK, so that goes back to a full WARN() which will make the generated
> code gigantic due to the whole printk() trainwreck :/

Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
>> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
>> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
>> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
>> is marked __much_check, we override few cases where the failure has
>> already been handled but we want to explicitly report it.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> ---
>>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>>  lib/Kconfig.debug        |  2 ++
>>  2 files changed, 24 insertions(+), 13 deletions(-)
>>
>> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
>> index 5b89cad62237..ef32910c7dd8 100644
>> --- a/include/linux/refcount.h
>> +++ b/include/linux/refcount.h
>> @@ -43,10 +43,10 @@
>>  #include <linux/spinlock.h>
>>
>>  #if CONFIG_DEBUG_REFCOUNT
>> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
>> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
>
> OK, so that goes back to a full WARN() which will make the generated
> code gigantic due to the whole printk() trainwreck :/

Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Sun, Feb 05, 2017 at 03:33:36PM -0800, Kees Cook wrote:
> On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
> >> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
> >> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
> >> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
> >> is marked __much_check, we override few cases where the failure has
> >> already been handled but we want to explicitly report it.
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
> >>  lib/Kconfig.debug        |  2 ++
> >>  2 files changed, 24 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> >> index 5b89cad62237..ef32910c7dd8 100644
> >> --- a/include/linux/refcount.h
> >> +++ b/include/linux/refcount.h
> >> @@ -43,10 +43,10 @@
> >>  #include <linux/spinlock.h>
> >>
> >>  #if CONFIG_DEBUG_REFCOUNT
> >> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
> >> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
> >
> > OK, so that goes back to a full WARN() which will make the generated
> > code gigantic due to the whole printk() trainwreck :/
> 
> Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?

Did consider that, didn't really know if that made sense.

Like I wrote, ideally we'd end up using something like the x86 exception
table with a custom handler. Just no idea how to pull that off without
doing a full blown arch specific implementation, so I didn't go there
quite yet.


That way refcount_inc() would end up being inlined to something like:

        mov    0x148(%rdi),%eax
        jmp    2f
  1:    lock cmpxchg %edx,0x148(%rdi)
        je     4f
  2:    lea    -0x1(%rax),%ecx
        lea    0x1(%rax),%edx
        cmp    $0xfffffffd,%ecx
        jbe    1b
  3:    ud2
  4:

	_ASM_EXTABLE_HANDLE(3b, 4b, ex_handler_refcount_inc)


where:

bool ex_handler_refcount_inc(const struct exception_table_entry *fixup,
			     struct pt_regs *regs, int trapnr)
{
	regs->ip = ex_fixup_addr(fixup);

	if (!regs->ax)
		WARN(1, "refcount_t: increment on 0; use-after-free.\n");
	else
		WARN(1, "refcount_t: saturated; leaking memory.\n");

	return true;
}

and the handler is shared between all instances and can be as big and
fancy as we'd like.

[kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Sun, Feb 05, 2017 at 03:33:36PM -0800, Kees Cook wrote:
> On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> > On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
> >> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
> >> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
> >> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
> >> is marked __much_check, we override few cases where the failure has
> >> already been handled but we want to explicitly report it.
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
> >>  lib/Kconfig.debug        |  2 ++
> >>  2 files changed, 24 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
> >> index 5b89cad62237..ef32910c7dd8 100644
> >> --- a/include/linux/refcount.h
> >> +++ b/include/linux/refcount.h
> >> @@ -43,10 +43,10 @@
> >>  #include <linux/spinlock.h>
> >>
> >>  #if CONFIG_DEBUG_REFCOUNT
> >> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
> >> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
> >
> > OK, so that goes back to a full WARN() which will make the generated
> > code gigantic due to the whole printk() trainwreck :/
> 
> Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?

Did consider that, didn't really know if that made sense.

Like I wrote, ideally we'd end up using something like the x86 exception
table with a custom handler. Just no idea how to pull that off without
doing a full blown arch specific implementation, so I didn't go there
quite yet.


That way refcount_inc() would end up being inlined to something like:

        mov    0x148(%rdi),%eax
        jmp    2f
  1:    lock cmpxchg %edx,0x148(%rdi)
        je     4f
  2:    lea    -0x1(%rax),%ecx
        lea    0x1(%rax),%edx
        cmp    $0xfffffffd,%ecx
        jbe    1b
  3:    ud2
  4:

	_ASM_EXTABLE_HANDLE(3b, 4b, ex_handler_refcount_inc)


where:

bool ex_handler_refcount_inc(const struct exception_table_entry *fixup,
			     struct pt_regs *regs, int trapnr)
{
	regs->ip = ex_fixup_addr(fixup);

	if (!regs->ax)
		WARN(1, "refcount_t: increment on 0; use-after-free.\n");
	else
		WARN(1, "refcount_t: saturated; leaking memory.\n");

	return true;
}

and the handler is shared between all instances and can be as big and
fancy as we'd like.

Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Mon, Feb 6, 2017 at 12:57 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sun, Feb 05, 2017 at 03:33:36PM -0800, Kees Cook wrote:
>> On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
>> >> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
>> >> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
>> >> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
>> >> is marked __much_check, we override few cases where the failure has
>> >> already been handled but we want to explicitly report it.
>> >>
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >> ---
>> >>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>> >>  lib/Kconfig.debug        |  2 ++
>> >>  2 files changed, 24 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
>> >> index 5b89cad62237..ef32910c7dd8 100644
>> >> --- a/include/linux/refcount.h
>> >> +++ b/include/linux/refcount.h
>> >> @@ -43,10 +43,10 @@
>> >>  #include <linux/spinlock.h>
>> >>
>> >>  #if CONFIG_DEBUG_REFCOUNT
>> >> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
>> >> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
>> >
>> > OK, so that goes back to a full WARN() which will make the generated
>> > code gigantic due to the whole printk() trainwreck :/
>>
>> Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?
>
> Did consider that, didn't really know if that made sense.
>
> Like I wrote, ideally we'd end up using something like the x86 exception
> table with a custom handler. Just no idea how to pull that off without
> doing a full blown arch specific implementation, so I didn't go there
> quite yet.

I haven't spent much time looking at the extable stuff. (Though
coincidentally, I was poking at it for x86's test_nx stuff...) I
thought there was a way to build arch-agnostic extables already?
kernel/extable.c is unconditionally built-in, for example.

> That way refcount_inc() would end up being inlined to something like:
>
>         mov    0x148(%rdi),%eax
>         jmp    2f
>   1:    lock cmpxchg %edx,0x148(%rdi)
>         je     4f
>   2:    lea    -0x1(%rax),%ecx
>         lea    0x1(%rax),%edx
>         cmp    $0xfffffffd,%ecx
>         jbe    1b
>   3:    ud2
>   4:
>
>         _ASM_EXTABLE_HANDLE(3b, 4b, ex_handler_refcount_inc)
>
>
> where:
>
> bool ex_handler_refcount_inc(const struct exception_table_entry *fixup,
>                              struct pt_regs *regs, int trapnr)
> {
>         regs->ip = ex_fixup_addr(fixup);
>
>         if (!regs->ax)
>                 WARN(1, "refcount_t: increment on 0; use-after-free.\n");
>         else
>                 WARN(1, "refcount_t: saturated; leaking memory.\n");
>
>         return true;
> }
>
> and the handler is shared between all instances and can be as big and
> fancy as we'd like.

I'll dig a bit to see what I can build. Can you add the lkdtm tests to
the series, though? That should be fine as-is.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

[kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Mon, Feb 6, 2017 at 12:57 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Sun, Feb 05, 2017 at 03:33:36PM -0800, Kees Cook wrote:
>> On Sun, Feb 5, 2017 at 7:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>> > On Fri, Feb 03, 2017 at 03:26:52PM -0800, Kees Cook wrote:
>> >> This converts from WARN_ON() to CHECK_DATA_CORRUPTION() in the
>> >> CONFIG_DEBUG_REFCOUNT case. Additionally moves refcount_t sanity check
>> >> conditionals into regular function flow. Since CHECK_DATA_CORRUPTION()
>> >> is marked __much_check, we override few cases where the failure has
>> >> already been handled but we want to explicitly report it.
>> >>
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >> ---
>> >>  include/linux/refcount.h | 35 ++++++++++++++++++++++-------------
>> >>  lib/Kconfig.debug        |  2 ++
>> >>  2 files changed, 24 insertions(+), 13 deletions(-)
>> >>
>> >> diff --git a/include/linux/refcount.h b/include/linux/refcount.h
>> >> index 5b89cad62237..ef32910c7dd8 100644
>> >> --- a/include/linux/refcount.h
>> >> +++ b/include/linux/refcount.h
>> >> @@ -43,10 +43,10 @@
>> >>  #include <linux/spinlock.h>
>> >>
>> >>  #if CONFIG_DEBUG_REFCOUNT
>> >> -#define REFCOUNT_WARN(cond, str) WARN_ON(cond)
>> >> +#define REFCOUNT_CHECK(cond, str) CHECK_DATA_CORRUPTION(cond, str)
>> >
>> > OK, so that goes back to a full WARN() which will make the generated
>> > code gigantic due to the whole printk() trainwreck :/
>>
>> Hrm, perhaps we need three levels? WARN_ON, WARN, and BUG?
>
> Did consider that, didn't really know if that made sense.
>
> Like I wrote, ideally we'd end up using something like the x86 exception
> table with a custom handler. Just no idea how to pull that off without
> doing a full blown arch specific implementation, so I didn't go there
> quite yet.

I haven't spent much time looking at the extable stuff. (Though
coincidentally, I was poking at it for x86's test_nx stuff...) I
thought there was a way to build arch-agnostic extables already?
kernel/extable.c is unconditionally built-in, for example.

> That way refcount_inc() would end up being inlined to something like:
>
>         mov    0x148(%rdi),%eax
>         jmp    2f
>   1:    lock cmpxchg %edx,0x148(%rdi)
>         je     4f
>   2:    lea    -0x1(%rax),%ecx
>         lea    0x1(%rax),%edx
>         cmp    $0xfffffffd,%ecx
>         jbe    1b
>   3:    ud2
>   4:
>
>         _ASM_EXTABLE_HANDLE(3b, 4b, ex_handler_refcount_inc)
>
>
> where:
>
> bool ex_handler_refcount_inc(const struct exception_table_entry *fixup,
>                              struct pt_regs *regs, int trapnr)
> {
>         regs->ip = ex_fixup_addr(fixup);
>
>         if (!regs->ax)
>                 WARN(1, "refcount_t: increment on 0; use-after-free.\n");
>         else
>                 WARN(1, "refcount_t: saturated; leaking memory.\n");
>
>         return true;
> }
>
> and the handler is shared between all instances and can be as big and
> fancy as we'd like.

I'll dig a bit to see what I can build. Can you add the lkdtm tests to
the series, though? That should be fine as-is.

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> >
> > Like I wrote, ideally we'd end up using something like the x86 exception
> > table with a custom handler. Just no idea how to pull that off without
> > doing a full blown arch specific implementation, so I didn't go there
> > quite yet.
> 
> I haven't spent much time looking at the extable stuff. (Though
> coincidentally, I was poking at it for x86's test_nx stuff...) I
> thought there was a way to build arch-agnostic extables already?
> kernel/extable.c is unconditionally built-in, for example.

That doesn't seem to be of much use. It only contains section sort and
search functions.

Another problem for generic code would be to figure out what register
the relevant variable would live in at the time of exception. Here its
'obviously' EAX because that's what cmpxchg requires, but in generic
you'd need a means of querying GCC's register allocator at the exception
point and somehow using that information for the generation of the
exception handler.

AFAIK that's not something GCC can do.

> > and the handler is shared between all instances and can be as big and
> > fancy as we'd like.
> 
> I'll dig a bit to see what I can build. Can you add the lkdtm tests to
> the series, though? That should be fine as-is.

Yes, I did. I also did the 's/--help--/help/' and 's/#if/#ifdef/' thing.

Thanks!

[kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> >
> > Like I wrote, ideally we'd end up using something like the x86 exception
> > table with a custom handler. Just no idea how to pull that off without
> > doing a full blown arch specific implementation, so I didn't go there
> > quite yet.
> 
> I haven't spent much time looking at the extable stuff. (Though
> coincidentally, I was poking at it for x86's test_nx stuff...) I
> thought there was a way to build arch-agnostic extables already?
> kernel/extable.c is unconditionally built-in, for example.

That doesn't seem to be of much use. It only contains section sort and
search functions.

Another problem for generic code would be to figure out what register
the relevant variable would live in at the time of exception. Here its
'obviously' EAX because that's what cmpxchg requires, but in generic
you'd need a means of querying GCC's register allocator at the exception
point and somehow using that information for the generation of the
exception handler.

AFAIK that's not something GCC can do.

> > and the handler is shared between all instances and can be as big and
> > fancy as we'd like.
> 
> I'll dig a bit to see what I can build. Can you add the lkdtm tests to
> the series, though? That should be fine as-is.

Yes, I did. I also did the 's/--help--/help/' and 's/#if/#ifdef/' thing.

Thanks!

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > >
> > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > table with a custom handler. Just no idea how to pull that off without
> > > doing a full blown arch specific implementation, so I didn't go there
> > > quite yet.
> > 
> > I haven't spent much time looking at the extable stuff. (Though
> > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > thought there was a way to build arch-agnostic extables already?
> > kernel/extable.c is unconditionally built-in, for example.
> 
> That doesn't seem to be of much use. It only contains section sort and
> search functions.
> 
> Another problem for generic code would be to figure out what register
> the relevant variable would live in at the time of exception. Here its
> 'obviously' EAX because that's what cmpxchg requires, but in generic
> you'd need a means of querying GCC's register allocator at the exception
> point and somehow using that information for the generation of the
> exception handler.

I think we only need two arch-specific primitives:
(a) mangle a GCC assigned register into an idx stored in the extable
(b) take said index, and grab the relevant register from pt_regs

Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
and mangle that into the idx in the extable. In the common case, I'd
hope GCC would leave the register in-place from the cmpxchg.

... or have I misundertood? :)

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > >
> > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > table with a custom handler. Just no idea how to pull that off without
> > > doing a full blown arch specific implementation, so I didn't go there
> > > quite yet.
> > 
> > I haven't spent much time looking at the extable stuff. (Though
> > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > thought there was a way to build arch-agnostic extables already?
> > kernel/extable.c is unconditionally built-in, for example.
> 
> That doesn't seem to be of much use. It only contains section sort and
> search functions.
> 
> Another problem for generic code would be to figure out what register
> the relevant variable would live in at the time of exception. Here its
> 'obviously' EAX because that's what cmpxchg requires, but in generic
> you'd need a means of querying GCC's register allocator at the exception
> point and somehow using that information for the generation of the
> exception handler.

I think we only need two arch-specific primitives:
(a) mangle a GCC assigned register into an idx stored in the extable
(b) take said index, and grab the relevant register from pt_regs

Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
and mangle that into the idx in the extable. In the common case, I'd
hope GCC would leave the register in-place from the cmpxchg.

... or have I misundertood? :)

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 11:10:12AM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> > On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > > >
> > > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > > table with a custom handler. Just no idea how to pull that off without
> > > > doing a full blown arch specific implementation, so I didn't go there
> > > > quite yet.
> > > 
> > > I haven't spent much time looking at the extable stuff. (Though
> > > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > > thought there was a way to build arch-agnostic extables already?
> > > kernel/extable.c is unconditionally built-in, for example.
> > 
> > That doesn't seem to be of much use. It only contains section sort and
> > search functions.
> > 
> > Another problem for generic code would be to figure out what register
> > the relevant variable would live in at the time of exception. Here its
> > 'obviously' EAX because that's what cmpxchg requires, but in generic
> > you'd need a means of querying GCC's register allocator at the exception
> > point and somehow using that information for the generation of the
> > exception handler.
> 
> I think we only need two arch-specific primitives:
> (a) mangle a GCC assigned register into an idx stored in the extable
> (b) take said index, and grab the relevant register from pt_regs
> 
> Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
> and mangle that into the idx in the extable. In the common case, I'd
> hope GCC would leave the register in-place from the cmpxchg.
> 
> ... or have I misundertood? :)

Right something along those lines. (a) will need GCC help, and (b) would
be kernel-arch specific. So this isn't something we can quickly do.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 11:10:12AM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> > On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > > >
> > > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > > table with a custom handler. Just no idea how to pull that off without
> > > > doing a full blown arch specific implementation, so I didn't go there
> > > > quite yet.
> > > 
> > > I haven't spent much time looking at the extable stuff. (Though
> > > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > > thought there was a way to build arch-agnostic extables already?
> > > kernel/extable.c is unconditionally built-in, for example.
> > 
> > That doesn't seem to be of much use. It only contains section sort and
> > search functions.
> > 
> > Another problem for generic code would be to figure out what register
> > the relevant variable would live in at the time of exception. Here its
> > 'obviously' EAX because that's what cmpxchg requires, but in generic
> > you'd need a means of querying GCC's register allocator at the exception
> > point and somehow using that information for the generation of the
> > exception handler.
> 
> I think we only need two arch-specific primitives:
> (a) mangle a GCC assigned register into an idx stored in the extable
> (b) take said index, and grab the relevant register from pt_regs
> 
> Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
> and mangle that into the idx in the extable. In the common case, I'd
> hope GCC would leave the register in-place from the cmpxchg.
> 
> ... or have I misundertood? :)

Right something along those lines. (a) will need GCC help, and (b) would
be kernel-arch specific. So this isn't something we can quickly do.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 01:36:30PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 11:10:12AM +0000, Mark Rutland wrote:
> > On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> > > On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > > > >
> > > > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > > > table with a custom handler. Just no idea how to pull that off without
> > > > > doing a full blown arch specific implementation, so I didn't go there
> > > > > quite yet.
> > > > 
> > > > I haven't spent much time looking at the extable stuff. (Though
> > > > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > > > thought there was a way to build arch-agnostic extables already?
> > > > kernel/extable.c is unconditionally built-in, for example.
> > > 
> > > That doesn't seem to be of much use. It only contains section sort and
> > > search functions.
> > > 
> > > Another problem for generic code would be to figure out what register
> > > the relevant variable would live in at the time of exception. Here its
> > > 'obviously' EAX because that's what cmpxchg requires, but in generic
> > > you'd need a means of querying GCC's register allocator at the exception
> > > point and somehow using that information for the generation of the
> > > exception handler.
> > 
> > I think we only need two arch-specific primitives:
> > (a) mangle a GCC assigned register into an idx stored in the extable
> > (b) take said index, and grab the relevant register from pt_regs
> > 
> > Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
> > and mangle that into the idx in the extable. In the common case, I'd
> > hope GCC would leave the register in-place from the cmpxchg.
> > 
> > ... or have I misundertood? :)
> 
> Right something along those lines. (a) will need GCC help, and (b) would
> be kernel-arch specific. So this isn't something we can quickly do.

I agree this isn't something that can be hacked together quickly, and
certainly shouldn't block these patches.

However, I don't think we need anything new from GCC, and I think we
already have a generic API for (b).

For (a) we don't need new GCC help if we do something like we did in
commit 72c5839515260dce to do the mangling. Prepend a prefix to the
register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
to hold the correct value.

For (b) we already have regs_get_register().

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 01:36:30PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 11:10:12AM +0000, Mark Rutland wrote:
> > On Tue, Feb 07, 2017 at 09:34:05AM +0100, Peter Zijlstra wrote:
> > > On Mon, Feb 06, 2017 at 08:54:38AM -0800, Kees Cook wrote:
> > > > >
> > > > > Like I wrote, ideally we'd end up using something like the x86 exception
> > > > > table with a custom handler. Just no idea how to pull that off without
> > > > > doing a full blown arch specific implementation, so I didn't go there
> > > > > quite yet.
> > > > 
> > > > I haven't spent much time looking at the extable stuff. (Though
> > > > coincidentally, I was poking at it for x86's test_nx stuff...) I
> > > > thought there was a way to build arch-agnostic extables already?
> > > > kernel/extable.c is unconditionally built-in, for example.
> > > 
> > > That doesn't seem to be of much use. It only contains section sort and
> > > search functions.
> > > 
> > > Another problem for generic code would be to figure out what register
> > > the relevant variable would live in at the time of exception. Here its
> > > 'obviously' EAX because that's what cmpxchg requires, but in generic
> > > you'd need a means of querying GCC's register allocator at the exception
> > > point and somehow using that information for the generation of the
> > > exception handler.
> > 
> > I think we only need two arch-specific primitives:
> > (a) mangle a GCC assigned register into an idx stored in the extable
> > (b) take said index, and grab the relevant register from pt_regs
> > 
> > Then you can have a BUG_VALUE(v, ...), where we use an input "r" (val),
> > and mangle that into the idx in the extable. In the common case, I'd
> > hope GCC would leave the register in-place from the cmpxchg.
> > 
> > ... or have I misundertood? :)
> 
> Right something along those lines. (a) will need GCC help, and (b) would
> be kernel-arch specific. So this isn't something we can quickly do.

I agree this isn't something that can be hacked together quickly, and
certainly shouldn't block these patches.

However, I don't think we need anything new from GCC, and I think we
already have a generic API for (b).

For (a) we don't need new GCC help if we do something like we did in
commit 72c5839515260dce to do the mangling. Prepend a prefix to the
register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
to hold the correct value.

For (b) we already have regs_get_register().

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 01:50:20PM +0000, Mark Rutland wrote:
> > Right something along those lines. (a) will need GCC help, and (b) would
> > be kernel-arch specific. So this isn't something we can quickly do.
> 
> I agree this isn't something that can be hacked together quickly, and
> certainly shouldn't block these patches.
> 
> However, I don't think we need anything new from GCC, and I think we
> already have a generic API for (b).
> 
> For (a) we don't need new GCC help if we do something like we did in
> commit 72c5839515260dce to do the mangling. Prepend a prefix to the
> register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
> to hold the correct value.

I'm not sure I can decipher that commit and therefore have no idea if
something similar can be done for other architectures.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 01:50:20PM +0000, Mark Rutland wrote:
> > Right something along those lines. (a) will need GCC help, and (b) would
> > be kernel-arch specific. So this isn't something we can quickly do.
> 
> I agree this isn't something that can be hacked together quickly, and
> certainly shouldn't block these patches.
> 
> However, I don't think we need anything new from GCC, and I think we
> already have a generic API for (b).
> 
> For (a) we don't need new GCC help if we do something like we did in
> commit 72c5839515260dce to do the mangling. Prepend a prefix to the
> register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
> to hold the correct value.

I'm not sure I can decipher that commit and therefore have no idea if
something similar can be done for other architectures.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 04:07:37PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 01:50:20PM +0000, Mark Rutland wrote:
> > > Right something along those lines. (a) will need GCC help, and (b) would
> > > be kernel-arch specific. So this isn't something we can quickly do.
> > 
> > I agree this isn't something that can be hacked together quickly, and
> > certainly shouldn't block these patches.
> > 
> > However, I don't think we need anything new from GCC, and I think we
> > already have a generic API for (b).
> > 
> > For (a) we don't need new GCC help if we do something like we did in
> > commit 72c5839515260dce to do the mangling. Prepend a prefix to the
> > register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
> > to hold the correct value.
> 
> I'm not sure I can decipher that commit and therefore have no idea if
> something similar can be done for other architectures.

For x86 it's a little painful due to '%' in the register names, but it looks
possible. The below appears to do the mangling correctly (then screams due to
the mangled result being nonexistent).

Thanks,
Mark.

---->8----
#define cmpxchg(ptr, old, new)						\
({									\
	typeof(*ptr) __ret;						\
	typeof(*ptr) __old = (old);					\
	typeof(*ptr) __new = (new);					\
									\
	volatile unsigned int *__ptr = (volatile unsigned int *)ptr;	\
	asm volatile("cmpxchgl %2, %1"					\
		     : "=a" (__ret), "+m" (*__ptr)			\
		     : "r" (__new), "0" (__old)				\
		     : "memory");					\
	__ret;								\
})

asm(
"	.macro	reg_to_offset	r\n"
"	.irp rs,eax,ebx,ecx,edx\n"
"	.ifc \\r, %\\rs\n"
"	__offset_of_\\rs\n"
"	.endif\n"
"	.endr\n"
"	.endm\n"
);

#define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))

int foo(void)
{
	unsigned int mem = 0;
	unsigned int new;
	int bar = 7, baz = 11;

	new = cmpxchg(&mem, 1, 2);
	asm_sym(new);
}

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 04:07:37PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 01:50:20PM +0000, Mark Rutland wrote:
> > > Right something along those lines. (a) will need GCC help, and (b) would
> > > be kernel-arch specific. So this isn't something we can quickly do.
> > 
> > I agree this isn't something that can be hacked together quickly, and
> > certainly shouldn't block these patches.
> > 
> > However, I don't think we need anything new from GCC, and I think we
> > already have a generic API for (b).
> > 
> > For (a) we don't need new GCC help if we do something like we did in
> > commit 72c5839515260dce to do the mangling. Prepend a prefix to the
> > register, e.g. changing 'x0' to '__pt_regs_offset_x0', which we arrange
> > to hold the correct value.
> 
> I'm not sure I can decipher that commit and therefore have no idea if
> something similar can be done for other architectures.

For x86 it's a little painful due to '%' in the register names, but it looks
possible. The below appears to do the mangling correctly (then screams due to
the mangled result being nonexistent).

Thanks,
Mark.

---->8----
#define cmpxchg(ptr, old, new)						\
({									\
	typeof(*ptr) __ret;						\
	typeof(*ptr) __old = (old);					\
	typeof(*ptr) __new = (new);					\
									\
	volatile unsigned int *__ptr = (volatile unsigned int *)ptr;	\
	asm volatile("cmpxchgl %2, %1"					\
		     : "=a" (__ret), "+m" (*__ptr)			\
		     : "r" (__new), "0" (__old)				\
		     : "memory");					\
	__ret;								\
})

asm(
"	.macro	reg_to_offset	r\n"
"	.irp rs,eax,ebx,ecx,edx\n"
"	.ifc \\r, %\\rs\n"
"	__offset_of_\\rs\n"
"	.endif\n"
"	.endr\n"
"	.endm\n"
);

#define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))

int foo(void)
{
	unsigned int mem = 0;
	unsigned int new;
	int bar = 7, baz = 11;

	new = cmpxchg(&mem, 1, 2);
	asm_sym(new);
}

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> For x86 it's a little painful due to '%' in the register names, but it looks
> possible. The below appears to do the mangling correctly (then screams due to
> the mangled result being nonexistent).

> asm(
> "	.macro	reg_to_offset	r\n"
> "	.irp rs,eax,ebx,ecx,edx\n"
> "	.ifc \\r, %\\rs\n"
> "	__offset_of_\\rs\n"
> "	.endif\n"
> "	.endr\n"
> "	.endm\n"
> );
> 
> #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))

Oh gawd that's a most gnarly hack.

Do we want to go do that for all archs or somehow cook a generic
fallback that ends up doing a full function call or something?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> For x86 it's a little painful due to '%' in the register names, but it looks
> possible. The below appears to do the mangling correctly (then screams due to
> the mangled result being nonexistent).

> asm(
> "	.macro	reg_to_offset	r\n"
> "	.irp rs,eax,ebx,ecx,edx\n"
> "	.ifc \\r, %\\rs\n"
> "	__offset_of_\\rs\n"
> "	.endif\n"
> "	.endr\n"
> "	.endm\n"
> );
> 
> #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))

Oh gawd that's a most gnarly hack.

Do we want to go do that for all archs or somehow cook a generic
fallback that ends up doing a full function call or something?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> > For x86 it's a little painful due to '%' in the register names, but it looks
> > possible. The below appears to do the mangling correctly (then screams due to
> > the mangled result being nonexistent).
> 
> > asm(
> > "	.macro	reg_to_offset	r\n"
> > "	.irp rs,eax,ebx,ecx,edx\n"
> > "	.ifc \\r, %\\rs\n"
> > "	__offset_of_\\rs\n"
> > "	.endif\n"
> > "	.endr\n"
> > "	.endm\n"
> > );
> > 
> > #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))
> 
> Oh gawd that's a most gnarly hack.

:)

> Do we want to go do that for all archs or somehow cook a generic
> fallback that ends up doing a full function call or something?

Given the arch-specific reg->blah mapping is so "fun", I guess a generic
fallback would be a good start.

I haven't figured out all the plumbing details. It'd be nice to reuse
the bug infrastructure so that arches don't have to implement another
trap and callback pair, but I guess the reg details need to live in
another data structure.

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
> On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> > For x86 it's a little painful due to '%' in the register names, but it looks
> > possible. The below appears to do the mangling correctly (then screams due to
> > the mangled result being nonexistent).
> 
> > asm(
> > "	.macro	reg_to_offset	r\n"
> > "	.irp rs,eax,ebx,ecx,edx\n"
> > "	.ifc \\r, %\\rs\n"
> > "	__offset_of_\\rs\n"
> > "	.endif\n"
> > "	.endr\n"
> > "	.endm\n"
> > );
> > 
> > #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))
> 
> Oh gawd that's a most gnarly hack.

:)

> Do we want to go do that for all archs or somehow cook a generic
> fallback that ends up doing a full function call or something?

Given the arch-specific reg->blah mapping is so "fun", I guess a generic
fallback would be a good start.

I haven't figured out all the plumbing details. It'd be nice to reuse
the bug infrastructure so that arches don't have to implement another
trap and callback pair, but I guess the reg details need to live in
another data structure.

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 05:55:42PM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> > > For x86 it's a little painful due to '%' in the register names, but it looks
> > > possible. The below appears to do the mangling correctly (then screams due to
> > > the mangled result being nonexistent).
> > 
> > > asm(
> > > "	.macro	reg_to_offset	r\n"
> > > "	.irp rs,eax,ebx,ecx,edx\n"
> > > "	.ifc \\r, %\\rs\n"
> > > "	__offset_of_\\rs\n"
> > > "	.endif\n"
> > > "	.endr\n"
> > > "	.endm\n"
> > > );
> > > 
> > > #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))
> > 
> > Oh gawd that's a most gnarly hack.
> 
> :)
> 
> > Do we want to go do that for all archs or somehow cook a generic
> > fallback that ends up doing a full function call or something?
> 
> Given the arch-specific reg->blah mapping is so "fun", I guess a generic
> fallback would be a good start.
> 
> I haven't figured out all the plumbing details. It'd be nice to reuse
> the bug infrastructure so that arches don't have to implement another
> trap and callback pair, but I guess the reg details need to live in
> another data structure.

On x86 have have __ex_table and __bug_table. The former is used for all
sorts of things, including fixing up faults.

Now, our struct exception_table_entry has a third field used to specify
a handler, see commit:

 548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

Also, given we trigger things with a known instruction at these sites,
the ->to field is reusable and can be used to encode the register
offset.

Still, if we want to allow a generic implementation that does a function
call, the handler prototype should probably look like:

	void exception_value(unsigned long value);

Which means the arch bits need a trampoline and we also need to encode
that. The best I've come up with is having nr_regs trampolines and
stuffing the trampoline function in the ->handler field and then using
the ->to field to encode the actual handler.

Something like:

#define EX_REG_HANDLER(_reg)					\
bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
			    struct pt_regs *regs, int trapnr)	\
{								\
	void (*handler)(unsigned long) =			\
		(void *)((unsigned long)&fixup->to + fixup->to); \
								\
	if (trapnr != X86_TRAP_UD)				\
		return false;					\
								\
	regs->ip += 2; /* size of UD2 instruction */		\
	handler(regs->_reg);					\
	return true;						\
}

EX_REG_HANDLER(bx);
EX_REG_HANDLER(cx);
...
EX_REG_HANDLER(ss);


asm (
" .macro reg_to_handler	r\n"
" .irp rs,bx,cx,...,ss\n"
" .ifc \\r, %\\rs\n"
" ex_handler_value_\\rs\n"
" .endif\n"
" .endr\n"
" .endm\n"
);

#define EXCEPTION_VALUE(val, handler)			\
	asm volatile ("1: ud2"				\
		      _ASM_EXTABLE_HANDLE(1b, handler,	\
				     reg_to_handler %0) \
		      : : "r" (val))


Where the generic version can simply be:

#define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)


Makes sense?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Tue, Feb 07, 2017 at 05:55:42PM +0000, Mark Rutland wrote:
> On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
> > On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
> > > For x86 it's a little painful due to '%' in the register names, but it looks
> > > possible. The below appears to do the mangling correctly (then screams due to
> > > the mangled result being nonexistent).
> > 
> > > asm(
> > > "	.macro	reg_to_offset	r\n"
> > > "	.irp rs,eax,ebx,ecx,edx\n"
> > > "	.ifc \\r, %\\rs\n"
> > > "	__offset_of_\\rs\n"
> > > "	.endif\n"
> > > "	.endr\n"
> > > "	.endm\n"
> > > );
> > > 
> > > #define asm_sym(var)		asm volatile("reg_to_offset %0\n" : : "r" (var))
> > 
> > Oh gawd that's a most gnarly hack.
> 
> :)
> 
> > Do we want to go do that for all archs or somehow cook a generic
> > fallback that ends up doing a full function call or something?
> 
> Given the arch-specific reg->blah mapping is so "fun", I guess a generic
> fallback would be a good start.
> 
> I haven't figured out all the plumbing details. It'd be nice to reuse
> the bug infrastructure so that arches don't have to implement another
> trap and callback pair, but I guess the reg details need to live in
> another data structure.

On x86 have have __ex_table and __bug_table. The former is used for all
sorts of things, including fixing up faults.

Now, our struct exception_table_entry has a third field used to specify
a handler, see commit:

 548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

Also, given we trigger things with a known instruction at these sites,
the ->to field is reusable and can be used to encode the register
offset.

Still, if we want to allow a generic implementation that does a function
call, the handler prototype should probably look like:

	void exception_value(unsigned long value);

Which means the arch bits need a trampoline and we also need to encode
that. The best I've come up with is having nr_regs trampolines and
stuffing the trampoline function in the ->handler field and then using
the ->to field to encode the actual handler.

Something like:

#define EX_REG_HANDLER(_reg)					\
bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
			    struct pt_regs *regs, int trapnr)	\
{								\
	void (*handler)(unsigned long) =			\
		(void *)((unsigned long)&fixup->to + fixup->to); \
								\
	if (trapnr != X86_TRAP_UD)				\
		return false;					\
								\
	regs->ip += 2; /* size of UD2 instruction */		\
	handler(regs->_reg);					\
	return true;						\
}

EX_REG_HANDLER(bx);
EX_REG_HANDLER(cx);
...
EX_REG_HANDLER(ss);


asm (
" .macro reg_to_handler	r\n"
" .irp rs,bx,cx,...,ss\n"
" .ifc \\r, %\\rs\n"
" ex_handler_value_\\rs\n"
" .endif\n"
" .endr\n"
" .endm\n"
);

#define EXCEPTION_VALUE(val, handler)			\
	asm volatile ("1: ud2"				\
		      _ASM_EXTABLE_HANDLE(1b, handler,	\
				     reg_to_handler %0) \
		      : : "r" (val))


Where the generic version can simply be:

#define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)


Makes sense?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 10:12:50AM +0100, Peter Zijlstra wrote:
> Something like:
> 
> #define EX_REG_HANDLER(_reg)					\
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
> 			    struct pt_regs *regs, int trapnr)	\
> {								\
> 	void (*handler)(unsigned long) =			\
> 		(void *)((unsigned long)&fixup->to + fixup->to); \
> 								\
> 	if (trapnr != X86_TRAP_UD)				\
> 		return false;					\
> 								\
> 	regs->ip += 2; /* size of UD2 instruction */		\
> 	handler(regs->_reg);					\
> 	return true;						\
> }
> 
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
> 
> 
> asm (
> " .macro reg_to_handler	r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
  " .ifc \\r, %e\\rs\n"
  " ex_handler_value_\\rs\n"
  " .endif\n"
  " .ifc \\r, %r\\rs\n"
  " ex_handler_value_\\rs\n"
  " .endif\n"
> " .endr\n"
> " .endm\n"
> );

to match the 16, 32 and 64 bit names of the same registers. The byte
registers will need additional magic :/


> #define EXCEPTION_VALUE(val, handler)			\
> 	asm volatile ("1: ud2"				\
> 		      _ASM_EXTABLE_HANDLE(1b, handler,	\
> 				     reg_to_handler %0) \
> 		      : : "r" (val))
> 
> 
> Where the generic version can simply be:
> 
> #define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)
> 
> 
> Makes sense?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 10:12:50AM +0100, Peter Zijlstra wrote:
> Something like:
> 
> #define EX_REG_HANDLER(_reg)					\
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
> 			    struct pt_regs *regs, int trapnr)	\
> {								\
> 	void (*handler)(unsigned long) =			\
> 		(void *)((unsigned long)&fixup->to + fixup->to); \
> 								\
> 	if (trapnr != X86_TRAP_UD)				\
> 		return false;					\
> 								\
> 	regs->ip += 2; /* size of UD2 instruction */		\
> 	handler(regs->_reg);					\
> 	return true;						\
> }
> 
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
> 
> 
> asm (
> " .macro reg_to_handler	r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
  " .ifc \\r, %e\\rs\n"
  " ex_handler_value_\\rs\n"
  " .endif\n"
  " .ifc \\r, %r\\rs\n"
  " ex_handler_value_\\rs\n"
  " .endif\n"
> " .endr\n"
> " .endm\n"
> );

to match the 16, 32 and 64 bit names of the same registers. The byte
registers will need additional magic :/


> #define EXCEPTION_VALUE(val, handler)			\
> 	asm volatile ("1: ud2"				\
> 		      _ASM_EXTABLE_HANDLE(1b, handler,	\
> 				     reg_to_handler %0) \
> 		      : : "r" (val))
> 
> 
> Where the generic version can simply be:
> 
> #define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)
> 
> 
> Makes sense?

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Wed, Feb 08, 2017 at 10:12:50AM +0100, Peter Zijlstra wrote:
> On x86 have have __ex_table and __bug_table. The former is used for all
> sorts of things, including fixing up faults.
> 
> Now, our struct exception_table_entry has a third field used to specify
> a handler, see commit:
> 
>  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

Ah; neat!

> Still, if we want to allow a generic implementation that does a function
> call, the handler prototype should probably look like:
> 
> 	void exception_value(unsigned long value);
> 
> Which means the arch bits need a trampoline and we also need to encode
> that. The best I've come up with is having nr_regs trampolines and
> stuffing the trampoline function in the ->handler field and then using
> the ->to field to encode the actual handler.
> 
> Something like:
> 
> #define EX_REG_HANDLER(_reg)					\
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
> 			    struct pt_regs *regs, int trapnr)	\
> {								\
> 	void (*handler)(unsigned long) =			\
> 		(void *)((unsigned long)&fixup->to + fixup->to); \
> 								\
> 	if (trapnr != X86_TRAP_UD)				\
> 		return false;					\
> 								\
> 	regs->ip += 2; /* size of UD2 instruction */		\
> 	handler(regs->_reg);					\
> 	return true;						\
> }
> 
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
> 
> 
> asm (
> " .macro reg_to_handler	r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
> " .endr\n"
> " .endm\n"
> );
> 
> #define EXCEPTION_VALUE(val, handler)			\
> 	asm volatile ("1: ud2"				\
> 		      _ASM_EXTABLE_HANDLE(1b, handler,	\
> 				     reg_to_handler %0) \
> 		      : : "r" (val))
> 
> 
> Where the generic version can simply be:
> 
> #define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)
> 
> Makes sense?

That all makes sense to me.

I'll take a look at putting together an arm64 equivalent to the x86
extable patch, along with cleanup to our SW breakpoint code (which we
use in lieu for x86's UD2).

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Mark Rutland]

On Wed, Feb 08, 2017 at 10:12:50AM +0100, Peter Zijlstra wrote:
> On x86 have have __ex_table and __bug_table. The former is used for all
> sorts of things, including fixing up faults.
> 
> Now, our struct exception_table_entry has a third field used to specify
> a handler, see commit:
> 
>  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")

Ah; neat!

> Still, if we want to allow a generic implementation that does a function
> call, the handler prototype should probably look like:
> 
> 	void exception_value(unsigned long value);
> 
> Which means the arch bits need a trampoline and we also need to encode
> that. The best I've come up with is having nr_regs trampolines and
> stuffing the trampoline function in the ->handler field and then using
> the ->to field to encode the actual handler.
> 
> Something like:
> 
> #define EX_REG_HANDLER(_reg)					\
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
> 			    struct pt_regs *regs, int trapnr)	\
> {								\
> 	void (*handler)(unsigned long) =			\
> 		(void *)((unsigned long)&fixup->to + fixup->to); \
> 								\
> 	if (trapnr != X86_TRAP_UD)				\
> 		return false;					\
> 								\
> 	regs->ip += 2; /* size of UD2 instruction */		\
> 	handler(regs->_reg);					\
> 	return true;						\
> }
> 
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
> 
> 
> asm (
> " .macro reg_to_handler	r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
> " .endr\n"
> " .endm\n"
> );
> 
> #define EXCEPTION_VALUE(val, handler)			\
> 	asm volatile ("1: ud2"				\
> 		      _ASM_EXTABLE_HANDLE(1b, handler,	\
> 				     reg_to_handler %0) \
> 		      : : "r" (val))
> 
> 
> Where the generic version can simply be:
> 
> #define EXCEPTION_VALUE(val, handler)	handler((unsigned long)val)
> 
> Makes sense?

That all makes sense to me.

I'll take a look at putting together an arm64 equivalent to the x86
extable patch, along with cleanup to our SW breakpoint code (which we
use in lieu for x86's UD2).

Thanks,
Mark.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Wed, Feb 8, 2017 at 1:12 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Feb 07, 2017 at 05:55:42PM +0000, Mark Rutland wrote:
>> On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
>> > On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
>> > > For x86 it's a little painful due to '%' in the register names, but it looks
>> > > possible. The below appears to do the mangling correctly (then screams due to
>> > > the mangled result being nonexistent).
>> >
>> > > asm(
>> > > " .macro  reg_to_offset   r\n"
>> > > " .irp rs,eax,ebx,ecx,edx\n"
>> > > " .ifc \\r, %\\rs\n"
>> > > " __offset_of_\\rs\n"
>> > > " .endif\n"
>> > > " .endr\n"
>> > > " .endm\n"
>> > > );
>> > >
>> > > #define asm_sym(var)              asm volatile("reg_to_offset %0\n" : : "r" (var))
>> >
>> > Oh gawd that's a most gnarly hack.
>>
>> :)
>>
>> > Do we want to go do that for all archs or somehow cook a generic
>> > fallback that ends up doing a full function call or something?
>>
>> Given the arch-specific reg->blah mapping is so "fun", I guess a generic
>> fallback would be a good start.
>>
>> I haven't figured out all the plumbing details. It'd be nice to reuse
>> the bug infrastructure so that arches don't have to implement another
>> trap and callback pair, but I guess the reg details need to live in
>> another data structure.
>
> On x86 have have __ex_table and __bug_table. The former is used for all
> sorts of things, including fixing up faults.
>
> Now, our struct exception_table_entry has a third field used to specify
> a handler, see commit:
>
>  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")
>
> Also, given we trigger things with a known instruction at these sites,
> the ->to field is reusable and can be used to encode the register
> offset.
>
> Still, if we want to allow a generic implementation that does a function
> call, the handler prototype should probably look like:
>
>         void exception_value(unsigned long value);
>
> Which means the arch bits need a trampoline and we also need to encode
> that. The best I've come up with is having nr_regs trampolines and
> stuffing the trampoline function in the ->handler field and then using
> the ->to field to encode the actual handler.
>
> Something like:
>
> #define EX_REG_HANDLER(_reg)                                    \
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
>                             struct pt_regs *regs, int trapnr)   \
> {                                                               \
>         void (*handler)(unsigned long) =                        \
>                 (void *)((unsigned long)&fixup->to + fixup->to); \
>                                                                 \
>         if (trapnr != X86_TRAP_UD)                              \
>                 return false;                                   \
>                                                                 \
>         regs->ip += 2; /* size of UD2 instruction */            \
>         handler(regs->_reg);                                    \
>         return true;                                            \
> }
>
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
>
>
> asm (
> " .macro reg_to_handler r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
> " .endr\n"
> " .endm\n"
> );
>
> #define EXCEPTION_VALUE(val, handler)                   \
>         asm volatile ("1: ud2"                          \
>                       _ASM_EXTABLE_HANDLE(1b, handler,  \
>                                      reg_to_handler %0) \
>                       : : "r" (val))
>
>
> Where the generic version can simply be:
>
> #define EXCEPTION_VALUE(val, handler)   handler((unsigned long)val)
>
>
> Makes sense?

Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
just there to catch whatever register gcc decides to stuff the value
into? *cover face* Sure, okay. :)

I wonder how many existing WARN callsites could be repurposed to use this?

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Wed, Feb 8, 2017 at 1:12 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Tue, Feb 07, 2017 at 05:55:42PM +0000, Mark Rutland wrote:
>> On Tue, Feb 07, 2017 at 06:30:36PM +0100, Peter Zijlstra wrote:
>> > On Tue, Feb 07, 2017 at 04:03:01PM +0000, Mark Rutland wrote:
>> > > For x86 it's a little painful due to '%' in the register names, but it looks
>> > > possible. The below appears to do the mangling correctly (then screams due to
>> > > the mangled result being nonexistent).
>> >
>> > > asm(
>> > > " .macro  reg_to_offset   r\n"
>> > > " .irp rs,eax,ebx,ecx,edx\n"
>> > > " .ifc \\r, %\\rs\n"
>> > > " __offset_of_\\rs\n"
>> > > " .endif\n"
>> > > " .endr\n"
>> > > " .endm\n"
>> > > );
>> > >
>> > > #define asm_sym(var)              asm volatile("reg_to_offset %0\n" : : "r" (var))
>> >
>> > Oh gawd that's a most gnarly hack.
>>
>> :)
>>
>> > Do we want to go do that for all archs or somehow cook a generic
>> > fallback that ends up doing a full function call or something?
>>
>> Given the arch-specific reg->blah mapping is so "fun", I guess a generic
>> fallback would be a good start.
>>
>> I haven't figured out all the plumbing details. It'd be nice to reuse
>> the bug infrastructure so that arches don't have to implement another
>> trap and callback pair, but I guess the reg details need to live in
>> another data structure.
>
> On x86 have have __ex_table and __bug_table. The former is used for all
> sorts of things, including fixing up faults.
>
> Now, our struct exception_table_entry has a third field used to specify
> a handler, see commit:
>
>  548acf19234d ("x86/mm: Expand the exception table logic to allow new handling options")
>
> Also, given we trigger things with a known instruction at these sites,
> the ->to field is reusable and can be used to encode the register
> offset.
>
> Still, if we want to allow a generic implementation that does a function
> call, the handler prototype should probably look like:
>
>         void exception_value(unsigned long value);
>
> Which means the arch bits need a trampoline and we also need to encode
> that. The best I've come up with is having nr_regs trampolines and
> stuffing the trampoline function in the ->handler field and then using
> the ->to field to encode the actual handler.
>
> Something like:
>
> #define EX_REG_HANDLER(_reg)                                    \
> bool ex_handler_value_##_reg(const struct exception_table_entry *fixup, \
>                             struct pt_regs *regs, int trapnr)   \
> {                                                               \
>         void (*handler)(unsigned long) =                        \
>                 (void *)((unsigned long)&fixup->to + fixup->to); \
>                                                                 \
>         if (trapnr != X86_TRAP_UD)                              \
>                 return false;                                   \
>                                                                 \
>         regs->ip += 2; /* size of UD2 instruction */            \
>         handler(regs->_reg);                                    \
>         return true;                                            \
> }
>
> EX_REG_HANDLER(bx);
> EX_REG_HANDLER(cx);
> ...
> EX_REG_HANDLER(ss);
>
>
> asm (
> " .macro reg_to_handler r\n"
> " .irp rs,bx,cx,...,ss\n"
> " .ifc \\r, %\\rs\n"
> " ex_handler_value_\\rs\n"
> " .endif\n"
> " .endr\n"
> " .endm\n"
> );
>
> #define EXCEPTION_VALUE(val, handler)                   \
>         asm volatile ("1: ud2"                          \
>                       _ASM_EXTABLE_HANDLE(1b, handler,  \
>                                      reg_to_handler %0) \
>                       : : "r" (val))
>
>
> Where the generic version can simply be:
>
> #define EXCEPTION_VALUE(val, handler)   handler((unsigned long)val)
>
>
> Makes sense?

Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
just there to catch whatever register gcc decides to stuff the value
into? *cover face* Sure, okay. :)

I wonder how many existing WARN callsites could be repurposed to use this?

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 01:20:26PM -0800, Kees Cook wrote:

> Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
> just there to catch whatever register gcc decides to stuff the value
> into? *cover face* Sure, okay. :)

Right, they shouldn't be big functions, but barring whole program LTO
there's just no knowing which are unused.

> I wonder how many existing WARN callsites could be repurposed to use this?

At the very least all WARN/BUG instances with trivial @format argument
that are inlined I think. For example, things like:

static inline some_function()
{
	/* ... */
	WARN(cond, "blah blah blah\n");
	/* ... */
}

where the format has no arguments. Here we can out-of-line the printk()
stuff, which, as is the purpose here, shrinks the size of the inline.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Peter Zijlstra]

On Wed, Feb 08, 2017 at 01:20:26PM -0800, Kees Cook wrote:

> Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
> just there to catch whatever register gcc decides to stuff the value
> into? *cover face* Sure, okay. :)

Right, they shouldn't be big functions, but barring whole program LTO
there's just no knowing which are unused.

> I wonder how many existing WARN callsites could be repurposed to use this?

At the very least all WARN/BUG instances with trivial @format argument
that are inlined I think. For example, things like:

static inline some_function()
{
	/* ... */
	WARN(cond, "blah blah blah\n");
	/* ... */
}

where the format has no arguments. Here we can out-of-line the printk()
stuff, which, as is the purpose here, shrinks the size of the inline.

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Thu, Feb 9, 2017 at 2:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Feb 08, 2017 at 01:20:26PM -0800, Kees Cook wrote:
>
>> Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
>> just there to catch whatever register gcc decides to stuff the value
>> into? *cover face* Sure, okay. :)
>
> Right, they shouldn't be big functions, but barring whole program LTO
> there's just no knowing which are unused.
>
>> I wonder how many existing WARN callsites could be repurposed to use this?
>
> At the very least all WARN/BUG instances with trivial @format argument
> that are inlined I think. For example, things like:
>
> static inline some_function()
> {
>         /* ... */
>         WARN(cond, "blah blah blah\n");
>         /* ... */
> }
>
> where the format has no arguments. Here we can out-of-line the printk()
> stuff, which, as is the purpose here, shrinks the size of the inline.

Unless there is some other unholy macros trick, I think we'd need a
separate "WARN_CONST" or something macro to do this (i.e.
WARN_CONST(const, const_str) instead of WARN(cond, fmt, ...)), since
detecting a single-item vararg in a macro is very weird/impossible to
do. Hrmm.

-Kees

-- 
Kees Cook
Pixel Security

Re: [kernel-hardening] Re: [PATCH 4/4] refcount: Report failures through CHECK_DATA_CORRUPTION

[Kees Cook]

On Thu, Feb 9, 2017 at 2:27 AM, Peter Zijlstra <peterz@infradead.org> wrote:
> On Wed, Feb 08, 2017 at 01:20:26PM -0800, Kees Cook wrote:
>
>> Ooooh, that is intense. And the trampolines (EX_REG_HANDLERs) are all
>> just there to catch whatever register gcc decides to stuff the value
>> into? *cover face* Sure, okay. :)
>
> Right, they shouldn't be big functions, but barring whole program LTO
> there's just no knowing which are unused.
>
>> I wonder how many existing WARN callsites could be repurposed to use this?
>
> At the very least all WARN/BUG instances with trivial @format argument
> that are inlined I think. For example, things like:
>
> static inline some_function()
> {
>         /* ... */
>         WARN(cond, "blah blah blah\n");
>         /* ... */
> }
>
> where the format has no arguments. Here we can out-of-line the printk()
> stuff, which, as is the purpose here, shrinks the size of the inline.

Unless there is some other unholy macros trick, I think we'd need a
separate "WARN_CONST" or something macro to do this (i.e.
WARN_CONST(const, const_str) instead of WARN(cond, fmt, ...)), since
detecting a single-item vararg in a macro is very weird/impossible to
do. Hrmm.

-Kees

-- 
Kees Cook
Pixel Security