[Qemu-devel] [PATCH v3 0/2] block: vpc - prevent overflow

[Qemu-devel] [PATCH v3 0/2] block: vpc - prevent overflow

[Jeff Cody]

Changes:

v3: check for INT_MAX / 4 as well, and use ROUND_UP (Thanks Max)

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

This series fixes a bug found by Richard Jones.

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.

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                                   |  18 +++++++--
 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, 74 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

-- 
1.9.3

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

[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 <rjones@redhat.com>
Signed-off-by: Jeff Cody <jcody@redhat.com>
---
 block/vpc.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/block/vpc.c b/block/vpc.c
index 37572ba..3e385d9 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,17 @@ 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 ||
+            s->max_table_entries > (int) INT_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 +288,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;
+            ROUND_UP(s->bat_offset + pagetable_size, 512);
 
         for (i = 0; i < s->max_table_entries; i++) {
             be32_to_cpus(&s->pagetable[i]);
-- 
1.9.3

[Qemu-devel] [PATCH v3 2/2] block: qemu-iotests - add check for multiplication overflow in vpc

[Jeff Cody]

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 <jcody@redhat.com>
---
 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

diff --git a/tests/qemu-iotests/135 b/tests/qemu-iotests/135
new file mode 100755
index 0000000..16bf736
--- /dev/null
+++ b/tests/qemu-iotests/135
@@ -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
+owner=jcody@redhat.com
+
+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
diff --git a/tests/qemu-iotests/135.out b/tests/qemu-iotests/135.out
new file mode 100644
index 0000000..793898b
--- /dev/null
+++ b/tests/qemu-iotests/135.out
@@ -0,0 +1,5 @@
+QA output created by 135
+
+=== Verify image open and failure ====
+qemu-img: Could not open 'TEST_DIR/afl5.img': Max Table Entries too large (1073741825)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index 6206765..c430b6c 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -133,3 +133,4 @@
 131 rw auto quick
 132 rw auto quick
 134 rw auto quick
+135 rw auto
diff --git a/tests/qemu-iotests/sample_images/afl5.img.bz2 b/tests/qemu-iotests/sample_images/afl5.img.bz2
new file mode 100644
index 0000000000000000000000000000000000000000..1614348865e5b2cfcb0340eab9474841717be2c5
GIT binary patch
literal 175
zcmV;g08sxzT4*^jL0KkKSqT!KVgLXwfB*jgAVdfNFaTf(B!Frw|3pDR00;sy03ZSY
z3IG5B1Sp^YbSh$=r=c<!nr#TgFeYj0XnKQ9RLxB?V^M@KMwpn4hJ!z<OVwhn^RnWP
zlo2h)1BC$~JU|O6a~hBm5oG|a2~t!d6NaCTwor7>gVwVQS9W$Kd2?dJ>H~Ej+J=Q^
dtom#MI_=bg;S5HeF^MqnF64@Ep&$|^KE!naKsf*a

literal 0
HcmV?d00001

-- 
1.9.3

Re: [Qemu-devel] [PATCH v3 0/2] block: vpc - prevent overflow

[Kevin Wolf]

Am 24.07.2015 um 16:26 hat Jeff Cody geschrieben:
> Changes:
> 
> v3: check for INT_MAX / 4 as well, and use ROUND_UP (Thanks Max)
> 
> v2 now also checks that Max Table Entries is < SIZE_MAX / 4 (Thanks Stefan)
> 
> This series fixes a bug found by Richard Jones.
> 
> 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.

Thanks, applied to my block branch for 2.4.

When posting patches that are meant for qemu-stable, it's best to not
only copy the mailing list for the emails, but to have an explicit
"Cc: qemu-stable@nongnu.org" in the commit message. I added the tag
for this series while applying.

Kevin