[Qemu-devel] [PATCH 0/2] qcow2: Do not overflow when writing an L1 sector

[Qemu-devel] [PATCH 0/2] qcow2: Do not overflow when writing an L1 sector

[Max Reitz]

qcow2_write_l1_entry() may read L1 entries from beyond the end of the
in-memory L1 table when updating a sector. Fix this and add a
qemu-iotest.


Max Reitz (2):
  qcow2: Do not overflow when writing an L1 sector
  iotests: Add test for qcow2 L1 table update

 block/qcow2-cluster.c      |  6 +++--
 tests/qemu-iotests/107     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/107.out | 10 ++++++++
 tests/qemu-iotests/group   |  1 +
 4 files changed, 76 insertions(+), 2 deletions(-)
 create mode 100755 tests/qemu-iotests/107
 create mode 100644 tests/qemu-iotests/107.out

-- 
2.1.2

[Qemu-devel] [PATCH 1/2] qcow2: Do not overflow when writing an L1 sector

[Max Reitz]

While writing an L1 table sector, qcow2_write_l1_entry() copies the
respective range from s->l1_table to the local "buf" array. The size of
s->l1_table does not have to be a multiple of L1_ENTRIES_PER_SECTOR;
thus, limit the index which is used for copying all entries to the L1
size.

Cc: qemu-stable@nongnu.org
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-cluster.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f7dd8c0..4d888c7 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -164,12 +164,14 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
 int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
 {
     BDRVQcowState *s = bs->opaque;
-    uint64_t buf[L1_ENTRIES_PER_SECTOR];
+    uint64_t buf[L1_ENTRIES_PER_SECTOR] = { 0 };
     int l1_start_index;
     int i, ret;
 
     l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);
-    for (i = 0; i < L1_ENTRIES_PER_SECTOR; i++) {
+    for (i = 0; i < L1_ENTRIES_PER_SECTOR && l1_start_index + i < s->l1_size;
+         i++)
+    {
         buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
     }
 
-- 
2.1.2

[Qemu-devel] [PATCH 2/2] iotests: Add test for qcow2 L1 table update

[Max Reitz]

Updating the L1 table should not result in random data being written.
This adds a test for that.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/107     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/107.out | 10 ++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 72 insertions(+)
 create mode 100755 tests/qemu-iotests/107
 create mode 100644 tests/qemu-iotests/107.out

diff --git a/tests/qemu-iotests/107 b/tests/qemu-iotests/107
new file mode 100755
index 0000000..cad1cf9
--- /dev/null
+++ b/tests/qemu-iotests/107
@@ -0,0 +1,61 @@
+#!/bin/bash
+#
+# Tests updates of the qcow2 L1 table
+#
+# Copyright (C) 2014 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=mreitz@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 qcow2
+_supported_proto file
+_supported_os Linux
+
+
+IMG_SIZE=64K
+
+echo
+echo '=== Updates should not write random data ==='
+echo
+
+_make_test_img $IMG_SIZE
+$QEMU_IO -c 'write 0 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c "open -o driver=raw $TEST_IMG" -c 'read -p -P 0 196616 65528' \
+    | _filter_qemu_io
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
+
diff --git a/tests/qemu-iotests/107.out b/tests/qemu-iotests/107.out
new file mode 100644
index 0000000..93445b7
--- /dev/null
+++ b/tests/qemu-iotests/107.out
@@ -0,0 +1,10 @@
+QA output created by 107
+
+=== Updates should not write random data ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=65536 
+wrote 65536/65536 bytes at offset 0
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 65528/65528 bytes at offset 196616
+63.992 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+*** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index b230996..4a10fef 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -106,3 +106,4 @@
 103 rw auto quick
 104 rw auto
 105 rw auto quick
+107 rw auto quick
-- 
2.1.2

Re: [Qemu-devel] [PATCH 0/2] qcow2: Do not overflow when writing an L1 sector

[Eric Blake]

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

On 10/16/2014 07:25 AM, Max Reitz wrote:
> qcow2_write_l1_entry() may read L1 entries from beyond the end of the
> in-memory L1 table when updating a sector. Fix this and add a
> qemu-iotest.
> 
> 
> Max Reitz (2):
>   qcow2: Do not overflow when writing an L1 sector
>   iotests: Add test for qcow2 L1 table update

Series:
Reviewed-by: Eric Blake <eblake@redhat.com>

> 
>  block/qcow2-cluster.c      |  6 +++--
>  tests/qemu-iotests/107     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/107.out | 10 ++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 76 insertions(+), 2 deletions(-)
>  create mode 100755 tests/qemu-iotests/107
>  create mode 100644 tests/qemu-iotests/107.out
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH 1/2] qcow2: Do not overflow when writing an L1 sector

[Peter Lieven]

