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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Feb 05, 2021 at 11:32:07AM +0000, 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.

I think I'm likely missing part of the point here - Jan's patch would
just return 0 for reads, so there's no leak of unhandled MSRs? Hence
I'm not seeing the XSA aspect of this.

Roger.