[PATCH v2] kfence: enable check kfence canary in panic via boot param

[PATCH v2] kfence: enable check kfence canary in panic via boot param

[Shaobo Huang]

From: huangshaobo <huangshaobo6@huawei.com>

when writing out of bounds to the red zone, it can only be
detected at kfree. However, the system may have been reset
before freeing the memory, which would result in undetected
oob. Therefore, it is necessary to detect oob behavior in
panic. Since only the allocated mem call stack is available,
it may be difficult to find the oob maker. Therefore, this
feature is disabled by default and can only be enabled via
boot parameter.

Suggested-by: chenzefeng <chenzefeng2@huawei.com>
Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
---
v2:
- it is only detected in panic.
- it is disabled by default.
- can only be enabled via boot parameter.
- the code is moved to the specified partition.
Thanks to Marco for the valuable modification suggestion.
---
 mm/kfence/core.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 9b2b5f56f4ae..0b2b934a1666 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -29,6 +29,8 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
+#include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 
 #include <asm/kfence.h>
 
@@ -99,6 +101,10 @@ module_param_named(skip_covered_thresh, kfence_skip_covered_thresh, ulong, 0644)
 static bool kfence_deferrable __read_mostly = IS_ENABLED(CONFIG_KFENCE_DEFERRABLE);
 module_param_named(deferrable, kfence_deferrable, bool, 0444);
 
+/* If true, check kfence canary in panic. */
+static bool kfence_check_on_panic;
+module_param_named(check_on_panic, kfence_check_on_panic, bool, 0444);
+
 /* The pool of pages used for guard pages and objects. */
 char *__kfence_pool __read_mostly;
 EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */
@@ -727,6 +733,30 @@ static int __init kfence_debugfs_init(void)
 
 late_initcall(kfence_debugfs_init);
 
+/* === Panic Notifier ====================================================== */
+static void kfence_check_all_canary(void)
+{
+	int i;
+
+	for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
+		struct kfence_metadata *meta = &kfence_metadata[i];
+
+		if (meta->state == KFENCE_OBJECT_ALLOCATED)
+			for_each_canary(meta, check_canary_byte);
+	}
+}
+
+static int kfence_check_canary_callback(struct notifier_block *nb,
+					unsigned long reason, void *arg)
+{
+	kfence_check_all_canary();
+	return NOTIFY_OK;
+}
+
+static struct notifier_block kfence_check_canary_notifier = {
+	.notifier_call = kfence_check_canary_callback,
+};
+
 /* === Allocation Gate Timer ================================================ */
 
 static struct delayed_work kfence_timer;
@@ -804,6 +834,9 @@ static void kfence_init_enable(void)
 	else
 		INIT_DELAYED_WORK(&kfence_timer, toggle_allocation_gate);
 
+	if (kfence_check_on_panic)
+		atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
+
 	WRITE_ONCE(kfence_enabled, true);
 	queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
 
-- 
2.12.3

Re: [PATCH v2] kfence: enable check kfence canary in panic via boot param

[Marco Elver]

On Sun, 24 Apr 2022 at 13:00, Shaobo Huang <huangshaobo6@huawei.com> wrote:
>
> From: huangshaobo <huangshaobo6@huawei.com>
>
> when writing out of bounds to the red zone, it can only be
> detected at kfree. However, the system may have been reset
> before freeing the memory, which would result in undetected
> oob. Therefore, it is necessary to detect oob behavior in
> panic. Since only the allocated mem call stack is available,
> it may be difficult to find the oob maker. Therefore, this
> feature is disabled by default and can only be enabled via
> boot parameter.

This description is still not telling the full story or usecase. The
story goes something like:
"""
Out-of-bounds accesses that aren't caught by a guard page will result
in corruption of canary memory. In pathological cases, where an object
has certain alignment requirements, an out-of-bounds access might
never be caught by the guard page. Such corruptions, however, are only
detected on kfree() normally. If the bug causes the kernel to panic
before kfree(), KFENCE has no opportunity to report the issue. Such
corruptions may also indicate failing memory or other faults.

To provide some more information in such cases, add the option to
check canary bytes on panic. This might help narrow the search for the
panic cause; but, due to only having the allocation stack trace, such
reports are difficult to use to diagnose an issue alone. In most
cases, such reports are inactionable, and is therefore an opt-in
feature (disabled by default).
"""

