Discussion:
[RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()
(too old to reply)
Hongbo Yao
2020-03-17 11:47:08 UTC
Permalink
Kpti cannot be disabled from the kernel cmdline after the commit
09e3c22a("arm64: Use a variable to store non-global mappings decision").

Bring back the missing check of kpti= command-line option to fix the
case where the SPE driver complains the missing "kpti-off" even it has
already been set.

Signed-off-by: Hongbo Yao <***@huawei.com>
---
arch/arm64/include/asm/mmu.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 3c9533322558..ebbc0d3ac2f7 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -34,7 +34,8 @@ extern bool arm64_use_ng_mappings;

static inline bool arm64_kernel_unmapped_at_el0(void)
{
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}

typedef void (*bp_hardening_cb_t)(void);
--
2.20.1
Mark Rutland
2020-03-17 12:10:51 UTC
Permalink
[Adding Catalin and LAKML]

Mark.
Post by Hongbo Yao
Kpti cannot be disabled from the kernel cmdline after the commit
09e3c22a("arm64: Use a variable to store non-global mappings decision").
Bring back the missing check of kpti= command-line option to fix the
case where the SPE driver complains the missing "kpti-off" even it has
already been set.
---
arch/arm64/include/asm/mmu.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 3c9533322558..ebbc0d3ac2f7 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -34,7 +34,8 @@ extern bool arm64_use_ng_mappings;
static inline bool arm64_kernel_unmapped_at_el0(void)
{
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}
typedef void (*bp_hardening_cb_t)(void);
--
2.20.1
Will Deacon
2020-03-17 12:43:24 UTC
Permalink
Post by Mark Rutland
[Adding Catalin and LAKML]
Mark.
Post by Hongbo Yao
Kpti cannot be disabled from the kernel cmdline after the commit
09e3c22a("arm64: Use a variable to store non-global mappings decision").
Bring back the missing check of kpti= command-line option to fix the
case where the SPE driver complains the missing "kpti-off" even it has
already been set.
---
arch/arm64/include/asm/mmu.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index 3c9533322558..ebbc0d3ac2f7 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -34,7 +34,8 @@ extern bool arm64_use_ng_mappings;
static inline bool arm64_kernel_unmapped_at_el0(void)
{
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}
This probably isn't the right fix, since this will mean that early mappings
will be global and we'll have to go through the painful page-table rewrite
logic when the cap gets enabled for KASLR-enabled kernels.

Maybe a better bodge is something like:

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0b6715625cf6..ad10f55b7bb9 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1085,6 +1085,8 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
if (!__kpti_forced) {
str = "KASLR";
__kpti_forced = 1;
+ } else if (__kpti_forced < 0) {
+ arm64_use_ng_mappings = false;
}
}


