| Message ID | 1510872316-13762-1-git-send-email-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
>>> 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
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
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); > } > >
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); }
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(+)