Re: [PATCH] x86/HVM: don't give the wrong impression of WRMSR succeeding

From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Kevin Tian <kevin.tian@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [PATCH] x86/HVM: don't give the wrong impression of WRMSR succeeding
Date: Tue, 27 Feb 2018 09:07:04 -0500	[thread overview]
Message-ID: <8d8038ba-a1fd-f182-b2e9-c74e4c4d9abb@oracle.com> (raw)
In-Reply-To: <5A95276C02000078001AC104@prv-mh.provo.novell.com>

On 02/27/2018 03:39 AM, Jan Beulich wrote:
>>>> On 23.02.18 at 08:55, <JBeulich@suse.com> wrote:
>>>>> On 22.02.18 at 23:16, <boris.ostrovsky@oracle.com> wrote:
>>> On 02/22/2018 10:44 AM, Jan Beulich wrote:
>>>>>>> On 22.02.18 at 15:53, <andrew.cooper3@citrix.com> wrote:
>>>>> On 22/02/18 13:44, Jan Beulich wrote:
>>>>>> ... for unknown MSRs: wrmsr_hypervisor_regs()'s comment clearly says
>>>>>> that the function returns 0 for unrecognized MSRs, so
>>>>>> {svm,vmx}_msr_write_intercept() should not convert this into success.
>>>>>>
>>>>>> At the time it went in, commit 013e34f5a6 ("x86: handle paged gfn in
>>>>>> wrmsr_hypervisor_regs") was probably okay, since prior to that the
>>>>>> return value wasn't checked at all. But that's not how we want things
>>>>>> to be handled nowadays.
>>>>>>
>>>>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>>>> I agree in principle, but this does have a large potential risk for
>>>>> guests.  Any unknown MSR which guests don't check for #GP faults from
>>>>> will now cause the guests to crash.
>>>>>
>>>>> That said, it is the correct direction to go long-term, and we've got to
>>>>> throw the switch some time, but I expect this will cause problems in the
>>>>> short term, especially for migrated-in guests.
>>>> Thinking about this again, the RDMSR side of things already raises
>>>> #GP for inaccessible MSRs. We obviously can't do a probing WRMSR
>>>> in {svm,vmx}_msr_write_intercept(), but couldn't we rdmsr_safe()
>>>> in the "case 0:" block, treating the result as the verdict whether to
>>>> raise #GP to the guest? As the read path does this anyway, we're
>>>> not exposing ourselves to new risks.
>>> What about write-only MSRs?
>> Bad luck (I'm sorry to say so, but we have an actual bug to fix here).
>> If we find any such is used, we'll have to add individual case labels.
> Since it wasn't clear with your question above and you earlier
> given R-b, I had dropped the latter from v2. Could you clarify
> whether I may reinstate it?

Yes, please.

-boris

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel