Re: [PATCH 2/6] arm64: add sysfs vulnerability show for meltdown

[Jeremy Linton <jeremy.linton@arm.com>]

Hi Julien,

Thanks for taking a look at this!

On 12/13/2018 04:46 AM, Julien Thierry wrote:
> 
> 
> On 13/12/2018 09:23, Julien Thierry wrote:
>> Hi Jeremy,
>>
>> On 06/12/2018 23:44, Jeremy Linton wrote:
>>> Add a simple state machine which will track whether
>>> all the online cores in a machine are vulnerable.
>>>
>>> Once that is done we have a fairly authoritative view
>>> of the machine vulnerability, which allows us to make a
>>> judgment about machine safety if it hasn't been mitigated.
>>>
>>> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com>
>>> ---
>>>   arch/arm64/kernel/cpufeature.c | 31 ++++++++++++++++++++++++++-----
>>>   1 file changed, 26 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>> index 242898395f68..bea9adfef7fa 100644
>>> --- a/arch/arm64/kernel/cpufeature.c
>>> +++ b/arch/arm64/kernel/cpufeature.c
>>> @@ -905,6 +905,8 @@ has_useable_cnp(const struct arm64_cpu_capabilities *entry, int scope)
>>>   	return has_cpuid_feature(entry, scope);
>>>   }
>>>   
>>> +static enum { A64_MELT_UNSET, A64_MELT_SAFE, A64_MELT_UNKN } __meltdown_safe = A64_MELT_UNSET;
>>> +
>>
>> I'm wondering, do we really need that tri state?
>>
>> Can't we consider that we are safe an move to unsafe/unkown if any cpu
>> during bring up is not in the safe list?
>>
>> The only user of this is cpu_show_meltdown, but I don't imagine it'll
>> get called before unmap_kernel_at_el0() is called for the boot CPU which
>> should initialise that state.
>>
>> Or is there another reason for having that UNSET state?
>>
> 
> Ok, I think I get the point of the UNSET as #ifndef
> CONFIG_UNMAP_KERNEL_AT_EL0 we don't set the state. But does that mean we
> always fall in the "Unknown" case when we don't build kpti in? Is that
> desirable?
> 
> If so, I'd suggest replacing the tri-state with the following change:
> 
> 
>>> +
>>> +#ifdef CONFIG_GENERIC_CPU_VULNERABILITIES
>>> +ssize_t cpu_show_meltdown(struct device *dev, struct device_attribute *attr,
>>> +		char *buf)
>>> +{
>>> +	if (arm64_kernel_unmapped_at_el0())
>>> +		return sprintf(buf, "Mitigation: KPTI\n");
>>> +
> 
> 	if (!IS_ENABLED(UNMAP_KERNEL_AT_EL0) || !meltdown_safe)
> 		sprintf(buf, "Unknown\n");
> 	else
> 		sprintf(buf, "Not affected\n");

If I'm understanding what your suggesting:

Isn't this only checking the current core, rather than the whole 
machine? IIRC that was the fundamental complaint with the original set.