'count * sizeof(*pfns)' can in principle overflow, but is implausible in practice as the time between checkpoints is typically sub-second. Nevertheless, simplify the code and remove the risk.
There is no need to loop over the bitmap to calculate count. The number of set bits is returned in xc_shadow_op_stats_t which is already collected (and ignored). Bounds check the count against what will fit in REC_LENGTH_MAX. At the time of writing, this allows up to 0xffffff pfns. Rearrange the pfns loop to check for errors both ways, not simply that there were more pfns than expected. Reported-by: Jan Beulich <jbeul...@suse.com> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> --- CC: Ian Jackson <i...@xenproject.org> CC: Wei Liu <w...@xen.org> CC: Juergen Gross <jgr...@suse.com> CC: Jan Beulich <jbeul...@suse.com> CC: Olaf Hering <o...@aepfle.de> --- tools/libs/guest/xg_sr_restore.c | 33 ++++++++++++++++----------------- 1 file changed, 16 insertions(+), 17 deletions(-) diff --git a/tools/libs/guest/xg_sr_restore.c b/tools/libs/guest/xg_sr_restore.c index 07c9e291610b..bda04ee42e3f 100644 --- a/tools/libs/guest/xg_sr_restore.c +++ b/tools/libs/guest/xg_sr_restore.c @@ -425,7 +425,7 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx) int rc = -1; unsigned int count, written; uint64_t i, *pfns = NULL; - xc_shadow_op_stats_t stats = { 0, ctx->restore.p2m_size }; + xc_shadow_op_stats_t stats; struct xc_sr_record rec = { .type = REC_TYPE_CHECKPOINT_DIRTY_PFN_LIST, }; @@ -444,14 +444,17 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx) goto err; } - for ( i = 0, count = 0; i < ctx->restore.p2m_size; i++ ) + count = stats.dirty_count; + + if ( ((REC_LENGTH_MAX - sizeof(rec)) / sizeof(*pfns)) < count ) { - if ( test_bit(i, dirty_bitmap) ) - count++; + ERROR("Too many PFNs (%u) to fit in record (limit %zu)", count, + ((REC_LENGTH_MAX - sizeof(rec)) / sizeof(*pfns))); + goto err; } - - pfns = malloc(count * sizeof(*pfns)); + iov[1].iov_len = rec.length = count * sizeof(*pfns); + iov[1].iov_base = pfns = malloc(rec.length); if ( !pfns ) { ERROR("Unable to allocate %zu bytes of memory for dirty pfn list", @@ -459,24 +462,20 @@ static int send_checkpoint_dirty_pfn_list(struct xc_sr_context *ctx) goto err; } - for ( i = 0, written = 0; i < ctx->restore.p2m_size; ++i ) + for ( i = 0, written = 0; count && i < ctx->restore.p2m_size; ++i, --count ) { if ( !test_bit(i, dirty_bitmap) ) continue; - if ( written > count ) - { - ERROR("Dirty pfn list exceed"); - goto err; - } - pfns[written++] = i; } - rec.length = count * sizeof(*pfns); - - iov[1].iov_base = pfns; - iov[1].iov_len = rec.length; + if ( written != stats.dirty_count ) + { + ERROR("Mismatch between dirty bitmap bits (%u), and dirty_count (%u)", + written, stats.dirty_count); + goto err; + } if ( writev_exact(ctx->restore.send_back_fd, iov, ARRAY_SIZE(iov)) ) { -- 2.11.0