Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220424105949.50016-1-huangshaobo6@huawei.com>
In-Reply-To: <20220424105949.50016-1-huangshaobo6@huawei.com>
From:   Marco Elver <elver@google.com>
Date:   Sun, 24 Apr 2022 15:31:42 +0200
Message-ID: <CANpmjNPEErc2mZMSB=QyT3wq08Q4yGyTGiU3BrOBGV3R5rNw-w@mail.gmail.com>
Subject: Re: [PATCH v2] kfence: enable check kfence canary in panic via boot param
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
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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
>