block: vpc - prevent overflow

PATCH Superseded Jeff Cody 3 participants 8 messages Jun 24, 2015 – Jul 9, 2015 Block layer

lore

Share & tools

lore

Jeff Cody

author

2015-06-24 19:54 UTC #0 lore raw

cover block: vpc - prevent overflow

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                                   |  10 +++--
 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, 66 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-06-24 19:54 UTC #1 lore raw

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

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

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

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

block/vpc.c +6 -4

index 37572ba..4108b5e 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,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]);

Signature · 1 line

1.9.3

Stefan Hajnoczi

2015-06-25 14:28 UTC #2 lore raw

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

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

Quoted 8 lines · Jeff Cody

block/vpc.c +3 -1

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

Jeff Cody

author

2015-06-25 15:05 UTC #3 lore raw

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

Quoted 5 lines · Stefan Hajnoczi

Quoted 4 lines · Jeff Cody

block/vpc.c +0 -0

@@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }

Quoted 8 lines

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

Stefan Hajnoczi

2015-06-26 09:57 UTC #4 lore raw

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

Quoted 24 lines · Jeff Cody

Quoted 5 lines · Stefan Hajnoczi
Quoted 4 lines · Jeff Cody
block/vpc.c +0 -0
@@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
 
Quoted 8 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 275
Quoted 4 lines
block/vpc.c +3 -1
-        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.

Quoted 7 lines

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.

Kevin Wolf

2015-07-08 11:55 UTC #5 lore raw

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

Am 26.06.2015 um 11:57 hat Stefan Hajnoczi geschrieben:

Quoted 29 lines · Jeff Cody

Quoted 28 lines · Stefan Hajnoczi
Quoted 5 lines · Jeff Cody
Quoted 4 lines
block/vpc.c +0 -0
@@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
 
Quoted 8 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 275
Quoted 4 lines
block/vpc.c +3 -1
-        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.

Quoted 5 lines

Quoted 3 lines
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

Stefan Hajnoczi

2015-07-09 10:12 UTC #6 lore raw

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

Quoted 30 lines · Kevin Wolf

Am 26.06.2015 um 11:57 hat Stefan Hajnoczi geschrieben:
Quoted 29 lines · Jeff Cody
Quoted 28 lines · Stefan Hajnoczi
Quoted 5 lines · Jeff Cody
Quoted 4 lines
block/vpc.c +0 -0
@@ -269,7 +270,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
             goto fail;
         }
 
Quoted 8 lines
block/vpc.c static int vpc_open(BlockDriverState *bs, QDict *options, int flags, · line 275
Quoted 4 lines
block/vpc.c +3 -1
-        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.

Quoted 7 lines

Quoted 5 lines
Quoted 3 lines
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.

Jeff Cody

author

2015-06-24 19:54 UTC #7 lore raw

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

In reply to 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 <[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 4597fc1..557e2a2 100644
@@ -132,3 +132,4 @@
 130 rw auto quick
 131 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