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

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

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: test for bz #1745922

 block/qcow2-cluster.c      | 26 +++++++------
 block/qcow2-threads.c      | 53 ++++++++++++++++++++------
 tests/qemu-iotests/263     | 76 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 19 ++++++++++
 tests/qemu-iotests/group   |  1 +
 5 files changed, 153 insertions(+), 22 deletions(-)
 create mode 100755 tests/qemu-iotests/263
 create mode 100644 tests/qemu-iotests/263.out

-- 
2.17.2

[Qemu-devel] [PATCH 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 | 53 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 46 insertions(+), 15 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..8bc339690f 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,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
     return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
 }
 
+
+/*
+ * qcow2_co_encrypt()
+ *
+ * Encrypts a sector size aligned contiguous area
+ *
+ * @host_cluster_offset - on disk offset of the cluster in which
+ *                        the buffer resides
+ *
+ * @guest_offset - guest (virtual) offset of the buffer
+ * @buf - buffer with the data to encrypt
+ * @len - length of the buffer
+ *
+ * Note that the area is not cluster aligned and might 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,
+    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
                              qcrypto_block_encrypt);
 }
 
+
+/*
+ * qcow2_co_decrypt()
+ *
+ * Decrypts a sector size aligned contiguous area
+ * 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,
+    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
                              qcrypto_block_decrypt);
 }
-- 
2.17.2

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

[Maxim Levitsky]

This fixes subltle 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
   * write to two or more unallocated clusters at once
   * write doesn't fully cover nether first nor last cluster

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

The above mentioned commit accidently made it so, we encrypt the
second COW are 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 that cluster size.
   instead just add the start and end COW are 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


Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/qcow2-cluster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index b95e64c237..32477f0156 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((guest_offset & ~BDRV_SECTOR_MASK) == 0);
+        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
         assert((bytes & ~BDRV_SECTOR_MASK) == 0);
         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 3/3] qemu-iotests: test for bz #1745922

[Maxim Levitsky]

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/263     | 76 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/263.out | 19 ++++++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 96 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..9cb23aa81e
--- /dev/null
+++ b/tests/qemu-iotests/263
@@ -0,0 +1,76 @@
+#!/usr/bin/env bash
+#
+# Test encrypted write that crosses cluster boundary of two unallocated clusters
+# Based on 188
+#
+# Copyright (C) 2017 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"
+SECRETALT="secret,id=sec0,data=platypus"
+
+_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 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 0/3] Fix qcow2+luks corruption introduced by commit 8ac0f15f335

[Maxim Levitsky]

On Fri, 2019-09-06 at 20:31 +0300, Maxim Levitsky wrote:
> 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 beeing 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>

I forgot to mention, huge thanks to Kevin Wolf,
for helping me to define the proper fix for this bug.

Best regards,
	Maxim Levitsky

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

[Eric Blake]

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

On 9/6/19 12:31 PM, 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 | 53 ++++++++++++++++++++++++++++++++++---------
>  2 files changed, 46 insertions(+), 15 deletions(-)
> 

> +++ 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)

Pre-existing bug in alignment...

> +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> +                  uint64_t guest_offset, void *buf, size_t len,
> +                  Qcow2EncDecFunc func)

...so this would be a great time to fix it.

>  {
>      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,

I'm ambivalent on whether the new 'offset' variable gains us anything.
But it doesn't hurt.


>          .buf = buf,
>          .len = len,
>          .func = func,
> @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
>      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
>  }
>  
> +
> +/*
> + * qcow2_co_encrypt()
> + *
> + * Encrypts a sector size aligned contiguous area
> + *
> + * @host_cluster_offset - on disk offset of the cluster in which
> + *                        the buffer resides
> + *
> + * @guest_offset - guest (virtual) offset of the buffer
> + * @buf - buffer with the data to encrypt
> + * @len - length of the buffer
> + *
> + * Note that the area is not cluster aligned and might cross a cluster
> + * boundary

Umm, how is it possible for a sector to cross a cluster boundary?  All
clusters are sector-aligned, and encryption only works on aligned
sectors.  Oh, I see - if @len is a multiple larger than sector size,
then we have multiple sectors, and then indeed we may cross clusters.
But then the docs about being 'a sector size aligned contiguous area' is
not quite right.

> + *
> + *
> + */
>  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,
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
>                               qcrypto_block_encrypt);


