[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

  • To: Jürgen Groß <jgross@xxxxxxxx>, Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Fri, 5 Feb 2021 11:32:07 +0000
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=kmQQmcdcYR1qlbvU5mwSsGaJFWNh/dzbjzITAV278Vw=; b=V+juD+e65ilVzdKKhoNxV0I6v6hif2aNtMviyDnM3y87qYg1osAKx+9p5WEbI+FSWUl3eLPIw3ip4NjnrTLDaEfLCvtBcCzwglnqXDzFYOzox76NyPmXyiU9OdoAoDNH07LIzaLeXYe0prwv9rEXFXZiL1U8fjvmC1K4cj8q6hNiW9IvmQNVRXSEm6RnCOoxOrNDAdlxktW4AiA6wEhjRMirkYnkjvE/fBuFv1Msl/2CT7DXaQfikJ+4rzwsLMYxIAqwXU1Iol/T5lmZdHBNYMxAxkJPd+wuiso9EYBIDpu97669Aj015gFgdhwQE7JTGF61pdGMWyQ5+8jdXGu4EQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=m9csYst7JAHauj+9taIQvKTKDRM3so++G34LjSoK7Ki22OUCVq8aLBwmhhhL4DKRVcXEhyRtP1O+Xr9JwackwweE4gH9Ht6OI259v63OWMPYFhBfAXrf/8VEcYzHOGKmaZAbSSHpl1tVvwRA7u+XK7RaLwcRkQQehTqg2CALbbf2BTHGb6Wjf96KbnEzTQNdbUoskFT74J/uTH00IqIy1KL0mlP/Gn3C5YpitAzldgaUyIoVEvohA/tGUiXD8+FzWTk4zYkjITsS7rrCJSLMkcTpAt6tgGzeeD3cwF0wBGqMKtKMA1kRaKpFME4ZMz61Zun3/9FyFyTOB967Wv2sMw==
  • Authentication-results: esa2.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>, "xen-devel@xxxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 05 Feb 2021 11:32:26 +0000
  • Ironport-sdr: 3sL55lhBnClkfU06ag6FaD7Iw2BBIHEewqjbgZtYxobP+SmNWPi8N6NrNVn78IacAodf/CVtuR mTg472yMK3u3xDJBJB5eCaYH+6rOChrh1KJ4wYSIlWGQdL0Fp3NtqKhGtXPtVicAA7nWKvEsSp hRrqkXRt4kVvKQGE1xr56NCvU2n3o8LjzDr+EKsrY5QsGitadDkec1OEjh1M9lcmgDoAlcgSGK 7D/ovTz2Ackoik2NrL1YzPNFKdyiUYqqi3EWznV0OiC49rS8F8XVAZ42HRzJuthGFg90bE1C19 qDc=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.