lkml.org 
[lkml]   [2019]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] x86/mm/mem_encrypt: Disable all instrumentation for SME early boot code
Date
On 4/8/19 2:08 PM, Borislav Petkov wrote:On 5/8/19 2:08 PM, Borislav 
Petkov wrote:> On Mon, Apr 08, 2019 at 06:41:30PM +0000, Gary R Hook
wrote:
>> Again, not arguing. I completely understand. However, to be fair,
this
>> isn't about SME having trouble with those facilities, this is about

>> using certain features (e.g. command line option processing) early
in
>> the boot. Any complex feature could have had that requirement, don't
you
>> think?

>
> Sure, but then why do we need that patch at all then? Why do we need to
> disable instrumentation for SME early code and not for other early code?
>
> I mean, if you grep around the tree you can see a bunch of
> KASAN_SANITIZE but in lib/ we only have
>
> lib/Makefile:210:KASAN_SANITIZE_stackdepot.o := n
>
> which is special. But the rest of the generic code in lib/ or
> arch/x86/lib/ isn't.
>
> Now, there's this:
>
> arch/x86/boot/Makefile:12:KASAN_SANITIZE := n
> arch/x86/boot/compressed/Makefile:20:KASAN_SANITIZE
:= n
>
> which disables KASAN for all boot code.
>
> And this is what you mean - all early boot code should not be sanitized.
>
> Which also gives the right solution, IMO:
>
> cmdline.o should not be sanitized only when used in the boot code. But
> that is already the case.
>
> So why do you need to disable KASAN for arch/x86/lib/cmdline.c?
>
> Because for those two:
>
> arch/x86/boot/cmdline.c
> arch/x86/boot/compressed/cmdline.c
>
> that should already be the case due to the Makefile defines above.

Except that we're not talking about that code.


I probably should have defined terms, so please allow me to back up.
When I say "early boot" I meant what happens -after- decompression, when
the kernel proper has been laid out in memory and starts to run. This
is -after- the boot code has been executed, which means that the
cmdline.c to which you refer above is no longer extant in memory.

If my usage of the term "early boot" is a misnomer, I can only
apologize. And ask what term is in common use to describe what is
happening at that point in time.

Since, for this discussion, we're already in start_kernel(), the only
cmdline.c available is the one in arch/x86/lib. That's that one that is
instrumented by KASAN, and the one that is causing problems in this
scenario. The strncmp(), too.

>> Right. My goal was to get a conversation started, because folks are
>> running into this problem when KASAN is enabled.
>
> You say KASAN. Why is there KCOV_INSTRUMENT_cmdline.o too?

I don't care if KCOV_INSTRUMENT is enabled or not, but it's disabled for
arch/x86/mm/mem_encrypt_identity.c, so it seems reasonable that it
should be disable for this file, too, in the context of resolving this
problem.

To be more precise, the change is for "instrumentation", in general.


>> N.B. Here's another facet of this problem: cmdline.c doesn't (today)
>> contain anything that would trigger the stack protector. However, it's
>> possible to enable the stack protector globally when building, right? In
>> which case, a boot would fail, so we have the same issue: early boot
>> code has special requirements / restrictions.
>
> How so?

<sidebar>
Makefile contains

stackp-flags-$(CONFIG_STACKPROTECTOR) := -fstack-protector
stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) :=
-fstack-protector-strong

This means that (as I understand it) stack protection is decided by the
compiler, and is based on certain conditions in the code. This implies
that not every function will necessarily be instrumented.

However, if you decide to force the issue with something like

stackp-flags-$(CONFIG_STACKPROTECTOR) :=
-fstack-protector-full
stackp-flags-$(CONFIG_STACKPROTECTOR_STRONG) :=
-fstack-protector-all

Unless otherwise disabled, I believe this causes everything to be
instrumented. Which results in a boot failure. (Actually, in my tests
the system restarts after the decompression.) Note that intrumentation
such as KASAN isn't involved here. And I figure that doing this is
unsupported. It was just an interesting discovery.
</sidebar>

However, not relevant to the KASAN instrumentation problem.

Recap:

mem_encrypt_identity.c uses two common functions. The code in
mem_encrypt_identity.c runs soon after start_kernel() is invoked. The
SME feature command line parameter is searched for, and uses those two
common functions.

If instrumentation is enabled, it is applied to those to common
functions (but not mem_encrypt_identity.c). But if support
infrastructure for instrumentation is not initialized before the code in
mem_encrypt_identity is invoked, the kernel fails to boot.

After discussion over several weeks, we see the following options for a
solution:

1) Create a local static copy of strncmp in mem_encrypt_identity.c

2) Turn off instrumentation for lib/cmdline.c. The risk is that any
changes to its code would not enjoy the benefits of KASAN/etc testing
(if enabled).

3) Make a local static copy of cmdline_find_option() inside of
mem_encrypt_identity.c.

4) Use #defines and #include to have cmdline.c included within
mem_encrypt_identity.c. This maintains a single source file and
continues to allow the function to be instrumented for use everywhere
elsewhere in the kernel.

We believe (1) is a simple and effective choice, similar to inlining.

If a single source file (for cmdline.c) is preferred, option (4)
maintains that paradigm but gets the job done fairly cleanly. I
understand if there is reticience about include source files, but it's
not IMO an abhorrent practice. This also points to a single file as the
origin of the required function, so no confusion ensues.

I believe TGLX raised the issue of "what happens if we find a bug in
cmdline_find_option()?" Despite the fact that the file has been changed
only once in 4 years (by Thomas) it's an important consideration.
Another reason I lean towards (4), above.

I'm -hoping- this is more clear and succinct. How shall we proceed?
\
 
 \ /
  Last update: 2019-04-26 17:12    [W:0.096 / U:0.288 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site