Re: [Qemu-devel] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1947 bytes --]

On 31.03.2017 15:13, Peter Maydell wrote:
> Coverity (CID 1307776) points out that in the multiply:
>   space = to_allocate * s->tracks;
> we are trying to calculate a 64 bit result but the types
> of to_allocate and s->tracks mean that we actually calculate
> a 32 bit result. Add an explicit cast to force a 64 bit
> multiply.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> NB: compile-and-make-check tested only...
> ---
>  block/parallels.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/parallels.c b/block/parallels.c
> index 4173b3f..3886c30 100644
> --- a/block/parallels.c
> +++ b/block/parallels.c
> @@ -206,7 +206,7 @@ static int64_t allocate_clusters(BlockDriverState *bs, int64_t sector_num,
>      }
>  
>      to_allocate = DIV_ROUND_UP(sector_num + *pnum, s->tracks) - idx;
> -    space = to_allocate * s->tracks;
> +    space = (int64_t)to_allocate * s->tracks;
>      if (s->data_end + space > bdrv_getlength(bs->file->bs) >> BDRV_SECTOR_BITS) {
>          int ret;
>          space += s->prealloc_size;

I think the division is technically fine because to_allocate will
roughly be *pnum / s->tracks (and since *pnum is an int, the
multiplication cannot overflow).

However, it's still good to fix this, but I would do it differently:
Make idx, to_allocate, and i all uint64_t or int64_t instead of
uint32_t. This would also prevent accidental overflow when storing the
result of the division in:

idx = sector_num / s->tracks;
if (idx >= s->bat_size) {
    [...]

The much greater problem to me appears to be that we don't check that
idx + to_allocate <= s->bat_size. I'm not sure whether there can be a
buffer overflow in the for loop below, but I'm not sure I really want to
know either... I think the block_status() call limits *pnum so that
there will not be an overflow, but then we should at least assert this.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]