Please feel free to copy or take pieces above to complete the commit message.

> Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
> ---
> v2:
> - it is only detected in panic.
> - it is disabled by default.
> - can only be enabled via boot parameter.
> - the code is moved to the specified partition.
> Thanks to Marco for the valuable modification suggestion.
> ---
>  mm/kfence/core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 9b2b5f56f4ae..0b2b934a1666 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -29,6 +29,8 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> +#include <linux/notifier.h>
> +#include <linux/panic_notifier.h>

Please keep these includes sorted alphabetically.

>  #include <asm/kfence.h>
>
> @@ -99,6 +101,10 @@ module_param_named(skip_covered_thresh, kfence_skip_covered_thresh, ulong, 0644)
>  static bool kfence_deferrable __read_mostly = IS_ENABLED(CONFIG_KFENCE_DEFERRABLE);
>  module_param_named(deferrable, kfence_deferrable, bool, 0444);
>
> +/* If true, check kfence canary in panic. */

It should be "on panic". E.g. "If true, check all canary bytes on panic."

> +static bool kfence_check_on_panic;
> +module_param_named(check_on_panic, kfence_check_on_panic, bool, 0444);
> +
>  /* The pool of pages used for guard pages and objects. */
>  char *__kfence_pool __read_mostly;
>  EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */
> @@ -727,6 +733,30 @@ static int __init kfence_debugfs_init(void)
>
>  late_initcall(kfence_debugfs_init);
>
> +/* === Panic Notifier ====================================================== */

Blank line between /* === ... */ and function.

> +static void kfence_check_all_canary(void)
> +{
> +       int i;
> +
> +       for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
> +               struct kfence_metadata *meta = &kfence_metadata[i];
> +
> +               if (meta->state == KFENCE_OBJECT_ALLOCATED)
> +                       for_each_canary(meta, check_canary_byte);
> +       }
> +}
> +
> +static int kfence_check_canary_callback(struct notifier_block *nb,
> +                                       unsigned long reason, void *arg)
> +{
> +       kfence_check_all_canary();
> +       return NOTIFY_OK;
> +}
> +
> +static struct notifier_block kfence_check_canary_notifier = {
> +       .notifier_call = kfence_check_canary_callback,
> +};
> +
>  /* === Allocation Gate Timer ================================================ */
>
>  static struct delayed_work kfence_timer;
> @@ -804,6 +834,9 @@ static void kfence_init_enable(void)
>         else
>                 INIT_DELAYED_WORK(&kfence_timer, toggle_allocation_gate);
>
> +       if (kfence_check_on_panic)
> +               atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
> +
>         WRITE_ONCE(kfence_enabled, true);
>         queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
>
> --
> 2.12.3
>

Re: [PATCH v2] kfence: enable check kfence canary in panic via boot param

[Shaobo Huang]