Another alignment worth fixing up while in the area.

>  }
>  
> +
> +/*
> + * qcow2_co_decrypt()
> + *
> + * Decrypts a sector size aligned contiguous area
> + * 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,
> +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
>                               qcrypto_block_decrypt);

and again.

>  }
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Maxim Levitsky]

On Fri, 2019-09-06 at 13:00 -0500, Eric Blake wrote:
> On 9/6/19 12:31 PM, 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 | 53 ++++++++++++++++++++++++++++++++++---------
> >  2 files changed, 46 insertions(+), 15 deletions(-)
> > 
> > +++ 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)
> 
> Pre-existing bug in alignment...
No problem will fix.

> 
> > +qcow2_co_encdec(BlockDriverState *bs, uint64_t host_cluster_offset,
> > +                  uint64_t guest_offset, void *buf, size_t len,
> > +                  Qcow2EncDecFunc func)
> 
> ...so this would be a great time to fix it.
> 
> >  {
> >      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,
> 
> I'm ambivalent on whether the new 'offset' variable gains us anything.
> But it doesn't hurt.
I added it, so that I won't need to wrap the lines even more that 
they are wrapped already since I increased the length of
the parameter names a bit.

> 
> 
> >          .buf = buf,
> >          .len = len,
> >          .func = func,
> > @@ -251,18 +255,45 @@ qcow2_co_encdec(BlockDriverState *bs, uint64_t file_cluster_offset,
> >      return qcow2_co_process(bs, qcow2_encdec_pool_func, &arg);
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_encrypt()
> > + *
> > + * Encrypts a sector size aligned contiguous area
> > + *
> > + * @host_cluster_offset - on disk offset of the cluster in which
> > + *                        the buffer resides
> > + *
> > + * @guest_offset - guest (virtual) offset of the buffer
> > + * @buf - buffer with the data to encrypt
> > + * @len - length of the buffer
> > + *
> > + * Note that the area is not cluster aligned and might cross a cluster
> > + * boundary
> 
> Umm, how is it possible for a sector to cross a cluster boundary?  All
> clusters are sector-aligned, and encryption only works on aligned
> sectors.  Oh, I see - if @len is a multiple larger than sector size,
> then we have multiple sectors, and then indeed we may cross clusters.
> But then the docs about being 'a sector size aligned contiguous area' is
> not quite right.

Why? the written area is always both aligned on _sector_ boundary
and multiple of the sector size. At least that what I see from
the existing asserts.


> 
> > + *
> > + *
> > + */
> >  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,
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> >                               qcrypto_block_encrypt);
> 
> 
> Another alignment worth fixing up while in the area.
> 
> >  }
> >  
> > +
> > +/*
> > + * qcow2_co_decrypt()
> > + *
> > + * Decrypts a sector size aligned contiguous area
> > + * 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,
> > +    return qcow2_co_encdec(bs, host_cluster_offset, guest_offset, buf, len,
> >                               qcrypto_block_decrypt);
> 
> and again.
> 
> >  }
> > 
> 
> 


Best regards,
	Thanks for the review,
		Maxim Levitsky

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

[Eric Blake]

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

On 9/6/19 1:55 PM, Maxim Levitsky wrote:

>>> +/*
>>> + * qcow2_co_encrypt()
>>> + *
>>> + * Encrypts a sector size aligned contiguous area
>>> + *
>>> + * @host_cluster_offset - on disk offset of the cluster in which
>>> + *                        the buffer resides
>>> + *
>>> + * @guest_offset - guest (virtual) offset of the buffer
>>> + * @buf - buffer with the data to encrypt
>>> + * @len - length of the buffer
>>> + *
>>> + * Note that the area is not cluster aligned and might cross a cluster
>>> + * boundary
>>
>> Umm, how is it possible for a sector to cross a cluster boundary?  All
>> clusters are sector-aligned, and encryption only works on aligned
>> sectors.  Oh, I see - if @len is a multiple larger than sector size,
>> then we have multiple sectors, and then indeed we may cross clusters.
>> But then the docs about being 'a sector size aligned contiguous area' is
>> not quite right.
> 
> Why? the written area is always both aligned on _sector_ boundary
> and multiple of the sector size. At least that what I see from
> the existing asserts.

I'm thinking it should read something like:

Encrypts one or more contiguous aligned sectors

to make it obvious that because there can be multiple sectors, there may
indeed be a cluster boundary mid-length.  My complaint was that the
original text made it sound like there was exactly one sector (at which
point @len is redundant, unless sectors are variably-sized)

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Maxim Levitsky]

On Fri, 2019-09-06 at 14:00 -0500, Eric Blake wrote:
> On 9/6/19 1:55 PM, Maxim Levitsky wrote:
> 
> > > > +/*
> > > > + * qcow2_co_encrypt()
> > > > + *
> > > > + * Encrypts a sector size aligned contiguous area
> > > > + *
> > > > + * @host_cluster_offset - on disk offset of the cluster in which
> > > > + *                        the buffer resides
> > > > + *
> > > > + * @guest_offset - guest (virtual) offset of the buffer
> > > > + * @buf - buffer with the data to encrypt
> > > > + * @len - length of the buffer
> > > > + *
> > > > + * Note that the area is not cluster aligned and might cross a cluster
> > > > + * boundary
> > > 
> > > Umm, how is it possible for a sector to cross a cluster boundary?  All
> > > clusters are sector-aligned, and encryption only works on aligned
> > > sectors.  Oh, I see - if @len is a multiple larger than sector size,
> > > then we have multiple sectors, and then indeed we may cross clusters.
> > > But then the docs about being 'a sector size aligned contiguous area' is
> > > not quite right.
> > 
> > Why? the written area is always both aligned on _sector_ boundary
> > and multiple of the sector size. At least that what I see from
> > the existing asserts.
> 
> I'm thinking it should read something like:
> 
> Encrypts one or more contiguous aligned sectors
> 
> to make it obvious that because there can be multiple sectors, there may
> indeed be a cluster boundary mid-length.  My complaint was that the
> original text made it sound like there was exactly one sector (at which
> point @len is redundant, unless sectors are variably-sized)
> 
I agree with you completely, I will change that.

Best regards,
	Maxim Levitsky

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

[Eric Blake]

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

On 9/6/19 12:32 PM, Maxim Levitsky wrote:
> This fixes subltle corruption introduced by luks threaded encryption

subtle

> in commit 8ac0f15f335
> 
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> 
> The corruption happens when we do
>    * write to two or more unallocated clusters at once
>    * write doesn't fully cover nether first nor last cluster

s/nether/neither/

or even:

write doesn't fully cover either the first or the last cluster

> 
> In this case, when allocating the new clusters we COW both area

areas

> prior to the write and after the write, and we encrypt them.
> 
> The above mentioned commit accidently made it so, we encrypt the

accidentally

s/made it so, we encrypt/changed the encryption of/

> second COW are using the physical cluster offset of the first area.

s/are using/to use/

> 
> Fix this by:
>  * remove the offset_in_cluster parameter of do_perform_cow_encrypt
>    since it is misleading. That offset can be larger that cluster size.
>    instead just add the start and end COW are 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
> 
> 
> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> ---
>  block/qcow2-cluster.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 

> +++ 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((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
>          assert((bytes & ~BDRV_SECTOR_MASK) == 0);

Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
slightly more legibility than open-coding the bit operation.

Neat trick about power-of-2 alignment checks:

assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
                       host_offset | bytes, BDRV_SECTOR_SIZE));

gives the same result in one assertion.  (I've used it elsewhere in the
code base, but I'm not opposed to one assert per variable if you think
batching is too dense.)

I'll let Dan review the actual code change, but offhand it makes sense
to me.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


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

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

[Maxim Levitsky]

On Fri, 2019-09-06 at 14:17 -0500, Eric Blake wrote:
> On 9/6/19 12:32 PM, Maxim Levitsky wrote:
> > This fixes subltle corruption introduced by luks threaded encryption
> 
> subtle

I usually put the commit messages to a spellchecker, but this time
I forgot to do this. I will try not to in the future.

> 
> > in commit 8ac0f15f335
> > 
> > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1745922
> > 
> > The corruption happens when we do
> >    * write to two or more unallocated clusters at once
> >    * write doesn't fully cover nether first nor last cluster
> 
> s/nether/neither/
> 
> or even:
> 
> write doesn't fully cover either the first or the last cluster
I think I didn't wrote the double negative correctly here.
I meant a write that doesn't cover first sector fully and doesn't cover second sector.
I'll just write it like that I guess.

> 
> > 
> > In this case, when allocating the new clusters we COW both area
> 
> areas
> 
> > prior to the write and after the write, and we encrypt them.
> > 
> > The above mentioned commit accidently made it so, we encrypt the
> 
> accidentally
> 
> s/made it so, we encrypt/changed the encryption of/
> 
> > second COW are using the physical cluster offset of the first area.
> 
> s/are using/to use/
I actually meant to write 'area' here. I just haven't proofed the commit
message at all I confess. Next time I do better.

> 
> > 
> > Fix this by:
> >  * remove the offset_in_cluster parameter of do_perform_cow_encrypt
> >    since it is misleading. That offset can be larger that cluster size.
> >    instead just add the start and end COW are 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
> > 
> > 
> > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
> > ---
> >  block/qcow2-cluster.c | 26 +++++++++++++++-----------
> >  1 file changed, 15 insertions(+), 11 deletions(-)
> > 
> > +++ 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((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> > +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
> >          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> 
> Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
> slightly more legibility than open-coding the bit operation.
> 
> Neat trick about power-of-2 alignment checks:
> 
> assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
>                        host_offset | bytes, BDRV_SECTOR_SIZE));

In my book, a shorter code is almost always better, so why not.
> 
> gives the same result in one assertion.  (I've used it elsewhere in the
> code base, but I'm not opposed to one assert per variable if you think
> batching is too dense.)
> 
> I'll let Dan review the actual code change, but offhand it makes sense
> to me.
> 

Best regards,
	Thanks for the review,
		Maxim Levitsky

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

[Kevin Wolf]

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

Am 06.09.2019 um 21:17 hat Eric Blake geschrieben:
> > -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > +        assert((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> > +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
> >          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> 
> Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
> slightly more legibility than open-coding the bit operation.
> 
> Neat trick about power-of-2 alignment checks:
> 
> assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
>                        host_offset | bytes, BDRV_SECTOR_SIZE));
> 
> gives the same result in one assertion.  (I've used it elsewhere in the
> code base, but I'm not opposed to one assert per variable if you think
> batching is too dense.)

A possible downside of this is that if a user reports an assertion
failure, you can't tell any more which of the variables ended up in a
bad state.

If you're lucky, you can still tell in gdb at least if the bug is
reproducible, but I wouldn't be surprised if in release builds, half of
the variables were actually optimised away, so that even this wouldn't
work.

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

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

[Maxim Levitsky]

On Mon, 2019-09-09 at 12:56 +0200, Kevin Wolf wrote:
> Am 06.09.2019 um 21:17 hat Eric Blake geschrieben:
> > > -        assert((offset_in_cluster & ~BDRV_SECTOR_MASK) == 0);
> > > +        assert((guest_offset & ~BDRV_SECTOR_MASK) == 0);
> > > +        assert((host_offset & ~BDRV_SECTOR_MASK) == 0);
> > >          assert((bytes & ~BDRV_SECTOR_MASK) == 0);
> > 
> > Pre-existing, but we could use QEMU_IS_ALIGNED(x, BDRV_SECTOR_SIZE) for
> > slightly more legibility than open-coding the bit operation.
> > 
> > Neat trick about power-of-2 alignment checks:
> > 
> > assert(QEMU_IS_ALIGNED(offset_in_cluster | guest_offset |
> >                        host_offset | bytes, BDRV_SECTOR_SIZE));
> > 
> > gives the same result in one assertion.  (I've used it elsewhere in the
> > code base, but I'm not opposed to one assert per variable if you think
> > batching is too dense.)
> 
> A possible downside of this is that if a user reports an assertion
> failure, you can't tell any more which of the variables ended up in a
> bad state.
> 
> If you're lucky, you can still tell in gdb at least if the bug is
> reproducible, but I wouldn't be surprised if in release builds, half of
> the variables were actually optimised away, so that even this wouldn't
> work.
Agreed. I guess I'll keep the separate asserts anyway after all, even though
I prefer shorter code.


Best regards,
	Maxim Levitsky