[PATCH v2 0/3] arm64 kpti fixes

[PATCH v2 0/3] arm64 kpti fixes

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

A couple of fixes and tweaks for KPTI - the first patch is a true
regression in v6.7, the others are minor tweaks.

v2:
- add two more tweaks
- use correct method to test system cap before alternatives have been
  applied
- move check into kpti_install_ng_mappings()

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Marc Zyngier <maz@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>

Ard Biesheuvel (3):
  arm64: Avoid enabling KPTI unnecessarily
  arm64: mm: Only map KPTI trampoline if it is going to be used
  arm64: Kconfig: drop KAISER reference from KPTI option description

 arch/arm64/Kconfig             | 2 +-
 arch/arm64/kernel/cpufeature.c | 4 ++++
 arch/arm64/mm/mmu.c            | 3 +++
 3 files changed, 8 insertions(+), 1 deletion(-)

-- 
2.43.0.rc1.413.gea7ed67945-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
in the use of non-global kernel mappings even on systems that have no
need for it, and even when KPTI has been disabled explicitly via the
command line.

Ensure that this only happens when we have decided (based on the
detected system-wide CPU features) that KPTI should be enabled.

Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/cpufeature.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 646591c67e7a..91d2d6714969 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
 
 static void __init kpti_install_ng_mappings(void)
 {
+	/* Check whether KPTI is going to be used */
+	if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
+		return;
+
 	/*
 	 * We don't need to rewrite the page-tables if either we've done
 	 * it already or we have KASLR enabled and therefore have not
-- 
2.43.0.rc1.413.gea7ed67945-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] arm64: mm: Only map KPTI trampoline if it is going to be used

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

Avoid creating the fixmap entries for the KPTI trampoline if KPTI is not
in use.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/mm/mmu.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
index 15f6347d23b6..6c8078916f5e 100644
--- a/arch/arm64/mm/mmu.c
+++ b/arch/arm64/mm/mmu.c
@@ -674,6 +674,9 @@ static int __init map_entry_trampoline(void)
 {
 	int i;
 
+	if (!arm64_kernel_unmapped_at_el0())
+		return 0;
+
 	pgprot_t prot = kernel_exec_prot();
 	phys_addr_t pa_start = __pa_symbol(__entry_tramp_text_start);
 
-- 
2.43.0.rc1.413.gea7ed67945-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] arm64: Kconfig: drop KAISER reference from KPTI option description

[Ard Biesheuvel]

From: Ard Biesheuvel <ardb@kernel.org>

KAISER is a reference to the KASLR hardening technique that already
existed before Meltdown happened, and by now, it is sufficiently obscure
that mentioning it does not actually clarify anything. So remove this
reference, and replace it with KPTI.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 7b071a00425d..b67e6934316f 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1549,7 +1549,7 @@ config ARCH_FORCE_MAX_ORDER
 	  Don't change if unsure.
 
 config UNMAP_KERNEL_AT_EL0
-	bool "Unmap kernel when running in userspace (aka \"KAISER\")" if EXPERT
+	bool "Unmap kernel when running in userspace (KPTI)" if EXPERT
 	default y
 	help
 	  Speculation attacks against some high-performance processors can
-- 
2.43.0.rc1.413.gea7ed67945-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] arm64 kpti fixes

[Mark Rutland]

On Mon, Nov 27, 2023 at 01:00:50PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> A couple of fixes and tweaks for KPTI - the first patch is a true
> regression in v6.7, the others are minor tweaks.
> 
> v2:
> - add two more tweaks
> - use correct method to test system cap before alternatives have been
>   applied
> - move check into kpti_install_ng_mappings()
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Marc Zyngier <maz@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>

Thanks for this! For the series:

Acked-by: Mark Rutland <mark.rutland@arm.com>

Mark.

> 
> Ard Biesheuvel (3):
>   arm64: Avoid enabling KPTI unnecessarily
>   arm64: mm: Only map KPTI trampoline if it is going to be used
>   arm64: Kconfig: drop KAISER reference from KPTI option description
> 
>  arch/arm64/Kconfig             | 2 +-
>  arch/arm64/kernel/cpufeature.c | 4 ++++
>  arch/arm64/mm/mmu.c            | 3 +++
>  3 files changed, 8 insertions(+), 1 deletion(-)
> 
> -- 
> 2.43.0.rc1.413.gea7ed67945-goog
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> in the use of non-global kernel mappings even on systems that have no
> need for it, and even when KPTI has been disabled explicitly via the
> command line.
> 
> Ensure that this only happens when we have decided (based on the
> detected system-wide CPU features) that KPTI should be enabled.
> 
> Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/cpufeature.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 646591c67e7a..91d2d6714969 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
>  
>  static void __init kpti_install_ng_mappings(void)
>  {
> +	/* Check whether KPTI is going to be used */
> +	if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> +		return;

Why can't you use arm64_kernel_unmapped_at_el0() here?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Ard Biesheuvel]

On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > From: Ard Biesheuvel <ardb@kernel.org>
> >
> > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > in the use of non-global kernel mappings even on systems that have no
> > need for it, and even when KPTI has been disabled explicitly via the
> > command line.
> >
> > Ensure that this only happens when we have decided (based on the
> > detected system-wide CPU features) that KPTI should be enabled.
> >
> > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  arch/arm64/kernel/cpufeature.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 646591c67e7a..91d2d6714969 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> >
> >  static void __init kpti_install_ng_mappings(void)
> >  {
> > +     /* Check whether KPTI is going to be used */
> > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > +             return;
>
> Why can't you use arm64_kernel_unmapped_at_el0() here?
>

Because it relies on alternatives patching, which hasn't occurred yet
at this point.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> >
> > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > From: Ard Biesheuvel <ardb@kernel.org>
> > >
> > > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > > in the use of non-global kernel mappings even on systems that have no
> > > need for it, and even when KPTI has been disabled explicitly via the
> > > command line.
> > >
> > > Ensure that this only happens when we have decided (based on the
> > > detected system-wide CPU features) that KPTI should be enabled.
> > >
> > > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > ---
> > >  arch/arm64/kernel/cpufeature.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index 646591c67e7a..91d2d6714969 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > >
> > >  static void __init kpti_install_ng_mappings(void)
> > >  {
> > > +     /* Check whether KPTI is going to be used */
> > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > +             return;
> >
> > Why can't you use arm64_kernel_unmapped_at_el0() here?
> >
> 
> Because it relies on alternatives patching, which hasn't occurred yet
> at this point.

Hmm. Keeping the determination of the capabilities separate from the
alternatives patching feels like it's asking for trouble given how
many of the boolean system_*() helpers in asm/cpufeature.h are using
the alternative_has_cap*() code.

Could we move the call to apply_alternatives_all() into
setup_system_features() and then you could do the kpti stuff after that?
I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
moving those later too, given how things like system_supports_sve() rely
on the alternatives as well.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Ard Biesheuvel]

On Mon, 27 Nov 2023 at 17:31, Will Deacon <will@kernel.org> wrote:
>
> On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> > On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > > > in the use of non-global kernel mappings even on systems that have no
> > > > need for it, and even when KPTI has been disabled explicitly via the
> > > > command line.
> > > >
> > > > Ensure that this only happens when we have decided (based on the
> > > > detected system-wide CPU features) that KPTI should be enabled.
> > > >
> > > > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  arch/arm64/kernel/cpufeature.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 646591c67e7a..91d2d6714969 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > > >
> > > >  static void __init kpti_install_ng_mappings(void)
> > > >  {
> > > > +     /* Check whether KPTI is going to be used */
> > > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > > +             return;
> > >
> > > Why can't you use arm64_kernel_unmapped_at_el0() here?
> > >
> >
> > Because it relies on alternatives patching, which hasn't occurred yet
> > at this point.
>
> Hmm. Keeping the determination of the capabilities separate from the
> alternatives patching feels like it's asking for trouble given how
> many of the boolean system_*() helpers in asm/cpufeature.h are using
> the alternative_has_cap*() code.
>
> Could we move the call to apply_alternatives_all() into
> setup_system_features() and then you could do the kpti stuff after that?
> I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
> moving those later too, given how things like system_supports_sve() rely
> on the alternatives as well.
>

AFAICT, just moving that call into setup_system_features() should work
fine, but I'll give Rutland the opportunity to chime in, as he is the
one who has been hacking on this recently.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Mon, Nov 27, 2023 at 04:31:03PM +0000, Will Deacon wrote:
> On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> > On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> > >
> > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > >
> > > > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > > > in the use of non-global kernel mappings even on systems that have no
> > > > need for it, and even when KPTI has been disabled explicitly via the
> > > > command line.
> > > >
> > > > Ensure that this only happens when we have decided (based on the
> > > > detected system-wide CPU features) that KPTI should be enabled.
> > > >
> > > > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > ---
> > > >  arch/arm64/kernel/cpufeature.c | 4 ++++
> > > >  1 file changed, 4 insertions(+)
> > > >
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 646591c67e7a..91d2d6714969 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > > >
> > > >  static void __init kpti_install_ng_mappings(void)
> > > >  {
> > > > +     /* Check whether KPTI is going to be used */
> > > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > > +             return;
> > >
> > > Why can't you use arm64_kernel_unmapped_at_el0() here?
> > >
> > 
> > Because it relies on alternatives patching, which hasn't occurred yet
> > at this point.
> 
> Hmm. Keeping the determination of the capabilities separate from the
> alternatives patching feels like it's asking for trouble given how
> many of the boolean system_*() helpers in asm/cpufeature.h are using
> the alternative_has_cap*() code.
> 
> Could we move the call to apply_alternatives_all() into
> setup_system_features() and then you could do the kpti stuff after that?
> I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
> moving those later too, given how things like system_supports_sve() rely
> on the alternatives as well.

Heh, so looking more closely at the history, I think I'm asking to undo
some of the recent changes from Mark :)

Mark -- looking at e.g. a76521d16028 ("arm64: Avoid cpus_have_const_cap()
for ARM64_{SVE,SME,SME2,FA64}") and 53d62e995d9e ("arm64: Avoid
cpus_have_const_cap() for ARM64_HAS_PAN"), why did you not just hoist
the alternatives patching slightly earlier instead of using
cpus_have_cap() directly?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Mark Rutland]

On Mon, Nov 27, 2023 at 04:41:27PM +0000, Will Deacon wrote:
> On Mon, Nov 27, 2023 at 04:31:03PM +0000, Will Deacon wrote:
> > On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> > > >
> > > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > > > From: Ard Biesheuvel <ardb@kernel.org>
> > > > >
> > > > > Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> > > > > in the use of non-global kernel mappings even on systems that have no
> > > > > need for it, and even when KPTI has been disabled explicitly via the
> > > > > command line.
> > > > >
> > > > > Ensure that this only happens when we have decided (based on the
> > > > > detected system-wide CPU features) that KPTI should be enabled.
> > > > >
> > > > > Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > > > > ---
> > > > >  arch/arm64/kernel/cpufeature.c | 4 ++++
> > > > >  1 file changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > > index 646591c67e7a..91d2d6714969 100644
> > > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > > > >
> > > > >  static void __init kpti_install_ng_mappings(void)
> > > > >  {
> > > > > +     /* Check whether KPTI is going to be used */
> > > > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > > > +             return;
> > > >
> > > > Why can't you use arm64_kernel_unmapped_at_el0() here?
> > > >
> > > 
> > > Because it relies on alternatives patching, which hasn't occurred yet
> > > at this point.
> > 
> > Hmm. Keeping the determination of the capabilities separate from the
> > alternatives patching feels like it's asking for trouble given how
> > many of the boolean system_*() helpers in asm/cpufeature.h are using
> > the alternative_has_cap*() code.
> > 
> > Could we move the call to apply_alternatives_all() into
> > setup_system_features() and then you could do the kpti stuff after that?
> > I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
> > moving those later too, given how things like system_supports_sve() rely
> > on the alternatives as well.
> 
> Heh, so looking more closely at the history, I think I'm asking to undo
> some of the recent changes from Mark :)
> 
> Mark -- looking at e.g. a76521d16028 ("arm64: Avoid cpus_have_const_cap()
> for ARM64_{SVE,SME,SME2,FA64}") and 53d62e995d9e ("arm64: Avoid
> cpus_have_const_cap() for ARM64_HAS_PAN"), why did you not just hoist
> the alternatives patching slightly earlier instead of using
> cpus_have_cap() directly?

There are a few reasons here. The overall point of those changes was to make
sure that patching the alternatives transitions the system into the patched
state atomically, and so that we don't have a weird transient state where
things can go wrong. My intent was that these initialization functions
initialize structures and state *before* patching the alternatives such that
once patched the system is immediately in a complete state. There are a few
other places that absolutely have to do this before patching, but for the cases
you mention above this isn't strictly necessary.

In addition to that intent, these functions are executed precisely once during
boot, so using cpus_have_cap() avoids the space for the alt_instr and the cost
of the patching of the site for a single check. I was also hoping to get rid of
most of the feature check helpers now that they're largely trivial wrappers
around alternative_has_cap(), which'd make it clearer which callsites care
about the alterntive-ness and clear up the inconsistent naming.

If you want these cases moves back to alternatives-based feature check helpers,
I can go do that.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Mon, Nov 27, 2023 at 05:49:21PM +0000, Mark Rutland wrote:
> On Mon, Nov 27, 2023 at 04:41:27PM +0000, Will Deacon wrote:
> > On Mon, Nov 27, 2023 at 04:31:03PM +0000, Will Deacon wrote:
> > > On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> > > > On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> > > > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > > > >
> > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > > > index 646591c67e7a..91d2d6714969 100644
> > > > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > > > > >
> > > > > >  static void __init kpti_install_ng_mappings(void)
> > > > > >  {
> > > > > > +     /* Check whether KPTI is going to be used */
> > > > > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > > > > +             return;
> > > > >
> > > > > Why can't you use arm64_kernel_unmapped_at_el0() here?
> > > > >
> > > > 
> > > > Because it relies on alternatives patching, which hasn't occurred yet
> > > > at this point.
> > > 
> > > Hmm. Keeping the determination of the capabilities separate from the
> > > alternatives patching feels like it's asking for trouble given how
> > > many of the boolean system_*() helpers in asm/cpufeature.h are using
> > > the alternative_has_cap*() code.
> > > 
> > > Could we move the call to apply_alternatives_all() into
> > > setup_system_features() and then you could do the kpti stuff after that?
> > > I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
> > > moving those later too, given how things like system_supports_sve() rely
> > > on the alternatives as well.
> > 
> > Heh, so looking more closely at the history, I think I'm asking to undo
> > some of the recent changes from Mark :)
> > 
> > Mark -- looking at e.g. a76521d16028 ("arm64: Avoid cpus_have_const_cap()
> > for ARM64_{SVE,SME,SME2,FA64}") and 53d62e995d9e ("arm64: Avoid
> > cpus_have_const_cap() for ARM64_HAS_PAN"), why did you not just hoist
> > the alternatives patching slightly earlier instead of using
> > cpus_have_cap() directly?
> 
> There are a few reasons here. The overall point of those changes was to make
> sure that patching the alternatives transitions the system into the patched
> state atomically, and so that we don't have a weird transient state where
> things can go wrong. My intent was that these initialization functions
> initialize structures and state *before* patching the alternatives such that
> once patched the system is immediately in a complete state. There are a few
> other places that absolutely have to do this before patching, but for the cases
> you mention above this isn't strictly necessary.

That all sounds reasonable, but we still have a weird period between the
call to update_cpu_capabilities(SCOPE_SYSTEM) in setup_system_features()
and the subsequent call to apply_alternatives_all() where code needs to
know not to inadvertently call something using alternative_has_cap()
otherwise it will silently get the wrong answer. I'm just saying we should
reduce that period as much as possible.

> In addition to that intent, these functions are executed precisely once during
> boot, so using cpus_have_cap() avoids the space for the alt_instr and the cost
> of the patching of the site for a single check. I was also hoping to get rid of
> most of the feature check helpers now that they're largely trivial wrappers
> around alternative_has_cap(), which'd make it clearer which callsites care
> about the alterntive-ness and clear up the inconsistent naming.

We can always remove the helpers when you get round to it, but ideally
many of these things would BUG() if they're called too early, much like
e.g. cpus_have_final_cap(). Inlining the alternative is going to make that
more difficult.

> If you want these cases moves back to alternatives-based feature check helpers,
> I can go do that.

I was thinking of something like the diff below to start with.

Will

--->8

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 646591c67e7a..0a5c23e449fe 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -3342,9 +3342,6 @@ void __init setup_system_features(void)
 	 * finalized. Finalize and log the available system capabilities.
 	 */
 	update_cpu_capabilities(SCOPE_SYSTEM);
-	if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) &&
-	    !cpus_have_cap(ARM64_HAS_PAN))
-		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
 
 	/*
 	 * Enable all the available capabilities which have not been enabled
@@ -3352,6 +3349,16 @@ void __init setup_system_features(void)
 	 */
 	enable_cpu_capabilities(SCOPE_ALL & ~SCOPE_BOOT_CPU);
 
+	/*
+	 * Patch in alternative instruction sequences now that the system-wide
+	 * CPU features have been enabled.
+	 */
+	apply_alternatives_all();
+
+
+	if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) && !system_uses_hw_pan())
+		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
+
 	kpti_install_ng_mappings();
 
 	sve_setup();
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index 1559c706d32d..bc9384517db3 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -1171,7 +1171,7 @@ void __init sve_setup(void)
 	unsigned long b;
 	int max_bit;
 
-	if (!cpus_have_cap(ARM64_SVE))
+	if (!system_supports_sve())
 		return;
 
 	/*
@@ -1301,7 +1301,7 @@ void __init sme_setup(void)
 	struct vl_info *info = &vl_info[ARM64_VEC_SME];
 	int min_bit, max_bit;
 
-	if (!cpus_have_cap(ARM64_SME))
+	if (!system_supports_sme())
 		return;
 
 	/*
diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
index defbab84e9e5..f59d6cea2187 100644
--- a/arch/arm64/kernel/smp.c
+++ b/arch/arm64/kernel/smp.c
@@ -441,7 +441,6 @@ void __init smp_cpus_done(unsigned int max_cpus)
 	pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());
 	setup_system_features();
 	hyp_mode_check();
-	apply_alternatives_all();
 	setup_user_features();
 	mark_linear_text_alias_ro();
 }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> Commit 42c5a3b04bf6 refactored the KPTI init code in a way that results
> in the use of non-global kernel mappings even on systems that have no
> need for it, and even when KPTI has been disabled explicitly via the
> command line.
> 
> Ensure that this only happens when we have decided (based on the
> detected system-wide CPU features) that KPTI should be enabled.
> 
> Fixes: 42c5a3b04bf6 ("arm64: Split kpti_install_ng_mappings()")
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/kernel/cpufeature.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 646591c67e7a..91d2d6714969 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
>  
>  static void __init kpti_install_ng_mappings(void)
>  {
> +	/* Check whether KPTI is going to be used */
> +	if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> +		return;
> +
>  	/*
>  	 * We don't need to rewrite the page-tables if either we've done
>  	 * it already or we have KASLR enabled and therefore have not

Since this is a fix:

Acked-by: Will Deacon <will@kernel.org>

and we can tidy up the caps thing as a follow-up.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Mark Rutland]

On Tue, Nov 28, 2023 at 11:03:40AM +0000, Will Deacon wrote:
> On Mon, Nov 27, 2023 at 05:49:21PM +0000, Mark Rutland wrote:
> > On Mon, Nov 27, 2023 at 04:41:27PM +0000, Will Deacon wrote:
> > > On Mon, Nov 27, 2023 at 04:31:03PM +0000, Will Deacon wrote:
> > > > On Mon, Nov 27, 2023 at 04:52:11PM +0100, Ard Biesheuvel wrote:
> > > > > On Mon, 27 Nov 2023 at 16:48, Will Deacon <will@kernel.org> wrote:
> > > > > > On Mon, Nov 27, 2023 at 01:00:51PM +0100, Ard Biesheuvel wrote:
> > > > > > >
> > > > > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > > > > index 646591c67e7a..91d2d6714969 100644
> > > > > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > > > > @@ -1839,6 +1839,10 @@ static int __init __kpti_install_ng_mappings(void *__unused)
> > > > > > >
> > > > > > >  static void __init kpti_install_ng_mappings(void)
> > > > > > >  {
> > > > > > > +     /* Check whether KPTI is going to be used */
> > > > > > > +     if (!cpus_have_cap(ARM64_UNMAP_KERNEL_AT_EL0))
> > > > > > > +             return;
> > > > > >
> > > > > > Why can't you use arm64_kernel_unmapped_at_el0() here?
> > > > > >
> > > > > 
> > > > > Because it relies on alternatives patching, which hasn't occurred yet
> > > > > at this point.
> > > > 
> > > > Hmm. Keeping the determination of the capabilities separate from the
> > > > alternatives patching feels like it's asking for trouble given how
> > > > many of the boolean system_*() helpers in asm/cpufeature.h are using
> > > > the alternative_has_cap*() code.
> > > > 
> > > > Could we move the call to apply_alternatives_all() into
> > > > setup_system_features() and then you could do the kpti stuff after that?
> > > > I think sve_setup() and sme_setup() are ok, but I'd be more comfortable
> > > > moving those later too, given how things like system_supports_sve() rely
> > > > on the alternatives as well.
> > > 
> > > Heh, so looking more closely at the history, I think I'm asking to undo
> > > some of the recent changes from Mark :)
> > > 
> > > Mark -- looking at e.g. a76521d16028 ("arm64: Avoid cpus_have_const_cap()
> > > for ARM64_{SVE,SME,SME2,FA64}") and 53d62e995d9e ("arm64: Avoid
> > > cpus_have_const_cap() for ARM64_HAS_PAN"), why did you not just hoist
> > > the alternatives patching slightly earlier instead of using
> > > cpus_have_cap() directly?
> > 
> > There are a few reasons here. The overall point of those changes was to make
> > sure that patching the alternatives transitions the system into the patched
> > state atomically, and so that we don't have a weird transient state where
> > things can go wrong. My intent was that these initialization functions
> > initialize structures and state *before* patching the alternatives such that
> > once patched the system is immediately in a complete state. There are a few
> > other places that absolutely have to do this before patching, but for the cases
> > you mention above this isn't strictly necessary.
> 
> That all sounds reasonable, but we still have a weird period between the
> call to update_cpu_capabilities(SCOPE_SYSTEM) in setup_system_features()
> and the subsequent call to apply_alternatives_all() where code needs to
> know not to inadvertently call something using alternative_has_cap()
> otherwise it will silently get the wrong answer. I'm just saying we should
> reduce that period as much as possible.

Sure. My thinking was that the window is small, and we're only using a few
__init functions there, but I agree that there's a risk, and I'm happy to go
rework as you say.

> > In addition to that intent, these functions are executed precisely once during
> > boot, so using cpus_have_cap() avoids the space for the alt_instr and the cost
> > of the patching of the site for a single check. I was also hoping to get rid of
> > most of the feature check helpers now that they're largely trivial wrappers
> > around alternative_has_cap(), which'd make it clearer which callsites care
> > about the alterntive-ness and clear up the inconsistent naming.
> 
> We can always remove the helpers when you get round to it, but ideally
> many of these things would BUG() if they're called too early, much like
> e.g. cpus_have_final_cap(). Inlining the alternative is going to make that
> more difficult.

That's fair.

My major goal is that it's clear where we're using an altnernative and where
we're not; if the thinking is the vast majority *should* be alternatives, I can
annotate the non-alternative cases to make that clear.

> > If you want these cases moves back to alternatives-based feature check helpers,
> > I can go do that.
> 
> I was thinking of something like the diff below to start with.

Sure; as-is that breaks the hyp offset patching, and we'll need ensure we still
call hyp_mode_check() before apply_alternatives_all(). In hyp_mode_check() we
call kvm_compute_layout(), which generates the 'va_mask', 'tag_val', and
'tag_lsb' values which are used by the kvm_update_va_mask() patching callback
(for kern_hyp_va() and __kern_hyp_va()).

I think that can be moved before enable_cpu_capabilities() ratehr than needing
to be between enable_cpu_capabilities() and apply_alternatives_all(), but I'll
need to go digging to make certain.

Do you want this as a cleanup for v6.7-rc*, or as something to queue for
v6.8-rc1?

Thanks,
Mark.

> 
> Will
> 
> --->8
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 646591c67e7a..0a5c23e449fe 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -3342,9 +3342,6 @@ void __init setup_system_features(void)
>  	 * finalized. Finalize and log the available system capabilities.
>  	 */
>  	update_cpu_capabilities(SCOPE_SYSTEM);
> -	if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) &&
> -	    !cpus_have_cap(ARM64_HAS_PAN))
> -		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
>  
>  	/*
>  	 * Enable all the available capabilities which have not been enabled
> @@ -3352,6 +3349,16 @@ void __init setup_system_features(void)
>  	 */
>  	enable_cpu_capabilities(SCOPE_ALL & ~SCOPE_BOOT_CPU);
>  
> +	/*
> +	 * Patch in alternative instruction sequences now that the system-wide
> +	 * CPU features have been enabled.
> +	 */
> +	apply_alternatives_all();
> +

