This continues in applying the CHECK_DATA_CORRUPTION tests where
appropriate, and pulling similar CONFIGs under the same check. Most
notably, this adds the checks to refcount_t so that system builders can
Oops their kernels when encountering a potential refcounter attack. (And
so now the LKDTM tests for refcount issues pass correctly.)
The series depends on the changes in -next made to lib/refcount.c,
so it might be easiest if this goes through the locking tree...
v2 is a rebase to -next and adjusts to using WARN_ONCE() instead of WARN().
-Kees
v1 was here: https://lkml.org/lkml/2017/3/6/720
Similar to CONFIG_DEBUG_CREDENTIALS, CONFIG_DEBUG_SG already handles
calling BUG, and performs inexpensive checks. This enables it under
CONFIG_BUG_ON_DATA_CORRUPTION.
Signed-off-by: Kees Cook <[email protected]>
---
lib/Kconfig.debug | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 9a1b6b56cef4..45bfc0be38fc 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1268,7 +1268,7 @@ config DEBUG_PI_LIST
config DEBUG_SG
bool "Debug SG table operations"
- depends on DEBUG_KERNEL
+ depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
help
Enable this to turn on checks on scatter-gather tables. This can
help find problems with drivers that do not properly initialize
@@ -1998,6 +1998,7 @@ config BUG_ON_DATA_CORRUPTION
bool "Trigger a BUG when data corruption is detected"
select DEBUG_CREDENTIALS
select DEBUG_LIST
+ select DEBUG_SG
help
This option enables several inexpensive data corruption checks.
Most of these checks normally just WARN and try to further avoid
--
2.7.4
This improves the compiler branch-hinting used in CHECK_DATA_CORRUPTION(),
similar to how it is done in WARN_ON() and friends.
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/bug.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index db1e41c69bac..b6cfcb7f778f 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -130,15 +130,15 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
static inline __must_check bool check_data_corruption(bool v) { return v; }
#define CHECK_DATA_CORRUPTION(condition, fmt, ...) \
check_data_corruption(({ \
- bool corruption = unlikely(condition); \
- if (corruption) { \
+ bool corruption = !!(condition); \
+ if (unlikely(corruption)) { \
if (IS_ENABLED(CONFIG_BUG_ON_DATA_CORRUPTION)) { \
pr_err(fmt, ##__VA_ARGS__); \
BUG(); \
} else \
WARN(1, fmt, ##__VA_ARGS__); \
} \
- corruption; \
+ unlikely(corruption); \
}))
#endif /* _LINUX_BUG_H */
--
2.7.4
This converts from WARN_ONCE() to CHECK_DATA_CORRUPTION() (so that system
builders can choose between WARN and BUG). Additionally moves refcount_t
sanity-check conditionals into regular function flow.
Now when built with CONFIG_BUG_ON_DATA_CORRUPTION, the LKDTM REFCOUNT_*
tests correctly kill offending processes.
Signed-off-by: Kees Cook <[email protected]>
---
lib/refcount.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/lib/refcount.c b/lib/refcount.c
index f42124ccf295..88289210d9fd 100644
--- a/lib/refcount.c
+++ b/lib/refcount.c
@@ -37,6 +37,13 @@
#include <linux/refcount.h>
#include <linux/bug.h>
+/*
+ * CHECK_DATA_CORRUPTION() is defined with __must_check, but we have a
+ * couple places where we want to report a condition that has already
+ * been checked, so this lets us cheat __must_check.
+ */
+#define REFCOUNT_CHECK(cond, str) unlikely(CHECK_DATA_CORRUPTION(cond, str))
+
/**
* refcount_add_not_zero - add a value to a refcount unless it is 0
* @i: the value to add to the refcount
@@ -72,7 +79,8 @@ bool refcount_add_not_zero(unsigned int i, refcount_t *r)
} while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
- WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+ REFCOUNT_CHECK(new == UINT_MAX,
+ "refcount_t: add saturated; leaking memory.\n");
return true;
}
@@ -96,7 +104,8 @@ EXPORT_SYMBOL_GPL(refcount_add_not_zero);
*/
void refcount_add(unsigned int i, refcount_t *r)
{
- WARN_ONCE(!refcount_add_not_zero(i, r), "refcount_t: addition on 0; use-after-free.\n");
+ REFCOUNT_CHECK(!refcount_add_not_zero(i, r),
+ "refcount_t: addition on 0; use-after-free.\n");
}
EXPORT_SYMBOL_GPL(refcount_add);
@@ -127,7 +136,8 @@ bool refcount_inc_not_zero(refcount_t *r)
} while (!atomic_try_cmpxchg_relaxed(&r->refs, &val, new));
- WARN_ONCE(new == UINT_MAX, "refcount_t: saturated; leaking memory.\n");
+ REFCOUNT_CHECK(new == UINT_MAX,
+ "refcount_t: inc saturated; leaking memory.\n");
return true;
}
@@ -147,7 +157,8 @@ EXPORT_SYMBOL_GPL(refcount_inc_not_zero);
*/
void refcount_inc(refcount_t *r)
{
- WARN_ONCE(!refcount_inc_not_zero(r), "refcount_t: increment on 0; use-after-free.\n");
+ REFCOUNT_CHECK(!refcount_inc_not_zero(r),
+ "refcount_t: increment on 0; use-after-free.\n");
}
EXPORT_SYMBOL_GPL(refcount_inc);
@@ -180,10 +191,9 @@ bool refcount_sub_and_test(unsigned int i, refcount_t *r)
return false;
new = val - i;
- if (new > val) {
- WARN_ONCE(new > val, "refcount_t: underflow; use-after-free.\n");
+ if (REFCOUNT_CHECK(new > val,
+ "refcount_t: sub underflow; use-after-free.\n"))
return false;
- }
} while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
@@ -222,7 +232,8 @@ EXPORT_SYMBOL_GPL(refcount_dec_and_test);
*/
void refcount_dec(refcount_t *r)
{
- WARN_ONCE(refcount_dec_and_test(r), "refcount_t: decrement hit 0; leaking memory.\n");
+ REFCOUNT_CHECK(refcount_dec_and_test(r),
+ "refcount_t: decrement hit 0; leaking memory.\n");
}
EXPORT_SYMBOL_GPL(refcount_dec);
@@ -273,10 +284,9 @@ bool refcount_dec_not_one(refcount_t *r)
return false;
new = val - 1;
- if (new > val) {
- WARN_ONCE(new > val, "refcount_t: underflow; use-after-free.\n");
+ if (REFCOUNT_CHECK(new > val,
+ "refcount_t: dec underflow; use-after-free.\n"))
return true;
- }
} while (!atomic_try_cmpxchg_release(&r->refs, &val, new));
@@ -345,4 +355,3 @@ bool refcount_dec_and_lock(refcount_t *r, spinlock_t *lock)
return true;
}
EXPORT_SYMBOL_GPL(refcount_dec_and_lock);
-
--
2.7.4
Since CONFIG_DEBUG_CREDENTIALS already handles reporting and issuing a
BUG when it encounters corruption, add this to the list of corruption
test CONFIGs that are enabled under CONFIG_BUG_ON_DATA_CORRUPTION.
Signed-off-by: Kees Cook <[email protected]>
---
lib/Kconfig.debug | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 5ac4d1148385..9a1b6b56cef4 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1288,7 +1288,7 @@ config DEBUG_NOTIFIERS
config DEBUG_CREDENTIALS
bool "Debug credential management"
- depends on DEBUG_KERNEL
+ depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
help
Enable this to turn on some debug checking for credential
management. The additional code keeps track of the number of
@@ -1996,6 +1996,7 @@ config TEST_STATIC_KEYS
config BUG_ON_DATA_CORRUPTION
bool "Trigger a BUG when data corruption is detected"
+ select DEBUG_CREDENTIALS
select DEBUG_LIST
help
This option enables several inexpensive data corruption checks.
--
2.7.4
When performing notifier function pointer sanity checking, allow
CONFIG_BUG_ON_DATA_CORRUPTION to upgrade from a WARN to a BUG.
Additionally enables CONFIG_DEBUG_NOTIFIERS when selecting
CONFIG_BUG_ON_DATA_CORRUPTION.
Signed-off-by: Kees Cook <[email protected]>
---
kernel/notifier.c | 5 +++--
lib/Kconfig.debug | 3 ++-
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/kernel/notifier.c b/kernel/notifier.c
index 6196af8a8223..58cc14958d92 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -84,8 +84,9 @@ static int notifier_call_chain(struct notifier_block **nl,
next_nb = rcu_dereference_raw(nb->next);
#ifdef CONFIG_DEBUG_NOTIFIERS
- if (unlikely(!func_ptr_is_kernel_text(nb->notifier_call))) {
- WARN(1, "Invalid notifier called!");
+ if (CHECK_DATA_CORRUPTION(
+ !func_ptr_is_kernel_text(nb->notifier_call),
+ "Invalid notifier called!")) {
nb = next_nb;
continue;
}
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 45bfc0be38fc..0dc8f6065be8 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1278,7 +1278,7 @@ config DEBUG_SG
config DEBUG_NOTIFIERS
bool "Debug notifier call chains"
- depends on DEBUG_KERNEL
+ depends on DEBUG_KERNEL || BUG_ON_DATA_CORRUPTION
help
Enable this to turn on sanity checking for notifier call chains.
This is most useful for kernel developers to make sure that
@@ -1998,6 +1998,7 @@ config BUG_ON_DATA_CORRUPTION
bool "Trigger a BUG when data corruption is detected"
select DEBUG_CREDENTIALS
select DEBUG_LIST
+ select DEBUG_NOTIFIERS
select DEBUG_SG
help
This option enables several inexpensive data corruption checks.
--
2.7.4
Since users of CHECK_DATA_CORRUPTION() should be failing safe, the
condition that triggers a WARN() may recur. This would mean a logging
DoS of the system, so switch to WARN_ONCE() instead. (Those wanting
per-instance notifications should already be building with
CONFIG_BUG_ON_DATA_CORRUPTION so there is no change in that case.)
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/bug.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index b6cfcb7f778f..c011438a7c6c 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -136,7 +136,7 @@ static inline __must_check bool check_data_corruption(bool v) { return v; }
pr_err(fmt, ##__VA_ARGS__); \
BUG(); \
} else \
- WARN(1, fmt, ##__VA_ARGS__); \
+ WARN_ONCE(1, fmt, ##__VA_ARGS__); \
} \
unlikely(corruption); \
}))
--
2.7.4
This expands on the Kconfig help text for CONFIG_BUG_ON_DATA_CORRUPTION to
more clearly explain the rationale of BUG vs WARN. Additionally fixes a
grammar glitch in the macro comment.
Signed-off-by: Kees Cook <[email protected]>
---
include/linux/bug.h | 3 ++-
lib/Kconfig.debug | 9 ++++++---
2 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/include/linux/bug.h b/include/linux/bug.h
index 687b557fc5eb..db1e41c69bac 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -124,7 +124,8 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
/*
* Since detected data corruption should stop operation on the affected
- * structures. Return value must be checked and sanely acted on by caller.
+ * structures, require that the condition is checked and sanely acted on
+ * by the caller.
*/
static inline __must_check bool check_data_corruption(bool v) { return v; }
#define CHECK_DATA_CORRUPTION(condition, fmt, ...) \
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 77fadface4f9..5ac4d1148385 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1998,9 +1998,12 @@ config BUG_ON_DATA_CORRUPTION
bool "Trigger a BUG when data corruption is detected"
select DEBUG_LIST
help
- Select this option if the kernel should BUG when it encounters
- data corruption in kernel memory structures when they get checked
- for validity.
+ This option enables several inexpensive data corruption checks.
+ Most of these checks normally just WARN and try to further avoid
+ the corruption. Selecting this option upgrades these to BUGs so
+ that the offending process is killed. Additionally, the system
+ owner can furhter configure the system for immediate reboots
+ (via panic_on_oops sysctl) or crash dumps.
If unsure, say N.
--
2.7.4
> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
> index 77fadface4f9..5ac4d1148385 100644
> --- a/lib/Kconfig.debug
> +++ b/lib/Kconfig.debug
> @@ -1998,9 +1998,12 @@ config BUG_ON_DATA_CORRUPTION
> > ? bool "Trigger a BUG when data corruption is detected"
> > ? select DEBUG_LIST
> > ? help
> > - ??Select this option if the kernel should BUG when it encounters
> > - ??data corruption in kernel memory structures when they get checked
> > - ??for validity.
> > + ??This option enables several inexpensive data corruption checks.
> > + ??Most of these checks normally just WARN and try to further avoid
> + ??the corruption. Selecting this option upgrades these to BUGs so
First it says it enables some checks, but here it says it upgrades them
to BUGs which seems inconsistent.
> + ??that the offending process is killed. Additionally, the system
> + ??owner can furhter configure the system for immediate reboots
"further"
> + ??(via panic_on_oops sysctl) or crash dumps.
> ?
> > ? ??If unsure, say N.
> ?
On Tue, Apr 4, 2017 at 10:47 PM, Ian Campbell <[email protected]> wrote:
>> diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
>> index 77fadface4f9..5ac4d1148385 100644
>> --- a/lib/Kconfig.debug
>> +++ b/lib/Kconfig.debug
>> @@ -1998,9 +1998,12 @@ config BUG_ON_DATA_CORRUPTION
>> > bool "Trigger a BUG when data corruption is detected"
>> > select DEBUG_LIST
>> > help
>> > - Select this option if the kernel should BUG when it encounters
>> > - data corruption in kernel memory structures when they get checked
>> > - for validity.
>> > + This option enables several inexpensive data corruption checks.
>> > + Most of these checks normally just WARN and try to further avoid
>> + the corruption. Selecting this option upgrades these to BUGs so
>
> First it says it enables some checks, but here it says it upgrades them
> to BUGs which seems inconsistent.
Right, it does both. It uses Kconfig "select" to enable checks, and
raises checks from WARN to BUG.
>
>> + that the offending process is killed. Additionally, the system
>> + owner can furhter configure the system for immediate reboots
>
> "further"
Ah, thanks!
>
>> + (via panic_on_oops sysctl) or crash dumps.
>>
>> > If unsure, say N.
>>
-Kees
--
Kees Cook
Pixel Security
On Tue, Apr 04, 2017 at 03:12:11PM -0700, Kees Cook wrote:
> This continues in applying the CHECK_DATA_CORRUPTION tests where
> appropriate, and pulling similar CONFIGs under the same check. Most
> notably, this adds the checks to refcount_t so that system builders can
> Oops their kernels when encountering a potential refcounter attack. (And
> so now the LKDTM tests for refcount issues pass correctly.)
>
> The series depends on the changes in -next made to lib/refcount.c,
> so it might be easiest if this goes through the locking tree...
>
> v2 is a rebase to -next and adjusts to using WARN_ONCE() instead of WARN().
>
> -Kees
>
> v1 was here: https://lkml.org/lkml/2017/3/6/720
Ping? Just wondering what ever happened to this 3+ year old series...
--
Josh