Date:   Tue, 25 Jul 2017 11:33:56 -0700
From:   Kees Cook <keescook@...omium.org>
To:     linux-kernel@...r.kernel.org
Cc:     Kees Cook <keescook@...omium.org>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        hw.likun@...wei.com, Arnd Bergmann <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: [PATCH 1/2] lkdtm: Provide more complete coverage for REFCOUNT tests

The existing REFCOUNT_* LKDTM tests were designed only for testing a narrow
portion of CONFIG_REFCOUNT_FULL. This moves the tests to their own file and
expands their testing to poke each boundary condition.

Since the protections (CONFIG_REFCOUNT_FULL and x86-fast) use different
saturation values and reach-zero behavior, those have to be build-time
set so the tests can actually validate things are happening at the
right places.

Notably, the x86-fast protection will fail REFCOUNT_INC_ZERO and
REFCOUNT_ADD_ZERO since those conditions are not checked (only overflow
is critical to protecting refcount_t). CONFIG_REFCOUNT_FULL will warn for
each REFCOUNT_*_NEGATIVE test since it provides zero-pinning behaviors
(which allows it to pass REFCOUNT_INC_ZERO and REFCOUNT_ADD_ZERO).

Signed-off-by: Kees Cook <keescook@...omium.org>
---
 drivers/misc/Makefile         |   1 +
 drivers/misc/lkdtm.h          |  21 ++-
 drivers/misc/lkdtm_bugs.c     |  83 ------------
 drivers/misc/lkdtm_core.c     |  19 ++-
 drivers/misc/lkdtm_refcount.c | 308 ++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 337 insertions(+), 95 deletions(-)
 create mode 100644 drivers/misc/lkdtm_refcount.c

diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index b0b766416306..d84819dc2468 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -60,6 +60,7 @@ lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_heap.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_perms.o
+lkdtm-$(CONFIG_LKDTM)		+= lkdtm_refcount.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_rodata_objcopy.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_usercopy.o
 
diff --git a/drivers/misc/lkdtm.h b/drivers/misc/lkdtm.h
index 3b4976396ec4..241d6f3a26dd 100644
--- a/drivers/misc/lkdtm.h
+++ b/drivers/misc/lkdtm.h
@@ -19,12 +19,6 @@ void lkdtm_SOFTLOCKUP(void);
 void lkdtm_HARDLOCKUP(void);
 void lkdtm_SPINLOCKUP(void);
 void lkdtm_HUNG_TASK(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);
 void lkdtm_CORRUPT_USER_DS(void);
@@ -49,6 +43,21 @@ void lkdtm_EXEC_RODATA(void);
 void lkdtm_EXEC_USERSPACE(void);
 void lkdtm_ACCESS_USERSPACE(void);
 
+/* lkdtm_refcount.c */
+void lkdtm_REFCOUNT_INC_OVERFLOW(void);
+void lkdtm_REFCOUNT_ADD_OVERFLOW(void);
+void lkdtm_REFCOUNT_DEC_ZERO(void);
+void lkdtm_REFCOUNT_DEC_NEGATIVE(void);
+void lkdtm_REFCOUNT_DEC_AND_TEST_NEGATIVE(void);
+void lkdtm_REFCOUNT_SUB_AND_TEST_NEGATIVE(void);
+void lkdtm_REFCOUNT_INC_ZERO(void);
+void lkdtm_REFCOUNT_ADD_ZERO(void);
+void lkdtm_REFCOUNT_INC_SATURATED(void);
+void lkdtm_REFCOUNT_DEC_SATURATED(void);
+void lkdtm_REFCOUNT_ADD_SATURATED(void);
+void lkdtm_REFCOUNT_DEC_AND_TEST_SATURATED(void);
+void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void);
+
 /* lkdtm_rodata.c */
 void lkdtm_rodata_do_nothing(void);
 
diff --git a/drivers/misc/lkdtm_bugs.c b/drivers/misc/lkdtm_bugs.c
index d9028ef50fbe..ef3d06f901fc 100644
--- a/drivers/misc/lkdtm_bugs.c
+++ b/drivers/misc/lkdtm_bugs.c
@@ -6,7 +6,6 @@
  */
 #include "lkdtm.h"
 #include <linux/list.h>
-#include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/sched/signal.h>
 #include <linux/uaccess.h>