On Sun, 24 Apr 2022 15:31:42 +0200, Marco Elver <elver@google.com> wrote:
> On Sun, 24 Apr 2022 at 13:00, Shaobo Huang <huangshaobo6@huawei.com> wrote:
> >
> > From: huangshaobo <huangshaobo6@huawei.com>
> >
> > when writing out of bounds to the red zone, it can only be
> > detected at kfree. However, the system may have been reset
> > before freeing the memory, which would result in undetected
> > oob. Therefore, it is necessary to detect oob behavior in
> > panic. Since only the allocated mem call stack is available,
> > it may be difficult to find the oob maker. Therefore, this
> > feature is disabled by default and can only be enabled via
> > boot parameter.
> 
> This description is still not telling the full story or usecase. The
> story goes something like:
> """
> Out-of-bounds accesses that aren't caught by a guard page will result
> in corruption of canary memory. In pathological cases, where an object
> has certain alignment requirements, an out-of-bounds access might
> never be caught by the guard page. Such corruptions, however, are only
> detected on kfree() normally. If the bug causes the kernel to panic
> before kfree(), KFENCE has no opportunity to report the issue. Such
> corruptions may also indicate failing memory or other faults.
> 
> To provide some more information in such cases, add the option to
> check canary bytes on panic. This might help narrow the search for the
> panic cause; but, due to only having the allocation stack trace, such
> reports are difficult to use to diagnose an issue alone. In most
> cases, such reports are inactionable, and is therefore an opt-in
> feature (disabled by default).
> """
> 
> Please feel free to copy or take pieces above to complete the commit message.
>
> [...]
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/string.h>
> > +#include <linux/notifier.h>
> > +#include <linux/panic_notifier.h>
> 
> Please keep these includes sorted alphabetically.
> 
> [...]
> > +/* If true, check kfence canary in panic. */
> 
> It should be "on panic". E.g. "If true, check all canary bytes on panic."
> 
> [...]
> > +/* === Panic Notifier ====================================================== */
> 
> Blank line between /* === ... */ and function.

thank you so much for your suggestion!

thanks,
ShaoBo Huang

[PATCH v3] kfence: enable check kfence canary on panic via boot param

[Shaobo Huang]

From: huangshaobo <huangshaobo6@huawei.com>

Out-of-bounds accesses that aren't caught by a guard page will result
in corruption of canary memory. In pathological cases, where an object
has certain alignment requirements, an out-of-bounds access might
never be caught by the guard page. Such corruptions, however, are only
detected on kfree() normally. If the bug causes the kernel to panic
before kfree(), KFENCE has no opportunity to report the issue. Such
corruptions may also indicate failing memory or other faults.

To provide some more information in such cases, add the option to
check canary bytes on panic. This might help narrow the search for the
panic cause; but, due to only having the allocation stack trace, such
reports are difficult to use to diagnose an issue alone. In most
cases, such reports are inactionable, and is therefore an opt-in
feature (disabled by default).

Suggested-by: chenzefeng <chenzefeng2@huawei.com>
Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
---
v3:
- use Marco's description replace the commit message
- keep these includes sorted alphabetically
- "in panic" replaced with "on panic" in title and comments
- Blank line between /* === ... */ and function.
v2:
- it is only detected in panic.
- it is disabled by default.
- can only be enabled via boot parameter.
- the code is moved to the specified partition.
  https://lore.kernel.org/all/20220424105949.50016-1-huangshaobo6@huawei.com/
v1:
  https://lore.kernel.org/all/20220420104927.59056-1-huangshaobo6@huawei.com/
Thanks again Marco for the suggestion.
---
 mm/kfence/core.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 9b2b5f56f4ae..06232d51e021 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -21,6 +21,8 @@
 #include <linux/log2.h>
 #include <linux/memblock.h>
 #include <linux/moduleparam.h>
+#include <linux/notifier.h>
+#include <linux/panic_notifier.h>
 #include <linux/random.h>
 #include <linux/rcupdate.h>
 #include <linux/sched/clock.h>
@@ -99,6 +101,10 @@ module_param_named(skip_covered_thresh, kfence_skip_covered_thresh, ulong, 0644)
 static bool kfence_deferrable __read_mostly = IS_ENABLED(CONFIG_KFENCE_DEFERRABLE);
 module_param_named(deferrable, kfence_deferrable, bool, 0444);
 
+/* If true, check all canary bytes on panic. */
+static bool kfence_check_on_panic;
+module_param_named(check_on_panic, kfence_check_on_panic, bool, 0444);
+
 /* The pool of pages used for guard pages and objects. */
 char *__kfence_pool __read_mostly;
 EXPORT_SYMBOL(__kfence_pool); /* Export for test modules. */
@@ -727,6 +733,31 @@ static int __init kfence_debugfs_init(void)
 
 late_initcall(kfence_debugfs_init);
 
