vcpu_guest_context_any_t is currently allocated on the stack, and copied from
a mutable buffer which is freed immediately after its use here.
Mutate the buffer in place instead of duplicating it.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
---
tools/libxc/xc_sr_restore_x86_pv.c | 29 +++++++++++++----------------
1 file changed, 13 insertions(+), 16 deletions(-)
diff --git a/tools/libxc/xc_sr_restore_x86_pv.c b/tools/libxc/xc_sr_restore_x86_pv.c
index 0a5b0016b4..70b8d2ad95 100644
--- a/tools/libxc/xc_sr_restore_x86_pv.c
+++ b/tools/libxc/xc_sr_restore_x86_pv.c
@@ -236,28 +236,25 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
unsigned int vcpuid)
{
xc_interface *xch = ctx->xch;
- vcpu_guest_context_any_t vcpu;
+ vcpu_guest_context_any_t *vcpu = ctx->x86_pv.restore.vcpus[vcpuid].basic;
xen_pfn_t pfn, mfn;
unsigned i, gdt_count;
int rc = -1;
- memcpy(&vcpu, ctx->x86_pv.restore.vcpus[vcpuid].basic,
- ctx->x86_pv.restore.vcpus[vcpuid].basicsz);
-
/* Vcpu 0 is special: Convert the suspend record to an mfn. */
if ( vcpuid == 0 )
{
- rc = process_start_info(ctx, &vcpu);
+ rc = process_start_info(ctx, vcpu);
if ( rc )
return rc;
rc = -1;
}
- SET_FIELD(&vcpu, flags,
- GET_FIELD(&vcpu, flags, ctx->x86_pv.width) | VGCF_online,
+ SET_FIELD(vcpu, flags,
+ GET_FIELD(vcpu, flags, ctx->x86_pv.width) | VGCF_online,
ctx->x86_pv.width);
- gdt_count = GET_FIELD(&vcpu, gdt_ents, ctx->x86_pv.width);
+ gdt_count = GET_FIELD(vcpu, gdt_ents, ctx->x86_pv.width);
if ( gdt_count > FIRST_RESERVED_GDT_ENTRY )
{
ERROR("GDT entry count (%u) out of range (max %u)",
@@ -270,7 +267,7 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
/* Convert GDT frames to mfns. */
for ( i = 0; i < gdt_count; ++i )
{
- pfn = GET_FIELD(&vcpu, gdt_frames[i], ctx->x86_pv.width);
+ pfn = GET_FIELD(vcpu, gdt_frames[i], ctx->x86_pv.width);
if ( pfn > ctx->x86_pv.max_pfn )
{
ERROR("GDT frame %u (pfn %#lx) out of range", i, pfn);
@@ -293,11 +290,11 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
goto err;
}
- SET_FIELD(&vcpu, gdt_frames[i], mfn, ctx->x86_pv.width);
+ SET_FIELD(vcpu, gdt_frames[i], mfn, ctx->x86_pv.width);
}
/* Convert CR3 to an mfn. */
- pfn = cr3_to_mfn(ctx, GET_FIELD(&vcpu, ctrlreg[3], ctx->x86_pv.width));
+ pfn = cr3_to_mfn(ctx, GET_FIELD(vcpu, ctrlreg[3], ctx->x86_pv.width));
if ( pfn > ctx->x86_pv.max_pfn )
{
ERROR("cr3 (pfn %#lx) out of range", pfn);
@@ -323,12 +320,12 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
goto err;
}
- SET_FIELD(&vcpu, ctrlreg[3], mfn_to_cr3(ctx, mfn), ctx->x86_pv.width);
+ SET_FIELD(vcpu, ctrlreg[3], mfn_to_cr3(ctx, mfn), ctx->x86_pv.width);
/* 64bit guests: Convert CR1 (guest pagetables) to mfn. */
- if ( ctx->x86_pv.levels == 4 && (vcpu.x64.ctrlreg[1] & 1) )
+ if ( ctx->x86_pv.levels == 4 && (vcpu->x64.ctrlreg[1] & 1) )
{
- pfn = vcpu.x64.ctrlreg[1] >> PAGE_SHIFT;
+ pfn = vcpu->x64.ctrlreg[1] >> PAGE_SHIFT;
if ( pfn > ctx->x86_pv.max_pfn )
{
@@ -355,10 +352,10 @@ static int process_vcpu_basic(struct xc_sr_context *ctx,
goto err;
}
- vcpu.x64.ctrlreg[1] = (uint64_t)mfn << PAGE_SHIFT;
+ vcpu->x64.ctrlreg[1] = (uint64_t)mfn << PAGE_SHIFT;
}
- if ( xc_vcpu_setcontext(xch, ctx->domid, vcpuid, &vcpu) )
+ if ( xc_vcpu_setcontext(xch, ctx->domid, vcpuid, vcpu) )
{
PERROR("Failed to set vcpu%u's basic info", vcpuid);
goto err;
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-develAndrew Cooper writes ("[PATCH] libxc/restore: Don't duplicate state in process_vcpu_basic()"):
> vcpu_guest_context_any_t is currently allocated on the stack, and copied from
> a mutable buffer which is freed immediately after its use here.
>
> Mutate the buffer in place instead of duplicating it.
It is not obvious to me that nothing uses the modified fields after
process_vcpu_basic has edited things. The ctx of which the vcpu is
part is passed to process_vcpu_<various> by update_vcpu_context.
Why do you think this copying (which is extra code) was introduced ?
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
On 20/12/2019 18:22, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH] libxc/restore: Don't duplicate state in process_vcpu_basic()"):
>> vcpu_guest_context_any_t is currently allocated on the stack, and copied from
>> a mutable buffer which is freed immediately after its use here.
>>
>> Mutate the buffer in place instead of duplicating it.
> It is not obvious to me that nothing uses the modified fields after
> process_vcpu_basic has edited things. The ctx of which the vcpu is
> part is passed to process_vcpu_<various> by update_vcpu_context.
>
> Why do you think this copying (which is extra code) was introduced ?
Yes. It was a consequence of adding Remus support (which happened on
xen-devel, post actually-getting-migration-v2-working).
Originally, this was copied out of the incoming xc_sr_record* pointer
which was const. Adding Remus support required buffering these records
on each checkpoint, which is why they are stashed sideways now.
By the time we get process_vcpu_basic(), we are committed to completing
state restoration and unpausing the guest.
In particular, it is not possible to process any further memory pages
because the hypercall used here changes typerefs on pagetables, which
makes them immutable from the toolstacks point of view (technically not,
but we can't map them and memcpy them any more).
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Andrew Cooper writes ("Re: [PATCH] libxc/restore: Don't duplicate state in process_vcpu_basic()"):
> On 20/12/2019 18:22, Ian Jackson wrote:
> > It is not obvious to me that nothing uses the modified fields after
> > process_vcpu_basic has edited things. The ctx of which the vcpu is
> > part is passed to process_vcpu_<various> by update_vcpu_context.
...
> Yes. It was a consequence of adding Remus support (which happened on
> xen-devel, post actually-getting-migration-v2-working).
>
> Originally, this was copied out of the incoming xc_sr_record* pointer
> which was const. Adding Remus support required buffering these records
> on each checkpoint, which is why they are stashed sideways now.
>
> By the time we get process_vcpu_basic(), we are committed to completing
> state restoration and unpausing the guest.
>
> In particular, it is not possible to process any further memory pages
> because the hypercall used here changes typerefs on pagetables, which
> makes them immutable from the toolstacks point of view (technically not,
> but we can't map them and memcpy them any more).
Thanks for the explanation. Can you add that to the commit message
please ? That way if this turns out to be wrong, or someone wants to
revert it again, we'll have some record of why this was thought to be
OK.
With that done,
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
Thanks,
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
© 2016 - 2024 Red Hat, Inc.