On Wed, Apr 20, 2022 at 06:49PM +0800, Shaobo Huang wrote:
> From: huangshaobo <huangshaobo6@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>
> Signed-off-by: huangshaobo <huangshaobo6@xxxxxxxxxx>
> ---
> 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
