[PATCH 0/2] Fix for write sharing on luks raw images

[PATCH 0/2] Fix for write sharing on luks raw images

[Maxim Levitsky]

A rebase gone wrong, and I ended up allowing a luks image
to be opened at the same time by two VMs without any warnings/overrides.

Fix that and also add an iotest to prevent this from happening.

Best regards,
	Maxim Levisky

Maxim Levitsky (2):
  block/crypto: disallow write sharing by default
  qemu-iotests: add testcase for bz #1857490

 block/crypto.c             |  2 +-
 tests/qemu-iotests/296     | 44 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/296.out | 12 +++++++++--
 3 files changed, 54 insertions(+), 4 deletions(-)

-- 
2.26.2

[PATCH 1/2] block/crypto: disallow write sharing by default

[Maxim Levitsky]

My commit 'block/crypto: implement the encryption key management'
accidently allowed raw luks images to be shared between different
qemu processes without share-rw=on explicit override.
Fix that.

Fixes: bbfdae91fb ("block/crypto: implement the encryption key management")
Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1857490

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 block/crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/crypto.c b/block/crypto.c
index 8725c1bc02..0807557763 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -881,7 +881,7 @@ block_crypto_child_perms(BlockDriverState *bs, BdrvChild *c,
      * For backward compatibility, manually share the write
      * and resize permission
      */
-    *nshared |= (BLK_PERM_WRITE | BLK_PERM_RESIZE);
+    *nshared |= shared & (BLK_PERM_WRITE | BLK_PERM_RESIZE);
     /*
      * Since we are not fully a format driver, don't always request
      * the read/resize permission but only when explicitly
-- 
2.26.2

[PATCH 2/2] qemu-iotests: add testcase for bz #1857490

[Maxim Levitsky]

Test that we can't write-share raw luks images by default,
but we still can with share-rw=on

Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com>
---
 tests/qemu-iotests/296     | 44 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/296.out | 12 +++++++++--
 2 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/296 b/tests/qemu-iotests/296
index ec69ec8974..fb7dec88aa 100755
--- a/tests/qemu-iotests/296
+++ b/tests/qemu-iotests/296
@@ -133,6 +133,21 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
         )
         self.assert_qmp(result, 'return', {})
 
+
+    ###########################################################################
+    # add virtio-blk consumer for a block device
+    def addImageUser(self, vm, id, disk_id, share_rw=False):
+        result = vm.qmp('device_add', **
+            {
+                'driver': 'virtio-blk',
+                'id': id,
+                'drive': disk_id,
+                'share-rw' : share_rw
+            }
+        )
+
+        iotests.log(result)
+
     # close the encrypted block device
     def closeImageQmp(self, vm, id):
         result = vm.qmp('blockdev-del', **{ 'node-name': id })
@@ -159,7 +174,7 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
         vm.run_job('job0')
 
     # test that when the image opened by two qemu processes,
-    # neither of them can update the image
+    # neither of them can update the encryption keys
     def test1(self):
         self.createImg(test_img, self.secrets[0]);
 
@@ -193,6 +208,9 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
         os.remove(test_img)
 
 
+    # test that when the image opened by two qemu processes,
+    # even if first VM opens it read-only, the second can't update encryption
+    # keys
     def test2(self):
         self.createImg(test_img, self.secrets[0]);
 
@@ -226,6 +244,30 @@ class EncryptionSetupTestCase(iotests.QMPTestCase):
         self.closeImageQmp(self.vm1, "testdev")
         os.remove(test_img)
 
+    # test that two VMs can't open the same luks image by default
+    # and attach it to a guest device
+    def test3(self):
+        self.createImg(test_img, self.secrets[0]);
+
+        self.openImageQmp(self.vm1, "testdev", test_img, self.secrets[0])
+        self.addImageUser(self.vm1, "testctrl", "testdev")
+
+        self.openImageQmp(self.vm2, "testdev", test_img, self.secrets[0])
+        self.addImageUser(self.vm2, "testctrl", "testdev")
+
+
+    # test that two VMs can attach the same luks image to a guest device,
+    # if both use share-rw=on
+    def test4(self):
+        self.createImg(test_img, self.secrets[0]);
+
+        self.openImageQmp(self.vm1, "testdev", test_img, self.secrets[0])
+        self.addImageUser(self.vm1, "testctrl", "testdev", share_rw=True)
+
+        self.openImageQmp(self.vm2, "testdev", test_img, self.secrets[0])
+        self.addImageUser(self.vm2, "testctrl", "testdev", share_rw=True)
+
+
 
 if __name__ == '__main__':
     # support only raw luks since luks encrypted qcow2 is a proper
diff --git a/tests/qemu-iotests/296.out b/tests/qemu-iotests/296.out
index afb6d2d09d..cb2859a15c 100644
--- a/tests/qemu-iotests/296.out
+++ b/tests/qemu-iotests/296.out
@@ -26,8 +26,16 @@ Job failed: Failed to get shared "consistent read" lock
 {"return": {}}
 {"execute": "job-dismiss", "arguments": {"id": "job0"}}
 {"return": {}}
-..
+Formatting 'TEST_DIR/test.img', fmt=luks size=1048576 key-secret=keysec0 iter-time=10
+
+{"return": {}}
+{"error": {"class": "GenericError", "desc": "Failed to get \"write\" lock"}}
+Formatting 'TEST_DIR/test.img', fmt=luks size=1048576 key-secret=keysec0 iter-time=10
+
+{"return": {}}
+{"return": {}}
+....
 ----------------------------------------------------------------------
-Ran 2 tests
+Ran 4 tests
 
 OK
-- 
2.26.2

Re: [PATCH 0/2] Fix for write sharing on luks raw images

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200719122059.59843-1-mlevitsk@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===




The full log is available at
http://patchew.org/logs/20200719122059.59843-1-mlevitsk@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] Fix for write sharing on luks raw images

[Max Reitz]

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

On 19.07.20 14:20, Maxim Levitsky wrote:
> A rebase gone wrong, and I ended up allowing a luks image
> to be opened at the same time by two VMs without any warnings/overrides.
> 
> Fix that and also add an iotest to prevent this from happening.
> 
> Best regards,
> 	Maxim Levisky
> 
> Maxim Levitsky (2):
>   block/crypto: disallow write sharing by default
>   qemu-iotests: add testcase for bz #1857490

Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block


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