Re: [PATCH v3 1/2][4.15] x86/PV: conditionally avoid raising #GP for early guest MSR reads

From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Jan Beulich <jbeulich@suse.com>,
	"xen-devel@lists.xenproject.org" <xen-devel@lists.xenproject.org>
Cc: "Wei Liu" <wl@xen.org>, "Roger Pau Monné" <roger.pau@citrix.com>,
	"Ian Jackson" <iwj@xenproject.org>
Subject: Re: [PATCH v3 1/2][4.15] x86/PV: conditionally avoid raising #GP for early guest MSR reads
Date: Fri, 12 Mar 2021 11:19:12 +0000	[thread overview]
Message-ID: <7504b027-f106-33e1-214a-eecf8ea5ff5d@citrix.com> (raw)
In-Reply-To: <90f87aa8-09da-1453-bd82-c722465c2881@suse.com>

On 12/03/2021 07:54, 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 this paths conditional upon the guest having
> installed a handler, provided of course the MSR can be read in the first
> place (we would have raised #GP in that case even before). Producing
> zero for reads 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@suse.com>
> Release-Acked-by: Ian Jackson <iwj@xenproject.org>

I am still of the firm belief that this is the wrong course of action.

It is deliberately papering over error handling bugs which, in the
NetBSD case, literally created memory corruption scenarios.  (Yes - that
was a WRMSR not RDMSR, but the general point still stands, particularly
in light of your expectation to do the same to the WRMSR).

It is one thing to not realise that we have a trainwreck here.  Its
totally another to take wilful action to keep current and all future
guests broken in the same way.

The *only* case where it is acceptable to skip error handling is if the
VM admin has specifically signed their life away to say that they'll
accept the, now discovered, potential-memory-corrupion consequences.

Rogers patch already does this.

~Andrew