+/* === Panic Notifier ====================================================== */
+
+static void kfence_check_all_canary(void)
+{
+	int i;
+
+	for (i = 0; i < CONFIG_KFENCE_NUM_OBJECTS; i++) {
+		struct kfence_metadata *meta = &kfence_metadata[i];
+
+		if (meta->state == KFENCE_OBJECT_ALLOCATED)
+			for_each_canary(meta, check_canary_byte);
+	}
+}
+
+static int kfence_check_canary_callback(struct notifier_block *nb,
+					unsigned long reason, void *arg)
+{
+	kfence_check_all_canary();
+	return NOTIFY_OK;
+}
+
+static struct notifier_block kfence_check_canary_notifier = {
+	.notifier_call = kfence_check_canary_callback,
+};
+
 /* === Allocation Gate Timer ================================================ */
 
 static struct delayed_work kfence_timer;
@@ -804,6 +835,9 @@ static void kfence_init_enable(void)
 	else
 		INIT_DELAYED_WORK(&kfence_timer, toggle_allocation_gate);
 
+	if (kfence_check_on_panic)
+		atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
+
 	WRITE_ONCE(kfence_enabled, true);
 	queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
 
-- 
2.12.3

Re: [PATCH v3] kfence: enable check kfence canary on panic via boot param

[Marco Elver]

On Mon, 25 Apr 2022 at 04:25, 'Shaobo Huang' via kasan-dev
<kasan-dev@googlegroups.com> wrote:
>
> From: huangshaobo <huangshaobo6@huawei.com>
>
> Out-of-bounds accesses that aren't caught by a guard page will result
> in corruption of canary memory. In pathological cases, where an object
> has certain alignment requirements, an out-of-bounds access might
> never be caught by the guard page. Such corruptions, however, are only
> detected on kfree() normally. If the bug causes the kernel to panic
> before kfree(), KFENCE has no opportunity to report the issue. Such
> corruptions may also indicate failing memory or other faults.
>
> To provide some more information in such cases, add the option to
> check canary bytes on panic. This might help narrow the search for the
> panic cause; but, due to only having the allocation stack trace, such
> reports are difficult to use to diagnose an issue alone. In most
> cases, such reports are inactionable, and is therefore an opt-in
> feature (disabled by default).
>
> Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> Signed-off-by: huangshaobo <huangshaobo6@huawei.com>

I missed one minor issue below (__read_mostly for param), but with
that in place:

Reviewed-by: Marco Elver <elver@google.com>

> ---
> v3:
> - use Marco's description replace the commit message
> - keep these includes sorted alphabetically
> - "in panic" replaced with "on panic" in title and comments
> - Blank line between /* === ... */ and function.
> v2:
> - it is only detected in panic.
> - it is disabled by default.
> - can only be enabled via boot parameter.
> - the code is moved to the specified partition.
>   https://lore.kernel.org/all/20220424105949.50016-1-huangshaobo6@huawei.com/
> v1:
>   https://lore.kernel.org/all/20220420104927.59056-1-huangshaobo6@huawei.com/
> Thanks again Marco for the suggestion.
> ---
>  mm/kfence/core.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 9b2b5f56f4ae..06232d51e021 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -21,6 +21,8 @@
>  #include <linux/log2.h>
>  #include <linux/memblock.h>
>  #include <linux/moduleparam.h>
> +#include <linux/notifier.h>
> +#include <linux/panic_notifier.h>
>  #include <linux/random.h>
>  #include <linux/rcupdate.h>
>  #include <linux/sched/clock.h>
> @@ -99,6 +101,10 @@ module_param_named(skip_covered_thresh, kfence_skip_covered_thresh, ulong, 0644)
>  static bool kfence_deferrable __read_mostly = IS_ENABLED(CONFIG_KFENCE_DEFERRABLE);
>  module_param_named(deferrable, kfence_deferrable, bool, 0444);
>
> +/* If true, check all canary bytes on panic. */
> +static bool kfence_check_on_panic;

This should be __read_mostly, like the other params.

Sorry for noticing this late.

> +module_param_named(check_on_panic, kfence_check_on_panic, bool, 0444);
> +

Thanks,
-- Marco