In vu_blk_discard_write_zeroes(), we read a 32-bit sector count from
the descriptor and convert it to a 64-bit byte count. Coverity warns
that the left shift is done with 32-bit arithmetic so it might
overflow before the conversion to 64-bit happens. Add a cast to
avoid this.

Fixes: Coverity CID 1435956
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Tested with 'make check' and 'make check-acceptance' only.
---
 block/export/vhost-user-blk-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 62672d1cb95..e5749451e65 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -70,7 +70,7 @@ vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov,
     }
 
     uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,
-                          le32_to_cpu(desc.num_sectors) << 9 };
+                          (uint64_t)le32_to_cpu(desc.num_sectors) << 9 };
     if (type == VIRTIO_BLK_T_DISCARD) {
         if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
             return 0;
-- 
2.20.1

[Cc-ing Stefan]

On 09.11.20 16:05, Peter Maydell wrote:
> In vu_blk_discard_write_zeroes(), we read a 32-bit sector count from
> the descriptor and convert it to a 64-bit byte count. Coverity warns
> that the left shift is done with 32-bit arithmetic so it might
> overflow before the conversion to 64-bit happens. Add a cast to
> avoid this.

This will silence Coverity, but both functions to which range[1] is then 
passed (blk_co_pdiscard() and blk_co_pwrite_zeroes()) only accept ints 
there, so this would only move the overflow to the function call.

Shouldn’t we verify that the number of sectors is in range and return an 
error if it isn’t?  (The same probably goes for the starting sector, 
then, too.)

Max

> Fixes: Coverity CID 1435956
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Tested with 'make check' and 'make check-acceptance' only.
> ---
>   block/export/vhost-user-blk-server.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 62672d1cb95..e5749451e65 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -70,7 +70,7 @@ vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec *iov,
>       }
>   
>       uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,
> -                          le32_to_cpu(desc.num_sectors) << 9 };
> +                          (uint64_t)le32_to_cpu(desc.num_sectors) << 9 };
>       if (type == VIRTIO_BLK_T_DISCARD) {
>           if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
>               return 0;
> 

On Mon, Nov 09, 2020 at 04:16:45PM +0100, Max Reitz wrote:
> [Cc-ing Stefan]
> 
> On 09.11.20 16:05, Peter Maydell wrote:
> > In vu_blk_discard_write_zeroes(), we read a 32-bit sector count from
> > the descriptor and convert it to a 64-bit byte count. Coverity warns
> > that the left shift is done with 32-bit arithmetic so it might
> > overflow before the conversion to 64-bit happens. Add a cast to
> > avoid this.
> 
> This will silence Coverity, but both functions to which range[1] is then
> passed (blk_co_pdiscard() and blk_co_pwrite_zeroes()) only accept ints
> there, so this would only move the overflow to the function call.
> 
> Shouldn’t we verify that the number of sectors is in range and return an
> error if it isn’t?  (The same probably goes for the starting sector, then,
> too.)

Yes, the input validation from hw/block/virtio-blk.c is missing.

I'll send a patch to add that.

Stefan