In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
get the size to write in bytes. Coverity notes that this means that
we do the multiply as a 32x32->32 multiply before converting to
64 bits, which has the potential to overflow.

This is very unlikely to happen, since the block map has 4 bytes per
block and the maximum number of blocks in the image must fit into a
32-bit integer.  But we can keep Coverity happy by including a cast
so we do a 64-bit multiply here.

Resolves: Coverity CID 1508076
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 block/vdi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/vdi.c b/block/vdi.c
index 6363da08cee..27c60ba18d0 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -728,7 +728,7 @@ nonallocating_write:
         logout("will write %u block map sectors starting from entry %u\n",
                n_sectors, bmap_first);
         ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
-                             n_sectors * SECTOR_SIZE, base, 0);
+                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
     }
 
     return ret;
-- 
2.34.1

Am 31.07.24 um 16:36 schrieb Peter Maydell:

> In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
> get the size to write in bytes. Coverity notes that this means that
> we do the multiply as a 32x32->32 multiply before converting to
> 64 bits, which has the potential to overflow.
>
> This is very unlikely to happen, since the block map has 4 bytes per
> block and the maximum number of blocks in the image must fit into a
> 32-bit integer.  But we can keep Coverity happy by including a cast
> so we do a 64-bit multiply here.
>
> Resolves: Coverity CID 1508076
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   block/vdi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/block/vdi.c b/block/vdi.c
> index 6363da08cee..27c60ba18d0 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -728,7 +728,7 @@ nonallocating_write:
>           logout("will write %u block map sectors starting from entry %u\n",
>                  n_sectors, bmap_first);
>           ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
> -                             n_sectors * SECTOR_SIZE, base, 0);
> +                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
>       }
>   
>       return ret;

Thanks.

Reviewed-by: Stefan Weil <sw@weilnetz.de>

Am 31.07.2024 um 16:36 hat Peter Maydell geschrieben:
> In vdi_co_pwritev() we multiply a sector count by SECTOR_SIZE to
> get the size to write in bytes. Coverity notes that this means that
> we do the multiply as a 32x32->32 multiply before converting to
> 64 bits, which has the potential to overflow.
> 
> This is very unlikely to happen, since the block map has 4 bytes per
> block and the maximum number of blocks in the image must fit into a
> 32-bit integer.  But we can keep Coverity happy by including a cast
> so we do a 64-bit multiply here.
> 
> Resolves: Coverity CID 1508076
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  block/vdi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/vdi.c b/block/vdi.c
> index 6363da08cee..27c60ba18d0 100644
> --- a/block/vdi.c
> +++ b/block/vdi.c
> @@ -728,7 +728,7 @@ nonallocating_write:
>          logout("will write %u block map sectors starting from entry %u\n",
>                 n_sectors, bmap_first);
>          ret = bdrv_co_pwrite(bs->file, bmap_offset * SECTOR_SIZE,
> -                             n_sectors * SECTOR_SIZE, base, 0);
> +                             n_sectors * (uint64_t)SECTOR_SIZE, base, 0);
>      }

I wonder if we shouldn't just make VDI's SECTOR_SIZE 64 bits like
BDRV_SECTOR_SIZE. It's easy to miss the cast in individual places.

Kevin