Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Date:   Wed, 20 Apr 2022 13:11:39 +0200
From:   Marco Elver <elver@google.com>
To:     Shaobo Huang <huangshaobo6@huawei.com>
Cc:     glider@google.com, dvyukov@google.com, akpm@linux-foundation.org,
        kasan-dev@googlegroups.com, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, young.liuyang@huawei.com,
        zengweilin@huawei.com, chenzefeng2@huawei.com,
        nixiaoming@huawei.com, wangbing6@huawei.com,
        wangfangpeng1@huawei.com, zhongjubin@huawei.com
Subject: Re: [PATCH] kfence: check kfence canary in panic and reboot
Message-ID: <Yl/qa2w3q9kyXcQl@elver.google.com>
References: <20220420104927.59056-1-huangshaobo6@huawei.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220420104927.59056-1-huangshaobo6@huawei.com>
User-Agent: Mutt/2.1.4 (2021-12-11)
Precedence: bulk

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