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

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

[Jan Beulich]

... 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>

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2124,7 +2124,6 @@ static int svm_msr_write_intercept(unsig
         case -ERESTART:
             result = X86EMUL_RETRY;
             break;
-        case 0:
         case 1:
             break;
         default:
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3160,7 +3160,6 @@ static int vmx_msr_write_intercept(unsig
                 {
                 case -ERESTART:
                     return X86EMUL_RETRY;
-                case 0:
                 case 1:
                     break;
                 default:




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

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

[Boris Ostrovsky]

On 02/22/2018 08:44 AM, 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>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>



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

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

[Andrew Cooper]

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.

As for the making this change, there is a better way of doing it by
moving viridian and Xen MSR handing into the new guest_{rd,wr}msr()
infrastructure, which means we won't call into both of these subsystems
for every unknown MSR.

I've already got half a patch to do this from the pending CPUID/MSR work
which I can dust off, if you like?

~Andrew

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

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

[Jan Beulich]

>>> 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.
> 
> As for the making this change, there is a better way of doing it by
> moving viridian and Xen MSR handing into the new guest_{rd,wr}msr()
> infrastructure, which means we won't call into both of these subsystems
> for every unknown MSR.

That would be harder to backport, and the original problem report
was on 4.7. I'll quote the offending Linux code for you:

static inline void cache_alloc_hsw_probe(void)
{
	struct rdt_resource *r  = &rdt_resources_all[RDT_RESOURCE_L3];
	u32 l, h, max_cbm = BIT_MASK(20) - 1;

	if (wrmsr_safe(IA32_L3_CBM_BASE, max_cbm, 0))
		return;
	rdmsr(IA32_L3_CBM_BASE, l, h);

That is, they assume that a successful WRMSR for this MSR
means the RDMSR is expected to also succeed. Quite
reasonable an expectation (leaving aside the fact that the
whole logic there looks rather clumsy), which we can't meet
without biting the bullet and accepting the risk you describe.

Do you have any other suggestion on how to eliminate the
risk while still addressing the problem at hand?

Jan


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

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

[Andrew Cooper]

On 22/02/18 15:00, 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.
>>
>> As for the making this change, there is a better way of doing it by
>> moving viridian and Xen MSR handing into the new guest_{rd,wr}msr()
>> infrastructure, which means we won't call into both of these subsystems
>> for every unknown MSR.
> That would be harder to backport, and the original problem report
> was on 4.7. I'll quote the offending Linux code for you:
>
> static inline void cache_alloc_hsw_probe(void)
> {
> 	struct rdt_resource *r  = &rdt_resources_all[RDT_RESOURCE_L3];
> 	u32 l, h, max_cbm = BIT_MASK(20) - 1;
>
> 	if (wrmsr_safe(IA32_L3_CBM_BASE, max_cbm, 0))
> 		return;
> 	rdmsr(IA32_L3_CBM_BASE, l, h);
>
> That is, they assume that a successful WRMSR for this MSR
> means the RDMSR is expected to also succeed. Quite
> reasonable an expectation (leaving aside the fact that the
> whole logic there looks rather clumsy), which we can't meet
> without biting the bullet and accepting the risk you describe.
>
> Do you have any other suggestion on how to eliminate the
> risk while still addressing the problem at hand?

As a gross hack, you can enumerate all the QoS MSRs and raise #GP, as we
never expose the CPUIDs bit to guests.  This extends the logical
blacklist that we have.  Ideally, Linux should also check the CPUID bits
before poking the MSRs.

I've unfortunately not got any clever ideas for the general case.  My
plan was to throw the switch early in a release and rely on a long test
window to shake out the bugs before we release, but problems like this
are going to keep biting us until we switch to a fully whitelisted MSR
model.

~Andrew

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

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

[Jan Beulich]

>>> 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.

Jan


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

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

[Boris Ostrovsky]

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?

-boris

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

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

[Jan Beulich]

>>> 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.

Jan


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

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

[Jan Beulich]

>>> 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?

Thanks, Jan


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

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

[Boris Ostrovsky]

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

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

[Jan Beulich]

... for non-existent 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. We
don't want to unconditionally fail the access though, as we can't be
certain the list of handled MSRs is complete enough for the guest types
we care about, so instead mirror what we do on the read paths and probe
the MSR to decide whether to raise #GP.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Probe MSR just like is already done on read paths.

--- a/xen/arch/x86/hvm/svm/svm.c
+++ b/xen/arch/x86/hvm/svm/svm.c
@@ -2125,6 +2125,13 @@ static int svm_msr_write_intercept(unsig
             result = X86EMUL_RETRY;
             break;
         case 0:
+            /*
+             * Match up with the RDMSR side for now; ultimately this entire
+             * case block should go away.
+             */
+            if ( rdmsr_safe(msr, msr_content) == 0 )
+                break;
+            goto gpf;
         case 1:
             break;
         default:
--- a/xen/arch/x86/hvm/vmx/vmx.c
+++ b/xen/arch/x86/hvm/vmx/vmx.c
@@ -3161,6 +3161,13 @@ static int vmx_msr_write_intercept(unsig
                 case -ERESTART:
                     return X86EMUL_RETRY;
                 case 0:
+                    /*
+                     * Match up with the RDMSR side for now; ultimately this
+                     * entire case block should go away.
+                     */
+                    if ( rdmsr_safe(msr, msr_content) == 0 )
+                        break;
+                    goto gp_fault;
                 case 1:
                     break;
                 default:




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

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

[Andrew Cooper]

On 23/02/2018 08:36, Jan Beulich wrote:
> ... for non-existent 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. We
> don't want to unconditionally fail the access though, as we can't be
> certain the list of handled MSRs is complete enough for the guest types
> we care about, so instead mirror what we do on the read paths and probe
> the MSR to decide whether to raise #GP.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

I'm not a fan of this approach, but I accept that it might be the least
bad option going.

However, I'm struggling to understand how it resolves the issue you
presented?  In the example, Linux did a wrmsr_safe() then blew up on a
rdmsr().  Was that in fact running on hardware lacking PSR/QoS support?

Irrespective, I think that entire block of MSRs wants blacklisting in
the short term, to make them inaccessible.

~Andrew

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

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

[Jan Beulich]

>>> On 23.02.18 at 11:07, <andrew.cooper3@citrix.com> wrote:
> On 23/02/2018 08:36, Jan Beulich wrote:
>> ... for non-existent 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. We
>> don't want to unconditionally fail the access though, as we can't be
>> certain the list of handled MSRs is complete enough for the guest types
>> we care about, so instead mirror what we do on the read paths and probe
>> the MSR to decide whether to raise #GP.
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> I'm not a fan of this approach, but I accept that it might be the least
> bad option going.
> 
> However, I'm struggling to understand how it resolves the issue you
> presented?  In the example, Linux did a wrmsr_safe() then blew up on a
> rdmsr().  Was that in fact running on hardware lacking PSR/QoS support?

Otherwise the RDMSR wouldn't have failed.

> Irrespective, I think that entire block of MSRs wants blacklisting in
> the short term, to make them inaccessible.

That would deal with this particular issue in Linux, but nor the
general pattern.

Jan


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

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

[Tian, Kevin]

> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Friday, February 23, 2018 4:37 PM
> 
> ... for non-existent 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. We
> don't want to unconditionally fail the access though, as we can't be
> certain the list of handled MSRs is complete enough for the guest types
> we care about, so instead mirror what we do on the read paths and probe
> the MSR to decide whether to raise #GP.
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Kevin Tian <kevin.tian@intel.com>

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

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

[Andrew Cooper]

On 23/02/18 08:36, Jan Beulich wrote:
> ... for non-existent 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. We
> don't want to unconditionally fail the access though, as we can't be
> certain the list of handled MSRs is complete enough for the guest types
> we care about, so instead mirror what we do on the read paths and probe
> the MSR to decide whether to raise #GP.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Having thought this through:

At the moment, a write to any unhandled MSR is treated as silent write
discard.  This is terrible behaviour from the guests point of view.

With this patch in place, a write to any unreadable MSR yields #GP,
which is better behaviour.

The only write-only MSRs I'm aware of are in the x2apic block, and
MSR_PRED_CMD, all of which are explicitly handled.

Therefore, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, as
this is an improvement in behaviour, even if the result still isn't great.

~Andrew

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