Will
Mark Brown
2020-03-17 13:57:19 UTC
Permalink
Post by Will Deacon
Post by Hongbo Yao
Kpti cannot be disabled from the kernel cmdline after the commit
09e3c22a("arm64: Use a variable to store non-global mappings decision").
Bring back the missing check of kpti= command-line option to fix the
case where the SPE driver complains the missing "kpti-off" even it has
already been set.
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}
This probably isn't the right fix, since this will mean that early mappings
will be global and we'll have to go through the painful page-table rewrite
logic when the cap gets enabled for KASLR-enabled kernels.
Aren't we looking for a rewrite from non-global to global here (disable
KPTI where we would otherwise have it), which we don't currently have
code for?
Post by Will Deacon
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0b6715625cf6..ad10f55b7bb9 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1085,6 +1085,8 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
if (!__kpti_forced) {
str = "KASLR";
__kpti_forced = 1;
+ } else if (__kpti_forced < 0) {
+ arm64_use_ng_mappings = false;
}
}
That is probably a good idea but I think that runs too late to affect
the early mappings, they're done based on kaslr_requires_kpti() well
before we start secondaries. My first pass not having paged everything
back in yet is that there needs to be command line parsing in
kaslr_requires_kpti() but as things stand the command line isn't
actually ready then...
Will Deacon
2020-03-17 15:18:14 UTC
Permalink
Post by Mark Brown
Post by Will Deacon
Post by Hongbo Yao
Kpti cannot be disabled from the kernel cmdline after the commit
09e3c22a("arm64: Use a variable to store non-global mappings decision").
Bring back the missing check of kpti= command-line option to fix the
case where the SPE driver complains the missing "kpti-off" even it has
already been set.
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}
This probably isn't the right fix, since this will mean that early mappings
will be global and we'll have to go through the painful page-table rewrite
logic when the cap gets enabled for KASLR-enabled kernels.
Aren't we looking for a rewrite from non-global to global here (disable
KPTI where we would otherwise have it), which we don't currently have
code for?
What I mean is that cpus_have_const_cap() will be false initially, so we'll
put down global mappings early on because PTE_MAYBE_NG will be 0, which
means that we'll have to invoke the rewriting code if we then realise we
want non-global mappings after the caps are finalised.
Post by Mark Brown
Post by Will Deacon
diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 0b6715625cf6..ad10f55b7bb9 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1085,6 +1085,8 @@ static bool unmap_kernel_at_el0(const struct arm64_cpu_capabilities *entry,
if (!__kpti_forced) {
str = "KASLR";
__kpti_forced = 1;
+ } else if (__kpti_forced < 0) {
+ arm64_use_ng_mappings = false;
}
}
That is probably a good idea but I think that runs too late to affect
the early mappings, they're done based on kaslr_requires_kpti() well
before we start secondaries. My first pass not having paged everything
back in yet is that there needs to be command line parsing in
kaslr_requires_kpti() but as things stand the command line isn't
actually ready then...
Yeah, and I think you probably run into chicken and egg problems mapping
the thing. With the change above, it's true that /some/ mappings will
still be nG if you pass kpti=off, but I was hoping that didn't really matter
:)

What was the behaviour prior to your patch? If it used to work without
any nG mappings, then I suppose we should try to restore that behaviour.

