[PATCH] kfence: check kfence canary in panic and reboot

[PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

From: huangshaobo <huangshaobo6@huawei.com>

when writing out of bounds to the red zone, it can only be detected at
kfree. However, there were many scenarios before kfree that caused this
out-of-bounds write to not be detected. Therefore, it is necessary to
provide a method for actively detecting out-of-bounds writing to the red
zone, so that users can actively detect, and can be detected in the
system reboot or panic.

for example, if the application memory is out of bounds and written to
the red zone in the kfence object, the system suddenly panics, and the
following log can be seen during system reset:
BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70

Corrupted memory at 0x(____ptrval____) [ ! ] (in kfence-#59):
 atomic_notifier_call_chain+0x49/0x70
 panic+0x134/0x278
 sysrq_handle_crash+0x11/0x20
 __handle_sysrq+0x99/0x160
 write_sysrq_trigger+0x26/0x30
 proc_reg_write+0x51/0x70
 vfs_write+0xb6/0x290
 ksys_write+0x9c/0xd0
 __do_fast_syscall_32+0x67/0xe0
 do_fast_syscall_32+0x2f/0x70
 entry_SYSCALL_compat_after_hwframe+0x45/0x4d

kfence-#59: 0x(____ptrval____)-0x(____ptrval____),size=100,cache=kmalloc-128
 allocated by task 77 on cpu 0 at 28.018073s:
 0xffffffffc007703d
 do_one_initcall+0x3c/0x1e0
 do_init_module+0x46/0x1d8
 load_module+0x2397/0x2860
 __do_sys_init_module+0x160/0x190
 __do_fast_syscall_32+0x67/0xe0
 do_fast_syscall_32+0x2f/0x70
 entry_SYSCALL_compat_after_hwframe+0x45/0x4d

Suggested-by: chenzefeng <chenzefeng2@huawei.com>
Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
---
 mm/kfence/core.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/mm/kfence/core.c b/mm/kfence/core.c
index 9b2b5f56f4ae..85cc3ca4b71c 100644
--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -29,6 +29,9 @@
 #include <linux/slab.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
+#include <linux/notifier.h>
+#include <linux/reboot.h>
+#include <linux/panic_notifier.h>
 
 #include <asm/kfence.h>
 
@@ -716,6 +719,29 @@ static const struct file_operations objects_fops = {
 	.release = seq_release,
 };
 
+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,
+};
+
 static int __init kfence_debugfs_init(void)
 {
 	struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
@@ -806,6 +832,8 @@ static void kfence_init_enable(void)
 
 	WRITE_ONCE(kfence_enabled, true);
 	queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
+	register_reboot_notifier(&kfence_check_canary_notifier);
+	atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
 
 	pr_info("initialized - using %lu bytes for %d objects at 0x%p-0x%p\n", KFENCE_POOL_SIZE,
 		CONFIG_KFENCE_NUM_OBJECTS, (void *)__kfence_pool,
-- 
2.12.3

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Marco Elver]

On Wed, Apr 20, 2022 at 06:49PM +0800, Shaobo Huang wrote:
> From: huangshaobo <huangshaobo6@huawei.com>
> 
> when writing out of bounds to the red zone, it can only be detected at
> kfree. However, there were many scenarios before kfree that caused this
> out-of-bounds write to not be detected. Therefore, it is necessary to
> provide a method for actively detecting out-of-bounds writing to the red
> zone, so that users can actively detect, and can be detected in the
> system reboot or panic.
> 
> for example, if the application memory is out of bounds and written to
> the red zone in the kfence object, the system suddenly panics, and the
> following log can be seen during system reset:

Interesting idea - however, when KFENCE is deployed to a fleet, the same
bug will eventually manifest as an OOB that hits a guard page (because
random placement), and produce the normal out-of-bounds message.

Have you found new bugs this way?

But doing this check on panic doesn't seem to hurt. But please see
comments below.

> BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70
> 
> Corrupted memory at 0x(____ptrval____) [ ! ] (in kfence-#59):
>  atomic_notifier_call_chain+0x49/0x70
>  panic+0x134/0x278
>  sysrq_handle_crash+0x11/0x20
>  __handle_sysrq+0x99/0x160
>  write_sysrq_trigger+0x26/0x30
>  proc_reg_write+0x51/0x70
>  vfs_write+0xb6/0x290
>  ksys_write+0x9c/0xd0
>  __do_fast_syscall_32+0x67/0xe0
>  do_fast_syscall_32+0x2f/0x70
>  entry_SYSCALL_compat_after_hwframe+0x45/0x4d
> 
> kfence-#59: 0x(____ptrval____)-0x(____ptrval____),size=100,cache=kmalloc-128
>  allocated by task 77 on cpu 0 at 28.018073s:
>  0xffffffffc007703d
>  do_one_initcall+0x3c/0x1e0
>  do_init_module+0x46/0x1d8
>  load_module+0x2397/0x2860
>  __do_sys_init_module+0x160/0x190
>  __do_fast_syscall_32+0x67/0xe0
>  do_fast_syscall_32+0x2f/0x70
>  entry_SYSCALL_compat_after_hwframe+0x45/0x4d

Is this a real bug? Or one you injected?

> Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
> ---
>  mm/kfence/core.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 9b2b5f56f4ae..85cc3ca4b71c 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -29,6 +29,9 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
> +#include <linux/panic_notifier.h>
>  
>  #include <asm/kfence.h>
>  
> @@ -716,6 +719,29 @@ static const struct file_operations objects_fops = {
>  	.release = seq_release,
>  };
>  
> +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,
> +};

Sorry to be pedantic, but this is a pretty random place to put this
code. Can you put it after the debugfs section, perhaps with:

--- a/mm/kfence/core.c
+++ b/mm/kfence/core.c
@@ -748,6 +748,10 @@ static int __init kfence_debugfs_init(void)
 
 late_initcall(kfence_debugfs_init);
 
+/* === Reboot Notifier ====================================================== */
+
+< your code here >
+
 /* === Allocation Gate Timer ================================================ */
 
 static struct delayed_work kfence_timer;

>  static int __init kfence_debugfs_init(void)
>  {
>  	struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
> @@ -806,6 +832,8 @@ static void kfence_init_enable(void)
>  
>  	WRITE_ONCE(kfence_enabled, true);
>  	queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
> +	register_reboot_notifier(&kfence_check_canary_notifier);
> +	atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);

Executing this on panic is reasonable. However,
register_reboot_notifier() tells me this is being executed on *every*
reboot (not just panic). I think that's not what we want, because that
may increase reboot latency depending on how many KFENCE objects we
have. Is it possible to *only* do the check on panic?

Thanks,
-- Marco

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

On Wed, 20 Apr 2022 13:11:39 +0200, Marco Elver wrote:
> On Wed, Apr 20, 2022 at 06:49PM +0800, Shaobo Huang wrote:
> > From: huangshaobo <huangshaobo6@huawei.com>
> > 
> > when writing out of bounds to the red zone, it can only be detected at
> > kfree. However, there were many scenarios before kfree that caused this
> > out-of-bounds write to not be detected. Therefore, it is necessary to
> > provide a method for actively detecting out-of-bounds writing to the red
> > zone, so that users can actively detect, and can be detected in the
> > system reboot or panic.
> > 
> > for example, if the application memory is out of bounds and written to
> > the red zone in the kfence object, the system suddenly panics, and the
> following log can be seen during system reset:
> 
> Interesting idea - however, when KFENCE is deployed to a fleet, the same
> bug will eventually manifest as an OOB that hits a guard page (because
> random placement), and produce the normal out-of-bounds message.
> 
> Have you found new bugs this way?

We haven't found bugs in this way yet, but we have proved that this way works through injection tests.

> But doing this check on panic doesn't seem to hurt. But please see
> comments below.
> 
> > BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70
> > 
> > Corrupted memory at 0x(____ptrval____) [ ! ] (in kfence-#59):
> >  atomic_notifier_call_chain+0x49/0x70
> >  panic+0x134/0x278
> >  sysrq_handle_crash+0x11/0x20
> >  __handle_sysrq+0x99/0x160
> >  write_sysrq_trigger+0x26/0x30
> >  proc_reg_write+0x51/0x70
> >  vfs_write+0xb6/0x290
> >  ksys_write+0x9c/0xd0
> >  __do_fast_syscall_32+0x67/0xe0
> >  do_fast_syscall_32+0x2f/0x70
> >  entry_SYSCALL_compat_after_hwframe+0x45/0x4d
> > 
> > kfence-#59: 0x(____ptrval____)-0x(____ptrval____),size=100,cache=kmalloc-128
> >  allocated by task 77 on cpu 0 at 28.018073s:
> >  0xffffffffc007703d
> >  do_one_initcall+0x3c/0x1e0
> >  do_init_module+0x46/0x1d8
> >  load_module+0x2397/0x2860
> >  __do_sys_init_module+0x160/0x190
> >  __do_fast_syscall_32+0x67/0xe0
> >  do_fast_syscall_32+0x2f/0x70
> >  entry_SYSCALL_compat_after_hwframe+0x45/0x4d
> 
> Is this a real bug? Or one you injected?

one injected, construct red zone oob, echo c > /proc/sysrq-trigger to trigger panic.
The call stack example here will be deleted later.

> > Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> > Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
> > ---
> >  mm/kfence/core.c | 28 ++++++++++++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> > 
> > diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> > index 9b2b5f56f4ae..85cc3ca4b71c 100644
> > --- a/mm/kfence/core.c
> > +++ b/mm/kfence/core.c
> > @@ -29,6 +29,9 @@
> >  #include <linux/slab.h>
> >  #include <linux/spinlock.h>
> >  #include <linux/string.h>
> > +#include <linux/notifier.h>
> > +#include <linux/reboot.h>
> +#include <linux/panic_notifier.h>
> >  
> >  #include <asm/kfence.h>
> >  
> > @@ -716,6 +719,29 @@ static const struct file_operations objects_fops = {
> >  	.release = seq_release,
> >  };
> >  
> > +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,
> > +};
> 
> Sorry to be pedantic, but this is a pretty random place to put this
> code. Can you put it after the debugfs section, perhaps with:
> 
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -748,6 +748,10 @@ static int __init kfence_debugfs_init(void)
>  
>  late_initcall(kfence_debugfs_init);
>  
> +/* === Reboot Notifier ====================================================== */
> +
> +< your code here >
> +
>  /* === Allocation Gate Timer ================================================ */
>  
>  static struct delayed_work kfence_timer;

thanks for your suggestion, I will modify it according to your suggestions later.

> >  static int __init kfence_debugfs_init(void)
> >  {
> >  	struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
> > @@ -806,6 +832,8 @@ static void kfence_init_enable(void)
> >  
> >  	WRITE_ONCE(kfence_enabled, true);
> >  	queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
> > +	register_reboot_notifier(&kfence_check_canary_notifier);
> > +	atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
> 
> Executing this on panic is reasonable. However,
> register_reboot_notifier() tells me this is being executed on *every*
> reboot (not just panic). I think that's not what we want, because that
> may increase reboot latency depending on how many KFENCE objects we
> have. Is it possible to *only* do the check on panic?

if oob occurs before reboot, reboot can also detect it, if not, the detection will be missing in this scenario.
reboot and panic are two scenarios of system reset, so I think both scenarios need to be added.
 
> Thanks,
> -- Marco

thanks,
ShaoBo Huang

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Marco Elver]

On Thu, 21 Apr 2022 at 10:37, Shaobo Huang <huangshaobo6@huawei.com> wrote:
[...]
> > >  static int __init kfence_debugfs_init(void)
> > >  {
> > >     struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
> > > @@ -806,6 +832,8 @@ static void kfence_init_enable(void)
> > >
> > >     WRITE_ONCE(kfence_enabled, true);
> > >     queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
> > > +   register_reboot_notifier(&kfence_check_canary_notifier);
> > > +   atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
> >
> > Executing this on panic is reasonable. However,
> > register_reboot_notifier() tells me this is being executed on *every*
> > reboot (not just panic). I think that's not what we want, because that
> > may increase reboot latency depending on how many KFENCE objects we
> > have. Is it possible to *only* do the check on panic?
>
> if oob occurs before reboot, reboot can also detect it, if not, the detection will be missing in this scenario.
> reboot and panic are two scenarios of system reset, so I think both scenarios need to be added.

That doesn't quite answer my question, why do you want to run the
check during normal reboot? As I understand it right now it will run
on any normal reboot, and also on panics. I have concerns adding these
checks to normal reboots because it may increase normal reboot
latency, which we do not want.

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

On Thu, 21 Apr 2022 10:50:10 +0200, Marco Elver wrote:
> On Thu, 21 Apr 2022 at 10:37, Shaobo Huang <huangshaobo6@huawei.com> wrote:
> [...]
> > > >  static int __init kfence_debugfs_init(void)
> > > >  {
> > > >     struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
> > > > @@ -806,6 +832,8 @@ static void kfence_init_enable(void)
> > > >
> > > >     WRITE_ONCE(kfence_enabled, true);
> > > >     queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
> > > > +   register_reboot_notifier(&kfence_check_canary_notifier);
> > > > +   atomic_notifier_chain_register(&panic_notifier_list, &kfence_check_canary_notifier);
> > >
> > > Executing this on panic is reasonable. However,
> > > register_reboot_notifier() tells me this is being executed on *every*
> > > reboot (not just panic). I think that's not what we want, because that
>> > may increase reboot latency depending on how many KFENCE objects we
> > > have. Is it possible to *only* do the check on panic?
> >
> > if oob occurs before reboot, reboot can also detect it, if not, the detection will be missing in this scenario.
> > reboot and panic are two scenarios of system reset, so I think both scenarios need to be added.
> 
> That doesn't quite answer my question, why do you want to run the
> check during normal reboot? As I understand it right now it will run
> on any normal reboot, and also on panics. I have concerns adding these
> checks to normal reboots because it may increase normal reboot
> latency, which we do not want.

as you said, the detection will indeed increase normal reboot latency, and the
detection of normal reboot is not very meaningful. considering the cost and benefit,
I agree with your suggestion to only detect in panic.

thanks,
ShaoBo Huang

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Alexander Potapenko]

[-- Attachment #1: Type: text/plain, Size: 5271 bytes --]

On Wed, Apr 20, 2022 at 12:50 PM 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, there were many scenarios before kfree that caused this
> out-of-bounds write to not be detected. Therefore, it is necessary to
> provide a method for actively detecting out-of-bounds writing to the red
> zone, so that users can actively detect, and can be detected in the
> system reboot or panic.
>
>
After having analyzed a couple of KFENCE memory corruption reports in the
wild, I have doubts that this approach will be helpful.

Note that KFENCE knows nothing about the memory access that performs the
actual corruption.

It's rather easy to investigate corruptions of short-living objects, e.g.
those that are allocated and freed within the same function. In that case,
one can examine the region of the code between these two events and try to
understand what exactly caused the corruption.

But for long-living objects checked at panic/reboot we'll effectively have
only the allocation stack and will have to check all the places where the
corrupted object was potentially used.
Most of the time, such reports won't be actionable.


> for example, if the application memory is out of bounds and written to
> the red zone in the kfence object, the system suddenly panics, and the
> following log can be seen during system reset:
> BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70
>
> Corrupted memory at 0x(____ptrval____) [ ! ] (in kfence-#59):
>  atomic_notifier_call_chain+0x49/0x70
>  panic+0x134/0x278
>  sysrq_handle_crash+0x11/0x20
>  __handle_sysrq+0x99/0x160
>  write_sysrq_trigger+0x26/0x30
>  proc_reg_write+0x51/0x70
>  vfs_write+0xb6/0x290
>  ksys_write+0x9c/0xd0
>  __do_fast_syscall_32+0x67/0xe0
>  do_fast_syscall_32+0x2f/0x70
>  entry_SYSCALL_compat_after_hwframe+0x45/0x4d
>
> kfence-#59:
> 0x(____ptrval____)-0x(____ptrval____),size=100,cache=kmalloc-128
>  allocated by task 77 on cpu 0 at 28.018073s:
>  0xffffffffc007703d
>  do_one_initcall+0x3c/0x1e0
>  do_init_module+0x46/0x1d8
>  load_module+0x2397/0x2860
>  __do_sys_init_module+0x160/0x190
>  __do_fast_syscall_32+0x67/0xe0
>  do_fast_syscall_32+0x2f/0x70
>  entry_SYSCALL_compat_after_hwframe+0x45/0x4d
>
> Suggested-by: chenzefeng <chenzefeng2@huawei.com>
> Signed-off-by: huangshaobo <huangshaobo6@huawei.com>
> ---
>  mm/kfence/core.c | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
>
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index 9b2b5f56f4ae..85cc3ca4b71c 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -29,6 +29,9 @@
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
>  #include <linux/string.h>
> +#include <linux/notifier.h>
> +#include <linux/reboot.h>
> +#include <linux/panic_notifier.h>
>
>  #include <asm/kfence.h>
>
> @@ -716,6 +719,29 @@ static const struct file_operations objects_fops = {
>         .release = seq_release,
>  };
>
> +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,
> +};
> +
>  static int __init kfence_debugfs_init(void)
>  {
>         struct dentry *kfence_dir = debugfs_create_dir("kfence", NULL);
> @@ -806,6 +832,8 @@ static void kfence_init_enable(void)
>
>         WRITE_ONCE(kfence_enabled, true);
>         queue_delayed_work(system_unbound_wq, &kfence_timer, 0);
> +       register_reboot_notifier(&kfence_check_canary_notifier);
> +       atomic_notifier_chain_register(&panic_notifier_list,
> &kfence_check_canary_notifier);
>
>         pr_info("initialized - using %lu bytes for %d objects at
> 0x%p-0x%p\n", KFENCE_POOL_SIZE,
>                 CONFIG_KFENCE_NUM_OBJECTS, (void *)__kfence_pool,
> --
> 2.12.3
>
>

-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten
haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter,
löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen,
dass die E-Mail an die falsche Person gesendet wurde.


This e-mail is confidential. If you received this communication by mistake,
please don't forward it to anyone else, please erase all copies and
attachments, and please let me know that it has gone to the wrong person.

[-- Attachment #2: Type: text/html, Size: 6570 bytes --]

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

> > From: huangshaobo <huangshaobo6@huawei.com>
> >
> > when writing out of bounds to the red zone, it can only be detected at
> > kfree. However, there were many scenarios before kfree that caused this
> > out-of-bounds write to not be detected. Therefore, it is necessary to
> > provide a method for actively detecting out-of-bounds writing to the red
> > zone, so that users can actively detect, and can be detected in the
> > system reboot or panic.
> >
> >
> After having analyzed a couple of KFENCE memory corruption reports in the
> wild, I have doubts that this approach will be helpful.
> 
> Note that KFENCE knows nothing about the memory access that performs the
> actual corruption.
> 
> It's rather easy to investigate corruptions of short-living objects, e.g.
> those that are allocated and freed within the same function. In that case,
> one can examine the region of the code between these two events and try to
> understand what exactly caused the corruption.
> 
> But for long-living objects checked at panic/reboot we'll effectively have
> only the allocation stack and will have to check all the places where the
> corrupted object was potentially used.
> Most of the time, such reports won't be actionable.
 
The detection mechanism of kfence is probabilistic. It is not easy to find a bug.
It is a pity to catch a bug without reporting it. and the cost of panic detection
is not large, so panic detection is still valuable.
 
> > for example, if the application memory is out of bounds and written to
> > the red zone in the kfence object, the system suddenly panics, and the
> > following log can be seen during system reset:
> > BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70
[...]

thanks,
ShaoBo Huang

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Alexander Potapenko]

[-- Attachment #1: Type: text/plain, Size: 4128 bytes --]

On Thu, Apr 21, 2022 at 2:10 PM 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, there were many scenarios before kfree that caused this
> > > out-of-bounds write to not be detected. Therefore, it is necessary to
> > > provide a method for actively detecting out-of-bounds writing to the
> red
> > > zone, so that users can actively detect, and can be detected in the
> > > system reboot or panic.
> > >
> > >
> > After having analyzed a couple of KFENCE memory corruption reports in the
> > wild, I have doubts that this approach will be helpful.
> >
> > Note that KFENCE knows nothing about the memory access that performs the
> > actual corruption.
> >
> > It's rather easy to investigate corruptions of short-living objects, e.g.
> > those that are allocated and freed within the same function. In that
> case,
> > one can examine the region of the code between these two events and try
> to
> > understand what exactly caused the corruption.
> >
> > But for long-living objects checked at panic/reboot we'll effectively
> have
> > only the allocation stack and will have to check all the places where the
> > corrupted object was potentially used.
> > Most of the time, such reports won't be actionable.
>
> The detection mechanism of kfence is probabilistic. It is not easy to find
> a bug.
> It is a pity to catch a bug without reporting it. and the cost of panic
> detection
> is not large, so panic detection is still valuable.
>
>
I am also a big fan of showing as much information as possible to help the
developers debug a memory corruption.
But I am still struggling to understand how the proposed patch helps.
Assume we have some generic allocation of an skbuff, so the reports looks
like this:

=============================================
BUG: KFENCE: memory corruption in <frame that triggered reboot>
Corrupted memory at <end+1>
<stack trace of reboot event>

kfence-#59: <start>-<end>,size=100,cache=kmalloc-128  allocated by task 77
on cpu 0 at 28.018073s:
kmem_cache_alloc
__alloc_skb
alloc_skb_with_frags
sock_alloc_send_pskb
unix_stream_sendmsg
sock_sendmsg
__sys_sendto
__x64_sys_sendto
=============================================

This report will denote that in a system that could have been running for
days a particular skbuff was corrupted by some unknown task at some unknown
point in time.
How do we figure out what exactly caused this corruption?

When we deploy KFENCE at scale, it is rarely possible for the kernel
developer to get access to the host that reported the bug and try to
reproduce it.
With that in mind, the report (plus the kernel source) must contain all the
necessary information to address the bug, otherwise reporting it will
result in wasting the developer's time.
Moreover, if we report such bugs too often, our tool loses the credit,
which is hard to regain.

> > for example, if the application memory is out of bounds and written to
> > > the red zone in the kfence object, the system suddenly panics, and the
> > > following log can be seen during system reset:
> > > BUG: KFENCE: memory corruption in atomic_notifier_call_chain+0x49/0x70
> [...]
>
> thanks,
> ShaoBo Huang
>


-- 
Alexander Potapenko
Software Engineer

Google Germany GmbH
Erika-Mann-Straße, 33
80636 München

Geschäftsführer: Paul Manicle, Liana Sebastian
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg

Diese E-Mail ist vertraulich. Falls Sie diese fälschlicherweise erhalten
haben sollten, leiten Sie diese bitte nicht an jemand anderes weiter,
löschen Sie alle Kopien und Anhänge davon und lassen Sie mich bitte wissen,
dass die E-Mail an die falsche Person gesendet wurde.


This e-mail is confidential. If you received this communication by mistake,
please don't forward it to anyone else, please erase all copies and
attachments, and please let me know that it has gone to the wrong person.

[-- Attachment #2: Type: text/html, Size: 5194 bytes --]

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Marco Elver]

On Thu, 21 Apr 2022 at 15:06, Alexander Potapenko <glider@google.com> wrote:
[...]
> This report will denote that in a system that could have been running for days a particular skbuff was corrupted by some unknown task at some unknown point in time.
> How do we figure out what exactly caused this corruption?
>
> When we deploy KFENCE at scale, it is rarely possible for the kernel developer to get access to the host that reported the bug and try to reproduce it.
> With that in mind, the report (plus the kernel source) must contain all the necessary information to address the bug, otherwise reporting it will result in wasting the developer's time.
> Moreover, if we report such bugs too often, our tool loses the credit, which is hard to regain.

I second this - in particular we'll want this off in fuzzers etc.,
because it'll just generate reports that nobody can use to debug an
issue. I do see the value in this in potentially narrowing the cause
of a panic, but that information is likely not enough to fully
diagnose the root cause of the panic - it might however prompt to
re-run with KASAN, or check if memory DIMMs are faulty etc.

We can still have this feature, but I suggest to make it
off-by-default, and only enable via a boot param. I'd call it
'kfence.check_on_panic'. For your setup, you can then use it to enable
where you see fit.

Thanks,
-- Marco

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

On Thu, 21 Apr 2022 15:28:45 +0200, Marco Elver <elver@google.com> wrote:
> On Thu, 21 Apr 2022 at 15:06, Alexander Potapenko <glider@google.com> wrote:
> [...]
> > This report will denote that in a system that could have been running for days a particular skbuff was corrupted by some unknown task at some unknown point in time.
> > How do we figure out what exactly caused this corruption?
> >
> > When we deploy KFENCE at scale, it is rarely possible for the kernel developer to get access to the host that reported the bug and try to reproduce it.
> > With that in mind, the report (plus the kernel source) must contain all the necessary information to address the bug, otherwise reporting it will result in wasting the developer's time.
> > Moreover, if we report such bugs too often, our tool loses the credit, which is hard to regain.
> 
> I second this - in particular we'll want this off in fuzzers etc.,
> because it'll just generate reports that nobody can use to debug an
> issue. I do see the value in this in potentially narrowing the cause
> of a panic, but that information is likely not enough to fully
> diagnose the root cause of the panic - it might however prompt to
> re-run with KASAN, or check if memory DIMMs are faulty etc.
> 
> We can still have this feature, but I suggest to make it
> off-by-default, and only enable via a boot param. I'd call it
> 'kfence.check_on_panic'. For your setup, you can then use it to enable
> where you see fit.

I agree to give users the option to use this feature.

> Thanks,
>-- Marco

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Shaobo Huang]

On Thu, 21 Apr 2022 15:28:45 +0200, Marco Elver <elver@google.com> wrote:
> On Thu, 21 Apr 2022 at 15:06, Alexander Potapenko <glider@google.com> wrote:
> [...]
> > This report will denote that in a system that could have been running for days a particular skbuff was corrupted by some unknown task at some unknown point in time.
> > How do we figure out what exactly caused this corruption?
> >
> > When we deploy KFENCE at scale, it is rarely possible for the kernel developer to get access to the host that reported the bug and try to reproduce it.
> > With that in mind, the report (plus the kernel source) must contain all the necessary information to address the bug, otherwise reporting it will result in wasting the developer's time.
> > Moreover, if we report such bugs too often, our tool loses the credit, which is hard to regain.
> 
> I second this - in particular we'll want this off in fuzzers etc.,
> because it'll just generate reports that nobody can use to debug an
> issue. I do see the value in this in potentially narrowing the cause
> of a panic, but that information is likely not enough to fully
> diagnose the root cause of the panic - it might however prompt to
> re-run with KASAN, or check if memory DIMMs are faulty etc.
> 
> We can still have this feature, but I suggest to make it
> off-by-default, and only enable via a boot param. I'd call it
> 'kfence.check_on_panic'. For your setup, you can then use it to enable
> where you see fit.

Can I implement your suggestion into the second patch and add the "Suggested-by: Marco Elver <elver@google.com>" tag to it?

> Thanks,
>-- Marco

Re: [PATCH] kfence: check kfence canary in panic and reboot

[Marco Elver]

On Sun, 24 Apr 2022 at 10:10, Shaobo Huang <huangshaobo6@huawei.com> wrote:
>
> On Thu, 21 Apr 2022 15:28:45 +0200, Marco Elver <elver@google.com> wrote:
> > On Thu, 21 Apr 2022 at 15:06, Alexander Potapenko <glider@google.com> wrote:
> > [...]
> > > This report will denote that in a system that could have been running for days a particular skbuff was corrupted by some unknown task at some unknown point in time.
> > > How do we figure out what exactly caused this corruption?
> > >
> > > When we deploy KFENCE at scale, it is rarely possible for the kernel developer to get access to the host that reported the bug and try to reproduce it.
> > > With that in mind, the report (plus the kernel source) must contain all the necessary information to address the bug, otherwise reporting it will result in wasting the developer's time.
> > > Moreover, if we report such bugs too often, our tool loses the credit, which is hard to regain.
> >
> > I second this - in particular we'll want this off in fuzzers etc.,
> > because it'll just generate reports that nobody can use to debug an
> > issue. I do see the value in this in potentially narrowing the cause
> > of a panic, but that information is likely not enough to fully
> > diagnose the root cause of the panic - it might however prompt to
> > re-run with KASAN, or check if memory DIMMs are faulty etc.
> >
> > We can still have this feature, but I suggest to make it
> > off-by-default, and only enable via a boot param. I'd call it
> > 'kfence.check_on_panic'. For your setup, you can then use it to enable
> > where you see fit.
>
> Can I implement your suggestion into the second patch and add the "Suggested-by: Marco Elver <elver@google.com>" tag to it?

I don't think it's necessary, after all the overall patch is still
your idea - you're just using our review feedback to improve it. In
the change-log (after ---) you can of course mention that, but it'll
be stripped upon applying.

Thanks,
-- Marco