block: vpc - prevent overflow

PATCH Superseded Jeff Cody 2 participants 8 messages Jul 21, 2015 – Jul 22, 2015 Block layer

lore

Share & tools

lore

Jeff Cody

author

2015-07-21 16:13 UTC #1 lore raw

PATCH v2 1/2 block: vpc - prevent overflow if max_table_entries >= 0x40000000

In reply to Jeff Cody

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 <[email protected]>

Signed-off-by Jeff Cody <[email protected]>

---
 block/vpc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

block/vpc.c +13 -4

index 37572ba..6ef21e6 100644
@@ -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]);

Signature · 1 line

1.9.3

Jeff Cody

author

2015-07-21 16:13 UTC #0 lore raw

v2 cover block: vpc - prevent overflow

Changes:

v2 now also checks that Max Table Entries is < SIZE_MAX / 4 (Thanks Stefan)

This series fixes a bug found by Richard Jones.

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.

Jeff Cody (2):
block: vpc - prevent overflow if max_table_entries >= 0x40000000
block: qemu-iotests - add check for multiplication overflow in vpc

 block/vpc.c                                   |  17 ++++++--
 tests/qemu-iotests/135                        |  54 ++++++++++++++++++++++++++
 tests/qemu-iotests/135.out                    |   5 +++
 tests/qemu-iotests/group                      |   1 +
 tests/qemu-iotests/sample_images/afl5.img.bz2 | Bin 0 -> 175 bytes
 5 files changed, 73 insertions(+), 4 deletions(-)
 create mode 100755 tests/qemu-iotests/135
 create mode 100644 tests/qemu-iotests/135.out
 create mode 100644 tests/qemu-iotests/sample_images/afl5.img.bz2

Signature · 1 line

1.9.3

Jeff Cody

author

2015-07-21 16:13 UTC #7 lore raw

PATCH v2 2/2 block: qemu-iotests - add check for multiplication overflow in vpc

This checks that VPC is able to successfully fail (without segfault) on an image file with a max_table_entries that exceeds 0x40000000.

This table entry is within the valid range for VPC (although too large for this sample image).

Signed-off-by Jeff Cody <[email protected]>

---
 tests/qemu-iotests/135                        |  54 ++++++++++++++++++++++++++
 tests/qemu-iotests/135.out                    |   5 +++
 tests/qemu-iotests/group                      |   1 +
 tests/qemu-iotests/sample_images/afl5.img.bz2 | Bin 0 -> 175 bytes
 4 files changed, 60 insertions(+)
 create mode 100755 tests/qemu-iotests/135
 create mode 100644 tests/qemu-iotests/135.out
 create mode 100644 tests/qemu-iotests/sample_images/afl5.img.bz2

tests/qemu-iotests/135 new file +54 -0

new file mode 100755
index 0000000..16bf736
@@ -0,0 +1,54 @@
+#!/bin/bash
+#
+# Test VPC open of image with large Max Table Entries value.
+#
+# Copyright (C) 2015 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
[email protected]
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+
+_cleanup()
+{
+    _cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt vpc
+_supported_proto generic
+_supported_os Linux
+
+_use_sample_img afl5.img.bz2
+
+echo
+echo "=== Verify image open and failure ===="
+$QEMU_IMG info "$TEST_IMG" 2>&1| _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0

tests/qemu-iotests/135.out new file +5 -0

new file mode 100644
index 0000000..70b305e
@@ -0,0 +1,5 @@
+QA output created by 135
+
+=== Verify image open and failure ====
+qemu-img: Could not open 'TEST_DIR/afl5.img': block-vpc: free_data_block_offset points after the end of file. The image has been truncated.
+*** done

tests/qemu-iotests/group +1 -0

index 6206765..c430b6c 100644
@@ -133,3 +133,4 @@
 131 rw auto quick
 132 rw auto quick
 134 rw auto quick
+135 rw auto

tests/qemu-iotests/sample_images/afl5.img.bz2 new file binary +0 -0

new file mode 100644
index 0000000000000000000000000000000000000000..1614348865e5b2cfcb0340eab9474841717be2c5
GIT binary patch

Signature · 1 line

1.9.3

Max Reitz

2015-07-22 17:02 UTC #2 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries >= 0x40000000

In reply to Jeff Cody

Quoted 56 lines · Jeff Cody

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 <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
  block/vpc.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)
