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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 25 Apr 2022 10:15:43 +0200
From:   Marco Elver <elver@...gle.com>
To:     Shaobo Huang <huangshaobo6@...wei.com>
Cc:     glider@...gle.com, dvyukov@...gle.com, akpm@...ux-foundation.org,
        kasan-dev@...glegroups.com, linux-mm@...ck.org,
        linux-kernel@...r.kernel.org, young.liuyang@...wei.com,
        zengweilin@...wei.com, chenzefeng2@...wei.com,
        nixiaoming@...wei.com, wangbing6@...wei.com,
        wangfangpeng1@...wei.com, zhongjubin@...wei.com
Subject: Re: [PATCH v3] kfence: enable check kfence canary on panic via boot param

On Mon, 25 Apr 2022 at 04:25, 'Shaobo Huang' via kasan-dev
<kasan-dev@...glegroups.com> wrote:
>
> From: huangshaobo <huangshaobo6@...wei.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@...wei.com>
> Signed-off-by: huangshaobo <huangshaobo6@...wei.com>

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

Reviewed-by: Marco Elver <elver@...gle.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