diff mbox

arm64: cpufeature: Trim feature reporting and include PAN emulation

Message ID 20180220224624.GA28218@beast (mailing list archive)
State New, archived
Headers show

Commit Message

Kees Cook Feb. 20, 2018, 10:46 p.m. UTC
The PAN emulation notification was only happening for non-boot CPUs
if CPU capabilities had already been configured. This seems to be the
wrong place, as it's system-wide and isn't attached to capabilities,
so its reporting didn't normally happen. Instead, report it once from
the boot CPU. Additionally removes the redundant "feature" word from the
"CPU features:" line.

Before (redundant "feature", and missing PAN emulation report):

 SMP: Total of 4 processors activated.
 CPU features: detected feature: 32-bit EL0 Support
 CPU features: detected feature: Kernel page table isolation (KPTI)
 CPU: All CPU(s) started at EL2

After:

 SMP: Total of 4 processors activated.
 CPU features: detected: 32-bit EL0 Support
 CPU features: detected: Kernel page table isolation (KPTI)
 CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching
 CPU: All CPU(s) started at EL2

Signed-off-by: Kees Cook <keescook@chromium.org>
---
 arch/arm64/kernel/cpufeature.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Dave Martin Feb. 21, 2018, 11:18 a.m. UTC | #1
On Tue, Feb 20, 2018 at 02:46:24PM -0800, Kees Cook wrote:
> The PAN emulation notification was only happening for non-boot CPUs
> if CPU capabilities had already been configured. This seems to be the
> wrong place, as it's system-wide and isn't attached to capabilities,
> so its reporting didn't normally happen. Instead, report it once from
> the boot CPU. Additionally removes the redundant "feature" word from the
> "CPU features:" line.
> 
> Before (redundant "feature", and missing PAN emulation report):
> 
>  SMP: Total of 4 processors activated.
>  CPU features: detected feature: 32-bit EL0 Support
>  CPU features: detected feature: Kernel page table isolation (KPTI)
>  CPU: All CPU(s) started at EL2
> 
> After:
> 
>  SMP: Total of 4 processors activated.
>  CPU features: detected: 32-bit EL0 Support
>  CPU features: detected: Kernel page table isolation (KPTI)
>  CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching
>  CPU: All CPU(s) started at EL2
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  arch/arm64/kernel/cpufeature.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 29b1f873e337..6c799ca58b53 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1333,9 +1333,6 @@ static void verify_local_cpu_capabilities(void)
>  
>  	if (system_supports_sve())
>  		verify_sve_features();
> -
> -	if (system_uses_ttbr0_pan())
> -		pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
>  }
>  
>  void check_local_cpu_capabilities(void)
> @@ -1360,7 +1357,7 @@ void check_local_cpu_capabilities(void)
>  
>  static void __init setup_feature_capabilities(void)
>  {
> -	update_cpu_capabilities(arm64_features, "detected feature:");
> +	update_cpu_capabilities(arm64_features, "detected:");

Although I get what you're saying about redundant use of the word
"features", this feels like cosmetic churn that is unrelated to the
problem this patch is addressing.

It could be worth reviewing the CPU errata messages and other
miscellaneous printks together to make them less verbose and more
consistent all in one go, but that would be a separate patch...

>  	enable_cpu_capabilities(arm64_features);
>  }
>  
> @@ -1394,6 +1391,9 @@ void __init setup_cpu_features(void)
>  	if (system_supports_32bit_el0())
>  		setup_elf_hwcaps(compat_elf_hwcaps);
>  
> +	if (system_uses_ttbr0_pan())
> +		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
> +

Moving this seems sensible.  The other option would be to paste it into
update_cpu_capabilities(), but the message would still potentially get
printed multiple times, so that doesn't feel like the right approach.

[...]

