linux-kernel - Re: [PATCH v3 09/21] arm64: cpufeature: Add global feature override facility

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] [day] [month] [year] [list]

Date:   Tue, 12 Jan 2021 14:54:07 +0000
From:   Marc Zyngier <maz@...nel.org>
To:     Suzuki K Poulose <suzuki.poulose@....com>
Cc:     Catalin Marinas <catalin.marinas@....com>,
        linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
        linux-kernel@...r.kernel.org, Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        David Brazdil <dbrazdil@...gle.com>,
        Alexandru Elisei <alexandru.elisei@....com>,
        Ard Biesheuvel <ardb@...nel.org>,
        Jing Zhang <jingzhangos@...gle.com>,
        Ajay Patil <pajay@....qualcomm.com>,
        Prasad Sodagudi <psodagud@...eaurora.org>,
        Srinivas Ramana <sramana@...eaurora.org>,
        James Morse <james.morse@....com>,
        Julien Thierry <julien.thierry.kdev@...il.com>,
        kernel-team@...roid.com
Subject: Re: [PATCH v3 09/21] arm64: cpufeature: Add global feature override
 facility

On 2021-01-12 11:59, Suzuki K Poulose wrote:
> On 1/12/21 11:50 AM, Marc Zyngier wrote:
>> Hi Suzuki,
>> 
>> On 2021-01-12 09:17, Suzuki K Poulose wrote:
>>> Hi Marc,
>>> 
>>> On 1/11/21 7:48 PM, Marc Zyngier wrote:
>> 
>> [...]
>> 
>>>> diff --git a/arch/arm64/kernel/cpufeature.c 
>>>> b/arch/arm64/kernel/cpufeature.c
>>>> index 894af60b9669..00d99e593b65 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -774,6 +774,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, 
>>>> u64 new)
>>>>       u64 strict_mask = ~0x0ULL;
>>>>       u64 user_mask = 0;
>>>>       u64 valid_mask = 0;
>>>> +    u64 override_val = 0, override_mask = 0;
>>>> 
>>>>       const struct arm64_ftr_bits *ftrp;
>>>>       struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
>>>> @@ -781,9 +782,35 @@ static void __init init_cpu_ftr_reg(u32 
>>>> sys_reg, u64 new)
>>>>       if (!reg)
>>>>           return;
>>>> 
>>>> +    if (reg->override_mask && reg->override_val) {
>>>> +        override_mask = *reg->override_mask;
>>>> +        override_val = *reg->override_val;
>>>> +    }
>>>> +
>>>>       for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
>>>>           u64 ftr_mask = arm64_ftr_mask(ftrp);
>>>>           s64 ftr_new = arm64_ftr_value(ftrp, new);
>>>> +        s64 ftr_ovr = arm64_ftr_value(ftrp, override_val);
>>>> +
>>>> +        if ((ftr_mask & override_mask) == ftr_mask) {
>>>> +            if (ftr_ovr < ftr_new) {
>>> 
>>> Here we assume that all the features are FTR_LOWER_SAFE. We could
>>> probably use arm64_ftr_safe_value(ftrp, ftr_new, ftr_ovr) here ?
>>> That would cover us for both HIGHER_SAFE and LOWER_SAFE features.
>>> However that may be restrictive for FTR_EXACT, as we the safe
>>> value would be set to "ftr->safe_val". I guess that may be better
>>> than forcing to use an unsafe value for the boot CPU, which could
>>> anyway conflict with the other CPUs and eventually trigger the
>>> ftr alue to be safe_val.
>> 
>> I like the idea of using the helper, as it cleanups up the code a bit.
>> However, not being to set a feature to a certain value could be 
>> restrictive,
>> as in general, it means that we can only disable a feature and not 
>> adjust
>> its level of support.
>> 
>> Take PMUVER for example: with the helper, I can't override it from 
>> v8.4 to
>> v8.1. I can only go to v8.0.
> 
> My point is, we set this only for the "init" of cpu features. So, even 
> if we
> init to a custom , non-(default-safe) value, the secondary CPUs could 
> scream,
> and the system wide safe value could fall back to the "safe" value for
> EXACT features, no matter what you did to init it.

Right. So let's go with the safe value for EXACT features for now,
and let the override fail if that's not what the user asked for.

After all, there are only so many things we want to support as
an override, and in all the cases at hand, using the safe value
actually matches what we want to do.

We can always revisit this if and when we need a different behaviour.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...