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

[Qemu-devel] [PATCH v2 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.

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      | 30 ++++++++-------
 block/qcow2-threads.c      | 61 ++++++++++++++++++++++++-------
 tests/qemu-iotests/263     | 75 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 19 ++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 160 insertions(+), 26 deletions(-)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

-- 
2.17.2

[Qemu-devel] [PATCH v2 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 | 10 +++----
 block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 53 insertions(+), 18 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f09cc992af..1989b423da 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;
         }
@@ -496,7 +496,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
     }
 
     ret = qcow2_pre_write_overlap_check(bs, 0,
-            cluster_offset + offset_in_cluster, qiov->size, true);
+              cluster_offset + offset_in_cluster, qiov->size, true);
     if (ret < 0) {
         return ret;
     }
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk 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
+ * @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
+ * Same function as 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 v2 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>
---
 block/qcow2-cluster.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 1989b423da..6df17dd296 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -463,20 +463,20 @@ 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 | host_offset | 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 +890,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 v2 3/3] qemu-iotests: Add test for bz #1745922

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/263     | 75 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 19 ++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 95 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..36951ff7b4
--- /dev/null
+++ b/tests/qemu-iotests/263
@@ -0,0 +1,75 @@
+#!/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"
+
+_make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size
+
+IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
+
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
+
+echo
+echo "== reading the whole image =="
+$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $IMGSPEC | _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 $IMGSPEC | _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 $IMGSPEC | _filter_qemu_io | _filter_testdir
+$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
+$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
+
+_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..fa4e4e0e4a
--- /dev/null
+++ b/tests/qemu-iotests/263.out
@@ -0,0 +1,19 @@
+QA output created by 263
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1048576 encrypt.format=luks encrypt.key-secret=sec0 encrypt.iter-time=10
+
+== reading 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 v2 2/3] block/qcow2: fix the corruption when rebasing luks encrypted files

[Vladimir Sementsov-Ogievskiy]

06.09.2019 22:57, Maxim Levitsky wrote:
> This fixes subtle corruption introduced by luks threaded encryption
> in commit 8ac0f15f335

My fault, I'm sorry :( And great thanks for investigating this!

> 
> 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.

Ohh, tricky thing. For sure I missed this logic.

> 
>     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 | 28 ++++++++++++++++------------
>   1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 1989b423da..6df17dd296 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -463,20 +463,20 @@ 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 | host_offset | 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 +890,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;
>           }
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

06.09.2019 22:57, 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 | 10 +++----
>   block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
>   2 files changed, 53 insertions(+), 18 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f09cc992af..1989b423da 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;
>           }
> @@ -496,7 +496,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
>       }
>   
>       ret = qcow2_pre_write_overlap_check(bs, 0,
> -            cluster_offset + offset_in_cluster, qiov->size, true);
> +              cluster_offset + offset_in_cluster, qiov->size, true);


Hmm, unrelated hunk.

>       if (ret < 0) {
>           return ret;
>       }
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
> + * the encrypted data will be written


It's not quite right, it's not on disk, but on .file child of qcow2 node, which
may be any other format or protocol node.. So, I called it file_cluster_offset.
But I'm OK with new naming anyway. And it may be better for encryption related
logic..

 > + * Used as an initialization vector for encryption

Hmm, is it default now?

> + *
> + * @guest_offset - guest (virtual) offset of the first sector of the
> + * data to be encrypted

Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first sector of
the data to be encrypted, but first sector in which guest writes data (to be encrypted
in meantime).

> + * Used as an initialization vector for older, qcow2 native encryption
> + *
> + * @buf - buffer with the data to encrypt
> + * @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.

And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
of the sectors crossing a cluster boundary, we will finish up with similar bug: we'll
use first cluster offset as a vector for all the sectors. We still never do it.. So,
I think it worth assertion and corresponding comment.

Or is it correct?

> + *
> + *
> + */
>   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
> + * Same function as qcow2_co_encrypt

Hmm, not exactly same :)

> + *
> + */
> +
>   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,
Vladimir

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

[Daniel P. Berrangé]

On Fri, Sep 06, 2019 at 10:57:50PM +0300, Maxim Levitsky wrote:
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  tests/qemu-iotests/263     | 75 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/263.out | 19 ++++++++++
>  tests/qemu-iotests/group   |  1 +
>  3 files changed, 95 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..36951ff7b4
> --- /dev/null
> +++ b/tests/qemu-iotests/263
> @@ -0,0 +1,75 @@
> +#!/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"
> +
> +_make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size
> +
> +IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
> +
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> +
> +echo
> +echo "== reading the whole image =="
> +$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $IMGSPEC | _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 $IMGSPEC | _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 $IMGSPEC | _filter_qemu_io | _filter_testdir
> +$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
> +$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir

This tests LUKS encryption, but the code you'r changing/fixing also used
for the traditionl qcow2 encryption. The difference in IV handling for
these two methods is what made this code confusing, so I'd like to see
that the test also covers traditional qcow2 encryption.

Also can you confirm that the test succeeds when run on a qemu
built against 8c1ecb590497b0349c550607db923972b37f6963  (the change
immediately before Vladimir's threading series) ?


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Maxim Levitsky]

On Mon, 2019-09-09 at 11:35 +0100, Daniel P. Berrangé wrote:
> On Fri, Sep 06, 2019 at 10:57:50PM +0300, Maxim Levitsky wrote:
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  tests/qemu-iotests/263     | 75 ++++++++++++++++++++++++++++++++++++++
> >  tests/qemu-iotests/263.out | 19 ++++++++++
> >  tests/qemu-iotests/group   |  1 +
> >  3 files changed, 95 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..36951ff7b4
> > --- /dev/null
> > +++ b/tests/qemu-iotests/263
> > @@ -0,0 +1,75 @@
> > +#!/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"
> > +
> > +_make_test_img --object $SECRET -o "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=64K" $size
> > +
> > +IMGSPEC="driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"
> > +
> > +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> > +
> > +echo
> > +echo "== reading the whole image =="
> > +$QEMU_IO --object $SECRET -c "read -P 0 0 $size" --image-opts $IMGSPEC | _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 $IMGSPEC | _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 $IMGSPEC | _filter_qemu_io | _filter_testdir
> > +$QEMU_IO --object $SECRET -c "read -P 0xAA 0x0FE00 0x400" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
> > +$QEMU_IO --object $SECRET -c "read -P 0x00 0x10200 0xEFE00" --image-opts $IMGSPEC | _filter_qemu_io | _filter_testdir
> 
> This tests LUKS encryption, but the code you'r changing/fixing also used
> for the traditionl qcow2 encryption. The difference in IV handling for
> these two methods is what made this code confusing, so I'd like to see
> that the test also covers traditional qcow2 encryption.
This is very good idea. Done.

> 
> Also can you confirm that the test succeeds when run on a qemu
> built against 8c1ecb590497b0349c550607db923972b37f6963  (the change
> immediately before Vladimir's threading series) ?
Yes, the test fails with this commit. It also fails on master and works
with my fix (both encryption case).

> 
> 
> Regards,
> Daniel


Best regards,
	Maxim Levitsky

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

[Maxim Levitsky]

On Sat, 2019-09-07 at 19:08 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 06.09.2019 22:57, 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 | 10 +++----
> >   block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
> >   2 files changed, 53 insertions(+), 18 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index f09cc992af..1989b423da 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;
> >           }
> > @@ -496,7 +496,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
> >       }
> >   
> >       ret = qcow2_pre_write_overlap_check(bs, 0,
> > -            cluster_offset + offset_in_cluster, qiov->size, true);
> > +              cluster_offset + offset_in_cluster, qiov->size, true);
> 
> 
> Hmm, unrelated hunk.

I was asked to do this to fix coding style, so that wrapped line,
is 4 characters shifted to the right.

> 
> >       if (ret < 0) {
> >           return ret;
> >       }
> > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
> > + * the encrypted data will be written
> 
> 
> It's not quite right, it's not on disk, but on .file child of qcow2 node, which
> may be any other format or protocol node.. So, I called it file_cluster_offset.
> But I'm OK with new naming anyway. And it may be better for encryption related
> logic..

Yes, the .file is the underlying storage for both qcow2 metadata and the data,
and it is unlikely be another qcow2 file. Usually it will be a raw file,
accessed with some protocol.
I will change the wording to not include the 'disk' word though.


