| Message ID | 38895e508e45514fbafafe437e25375b66731942.1437495012.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 21.07.2015 18:13, Jeff Cody wrote: > When we allocate the pagetable based on max_table_entries, we multiply > the max table entry value by 4 to accomodate a table of 32-bit integers. > However, max_table_entries is a uint32_t, and the VPC driver accepts > ranges for that entry over 0x40000000. So during this allocation: > > s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > The size arg overflows, allocating significantly less memory than > expected. > > Since qemu_try_blockalign() size argument is size_t, cast the > multiplication correctly to prevent overflow. > > The value of "max_table_entries * 4" is used elsewhere in the code as > well, so store the correct value for use in all those cases. > > We also check the Max Tables Entries value, to make sure that it is < > SIZE_MAX / 4, so we know the pagetable size will fit in size_t. > > Reported-by: Richard W.M. Jones <rjones@redhat.com> > Signed-off-by: Jeff Cody <jcody@redhat.com> > --- > block/vpc.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/block/vpc.c b/block/vpc.c > index 37572ba..6ef21e6 100644 > --- a/block/vpc.c > +++ b/block/vpc.c > @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > uint8_t buf[HEADER_SIZE]; > uint32_t checksum; > uint64_t computed_size; > + uint64_t pagetable_size; > int disk_type = VHD_DYNAMIC; > int ret; > > @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > + if (s->max_table_entries > SIZE_MAX / 4) { > + error_setg(errp, "Max Table Entries too large (%" PRId32 ")", > + s->max_table_entries); > + ret = -EINVAL; > + goto fail; > + } > + > + pagetable_size = (uint64_t) s->max_table_entries * 4; > + > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > if (s->pagetable == NULL) { > ret = -ENOMEM; > goto fail; > @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); > > - ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, > - s->max_table_entries * 4); > + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); The last parameter of bdrv_pread() is an int, so this will still overflow if s->max_table_entries > INT_MAX / 4. > if (ret < 0) { > goto fail; > } > > s->free_data_block_offset = > - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; > + (s->bat_offset + pagetable_size + 511) & ~511; Not necessary, but perhaps we should be using ROUND_UP() here... Max > for (i = 0; i < s->max_table_entries; i++) { > be32_to_cpus(&s->pagetable[i]); >
On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote: > On 21.07.2015 18:13, Jeff Cody wrote: > >When we allocate the pagetable based on max_table_entries, we multiply > >the max table entry value by 4 to accomodate a table of 32-bit integers. > >However, max_table_entries is a uint32_t, and the VPC driver accepts > >ranges for that entry over 0x40000000. So during this allocation: > > > >s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > > >The size arg overflows, allocating significantly less memory than > >expected. > > > >Since qemu_try_blockalign() size argument is size_t, cast the > >multiplication correctly to prevent overflow. > > > >The value of "max_table_entries * 4" is used elsewhere in the code as > >well, so store the correct value for use in all those cases. > > > >We also check the Max Tables Entries value, to make sure that it is < > >SIZE_MAX / 4, so we know the pagetable size will fit in size_t. > > > >Reported-by: Richard W.M. Jones <rjones@redhat.com> > >Signed-off-by: Jeff Cody <jcody@redhat.com> > >--- > > block/vpc.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > >diff --git a/block/vpc.c b/block/vpc.c > >index 37572ba..6ef21e6 100644 > >--- a/block/vpc.c > >+++ b/block/vpc.c > >@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > uint8_t buf[HEADER_SIZE]; > > uint32_t checksum; > > uint64_t computed_size; > >+ uint64_t pagetable_size; > > int disk_type = VHD_DYNAMIC; > > int ret; > > > >@@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > goto fail; > > } > > > >- s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > >+ if (s->max_table_entries > SIZE_MAX / 4) { > >+ error_setg(errp, "Max Table Entries too large (%" PRId32 ")", > >+ s->max_table_entries); > >+ ret = -EINVAL; > >+ goto fail; > >+ } > >+ > >+ pagetable_size = (uint64_t) s->max_table_entries * 4; > >+ > >+ s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > > if (s->pagetable == NULL) { > > ret = -ENOMEM; > > goto fail; > >@@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > > > s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); > > > >- ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, > >- s->max_table_entries * 4); > >+ ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); > > The last parameter of bdrv_pread() is an int, so this will still > overflow if s->max_table_entries > INT_MAX / 4. > I thought about checking for INT_MAX overflow as well - I probably should. I wasn't too concerned about it, however, as bdrv_read() will return -EINVAL for nb_sectors < 0. > > if (ret < 0) { > > goto fail; > > } > > > > s->free_data_block_offset = > >- (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; > >+ (s->bat_offset + pagetable_size + 511) & ~511; > > Not necessary, but perhaps we should be using ROUND_UP() here... > Sure, why not :) > Max > > > for (i = 0; i < s->max_table_entries; i++) { > > be32_to_cpus(&s->pagetable[i]); > > >
On 22.07.2015 19:26, Jeff Cody wrote: > On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote: >> On 21.07.2015 18:13, Jeff Cody wrote: >>> When we allocate the pagetable based on max_table_entries, we multiply >>> the max table entry value by 4 to accomodate a table of 32-bit integers. >>> However, max_table_entries is a uint32_t, and the VPC driver accepts >>> ranges for that entry over 0x40000000. So during this allocation: >>> >>> s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); >>> >>> The size arg overflows, allocating significantly less memory than >>> expected. >>> >>> Since qemu_try_blockalign() size argument is size_t, cast the >>> multiplication correctly to prevent overflow. >>> >>> The value of "max_table_entries * 4" is used elsewhere in the code as >>> well, so store the correct value for use in all those cases. >>> >>> We also check the Max Tables Entries value, to make sure that it is < >>> SIZE_MAX / 4, so we know the pagetable size will fit in size_t. >>> >>> Reported-by: Richard W.M. Jones <rjones@redhat.com> >>> Signed-off-by: Jeff Cody <jcody@redhat.com> >>> --- >>> block/vpc.c | 17 +++++++++++++---- >>> 1 file changed, 13 insertions(+), 4 deletions(-) >>> >>> diff --git a/block/vpc.c b/block/vpc.c >>> index 37572ba..6ef21e6 100644 >>> --- a/block/vpc.c >>> +++ b/block/vpc.c >>> @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>> uint8_t buf[HEADER_SIZE]; >>> uint32_t checksum; >>> uint64_t computed_size; >>> + uint64_t pagetable_size; >>> int disk_type = VHD_DYNAMIC; >>> int ret; >>> >>> @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>> goto fail; >>> } >>> >>> - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); >>> + if (s->max_table_entries > SIZE_MAX / 4) { >>> + error_setg(errp, "Max Table Entries too large (%" PRId32 ")", >>> + s->max_table_entries); >>> + ret = -EINVAL; >>> + goto fail; >>> + } >>> + >>> + pagetable_size = (uint64_t) s->max_table_entries * 4; >>> + >>> + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); >>> if (s->pagetable == NULL) { >>> ret = -ENOMEM; >>> goto fail; >>> @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>> >>> s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); >>> >>> - ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, >>> - s->max_table_entries * 4); >>> + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); >> >> The last parameter of bdrv_pread() is an int, so this will still >> overflow if s->max_table_entries > INT_MAX / 4. >> > > I thought about checking for INT_MAX overflow as well - I probably > should. I wasn't too concerned about it, however, as bdrv_read() will > return -EINVAL for nb_sectors < 0. Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit systems, pagetable_size might become large enough for (int)pagetable_size to be positive, with (int)pagetable_size != pagetable_size, which would make bdrv_pread() work just fine, but only read a part of the table. Max >>> if (ret < 0) { >>> goto fail; >>> } >>> >>> s->free_data_block_offset = >>> - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; >>> + (s->bat_offset + pagetable_size + 511) & ~511; >> >> Not necessary, but perhaps we should be using ROUND_UP() here... >> > > Sure, why not :) > >> Max >> >>> for (i = 0; i < s->max_table_entries; i++) { >>> be32_to_cpus(&s->pagetable[i]); >>> >>
On Wed, Jul 22, 2015 at 07:29:47PM +0200, Max Reitz wrote: > On 22.07.2015 19:26, Jeff Cody wrote: > >On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote: > >>On 21.07.2015 18:13, Jeff Cody wrote: > >>>When we allocate the pagetable based on max_table_entries, we multiply > >>>the max table entry value by 4 to accomodate a table of 32-bit integers. > >>>However, max_table_entries is a uint32_t, and the VPC driver accepts > >>>ranges for that entry over 0x40000000. So during this allocation: > >>> > >>>s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > >>> > >>>The size arg overflows, allocating significantly less memory than > >>>expected. > >>> > >>>Since qemu_try_blockalign() size argument is size_t, cast the > >>>multiplication correctly to prevent overflow. > >>> > >>>The value of "max_table_entries * 4" is used elsewhere in the code as > >>>well, so store the correct value for use in all those cases. > >>> > >>>We also check the Max Tables Entries value, to make sure that it is < > >>>SIZE_MAX / 4, so we know the pagetable size will fit in size_t. > >>> > >>>Reported-by: Richard W.M. Jones <rjones@redhat.com> > >>>Signed-off-by: Jeff Cody <jcody@redhat.com> > >>>--- > >>> block/vpc.c | 17 +++++++++++++---- > >>> 1 file changed, 13 insertions(+), 4 deletions(-) > >>> > >>>diff --git a/block/vpc.c b/block/vpc.c > >>>index 37572ba..6ef21e6 100644 > >>>--- a/block/vpc.c > >>>+++ b/block/vpc.c > >>>@@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > >>> uint8_t buf[HEADER_SIZE]; > >>> uint32_t checksum; > >>> uint64_t computed_size; > >>>+ uint64_t pagetable_size; > >>> int disk_type = VHD_DYNAMIC; > >>> int ret; > >>> > >>>@@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > >>> goto fail; > >>> } > >>> > >>>- s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > >>>+ if (s->max_table_entries > SIZE_MAX / 4) { > >>>+ error_setg(errp, "Max Table Entries too large (%" PRId32 ")", > >>>+ s->max_table_entries); > >>>+ ret = -EINVAL; > >>>+ goto fail; > >>>+ } > >>>+ > >>>+ pagetable_size = (uint64_t) s->max_table_entries * 4; > >>>+ > >>>+ s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > >>> if (s->pagetable == NULL) { > >>> ret = -ENOMEM; > >>> goto fail; > >>>@@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > >>> > >>> s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); > >>> > >>>- ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, > >>>- s->max_table_entries * 4); > >>>+ ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); > >> > >>The last parameter of bdrv_pread() is an int, so this will still > >>overflow if s->max_table_entries > INT_MAX / 4. > >> > > > >I thought about checking for INT_MAX overflow as well - I probably > >should. I wasn't too concerned about it, however, as bdrv_read() will > >return -EINVAL for nb_sectors < 0. > > Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit > systems, pagetable_size might become large enough for > (int)pagetable_size to be positive, with (int)pagetable_size != > pagetable_size, which would make bdrv_pread() work just fine, but > only read a part of the table. The maximum size pagetable_size can be is UINT32_MAX * 4, because max_table_entries is a 32-bit value from the VHD header. But either way, I'll add a check for INT_MAX, as that really is the correct (and clearest) way to handle it. > > Max > > >>> if (ret < 0) { > >>> goto fail; > >>> } > >>> > >>> s->free_data_block_offset = > >>>- (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; > >>>+ (s->bat_offset + pagetable_size + 511) & ~511; > >> > >>Not necessary, but perhaps we should be using ROUND_UP() here... > >> > > > >Sure, why not :) > > > >>Max > >> > >>> for (i = 0; i < s->max_table_entries; i++) { > >>> be32_to_cpus(&s->pagetable[i]); > >>> > >> >
On 22.07.2015 19:40, Jeff Cody wrote: > On Wed, Jul 22, 2015 at 07:29:47PM +0200, Max Reitz wrote: >> On 22.07.2015 19:26, Jeff Cody wrote: >>> On Wed, Jul 22, 2015 at 07:02:02PM +0200, Max Reitz wrote: >>>> On 21.07.2015 18:13, Jeff Cody wrote: >>>>> When we allocate the pagetable based on max_table_entries, we multiply >>>>> the max table entry value by 4 to accomodate a table of 32-bit integers. >>>>> However, max_table_entries is a uint32_t, and the VPC driver accepts >>>>> ranges for that entry over 0x40000000. So during this allocation: >>>>> >>>>> s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); >>>>> >>>>> The size arg overflows, allocating significantly less memory than >>>>> expected. >>>>> >>>>> Since qemu_try_blockalign() size argument is size_t, cast the >>>>> multiplication correctly to prevent overflow. >>>>> >>>>> The value of "max_table_entries * 4" is used elsewhere in the code as >>>>> well, so store the correct value for use in all those cases. >>>>> >>>>> We also check the Max Tables Entries value, to make sure that it is < >>>>> SIZE_MAX / 4, so we know the pagetable size will fit in size_t. >>>>> >>>>> Reported-by: Richard W.M. Jones <rjones@redhat.com> >>>>> Signed-off-by: Jeff Cody <jcody@redhat.com> >>>>> --- >>>>> block/vpc.c | 17 +++++++++++++---- >>>>> 1 file changed, 13 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/block/vpc.c b/block/vpc.c >>>>> index 37572ba..6ef21e6 100644 >>>>> --- a/block/vpc.c >>>>> +++ b/block/vpc.c >>>>> @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>>>> uint8_t buf[HEADER_SIZE]; >>>>> uint32_t checksum; >>>>> uint64_t computed_size; >>>>> + uint64_t pagetable_size; >>>>> int disk_type = VHD_DYNAMIC; >>>>> int ret; >>>>> >>>>> @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>>>> goto fail; >>>>> } >>>>> >>>>> - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); >>>>> + if (s->max_table_entries > SIZE_MAX / 4) { >>>>> + error_setg(errp, "Max Table Entries too large (%" PRId32 ")", >>>>> + s->max_table_entries); >>>>> + ret = -EINVAL; >>>>> + goto fail; >>>>> + } >>>>> + >>>>> + pagetable_size = (uint64_t) s->max_table_entries * 4; >>>>> + >>>>> + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); >>>>> if (s->pagetable == NULL) { >>>>> ret = -ENOMEM; >>>>> goto fail; >>>>> @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, >>>>> >>>>> s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); >>>>> >>>>> - ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, >>>>> - s->max_table_entries * 4); >>>>> + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); >>>> >>>> The last parameter of bdrv_pread() is an int, so this will still >>>> overflow if s->max_table_entries > INT_MAX / 4. >>>> >>> >>> I thought about checking for INT_MAX overflow as well - I probably >>> should. I wasn't too concerned about it, however, as bdrv_read() will >>> return -EINVAL for nb_sectors < 0. >> >> Right, but SIZE_MAX may be larger than 2ull * INT_MAX, so on 64 bit >> systems, pagetable_size might become large enough for >> (int)pagetable_size to be positive, with (int)pagetable_size != >> pagetable_size, which would make bdrv_pread() work just fine, but >> only read a part of the table. > > The maximum size pagetable_size can be is UINT32_MAX * 4, because > max_table_entries is a 32-bit value from the VHD header. But either > way, I'll add a check for INT_MAX, as that really is the correct (and > clearest) way to handle it. Ah, right. Too obvious for me to see, I guess. *cough* Max >>>>> if (ret < 0) { >>>>> goto fail; >>>>> } >>>>> >>>>> s->free_data_block_offset = >>>>> - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; >>>>> + (s->bat_offset + pagetable_size + 511) & ~511; >>>> >>>> Not necessary, but perhaps we should be using ROUND_UP() here... >>>> >>> >>> Sure, why not :) >>> >>>> Max >>>> >>>>> for (i = 0; i < s->max_table_entries; i++) { >>>>> be32_to_cpus(&s->pagetable[i]); >>>>> >>>> >> >
diff --git a/block/vpc.c b/block/vpc.c index 37572ba..6ef21e6 100644 --- a/block/vpc.c +++ b/block/vpc.c @@ -168,6 +168,7 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, uint8_t buf[HEADER_SIZE]; uint32_t checksum; uint64_t computed_size; + uint64_t pagetable_size; int disk_type = VHD_DYNAMIC; int ret; @@ -269,7 +270,16 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); + if (s->max_table_entries > SIZE_MAX / 4) { + error_setg(errp, "Max Table Entries too large (%" PRId32 ")", + s->max_table_entries); + ret = -EINVAL; + goto fail; + } + + pagetable_size = (uint64_t) s->max_table_entries * 4; + + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); if (s->pagetable == NULL) { ret = -ENOMEM; goto fail; @@ -277,14 +287,13 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, s->bat_offset = be64_to_cpu(dyndisk_header->table_offset); - ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, - s->max_table_entries * 4); + ret = bdrv_pread(bs->file, s->bat_offset, s->pagetable, pagetable_size); if (ret < 0) { goto fail; } s->free_data_block_offset = - (s->bat_offset + (s->max_table_entries * 4) + 511) & ~511; + (s->bat_offset + pagetable_size + 511) & ~511; for (i = 0; i < s->max_table_entries; i++) { be32_to_cpus(&s->pagetable[i]);
When we allocate the pagetable based on max_table_entries, we multiply the max table entry value by 4 to accomodate a table of 32-bit integers. However, max_table_entries is a uint32_t, and the VPC driver accepts ranges for that entry over 0x40000000. So during this allocation: s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); The size arg overflows, allocating significantly less memory than expected. Since qemu_try_blockalign() size argument is size_t, cast the multiplication correctly to prevent overflow. The value of "max_table_entries * 4" is used elsewhere in the code as well, so store the correct value for use in all those cases. We also check the Max Tables Entries value, to make sure that it is < SIZE_MAX / 4, so we know the pagetable size will fit in size_t. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/vpc.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)