[RFC PATCH 0/2] Allow use of lib/string in early boot

[RFC PATCH 0/2] Allow use of lib/string in early boot

[Arvind Sankar]

The string functions can currently not be used safely in early boot
code, at least on x86, as some of that code will be executing out of
the identity mapping rather than kernel virtual address space.
Instrumentation options that insert accesses to any global data will
cause a crash.

I'm proposing to disable instrumentation for lib/string.c to allow the
string functions to be usable, and the second patch is an example use
case.

However, I'm not very familiar with the actual uses of that
instrumentation and don't know whether disabling it all for lib/string
would be a terrible idea, hence the RFC.

Thanks.

Arvind Sankar (2):
  lib/string: Disable instrumentation
  x86/cmdline: Use strscpy to initialize boot_command_line

 arch/x86/kernel/head64.c  |  2 +-
 arch/x86/kernel/head_32.S | 11 +++++------
 lib/Makefile              | 11 +++++++----
 3 files changed, 13 insertions(+), 11 deletions(-)

-- 
2.26.2

[RFC PATCH 1/2] lib/string: Disable instrumentation

[Arvind Sankar]

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 <nivedita@alum.mit.edu>
---
 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

[RFC PATCH 2/2] x86/cmdline: Use strscpy to initialize boot_command_line

[Arvind Sankar]

The x86 boot protocol requires the kernel command line to be a
NUL-terminated string of length at most COMMAND_LINE_SIZE (including the
terminating NUL). In case the bootloader messed up and the command line
is too long (hence not NUL-terminated), use strscpy to copy the command
line into boot_command_line. This ensures that boot_command_line is
NUL-terminated, and it also avoids accessing beyond the actual end of
the command line if it was properly NUL-terminated.

Note that setup_arch() will already force command_line to be
NUL-terminated by using strlcpy(), as well as boot_command_line if a
builtin command line is configured. If boot_command_line was not
initially NUL-terminated, the strlen() inside of strlcpy()/strlcat()
will run beyond boot_command_line, but this is almost certainly
harmless in practice.

Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
---
 arch/x86/kernel/head64.c  |  2 +-
 arch/x86/kernel/head_32.S | 11 +++++------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