Will
Mark Brown
2020-03-17 16:36:38 UTC
Permalink
Post by Will Deacon
Post by Mark Brown
Post by Will Deacon
Post by Hongbo Yao
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
This probably isn't the right fix, since this will mean that early mappings
will be global and we'll have to go through the painful page-table rewrite
logic when the cap gets enabled for KASLR-enabled kernels.
Aren't we looking for a rewrite from non-global to global here (disable
KPTI where we would otherwise have it), which we don't currently have
code for?
What I mean is that cpus_have_const_cap() will be false initially, so we'll
put down global mappings early on because PTE_MAYBE_NG will be 0, which
means that we'll have to invoke the rewriting code if we then realise we
want non-global mappings after the caps are finalised.
Ah, I see - a different case to the one originally reported but also an
issue.
Post by Will Deacon
Post by Mark Brown
That is probably a good idea but I think that runs too late to affect
the early mappings, they're done based on kaslr_requires_kpti() well
before we start secondaries. My first pass not having paged everything
back in yet is that there needs to be command line parsing in
kaslr_requires_kpti() but as things stand the command line isn't
actually ready then...
Yeah, and I think you probably run into chicken and egg problems mapping
The whole area is just a mess.
Post by Will Deacon
the thing. With the change above, it's true that /some/ mappings will
still be nG if you pass kpti=off, but I was hoping that didn't really matter
:)
What was the behaviour prior to your patch? If it used to work without
any nG mappings, then I suppose we should try to restore that behaviour.
I'd need to go back and retest to confirm but it looks like always had
the issue that we'd install some nG mappings early even with KPTI
disabled on the command line so your change is just restoring the
previous behaviour and we're no worse than we were before.
Will Deacon
2020-03-17 21:01:54 UTC
Permalink
Post by Mark Brown
Post by Will Deacon
Post by Mark Brown
Post by Will Deacon
Post by Hongbo Yao
- return arm64_use_ng_mappings;
+ return arm64_use_ng_mappings &&
+ cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
This probably isn't the right fix, since this will mean that early mappings
will be global and we'll have to go through the painful page-table rewrite
logic when the cap gets enabled for KASLR-enabled kernels.
Aren't we looking for a rewrite from non-global to global here (disable
KPTI where we would otherwise have it), which we don't currently have
code for?
What I mean is that cpus_have_const_cap() will be false initially, so we'll
put down global mappings early on because PTE_MAYBE_NG will be 0, which
means that we'll have to invoke the rewriting code if we then realise we
want non-global mappings after the caps are finalised.
Ah, I see - a different case to the one originally reported but also an
issue.
Post by Will Deacon
Post by Mark Brown
That is probably a good idea but I think that runs too late to affect
the early mappings, they're done based on kaslr_requires_kpti() well
before we start secondaries. My first pass not having paged everything
back in yet is that there needs to be command line parsing in
kaslr_requires_kpti() but as things stand the command line isn't
actually ready then...
Yeah, and I think you probably run into chicken and egg problems mapping
The whole area is just a mess.
Post by Will Deacon
the thing. With the change above, it's true that /some/ mappings will
still be nG if you pass kpti=off, but I was hoping that didn't really matter
:)
What was the behaviour prior to your patch? If it used to work without
any nG mappings, then I suppose we should try to restore that behaviour.
I'd need to go back and retest to confirm but it looks like always had
the issue that we'd install some nG mappings early even with KPTI
disabled on the command line so your change is just restoring the
previous behaviour and we're no worse than we were before.
Urgh, this code brings back really bad memories :( :( :(

I just ran 5.4 and it looks like we leave everything non-global with KASLR,
even when "kpti=off". Great -- that means we're ok with my patch! Well, we
would be except that when we finalise the linear mapping we'll end up trying
to transition the old non-global entry to global, which is a break-before-make
violation (we panic early in __create_pgd_mapping()).

Staring more at the code, I think we're conflating the global/non-global
mappings with whether or not the kpti trampoline is active and it looks like
this might lead to other issues in mainline right now -- for example, I
don't think we clear TPIDRRO_EL0 properly for native tasks because the
arm64_kernel_unmapped_at_el0() check in tls_thread_switch() will defer the
zeroing to the trampoline code, but that might not even run!

So I've hacked the following, which appears to work but damn I'd like
somebody else to look at this. I also have a nagging feeling that you
implemented it like this at some point, but we tried to consolidate things
during review.

Thoughts?

Will

--->8

diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
index e4d862420bb4..d79ce6df9e12 100644
--- a/arch/arm64/include/asm/mmu.h
+++ b/arch/arm64/include/asm/mmu.h
@@ -29,11 +29,9 @@ typedef struct {
*/
#define ASID(mm) ((mm)->context.id.counter & 0xffff)

-extern bool arm64_use_ng_mappings;
-
static inline bool arm64_kernel_unmapped_at_el0(void)
{
- return arm64_use_ng_mappings;
+ return cpus_have_const_cap(ARM64_UNMAP_KERNEL_AT_EL0);
}

typedef void (*bp_hardening_cb_t)(void);
diff --git a/arch/arm64/include/asm/pgtable-prot.h b/arch/arm64/include/asm/pgtable-prot.h
index 6f87839f0249..1305e28225fc 100644
--- a/arch/arm64/include/asm/pgtable-prot.h
+++ b/arch/arm64/include/asm/pgtable-prot.h
@@ -23,11 +23,13 @@

#include <asm/pgtable-types.h>

+extern bool arm64_use_ng_mappings;
+
#define _PROT_DEFAULT (PTE_TYPE_PAGE | PTE_AF | PTE_SHARED)
#define _PROT_SECT_DEFAULT (PMD_TYPE_SECT | PMD_SECT_AF | PMD_SECT_S)

-#define PTE_MAYBE_NG (arm64_kernel_unmapped_at_el0() ? PTE_NG : 0)
-#define PMD_MAYBE_NG (arm64_kernel_unmapped_at_el0() ? PMD_SECT_NG : 0)
+#define PTE_MAYBE_NG (arm64_use_ng_mappings ? PTE_NG : 0)
+#define PMD_MAYBE_NG (arm64_use_ng_mappings ? PMD_SECT_NG : 0)

#define PROT_DEFAULT (_PROT_DEFAULT | PTE_MAYBE_NG)
#define PROT_SECT_DEFAULT (_PROT_SECT_DEFAULT | PMD_MAYBE_NG)

Loading...