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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1834 bytes --]

On 17.08.18 14:22, Vladimir Sementsov-Ogievskiy wrote:
> qcow2_inc_refcounts_imrt() (through realloc_refcount_array()) can eat
> an unpredictable amount of memory on corrupted table entries, which are
> 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 | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 615847eb09..566c19fbfa 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -1499,12 +1499,26 @@ int qcow2_inc_refcounts_imrt(BlockDriverState *bs, BdrvCheckResult *res,
>  {
>      BDRVQcow2State *s = bs->opaque;
>      uint64_t start, last, cluster_offset, k, refcount;
> +    int64_t file_len;
>      int ret;
>  
>      if (size <= 0) {
>          return 0;
>      }
>  
> +    file_len = bdrv_getlength(bs->file->bs);
> +    if (file_len < 0) {
> +        return file_len;
> +    }

Doesn't this slow things down?  Can we not cache the length somewhere
and update it whenever the image is modified?

> +
> +    if (offset + size - file_len > s->cluster_size) {
> +        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 one cluster OK?  Is there a specific case you're trying to catch
here?

Max

> +        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;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]