[Qemu-devel] [PATCH] qcow2-refcount: Sanitize refcount table entry

[Qemu-devel] [PATCH] qcow2-refcount: Sanitize refcount table entry

[Max Reitz]

When reading the refcount table entry in get_refcount(), only bits which
are actually significant for the refcount block offset should be taken
into account.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-refcount.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 8712d8b..6151148 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -96,7 +96,8 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
     refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
     if (refcount_table_index >= s->refcount_table_size)
         return 0;
-    refcount_block_offset = s->refcount_table[refcount_table_index];
+    refcount_block_offset =
+        s->refcount_table[refcount_table_index] & REFT_OFFSET_MASK;
     if (!refcount_block_offset)
         return 0;
 
-- 
1.9.0

Re: [Qemu-devel] [PATCH] qcow2-refcount: Sanitize refcount table entry

[Laszlo Ersek]

On 03/07/14 23:10, Max Reitz wrote:
> When reading the refcount table entry in get_refcount(), only bits which
> are actually significant for the refcount block offset should be taken
> into account.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 8712d8b..6151148 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -96,7 +96,8 @@ static int get_refcount(BlockDriverState *bs, int64_t cluster_index)
>      refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
>      if (refcount_table_index >= s->refcount_table_size)
>          return 0;
> -    refcount_block_offset = s->refcount_table[refcount_table_index];
> +    refcount_block_offset =
> +        s->refcount_table[refcount_table_index] & REFT_OFFSET_MASK;
>      if (!refcount_block_offset)
>          return 0;

Sigh.

I grepped the source for the string "refcount_table[". I was immediately
confused by two different element types, uint64_t vs. uint16_t.
Thankfully, docs/specs/qcow2.txt explains the two-level structure, and
now it's clear to me that here we care about entries of the *first*
(high) level table.

But why on God's green Earth is the *other* kind called "refcount_table"
too, in qcow2_check_refcounts(), and in inc_refcounts(), and in
check_refcounts_l1(), when those arrays should be called
*refcount_block*? It's the second (low) level structure.

So, this is what I've found:
(1) qcow2_refcount_init(): loads table from disk and does endianness
conversion. Resultant table entries are not masked, should be OK.

(2) get_refcount(): fixed by this patch.

(3) alloc_refcount_block(), first use (read access): masked correctly.

(4) alloc_refcount_block(), 2nd use (write access): the value being
assigned, new_block, is derived from:

  alloc_clusters_noref()
    get_refcount()

  and get_refcount() is fixed by this patch.

(5) inc_refcounts(): works on the refcount block, not the table, hence
irrelevant

(6) write_reftable_entry(): seems to handle endianness and write stuff
to disk, masking is neither done nor necessary (I think).

(7) realloc_refcount_block(): the value assigned, "new_offset", comes from

  qcow2_alloc_clusters()
    alloc_clusters_noref()
      get_refcount()()

  and get_refcount() is fixed by this patch.

(8) qcow2_check_refcounts(): this is a horribly complicated function.

First, it uses the word "refcount_table" in *both* senses: both for the
first and the second level structure. My brain kinda halts as soon as
seeing this.

Second, the uses of s->refcount_table[i] in this function are correctly
masked. When assigning to "cluster", the low bits are shifted out, so
that's fine. Then, before comparing "offset" against zero, we check the
low bits specifically, with offset_into_cluster(). Good.

(9) qcow2_check_metadata_overlap(): masked OK, both times

In total, fixing get_refcount() affects several call chains (minimally
(4) and (7)), and I could find no other read or write access to the
refcount table that needed fixing. (I did a quick search for pointer
arithmetic too, ie. '\<refcount_table *\+' -- no matches, luckily.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Laszlo

Re: [Qemu-devel] [PATCH] qcow2-refcount: Sanitize refcount table entry

[Stefan Hajnoczi]

On Fri, Mar 07, 2014 at 11:10:12PM +0100, Max Reitz wrote:
> When reading the refcount table entry in get_refcount(), only bits which
> are actually significant for the refcount block offset should be taken
> into account.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan