| Message ID | 20201109150522.10350-1-peter.maydell@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-5.2] block/export/vhost-user-blk-server.c: Avoid potential integer overflow | expand |
[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
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;
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(-)