On 16.10.2014 15:25, Max Reitz wrote:
> While writing an L1 table sector, qcow2_write_l1_entry() copies the
> respective range from s->l1_table to the local "buf" array. The size of
> s->l1_table does not have to be a multiple of L1_ENTRIES_PER_SECTOR;
> thus, limit the index which is used for copying all entries to the L1
> size.
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/qcow2-cluster.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f7dd8c0..4d888c7 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -164,12 +164,14 @@ static int l2_load(BlockDriverState *bs, uint64_t l2_offset,
>   int qcow2_write_l1_entry(BlockDriverState *bs, int l1_index)
>   {
>       BDRVQcowState *s = bs->opaque;
> -    uint64_t buf[L1_ENTRIES_PER_SECTOR];
> +    uint64_t buf[L1_ENTRIES_PER_SECTOR] = { 0 };
>       int l1_start_index;
>       int i, ret;
>   
>       l1_start_index = l1_index & ~(L1_ENTRIES_PER_SECTOR - 1);
> -    for (i = 0; i < L1_ENTRIES_PER_SECTOR; i++) {
> +    for (i = 0; i < L1_ENTRIES_PER_SECTOR && l1_start_index + i < s->l1_size;
> +         i++)
> +    {
>           buf[i] = cpu_to_be64(s->l1_table[l1_start_index + i]);
>       }
>   

Reviewed-by: Peter Lieven <pl@kamp.de>

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for qcow2 L1 table update

[Peter Lieven]

On 16.10.2014 15:25, Max Reitz wrote:
> Updating the L1 table should not result in random data being written.
> This adds a test for that.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/107     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/107.out | 10 ++++++++
>   tests/qemu-iotests/group   |  1 +
>   3 files changed, 72 insertions(+)
>   create mode 100755 tests/qemu-iotests/107
>   create mode 100644 tests/qemu-iotests/107.out
>
> diff --git a/tests/qemu-iotests/107 b/tests/qemu-iotests/107
> new file mode 100755
> index 0000000..cad1cf9
> --- /dev/null
> +++ b/tests/qemu-iotests/107
> @@ -0,0 +1,61 @@
> +#!/bin/bash
> +#
> +# Tests updates of the qcow2 L1 table
> +#
> +# Copyright (C) 2014 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=mreitz@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 qcow2
> +_supported_proto file

This (and maybe other recently added tests) also works on NFS.
As NFS on QCOW2 might be a reasonable combination I would add it.

Peter

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for qcow2 L1 table update

[Max Reitz]

On 2014-10-20 at 08:25, Peter Lieven wrote:
> On 16.10.2014 15:25, Max Reitz wrote:
>> Updating the L1 table should not result in random data being written.
>> This adds a test for that.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/107     | 61 
>> ++++++++++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/107.out | 10 ++++++++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 72 insertions(+)
>>   create mode 100755 tests/qemu-iotests/107
>>   create mode 100644 tests/qemu-iotests/107.out
>>
>> diff --git a/tests/qemu-iotests/107 b/tests/qemu-iotests/107
>> new file mode 100755
>> index 0000000..cad1cf9
>> --- /dev/null
>> +++ b/tests/qemu-iotests/107
>> @@ -0,0 +1,61 @@
>> +#!/bin/bash
>> +#
>> +# Tests updates of the qcow2 L1 table
>> +#
>> +# Copyright (C) 2014 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=mreitz@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 qcow2
>> +_supported_proto file
>
> This (and maybe other recently added tests) also works on NFS.
> As NFS on QCOW2 might be a reasonable combination I would add it.

It probably works over some other protocols as well. I think we want to 
go through all the tests in a separate series and fix all of these 
cases. I'll fix it in a v2 if I need to do a v2, if not, I'll leave it 
as-is. If you want to test qcow2, you want to run all tests over file, 
probably. If you want to test nfs, you probably don't want to use qcow2 
but raw (because many of the currently existing tests are too limited in 
their format and protocol choices).

Since you suggested looking into which tests actually support more 
formats and protocols than they currently pretend to, feel free to send 
such a series. ;-)

Max

Re: [Qemu-devel] [PATCH 2/2] iotests: Add test for qcow2 L1 table update

[Peter Lieven]

On 20.10.2014 11:09, Max Reitz wrote:
> On 2014-10-20 at 08:25, Peter Lieven wrote:
>> On 16.10.2014 15:25, Max Reitz wrote:
>>> Updating the L1 table should not result in random data being written.
>>> This adds a test for that.
>>>
>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>> ---
>>>   tests/qemu-iotests/107     | 61 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   tests/qemu-iotests/107.out | 10 ++++++++
>>>   tests/qemu-iotests/group   |  1 +
>>>   3 files changed, 72 insertions(+)
>>>   create mode 100755 tests/qemu-iotests/107
>>>   create mode 100644 tests/qemu-iotests/107.out
>>>
>>> diff --git a/tests/qemu-iotests/107 b/tests/qemu-iotests/107
>>> new file mode 100755
>>> index 0000000..cad1cf9
>>> --- /dev/null
>>> +++ b/tests/qemu-iotests/107
>>> @@ -0,0 +1,61 @@
>>> +#!/bin/bash
>>> +#
>>> +# Tests updates of the qcow2 L1 table
>>> +#
>>> +# Copyright (C) 2014 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=mreitz@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 qcow2
>>> +_supported_proto file
>>
>> This (and maybe other recently added tests) also works on NFS.
>> As NFS on QCOW2 might be a reasonable combination I would add it.
>
> It probably works over some other protocols as well. I think we want to go through all the tests in a separate series and fix all of these cases. I'll fix it in a v2 if I need to do a v2, if not, I'll leave it as-is. If you want to test qcow2, you want 
> to run all tests over file, probably. If you want to test nfs, you probably don't want to use qcow2 but raw (because many of the currently existing tests are too limited in their format and protocol choices).
>
> Since you suggested looking into which tests actually support more formats and protocols than they currently pretend to, feel free to send such a series. ;-)

I actually did when writing the NFS driver. I did modifications to a lot of tests to support other protocols. But the approach was too hackish. But I would like to see at least
the tests that work without modifications to have supported protocols listed. And as someone likely will use a QCOW2 container on NFS I think is is an important
test case. At least to have all the tests included that are working out of the box.

Peter

Re: [Qemu-devel] [PATCH 0/2] qcow2: Do not overflow when writing an L1 sector

[Kevin Wolf]

Am 16.10.2014 um 15:25 hat Max Reitz geschrieben:
> qcow2_write_l1_entry() may read L1 entries from beyond the end of the
> in-memory L1 table when updating a sector. Fix this and add a
> qemu-iotest.

Thanks, applied to the block branch.

Kevin