| 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 Mar 2021 08:50:03 +0100
From: Marco Elver <elver@...gle.com>
To: Michael Ellerman <mpe@...erman.id.au>
Cc: Christophe Leroy <christophe.leroy@...roup.eu>,
Alexander Potapenko <glider@...gle.com>,
Benjamin Herrenschmidt <benh@...nel.crashing.org>,
Paul Mackerras <paulus@...ba.org>,
Dmitry Vyukov <dvyukov@...gle.com>,
LKML <linux-kernel@...r.kernel.org>,
linuxppc-dev@...ts.ozlabs.org,
kasan-dev <kasan-dev@...glegroups.com>
Subject: Re: [RFC PATCH v1] powerpc: Enable KFENCE for PPC32
On Fri, Mar 05, 2021 at 04:01PM +1100, Michael Ellerman wrote:
> Marco Elver <elver@...gle.com> writes:
> > On Thu, Mar 04, 2021 at 12:48PM +0100, Christophe Leroy wrote:
> >> Le 04/03/2021 à 12:31, Marco Elver a écrit :
> >> > On Thu, 4 Mar 2021 at 12:23, Christophe Leroy
> >> > <christophe.leroy@...roup.eu> wrote:
> >> > > Le 03/03/2021 à 11:56, Marco Elver a écrit :
> >> > > >
> >> > > > Somewhat tangentially, I also note that e.g. show_regs(regs) (which
> >> > > > was printed along the KFENCE report above) didn't include the top
> >> > > > frame in the "Call Trace", so this assumption is definitely not
> >> > > > isolated to KFENCE.
> >> > > >
> >> > >
> >> > > Now, I have tested PPC64 (with the patch I sent yesterday to modify save_stack_trace_regs()
> >> > > applied), and I get many failures. Any idea ?
> >> > >
> >> > > [ 17.653751][ T58] ==================================================================
> >> > > [ 17.654379][ T58] BUG: KFENCE: invalid free in .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.654379][ T58]
> >> > > [ 17.654831][ T58] Invalid free of 0xc00000003c9c0000 (in kfence-#77):
> >> > > [ 17.655358][ T58] .kfence_guarded_free+0x2e4/0x530
> >> > > [ 17.655775][ T58] .__slab_free+0x320/0x5a0
> >> > > [ 17.656039][ T58] .test_double_free+0xe0/0x198
> >> > > [ 17.656308][ T58] .kunit_try_run_case+0x80/0x110
> >> > > [ 17.656523][ T58] .kunit_generic_run_threadfn_adapter+0x38/0x50
> >> > > [ 17.657161][ T58] .kthread+0x18c/0x1a0
> >> > > [ 17.659148][ T58] .ret_from_kernel_thread+0x58/0x70
> >> > > [ 17.659869][ T58]
> > [...]
> >> >
> >> > Looks like something is prepending '.' to function names. We expect
> >> > the function name to appear as-is, e.g. "kfence_guarded_free",
> >> > "test_double_free", etc.
> >> >
> >> > Is there something special on ppc64, where the '.' is some convention?
> >> >
> >>
> >> I think so, see https://refspecs.linuxfoundation.org/ELF/ppc64/PPC-elf64abi.html#FUNC-DES
> >>
> >> Also see commit https://github.com/linuxppc/linux/commit/02424d896
> >
> > Thanks -- could you try the below patch? You'll need to define
> > ARCH_FUNC_PREFIX accordingly.
> >
> > We think, since there are only very few architectures that add a prefix,
> > requiring <asm/kfence.h> to define something like ARCH_FUNC_PREFIX is
> > the simplest option. Let me know if this works for you.
> >
> > There an alternative option, which is to dynamically figure out the
> > prefix, but if this simpler option is fine with you, we'd prefer it.
>
> We have rediscovered this problem in basically every tracing / debugging
> feature added in the last 20 years :)
>
> I think the simplest solution is the one tools/perf/util/symbol.c uses,
> which is to just skip a leading '.'.
>
> Does that work?
>
> diff --git a/mm/kfence/report.c b/mm/kfence/report.c
> index ab83d5a59bb1..67b49dc54b38 100644
> --- a/mm/kfence/report.c
> +++ b/mm/kfence/report.c
> @@ -67,6 +67,9 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
> for (skipnr = 0; skipnr < num_entries; skipnr++) {
> int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
>
> + if (buf[0] == '.')
> + buf++;
> +
Unfortunately this does not work, since buf is an array. We'd need an
offset, and it should be determined outside the loop. I had a solution
like this, but it turned out quite complex (see below). And since most
architectures do not require this, decided that the safest option is to
use the macro approach with ARCH_FUNC_PREFIX, for which Christophe
already prepared a patch and tested:
https://lore.kernel.org/linux-mm/20210304144000.1148590-1-elver@google.com/
https://lkml.kernel.org/r/afaec81a551ef15345cb7d7563b3fac3d7041c3a.1614868445.git.christophe.leroy@csgroup.eu
Since KFENCE requires <asm/kfence.h> anyway, we'd prefer this approach
(vs. dynamically detecting).
Thanks,
-- Marco
------ >8 ------
diff --git a/mm/kfence/report.c b/mm/kfence/report.c
index 519f037720f5..b0590199b039 100644
--- a/mm/kfence/report.c
+++ b/mm/kfence/report.c
@@ -43,8 +43,8 @@ static void seq_con_printf(struct seq_file *seq, const char *fmt, ...)
static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries,
const enum kfence_error_type *type)
{
+ int skipnr, fallback = 0, fprefix_chars = 0;
char buf[64];
- int skipnr, fallback = 0;
if (type) {
/* Depending on error type, find different stack entries. */
@@ -64,11 +64,24 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
}
+ if (scnprintf(buf, sizeof(buf), "%ps", (void *)kfree)) {
+ /*
+ * Some architectures (e.g. ppc64) add a constant prefix to
+ * function names. Determine if such a prefix exists.
+ */
+ const char *str = strstr(buf, "kfree");
+
+ if (str)
+ fprefix_chars = str - buf;
+ }
+
for (skipnr = 0; skipnr < num_entries; skipnr++) {
- int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]);
+ int len = scnprintf(buf, sizeof(buf), "%ps", (void *)stack_entries[skipnr]) -
+ fprefix_chars;
- if (str_has_prefix(buf, "kfence_") || str_has_prefix(buf, "__kfence_") ||
- !strncmp(buf, "__slab_free", len)) {
+ if (str_has_prefix(buf + fprefix_chars, "kfence_") ||
+ str_has_prefix(buf + fprefix_chars, "__kfence_") ||
+ !strncmp(buf + fprefix_chars, "__slab_free", len)) {
/*
* In case of tail calls from any of the below
* to any of the above.
@@ -77,10 +90,10 @@ static int get_stack_skipnr(const unsigned long stack_entries[], int num_entries
}
/* Also the *_bulk() variants by only checking prefixes. */
- if (str_has_prefix(buf, "kfree") ||
- str_has_prefix(buf, "kmem_cache_free") ||
- str_has_prefix(buf, "__kmalloc") ||
- str_has_prefix(buf, "kmem_cache_alloc"))
+ if (str_has_prefix(buf + fprefix_chars, "kfree") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_free") ||
+ str_has_prefix(buf + fprefix_chars, "__kmalloc") ||
+ str_has_prefix(buf + fprefix_chars, "kmem_cache_alloc"))
goto found;
}
if (fallback < num_entries)
Powered by blists - more mailing lists