[Qemu-devel] [PATCH v3 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Qemu-devel] [PATCH v3 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

Commit 8ac0f15f335 accidently broke the COW of non changed areas
of newly allocated clusters, when the write spans multiple clusters,
and needs COW both prior and after the write.
This results in 'after' COW area being encrypted with wrong
sector address, which render it corrupted.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922

CC: qemu-stable <qemu-stable@nongnu.org>

V2: grammar, spelling and code style fixes.
V3: more fixes after the review.

Best regards,
	Maxim Levitsky

Maxim Levitsky (3):
  block/qcow2: refactoring of threaded encryption code
  block/qcow2: fix the corruption when rebasing luks encrypted files
  qemu-iotests: Add test for bz #1745922

 block/qcow2-cluster.c      | 29 +++++++-----
 block/qcow2-threads.c      | 63 ++++++++++++++++++++------
 tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 40 +++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 199 insertions(+), 25 deletions(-)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

-- 
2.17.2

[Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code

[Maxim Levitsky]

This commit tries to clarify few function arguments,
and add comments describing the encrypt/decrypt interface

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c |  8 +++---
 block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f09cc992af..b95e64c237 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 }
 
 static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t src_cluster_offset,
-                                                uint64_t cluster_offset,
+                                                uint64_t guest_cluster_offset,
+                                                uint64_t host_cluster_offset,
                                                 unsigned offset_in_cluster,
                                                 uint8_t *buffer,
                                                 unsigned bytes)
@@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
         assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs, cluster_offset,
-                             src_cluster_offset + offset_in_cluster,
+        if (qcow2_co_encrypt(bs, host_cluster_offset,
+                             guest_cluster_offset + offset_in_cluster,
                              buffer, bytes) < 0) {
             return false;
         }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..6da1838e95 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -234,15 +234,19 @@ static int qcow2_encdec_pool_func(void *opaque)
 }
 
 static int coroutine_fn
-qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
-                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
+qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
+                uint64_t guest_offset, void *buf, size_t len,
+                Qcow2EncDecFunc func)
 {
     BDRVQcow2State *s = bs->opaque;
+
+    uint64_t offset = s->crypt_physical_offset ?
+        host_cluster_offset + offset_into_cluster(s, guest_offset) :
+        guest_offset;
+
     Qcow2EncDecData arg = {
         .block = s->crypto,
-        .offset = s->crypt_physical_offset ?
-                      file_cluster_offset + offset_into_cluster(s, offset) :
-                      offset,
+        .offset = offset,
         .buf = buf,
         .len = len,
         .func = func,
@@ -251,18 +255,51 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
     return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
 }
 
+
+/*
+ * qcow2_co_encrypt()
+ *
+ * Encrypts one or more contiguous aligned sectors
+ *
+ * @host_cluster_offset - underlying storage offset of the first cluster
+ * in which the encrypted data will be written
+ * Used as an initialization vector for encryption
+ *
+ * @guest_offset - guest (virtual) offset of the first sector of the
+ * data to be encrypted
+ * Used as an initialization vector for older, qcow2 native encryption
+ *
+ * @buf - buffer with the data to encrypt, that after encryption
+ *        will be written to the underlying storage device at
+ *        @host_cluster_offset
+ *
+ * @len - length of the buffer (in sector size multiplies)
+ *
+ * Note that the group of the sectors, don't have to be aligned
+ * on cluster boundary and can also cross a cluster boundary.
+ *
+ */
 int coroutine_fn
-qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_encrypt);
+    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
+                           qcrypto_block_encrypt);
 }
 
+
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts one or more contiguous aligned sectors
+ * Similar to qcow2_co_encrypt
+ *
+ */
+
 int coroutine_fn
-qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
-                 uint64_t offset, void *buf, size_t len)
+qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
+                 uint64_t guest_offset, void *buf, size_t len)
 {
-    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
-                             qcrypto_block_decrypt);
+    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
+                           qcrypto_block_decrypt);
 }
-- 
2.17.2

