The sum "cluster_index + count" may overflow uint32_t.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
---
 block/parallels.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/parallels.c b/block/parallels.c
index 9205a0864f..071b6dcaf8 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -184,11 +184,11 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
     BDRVParallelsState *s = bs->opaque;
     uint32_t cluster_index = host_cluster_index(s, off);
     unsigned long next_used;
-    if (cluster_index + count > bitmap_size) {
+    if ((uint64_t)cluster_index + count > bitmap_size) {
         return -E2BIG;
     }
     next_used = find_next_bit(bitmap, bitmap_size, cluster_index);
-    if (next_used < cluster_index + count) {
+    if (next_used < (uint64_t)cluster_index + count) {
         return -EBUSY;
     }
     bitmap_set(bitmap, cluster_index, count);
-- 
2.43.0

[ Cc: qemu-block ]

Am 06.11.2024 um 09:04 hat Dmitry Frolov geschrieben:
> The sum "cluster_index + count" may overflow uint32_t.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>

Thanks, applied to the block branch.

While trying to check if this can be triggered in practice, I found this
line in parallels_fill_used_bitmap():

    s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);

s->used_bmap_size is unsigned long, payload_bytes is the int64_t result
of bdrv_getlength() for the image file, which could certainly be made
more than 4 GB * cluster_size. I think we need an overflow check there,
too.

When allocate_clusters() calculates new_usedsize, it doesn't seem to
consider the overflow case either.

Denis, can you take a look?

Kevin

On 11/6/24 10:53, Kevin Wolf wrote:
> [ Cc: qemu-block ]
>
> Am 06.11.2024 um 09:04 hat Dmitry Frolov geschrieben:
>> The sum "cluster_index + count" may overflow uint32_t.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> Thanks, applied to the block branch.
>
> While trying to check if this can be triggered in practice, I found this
> line in parallels_fill_used_bitmap():
>
>      s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
>
> s->used_bmap_size is unsigned long, payload_bytes is the int64_t result
> of bdrv_getlength() for the image file, which could certainly be made
> more than 4 GB * cluster_size. I think we need an overflow check there,
> too.
>
> When allocate_clusters() calculates new_usedsize, it doesn't seem to
> consider the overflow case either.
>
> Denis, can you take a look?
>
> Kevin
>
We definitely have more places inside the code and I'll take a look.

Speaking about this particular change - this will not work. In general
we should signal corruption when the cluster number is overflowed.
This data would not be accessible due to format restrictions.

Den

On 11/6/24 10:53, Kevin Wolf wrote:
> [ Cc: qemu-block ]
>
> Am 06.11.2024 um 09:04 hat Dmitry Frolov geschrieben:
>> The sum "cluster_index + count" may overflow uint32_t.
>>
>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>
>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> Thanks, applied to the block branch.
>
> While trying to check if this can be triggered in practice, I found this
> line in parallels_fill_used_bitmap():
>
>      s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
>
> s->used_bmap_size is unsigned long, payload_bytes is the int64_t result
> of bdrv_getlength() for the image file, which could certainly be made
> more than 4 GB * cluster_size. I think we need an overflow check there,
> too.
>
> When allocate_clusters() calculates new_usedsize, it doesn't seem to
> consider the overflow case either.
>
> Denis, can you take a look?
>
> Kevin
>
Hi, Kevin, Dmitry!

In general, the situation is the following.

On-disk format heavily uses offsets from the beginning of the disk
denominated in clusters. These offsets are saved in uint32 on disk.
This means that the image with 4T virtual size and 1M cluster size
will use offsets from 0 to 4 * 2^10 in different tables on disk.

There is existing problem in the format specification that we
can not easily apply limits to the virtual size of the disk as
we also can have arbitrary size growing metadata like CBT, which
is kept in the same address space (cluster offsets).

Though in reality I have never seen images with non-1 Mb cluster
size and in order to nearly overflow them we would need really
allocated images of 4 PB.

Theoretically the problem is possible but it looks impractical
to me in the real life so far.

Thank you in advance,
     Den

Am 06.11.2024 um 16:45 hat Denis V. Lunev geschrieben:
> On 11/6/24 10:53, Kevin Wolf wrote:
> > [ Cc: qemu-block ]
> > 
> > Am 06.11.2024 um 09:04 hat Dmitry Frolov geschrieben:
> > > The sum "cluster_index + count" may overflow uint32_t.
> > > 
> > > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> > > 
> > > Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
> > Thanks, applied to the block branch.
> > 
> > While trying to check if this can be triggered in practice, I found this
> > line in parallels_fill_used_bitmap():
> > 
> >      s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
> > 
> > s->used_bmap_size is unsigned long, payload_bytes is the int64_t result
> > of bdrv_getlength() for the image file, which could certainly be made
> > more than 4 GB * cluster_size. I think we need an overflow check there,
> > too.
> > 
> > When allocate_clusters() calculates new_usedsize, it doesn't seem to
> > consider the overflow case either.
> > 
> > Denis, can you take a look?
> > 
> > Kevin
> > 
> Hi, Kevin, Dmitry!
> 
> In general, the situation is the following.
> 
> On-disk format heavily uses offsets from the beginning of the disk
> denominated in clusters. These offsets are saved in uint32 on disk.
> This means that the image with 4T virtual size and 1M cluster size
> will use offsets from 0 to 4 * 2^10 in different tables on disk.
> 
> There is existing problem in the format specification that we
> can not easily apply limits to the virtual size of the disk as
> we also can have arbitrary size growing metadata like CBT, which
> is kept in the same address space (cluster offsets).
> 
> Though in reality I have never seen images with non-1 Mb cluster
> size and in order to nearly overflow them we would need really
> allocated images of 4 PB.
> 
> Theoretically the problem is possible but it looks impractical
> to me in the real life so far.

It probably won't happen with normal images, but we need to consider
malicious images, and I think they could be constructed in a way that
causes integer overflows here.

At least the one that directly takes bdrv_getlength() should be trivial
to trigger, you just need to extend the file size enough outside of
QEMU.

Kevin

On 11/6/24 17:00, Kevin Wolf wrote:
> Am 06.11.2024 um 16:45 hat Denis V. Lunev geschrieben:
>> On 11/6/24 10:53, Kevin Wolf wrote:
>>> [ Cc: qemu-block ]
>>>
>>> Am 06.11.2024 um 09:04 hat Dmitry Frolov geschrieben:
>>>> The sum "cluster_index + count" may overflow uint32_t.
>>>>
>>>> Found by Linux Verification Center (linuxtesting.org) with SVACE.
>>>>
>>>> Signed-off-by: Dmitry Frolov <frolov@swemel.ru>
>>> Thanks, applied to the block branch.
>>>
>>> While trying to check if this can be triggered in practice, I found this
>>> line in parallels_fill_used_bitmap():
>>>
>>>       s->used_bmap_size = DIV_ROUND_UP(payload_bytes, s->cluster_size);
>>>
>>> s->used_bmap_size is unsigned long, payload_bytes is the int64_t result
>>> of bdrv_getlength() for the image file, which could certainly be made
>>> more than 4 GB * cluster_size. I think we need an overflow check there,
>>> too.
>>>
>>> When allocate_clusters() calculates new_usedsize, it doesn't seem to
>>> consider the overflow case either.
>>>
>>> Denis, can you take a look?
>>>
>>> Kevin
>>>
>> Hi, Kevin, Dmitry!
>>
>> In general, the situation is the following.
>>
>> On-disk format heavily uses offsets from the beginning of the disk
>> denominated in clusters. These offsets are saved in uint32 on disk.
>> This means that the image with 4T virtual size and 1M cluster size
>> will use offsets from 0 to 4 * 2^10 in different tables on disk.
>>
>> There is existing problem in the format specification that we
>> can not easily apply limits to the virtual size of the disk as
>> we also can have arbitrary size growing metadata like CBT, which
>> is kept in the same address space (cluster offsets).
>>
>> Though in reality I have never seen images with non-1 Mb cluster
>> size and in order to nearly overflow them we would need really
>> allocated images of 4 PB.
>>
>> Theoretically the problem is possible but it looks impractical
>> to me in the real life so far.
> It probably won't happen with normal images, but we need to consider
> malicious images, and I think they could be constructed in a way that
> causes integer overflows here.
>
> At least the one that directly takes bdrv_getlength() should be trivial
> to trigger, you just need to extend the file size enough outside of
> QEMU.
>
> Kevin
>
yah. That is reasonable and has to be fixed. You are correct.

Den