[Qemu-commits] [qemu/qemu] 93d487: iotests: 194: wait for migration completion on tar...

[Peter Maydell]

  Branch: refs/heads/master
  Home:   https://github.com/qemu/qemu
  Commit: 93d487807bf8fb9a5867d1a0a77d7afbc26e6c5c
      
https://github.com/qemu/qemu/commit/93d487807bf8fb9a5867d1a0a77d7afbc26e6c5c
  Author: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
  Date:   2020-06-09 (Tue, 09 Jun 2020)

  Changed paths:
    M tests/qemu-iotests/194
    M tests/qemu-iotests/194.out

  Log Message:
  -----------
  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 <thuth@redhat.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Tested-by: Thomas Huth <thuth@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
[eblake: grammar tweak]
Message-Id: <20200604083341.26978-1-vsementsov@virtuozzo.com>
Signed-off-by: Eric Blake <eblake@redhat.com>


  Commit: 5c4fe018c025740fef4a0a4421e8162db0c3eefd
      
https://github.com/qemu/qemu/commit/5c4fe018c025740fef4a0a4421e8162db0c3eefd
  Author: Eric Blake <eblake@redhat.com>
  Date:   2020-06-10 (Wed, 10 Jun 2020)

  Changed paths:
    M nbd/server.c
    M tests/qemu-iotests/143
    M tests/qemu-iotests/143.out

  Log Message:
  -----------
  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 <xuwei@redhat.com>
CC: qemu-stable@nongnu.org
Fixes: https://bugzilla.redhat.com/1843684 CVE-2020-10761
Fixes: 93676c88d7
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-Id: <20200610163741.3745251-2-eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>


  Commit: 5c86bdf1208916ece0b87e1151c9b48ee54faa3e
      
https://github.com/qemu/qemu/commit/5c86bdf1208916ece0b87e1151c9b48ee54faa3e
  Author: Eric Blake <eblake@redhat.com>
  Date:   2020-06-10 (Wed, 10 Jun 2020)

  Changed paths:
    M block.c
    M block/nbd.c

  Log Message:
  -----------
  block: Call attention to truncation of long NBD exports

Commit 93676c88 relaxed our NBD client code to request export names up
to the NBD protocol maximum of 4096 bytes without NUL terminator, even
though the block layer can't store anything longer than 4096 bytes
including NUL terminator for display to the user.  Since this means
there are some export names where we have to truncate things, we can
at least try to make the truncation a bit more obvious for the user.
Note that in spite of the truncated display name, we can still
communicate with an NBD server using such a long export name; this was
deemed nicer than refusing to even connect to such a server (since the
server may not be under our control, and since determining our actual
length limits gets tricky when nbd://host:port/export and
nbd+unix:///export?socket=/path are themselves variable-length
expansions beyond the export name but count towards the block layer
name length).

Reported-by: Xueqiang Wei <xuwei@redhat.com>
Fixes: https://bugzilla.redhat.com/1843684
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Message-Id: <20200610163741.3745251-3-eblake@redhat.com>


  Commit: 9f1f264edbdf5516d6f208497310b3eedbc7b74c
      
https://github.com/qemu/qemu/commit/9f1f264edbdf5516d6f208497310b3eedbc7b74c
  Author: Peter Maydell <peter.maydell@linaro.org>
  Date:   2020-06-11 (Thu, 11 Jun 2020)

  Changed paths:
    M block.c
    M block/nbd.c
    M nbd/server.c
    M tests/qemu-iotests/143
    M tests/qemu-iotests/143.out
    M tests/qemu-iotests/194
    M tests/qemu-iotests/194.out

  Log Message:
  -----------
  Merge remote-tracking branch 'remotes/ericb/tags/pull-nbd-2020-06-09-v2' into 
staging

NBD patches for 2020-06-09

- fix iotest 194 race
- fix CVE-2020-10761: server DoS from assertion on long NBD error messages

# gpg: Signature made Wed 10 Jun 2020 18:59:19 BST
# gpg:                using RSA key 71C2CC22B1C4602927D2F3AAA7A16B4A2527436A
# gpg: Good signature from "Eric Blake <eblake@redhat.com>" [full]
# gpg:                 aka "Eric Blake (Free Software Programmer) 
<ebb9@byu.net>" [full]
# gpg:                 aka "[jpeg image of size 6874]" [full]
# Primary key fingerprint: 71C2 CC22 B1C4 6029 27D2  F3AA A7A1 6B4A 2527 436A

* remotes/ericb/tags/pull-nbd-2020-06-09-v2:
  block: Call attention to truncation of long NBD exports
  nbd/server: Avoid long error message assertions CVE-2020-10761
  iotests: 194: wait for migration completion on target too

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>


Compare: https://github.com/qemu/qemu/compare/77c9e078b4da...9f1f264edbdf