| Message ID | 20240731143617.3391947-2-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | block: Miscellaneous minor Coverity fixes | expand |
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
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>
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;
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(-)