Messages in this thread | | | From | Gary R Hook <> | Subject | Re: [PATCH] x86/mm/mem_encrypt: Disable all instrumentation for SME early boot code | Date | Fri, 26 Apr 2019 15:11:17 +0000 |
| |
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?
| |