Re: [Qemu-block] [Qemu-devel] [PATCH 2/7] block/qcow2-refcount: avoid eating RAM

[Eric Blake]

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 <address@hidden>
> ---
>   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