Cheers
---Dave
Mark Rutland Feb. 21, 2018, 2:39 p.m. UTC | #2
On Wed, Feb 21, 2018 at 11:18:27AM +0000, Dave Martin wrote:
> On Tue, Feb 20, 2018 at 02:46:24PM -0800, Kees Cook wrote:
> > The PAN emulation notification was only happening for non-boot CPUs
> > if CPU capabilities had already been configured. This seems to be the
> > wrong place, as it's system-wide and isn't attached to capabilities,
> > so its reporting didn't normally happen. Instead, report it once from
> > the boot CPU. Additionally removes the redundant "feature" word from the
> > "CPU features:" line.
> > 
> > Before (redundant "feature", and missing PAN emulation report):
> > 
> >  SMP: Total of 4 processors activated.
> >  CPU features: detected feature: 32-bit EL0 Support
> >  CPU features: detected feature: Kernel page table isolation (KPTI)
> >  CPU: All CPU(s) started at EL2
> > 
> > After:
> > 
> >  SMP: Total of 4 processors activated.
> >  CPU features: detected: 32-bit EL0 Support
> >  CPU features: detected: Kernel page table isolation (KPTI)
> >  CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching
> >  CPU: All CPU(s) started at EL2
> > 
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > ---
> >  arch/arm64/kernel/cpufeature.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index 29b1f873e337..6c799ca58b53 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -1333,9 +1333,6 @@ static void verify_local_cpu_capabilities(void)
> >  
> >  	if (system_supports_sve())
> >  		verify_sve_features();
> > -
> > -	if (system_uses_ttbr0_pan())
> > -		pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
> >  }
> >  
> >  void check_local_cpu_capabilities(void)
> > @@ -1360,7 +1357,7 @@ void check_local_cpu_capabilities(void)
> >  
> >  static void __init setup_feature_capabilities(void)
> >  {
> > -	update_cpu_capabilities(arm64_features, "detected feature:");
> > +	update_cpu_capabilities(arm64_features, "detected:");
> 
> Although I get what you're saying about redundant use of the word
> "features", this feels like cosmetic churn that is unrelated to the
> problem this patch is addressing.

Given it seems sensible, shall we just split that into a separate patch? 

FWIW, for a patch with just this change:

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

> It could be worth reviewing the CPU errata messages and other
> miscellaneous printks together to make them less verbose and more
> consistent all in one go, but that would be a separate patch...
> 
> >  	enable_cpu_capabilities(arm64_features);
> >  }
> >  
> > @@ -1394,6 +1391,9 @@ void __init setup_cpu_features(void)
> >  	if (system_supports_32bit_el0())
> >  		setup_elf_hwcaps(compat_elf_hwcaps);
> >  
> > +	if (system_uses_ttbr0_pan())
> > +		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
> > +
> 
> Moving this seems sensible.  The other option would be to paste it into
> update_cpu_capabilities(), but the message would still potentially get
> printed multiple times, so that doesn't feel like the right approach.

I think that more ideally, we'd give this an entry in the arm64_features array,
but because it's effectively a negative feature, it's a little tricky.

This also looks fine to me, so FWIW:

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