To be really honest, the best naming here would be one that follows the virtual memory concepts.
A virtual block/cluster address and a physical block/cluster address.
However we talked with Kevin recently and I also studied quite a lot of qcow2 code,
and the usual convention is guest cluster offset and host cluster offset,
and often guest offsets are just called offsets, which is very confusing IMHO.


> 
>  > + * Used as an initialization vector for encryption
> 
> Hmm, is it default now?


Most of block crypto implementations have IV which derive
some way or another from the sector address.

From what I see, the block address is either used as is,
or encrypted itself with same encryption key,
and the result is used as IV. Even the legacy qcow
encryption uses this, although it uses the virtual block
address, and apparently this is one of its security flaws.

If you don't use any IV, you end up with major security
hole - sectors of the same content will be encrypted
to the same cipertext.

I added this comment to clarify the usage of offset,
since other that this aspect of IV generation, 
the crypto routines only need the data to be encrypted and 
the encryption key which is stored in the crypto context.


> 
> > + *
> > + * @guest_offset - guest (virtual) offset of the first sector of the
> > + * data to be encrypted
> 
> Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first sector of
> the data to be encrypted, but first sector in which guest writes data (to be encrypted
> in meantime).
No no no!

guest_offset is literally the guest's disk address of the first sector that is in the buffer.
The qcow2_co_encrypt is called from 2 places:

1. qcow2_co_pwritev_part - here indeed the actually guest written data
is encrypted, and the 'offset' is passed which is the offset on which pwritev was called


2. do_perform_cow_encrypt - here the just read data from before or after actually written guest
data is encrypted, and the guest_offset represents the address of that data.
I changed the do_perform_cow_encrypt, so that it receives from the caller the host and guest offset
of the data to be encrypted. It then aligns the host offset on start of the cluster, and passes
the guest offset as is, so that it does the same as qcow2_co_pwritev_part.


So what is wrong here?

>  
> 
> > + * Used as an initialization vector for older, qcow2 native encryption
> > + *
> > + * @buf - buffer with the data to encrypt
> > + * @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.
> 
> And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
> of the sectors crossing a cluster boundary, we will finish up with similar bug: we'll
> use first cluster offset as a vector for all the sectors. We still never do it.. So,
> I think it worth assertion and corresponding comment.
> 
> Or is it correct?

Crypto code receives the data to be encrypted and decrypted,
and offset of first sector (512 bytes aligned) of that data (for IV calculation).
If the data spans multiple sectors (this happens already a lot) 
then it able to handle this since the code works.
The relevant code is in do_qcrypto_block_cipher_encdec, and
is actually generic for all the crypto modes.

So there should not be anything special about crossing the cluster
boundary, other that noting this here, just in case.

No only that code can cross a cluster boundary, but it can even be
called for more that one cluster at once. It looks like qcow2 code limits all
the IO in case crypto is used to QCOW_MAX_CRYPT_CLUSTERS, which is 32
currently. I don't know why to be honest.

Remember that crypto code has no notion of clusters. It works purely
on sector (512 bytes) level, and each sector will have its own IV calculated,
based on its sector address.

> 
> > + *
> > + *
> > + */
> >   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
> > + * Same function as qcow2_co_encrypt
> 
> Hmm, not exactly same :)
I'll fix that.


> 
> > + *
> > + */
> > +
> >   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,
	Thanks for the review,
		Maxim Levitsky

> 
> -- 
> Best regards,
> Vladimir

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

[Vladimir Sementsov-Ogievskiy]

10.09.2019 15:31, Maxim Levitsky wrote:
> On Sat, 2019-09-07 at 19:08 +0000, Vladimir Sementsov-Ogievskiy wrote:
>> 06.09.2019 22:57, 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 | 10 +++----
>>>    block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
>>>    2 files changed, 53 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index f09cc992af..1989b423da 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;
>>>            }
>>> @@ -496,7 +496,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
>>>        }
>>>    
>>>        ret = qcow2_pre_write_overlap_check(bs, 0,
>>> -            cluster_offset + offset_in_cluster, qiov->size, true);
>>> +              cluster_offset + offset_in_cluster, qiov->size, true);
>>
>>
>> Hmm, unrelated hunk.
> 
> I was asked to do this to fix coding style, so that wrapped line,
> is 4 characters shifted to the right.