[Qemu-devel] [PATCH v3 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files

[Maxim Levitsky]

This fixes subtle corruption introduced by luks threaded encryption
in commit 8ac0f15f335

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922

The corruption happens when we do a write that
   * writes to two or more unallocated clusters at once
   * doesn't fully cover the first sector
   * doesn't fully cover the last sector

In this case, when allocating the new clusters we COW both areas
prior to the write and after the write, and we encrypt them.

The above mentioned commit accidentally made it so we encrypt the
second COW area using the physical cluster offset of the first area.

Fix this by:
 * Remove the offset_in_cluster parameter of do_perform_cow_encrypt,
   since it is misleading. That offset can be larger than cluster size
   currently.

   Instead just add the start and the end COW area offsets to both host
   and guest offsets that do_perform_cow_encrypt receives.

*  in do_perform_cow_encrypt, remove the cluster offset from the host_offset,
   and thus pass correctly to the qcow2_co_encrypt, the host cluster offset
   and full guest offset

In the bugreport that was triggered by rebasing a luks image to new,
zero filled base, which lot of such writes, and causes some files
with zero areas to contain garbage there instead.
But as described above it can happen elsewhere as well


Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-cluster.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b95e64c237..7203d4cb85 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,20 +463,21 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
 }
 
 static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
-                                                uint64_t guest_cluster_offset,
-                                                uint64_t host_cluster_offset,
-                                                unsigned offset_in_cluster,
+                                                uint64_t guest_offset,
+                                                uint64_t host_offset,
                                                 uint8_t *buffer,
                                                 unsigned bytes)
 {
     if (bytes && bs->encrypted) {
         BDRVQcow2State *s = bs->opaque;
-        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
-        assert((bytes & ~BDRV_SECTOR_MASK) == 0);
+
+        assert(QEMU_IS_ALIGNED(guest_offset, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(host_offset, BDRV_SECTOR_SIZE));
+        assert(QEMU_IS_ALIGNED(bytes, BDRV_SECTOR_SIZE));
         assert(s->crypto);
-        if (qcow2_co_encrypt(bs, host_cluster_offset,
-                             guest_cluster_offset + offset_in_cluster,
-                             buffer, bytes) < 0) {
+
+        if (qcow2_co_encrypt(bs, start_of_cluster(s, host_offset),
+                             guest_offset, buffer, bytes) < 0) {
             return false;
         }
     }
@@ -890,11 +891,15 @@ static int perform_cow(BlockDriverState *bs, QCowL2Meta *m)
 
     /* Encrypt the data if necessary before writing it */
     if (bs->encrypted) {
-        if (!do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    start->offset, start_buffer,
+        if (!do_perform_cow_encrypt(bs,
+                                    m->offset + start->offset,
+                                    m->alloc_offset + start->offset,
+                                    start_buffer,
                                     start->nb_bytes) ||
-            !do_perform_cow_encrypt(bs, m->offset, m->alloc_offset,
-                                    end->offset, end_buffer, end->nb_bytes)) {
+            !do_perform_cow_encrypt(bs,
+                                    m->offset + end->offset,
+                                    m->alloc_offset + end->offset,
+                                    end_buffer, end->nb_bytes)) {
             ret = -EIO;
             goto fail;
         }
-- 
2.17.2

[Qemu-devel] [PATCH v3 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 40 +++++++++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 132 insertions(+)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
new file mode 100755
index 0000000000..afbd668cda
--- /dev/null
+++ b/tests/qemu-iotests/263
@@ -0,0 +1,91 @@
+#!/usr/bin/env bash
+#
+# Test encrypted write that crosses cluster boundary of two unallocated clusters
+# Based on 188
+#
+# Copyright (C) 2019 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=mlevitsk@redhat.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+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 generic
+_supported_os Linux
+
+
+size=1M
+
+SECRET="secret,id=sec0,data=astrochicken"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+
+_run_test()
+{
+	echo "== reading the whole image =="
+	$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $1 | _filter_qemu_io | _filter_testdir
+
+	echo
+	echo "== write two 512 byte sectors on a cluster boundary =="
+	$QEMU_IO --object $SECRET -c "write -P 0xAA 0xFE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
+
+	echo
+	echo "== verify that the rest of the image is not changed =="
+	$QEMU_IO --object $SECRET -c "read -P 0x00 0x00000 0xFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir
+	$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
+	$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir
+
+}
+
+
+echo
+echo "testing LUKS qcow2 encryption"
+echo
+
+_make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+echo
+echo "testing legacy AES qcow2 encryption"
+echo
+
+
+_make_test_img --object $SECRET -o "encrypt.format=aes,encrypt.key-secret=sec0,cluster_size=64K" $size
+_run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+_cleanup_test_img
+
+
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/263.out b/tests/qemu-iotests/263.out
new file mode 100644
index 0000000000..0c982c55cb
--- /dev/null
+++ b/tests/qemu-iotests/263.out
@@ -0,0 +1,40 @@
+QA output created by 263
+
+testing LUKS qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 982528/982528 bytes at offset 66048
+959.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+testing legacy AES qcow2 encryption
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=aes encrypt.key-secret=sec0
+== reading the whole image ==
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== write two 512 byte sectors on a cluster boundary ==
+wrote 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+== verify that the rest of the image is not changed ==
+read 65024/65024 bytes at offset 0
+63.500 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 1024/1024 bytes at offset 65024
+1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 982528/982528 bytes at offset 66048
+959.500 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 d95d556414..be1c4a3baa 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -274,3 +274,4 @@
 257 rw
 258 rw quick
 262 rw quick migration
+263 rw quick
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 7146 bytes --]

On 13.09.19 00:37, Maxim Levitsky wrote:
> This commit tries to clarify few function arguments,
> and add comments describing the encrypt/decrypt interface
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2-cluster.c |  8 +++---
>  block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 54 insertions(+), 17 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f09cc992af..b95e64c237 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
>  }
>  
>  static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> -                                                uint64_t src_cluster_offset,
> -                                                uint64_t cluster_offset,
> +                                                uint64_t guest_cluster_offset,
> +                                                uint64_t host_cluster_offset,
>                                                  unsigned offset_in_cluster,
>                                                  uint8_t *buffer,
>                                                  unsigned bytes)
> @@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
>          assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
>          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
>          assert(s->crypto);
> -        if (qcow2_co_encrypt(bs, cluster_offset,
> -                             src_cluster_offset + offset_in_cluster,
> +        if (qcow2_co_encrypt(bs, host_cluster_offset,
> +                             guest_cluster_offset + offset_in_cluster,
>                               buffer, bytes) < 0) {
>              return false;
>          }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..6da1838e95 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -234,15 +234,19 @@ static int qcow2_encdec_pool_func(void *opaque)
>  }
>  
>  static int coroutine_fn
> -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> +                uint64_t guest_offset, void *buf, size_t len,
> +                Qcow2EncDecFunc func)
>  {
>      BDRVQcow2State *s = bs->opaque;
> +
> +    uint64_t offset = s->crypt_physical_offset ?
> +        host_cluster_offset + offset_into_cluster(s, guest_offset) :
> +        guest_offset;
> +
>      Qcow2EncDecData arg = {
>          .block = s->crypto,
> -        .offset = s->crypt_physical_offset ?
> -                      file_cluster_offset + offset_into_cluster(s, offset) :
> -                      offset,
> +        .offset = offset,
>          .buf = buf,
>          .len = len,
>          .func = func,
> @@ -251,18 +255,51 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
>      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
>  }
>  
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts one or more contiguous aligned sectors
> + *
> + * @host_cluster_offset - underlying storage offset of the first cluster
> + * in which the encrypted data will be written
> + * Used as an initialization vector for encryption

s/as an/for generating the/

(AFAIU)

> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted
> + * Used as an initialization vector for older, qcow2 native encryption

I wouldn’t be so specific here.  I think it’d be better to just have a
common sentence like “Depending on the encryption method, either of
these offsets may be used for generating the initialization vector for
encryption.”

Well, technically, host_cluster_offset will not be used itself.  Before
we can use it, of course we have to add the in-cluster offset to it
(which qcow2_co_encdec() does).

This makes me wonder whether it wouldn’t make more sense to pass a
host_offset instead of a host_cluster_offset (i.e. make the callers add
the in-cluster offset to the host offset already)?

> + *
> + * @buf - buffer with the data to encrypt, that after encryption
> + *        will be written to the underlying storage device at
> + *        @host_cluster_offset

I think just “buffer with the data to encrypt” is sufficient.  (The rest
is just the same as above.)

> + *
> + * @len - length of the buffer (in sector size multiplies)

“In sector size multiples” to me means that it is a sector count (in
that “one sector size multiple” is equal to 512 byes).

Maybe “must be a BDRV_SECTOR_SIZE multiple” instead?

> + *
> + * Note that the group of the sectors, don't have to be aligned
> + * on cluster boundary and can also cross a cluster boundary.

Maybe “Note that while the whole range must be aligned on sectors, it
does not have to be aligned on clusters and can also cross cluster
boundaries”?

(“The group of sectors” sounds a bit weird to me.  I don’t quite know,
why.  I think that for some reason it makes me think of a non-continuous
set of sectors.  Also, the caller doesn’t pass sector indices, but byte
offsets, that just happen to have to be aligned to sectors.  (I suppose
because that’s the simplest way to make it a multiple of the encryption
block size.))

> + *
> + */
>  int coroutine_fn
> -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>  {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_encrypt);
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> +                           qcrypto_block_encrypt);
>  }
>  
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts one or more contiguous aligned sectors
> + * Similar to qcow2_co_encrypt

Hm.  This would make me wonder in what way it is only similar to
qcow2_co_encrypt().  Sure, it decrypts instead of encrypting, but maybe
there’s more...?

So maybe “Its interface is the same as qcow2_co_encrypt()'s”?

Max

> + *
> + */
> +
>  int coroutine_fn
> -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> -                 uint64_t offset, void *buf, size_t len)
> +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> +                 uint64_t guest_offset, void *buf, size_t len)
>  {
> -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> -                             qcrypto_block_decrypt);
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> +                           qcrypto_block_decrypt);
>  }
> 



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

