linux-kernel - Re: [PATCH] arm64: Run enable method for errata work arounds on late CPUs

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, 17 Jan 2018 13:43:35 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Suzuki K Poulose <Suzuki.Poulose@....com>,
        Dave Martin <Dave.Martin@....com>
Cc:     Mark Rutland <mark.rutland@....com>, catalin.marinas@....com,
        will.deacon@....com, linux-kernel@...r.kernel.org,
        Andre Przywara <andre.przywara@....com>,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] arm64: Run enable method for errata work arounds on late
 CPUs

On 17/01/18 13:31, Suzuki K Poulose wrote:
> On 17/01/18 13:20, Robin Murphy wrote:
>> On 17/01/18 12:25, Dave Martin wrote:
>>> On Wed, Jan 17, 2018 at 10:05:56AM +0000, Suzuki K Poulose wrote:
>>>> When a CPU is brought up after we have finalised the system
>>>> wide capabilities (i.e, features and errata), we make sure the
>>>> new CPU doesn't need a new errata work around which has not been
>>>> detected already. However we don't run enable() method on the new
>>>> CPU for the errata work arounds already detected. This could
>>>> cause the new CPU running without potential work arounds.
>>>> It is upto the "enable()" method to decide if this CPU should
>>>> do something about the errata.
>>>>
>>>> Fixes: commit 6a6efbb45b7d95c84 ("arm64: Verify CPU errata work 
>>>> arounds on hotplugged CPU")
>>>> Cc: Will Deacon <will.deacon@....com>
>>>> Cc: Mark Rutland <mark.rutland@....com>
>>>> Cc: Andre Przywara <andre.przywara@....com>
>>>> Cc: Catalin Marinas <catalin.marinas@....com>
>>>> Cc: Dave Martin <dave.martin@....com>
>>>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>>>> ---
>>>>   arch/arm64/kernel/cpu_errata.c | 9 ++++++---
>>>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpu_errata.c 
>>>> b/arch/arm64/kernel/cpu_errata.c
>>>> index 90a9e465339c..54e41dfe41f6 100644
>>>> --- a/arch/arm64/kernel/cpu_errata.c
>>>> +++ b/arch/arm64/kernel/cpu_errata.c
>>>> @@ -373,15 +373,18 @@ void verify_local_cpu_errata_workarounds(void)
>>>>   {
>>>>       const struct arm64_cpu_capabilities *caps = arm64_errata;
>>>> -    for (; caps->matches; caps++)
>>>> -        if (!cpus_have_cap(caps->capability) &&
>>>> -            caps->matches(caps, SCOPE_LOCAL_CPU)) {
>>>> +    for (; caps->matches; caps++) {
>>>> +        if (cpus_have_cap(caps->capability)) {
>>>> +            if (caps->enable)
>>>> +                caps->enable((void *)caps);
>>>
>>> Do we really need this cast?
>>
>> Seems to me like the prototype for .enable needs updating. If any 
>> existing callback was actually using the (non-const) void* for some 
>> purpose (thankfully nothing seems to be), then passing the capability 
>> pointer into that would be unlikely to end well anyway.
> 
> I agree. This was initially written such that we could call it via 
> on_each_cpu().
> But then we later switched to stop_machine(). And we weren't using the 
> argument until
> very recently with the introduction of multiple entries for the same 
> capability.
> 
> I will try to clean this up in a separate series, which would involve 
> cleaning up
> all the enable(), quite invasive. I would like this to go in for 4.16, 
> as it is
> needed for things like KPTI and some of the existing caps.

OK, sounds good. For the sake of the immediate fix, perhaps it's cleaner 
to just pass NULL here if the current callbacks ignore it?

Robin.