| Message ID | 20180220224624.GA28218@beast (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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 --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 */
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(-)