block/vpc.c +11 -1
index 37572ba..6ef21e6 100644
@@ -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;

block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 290

Quoted 6 lines

block/vpc.c +1 -2

@@ -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.

block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 296

Quoted 7 lines

block/vpc.c +1 -1

          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

Quoted 3 lines

block/vpc.c +0 -0

          for (i = 0; i < s->max_table_entries; i++) {
              be32_to_cpus(&s->pagetable[i]);

Jeff Cody

author

2015-07-22 17:26 UTC #3 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries >= 0x40000000

Quoted 58 lines · Max Reitz

Quoted 57 lines · Jeff Cody
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 <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
 block/vpc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
block/vpc.c +11 -1
index 37572ba..6ef21e6 100644
@@ -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,
 

Quoted 9 lines

block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 290
Quoted 5 lines
block/vpc.c +1 -2
         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.

Quoted 10 lines

block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 296
Quoted 7 lines
block/vpc.c +1 -1
         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 :)

Quoted 6 lines

Max

Quoted 3 lines

block/vpc.c +0 -0

         for (i = 0; i < s->max_table_entries; i++) {
             be32_to_cpus(&s->pagetable[i]);

Max Reitz

2015-07-22 17:30 UTC #4 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries >= 0x40000000

Quoted 69 lines · Jeff Cody

Quoted 58 lines · Max Reitz
Quoted 57 lines · Jeff Cody
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 <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
  block/vpc.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)
block/vpc.c +11 -1
index 37572ba..6ef21e6 100644
@@ -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,
 
Quoted 9 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 290
Quoted 5 lines
block/vpc.c +1 -2
          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.

Quoted 3 lines

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

Quoted 19 lines

Quoted 10 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 296
Quoted 7 lines
block/vpc.c +1 -1
          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 :)
Quoted 6 lines
Max
Quoted 3 lines
block/vpc.c +0 -0
          for (i = 0; i < s->max_table_entries; i++) {
              be32_to_cpus(&s->pagetable[i]);
 

Jeff Cody

author

2015-07-22 17:41 UTC #5 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries >= 0x40000000

Quoted 74 lines · Max Reitz

Quoted 69 lines · Jeff Cody
Quoted 58 lines · Max Reitz
Quoted 57 lines · Jeff Cody
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 <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
 block/vpc.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)
block/vpc.c +11 -1
index 37572ba..6ef21e6 100644
@@ -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,
 
Quoted 9 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 290
Quoted 5 lines
block/vpc.c +1 -2
         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.
Quoted 3 lines
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.

Quoted 5 lines

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.

Quoted 23 lines

Max
Quoted 19 lines
Quoted 10 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 296
Quoted 7 lines
block/vpc.c +1 -1
         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 :)
Quoted 6 lines
Max
Quoted 3 lines
block/vpc.c +0 -0
         for (i = 0; i < s->max_table_entries; i++) {
             be32_to_cpus(&s->pagetable[i]);
 

Max Reitz

2015-07-22 18:01 UTC #6 lore raw

Re: [Qemu-devel] [Qemu-block] [PATCH v2 1/2] block: vpc - prevent overflow if max_table_entries >= 0x40000000

Quoted 81 lines · Jeff Cody

Quoted 74 lines · Max Reitz
Quoted 69 lines · Jeff Cody
Quoted 58 lines · Max Reitz
Quoted 57 lines · Jeff Cody
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 <[email protected]>
Signed-off-by Jeff Cody <[email protected]>
---
  block/vpc.c | 17 +++++++++++++----
  1 file changed, 13 insertions(+), 4 deletions(-)
block/vpc.c +11 -1
index 37572ba..6ef21e6 100644
@@ -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,
 
Quoted 9 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 290
Quoted 5 lines
block/vpc.c +1 -2
          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.
Quoted 3 lines
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.
Quoted 5 lines
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.

Quoted 4 lines

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

Quoted 21 lines

Quoted 20 lines
Quoted 19 lines
Quoted 10 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 296
Quoted 7 lines
block/vpc.c +1 -1
          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 :)
Quoted 6 lines
Max
Quoted 3 lines
block/vpc.c +0 -0
          for (i = 0; i < s->max_table_entries; i++) {
              be32_to_cpus(&s->pagetable[i]);
 