[PATCH for-4.10] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[PATCH for-4.10] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[Andrew Cooper]

Ever since it was introduced in c/s bd1f0b45ff, hvm_save_cpu_msrs() has had a
bug whereby it corrupts the HVM context stream if some, but fewer than the
maximum number of MSRs are written.

_hvm_init_entry() creates an hvm_save_descriptor with length for
msr_count_max, but in the case that we write fewer than max, h->cur only moves
forward by the amount of space used, causing the subsequent
hvm_save_descriptor to be written within the bounds of the previous one.

To resolve this, reduce the length reported by the descriptor to match the
actual number of bytes used.

A typical failure on the destination side looks like:

    (XEN) HVM4 restore: CPU_MSR 0
    (XEN) HVM4.0 restore: not enough data left to read 56 MSR bytes
    (XEN) HVM4 restore: failed to load entry 20/0

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.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.
---
 xen/arch/x86/hvm/hvm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 0af498a..c5e8467 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -1330,6 +1330,7 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
 
     for_each_vcpu ( d, v )
     {
+        struct hvm_save_descriptor *d = _p(&h->data[h->cur]);
         struct hvm_msr *ctxt;
         unsigned int i;
 
@@ -1348,8 +1349,13 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
             ctxt->msr[i]._rsvd = 0;
 
         if ( ctxt->count )
+        {
+            /* Rewrite length to indicate how much space we actually used. */
+            d->length = HVM_CPU_MSR_SIZE(ctxt->count);
             h->cur += HVM_CPU_MSR_SIZE(ctxt->count);
+        }
         else
+            /* or rewind and remove the descriptor from the stream. */
             h->cur -= sizeof(struct hvm_save_descriptor);
     }
 
-- 
2.1.4


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

Re: [PATCH for-4.10] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[Wei Liu]

On Thu, Nov 16, 2017 at 10:45:16PM +0000, Andrew Cooper wrote:
> Ever since it was introduced in c/s bd1f0b45ff, hvm_save_cpu_msrs() has had a
> bug whereby it corrupts the HVM context stream if some, but fewer than the
> maximum number of MSRs are written.
> 
> _hvm_init_entry() creates an hvm_save_descriptor with length for
> msr_count_max, but in the case that we write fewer than max, h->cur only moves
> forward by the amount of space used, causing the subsequent
> hvm_save_descriptor to be written within the bounds of the previous one.
> 
> To resolve this, reduce the length reported by the descriptor to match the
> actual number of bytes used.
> 
> A typical failure on the destination side looks like:
> 
>     (XEN) HVM4 restore: CPU_MSR 0
>     (XEN) HVM4.0 restore: not enough data left to read 56 MSR bytes
>     (XEN) HVM4 restore: failed to load entry 20/0
> 
> 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] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[Jan Beulich]

