Re: [Qemu-devel] [PATCH v2 03/12] nbd/server: get rid of nbd_negotiate_read and friends

[Eric Blake]

On 06/02/2017 10:01 AM, Vladimir Sementsov-Ogievskiy wrote:
> Functions nbd_negotiate_{read,write,drop_sync} were introduced in
> 1a6245a5b, when nbd_rwv (was nbd_wr_sync) was working through
> qemu_co_sendv_recvv (the path is nbd_wr_sync -> qemu_co_{recv/send} ->
> qemu_co_send_recv -> qemu_co_sendv_recvv), which just yields, without
> setting any handlers. But starting from ff82911cd nbd_rwv (was
> nbd_wr_syncv) works through qio_channel_yield() which sets handlers, so
> watchers are redundant in nbd_negotiate_{read,write,drop_sync}, then,
> let's just use nbd_{read,write,drop} functions.
> 
> Functions nbd_{read,write,drop} has errp parameter, which is unused in
> this patch. This will be fixed later.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
>  nbd/server.c | 107 
> ++++++++++++-----------------------------------------------
>  1 file changed, 22 insertions(+), 85 deletions(-)

I did not realize it at the time, but this patch plugs a
denial-of-service security hole against malicious clients that were able
to trigger an assertion failure in the server by sending garbage during
negotiation; which was a regression introduced in the mentioned commit
ff82911cd.  This has now been assigned the identifier CVE-2017-7539

The fact that we have now had 4 CVEs against qemu's NBD implementation
in the last year means we are not doing a very good job of unit testing
either the server or the client against a malicious partner; I'm still
trying to figure out ways that we can improve our testsuite coverage
(testing that a sane client can still connect happens during
qemu-iotests, but most of our CVEs have happened due to poor reactions
to out-of-spec clients).

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

signature.asc
Description: OpenPGP digital signature