Re: [Qemu-devel] [PATCH v3 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1767 bytes --]

On 13.09.19 00:37, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do a write that
>    * writes to two or more unallocated clusters at once
>    * doesn't fully cover the first sector
>    * doesn't fully cover the last sector
> 
> In this case, when allocating the new clusters we COW both areas
> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidentally made it so we encrypt the
> second COW area using the physical cluster offset of the first area.
> 
> Fix this by:
>  * Remove the offset_in_cluster parameter of do_perform_cow_encrypt,
>    since it is misleading. That offset can be larger than cluster size
>    currently.
> 
>    Instead just add the start and the end COW area offsets to both host
>    and guest offsets that do_perform_cow_encrypt receives.
> 
> *  in do_perform_cow_encrypt, remove the cluster offset from the host_offset,
>    and thus pass correctly to the qcow2_co_encrypt, the host cluster offset
>    and full guest offset
> 
> In the bugreport that was triggered by rebasing a luks image to new,
> zero filled base, which lot of such writes, and causes some files
> with zero areas to contain garbage there instead.
> But as described above it can happen elsewhere as well
> 
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-cluster.c | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


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

Re: [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Add test for bz #1745922

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2950 bytes --]

On 13.09.19 00:37, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/263.out | 40 +++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 132 insertions(+)
>  create mode 100755 tests/qemu-iotests/263
>  create mode 100644 tests/qemu-iotests/263.out
> 
> diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
> new file mode 100755
> index 0000000000..afbd668cda
> --- /dev/null
> +++ b/tests/qemu-iotests/263
> @@ -0,0 +1,91 @@
> +#!/usr/bin/env bash
> +#
> +# Test encrypted write that crosses cluster boundary of two unallocated clusters
> +# Based on 188
> +#
> +# Copyright (C) 2019 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=mlevitsk@redhat.com
> +
> +seq=`basename $0`
> +echo "QA output created by $seq"
> +
> +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 generic
> +_supported_os Linux
> +
> +
> +size=1M
> +
> +SECRET="secret,id=sec0,data=astrochicken"
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> +
> +
> +_run_test()
> +{
> +	echo "== reading the whole image =="
> +	$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $1 | _filter_qemu_io | _filter_testdir
> +
> +	echo
> +	echo "== write two 512 byte sectors on a cluster boundary =="
> +	$QEMU_IO --object $SECRET -c "write -P 0xAA 0xFE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
> +
> +	echo
> +	echo "== verify that the rest of the image is not changed =="
> +	$QEMU_IO --object $SECRET -c "read -P 0x00 0x00000 0xFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir
> +	$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
> +	$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir

Looks good to me overall, but in case you respin, I don’t think there’s
reason not to quote the $1.

Max


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

Re: [Qemu-devel] [PATCH v3 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

On Fri, 2019-09-13 at 13:01 +0200, Max Reitz wrote:
> On 13.09.19 00:37, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  tests/qemu-iotests/263     | 91 ++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/263.out | 40 +++++++++++++++++
> >  tests/qemu-iotests/group   |  1 +
> >  3 files changed, 132 insertions(+)
> >  create mode 100755 tests/qemu-iotests/263
> >  create mode 100644 tests/qemu-iotests/263.out
> > 
> > diff --git a/tests/qemu-iotests/263 b/tests/qemu-iotests/263
> > new file mode 100755
> > index 0000000000..afbd668cda
> > --- /dev/null
> > +++ b/tests/qemu-iotests/263
> > @@ -0,0 +1,91 @@
> > +#!/usr/bin/env bash
> > +#
> > +# Test encrypted write that crosses cluster boundary of two unallocated clusters
> > +# Based on 188
> > +#
> > +# Copyright (C) 2019 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=mlevitsk@redhat.com
> > +
> > +seq=`basename $0`
> > +echo "QA output created by $seq"
> > +
> > +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 generic
> > +_supported_os Linux
> > +
> > +
> > +size=1M
> > +
> > +SECRET="secret,id=sec0,data=astrochicken"
> > +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> > +
> > +
> > +_run_test()
> > +{
> > +	echo "== reading the whole image =="
> > +	$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $1 | _filter_qemu_io | _filter_testdir
> > +
> > +	echo
> > +	echo "== write two 512 byte sectors on a cluster boundary =="
> > +	$QEMU_IO --object $SECRET -c "write -P 0xAA 0xFE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
> > +
> > +	echo
> > +	echo "== verify that the rest of the image is not changed =="
> > +	$QEMU_IO --object $SECRET -c "read -P 0x00 0x00000 0xFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir
> > +	$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $1 | _filter_qemu_io | _filter_testdir
> > +	$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $1 | _filter_qemu_io | _filter_testdir
> 
> Looks good to me overall, but in case you respin, I don’t think there’s
> reason not to quote the $1.
> 
> Max
> 
Fixed, good idea.

Best regards,
	Maxim Levitsky

Re: [Qemu-devel] [PATCH v3 1/3] block/qcow2: refactoring of threaded encryption code

[Maxim Levitsky]

On Fri, 2019-09-13 at 12:37 +0200, Max Reitz wrote:
> On 13.09.19 00:37, Maxim Levitsky wrote:
> > This commit tries to clarify few function arguments,
> > and add comments describing the encrypt/decrypt interface
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/qcow2-cluster.c |  8 +++---
> >  block/qcow2-threads.c | 63 ++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 54 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index f09cc992af..b95e64c237 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -463,8 +463,8 @@ static int coroutine_fn do_perform_cow_read(BlockDriverState *bs,
> >  }
> >  
> >  static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> > -                                                uint64_t src_cluster_offset,
> > -                                                uint64_t cluster_offset,
> > +                                                uint64_t guest_cluster_offset,
> > +                                                uint64_t host_cluster_offset,
> >                                                  unsigned offset_in_cluster,
> >                                                  uint8_t *buffer,
> >                                                  unsigned bytes)
> > @@ -474,8 +474,8 @@ static bool coroutine_fn do_perform_cow_encrypt(BlockDriverState *bs,
> >          assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> >          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> >          assert(s->crypto);
> > -        if (qcow2_co_encrypt(bs, cluster_offset,
> > -                             src_cluster_offset + offset_in_cluster,
> > +        if (qcow2_co_encrypt(bs, host_cluster_offset,
> > +                             guest_cluster_offset + offset_in_cluster,
> >                               buffer, bytes) < 0) {
> >              return false;
> >          }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..6da1838e95 100644
> > --- a/block/qcow2-threads.c
> > +++ b/block/qcow2-threads.c
> > @@ -234,15 +234,19 @@ static int qcow2_encdec_pool_func(void *opaque)
> >  }
> >  
> >  static int coroutine_fn
> > -qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                  uint64_t offset, void *buf, size_t len, Qcow2EncDecFunc func)
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> > +                uint64_t guest_offset, void *buf, size_t len,
> > +                Qcow2EncDecFunc func)
> >  {
> >      BDRVQcow2State *s = bs->opaque;
> > +
> > +    uint64_t offset = s->crypt_physical_offset ?
> > +        host_cluster_offset + offset_into_cluster(s, guest_offset) :
> > +        guest_offset;
> > +
> >      Qcow2EncDecData arg = {
> >          .block = s->crypto,
> > -        .offset = s->crypt_physical_offset ?
> > -                      file_cluster_offset + offset_into_cluster(s, offset) :
> > -                      offset,
> > +        .offset = offset,
> >          .buf = buf,
> >          .len = len,
> >          .func = func,
> > @@ -251,18 +255,51 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> >      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts one or more contiguous aligned sectors
> > + *
> > + * @host_cluster_offset - underlying storage offset of the first cluster
> > + * in which the encrypted data will be written
> > + * Used as an initialization vector for encryption
> 
> s/as an/for generating the/
> 
> (AFAIU)
Yes, this is better.


> 
> > + *
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> > + * Used as an initialization vector for older, qcow2 native encryption
> 
> I wouldn’t be so specific here.  I think it’d be better to just have a
> common sentence like “Depending on the encryption method, either of
> these offsets may be used for generating the initialization vector for
> encryption.”
Nothing against this either.


> 
> Well, technically, host_cluster_offset will not be used itself.  Before
> we can use it, of course we have to add the in-cluster offset to it
> (which qcow2_co_encdec() does).
> 
> This makes me wonder whether it wouldn’t make more sense to pass a
> host_offset instead of a host_cluster_offset (i.e. make the callers add
> the in-cluster offset to the host offset already)?

This is what I wanted to do in first place, and it would be the cleanest
solution, but that would need to update the 3rd caller of this function,
the qcow2_co_pwritev_part, which does pass the cluster offset and guest full offset.
You know what, I'll just do it. A bit more changes, but much cleaner code that
eliminates the possibility of this bug of happening again.


> 
> > + *
> > + * @buf - buffer with the data to encrypt, that after encryption
> > + *        will be written to the underlying storage device at
> > + *        @host_cluster_offset
> 
> I think just “buffer with the data to encrypt” is sufficient.  (The rest
> is just the same as above.)
I wrote it to clarify this since Vladimir told me that its not clear enough.
Note though that I made a mistake here since the data will be written not at
host_cluster_offset but in host_cluster_offset + offset_into_cluster(s, guest_offset


> + * @len - length of the buffer (in sector size multiplies)
> 
> “In sector size multiples” to me means that it is a sector count (in
> that “one sector size multiple” is equal to 512 byes).
> 
> Maybe “must be a BDRV_SECTOR_SIZE multiple” instead?
All right.

> 
> > + *
> > + * Note that the group of the sectors, don't have to be aligned
> > + * on cluster boundary and can also cross a cluster boundary.
> 
> Maybe “Note that while the whole range must be aligned on sectors, it
> does not have to be aligned on clusters and can also cross cluster
> boundaries”?
> 
> (“The group of sectors” sounds a bit weird to me.  I don’t quite know,
> why.  I think that for some reason it makes me think of a non-continuous
> set of sectors.  Also, the caller doesn’t pass sector indices, but byte
> offsets, that just happen to have to be aligned to sectors.  (I suppose
> because that’s the simplest way to make it a multiple of the encryption
> block size.))
All right, this sounds better

> 
> > + *
> > + */
> >  int coroutine_fn
> > -qcow2_co_encrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_encrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >  {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_encrypt);
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> > +                           qcrypto_block_encrypt);
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts one or more contiguous aligned sectors
> > + * Similar to qcow2_co_encrypt
> 
> Hm.  This would make me wonder in what way it is only similar to
> qcow2_co_encrypt().  Sure, it decrypts instead of encrypting, but maybe
> there’s more...?
I don't think so, since we have symmetrical encryption here.

> 
> So maybe “Its interface is the same as qcow2_co_encrypt()'s”?
That would be lawyer territory, since interface is not technically the same,
since it decrypts rather that encrypts the data in the buffer...
I think that this wording should be good enough.


> 
> Max
> 
> > + *
> > + */
> > +
> >  int coroutine_fn
> > -qcow2_co_decrypt(BlockDriverState *bs, uint64_t file_cluster_offset,
> > -                 uint64_t offset, void *buf, size_t len)
> > +qcow2_co_decrypt(BlockDriverState *bs, uint64_t host_cluster_offset,
> > +                 uint64_t guest_offset, void *buf, size_t len)
> >  {
> > -    return qcow2_co_encdec(bs, file_cluster_offset, offset, buf, len,
> > -                             qcrypto_block_decrypt);
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> > +                           qcrypto_block_decrypt);
> >  }
> > 
> 
> 


Best regards,
	Maxim Levitsky