[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in al
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-block] [PATCH] block/parallels.c: avoid integer overflow in allocate_clusters() |
Date: |
Fri, 31 Mar 2017 17:20:43 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
On Fri, Mar 31, 2017 at 03:47:39PM +0200, Max Reitz wrote:
> 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 <address@hidden>
> > ---
> > 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.
Will you send a new patch that supercedes this one?
Stefan
signature.asc
Description: PGP signature