On 06/19/2018 01:34 PM, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
> unpredicted amount of memory on corrupted table entries, which are
s/unpredicted/an unpredictable/
> referencing regions far beyond the end of file.
>
> Prevent this, by skipping such regions from further processing.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> block/qcow2-refcount.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index f9d095aa2d..28d21bedc3 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1505,6 +1505,14 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
> return 0;
> }
>
> + if (offset + size - bdrv_getlength(bs->file->bs) > s->cluster_size) {
bdrv_getlength() can fail (returning a negative value); this needs to be
refactored so that you aren't performing arithmetic comparisons after
such a failure (even if that failure is unlikely).
> + fprintf(stderr, "ERROR: counting reference for region exceeding the "
> + "end of the file by more than one cluster: offset 0x%" PRIx64
> + " size 0x%" PRIx64 "\n", offset, size);
Why is this dumping directly to stderr?
/me reads the file
Oh. We probably ought to fix the code to pass an Error **errp parameter
through the callstack, but that's a bigger audit (and not the fault of
your patch for copying existing usage).
> + res->corruptions++;
> + return 0;
> + }
> +
> start = start_of_cluster(s, offset);
> last = start_of_cluster(s, offset + size - 1);
> for(cluster_offset = start; cluster_offset <= last;
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org