From 93d487807bf8fb9a5867d1a0a77d7afbc26e6c5c Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Thu, 4 Jun 2020 11:33:41 +0300 Subject: iotests: 194: wait for migration completion on target too It is possible, that shutdown on target occurs earlier than migration finish. In this case we crash in bdrv_release_dirty_bitmap_locked() on assertion "assert(!bdrv_dirty_bitmap_busy(bitmap));" as we do have busy bitmap, as bitmap migration is ongoing. We'll fix bitmap migration to gracefully cancel on early shutdown soon. Now let's fix iotest 194 to wait migration completion before shutdown. Note that in this test dest_vm.shutdown() is called implicitly, as vms used as context-providers, see __exit__() method of QEMUMachine class. Actually, not waiting migration finish is a wrong thing, but the test started to crash after commit ae00aa239847682 "iotests: 194: test also migration of dirty bitmap", which added dirty bitmaps here. So, Fixes: tag won't hurt. Fixes: ae00aa2398476824f0eca80461da215e7cdc1c3b Reported-by: Thomas Huth Signed-off-by: Vladimir Sementsov-Ogievskiy Tested-by: Thomas Huth Reviewed-by: Eric Blake [eblake: grammar tweak] Message-Id: <20200604083341.26978-1-vsementsov@virtuozzo.com> Signed-off-by: Eric Blake --- tests/qemu-iotests/194 | 10 ++++++++++ tests/qemu-iotests/194.out | 5 +++++ 2 files changed, 15 insertions(+) (limited to 'tests') diff --git a/tests/qemu-iotests/194 b/tests/qemu-iotests/194 index 3fad7c6c1a..da7c4265ec 100755 --- a/tests/qemu-iotests/194 +++ b/tests/qemu-iotests/194 @@ -87,4 +87,14 @@ with iotests.FilePath('source.img') as source_img_path, \ iotests.log(dest_vm.qmp('nbd-server-stop')) break + iotests.log('Wait for migration completion on target...') + migr_events = (('MIGRATION', {'data': {'status': 'completed'}}), + ('MIGRATION', {'data': {'status': 'failed'}})) + event = dest_vm.events_wait(migr_events) + iotests.log(event, filters=[iotests.filter_qmp_event]) + + iotests.log('Check bitmaps on source:') iotests.log(source_vm.qmp('query-block')['return'][0]['dirty-bitmaps']) + + iotests.log('Check bitmaps on target:') + iotests.log(dest_vm.qmp('query-block')['return'][0]['dirty-bitmaps']) diff --git a/tests/qemu-iotests/194.out b/tests/qemu-iotests/194.out index dd60dcc14f..a51bdb2d4f 100644 --- a/tests/qemu-iotests/194.out +++ b/tests/qemu-iotests/194.out @@ -21,4 +21,9 @@ Gracefully ending the `drive-mirror` job on source... {"data": {"device": "mirror-job0", "len": 1073741824, "offset": 1073741824, "speed": 0, "type": "mirror"}, "event": "BLOCK_JOB_COMPLETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} Stopping the NBD server on destination... {"return": {}} +Wait for migration completion on target... +{"data": {"status": "completed"}, "event": "MIGRATION", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}} +Check bitmaps on source: +[{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}] +Check bitmaps on target: [{"busy": false, "count": 0, "granularity": 65536, "name": "bitmap0", "persistent": false, "recording": true, "status": "active"}] -- cgit v1.2.3 From 5c4fe018c025740fef4a0a4421e8162db0c3eefd Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 8 Jun 2020 13:26:37 -0500 Subject: nbd/server: Avoid long error message assertions CVE-2020-10761 Ever since commit 36683283 (v2.8), the server code asserts that error strings sent to the client are well-formed per the protocol by not exceeding the maximum string length of 4096. At the time the server first started sending error messages, the assertion could not be triggered, because messages were completely under our control. However, over the years, we have added latent scenarios where a client could trigger the server to attempt an error message that would include the client's information if it passed other checks first: - requesting NBD_OPT_INFO/GO on an export name that is not present (commit 0cfae925 in v2.12 echoes the name) - requesting NBD_OPT_LIST/SET_META_CONTEXT on an export name that is not present (commit e7b1948d in v2.12 echoes the name) At the time, those were still safe because we flagged names larger than 256 bytes with a different message; but that changed in commit 93676c88 (v4.2) when we raised the name limit to 4096 to match the NBD string limit. (That commit also failed to change the magic number 4096 in nbd_negotiate_send_rep_err to the just-introduced named constant.) So with that commit, long client names appended to server text can now trigger the assertion, and thus be used as a denial of service attack against a server. As a mitigating factor, if the server requires TLS, the client cannot trigger the problematic paths unless it first supplies TLS credentials, and such trusted clients are less likely to try to intentionally crash the server. We may later want to further sanitize the user-supplied strings we place into our error messages, such as scrubbing out control characters, but that is less important to the CVE fix, so it can be a later patch to the new nbd_sanitize_name. Consideration was given to changing the assertion in nbd_negotiate_send_rep_verr to instead merely log a server error and truncate the message, to avoid leaving a latent path that could trigger a future CVE DoS on any new error message. However, this merely complicates the code for something that is already (correctly) flagging coding errors, and now that we are aware of the long message pitfall, we are less likely to introduce such errors in the future, which would make such error handling dead code. Reported-by: Xueqiang Wei CC: qemu-stable@nongnu.org Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761 Fixes: 93676c88d7 Signed-off-by: Eric Blake Message-Id: <20200610163741.3745251-2-eblake@redhat.com> Reviewed-by: Vladimir Sementsov-Ogievskiy --- tests/qemu-iotests/143 | 4 ++++ tests/qemu-iotests/143.out | 2 ++ 2 files changed, 6 insertions(+) (limited to 'tests') diff --git a/tests/qemu-iotests/143 b/tests/qemu-iotests/143 index f649b36195..d2349903b1 100755 --- a/tests/qemu-iotests/143 +++ b/tests/qemu-iotests/143 @@ -58,6 +58,10 @@ _send_qemu_cmd $QEMU_HANDLE \ $QEMU_IO_PROG -f raw -c quit \ "nbd+unix:///no_such_export?socket=$SOCK_DIR/nbd" 2>&1 \ | _filter_qemu_io | _filter_nbd +# Likewise, with longest possible name permitted in NBD protocol +$QEMU_IO_PROG -f raw -c quit \ + "nbd+unix:///$(printf %4096d 1 | tr ' ' a)?socket=$SOCK_DIR/nbd" 2>&1 \ + | _filter_qemu_io | _filter_nbd | sed 's/aaaa*aa/aa--aa/' _send_qemu_cmd $QEMU_HANDLE \ "{ 'execute': 'quit' }" \ diff --git a/tests/qemu-iotests/143.out b/tests/qemu-iotests/143.out index 1f4001c601..fc9c0a761f 100644 --- a/tests/qemu-iotests/143.out +++ b/tests/qemu-iotests/143.out @@ -5,6 +5,8 @@ QA output created by 143 {"return": {}} qemu-io: can't open device nbd+unix:///no_such_export?socket=SOCK_DIR/nbd: Requested export not available server reported: export 'no_such_export' not present +qemu-io: can't open device nbd+unix:///aa--aa1?socket=SOCK_DIR/nbd: Requested export not available +server reported: export 'aa--aa...' not present { 'execute': 'quit' } {"return": {}} {"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "SHUTDOWN", "data": {"guest": false, "reason": "host-qmp-quit"}} -- cgit v1.2.3