index cbb71c1b574f..740dd05b9462 100644
--- a/arch/x86/kernel/head64.c
+++ b/arch/x86/kernel/head64.c
@@ -410,7 +410,7 @@ static void __init copy_bootdata(char *real_mode_data)
 	cmd_line_ptr = get_cmd_line_ptr();
 	if (cmd_line_ptr) {
 		command_line = __va(cmd_line_ptr);
-		memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
+		strscpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
 	}
 
 	/*
diff --git a/arch/x86/kernel/head_32.S b/arch/x86/kernel/head_32.S
index 7ed84c282233..2a7ced159d6b 100644
--- a/arch/x86/kernel/head_32.S
+++ b/arch/x86/kernel/head_32.S
@@ -102,13 +102,12 @@ SYM_CODE_START(startup_32)
 	cld
 	rep
 	movsl
-	movl pa(boot_params) + NEW_CL_POINTER,%esi
-	andl %esi,%esi
+	movl pa(boot_params) + NEW_CL_POINTER,%edx
+	testl %edx,%edx
 	jz 1f			# No command line
-	movl $pa(boot_command_line),%edi
-	movl $(COMMAND_LINE_SIZE/4),%ecx
-	rep
-	movsl
+	movl $pa(boot_command_line),%eax
+	movl $COMMAND_LINE_SIZE,%ecx
+	call strscpy
 1:
 
 #ifdef CONFIG_OLPC
-- 
2.26.2

Re: [RFC PATCH 2/2] x86/cmdline: Use strscpy to initialize boot_command_line

[Randy Dunlap]

On 9/5/20 3:23 PM, Arvind Sankar wrote:
> The x86 boot protocol requires the kernel command line to be a
> NUL-terminated string of length at most COMMAND_LINE_SIZE (including the
> terminating NUL). In case the bootloader messed up and the command line
> is too long (hence not NUL-terminated), use strscpy to copy the command
> line into boot_command_line. This ensures that boot_command_line is
> NUL-terminated, and it also avoids accessing beyond the actual end of
> the command line if it was properly NUL-terminated.
> 
> Note that setup_arch() will already force command_line to be
> NUL-terminated by using strlcpy(), as well as boot_command_line if a
> builtin command line is configured. If boot_command_line was not
> initially NUL-terminated, the strlen() inside of strlcpy()/strlcat()
> will run beyond boot_command_line, but this is almost certainly
> harmless in practice.
> 
> Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>

Hi,
Just for my enlightenment, what would be wrong with:

(which is done in arch/m68/kernel/setup_no.c)

> ---
>  arch/x86/kernel/head64.c  |  2 +-
>  arch/x86/kernel/head_32.S | 11 +++++------
>  2 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> index cbb71c1b574f..740dd05b9462 100644
> --- a/arch/x86/kernel/head64.c
> +++ b/arch/x86/kernel/head64.c
> @@ -410,7 +410,7 @@ static void __init copy_bootdata(char *real_mode_data)
>  	cmd_line_ptr = get_cmd_line_ptr();
>  	if (cmd_line_ptr) {
>  		command_line = __va(cmd_line_ptr);
> 		memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
> +		boot_command_line[COMMAND_LINE_SIZE - 1] = 0;
>  	}
>  
>  	/*


thanks.
-- 
~Randy

Re: [RFC PATCH 2/2] x86/cmdline: Use strscpy to initialize boot_command_line

[Arvind Sankar]

On Sat, Sep 05, 2020 at 03:59:04PM -0700, Randy Dunlap wrote:
> On 9/5/20 3:23 PM, Arvind Sankar wrote:
> > The x86 boot protocol requires the kernel command line to be a
> > NUL-terminated string of length at most COMMAND_LINE_SIZE (including the
> > terminating NUL). In case the bootloader messed up and the command line
> > is too long (hence not NUL-terminated), use strscpy to copy the command
> > line into boot_command_line. This ensures that boot_command_line is
> > NUL-terminated, and it also avoids accessing beyond the actual end of
> > the command line if it was properly NUL-terminated.
> > 
> > Note that setup_arch() will already force command_line to be
> > NUL-terminated by using strlcpy(), as well as boot_command_line if a
> > builtin command line is configured. If boot_command_line was not
> > initially NUL-terminated, the strlen() inside of strlcpy()/strlcat()
> > will run beyond boot_command_line, but this is almost certainly
> > harmless in practice.
> > 
> > Signed-off-by: Arvind Sankar <nivedita@alum.mit.edu>
> 
> Hi,
> Just for my enlightenment, what would be wrong with:
> 
> (which is done in arch/m68/kernel/setup_no.c)
> 
> > ---
> >  arch/x86/kernel/head64.c  |  2 +-
> >  arch/x86/kernel/head_32.S | 11 +++++------
> >  2 files changed, 6 insertions(+), 7 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/head64.c b/arch/x86/kernel/head64.c
> > index cbb71c1b574f..740dd05b9462 100644
> > --- a/arch/x86/kernel/head64.c
> > +++ b/arch/x86/kernel/head64.c
> > @@ -410,7 +410,7 @@ static void __init copy_bootdata(char *real_mode_data)
> >  	cmd_line_ptr = get_cmd_line_ptr();
> >  	if (cmd_line_ptr) {
> >  		command_line = __va(cmd_line_ptr);
> > 		memcpy(boot_command_line, command_line, COMMAND_LINE_SIZE);
> > +		boot_command_line[COMMAND_LINE_SIZE - 1] = 0;
> >  	}
> >  
> >  	/*
> 
> 
> thanks.
> -- 
> ~Randy
> 

That still accesses beyond the end of the bootloader's command line,
which could theoretically be a bad thing: eg the EFI stub only allocates
enough space for the actual length of the command line, rather than the
full COMMAND_LINE_SIZE. But yeah, that was my first version of this
patch.

> > NUL-terminated, and it also avoids accessing beyond the actual end of
> > the command line if it was properly NUL-terminated.

Re: [RFC PATCH 1/2] lib/string: Disable instrumentation

[Marco Elver]

On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <nivedita@alum.mit.edu> 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 <nivedita@alum.mit.edu>
> ---
>  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

Re: [RFC PATCH 1/2] lib/string: Disable instrumentation

[Kees Cook]

On Tue, Sep 08, 2020 at 11:39:11AM +0200, Marco Elver wrote:
> On Sun, 6 Sep 2020 at 00:23, Arvind Sankar <nivedita@alum.mit.edu> 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 <nivedita@alum.mit.edu>
> > ---
> >  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

Re: [RFC PATCH 1/2] lib/string: Disable instrumentation

[Arvind Sankar]

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 <nivedita@alum.mit.edu> 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.

Re: [RFC PATCH 1/2] lib/string: Disable instrumentation

[Dmitry Vyukov]

On Tue, Sep 8, 2020 at 8:40 PM Arvind Sankar <nivedita@alum.mit.edu> 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 <nivedita@alum.mit.edu> 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.