| 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
| ||
|
Message-ID: <4f39ad95-a773-acc6-dd9e-cb04f897ca16@suse.cz>
Date: Tue, 9 Feb 2021 19:06:50 +0100
From: Vlastimil Babka <vbabka@...e.cz>
To: Marco Elver <elver@...gle.com>, akpm@...ux-foundation.org
Cc: glider@...gle.com, dvyukov@...gle.com, andreyknvl@...gle.com,
jannh@...gle.com, linux-kernel@...r.kernel.org, linux-mm@...ck.org,
kasan-dev@...glegroups.com, Timur Tabi <timur@...nel.org>,
Petr Mladek <pmladek@...e.cz>,
Kees Cook <keescook@...omium.org>,
Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH mm] kfence: make reporting sensitive information
configurable
On 2/9/21 4:13 PM, Marco Elver wrote:
> We cannot rely on CONFIG_DEBUG_KERNEL to decide if we're running a
> "debug kernel" where we can safely show potentially sensitive
> information in the kernel log.
>
> Therefore, add the option CONFIG_KFENCE_REPORT_SENSITIVE to decide if we
> should add potentially sensitive information to KFENCE reports. The
> default behaviour remains unchanged.
>
> Signed-off-by: Marco Elver <elver@...gle.com>
Hi,
could we drop this kconfig approach in favour of the boot option proposed here?
[1] Just do the prints with %p unconditionally and the boot option takes care of
it? Also Linus mentioned dislike of controlling potential memory leak to be a
config option [2]
Thanks,
Vlastimil
[1] https://lore.kernel.org/linux-mm/20210202213633.755469-1-timur@kernel.org/
[2]
https://lore.kernel.org/linux-mm/CAHk-=wgaK4cz=K-JB4p-KPXBV73m9bja2w1W1Lr3iu8+NEPk7A@mail.gmail.com/
> ---
> Documentation/dev-tools/kfence.rst | 6 +++---
> lib/Kconfig.kfence | 8 ++++++++
> mm/kfence/core.c | 2 +-
> mm/kfence/kfence.h | 3 +--
> mm/kfence/report.c | 6 +++---
> 5 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/dev-tools/kfence.rst b/Documentation/dev-tools/kfence.rst
> index 58a0a5fa1ddc..5280d644f826 100644
> --- a/Documentation/dev-tools/kfence.rst
> +++ b/Documentation/dev-tools/kfence.rst
> @@ -89,7 +89,7 @@ A typical out-of-bounds access looks like this::
> The header of the report provides a short summary of the function involved in
> the access. It is followed by more detailed information about the access and
> its origin. Note that, real kernel addresses are only shown for
> -``CONFIG_DEBUG_KERNEL=y`` builds.
> +``CONFIG_KFENCE_REPORT_SENSITIVE=y`` builds.
>
> Use-after-free accesses are reported as::
>
> @@ -184,8 +184,8 @@ invalidly written bytes (offset from the address) are shown; in this
> representation, '.' denote untouched bytes. In the example above ``0xac`` is
> the value written to the invalid address at offset 0, and the remaining '.'
> denote that no following bytes have been touched. Note that, real values are
> -only shown for ``CONFIG_DEBUG_KERNEL=y`` builds; to avoid information
> -disclosure for non-debug builds, '!' is used instead to denote invalidly
> +only shown for ``CONFIG_KFENCE_REPORT_SENSITIVE=y`` builds; to avoid
> +information disclosure otherwise, '!' is used instead to denote invalidly
> written bytes.
>
> And finally, KFENCE may also report on invalid accesses to any protected page
> diff --git a/lib/Kconfig.kfence b/lib/Kconfig.kfence
> index 78f50ccb3b45..141494a5f530 100644
> --- a/lib/Kconfig.kfence
> +++ b/lib/Kconfig.kfence
> @@ -55,6 +55,14 @@ config KFENCE_NUM_OBJECTS
> pages are required; with one containing the object and two adjacent
> ones used as guard pages.
>
> +config KFENCE_REPORT_SENSITIVE
> + bool "Show potentially sensitive information in reports"
> + default y if DEBUG_KERNEL
> + help
> + Show potentially sensitive information such as unhashed pointers,
> + context bytes on memory corruptions, as well as dump registers in
> + KFENCE reports.
> +
> config KFENCE_STRESS_TEST_FAULTS
> int "Stress testing of fault handling and error reporting" if EXPERT
> default 0
> diff --git a/mm/kfence/core.c b/mm/kfence/core.c
> index cfe3d32ac5b7..5f7e02db5f53 100644
> --- a/mm/kfence/core.c
> +++ b/mm/kfence/core.c
> @@ -648,7 +648,7 @@ void __init kfence_init(void)
> schedule_delayed_work(&kfence_timer, 0);
> pr_info("initialized - using %lu bytes for %d objects", KFENCE_POOL_SIZE,
> CONFIG_KFENCE_NUM_OBJECTS);
> - if (IS_ENABLED(CONFIG_DEBUG_KERNEL))
> + if (IS_ENABLED(CONFIG_KFENCE_REPORT_SENSITIVE))
> pr_cont(" at 0x%px-0x%px\n", (void *)__kfence_pool,
> (void *)(__kfence_pool + KFENCE_POOL_SIZE));
> else
> diff --git a/mm/kfence/kfence.h b/mm/kfence/kfence.h
> index 1accc840dbbe..48a8196b947b 100644
> --- a/mm/kfence/kfence.h
> +++ b/mm/kfence/kfence.h
> @@ -16,8 +16,7 @@
>
> #include "../slab.h" /* for struct kmem_cache */
>
> -/* For non-debug builds, avoid leaking kernel pointers into dmesg. */
> -#ifdef CONFIG_DEBUG_KERNEL
> +#ifdef CONFIG_KFENCE_REPORT_SENSITIVE
> #define PTR_FMT "%px"
> #else
> #define PTR_FMT "%p"
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index 901bd7ee83d8..5e2dbabbab1d 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -148,9 +148,9 @@ static void print_diff_canary(unsigned long address, size_t bytes_to_show,
> for (cur = (const u8 *)address; cur < end; cur++) {
> if (*cur == KFENCE_CANARY_PATTERN(cur))
> pr_cont(" .");
> - else if (IS_ENABLED(CONFIG_DEBUG_KERNEL))
> + else if (IS_ENABLED(CONFIG_KFENCE_REPORT_SENSITIVE))
> pr_cont(" 0x%02x", *cur);
> - else /* Do not leak kernel memory in non-debug builds. */
> + else /* Do not leak kernel memory. */
> pr_cont(" !");
> }
> pr_cont(" ]");
> @@ -242,7 +242,7 @@ void kfence_report_error(unsigned long address, bool is_write, struct pt_regs *r
>
> /* Print report footer. */
> pr_err("\n");
> - if (IS_ENABLED(CONFIG_DEBUG_KERNEL) && regs)
> + if (IS_ENABLED(CONFIG_KFENCE_REPORT_SENSITIVE) && regs)
> show_regs(regs);
> else
> dump_stack_print_info(KERN_ERR);
>
Powered by blists - more mailing lists