AFAIS, Eric asked only about qcow2_co_encdec calls and definition.. It's OK to fix style in code
you touch in you patch anyway, but no reason to fix style somewhere else, it dirties patch for
no reason, making it more difficult (a bit, but still) to review and more difficult to backport..

> 
>>
>>>        if (ret < 0) {
>>>            return ret;
>>>        }
>>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>>> index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
>>> + * the encrypted data will be written
>>
>>
>> It's not quite right, it's not on disk, but on .file child of qcow2 node, which
>> may be any other format or protocol node.. So, I called it file_cluster_offset.
>> But I'm OK with new naming anyway. And it may be better for encryption related
>> logic..
> 
> Yes, the .file is the underlying storage for both qcow2 metadata and the data,
> and it is unlikely be another qcow2 file. Usually it will be a raw file,
> accessed with some protocol.
> I will change the wording to not include the 'disk' word though.
> 
> 
> To be really honest, the best naming here would be one that follows the virtual memory concepts.
> A virtual block/cluster address and a physical block/cluster address.
> However we talked with Kevin recently and I also studied quite a lot of qcow2 code,
> and the usual convention is guest cluster offset and host cluster offset,
> and often guest offsets are just called offsets, which is very confusing IMHO.

It don't confuse me, as it's an interface of block-layer. Interface user just writes at some offset.
And the fact that internally, we consider interface parameter "offset" as "guest offset" and map it
to some physical offset - it's an implementation details. In other words, guest don't know that it
writes at "guest offset", it just writes at "offset" and don't care.

> 
> 
>>
>>   > + * Used as an initialization vector for encryption
>>
>> Hmm, is it default now?
> 
> 
> Most of block crypto implementations have IV which derive
> some way or another from the sector address.
> 
>  From what I see, the block address is either used as is,
> or encrypted itself with same encryption key,
> and the result is used as IV. Even the legacy qcow
> encryption uses this, although it uses the virtual block
> address, and apparently this is one of its security flaws.
> 
> If you don't use any IV, you end up with major security
> hole - sectors of the same content will be encrypted
> to the same cipertext.
> 
> I added this comment to clarify the usage of offset,
> since other that this aspect of IV generation,
> the crypto routines only need the data to be encrypted and
> the encryption key which is stored in the crypto context.
> 
> 
>>
>>> + *
>>> + * @guest_offset - guest (virtual) offset of the first sector of the
>>> + * data to be encrypted
>>
>> Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first sector of
>> the data to be encrypted, but first sector in which guest writes data (to be encrypted
>> in meantime).
> No no no!
> 
> guest_offset is literally the guest's disk address of the first sector that is in the buffer.
> The qcow2_co_encrypt is called from 2 places:
> 
> 1. qcow2_co_pwritev_part - here indeed the actually guest written data
> is encrypted, and the 'offset' is passed which is the offset on which pwritev was called

I mean, that in this case your comment for me sounds like "data to be encrypted is on disk, at this offset".
but it's not actually on disk, data is in buffer. And what is on disk we don't know, we are going to
write...

But I don't really care. Hope it's actually obvious for averyone, where is data on write path.

> 
> 
> 2. do_perform_cow_encrypt - here the just read data from before or after actually written guest
> data is encrypted, and the guest_offset represents the address of that data.
> I changed the do_perform_cow_encrypt, so that it receives from the caller the host and guest offset
> of the data to be encrypted. It then aligns the host offset on start of the cluster, and passes
> the guest offset as is, so that it does the same as qcow2_co_pwritev_part.
> 
> 
> So what is wrong here?
> 
>>   
>>
>>> + * Used as an initialization vector for older, qcow2 native encryption
>>> + *
>>> + * @buf - buffer with the data to encrypt
>>> + * @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.
>>
>> And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
>> of the sectors crossing a cluster boundary, we will finish up with similar bug: we'll
>> use first cluster offset as a vector for all the sectors. We still never do it.. So,
>> I think it worth assertion and corresponding comment.
>>
>> Or is it correct?
> 
> Crypto code receives the data to be encrypted and decrypted,
> and offset of first sector (512 bytes aligned) of that data (for IV calculation).
> If the data spans multiple sectors (this happens already a lot)

