linux-kernel - Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods

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]

Date:   Wed, 29 Mar 2017 16:27:23 +0200
From:   Daniel Lezcano <daniel.lezcano@...aro.org>
To:     Marc Zyngier <marc.zyngier@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will.deacon@....com>,
        Scott Wood <oss@...error.net>,
        Hanjun Guo <hanjun.guo@...aro.org>,
        Ding Tianhong <dingtianhong@...wei.com>,
        dann frazier <dann.frazier@...onical.com>
Subject: Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for
 multiple erratum detection methods

On Tue, Mar 28, 2017 at 04:38:41PM +0100, Marc Zyngier wrote:
> On 28/03/17 15:55, Daniel Lezcano wrote:
> > On Tue, Mar 28, 2017 at 03:48:23PM +0100, Marc Zyngier wrote:
> >> On 28/03/17 15:36, Daniel Lezcano wrote:
> >>> On Tue, Mar 28, 2017 at 03:07:52PM +0100, Marc Zyngier wrote:
> >>>
> >>> [ ... ]
> >>>
> >>>>>>> -bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
> >>>>>>> -					 const void *arg)
> >>>>>>> +bool arch_timer_check_cap_erratum(const struct arch_timer_erratum_workaround *wa,
> >>>>>>> +				  const void *arg)
> >>>>>>>  {
> >>>>>>> -	return cpus_have_cap((uintptr_t)wa->id);
> >>>>>>> +	return cpus_have_cap((uintptr_t)wa->id) | this_cpu_has_cap((uintptr_t)wa->id);
> >>>>>>
> >>>>>> Not quite. Here, you're making all capability-based errata to be be
> >>>>>> global (if a single CPU in the system has a capability, then by
> >>>>>> transitivity cpus_have_cap returns true). If that's a big-little system,
> >>>>>> you end-up applying the workaround to all CPUs, including those unaffected.
> >>>>>>
> >>>>>> I'd rather drop cpus_have_cap altogether and rely on individual CPU
> >>>>>> matching (since we don't have a need for a global capability erratum
> >>>>>> handling yet).
> >>>>>
> >>>>> Ok, thanks.
> >>>>
> >>>> Quick update. I've just implemented this, and found out that getting rid
> >>>> of local/global has an unfortunate effect:
> >>>>
> >>>> Since we only probe the global errata (using ACPI for example) on the
> >>>> boot CPU path, we lose propagation of the erratum across the secondary
> >>>> CPUs. One way of solving this is to convert the secondary boot path to
> >>>> be aware of DT vs ACPI vs detection method of the month. Which isn't
> >>>> easy, since by the time we boot secondary CPUs, we don't have the
> >>>> pointers to the various ACPI tables anymore. Also, assuming we were
> >>>> careful and saved the pointers, the tables may have been unmapped. Fun.
> >>>
> >>> My proposal was supposed to prevent that. The detecion is done in the
> >>> subsystems, ACPI detects ACPI errata, DT detects DT errata and CPU detects CPU
> >>> errata. The drivers get the errata and enable the workaround. The id
> >>> association <-> errata self contains errata types (void *, char *, int). So
> >>> everything can be done in a CPU basis without local / global dance.
> >>
> >> I'm sorry, but it feels like a Jumbo-Jet sized hammer to try and squash
> >> a fly (I'm staying away from the frozen shark metaphor here). You're
> >> willing to add a whole list of things with private ids that need
> >> matching to kill a flag? I don't think this buys us anything but extra
> >> complexity and another maintenance headache.
> > 
> > Well, it is like your approach except it is split in two steps.
> > 
> > Can you explain where is the extra complexity ? May be I am missing the point.
> 
> This is how I understand your approach:
> 
> - Boot the first CPU
> - Build a list of errata discovered at that time
> - Apply erratum on the boot CPU if required, using a yet-to-be-invented
> private id matching mechanism,
> - Boot a secondary CPU
> - Apply erratum if required, parsing the list
> - Realise that you don't have the full list (this CPU comes with an
> erratum that was not in the initial list)
> - Add more to the list
> - Apply erratum, using the same matching mechanism
> 
> This is mine:
> 
> - Boot the first CPU
> - Apply global erratum to all CPUs
> - Apply local erratum
> - Boot a secondary CPU
> - Apply local erratum
> 
> In my case, everything is static, and I don't need to rematch each CPU
> against the list of globally applicable errata.
> 
> If my understanding is flawed, let me know.

Any of our understanding is flawed. I think that needs a maturation period.