@@ -137,88 +136,6 @@ void lkdtm_HUNG_TASK(void)
 	schedule();
 }
 
-void lkdtm_REFCOUNT_SATURATE_INC(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 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_REFCOUNT_ZERO_SUB(void)
-{
-	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");
-}
-
-void lkdtm_REFCOUNT_ZERO_INC(void)
-{
-	refcount_t zero = REFCOUNT_INIT(0);
-
-	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 42d2b8e31e6b..c3b045aec10e 100644
--- a/drivers/misc/lkdtm_core.c
+++ b/drivers/misc/lkdtm_core.c
@@ -221,12 +221,19 @@ struct crashtype crashtypes[] = {
 	CRASHTYPE(WRITE_RO),
 	CRASHTYPE(WRITE_RO_AFTER_INIT),
 	CRASHTYPE(WRITE_KERN),
-	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(REFCOUNT_INC_OVERFLOW),
+	CRASHTYPE(REFCOUNT_ADD_OVERFLOW),
+	CRASHTYPE(REFCOUNT_DEC_ZERO),
+	CRASHTYPE(REFCOUNT_DEC_NEGATIVE),
+	CRASHTYPE(REFCOUNT_DEC_AND_TEST_NEGATIVE),
+	CRASHTYPE(REFCOUNT_SUB_AND_TEST_NEGATIVE),
+	CRASHTYPE(REFCOUNT_INC_ZERO),
+	CRASHTYPE(REFCOUNT_ADD_ZERO),
+	CRASHTYPE(REFCOUNT_INC_SATURATED),
+	CRASHTYPE(REFCOUNT_DEC_SATURATED),
+	CRASHTYPE(REFCOUNT_ADD_SATURATED),
+	CRASHTYPE(REFCOUNT_DEC_AND_TEST_SATURATED),
+	CRASHTYPE(REFCOUNT_SUB_AND_TEST_SATURATED),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_TO),
 	CRASHTYPE(USERCOPY_HEAP_SIZE_FROM),
 	CRASHTYPE(USERCOPY_HEAP_FLAG_TO),
diff --git a/drivers/misc/lkdtm_refcount.c b/drivers/misc/lkdtm_refcount.c
new file mode 100644
index 000000000000..234d39a07a99
--- /dev/null
+++ b/drivers/misc/lkdtm_refcount.c
@@ -0,0 +1,308 @@
+/*
+ * This is for all the tests related to refcount bugs (e.g. overflow,
+ * underflow, reaching zero untested, etc).
+ */
+#include "lkdtm.h"
+#include <linux/refcount.h>
+
+#ifdef CONFIG_REFCOUNT_FULL
+#define REFCOUNT_MAX		(UINT_MAX - 1)
+#define REFCOUNT_SATURATED	UINT_MAX
+#else
+#define REFCOUNT_MAX		INT_MAX
+#define REFCOUNT_SATURATED	(INT_MIN / 2)
+#endif
+
+static void overflow_check(refcount_t *ref)
+{
+	switch (refcount_read(ref)) {
+	case REFCOUNT_SATURATED:
+		pr_info("Overflow detected: saturated\n");
+		break;
+	case REFCOUNT_MAX:
+		pr_warn("Overflow detected: unsafely reset to max\n");
+		break;
+	default:
+		pr_err("Fail: refcount wrapped to %d\n", refcount_read(ref));
+	}
+}
+
+/*
+ * A refcount_inc() above the maximum value of the refcount implementation,
+ * should at least saturate, and at most also WARN.
+ */
+void lkdtm_REFCOUNT_INC_OVERFLOW(void)
+{
+	refcount_t over = REFCOUNT_INIT(REFCOUNT_MAX - 1);
+
+	pr_info("attempting good refcount_inc() without overflow\n");
+	refcount_dec(&over);
+	refcount_inc(&over);
+
+	pr_info("attempting bad refcount_inc() overflow\n");
+	refcount_inc(&over);
+	refcount_inc(&over);
+
+	overflow_check(&over);
+}
+
+/* refcount_add() should behave just like refcount_inc() above. */
+void lkdtm_REFCOUNT_ADD_OVERFLOW(void)
+{
+	refcount_t over = REFCOUNT_INIT(REFCOUNT_MAX - 1);
+
+	pr_info("attempting good refcount_add() without overflow\n");
+	refcount_dec(&over);
+	refcount_dec(&over);
+	refcount_dec(&over);
+	refcount_dec(&over);
+	refcount_add(4, &over);
+
+	pr_info("attempting bad refcount_add() overflow\n");
+	refcount_add(4, &over);
+
+	overflow_check(&over);
+}
+
+static void check_zero(refcount_t *ref)
+{
+	switch (refcount_read(ref)) {
+	case REFCOUNT_SATURATED:
+		pr_info("Zero detected: saturated\n");
+		break;
+	case REFCOUNT_MAX:
+		pr_warn("Zero detected: unsafely reset to max\n");
+		break;
+	case 0:
+		pr_warn("Still at zero: refcount_inc/add() must not inc-from-0\n");
+		break;
+	default:
+		pr_err("Fail: refcount went crazy: %d\n", refcount_read(ref));
+	}
+}
+
+/*
+ * A refcount_dec(), as opposed to a refcount_dec_and_test(), when it hits
+ * zero it should either saturate (when inc-from-zero isn't protected)
+ * or stay at zero (when inc-from-zero is protected) and should WARN for both.
+ */
+void lkdtm_REFCOUNT_DEC_ZERO(void)
+{
+	refcount_t zero = REFCOUNT_INIT(2);
+
+	pr_info("attempting good refcount_dec()\n");
+	refcount_dec(&zero);
+
+	pr_info("attempting bad refcount_dec() to zero\n");
+	refcount_dec(&zero);
+
+	check_zero(&zero);
+}
+
+static void check_negative(refcount_t *ref, int start)
+{
+	/*
+	 * CONFIG_REFCOUNT_FULL refuses to move a refcount at all on an
+	 * over-sub, so we have to track our starting position instead of
+	 * looking only at zero-pinning.
+	 */
+	if (refcount_read(ref) == start) {
+		pr_warn("Still at %d: refcount_inc/add() must not inc-from-0\n",
+			start);
+		return;
+	}
+
+	switch (refcount_read(ref)) {
+	case REFCOUNT_SATURATED:
+		pr_info("Negative detected: saturated\n");
+		break;
+	case REFCOUNT_MAX:
+		pr_warn("Negative detected: unsafely reset to max\n");
+		break;
+	default:
+		pr_err("Fail: refcount went crazy: %d\n", refcount_read(ref));
+	}
+}
+
+/* A refcount_dec() going negative should saturate and may WARN. */
+void lkdtm_REFCOUNT_DEC_NEGATIVE(void)
+{
+	refcount_t neg = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount_dec() below zero\n");
+	refcount_dec(&neg);
+
+	check_negative(&neg, 0);
+}
+
+/*
+ * A refcount_dec_and_test() should act like refcount_dec() above when
+ * going negative.
+ */
+void lkdtm_REFCOUNT_DEC_AND_TEST_NEGATIVE(void)
+{
+	refcount_t neg = REFCOUNT_INIT(0);
+
+	pr_info("attempting bad refcount_dec_and_test() below zero\n");
+	if (refcount_dec_and_test(&neg))
+		pr_warn("Weird: refcount_dec_and_test() reported zero\n");
+
+	check_negative(&neg, 0);
+}
+
+/*
+ * A refcount_sub_and_test() should act like refcount_dec_and_test()
+ * above when going negative.
+ */
+void lkdtm_REFCOUNT_SUB_AND_TEST_NEGATIVE(void)
+{
+	refcount_t neg = REFCOUNT_INIT(3);
+
+	pr_info("attempting bad refcount_sub_and_test() below zero\n");
+	if (refcount_sub_and_test(5, &neg))
+		pr_warn("Weird: refcount_sub_and_test() reported zero\n");
+
+	check_negative(&neg, 3);
+}
+
+static void check_from_zero(refcount_t *ref)
+{
+	switch (refcount_read(ref)) {
+	case 0:
+		pr_info("Zero detected: stayed at zero\n");
+		break;
+	case REFCOUNT_SATURATED:
+		pr_info("Zero detected: saturated\n");
+		break;
+	case REFCOUNT_MAX:
+		pr_warn("Zero detected: unsafely reset to max\n");
+		break;
+	default:
+		pr_info("Fail: zero not detected, incremeted to %d\n",
+			refcount_read(ref));
+	}
+}
+
+/*
+ * A refcount_inc() from zero should pin to zero or saturate and may WARN.
+ * Only CONFIG_REFCOUNT_FULL provides this protection currently.
+ */
+void lkdtm_REFCOUNT_INC_ZERO(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
+
+	pr_info("attempting safe refcount_inc_not_zero() from zero\n");
+	if (!refcount_inc_not_zero(&zero)) {
+		pr_info("Good: zero detected\n");
+		if (refcount_read(&zero) == 0)
+			pr_info("Correctly stayed at zero\n");
+		else
+			pr_err("Fail: refcount went past zero!\n");
+	} else {
+		pr_err("Fail: Zero not detected!?\n");
+	}
+
+	pr_info("attempting bad refcount_inc() from zero\n");
+	refcount_inc(&zero);
+
+	check_from_zero(&zero);
+}
+
+/*
+ * A refcount_add() should act like refcount_inc() above when starting
+ * at zero.
+ */
+void lkdtm_REFCOUNT_ADD_ZERO(void)
+{
+	refcount_t zero = REFCOUNT_INIT(0);
+
+	pr_info("attempting safe refcount_add_not_zero() from zero\n");
+	if (!refcount_add_not_zero(3, &zero)) {
+		pr_info("Good: zero detected\n");
+		if (refcount_read(&zero) == 0)
+			pr_info("Correctly stayed at zero\n");
+		else
+			pr_err("Fail: refcount went past zero\n");
+	} else {
+		pr_err("Fail: Zero not detected!?\n");
+	}
+
+	pr_info("attempting bad refcount_add() from zero\n");
+	refcount_add(3, &zero);
+
+	check_from_zero(&zero);
+}
+
+static void check_saturated(refcount_t *ref)
+{
+	switch (refcount_read(ref)) {
+	case REFCOUNT_SATURATED:
+		pr_info("Saturation detected: still saturated\n");
+		break;
+	case REFCOUNT_MAX:
+		pr_warn("Saturation detected: unsafely reset to max\n");
+		break;
+	default:
+		pr_err("Fail: refcount went crazy: %d\n", refcount_read(ref));
+	}
+}
+
+/*
+ * A refcount_inc() from a saturated value should at most warn about
+ * being saturated already.
+ */
+void lkdtm_REFCOUNT_INC_SATURATED(void)
+{
+	refcount_t sat = REFCOUNT_INIT(REFCOUNT_SATURATED);
+
+	pr_info("attempting bad refcount_inc() from saturated\n");
+	refcount_inc(&sat);
+
+	check_saturated(&sat);
+}
+
+/* Should act like refcount_inc() above from saturated. */
+void lkdtm_REFCOUNT_DEC_SATURATED(void)
+{
+	refcount_t sat = REFCOUNT_INIT(REFCOUNT_SATURATED);
+
+	pr_info("attempting bad refcount_dec() from saturated\n");
+	refcount_dec(&sat);
+
+	check_saturated(&sat);
+}
+
+/* Should act like refcount_inc() above from saturated. */
+void lkdtm_REFCOUNT_ADD_SATURATED(void)
+{
+	refcount_t sat = REFCOUNT_INIT(REFCOUNT_SATURATED);
+
+	pr_info("attempting bad refcount_dec() from saturated\n");
+	refcount_add(8, &sat);
+
+	check_saturated(&sat);
+}
+
+/* Should act like refcount_inc() above from saturated. */
+void lkdtm_REFCOUNT_DEC_AND_TEST_SATURATED(void)
+{
+	refcount_t sat = REFCOUNT_INIT(REFCOUNT_SATURATED);
+
+	pr_info("attempting bad refcount_dec_and_test() from saturated\n");
+	if (refcount_dec_and_test(&sat))
+		pr_warn("Weird: refcount_dec_and_test() reported zero\n");
+
+	check_saturated(&sat);
+}
+
+/* Should act like refcount_inc() above from saturated. */
+void lkdtm_REFCOUNT_SUB_AND_TEST_SATURATED(void)
+{
+	refcount_t sat = REFCOUNT_INIT(REFCOUNT_SATURATED);
+
+	pr_info("attempting bad refcount_sub_and_test() from saturated\n");
+	if (refcount_sub_and_test(8, &sat))
+		pr_warn("Weird: refcount_sub_and_test() reported zero\n");
+
+	check_saturated(&sat);
+}
-- 
2.7.4

Powered by blists - more mailing lists