> +
> +	if (IS_ENABLED(CONFIG_ARM64_SW_TTBR0_PAN) && !system_uses_hw_pan())
> +		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
> +
>  	kpti_install_ng_mappings();
>  
>  	sve_setup();
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index 1559c706d32d..bc9384517db3 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -1171,7 +1171,7 @@ void __init sve_setup(void)
>  	unsigned long b;
>  	int max_bit;
>  
> -	if (!cpus_have_cap(ARM64_SVE))
> +	if (!system_supports_sve())
>  		return;
>  
>  	/*
> @@ -1301,7 +1301,7 @@ void __init sme_setup(void)
>  	struct vl_info *info = &vl_info[ARM64_VEC_SME];
>  	int min_bit, max_bit;
>  
> -	if (!cpus_have_cap(ARM64_SME))
> +	if (!system_supports_sme())
>  		return;
>  
>  	/*
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index defbab84e9e5..f59d6cea2187 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -441,7 +441,6 @@ void __init smp_cpus_done(unsigned int max_cpus)
>  	pr_info("SMP: Total of %d processors activated.\n", num_online_cpus());
>  	setup_system_features();
>  	hyp_mode_check();
> -	apply_alternatives_all();
>  	setup_user_features();
>  	mark_linear_text_alias_ro();
>  }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (subset) [PATCH v2 0/3] arm64 kpti fixes

[Catalin Marinas]

On Mon, 27 Nov 2023 13:00:50 +0100, Ard Biesheuvel wrote:
> A couple of fixes and tweaks for KPTI - the first patch is a true
> regression in v6.7, the others are minor tweaks.
> 
> v2:
> - add two more tweaks
> - use correct method to test system cap before alternatives have been
>   applied
> - move check into kpti_install_ng_mappings()
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/3] arm64: Avoid enabling KPTI unnecessarily
      https://git.kernel.org/arm64/c/f5259997f3e8

I only picked up the first fix in the series, I'll leave the other two
for 6.8.

-- 
Catalin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Will Deacon]

On Tue, Nov 28, 2023 at 02:13:16PM +0000, Mark Rutland wrote:
> On Tue, Nov 28, 2023 at 11:03:40AM +0000, Will Deacon wrote:
> > On Mon, Nov 27, 2023 at 05:49:21PM +0000, Mark Rutland wrote:
> > > In addition to that intent, these functions are executed precisely once during
> > > boot, so using cpus_have_cap() avoids the space for the alt_instr and the cost
> > > of the patching of the site for a single check. I was also hoping to get rid of
> > > most of the feature check helpers now that they're largely trivial wrappers
> > > around alternative_has_cap(), which'd make it clearer which callsites care
> > > about the alterntive-ness and clear up the inconsistent naming.
> > 
> > We can always remove the helpers when you get round to it, but ideally
> > many of these things would BUG() if they're called too early, much like
> > e.g. cpus_have_final_cap(). Inlining the alternative is going to make that
> > more difficult.
> 
> That's fair.
> 
> My major goal is that it's clear where we're using an altnernative and where
> we're not; if the thinking is the vast majority *should* be alternatives, I can
> annotate the non-alternative cases to make that clear.
> 
> > > If you want these cases moves back to alternatives-based feature check helpers,
> > > I can go do that.
> > 
> > I was thinking of something like the diff below to start with.
> 
> Sure; as-is that breaks the hyp offset patching, and we'll need ensure we still
> call hyp_mode_check() before apply_alternatives_all(). In hyp_mode_check() we
> call kvm_compute_layout(), which generates the 'va_mask', 'tag_val', and
> 'tag_lsb' values which are used by the kvm_update_va_mask() patching callback
> (for kern_hyp_va() and __kern_hyp_va()).
> 
> I think that can be moved before enable_cpu_capabilities() ratehr than needing
> to be between enable_cpu_capabilities() and apply_alternatives_all(), but I'll
> need to go digging to make certain.
> 
> Do you want this as a cleanup for v6.7-rc*, or as something to queue for
> v6.8-rc1?

Sorry, I keep forgetting to reply to this after we spoke in-person. If
you're able to spin something in the next day or so, I can pick it up for
the next merge window. In the meantime, I'll grab Ard's changes so this can
go on top.

Cheers,

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/3] arm64 kpti fixes

[Will Deacon]

On Mon, 27 Nov 2023 13:00:50 +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> A couple of fixes and tweaks for KPTI - the first patch is a true
> regression in v6.7, the others are minor tweaks.
> 
> v2:
> - add two more tweaks
> - use correct method to test system cap before alternatives have been
>   applied
> - move check into kpti_install_ng_mappings()
> 
> [...]

Applied last two to arm64 (for-next/cpufeature), thanks!

[2/3] arm64: mm: Only map KPTI trampoline if it is going to be used
      https://git.kernel.org/arm64/c/8885c7398fe5
[3/3] arm64: Kconfig: drop KAISER reference from KPTI option description
      https://git.kernel.org/arm64/c/7540f70df98f

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] arm64: Avoid enabling KPTI unnecessarily

[Mark Rutland]

On Mon, Dec 11, 2023 at 11:39:39AM +0000, Will Deacon wrote:
> On Tue, Nov 28, 2023 at 02:13:16PM +0000, Mark Rutland wrote:
> > On Tue, Nov 28, 2023 at 11:03:40AM +0000, Will Deacon wrote:
> > > On Mon, Nov 27, 2023 at 05:49:21PM +0000, Mark Rutland wrote:
> > > > If you want these cases moves back to alternatives-based feature check helpers,
> > > > I can go do that.
> > > 
> > > I was thinking of something like the diff below to start with.
> > 
> > Sure; as-is that breaks the hyp offset patching, and we'll need ensure we still
> > call hyp_mode_check() before apply_alternatives_all(). In hyp_mode_check() we
> > call kvm_compute_layout(), which generates the 'va_mask', 'tag_val', and
> > 'tag_lsb' values which are used by the kvm_update_va_mask() patching callback
> > (for kern_hyp_va() and __kern_hyp_va()).
> > 
> > I think that can be moved before enable_cpu_capabilities() ratehr than needing
> > to be between enable_cpu_capabilities() and apply_alternatives_all(), but I'll
> > need to go digging to make certain.
> > 
> > Do you want this as a cleanup for v6.7-rc*, or as something to queue for
> > v6.8-rc1?
> 
> Sorry, I keep forgetting to reply to this after we spoke in-person. If
> you're able to spin something in the next day or so, I can pick it up for
> the next merge window. In the meantime, I'll grab Ard's changes so this can
> go on top.

I've sent that to:

  https://lore.kernel.org/linux-arm-kernel/20231212170910.3745497-1-mark.rutland@arm.com/

... based on the arm64 for-next/fixes branch.

Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel