| Message ID | 4273ff9b577ffc2b2b3ff7137beabfb831a38d65.1435175476.git.jcody@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Jun 24, 2015 at 03:54:27PM -0400, Jeff Cody wrote: > @@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > goto fail; > } > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > + pagetable_size = (size_t) s->max_table_entries * 4; > + > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); On 32-bit hosts size_t is 32-bit so the overflow hasn't been solved. Does it make sense to impose a limit on pagetable_size?
On Thu, Jun 25, 2015 at 03:28:35PM +0100, Stefan Hajnoczi wrote: > On Wed, Jun 24, 2015 at 03:54:27PM -0400, Jeff Cody wrote: > > @@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > goto fail; > > } > > > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > + pagetable_size = (size_t) s->max_table_entries * 4; > > + > > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > > On 32-bit hosts size_t is 32-bit so the overflow hasn't been solved. > > Does it make sense to impose a limit on pagetable_size? Good point. Yes, it does. The VHD spec says that the "Max Table Entries" field should be equal to the disk size / block size. I don't know if there are images out there that treat that as ">= disk size / block size" rather than "==", however. But if we assume max size of 2TB for a VHD disk, and a minimal block size of 512 bytes, that would give us a max_table_entries of 0x100000000, which exceeds 32 bits by itself. For pagetable_size to fit in a 32-bit, that means to support 2TB on a 32-bit host in the current implementation, the minimum block size is 4096. We could check during open / create that (disk_size / block_size) * 4 < SIZE_MAX, and refuse to open if this is not true (and also validate max_table_entries to fit in the same). It should be noted that the default (per spec) block size for VHD is 2MB, so it may not be too limiting in practice for a 32-bit host to not be able to open all image sizes / block size combinations.
On Thu, Jun 25, 2015 at 11:05:20AM -0400, Jeff Cody wrote: > On Thu, Jun 25, 2015 at 03:28:35PM +0100, Stefan Hajnoczi wrote: > > On Wed, Jun 24, 2015 at 03:54:27PM -0400, Jeff Cody wrote: > > > @@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > > goto fail; > > > } > > > > > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > > + pagetable_size = (size_t) s->max_table_entries * 4; > > > + > > > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > > > > On 32-bit hosts size_t is 32-bit so the overflow hasn't been solved. > > > > Does it make sense to impose a limit on pagetable_size? > > Good point. Yes, it does. > > The VHD spec says that the "Max Table Entries" field should be equal > to the disk size / block size. I don't know if there are images out > there that treat that as ">= disk size / block size" rather than "==", > however. But if we assume max size of 2TB for a VHD disk, and a > minimal block size of 512 bytes, that would give us a > max_table_entries of 0x100000000, which exceeds 32 bits by itself. > > For pagetable_size to fit in a 32-bit, that means to support 2TB on a > 32-bit host in the current implementation, the minimum block size is > 4096. > > We could check during open / create that > (disk_size / block_size) * 4 < SIZE_MAX, and refuse to open if this is > not true (and also validate max_table_entries to fit in the same). Sounds good.
Am 26.06.2015 um 11:57 hat Stefan Hajnoczi geschrieben: > On Thu, Jun 25, 2015 at 11:05:20AM -0400, Jeff Cody wrote: > > On Thu, Jun 25, 2015 at 03:28:35PM +0100, Stefan Hajnoczi wrote: > > > On Wed, Jun 24, 2015 at 03:54:27PM -0400, Jeff Cody wrote: > > > > @@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > > > goto fail; > > > > } > > > > > > > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > > > + pagetable_size = (size_t) s->max_table_entries * 4; > > > > + > > > > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > > > > > > On 32-bit hosts size_t is 32-bit so the overflow hasn't been solved. > > > > > > Does it make sense to impose a limit on pagetable_size? > > > > Good point. Yes, it does. > > > > The VHD spec says that the "Max Table Entries" field should be equal > > to the disk size / block size. I don't know if there are images out > > there that treat that as ">= disk size / block size" rather than "==", > > however. But if we assume max size of 2TB for a VHD disk, and a > > minimal block size of 512 bytes, that would give us a > > max_table_entries of 0x100000000, which exceeds 32 bits by itself. > > > > For pagetable_size to fit in a 32-bit, that means to support 2TB on a > > 32-bit host in the current implementation, the minimum block size is > > 4096. > > > > We could check during open / create that > > (disk_size / block_size) * 4 < SIZE_MAX, and refuse to open if this is > > not true (and also validate max_table_entries to fit in the same). > > Sounds good. Jeff, I couldn't find a v2 anywhere. Are you still planning to send it? Kevin
On Wed, Jul 08, 2015 at 01:55:34PM +0200, Kevin Wolf wrote: > Am 26.06.2015 um 11:57 hat Stefan Hajnoczi geschrieben: > > On Thu, Jun 25, 2015 at 11:05:20AM -0400, Jeff Cody wrote: > > > On Thu, Jun 25, 2015 at 03:28:35PM +0100, Stefan Hajnoczi wrote: > > > > On Wed, Jun 24, 2015 at 03:54:27PM -0400, Jeff Cody wrote: > > > > > @@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, > > > > > goto fail; > > > > > } > > > > > > > > > > - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); > > > > > + pagetable_size = (size_t) s->max_table_entries * 4; > > > > > + > > > > > + s->pagetable = qemu_try_blockalign(bs->file, pagetable_size); > > > > > > > > On 32-bit hosts size_t is 32-bit so the overflow hasn't been solved. > > > > > > > > Does it make sense to impose a limit on pagetable_size? > > > > > > Good point. Yes, it does. > > > > > > The VHD spec says that the "Max Table Entries" field should be equal > > > to the disk size / block size. I don't know if there are images out > > > there that treat that as ">= disk size / block size" rather than "==", > > > however. But if we assume max size of 2TB for a VHD disk, and a > > > minimal block size of 512 bytes, that would give us a > > > max_table_entries of 0x100000000, which exceeds 32 bits by itself. > > > > > > For pagetable_size to fit in a 32-bit, that means to support 2TB on a > > > 32-bit host in the current implementation, the minimum block size is > > > 4096. > > > > > > We could check during open / create that > > > (disk_size / block_size) * 4 < SIZE_MAX, and refuse to open if this is > > > not true (and also validate max_table_entries to fit in the same). > > > > Sounds good. > > Jeff, I couldn't find a v2 anywhere. Are you still planning to send it? Jeff is on vacation. I think he's back next week.
diff --git a/block/vpc.c b/block/vpc.c index 37572ba..4108b5e 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,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } - s->pagetable = qemu_try_blockalign(bs->file, s->max_table_entries * 4); + pagetable_size = (size_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 +280,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. Reported-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Jeff Cody <jcody@redhat.com> --- block/vpc.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)