Ah, yes, that's OK, sorry.

> then it able to handle this since the code works.
> The relevant code is in do_qcrypto_block_cipher_encdec, and
> is actually generic for all the crypto modes.
> 
> So there should not be anything special about crossing the cluster
> boundary, other that noting this here, just in case.
> 
> No only that code can cross a cluster boundary, but it can even be
> called for more that one cluster at once. It looks like qcow2 code limits all
> the IO in case crypto is used to QCOW_MAX_CRYPT_CLUSTERS, which is 32
> currently. I don't know why to be honest.
> 
> Remember that crypto code has no notion of clusters. It works purely
> on sector (512 bytes) level, and each sector will have its own IV calculated,
> based on its sector address.
> 
>>
>>> + *
>>> + *
>>> + */
>>>    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
>>> + * Same function as qcow2_co_encrypt
>>
>> Hmm, not exactly same :)
> I'll fix that.
> 
> 
>>
>>> + *
>>> + */
>>> +
>>>    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,
> 	Thanks for the review,
> 		Maxim Levitsky
> 
>>
>> -- 
>> Best regards,
>> Vladimir
> 
> 


-- 
Best regards,
Vladimir

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

[Maxim Levitsky]

On Tue, 2019-09-10 at 14:17 +0000, Vladimir Sementsov-Ogievskiy wrote:
> 10.09.2019 15:31, Maxim Levitsky wrote:
> > On Sat, 2019-09-07 at 19:08 +0000, Vladimir Sementsov-Ogievskiy wrote:
> > > 06.09.2019 22:57, 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 | 10 +++----
> > > >    block/qcow2-threads.c | 61 ++++++++++++++++++++++++++++++++++---------
> > > >    2 files changed, 53 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > > > index f09cc992af..1989b423da 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;
> > > >            }
> > > > @@ -496,7 +496,7 @@ static int coroutine_fn do_perform_cow_write(BlockDriverState *bs,
> > > >        }
> > > >    
> > > >        ret = qcow2_pre_write_overlap_check(bs, 0,
> > > > -            cluster_offset + offset_in_cluster, qiov->size, true);
> > > > +              cluster_offset + offset_in_cluster, qiov->size, true);
> > > 
> > > 
> > > Hmm, unrelated hunk.
> > 
> > I was asked to do this to fix coding style, so that wrapped line,
> > is 4 characters shifted to the right.
> 
> AFAIS, Eric asked only about qcow2_co_encdec calls and definition.. It's OK to fix style in code
> you touch in you patch anyway, but no reason to fix style somewhere else, it dirties patch for
> no reason, making it more difficult (a bit, but still) to review and more difficult to backport..
All right, then I'll drop that change. I kind of agree with this, but I also didn't mind doing these fixes
either.