>>> On 16.11.17 at 23:45, <andrew.cooper3@citrix.com> wrote:
> Ever since it was introduced in c/s bd1f0b45ff, hvm_save_cpu_msrs() has had a
> bug whereby it corrupts the HVM context stream if some, but fewer than the
> maximum number of MSRs are written.
> 
> _hvm_init_entry() creates an hvm_save_descriptor with length for
> msr_count_max, but in the case that we write fewer than max, h->cur only moves
> forward by the amount of space used, causing the subsequent
> hvm_save_descriptor to be written within the bounds of the previous one.
> 
> To resolve this, reduce the length reported by the descriptor to match the
> actual number of bytes used.
> 
> A typical failure on the destination side looks like:
> 
>     (XEN) HVM4 restore: CPU_MSR 0
>     (XEN) HVM4.0 restore: not enough data left to read 56 MSR bytes
>     (XEN) HVM4 restore: failed to load entry 20/0
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1330,6 +1330,7 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>  
>      for_each_vcpu ( d, v )
>      {
> +        struct hvm_save_descriptor *d = _p(&h->data[h->cur]);
>          struct hvm_msr *ctxt;
>          unsigned int i;
>  
> @@ -1348,8 +1349,13 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>              ctxt->msr[i]._rsvd = 0;
>  
>          if ( ctxt->count )
> +        {
> +            /* Rewrite length to indicate how much space we actually used. */
> +            d->length = HVM_CPU_MSR_SIZE(ctxt->count);

Would of course be nice if we had a function to do this, such that
the (sufficiently hidden) cast above also wouldn't be necessary to
open code in places like this one.

Jan


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

Re: [PATCH for-4.10] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[Andrew Cooper]

On 17/11/17 12:15, Jan Beulich wrote:
>>>> On 16.11.17 at 23:45, <andrew.cooper3@citrix.com> wrote:
>> Ever since it was introduced in c/s bd1f0b45ff, hvm_save_cpu_msrs() has had a
>> bug whereby it corrupts the HVM context stream if some, but fewer than the
>> maximum number of MSRs are written.
>>
>> _hvm_init_entry() creates an hvm_save_descriptor with length for
>> msr_count_max, but in the case that we write fewer than max, h->cur only moves
>> forward by the amount of space used, causing the subsequent
>> hvm_save_descriptor to be written within the bounds of the previous one.
>>
>> To resolve this, reduce the length reported by the descriptor to match the
>> actual number of bytes used.
>>
>> A typical failure on the destination side looks like:
>>
>>     (XEN) HVM4 restore: CPU_MSR 0
>>     (XEN) HVM4.0 restore: not enough data left to read 56 MSR bytes
>>     (XEN) HVM4 restore: failed to load entry 20/0
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -1330,6 +1330,7 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>>  
>>      for_each_vcpu ( d, v )
>>      {
>> +        struct hvm_save_descriptor *d = _p(&h->data[h->cur]);
>>          struct hvm_msr *ctxt;
>>          unsigned int i;
>>  
>> @@ -1348,8 +1349,13 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>>              ctxt->msr[i]._rsvd = 0;
>>  
>>          if ( ctxt->count )
>> +        {
>> +            /* Rewrite length to indicate how much space we actually used. */
>> +            d->length = HVM_CPU_MSR_SIZE(ctxt->count);
> Would of course be nice if we had a function to do this, such that
> the (sufficiently hidden) cast above also wouldn't be necessary to
> open code in places like this one.

This is the one and only case where we need to rewrite the length.  All
records (other than XSAVE) are fixed size, and XSAVE can calculate the
exact required size ahead of setting up descriptor in the first place.

(Also, this code isn't long for the world.  It will be changing when the
CPUID/MSR policy work is complete, and we can rearrange the migration
stream to put data in the order the destination needs to receive it.)

~Andrew

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

Re: [PATCH for-4.10] x86/hvm: Don't corrupt the HVM context stream when writing the MSR record

[Julien Grall]

Hi,

On 11/16/2017 10:45 PM, Andrew Cooper wrote:
> Ever since it was introduced in c/s bd1f0b45ff, hvm_save_cpu_msrs() has had a
> bug whereby it corrupts the HVM context stream if some, but fewer than the
> maximum number of MSRs are written.
> 
> _hvm_init_entry() creates an hvm_save_descriptor with length for
> msr_count_max, but in the case that we write fewer than max, h->cur only moves
> forward by the amount of space used, causing the subsequent
> hvm_save_descriptor to be written within the bounds of the previous one.
> 
> To resolve this, reduce the length reported by the descriptor to match the
> actual number of bytes used.
> 
> A typical failure on the destination side looks like:
> 
>      (XEN) HVM4 restore: CPU_MSR 0
>      (XEN) HVM4.0 restore: not enough data left to read 56 MSR bytes
>      (XEN) HVM4 restore: failed to load entry 20/0
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.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,

> ---
>   xen/arch/x86/hvm/hvm.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 0af498a..c5e8467 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -1330,6 +1330,7 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>   
>       for_each_vcpu ( d, v )
>       {
> +        struct hvm_save_descriptor *d = _p(&h->data[h->cur]);
>           struct hvm_msr *ctxt;
>           unsigned int i;
>   
> @@ -1348,8 +1349,13 @@ static int hvm_save_cpu_msrs(struct domain *d, hvm_domain_context_t *h)
>               ctxt->msr[i]._rsvd = 0;
>   
>           if ( ctxt->count )
> +        {
> +            /* Rewrite length to indicate how much space we actually used. */
> +            d->length = HVM_CPU_MSR_SIZE(ctxt->count);
>               h->cur += HVM_CPU_MSR_SIZE(ctxt->count);
> +        }
>           else
> +            /* or rewind and remove the descriptor from the stream. */
>               h->cur -= sizeof(struct hvm_save_descriptor);
>       }
>   
> 

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