Re: [Libguestfs] [libnbd PATCH v2 5/6] block_status: Fix assertion on bad 64-bit block status reply

If a server replies to a block status command with an invalid count in NBD_REPLY_TYPE_BLOCK_STATUS_EXT, we correctly detect the server's error, but fail to mark that we've consumed enough data off the wire to resync back to the server's next reply. Rich's fuzzing run initially found this, but I was able to quickly write a one-byte patch on top of my pending qemu patches [1] to reproduce it: [1] https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg05231.html | diff --git i/nbd/server.c w/nbd/server.c | index 898580a9b0b..bd8d46ba3c4 100644 | --- i/nbd/server.c | +++ w/nbd/server.c | @@ -2326,7 +2326,7 @@ nbd_co_send_extents(NBDClient *client, NBDRequest *request, NBDExtentArray *ea, | iov[1].iov_base = &meta_ext; | iov[1].iov_len = sizeof(meta_ext); | stl_be_p(&meta_ext.context_id, context_id); | - stl_be_p(&meta_ext.count, ea->count); | + stl_be_p(&meta_ext.count, !ea->count); | | nbd_extent_array_convert_to_be(ea); | iov[2].iov_base = ea->extents; then with a just-built 'qemu-nbd -f raw -r file -t &' running, we have pre-patch: $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF > def f(*k): > pass > try: > h.block_status(1,0,f) > except nbd.Error as ex: > print(ex.string) > h.shutdown() > EOF nbdsh: generator/states-reply-chunk.c:701: enter_STATE_REPLY_CHUNK_REPLY_FINISH: Assertion `h->payload_left == 0' failed. Aborted (core dumped) vs. post-patch: $ ./run nbdsh --base -u nbd://localhost -c - <<\EOF > def f(*k): > pass > try: > h.block_status(1,0,f) > except nbd.Error as ex: > print(ex.string) > h.shutdown() > EOF nbd_block_status: block-status: command failed: Protocol error Appears to be a casualty of rebasing: I added h->payload_left verification fairly late in the game, then floated it earlier in the series, and missed a spot where I added a state machine jump to RESYNC without having updated h->payload_left. An audit of h->hlen modification sites show that all other chunked reads updated h->payload_left appropriately (often in the next statement, but sometimes in a later state when that made logic easier). Requires a non-compliant server, and only possible when extended headers are negotiated, which does not affect any stable released libnbd. Thus, there is no reason to create a CVE, although since I will already be doing a security info email about previous patches also addressing fuzzer findings, I can mention this at the same time. Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status") Thanks: Richard W.M. Jones <rjones(a)redhat.com&gt; Signed-off-by: Eric Blake <eblake(a)redhat.com&gt; --- generator/states-reply-chunk.c | 1 + 1 file changed, 1 insertion(+) diff --git a/generator/states-reply-chunk.c b/generator/states-reply-chunk.c index 20407d91..5a31c192 100644 --- a/generator/states-reply-chunk.c +++ b/generator/states-reply-chunk.c @@ -476,6 +476,7 @@ REPLY.CHUNK_REPLY.RECV_BS_HEADER: if (h->bs_count != be32toh (h->sbuf.reply.payload.bs_hdr_64.count)) { h->rbuf = NULL; h->rlen = h->payload_left; + h->payload_left = 0; SET_NEXT_STATE (%RESYNC); return 0; }