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

[Jeremy Linton]

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@xxxxxxx>
> > > ---
> > >   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.