Re: [PATCH 0/6] add system vulnerability sysfs entries

[Dave Martin]

On Wed, Dec 12, 2018 at 09:48:03AM -0600, Jeremy Linton wrote:
> Hi Dave,
> 
> Thanks for looking at this!
> 
> On 12/13/2018 06:07 AM, Dave Martin wrote:
> >On Thu, Dec 06, 2018 at 05:44:02PM -0600, Jeremy Linton wrote:
> >>Part of this series was originally by Mian Yousaf Kaukab.
> >>
> >>Arm64 machines should be displaying a human readable
> >>vulnerability status to speculative execution attacks in
> >>/sys/devices/system/cpu/vulnerabilities
> >
> >Is there any agreement on the strings that will be returned in there?
> >
> >A quick search didn't find anything obvious upstream.  There is
> >documentation proposed in [1], but I don't know what happened to it and
> >it doesn't define the mitigation strings at all.  (I didn't follow the
> >discussion, so there is likely background here I'm not aware of.)
> >
> >If the mitigation strings are meaningful at all, they really ought to be
> >documented somewhere since this is ABI.
> 
> I think they are in testing? But that documentation is missing the "Unknown"
> state which tends to be our default case for "we don't have a complete
> white/black list", or "mitigation disabled, we don't know if your
> vulnerable", etc.
> 
> I'm not sure I like the "Unknown" state, but we can try to add it to the
> documentation.
> 
> >
> >>This series enables that behavior by providing the expected
> >>functions. Those functions expose the cpu errata and feature
> >>states, as well as whether firmware is responding appropriately
> >>to display the overall machine status. This means that in a
> >>heterogeneous machine we will only claim the machine is mitigated
> >>or safe if we are confident all booted cores are safe or
> >>mitigated. Otherwise, we will display unknown or unsafe
> >>depending on how much of the machine configuration can
> >>be assured.
> >
> >Can the vulnerability status change once we enter userspace?
> 
> Generally no, for heterogeneous machines I think the answer here is yes, a
> user could check the state, and have it read "Not affected" then bring
> another core online which causes the state to change at which point if they

This feels like a potential bug, since userspace may already be making
assumptions about the vulnerability state by this point.

Shouldn't we reject late cpus that are noncompliant with the status quo
(as for unreconcilable cpu feature mismatches)?  I thought we already
did this for some mitigations.

> re-read /sys it may reflect another state. OTOH, given that we tend to
> default to mitigated usually this shouldn't be an issue unless someone has
> disabled the mitigation.
> 
> 
> >
> >I see no locking or other concurrency protections, and various global
> >variables that could be __ro_after_init if nothing will change them
> >after boot.
> 
> I think the state changes are all protected due to the fact the bringing a
> core online/offline is serialized.

Only with each other, not with random sysfs code running on another
online cpu.

If the concurrently accessed variables are only of fundamental integer
types there is unlikely to be a problem in practice, since transforming
a single, small isolated access into a memcpy() or similar (although
permitted by the language) is very unlikely to be a performance win --
so the compiler is unlikely to do it.

This is rather theoretical: if nobody else is bothering with atomics or
locking here, it may not be worth us adding anything specific just for
arm64.

[...]

Cheers
---Dave