Mark.
Dave Martin Feb. 21, 2018, 4:26 p.m. UTC | #3
On Wed, Feb 21, 2018 at 02:39:55PM +0000, Mark Rutland wrote:
> On Wed, Feb 21, 2018 at 11:18:27AM +0000, Dave Martin wrote:
> > On Tue, Feb 20, 2018 at 02:46:24PM -0800, Kees Cook wrote:
> > > The PAN emulation notification was only happening for non-boot CPUs
> > > if CPU capabilities had already been configured. This seems to be the
> > > wrong place, as it's system-wide and isn't attached to capabilities,
> > > so its reporting didn't normally happen. Instead, report it once from
> > > the boot CPU. Additionally removes the redundant "feature" word from the
> > > "CPU features:" line.
> > > 
> > > Before (redundant "feature", and missing PAN emulation report):
> > > 
> > >  SMP: Total of 4 processors activated.
> > >  CPU features: detected feature: 32-bit EL0 Support
> > >  CPU features: detected feature: Kernel page table isolation (KPTI)
> > >  CPU: All CPU(s) started at EL2
> > > 
> > > After:
> > > 
> > >  SMP: Total of 4 processors activated.
> > >  CPU features: detected: 32-bit EL0 Support
> > >  CPU features: detected: Kernel page table isolation (KPTI)
> > >  CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching
> > >  CPU: All CPU(s) started at EL2
> > > 
> > > Signed-off-by: Kees Cook <keescook@chromium.org>
> > > ---
> > >  arch/arm64/kernel/cpufeature.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index 29b1f873e337..6c799ca58b53 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -1333,9 +1333,6 @@ static void verify_local_cpu_capabilities(void)
> > >  
> > >  	if (system_supports_sve())
> > >  		verify_sve_features();
> > > -
> > > -	if (system_uses_ttbr0_pan())
> > > -		pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
> > >  }
> > >  
> > >  void check_local_cpu_capabilities(void)
> > > @@ -1360,7 +1357,7 @@ void check_local_cpu_capabilities(void)
> > >  
> > >  static void __init setup_feature_capabilities(void)
> > >  {
> > > -	update_cpu_capabilities(arm64_features, "detected feature:");
> > > +	update_cpu_capabilities(arm64_features, "detected:");
> > 
> > Although I get what you're saying about redundant use of the word
> > "features", this feels like cosmetic churn that is unrelated to the
> > problem this patch is addressing.
> 
> Given it seems sensible, shall we just split that into a separate patch? 

Can do, though I still think it's a bit dubious.  The original message
as it stands isn't wrong.  I won't lose sleep over it though.

[...]

Cheers
---Dave
Kees Cook Feb. 21, 2018, 6:04 p.m. UTC | #4
On Wed, Feb 21, 2018 at 6:39 AM, Mark Rutland <mark.rutland@arm.com> wrote:
> On Wed, Feb 21, 2018 at 11:18:27AM +0000, Dave Martin wrote:
>> On Tue, Feb 20, 2018 at 02:46:24PM -0800, Kees Cook wrote:
>> > The PAN emulation notification was only happening for non-boot CPUs
>> > if CPU capabilities had already been configured. This seems to be the
>> > wrong place, as it's system-wide and isn't attached to capabilities,
>> > so its reporting didn't normally happen. Instead, report it once from
>> > the boot CPU. Additionally removes the redundant "feature" word from the
>> > "CPU features:" line.
>> >
>> > Before (redundant "feature", and missing PAN emulation report):
>> >
>> >  SMP: Total of 4 processors activated.
>> >  CPU features: detected feature: 32-bit EL0 Support
>> >  CPU features: detected feature: Kernel page table isolation (KPTI)
>> >  CPU: All CPU(s) started at EL2
>> >
>> > After:
>> >
>> >  SMP: Total of 4 processors activated.
>> >  CPU features: detected: 32-bit EL0 Support
>> >  CPU features: detected: Kernel page table isolation (KPTI)
>> >  CPU features: emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching
>> >  CPU: All CPU(s) started at EL2
>> >
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > ---
>> >  arch/arm64/kernel/cpufeature.c | 8 ++++----
>> >  1 file changed, 4 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> > index 29b1f873e337..6c799ca58b53 100644
>> > --- a/arch/arm64/kernel/cpufeature.c
>> > +++ b/arch/arm64/kernel/cpufeature.c
>> > @@ -1333,9 +1333,6 @@ static void verify_local_cpu_capabilities(void)
>> >
>> >     if (system_supports_sve())
>> >             verify_sve_features();
>> > -
>> > -   if (system_uses_ttbr0_pan())
>> > -           pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
>> >  }
>> >
>> >  void check_local_cpu_capabilities(void)
>> > @@ -1360,7 +1357,7 @@ void check_local_cpu_capabilities(void)
>> >
>> >  static void __init setup_feature_capabilities(void)
>> >  {
>> > -   update_cpu_capabilities(arm64_features, "detected feature:");
>> > +   update_cpu_capabilities(arm64_features, "detected:");
>>
>> Although I get what you're saying about redundant use of the word
>> "features", this feels like cosmetic churn that is unrelated to the
>> problem this patch is addressing.
>
> Given it seems sensible, shall we just split that into a separate patch?
>
> FWIW, for a patch with just this change:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Sure!

>> It could be worth reviewing the CPU errata messages and other
>> miscellaneous printks together to make them less verbose and more
>> consistent all in one go, but that would be a separate patch...
>>
>> >     enable_cpu_capabilities(arm64_features);
>> >  }
>> >
>> > @@ -1394,6 +1391,9 @@ void __init setup_cpu_features(void)
>> >     if (system_supports_32bit_el0())
>> >             setup_elf_hwcaps(compat_elf_hwcaps);
>> >
>> > +   if (system_uses_ttbr0_pan())
>> > +           pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
>> > +
>>
>> Moving this seems sensible.  The other option would be to paste it into
>> update_cpu_capabilities(), but the message would still potentially get
>> printed multiple times, so that doesn't feel like the right approach.
>
> I think that more ideally, we'd give this an entry in the arm64_features array,
> but because it's effectively a negative feature, it's a little tricky.

Yeah, I looked at that but it seemed like it would needlessly burn a
capability bit for no real gain.

> This also looks fine to me, so FWIW:
>
> Acked-by: Mark Rutland <mark.rutland@arm.com>

Thanks, I'll split the patches!

-Kees
diff mbox

Patch

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index 29b1f873e337..6c799ca58b53 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -1333,9 +1333,6 @@  static void verify_local_cpu_capabilities(void)
 
 	if (system_supports_sve())
 		verify_sve_features();
-
-	if (system_uses_ttbr0_pan())
-		pr_info("Emulating Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
 }
 
 void check_local_cpu_capabilities(void)
@@ -1360,7 +1357,7 @@  void check_local_cpu_capabilities(void)
 
 static void __init setup_feature_capabilities(void)
 {
-	update_cpu_capabilities(arm64_features, "detected feature:");
+	update_cpu_capabilities(arm64_features, "detected:");
 	enable_cpu_capabilities(arm64_features);
 }
 
@@ -1394,6 +1391,9 @@  void __init setup_cpu_features(void)
 	if (system_supports_32bit_el0())
 		setup_elf_hwcaps(compat_elf_hwcaps);
 
+	if (system_uses_ttbr0_pan())
+		pr_info("emulated: Privileged Access Never (PAN) using TTBR0_EL1 switching\n");
+
 	sve_setup();
 
 	/* Advertise that we have computed the system capabilities */