String functions can be useful in early boot, but using instrumented
versions can be problematic: eg on x86, some of the early boot code is
executing out of an identity mapping rather than the kernel virtual
addresses. Accessing any global variables at this point will lead to a
crash.
Tracing and KCOV are already disabled, and CONFIG_AMD_MEM_ENCRYPT will
additionally disable KASAN and stack protector.
Additionally disable GCOV, UBSAN, KCSAN, STACKLEAK_PLUGIN and branch
profiling, and make it unconditional to allow safe use of string
functions.
Signed-off-by: Arvind Sankar <[email protected]>
---
lib/Makefile | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/lib/Makefile b/lib/Makefile
index a4a4c6864f51..5e421769bbc6 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -8,7 +8,6 @@ ccflags-remove-$(CONFIG_FUNCTION_TRACER) += $(CC_FLAGS_FTRACE)
# These files are disabled because they produce lots of non-interesting and/or
# flaky coverage that is not a function of syscall inputs. For example,
# rbtree can be global and individual rotations don't correlate with inputs.
-KCOV_INSTRUMENT_string.o := n
KCOV_INSTRUMENT_rbtree.o := n
KCOV_INSTRUMENT_list_debug.o := n
KCOV_INSTRUMENT_debugobjects.o := n
@@ -20,12 +19,16 @@ KCOV_INSTRUMENT_fault-inject.o := n
# them into calls to themselves.
CFLAGS_string.o := -ffreestanding
-# Early boot use of cmdline, don't instrument it
-ifdef CONFIG_AMD_MEM_ENCRYPT
+# Early boot use of string functions, disable instrumentation
+GCOV_PROFILE_string.o := n
+KCOV_INSTRUMENT_string.o := n
KASAN_SANITIZE_string.o := n
+UBSAN_SANITIZE_string.o := n
+KCSAN_SANITIZE_string.o := n
CFLAGS_string.o += -fno-stack-protector
-endif
+CFLAGS_string.o += $(DISABLE_STACKLEAK_PLUGIN)
+CFLAGS_string.o += -DDISABLE_BRANCH_PROFILING
# Used by KCSAN while enabled, avoid recursion.
KCSAN_SANITIZE_random32.o := n
--
2.26.2
On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <[email protected]> wrote:
>
> String functions can be useful in early boot, but using instrumented
> versions can be problematic: eg on x86, some of the early boot code is
> executing out of an identity mapping rather than the kernel virtual
> addresses. Accessing any global variables at this point will lead to a
> crash.
>
> Tracing and KCOV are already disabled, and CONFIG_AMD_MEM_ENCRYPT will
> additionally disable KASAN and stack protector.
>
> Additionally disable GCOV, UBSAN, KCSAN, STACKLEAK_PLUGIN and branch
> profiling, and make it unconditional to allow safe use of string
> functions.
>
> Signed-off-by: Arvind Sankar <[email protected]>
> ---
> lib/Makefile | 11 +++++++----
> 1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/lib/Makefile b/lib/Makefile
> index a4a4c6864f51..5e421769bbc6 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -8,7 +8,6 @@ ccflags-remove-$(CONFIG_FUNCTION_TRACER) += $(CC_FLAGS_FTRACE)
> # These files are disabled because they produce lots of non-interesting and/or
> # flaky coverage that is not a function of syscall inputs. For example,
> # rbtree can be global and individual rotations don't correlate with inputs.
> -KCOV_INSTRUMENT_string.o := n
> KCOV_INSTRUMENT_rbtree.o := n
> KCOV_INSTRUMENT_list_debug.o := n
> KCOV_INSTRUMENT_debugobjects.o := n
> @@ -20,12 +19,16 @@ KCOV_INSTRUMENT_fault-inject.o := n
> # them into calls to themselves.
> CFLAGS_string.o := -ffreestanding
>
> -# Early boot use of cmdline, don't instrument it
> -ifdef CONFIG_AMD_MEM_ENCRYPT
> +# Early boot use of string functions, disable instrumentation
> +GCOV_PROFILE_string.o := n
> +KCOV_INSTRUMENT_string.o := n
> KASAN_SANITIZE_string.o := n
> +UBSAN_SANITIZE_string.o := n
> +KCSAN_SANITIZE_string.o := n
Ouch.
We have found manifestations of bugs in lib/string.c functions, e.g.:
https://groups.google.com/forum/#!msg/syzkaller-bugs/atbKWcFqE9s/x7AtoVoBAgAJ
https://groups.google.com/forum/#!msg/syzkaller-bugs/iGBUm-FDhkM/chl05uEgBAAJ
Is there any way this can be avoided?
If the use of string functions is really necessary, we could introduce
'__'-prefixed variants (maybe only for the ones that are needed?),
a'la
static void __always_inline strfoo_impl(...) { ... }
void strfoo(...) { strfoo_impl(...); }
EXPORT_SYMBOL(strfoo);
noinstr void __strfoo(...) { strfoo_impl(...); }
EXPORT_SYMBOL(__strfoo);
// If __HAVE_ARCH_STRFOO then we can probably just alias __strfoo to strfoo.
But if the whole thing could be avoided entirely would be even better.
Thanks,
-- Marco
> CFLAGS_string.o += -fno-stack-protector
> -endif
> +CFLAGS_string.o += $(DISABLE_STACKLEAK_PLUGIN)
> +CFLAGS_string.o += -DDISABLE_BRANCH_PROFILING
On Tue, Sep 08, 2020 at 11:39:11AM +0200, Marco Elver wrote:
> On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <[email protected]> wrote:
> >
> > String functions can be useful in early boot, but using instrumented
> > versions can be problematic: eg on x86, some of the early boot code is
> > executing out of an identity mapping rather than the kernel virtual
> > addresses. Accessing any global variables at this point will lead to a
> > crash.
> >
> > Tracing and KCOV are already disabled, and CONFIG_AMD_MEM_ENCRYPT will
> > additionally disable KASAN and stack protector.
> >
> > Additionally disable GCOV, UBSAN, KCSAN, STACKLEAK_PLUGIN and branch
> > profiling, and make it unconditional to allow safe use of string
> > functions.
> >
> > Signed-off-by: Arvind Sankar <[email protected]>
> > ---
> > lib/Makefile | 11 +++++++----
> > 1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/lib/Makefile b/lib/Makefile
> > index a4a4c6864f51..5e421769bbc6 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -8,7 +8,6 @@ ccflags-remove-$(CONFIG_FUNCTION_TRACER) += $(CC_FLAGS_FTRACE)
> > # These files are disabled because they produce lots of non-interesting and/or
> > # flaky coverage that is not a function of syscall inputs. For example,
> > # rbtree can be global and individual rotations don't correlate with inputs.
> > -KCOV_INSTRUMENT_string.o := n
> > KCOV_INSTRUMENT_rbtree.o := n
> > KCOV_INSTRUMENT_list_debug.o := n
> > KCOV_INSTRUMENT_debugobjects.o := n
> > @@ -20,12 +19,16 @@ KCOV_INSTRUMENT_fault-inject.o := n
> > # them into calls to themselves.
> > CFLAGS_string.o := -ffreestanding
> >
> > -# Early boot use of cmdline, don't instrument it
> > -ifdef CONFIG_AMD_MEM_ENCRYPT
> > +# Early boot use of string functions, disable instrumentation
> > +GCOV_PROFILE_string.o := n
> > +KCOV_INSTRUMENT_string.o := n
> > KASAN_SANITIZE_string.o := n
> > +UBSAN_SANITIZE_string.o := n
> > +KCSAN_SANITIZE_string.o := n
>
> Ouch.
>
> We have found manifestations of bugs in lib/string.c functions, e.g.:
> https://groups.google.com/forum/#!msg/syzkaller-bugs/atbKWcFqE9s/x7AtoVoBAgAJ
> https://groups.google.com/forum/#!msg/syzkaller-bugs/iGBUm-FDhkM/chl05uEgBAAJ
>
> Is there any way this can be avoided?
Agreed: I would like to keep this instrumentation; it's a common place
to find bugs, security issues, etc.
--
Kees Cook
On Tue, Sep 08, 2020 at 10:21:32AM -0700, Kees Cook wrote:
> On Tue, Sep 08, 2020 at 11:39:11AM +0200, Marco Elver wrote:
> > On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <[email protected]> wrote:
> > >
> > > String functions can be useful in early boot, but using instrumented
> > > versions can be problematic: eg on x86, some of the early boot code is
> > > executing out of an identity mapping rather than the kernel virtual
> > > addresses. Accessing any global variables at this point will lead to a
> > > crash.
> > >
> >
> > Ouch.
> >
> > We have found manifestations of bugs in lib/string.c functions, e.g.:
> > https://groups.google.com/forum/#!msg/syzkaller-bugs/atbKWcFqE9s/x7AtoVoBAgAJ
> > https://groups.google.com/forum/#!msg/syzkaller-bugs/iGBUm-FDhkM/chl05uEgBAAJ
> >
> > Is there any way this can be avoided?
>
> Agreed: I would like to keep this instrumentation; it's a common place
> to find bugs, security issues, etc.
>
> --
> Kees Cook
Ok, understood. I'll revise to open-code the strscpy instead.
Is instrumentation supported on x86-32? load_ucode_bsp() on 32-bit is
called before paging is enabled, and load_ucode_bsp() itself, along with
eg lib/earlycpio and lib/string that it uses, don't have anything to
disable instrumentation. kcov, kasan, kcsan are unsupported already on
32-bit, but the others like gcov and PROFILE_ALL_BRANCHES look like they
would just cause a crash if microcode loading is enabled.
On Tue, Sep 8, 2020 at 8:40 PM Arvind Sankar <[email protected]> wrote:
>
> On Tue, Sep 08, 2020 at 10:21:32AM -0700, Kees Cook wrote:
> > On Tue, Sep 08, 2020 at 11:39:11AM +0200, Marco Elver wrote:
> > > On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <[email protected]> wrote:
> > > >
> > > > String functions can be useful in early boot, but using instrumented
> > > > versions can be problematic: eg on x86, some of the early boot code is
> > > > executing out of an identity mapping rather than the kernel virtual
> > > > addresses. Accessing any global variables at this point will lead to a
> > > > crash.
> > > >
> > >
> > > Ouch.
> > >
> > > We have found manifestations of bugs in lib/string.c functions, e.g.:
> > > https://groups.google.com/forum/#!msg/syzkaller-bugs/atbKWcFqE9s/x7AtoVoBAgAJ
> > > https://groups.google.com/forum/#!msg/syzkaller-bugs/iGBUm-FDhkM/chl05uEgBAAJ
> > >
> > > Is there any way this can be avoided?
> >
> > Agreed: I would like to keep this instrumentation; it's a common place
> > to find bugs, security issues, etc.
> >
> > --
> > Kees Cook
>
> Ok, understood. I'll revise to open-code the strscpy instead.
>
> Is instrumentation supported on x86-32? load_ucode_bsp() on 32-bit is
> called before paging is enabled, and load_ucode_bsp() itself, along with
> eg lib/earlycpio and lib/string that it uses, don't have anything to
> disable instrumentation. kcov, kasan, kcsan are unsupported already on
> 32-bit, but the others like gcov and PROFILE_ALL_BRANCHES look like they
> would just cause a crash if microcode loading is enabled.
I agree we should not disable instrumentation of such common functions.
Instead of open-coding these functions maybe we could produce both
instrumented and non-instrumented versions from the same source
implementation. Namely, place implementation in a header function with
always_inline attribute and include it from 2 source files, one with
instrumentation enabled and another with instrumentation disabled.
This way we could produce strscpy (instrumented) and __strscpy
(non-instrumented) from the same source.