linux-kernel - Re: [PATCH 13/16] arm64: Add support for checking errata based on a list of MIDRS

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180130151644.GG5862@e103592.cambridge.arm.com>
Date:   Tue, 30 Jan 2018 15:16:44 +0000
From:   Dave Martin <Dave.Martin@....com>
To:     Suzuki K Poulose <Suzuki.Poulose@....com>
Cc:     mark.rutland@....com, ckadabi@...eaurora.org,
        ard.biesheuvel@...aro.org, marc.zyngier@....com,
        catalin.marinas@....com, will.deacon@....com,
        linux-kernel@...r.kernel.org, jnair@...iumnetworks.com,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 13/16] arm64: Add support for checking errata based on a
 list of MIDRS

On Fri, Jan 26, 2018 at 03:57:44PM +0000, Suzuki K Poulose wrote:
> On 26/01/18 14:16, Dave Martin wrote:
> >On Tue, Jan 23, 2018 at 12:28:06PM +0000, Suzuki K Poulose wrote:
> >>Add helpers for detecting an errata on list of midr ranges
> >>of affected CPUs.
> >
> >This doesn't describe what the patch does: instead, helpers are being
> >added for checking whether an MIDR falls in one of multiple affected
> >model(s) and or revision(s).
> >
> >Doing this makes sense, but is it really worth it?
> 
> Well, we need th MIDR list helpers anyway for other things:
>   - White list of CPUs where we know KPTI is not needed
>   - Black list of CPUs where DBM shouldn't be enabled.
> 
> So all we do is add a new type which could reduce the number of entries.
> 
> >
> >We might save 100-200 bytes in the kernel image for now, but a common
> >workaround for errata on multiple unrelated cpus is surely a rare case.
> >
> >Only if there are many such lists, or if the lists become large does
> >this start to seem a clear win.
> >
> >>
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
> >>---
> >>  arch/arm64/include/asm/cpufeature.h |  1 +
> >>  arch/arm64/kernel/cpu_errata.c      | 40 ++++++++++++++++++++++---------------
> >>  2 files changed, 25 insertions(+), 16 deletions(-)
> >>
> >>diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> >>index a3d54c2c411f..70712de687c7 100644
> >>--- a/arch/arm64/include/asm/cpufeature.h
> >>+++ b/arch/arm64/include/asm/cpufeature.h
> >
> >[...]
> >
> >>@@ -330,22 +353,7 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
> >>  #ifdef CONFIG_HARDEN_BRANCH_PREDICTOR
> >>  	{
> >>  		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,
> >>-		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A57),
> >>-		.enable = enable_psci_bp_hardening,
> >>-	},
> >>-	{
> >>-		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,
> >>-		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A72),
> >>-		.enable = enable_psci_bp_hardening,
> >>-	},
> >>-	{
> >>-		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,
> >>-		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A73),
> >>-		.enable = enable_psci_bp_hardening,
> >>-	},
> >>-	{
> >>-		.capability = ARM64_HARDEN_BRANCH_PREDICTOR,
> >>-		ERRATA_MIDR_ALL_VERSIONS(MIDR_CORTEX_A75),
> >>+		ERRATA_MIDR_RANGE_LIST(cortex_bp_harden_cpus),
> >
> >Could we just use a macro to generate multiple structs, instead of
> >inventing a new type of struct?
> 
> We could. Somehow, I don't think we are over engineering much here.

There is a flipside to this: I commented elsewhere that not allowing
mutiple match criteria per capability struct complicates verification
for late CPUs and/or makes it more costly.

Your changes here do implement support for multiple match criteria,
albeit only for the specific case of MIDR matching.

It could be worth generalising this in the future, but that's
probably not for this series.

OTOH, if MIDR matching is the only scenario where we have duplicate
cap structs with different match criteria and this patch allows all
those duplicates to be removed, then is there still a need to walk
the whole list in verify_local_cpu_features(), as introduced in
67948af41f2e ("arm64: capabilities: Handle duplicate entries for a
capability")?  Or can that now be simplified?

Cheers
---Dave