[RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[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.

Signed-off-by: Hongbo Yao <yaohongbo@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

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Mark Rutland]

[Adding Catalin and LAKML]

Mark.

On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:
> 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 <yaohongbo@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
> 

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Will Deacon]

On Tue, Mar 17, 2020 at 12:10:51PM +0000, Mark Rutland wrote:
> [Adding Catalin and LAKML]
> 
> Mark.
> 
> On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:
> > 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 <yaohongbo@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);
> >  }

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

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1897 bytes --]

On Tue, Mar 17, 2020 at 12:43:24PM +0000, Will Deacon wrote:
> On Tue, Mar 17, 2020 at 12:10:51PM +0000, Mark Rutland wrote:
> > On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:

> > > 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?

> 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;
>  		}
>  	}

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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Will Deacon]

On Tue, Mar 17, 2020 at 01:57:19PM +0000, Mark Brown wrote:
> On Tue, Mar 17, 2020 at 12:43:24PM +0000, Will Deacon wrote:
> > On Tue, Mar 17, 2020 at 12:10:51PM +0000, Mark Rutland wrote:
> > > On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:
> 
> > > > 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.

> > 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;
> >  		}
> >  	}
> 
> 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

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2294 bytes --]

On Tue, Mar 17, 2020 at 03:18:14PM +0000, Will Deacon wrote:
> On Tue, Mar 17, 2020 at 01:57:19PM +0000, Mark Brown wrote:
> > On Tue, Mar 17, 2020 at 12:43:24PM +0000, Will Deacon wrote:
> > > On Tue, Mar 17, 2020 at 12:10:51PM +0000, Mark Rutland wrote:
> > > > On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:

> > > > > -	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.

> > 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.

> 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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Will Deacon]

On Tue, Mar 17, 2020 at 04:36:38PM +0000, Mark Brown wrote:
> On Tue, Mar 17, 2020 at 03:18:14PM +0000, Will Deacon wrote:
> > On Tue, Mar 17, 2020 at 01:57:19PM +0000, Mark Brown wrote:
> > > On Tue, Mar 17, 2020 at 12:43:24PM +0000, Will Deacon wrote:
> > > > On Tue, Mar 17, 2020 at 12:10:51PM +0000, Mark Rutland wrote:
> > > > > On Tue, Mar 17, 2020 at 07:47:08PM +0800, Hongbo Yao wrote:
> 
> > > > > > -	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.
> 
> > > 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.
> 
> > 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)

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 886 bytes --]

On Tue, Mar 17, 2020 at 09:01:54PM +0000, Will Deacon wrote:
> On Tue, Mar 17, 2020 at 04:36:38PM +0000, Mark Brown wrote:

> > 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 :( :( :(

Tell me about it.

> 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?

I don't think I did *exactly* this but it's familiar yeah.  It looks
sensible and I can't think of any problems but that doesn't mean there
aren't any.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Will Deacon]

On Wed, Mar 18, 2020 at 11:32:17AM +0000, Mark Brown wrote:
> On Tue, Mar 17, 2020 at 09:01:54PM +0000, Will Deacon wrote:
> > On Tue, Mar 17, 2020 at 04:36:38PM +0000, Mark Brown wrote:
> 
> > > 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 :( :( :(
> 
> Tell me about it.
> 
> > 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?
> 
> I don't think I did *exactly* this but it's familiar yeah.  It looks
> sensible and I can't think of any problems but that doesn't mean there
> aren't any.

Well, thanks for having a look!

Hongbo -- please can you confirm that this fixes the problem that you are
seeing?

Will

Re: [RFC PATCH] arm64: fix the missing ktpi= cmdline check in arm64_kernel_unmapped_at_el0()

[Hongbo Yao]

On 2020/3/19 4:13, Will Deacon wrote:
> On Wed, Mar 18, 2020 at 11:32:17AM +0000, Mark Brown wrote:
>> On Tue, Mar 17, 2020 at 09:01:54PM +0000, Will Deacon wrote:
>>> On Tue, Mar 17, 2020 at 04:36:38PM +0000, Mark Brown wrote:
>>
>>>> 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 :( :( :(
>>
>> Tell me about it.
>>
>>> 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?
>>
>> I don't think I did *exactly* this but it's familiar yeah.  It looks
>> sensible and I can't think of any problems but that doesn't mean there
>> aren't any.
> 
> Well, thanks for having a look!
> 
> Hongbo -- please can you confirm that this fixes the problem that you are
> seeing?
Hi, Will,
Sorry for the late response.

This fixes the problem for me.
Tested-by: Hongbo Yao <yaohongbo@huawei.com>

Thanks,
Hongbo.

> Will
> 
> .
>