[Qemu-devel] [PATCH] qcow2: Fix alloc_clusters_noref() overflow detection

[Qemu-devel] [PATCH] qcow2: Fix alloc_clusters_noref() overflow detection

[Max Reitz]

If the very first allocation has a length of 0, the free_cluster_index
is still 0 after the for loop, which means that subtracting one from it
will underflow and signal an invalid range of clusters by returning
-EFBIG. However, there is no such range, as its length is 0.

Fix this by preventing underflows on free_cluster_index during the
check.

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

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index e79895d..9507aef 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -656,7 +656,9 @@ retry:
 
     /* Make sure that all offsets in the "allocated" range are representable
      * in an int64_t */
-    if (s->free_cluster_index - 1 > (INT64_MAX >> s->cluster_bits)) {
+    if (s->free_cluster_index > 0 &&
+        s->free_cluster_index - 1 > (INT64_MAX >> s->cluster_bits))
+    {
         return -EFBIG;
     }
 
-- 
1.9.2

Re: [Qemu-devel] [PATCH] qcow2: Fix alloc_clusters_noref() overflow detection

[Kevin Wolf]

Am 04.05.2014 um 05:31 hat Max Reitz geschrieben:
> If the very first allocation has a length of 0, the free_cluster_index
> is still 0 after the for loop, which means that subtracting one from it
> will underflow and signal an invalid range of clusters by returning
> -EFBIG. However, there is no such range, as its length is 0.
> 
> Fix this by preventing underflows on free_cluster_index during the
> check.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Heh, I wondered about this when I reviewed that other patch, and came to
the conclusion that it probably doesn't happen. Did you find a case
where it does happen in fact?

Anyway, this can't hurt:

Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH] qcow2: Fix alloc_clusters_noref() overflow detection

[Max Reitz]

On 05.05.2014 11:36, Kevin Wolf wrote:
> Am 04.05.2014 um 05:31 hat Max Reitz geschrieben:
>> If the very first allocation has a length of 0, the free_cluster_index
>> is still 0 after the for loop, which means that subtracting one from it
>> will underflow and signal an invalid range of clusters by returning
>> -EFBIG. However, there is no such range, as its length is 0.
>>
>> Fix this by preventing underflows on free_cluster_index during the
>> check.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
> Heh, I wondered about this when I reviewed that other patch, and came to
> the conclusion that it probably doesn't happen. Did you find a case
> where it does happen in fact?

Yes, deleting the last internal snapshot results in a 0-byte allocation 
for the new (empty) snapshot table. If this is the first allocation 
after an image has been opened (which is the case for qemu-img snapshot 
-d), you'll receive the “File too big” error message (which confused me 
quite a bit at first, as I wasn't specifically testing this series).

Max

> Anyway, this can't hurt:
>
> Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH] qcow2: Fix alloc_clusters_noref() overflow detection

[Stefan Hajnoczi]

On Sun, May 04, 2014 at 05:31:40AM +0200, Max Reitz wrote:
> If the very first allocation has a length of 0, the free_cluster_index
> is still 0 after the for loop, which means that subtracting one from it
> will underflow and signal an invalid range of clusters by returning
> -EFBIG. However, there is no such range, as its length is 0.
> 
> Fix this by preventing underflows on free_cluster_index during the
> check.
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/qcow2-refcount.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)

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

Stefan