Enablement of AMD's Secure Memory Encryption feature is determined
very early in the boot cycle. Part of this procedure involves scanning
the command line for the paramater 'mem_encrypt'.
To determine intended state, the function sme_enable() uses library
functions cmdline_find_option() and strncmp(). Their use occurs early
enough such that we can't assume that any instrumentation subsystem is
initialized. For example, making calls to a KASAN-instrumented
function before KASAN is set up will likely result in the use of
uninitialized memory and a boot failure.
Avoid instrumenting these dependent functions by:
1) Making a local, static, renamed copy of strncpy() for use solely in
mem_encrypt_identity.c. In this file we are able to vet its few uses
and avoid exposing the rest of the kernel to a ubiquitously used but
un-instrumented function.
2) Disable instrumention of arch/x86/lib/cmdline.c based on the
assumption that the needed function (cmdline_find_option()) is vetted
through its use to date, and contains no lurking flaws that have not
yet been found through instrumentation such as KASAN.
Fixes: aca20d546214 ("x86/mm: Add support to make use of Secure Memory Encryption")
Reported-by: Li RongQing <[email protected]>
Signed-off-by: Gary R Hook <[email protected]>
---
arch/x86/lib/Makefile | 13 +++++++++++++
arch/x86/mm/mem_encrypt_identity.c | 26 ++++++++++++++++++++++++--
2 files changed, 37 insertions(+), 2 deletions(-)
diff --git a/arch/x86/lib/Makefile b/arch/x86/lib/Makefile
index 140e61843a07..38182a64fa81 100644
--- a/arch/x86/lib/Makefile
+++ b/arch/x86/lib/Makefile
@@ -6,6 +6,19 @@
# Produces uninteresting flaky coverage.
KCOV_INSTRUMENT_delay.o := n
+# SME early boot code checks the cmdline, so don't instrument
+KCOV_INSTRUMENT_cmdline.o := n
+
+KASAN_SANITIZE_cmdline.o := n
+
+ifdef CONFIG_FUNCTION_TRACER
+CFLAGS_REMOVE_cmdline.o = -pg
+endif
+
+# No stack protector
+nostackp := $(call cc-option, -fno-stack-protector)
+CFLAGS_cmdline.o := $(nostackp)
+
inat_tables_script = $(srctree)/arch/x86/tools/gen-insn-attr-x86.awk
inat_tables_maps = $(srctree)/arch/x86/lib/x86-opcode-map.txt
quiet_cmd_inat_tables = GEN $@
diff --git a/arch/x86/mm/mem_encrypt_identity.c b/arch/x86/mm/mem_encrypt_identity.c
index 4aa9b1480866..0a68d7ccb371 100644
--- a/arch/x86/mm/mem_encrypt_identity.c
+++ b/arch/x86/mm/mem_encrypt_identity.c
@@ -77,6 +77,28 @@ static char sme_cmdline_arg[] __initdata = "mem_encrypt";
static char sme_cmdline_on[] __initdata = "on";
static char sme_cmdline_off[] __initdata = "off";
+/*
+ * Local copy to avoid instrumentation
+ * Copied from lib/string.c and renamed to be unique.
+ * This file is early boot code, and we assume that instrumentation
+ * subsystems (e.g. KASAN) are not yet initialized.
+ */
+static int sme_strncmp(const char *cs, const char *ct, size_t count)
+{
+ unsigned char c1, c2;
+
+ while (count) {
+ c1 = *cs++;
+ c2 = *ct++;
+ if (c1 != c2)
+ return c1 < c2 ? -1 : 1;
+ if (!c1)
+ break;
+ count--;
+ }
+ return 0;
+}
+
static void __init sme_clear_pgd(struct sme_populate_pgd_data *ppd)
{
unsigned long pgd_start, pgd_end, pgd_size;
@@ -557,9 +579,9 @@ void __init sme_enable(struct boot_params *bp)
cmdline_find_option(cmdline_ptr, cmdline_arg, buffer, sizeof(buffer));
- if (!strncmp(buffer, cmdline_on, sizeof(buffer)))
+ if (!sme_strncmp(buffer, cmdline_on, sizeof(buffer)))
sme_me_mask = me_mask;
- else if (!strncmp(buffer, cmdline_off, sizeof(buffer)))
+ else if (!sme_strncmp(buffer, cmdline_off, sizeof(buffer)))
sme_me_mask = 0;
else
sme_me_mask = active_by_default ? me_mask : 0;
On Thu, 4 Apr 2019, Hook, Gary wrote:
> Enablement of AMD's Secure Memory Encryption feature is determined
> very early in the boot cycle. Part of this procedure involves scanning
> the command line for the paramater 'mem_encrypt'.
>
> To determine intended state, the function sme_enable() uses library
> functions cmdline_find_option() and strncmp(). Their use occurs early
> enough such that we can't assume that any instrumentation subsystem is
> initialized. For example, making calls to a KASAN-instrumented
> function before KASAN is set up will likely result in the use of
> uninitialized memory and a boot failure.
>
> Avoid instrumenting these dependent functions by:
>
> 1) Making a local, static, renamed copy of strncpy() for use solely in
> mem_encrypt_identity.c. In this file we are able to vet its few uses
> and avoid exposing the rest of the kernel to a ubiquitously used but
> un-instrumented function.
>
> 2) Disable instrumention of arch/x86/lib/cmdline.c based on the
> assumption that the needed function (cmdline_find_option()) is vetted
> through its use to date, and contains no lurking flaws that have not
> yet been found through instrumentation such as KASAN.
Not happy about that :)
> +# SME early boot code checks the cmdline, so don't instrument
> +KCOV_INSTRUMENT_cmdline.o := n
> +
> +KASAN_SANITIZE_cmdline.o := n
If we can't come up with a better solution then this needs to depend on
CONFIG_MEM_ENCRYPT so we still can run KASAN on cmdline.c to catch crap
when the code is modified in the future.
Thanks,
tglx
On 4/4/19 3:42 PM, Thomas Gleixner wrote:
> On Thu, 4 Apr 2019, Hook, Gary wrote:
>
>> Enablement of AMD's Secure Memory Encryption feature is determined
>> very early in the boot cycle. Part of this procedure involves scanning
>> the command line for the paramater 'mem_encrypt'.
>>
>> To determine intended state, the function sme_enable() uses library
>> functions cmdline_find_option() and strncmp(). Their use occurs early
>> enough such that we can't assume that any instrumentation subsystem is
>> initialized. For example, making calls to a KASAN-instrumented
>> function before KASAN is set up will likely result in the use of
>> uninitialized memory and a boot failure.
>>
>> Avoid instrumenting these dependent functions by:
>>
>> 1) Making a local, static, renamed copy of strncpy() for use solely in
>> mem_encrypt_identity.c. In this file we are able to vet its few uses
>> and avoid exposing the rest of the kernel to a ubiquitously used but
>> un-instrumented function.
>>
>> 2) Disable instrumention of arch/x86/lib/cmdline.c based on the
>> assumption that the needed function (cmdline_find_option()) is vetted
>> through its use to date, and contains no lurking flaws that have not
>> yet been found through instrumentation such as KASAN.
>
> Not happy about that :)
My reasoning (not arguing): the file has been touched exactly one time
in 4 years, by Thomas. Doesn't appear to be a candidate for constant
modification, so this approach doesn't seem risky to me. I could be wrong.
>> +# SME early boot code checks the cmdline, so don't instrument
>> +KCOV_INSTRUMENT_cmdline.o := n
>> +
>> +KASAN_SANITIZE_cmdline.o := n
>
> If we can't come up with a better solution then this needs to depend on
> CONFIG_MEM_ENCRYPT so we still can run KASAN on cmdline.c to catch crap
> when the code is modified in the future.
We have considered this problem, and see no alternative solution. But I
would love to come up with one.
In the meantime, I've already created a v2 patch that fulfills your
request. I'll wait a few more days for comments before posting.
On Mon, Apr 08, 2019 at 04:46:31PM +0000, Gary R Hook wrote:
> My reasoning (not arguing): the file has been touched exactly one time
> in 4 years, by Thomas. Doesn't appear to be a candidate for constant
> modification, so this approach doesn't seem risky to me. I could be wrong.
The problem, like we discussed it with Tom offlist, is that you simply
cannot turn off instrumentation for those generic files just because SME
has trouble with them, and that last thing can be any vendor-specific
feature.
Even if the functions there are "trivial" now (doesn't mean new stuff
won't be added there in the future and we forget about the disabled
instrumentation here.)
We simply cannot constrain generic compilation units like that. So the
functions there either need to be copied or ifdeffed. At the time SME
was going in, the intention was to reuse code so that there is less
duplication. But if there's trouble doing that sharing, then we need to
"unshare" it again. Or come up with something else slick and clean.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 4/8/19 11:58 AM, Borislav Petkov wrote:
> On Mon, Apr 08, 2019 at 04:46:31PM +0000, Gary R Hook wrote:
>> My reasoning (not arguing): the file has been touched exactly one time
>> in 4 years, by Thomas. Doesn't appear to be a candidate for constant
>> modification, so this approach doesn't seem risky to me. I could be wrong.
>
> The problem, like we discussed it with Tom offlist, is that you simply
> cannot turn off instrumentation for those generic files just because SME
> has trouble with them, and that last thing can be any vendor-specific
> feature.
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?
> Even if the functions there are "trivial" now (doesn't mean new stuff
> won't be added there in the future and we forget about the disabled
> instrumentation here.)
Absolutely agree. Recognizing that things can be forgotten has weighed
heavily on me in light of this. I don't like that possibility at all.
> We simply cannot constrain generic compilation units like that. So the
> functions there either need to be copied or ifdeffed. At the time SME
> was going in, the intention was to reuse code so that there is less
> duplication. But if there's trouble doing that sharing, then we need to
> "unshare" it again. Or come up with something else slick and clean.
Right. My goal was to get a conversation started, because folks are
running into this problem when KASAN is enabled.
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.
I'm not sure what the "right" solution is, but I appreciate your time
and expertise to discuss. And now I have another idea I'm going to go
try out.
grh
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.
> 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?
> 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?
This .config boots here in a vm just fine.
$ grep STACKPROT .config
CONFIG_CC_HAS_SANE_STACKPROTECTOR=y
CONFIG_HAVE_STACKPROTECTOR=y
CONFIG_CC_HAS_STACKPROTECTOR_NONE=y
CONFIG_STACKPROTECTOR=y
CONFIG_STACKPROTECTOR_STRONG=y
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 4/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.
This isn't the code in arch/x86/boot. This is post decompression phase and
part of the actual kernel early boot code where the page tables are being
modified with the encryption mask and the actual kernel load address (see
the sme_enable() call from __startup_64() in arch/x86/kernel/head64.c).
>
>> 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?
>
>> 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?
>
> This .config boots here in a vm just fine.
The SME path that this addresses is bypassed when you boot in a VM. The
sme_enable() code detects that it is executing under a hypervisor and does
not check the command line or do a string compare.
Thanks,
Tom
>
> $ grep STACKPROT .config
> CONFIG_CC_HAS_SANE_STACKPROTECTOR=y
> CONFIG_HAVE_STACKPROTECTOR=y
> CONFIG_CC_HAS_STACKPROTECTOR_NONE=y
> CONFIG_STACKPROTECTOR=y
> CONFIG_STACKPROTECTOR_STRONG=y
>
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?
On Fri, Apr 26, 2019 at 03:11:17PM +0000, Gary R Hook wrote:
> 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).
What happened to Thomas' suggestion to turn off instrumentation for
those files only when CONFIG_AMD_MEM_ENCRYPT=y?
Which is a variant of 2) above with ifdeffery.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 4/26/19 11:24 AM, Borislav Petkov wrote:
> On Fri, Apr 26, 2019 at 03:11:17PM +0000, Gary R Hook wrote:
>> 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).
>
> What happened to Thomas' suggestion to turn off instrumentation for
> those files only when CONFIG_AMD_MEM_ENCRYPT=y?
>
> Which is a variant of 2) above with ifdeffery.
>
Ah, very good. That one escaped my list.
Yes, option 4 would be a combination of using a local copy of strncmp()
and disabling instrumentation (KASAN, KCOV, whatever) for
arch/x86/lib/cmdline.c when SME is enabled.
I have any/all of these ready to repost as a version 2 offering. What
say you?
grh
On Mon, Apr 29, 2019 at 08:16:07PM +0000, Gary R Hook wrote:
> Yes, option 4 would be a combination of using a local copy of strncmp()
Why the local copy?
> and disabling instrumentation (KASAN, KCOV, whatever) for
> arch/x86/lib/cmdline.c when SME is enabled.
I think this should suffice. You only disable instrumentation when
CONFIG_AMD_MEM_ENCRYPT=y and not do any local copies but use the generic
functions.
Hmm.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 4/29/19 3:51 PM, Borislav Petkov wrote:
> [CAUTION: External Email]
>
> On Mon, Apr 29, 2019 at 08:16:07PM +0000, Gary R Hook wrote:
>> Yes, option 4 would be a combination of using a local copy of strncmp()
>
> Why the local copy?
Seemed suitable, since it's tiny. But I'm not married to the idea.
>
>> and disabling instrumentation (KASAN, KCOV, whatever) for
>> arch/x86/lib/cmdline.c when SME is enabled.
>
> I think this should suffice. You only disable instrumentation when
> CONFIG_AMD_MEM_ENCRYPT=y and not do any local copies but use the generic
> functions.
Okay, super. I'll post a v2 that does that.
Do we want to document the subordinate functions in their respective
source files so that a future editor would (hopefully) be made aware of
this use?
grh