[Qemu-devel] [PATCH 0/2] qed: additional input validation

[Qemu-devel] [PATCH 0/2] qed: additional input validation

[Stefan Hajnoczi]

These patches add an overflow check and a test case for invalid QED headers.
Note that this has no security impact because reading the backing filename is
limited to sizeof(bs->backing_file).

Stefan Hajnoczi (2):
  qed: check for header size overflow
  qemu-iotests: add 116 invalid QED input file tests

 block/qed.c                |  6 +++
 tests/qemu-iotests/116     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/116.out | 37 ++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 140 insertions(+)
 create mode 100755 tests/qemu-iotests/116
 create mode 100644 tests/qemu-iotests/116.out

-- 
2.1.0

[Qemu-devel] [PATCH 1/2] qed: check for header size overflow

[Stefan Hajnoczi]

Header size is denoted in clusters.  The maximum cluster size is 64 MB
but there is no limit on header size.  Check for uint32_t overflow in
case the header size field has a whacky value.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qed.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/qed.c b/block/qed.c
index 80f18d8..d2c6045 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -440,6 +440,12 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
     s->l2_mask = s->table_nelems - 1;
     s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
 
+    /* Header size calculation must not overflow uint32_t */
+    if ((uint64_t)s->header.cluster_size * s->header.header_size !=
+        s->header.cluster_size * s->header.header_size) {
+        return -EINVAL;
+    }
+
     if ((s->header.features & QED_F_BACKING_FILE)) {
         if ((uint64_t)s->header.backing_filename_offset +
             s->header.backing_filename_size >
-- 
2.1.0

[Qemu-devel] [PATCH 2/2] qemu-iotests: add 116 invalid QED input file tests

[Stefan Hajnoczi]

These tests exercise error code paths in the QED image format.  The
tests are very simple, they just prove that the error path exits
cleanly.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 tests/qemu-iotests/116     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/116.out | 37 ++++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 134 insertions(+)
 create mode 100755 tests/qemu-iotests/116
 create mode 100644 tests/qemu-iotests/116.out

diff --git a/tests/qemu-iotests/116 b/tests/qemu-iotests/116
new file mode 100755
index 0000000..713ed48
--- /dev/null
+++ b/tests/qemu-iotests/116
@@ -0,0 +1,96 @@
+#!/bin/bash
+#
+# Test error code paths for invalid QED images
+#
+# The aim of this test is to exercise the error paths in qed_open() to ensure
+# there are no crashes with invalid input files.
+#
+# 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=stefanha@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 qed
+_supported_proto generic
+_supported_os Linux
+
+
+size=128M
+
+echo
+echo "== truncated header cluster =="
+_make_test_img $size
+truncate -s 512 "$TEST_IMG"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid header magic =="
+_make_test_img $size
+poke_file "$TEST_IMG" "0" "QEDX"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid cluster size =="
+_make_test_img $size
+poke_file "$TEST_IMG" "4" "\xff\xff\xff\xff"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid table size =="
+_make_test_img $size
+poke_file "$TEST_IMG" "8" "\xff\xff\xff\xff"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid header size =="
+_make_test_img $size
+poke_file "$TEST_IMG" "12" "\xff\xff\xff\xff"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid L1 table offset =="
+_make_test_img $size
+poke_file "$TEST_IMG" "40" "\xff\xff\xff\xff\xff\xff\xff\xff"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+echo
+echo "== invalid image size =="
+_make_test_img $size
+poke_file "$TEST_IMG" "48" "\xff\xff\xff\xff\xff\xff\xff\xff"
+$QEMU_IO -f "$IMGFMT" -c "read 0 $size" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/116.out b/tests/qemu-iotests/116.out
new file mode 100644
index 0000000..b679cee
--- /dev/null
+++ b/tests/qemu-iotests/116.out
@@ -0,0 +1,37 @@
+QA output created by 116
+
+== truncated header cluster ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+
+== invalid header magic ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Image not in QED format
+no file open, try 'help open'
+
+== invalid cluster size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+
+== invalid table size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+
+== invalid header size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+
+== invalid L1 table offset ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+
+== invalid image size ==
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728
+qemu-io: can't open device TEST_DIR/t.qed: Could not open 'TEST_DIR/t.qed': Invalid argument
+no file open, try 'help open'
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index a4742c6..5543c1e 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -115,3 +115,4 @@
 111 rw auto quick
 113 rw auto quick
 114 rw auto quick
+116 rw auto quick
-- 
2.1.0

Re: [Qemu-devel] [PATCH 1/2] qed: check for header size overflow

[Kevin Wolf]

Am 12.01.2015 um 13:31 hat Stefan Hajnoczi geschrieben:
> Header size is denoted in clusters.  The maximum cluster size is 64 MB
> but there is no limit on header size.  Check for uint32_t overflow in
> case the header size field has a whacky value.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  block/qed.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/block/qed.c b/block/qed.c
> index 80f18d8..d2c6045 100644
> --- a/block/qed.c
> +++ b/block/qed.c
> @@ -440,6 +440,12 @@ static int bdrv_qed_open(BlockDriverState *bs, QDict *options, int flags,
>      s->l2_mask = s->table_nelems - 1;
>      s->l1_shift = s->l2_shift + ffs(s->table_nelems) - 1;
>  
> +    /* Header size calculation must not overflow uint32_t */
> +    if ((uint64_t)s->header.cluster_size * s->header.header_size !=
> +        s->header.cluster_size * s->header.header_size) {

Generally, I find checks that don't exercise the integer overflow easier
to read, i.e. in this case:

    if (s->header.header_size > UINT32_MAX / s->header.cluster_size)

Or even just INT_MAX to be on the safe side. But I'll admit that it's a
matter of taste.

> +        return -EINVAL;
> +    }
> +
>      if ((s->header.features & QED_F_BACKING_FILE)) {
>          if ((uint64_t)s->header.backing_filename_offset +
>              s->header.backing_filename_size >

Series: Reviewed-by: Kevin Wolf <kwolf@redhat.com>

Re: [Qemu-devel] [PATCH 0/2] qed: additional input validation

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 974 bytes --]

On Mon, Jan 12, 2015 at 12:31:31PM +0000, Stefan Hajnoczi wrote:
> These patches add an overflow check and a test case for invalid QED headers.
> Note that this has no security impact because reading the backing filename is
> limited to sizeof(bs->backing_file).
> 
> Stefan Hajnoczi (2):
>   qed: check for header size overflow
>   qemu-iotests: add 116 invalid QED input file tests
> 
>  block/qed.c                |  6 +++
>  tests/qemu-iotests/116     | 96 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/116.out | 37 ++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 140 insertions(+)
>  create mode 100755 tests/qemu-iotests/116
>  create mode 100644 tests/qemu-iotests/116.out

Kevin: Thanks for the style suggestion, I have applied your tweak.  It
does read clearer when the expression checks UINT32_MAX.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]