Re: Ping²: [PATCH] x86/PV: conditionally avoid raising #GP for early guest MSR accesses

[Jürgen Groß <jgross@xxxxxxxx>]

On 05.02.21 12:32, Andrew Cooper wrote:
> On 05/02/2021 10:56, Jürgen Groß wrote:
> > On 05.02.21 11:14, Jan Beulich wrote:
> > > (simply re-sending what was sent over 2 months ago)
> > >
> > > On 04.11.2020 11:50, Jan Beulich wrote:
> > > > On 03.11.2020 18:31, Andrew Cooper wrote:
> > > > > On 03/11/2020 17:06, Jan Beulich wrote:
> > > > > > Prior to 4.15 Linux, when running in PV mode, did not install a #GP
> > > > > > handler early enough to cover for example the rdmsrl_safe() of
> > > > > > MSR_K8_TSEG_ADDR in bsp_init_amd() (not to speak of the unguarded
> > > > > > read
> > > > > > of MSR_K7_HWCR later in the same function). The respective change
> > > > > > (42b3a4cb5609 "x86/xen: Support early interrupts in xen pv
> > > > > > guests") was
> > > > > > backported to 4.14, but no further - presumably since it wasn't
> > > > > > really
> > > > > > easy because of other dependencies.
> > > > > >
> > > > > > Therefore, to prevent our change in the handling of guest MSR
> > > > > > accesses
> > > > > > to render PV Linux 4.13 and older unusable on at least AMD
> > > > > > systems, make
> > > > > > the raising of #GP on these paths conditional upon the guest having
> > > > > > installed a handler. Producing zero for reads and discarding writes
> > > > > > isn't necessarily correct and may trip code trying to detect
> > > > > > presence of
> > > > > > MSRs early, but since such detection logic won't work without a #GP
> > > > > > handler anyway, this ought to be a fair workaround.
> > > > > >
> > > > > > Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > > > I appreciate that we probably have to do something, but I don't think
> > > > > this is a wise move.
> > > > I wouldn't call it wise either, but I'm afraid something along
> > > > these lines is necessary.
> > > >
> > > > > Linux is fundamentally buggy.  It is deliberately looking for a
> > > > > potential #GP fault given its use of rdmsrl_safe().  The reason
> > > > > this bug
> > > > > stayed hidden for so long was as a consequence of Xen's inappropriate
> > > > > MSR handling for guests, and the reasons for changing Xen's behaviour
> > > > > still stand.
> > > > I agree.
> > > >
> > > > > This change, in particular, does not apply to any explicitly handled
> > > > > MSRs, and therefore is not a comprehensive fix.
> > > > But it's intentional that this deals with the situation in a
> > > > generic way, not on a per-MSR basis. If we did as you suggest
> > > > further down, we'd have to audit all Linux versions up to 4.14
> > > > for similar issues with other MSRs. I don't think this would
> > > > be a practical thing to do, and I also don't think that leaving
> > > > things as they are until we have concrete reports of problems
> > > > is a viable option either.
> > > >
> > > > Adding explicit handling for the two offending MSRs (and any
> > > > possible further ones we discover) would imo only be to avoid
> > > > issuing the respective log messages.
> > > >
> > > > >     Nor is it robust to
> > > > > someone adding code to explicitly handling the impacted MSRs at a
> > > > > later
> > > > > date (which are are likely to need to do for HWCR), and which would
> > > > > reintroduce this failure to boot.
> > > > I'm afraid I don't understand. Looking at the two functions
> > > > the patch alters, only X86EMUL_OKAY is used in return statements
> > > > other than the final one. If this model is to be followed by
> > > > future additions (which I think it ought to be; perhaps we
> > > > should add comments to this effect), the code introduced here
> > > > will take care of the situation nevertheless.
> > > >
> > > > > We should have the impacted MSRs handled explicitly, with a note
> > > > > stating
> > > > > this was a bug in Linux 4.14 and older.  We already have workaround
> > > > > for
> > > > > similar bugs in Windows, and it also gives us a timeline to eventually
> > > > > removing support for obsolete workarounds, rather than having a
> > > > > "now and
> > > > > in the future, we'll explicitly tolerate broken PV behaviour for
> > > > > one bug
> > > > > back in ancient linux".
> > > > Comparing with Windows isn't very helpful; the patch here is
> > > > specifically about PV, and would help other OSes as well in
> > > > case they would have missed setting up exceptions early in
> > > > just the PV-on-Xen case. For the HVM case I'd indeed rather
> > > > see us go the route we've gone for Windows, if need be.
> > > As can be seen from this reply, we're not in agreement what to
> > > do here. But we need to do something. I'm not sure how to get
> > > unstuck discussions like this one ...
> > >
> > > Besides this suggestion of yours I also continue to have
> > > trouble seeing what good it will do to record an exception to
> > > inject into a guest when we know it didn't install a handler
> > > yet.
> > As we need to consider backports of processor bug mitigations
> > in old guests, too, I think we need to have a "catch-all"
> > fallback.
> >
> > Not being able to run an old updated guest until we add handling
> > for a new MSR isn't a viable option IMO.
> You're trading off against issuing XSAs for all the unknown quantities
> of sensitive in MSRs on all past and future platforms.  This has
> unbounded scope.
>
> Xen's previous behaviour was astoundingly stupid, and yes - we're
> playing more than a decades worth of catchup in one release cycle.
>
> I'll absolutely take "care/tweaks need to happen crossing the Xen
> 4.14=>4.15 boundary" over whack-a-mole for MSRs in the form of security
> advisories.
What about making this behavior controllable via a boot parameter?

Selecting that could result in a boot warning issued via warning_add().


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature