Bug 206337

Summary:	KASAN: str* functions are not instrumented with CONFIG_AMD_MEM_ENCRYPT
Product:	Memory Management	Reporter:	Dmitry Vyukov (dvyukov)
Component:	Sanitizers	Assignee:	MM/Sanitizers virtual assignee (mm_sanitizers)
Status:	NEW ---
Severity:	normal	CC:	bp, kasan-dev
Priority:	P1
Hardware:	All
OS:	Linux
Kernel Version:	5.1+	Subsystem:
Regression:	No	Bisected commit-id:

Description Dmitry Vyukov 2020-01-28 08:25:49 UTC

The following commit adds the following change:

commit b51ce3744f115850166f3d6c292b9c8cb849ad4f
Author: Gary Hook <Gary.Hook@amd.com>
Date:   Mon Apr 29 22:22:58 2019 +0000

    x86/mm/mem_encrypt: Disable all instrumentation for early SME setup


--- a/lib/Makefile
+++ b/lib/Makefile
@@ -17,6 +17,17 @@ KCOV_INSTRUMENT_list_debug.o := n
+# Early boot use of cmdline, don't instrument it
+ifdef CONFIG_AMD_MEM_ENCRYPT
+KASAN_SANITIZE_string.o := n
+endif


This is way too coarse-gained instrumentation suppression for an early-boot problem. str* functions are widely used throughout kernel during it's whole lifetime. They should not be disabled because of a single boot-time problem.

We probably need to do something similar to what we do for mem* functions:

// arch/x86/include/asm/string_64.h
#if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
/*
 * For files that not instrumented (e.g. mm/slub.c) we
 * should use not instrumented version of mem* functions.
 */
#undef memcpy
#define memcpy(dst, src, len) __memcpy(dst, src, len)

Then disabling instrumentation in the single problematic file should help for direct calls (I don't know if that was a direct call, though).
Or do something else instead.

Comment 1 Dmitry Vyukov 2020-01-28 08:30:27 UTC

On Tue, Jan 28, 2020 at 9:25 AM <bugzilla-daemon@bugzilla.kernel.org> wrote:
>
> https://bugzilla.kernel.org/show_bug.cgi?id=206337
>
>             Bug ID: 206337
>            Summary: KASAN: str* functions are not instrumented with
>                     CONFIG_AMD_MEM_ENCRYPT
>            Product: Memory Management
>            Version: 2.5
>     Kernel Version: 5.1+
>           Hardware: All
>                 OS: Linux
>               Tree: Mainline
>             Status: NEW
>           Severity: normal
>           Priority: P1
>          Component: Sanitizers
>           Assignee: mm_sanitizers@kernel-bugs.kernel.org
>           Reporter: dvyukov@google.com
>                 CC: kasan-dev@googlegroups.com
>         Regression: No
>
> The following commit adds the following change:
>
> commit b51ce3744f115850166f3d6c292b9c8cb849ad4f
> Author: Gary Hook <Gary.Hook@amd.com>
> Date:   Mon Apr 29 22:22:58 2019 +0000
>
>     x86/mm/mem_encrypt: Disable all instrumentation for early SME setup
>
>
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -17,6 +17,17 @@ KCOV_INSTRUMENT_list_debug.o := n
> +# Early boot use of cmdline, don't instrument it
> +ifdef CONFIG_AMD_MEM_ENCRYPT
> +KASAN_SANITIZE_string.o := n
> +endif
>
>
> This is way too coarse-gained instrumentation suppression for an early-boot
> problem. str* functions are widely used throughout kernel during it's whole
> lifetime. They should not be disabled because of a single boot-time problem.
>
> We probably need to do something similar to what we do for mem* functions:
>
> // arch/x86/include/asm/string_64.h
> #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__)
> /*
>  * For files that not instrumented (e.g. mm/slub.c) we
>  * should use not instrumented version of mem* functions.
>  */
> #undef memcpy
> #define memcpy(dst, src, len) __memcpy(dst, src, len)
>
> Then disabling instrumentation in the single problematic file should help for
> direct calls (I don't know if that was a direct call, though).
> Or do something else instead.

+Gary, I can't find you in bugzilla, so CCing here, but please comment
on the bug report.

Comment 2 Dmitry Vyukov 2020-01-28 08:34:27 UTC

FTR, I got auto-response that the email can't be delivered to the Gary's email address (does not exist anymore).

Comment 3 Borislav Petkov 2020-01-28 14:37:39 UTC

We could revert back to the first version of this:

https://marc.info/?l=linux-kernel&m=155440967805174&w=2

which excluded arch/x86/lib/cmdline.c only from instrumentation.

Comment 4 Dmitry Vyukov 2020-01-28 14:40:48 UTC

Overall this looks better. arch/x86/lib/cmdline.c is only used during boot and with fixed data that is not controlled by any untrusted subjects (?).

Comment 5 Borislav Petkov 2020-01-28 14:46:02 UTC

The only data I see controlled from the outside is boot_command_line and that by whoever is able to change it. But if one can change boot_command_line, the setup has much bigger problems AFAICT.