| 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
| ||
|
Date: Fri, 5 Jun 2020 12:57:15 +0200
From: Dmitry Vyukov <dvyukov@...gle.com>
To: Marco Elver <elver@...gle.com>, Mark Rutland <mark.rutland@....com>
Cc: Peter Zijlstra <peterz@...radead.org>,
Borislav Petkov <bp@...en8.de>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...nel.org>,
clang-built-linux <clang-built-linux@...glegroups.com>,
"Paul E. McKenney" <paulmck@...nel.org>,
Alexander Potapenko <glider@...gle.com>,
Andrey Konovalov <andreyknvl@...gle.com>,
kasan-dev <kasan-dev@...glegroups.com>,
LKML <linux-kernel@...r.kernel.org>,
"the arch/x86 maintainers" <x86@...nel.org>,
Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH -tip v3 1/2] kcov: Make runtime functions noinstr-compatible
On Fri, Jun 5, 2020 at 10:28 AM Marco Elver <elver@...gle.com> wrote:
>
> While we lack a compiler attribute to add to noinstr that would disable
> KCOV, make the KCOV runtime functions return if the caller is in a
> noinstr section, and mark them noinstr.
>
> Declare write_comp_data() as __always_inline to ensure it is inlined,
> which also reduces stack usage and removes one extra call from the
> fast-path.
>
> In future, our compilers may provide an attribute to implement
> __no_sanitize_coverage, which can then be added to noinstr, and the
> checks added in this patch can be guarded by an #ifdef checking if the
> compiler has such an attribute or not.
Adding noinstr attribute to instrumentation callbacks looks fine to me.
But I don't understand the within_noinstr_section part.
As the cover letter mentions, kcov callbacks don't do much and we
already have it inserted and called. What is the benefit of bailing
out a bit earlier rather than letting it run to completion?
Is the only reason for potential faults on access to the vmalloc-ed
region? If so, I think the right approach is to eliminate the faults
(if it's possible). We don't want faults for other reasons: they
caused recursion on ARM and these callbacks are inserted into lots of
sensitive code, so I am not sure checking only noinstr will resolve
all potential issues. E.g. we may get a deadlock if we fault from a
code that holds some lock, or we still can get that recursion on ARM (
I don't think all of page fault handling code is noinstr).
The fact that we started getting faults again (did we?) looks like a
regression related to remote KCOV.
Andrey, Mark, do you know if it's possible to pre-fault these areas?
The difference is that they run in a context of kernel threads. Maybe
we could do kcov_fault_in_area when we activate and remove KCOV on an
area? This way we get all faults in a very well-defined place (which
is not noinstr and holds known locks).
> Signed-off-by: Marco Elver <elver@...gle.com>
> ---
> Applies to -tip only currently, because of the use of instrumentation.h
> markers.
>
> v3:
> * Remove objtool hack, and instead properly mark __sanitizer_cov
> functions as noinstr.
> * Add comment about .entry.text.
>
> v2: https://lkml.kernel.org/r/20200604145635.21565-1-elver@google.com
> * Rewrite based on Peter's and Andrey's feedback -- v1 worked because we
> got lucky. Let's not rely on luck, as it will be difficult to ensure the
> same conditions remain true in future.
>
> v1: https://lkml.kernel.org/r/20200604095057.259452-1-elver@google.com
>
> Note: There are a set of KCOV patches from Andrey in -next:
> https://lkml.kernel.org/r/cover.1585233617.git.andreyknvl@google.com --
> Git cleanly merges this patch with those patches, and no merge conflict
> is expected.
> ---
> kernel/kcov.c | 59 +++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 45 insertions(+), 14 deletions(-)
>
> diff --git a/kernel/kcov.c b/kernel/kcov.c
> index 8accc9722a81..84cdc30d478e 100644
> --- a/kernel/kcov.c
> +++ b/kernel/kcov.c
> @@ -6,6 +6,7 @@
> #include <linux/compiler.h>
> #include <linux/errno.h>
> #include <linux/export.h>
> +#include <linux/instrumentation.h>
> #include <linux/types.h>
> #include <linux/file.h>
> #include <linux/fs.h>
> @@ -24,6 +25,7 @@
> #include <linux/refcount.h>
> #include <linux/log2.h>
> #include <asm/setup.h>
> +#include <asm/sections.h>
>
> #define kcov_debug(fmt, ...) pr_debug("%s: " fmt, __func__, ##__VA_ARGS__)
>
> @@ -172,20 +174,38 @@ static notrace unsigned long canonicalize_ip(unsigned long ip)
> return ip;
> }
>
> +/* Return true if @ip is within a noinstr section. */
> +static __always_inline bool within_noinstr_section(unsigned long ip)
> +{
> + /*
> + * Note: .entry.text is also considered noinstr, but for now, since all
> + * .entry.text code lives in .S files, these are never instrumented.
> + */
> + return (unsigned long)__noinstr_text_start <= ip &&
> + ip < (unsigned long)__noinstr_text_end;
> +}
> +
> /*
> * Entry point from instrumented code.
> * This is called once per basic-block/edge.
> */
> -void notrace __sanitizer_cov_trace_pc(void)
> +void noinstr __sanitizer_cov_trace_pc(void)
> {
> struct task_struct *t;
> unsigned long *area;
> - unsigned long ip = canonicalize_ip(_RET_IP_);
> + unsigned long ip;
> unsigned long pos;
>
> + if (unlikely(within_noinstr_section(_RET_IP_)))
> + return;
> +
> + instrumentation_begin();
> +
> t = current;
> if (!check_kcov_mode(KCOV_MODE_TRACE_PC, t))
> - return;
> + goto out;
> +
> + ip = canonicalize_ip(_RET_IP_);
>
> area = t->kcov_area;
> /* The first 64-bit word is the number of subsequent PCs. */
> @@ -194,19 +214,27 @@ void notrace __sanitizer_cov_trace_pc(void)
> area[pos] = ip;
> WRITE_ONCE(area[0], pos);
> }
> +
> +out:
> + instrumentation_end();
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_pc);
>
> #ifdef CONFIG_KCOV_ENABLE_COMPARISONS
> -static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> +static __always_inline void write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> {
> struct task_struct *t;
> u64 *area;
> u64 count, start_index, end_pos, max_pos;
>
> + if (unlikely(within_noinstr_section(ip)))
> + return;
> +
> + instrumentation_begin();
> +
> t = current;
> if (!check_kcov_mode(KCOV_MODE_TRACE_CMP, t))
> - return;
> + goto out;
>
> ip = canonicalize_ip(ip);
>
> @@ -229,61 +257,64 @@ static void notrace write_comp_data(u64 type, u64 arg1, u64 arg2, u64 ip)
> area[start_index + 3] = ip;
> WRITE_ONCE(area[0], count + 1);
> }
> +
> +out:
> + instrumentation_end();
> }
>
> -void notrace __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
> +void noinstr __sanitizer_cov_trace_cmp1(u8 arg1, u8 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(0), arg1, arg2, _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_cmp1);
>
> -void notrace __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
> +void noinstr __sanitizer_cov_trace_cmp2(u16 arg1, u16 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(1), arg1, arg2, _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_cmp2);
>
> -void notrace __sanitizer_cov_trace_cmp4(u32 arg1, u32 arg2)
> +void noinstr __sanitizer_cov_trace_cmp4(u32 arg1, u32 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(2), arg1, arg2, _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_cmp4);
>
> -void notrace __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
> +void noinstr __sanitizer_cov_trace_cmp8(u64 arg1, u64 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(3), arg1, arg2, _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_cmp8);
>
> -void notrace __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
> +void noinstr __sanitizer_cov_trace_const_cmp1(u8 arg1, u8 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(0) | KCOV_CMP_CONST, arg1, arg2,
> _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp1);
>
> -void notrace __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
> +void noinstr __sanitizer_cov_trace_const_cmp2(u16 arg1, u16 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(1) | KCOV_CMP_CONST, arg1, arg2,
> _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp2);
>
> -void notrace __sanitizer_cov_trace_const_cmp4(u32 arg1, u32 arg2)
> +void noinstr __sanitizer_cov_trace_const_cmp4(u32 arg1, u32 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(2) | KCOV_CMP_CONST, arg1, arg2,
> _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp4);
>
> -void notrace __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
> +void noinstr __sanitizer_cov_trace_const_cmp8(u64 arg1, u64 arg2)
> {
> write_comp_data(KCOV_CMP_SIZE(3) | KCOV_CMP_CONST, arg1, arg2,
> _RET_IP_);
> }
> EXPORT_SYMBOL(__sanitizer_cov_trace_const_cmp8);
>
> -void notrace __sanitizer_cov_trace_switch(u64 val, u64 *cases)
> +void noinstr __sanitizer_cov_trace_switch(u64 val, u64 *cases)
> {
> u64 i;
> u64 count = cases[0];
> --
> 2.27.0.278.ge193c7cf3a9-goog
>
Powered by blists - more mailing lists