[PATCH for-4.10] tools/libxc: Fix restoration of PV MSRs after migrate

[PATCH for-4.10] tools/libxc: Fix restoration of PV MSRs after migrate

[Andrew Cooper]

There are two bugs in process_vcpu_msrs() which clearly demonstrate that I
didn't test this bit of Migration v2 very well when writing it...

vcpu->msrsz is always expected to be a multiple of xen_domctl_vcpu_msr_t
records in a spec-compliant stream, so the modulo yields 0 for the msr_count,
rather than the actual number sent in the stream.

Passing 0 for the msr_count causes the hypercall to exit early, and hides the
fact that the guest handle is inserted into the wrong field in the domctl
union.

The reason that these bugs have gone unnoticed for so long is that the only
MSRs passed like this for PV guests are the AMD DBGEXT MSRs, which only exist
in fairly modern hardware, and whose use doesn't appear to be implemented in
any contemporary PV guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Julien Grall <julien.grall@arm.com>

This wants backporting to all stable trees, so should also be considered for
inclusion into 4.10 at this point.
---
 tools/libxc/xc_sr_restore_x86_pv.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 50e25c1..ed0fd0e 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -455,8 +455,8 @@ static int process_vcpu_msrs(struct xc_sr_context *ctx,
     domctl.cmd = XEN_DOMCTL_set_vcpu_msrs;
     domctl.domain = ctx->domid;
     domctl.u.vcpu_msrs.vcpu = vcpuid;
-    domctl.u.vcpu_msrs.msr_count = vcpu->msrsz % sizeof(xen_domctl_vcpu_msr_t);
-    set_xen_guest_handle(domctl.u.vcpuextstate.buffer, buffer);
+    domctl.u.vcpu_msrs.msr_count = vcpu->msrsz / sizeof(xen_domctl_vcpu_msr_t);
+    set_xen_guest_handle(domctl.u.vcpu_msrs.msrs, buffer);
 
     memcpy(buffer, vcpu->msr, vcpu->msrsz);
 
-- 
2.1.4


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

Re: [PATCH for-4.10] tools/libxc: Fix restoration of PV MSRs after migrate

[Wei Liu]

On Thu, Nov 16, 2017 at 09:13:22PM +0000, Andrew Cooper wrote:
> There are two bugs in process_vcpu_msrs() which clearly demonstrate that I
> didn't test this bit of Migration v2 very well when writing it...
> 
> vcpu->msrsz is always expected to be a multiple of xen_domctl_vcpu_msr_t
> records in a spec-compliant stream, so the modulo yields 0 for the msr_count,
> rather than the actual number sent in the stream.
> 
> Passing 0 for the msr_count causes the hypercall to exit early, and hides the
> fact that the guest handle is inserted into the wrong field in the domctl
> union.
> 
> The reason that these bugs have gone unnoticed for so long is that the only
> MSRs passed like this for PV guests are the AMD DBGEXT MSRs, which only exist
> in fairly modern hardware, and whose use doesn't appear to be implemented in
> any contemporary PV guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Wei Liu <wei.liu2@citrix.com>

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

Re: [PATCH for-4.10] tools/libxc: Fix restoration of PV MSRs after migrate

[Jan Beulich]

>>> On 16.11.17 at 22:13, <andrew.cooper3@citrix.com> wrote:
> There are two bugs in process_vcpu_msrs() which clearly demonstrate that I
> didn't test this bit of Migration v2 very well when writing it...
> 
> vcpu->msrsz is always expected to be a multiple of xen_domctl_vcpu_msr_t
> records in a spec-compliant stream, so the modulo yields 0 for the msr_count,
> rather than the actual number sent in the stream.
> 
> Passing 0 for the msr_count causes the hypercall to exit early, and hides the
> fact that the guest handle is inserted into the wrong field in the domctl
> union.

Oops.

> The reason that these bugs have gone unnoticed for so long is that the only
> MSRs passed like this for PV guests are the AMD DBGEXT MSRs, which only exist
> in fairly modern hardware, and whose use doesn't appear to be implemented in
> any contemporary PV guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH for-4.10] tools/libxc: Fix restoration of PV MSRs after migrate

[Julien Grall]

Hi,

On 11/16/2017 09:13 PM, Andrew Cooper wrote:
> There are two bugs in process_vcpu_msrs() which clearly demonstrate that I
> didn't test this bit of Migration v2 very well when writing it...
> 
> vcpu->msrsz is always expected to be a multiple of xen_domctl_vcpu_msr_t
> records in a spec-compliant stream, so the modulo yields 0 for the msr_count,
> rather than the actual number sent in the stream.
> 
> Passing 0 for the msr_count causes the hypercall to exit early, and hides the
> fact that the guest handle is inserted into the wrong field in the domctl
> union.
> 
> The reason that these bugs have gone unnoticed for so long is that the only
> MSRs passed like this for PV guests are the AMD DBGEXT MSRs, which only exist
> in fairly modern hardware, and whose use doesn't appear to be implemented in
> any contemporary PV guests.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Julien Grall <julien.grall@arm.com>
> 
> This wants backporting to all stable trees, so should also be considered for
> inclusion into 4.10 at this point.

Release-acked-by: Julien Grall <julien.grall@linaro.org>

Cheers,

> ---
>   tools/libxc/xc_sr_restore_x86_pv.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
> index 50e25c1..ed0fd0e 100644
> --- a/tools/libxc/xc_sr_restore_x86_pv.c
> +++ b/tools/libxc/xc_sr_restore_x86_pv.c
> @@ -455,8 +455,8 @@ static int process_vcpu_msrs(struct xc_sr_context *ctx,
>       domctl.cmd = XEN_DOMCTL_set_vcpu_msrs;
>       domctl.domain = ctx->domid;
>       domctl.u.vcpu_msrs.vcpu = vcpuid;
> -    domctl.u.vcpu_msrs.msr_count = vcpu->msrsz % sizeof(xen_domctl_vcpu_msr_t);
> -    set_xen_guest_handle(domctl.u.vcpuextstate.buffer, buffer);
> +    domctl.u.vcpu_msrs.msr_count = vcpu->msrsz / sizeof(xen_domctl_vcpu_msr_t);
> +    set_xen_guest_handle(domctl.u.vcpu_msrs.msrs, buffer);
>   
>       memcpy(buffer, vcpu->msr, vcpu->msrsz);
>   
> 

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