> 
> > 
> > > 
> > > >        if (ret < 0) {
> > > >            return ret;
> > > >        }
> > > > diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> > > > index 3b1e63fe41..c3cda0c6a5 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,49 @@ 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 - on disk offset of the first cluster in which
> > > > + * the encrypted data will be written
> > > 
> > > 
> > > It's not quite right, it's not on disk, but on .file child of qcow2 node, which
> > > may be any other format or protocol node.. So, I called it file_cluster_offset.
> > > But I'm OK with new naming anyway. And it may be better for encryption related
> > > logic..
> > 
> > Yes, the .file is the underlying storage for both qcow2 metadata and the data,
> > and it is unlikely be another qcow2 file. Usually it will be a raw file,
> > accessed with some protocol.
> > I will change the wording to not include the 'disk' word though.
> > 
> > 
> > To be really honest, the best naming here would be one that follows the virtual memory concepts.
> > A virtual block/cluster address and a physical block/cluster address.
> > However we talked with Kevin recently and I also studied quite a lot of qcow2 code,
> > and the usual convention is guest cluster offset and host cluster offset,
> > and often guest offsets are just called offsets, which is very confusing IMHO.
> 
> It don't confuse me, as it's an interface of block-layer. Interface user just writes at some offset.
> And the fact that internally, we consider interface parameter "offset" as "guest offset" and map it
> to some physical offset - it's an implementation details. In other words, guest don't know that it
> writes at "guest offset", it just writes at "offset" and don't care.
> 
> > 
> > 
> > > 
> > >   > + * Used as an initialization vector for encryption
> > > 
> > > Hmm, is it default now?
> > 
> > 
> > Most of block crypto implementations have IV which derive
> > some way or another from the sector address.
> > 
> >  From what I see, the block address is either used as is,
> > or encrypted itself with same encryption key,
> > and the result is used as IV. Even the legacy qcow
> > encryption uses this, although it uses the virtual block
> > address, and apparently this is one of its security flaws.
> > 
> > If you don't use any IV, you end up with major security
> > hole - sectors of the same content will be encrypted
> > to the same cipertext.
> > 
> > I added this comment to clarify the usage of offset,
> > since other that this aspect of IV generation,
> > the crypto routines only need the data to be encrypted and
> > the encryption key which is stored in the crypto context.
> > 
> > 
> > > 
> > > > + *
> > > > + * @guest_offset - guest (virtual) offset of the first sector of the
> > > > + * data to be encrypted
> > > 
> > > Hmm, stop. It's wrong. Data to be encrypted is in buffer, so, it's not first sector of
> > > the data to be encrypted, but first sector in which guest writes data (to be encrypted
> > > in meantime).
> > 
> > No no no!
> > 
> > guest_offset is literally the guest's disk address of the first sector that is in the buffer.
> > The qcow2_co_encrypt is called from 2 places:
> > 
> > 1. qcow2_co_pwritev_part - here indeed the actually guest written data
> > is encrypted, and the 'offset' is passed which is the offset on which pwritev was called
> 
> I mean, that in this case your comment for me sounds like "data to be encrypted is on disk, at this offset".
> but it's not actually on disk, data is in buffer. And what is on disk we don't know, we are going to
> write...
> 
> But I don't really care. Hope it's actually obvious for averyone, where is data on write path.

Ah, now I understand. I don't mind adding a word or two explicitly mentioning that the *data* to be
encrypted is in the given buffer.

> 
> > 
> > 
> > 2. do_perform_cow_encrypt - here the just read data from before or after actually written guest
> > data is encrypted, and the guest_offset represents the address of that data.
> > I changed the do_perform_cow_encrypt, so that it receives from the caller the host and guest offset
> > of the data to be encrypted. It then aligns the host offset on start of the cluster, and passes
> > the guest offset as is, so that it does the same as qcow2_co_pwritev_part.
> > 
> > 
> > So what is wrong here?
> > 
> > >   
> > > 
> > > > + * Used as an initialization vector for older, qcow2 native encryption
> > > > + *
> > > > + * @buf - buffer with the data to encrypt
> > > > + * @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.
> > > 
> > > And I doubt in it now. I'm afraid that if we call qcow2_co_encrypt for a group
> > > of the sectors crossing a cluster boundary, we will finish up with similar bug: we'll
> > > use first cluster offset as a vector for all the sectors. We still never do it.. So,
> > > I think it worth assertion and corresponding comment.
> > > 
> > > Or is it correct?
> > 
> > Crypto code receives the data to be encrypted and decrypted,
> > and offset of first sector (512 bytes aligned) of that data (for IV calculation).
> > If the data spans multiple sectors (this happens already a lot)
> 
> Ah, yes, that's OK, sorry.
> 
> > then it able to handle this since the code works.
> > The relevant code is in do_qcrypto_block_cipher_encdec, and
> > is actually generic for all the crypto modes.
> > 
> > So there should not be anything special about crossing the cluster
> > boundary, other that noting this here, just in case.
> > 
> > No only that code can cross a cluster boundary, but it can even be
> > called for more that one cluster at once. It looks like qcow2 code limits all
> > the IO in case crypto is used to QCOW_MAX_CRYPT_CLUSTERS, which is 32
> > currently. I don't know why to be honest.
> > 
> > Remember that crypto code has no notion of clusters. It works purely
> > on sector (512 bytes) level, and each sector will have its own IV calculated,
> > based on its sector address.
> > 
> > > 
> > > > + *
> > > > + *
> > > > + */
> > > >    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
> > > > + * Same function as qcow2_co_encrypt
> > > 
> > > Hmm, not exactly same :)
> > 
> > I'll fix that.
> > 
> > 
> > > 
> > > > + *
> > > > + */
> > > > +
> > > >    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