On Thu, Sep 21, 2023 at 03:58:04PM -0500, Eric Blake wrote: > 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 <rjo...@redhat.com> > Signed-off-by: Eric Blake <ebl...@redhat.com> > --- > 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; > }
Reviewed-by: Richard W.M. Jones <rjo...@redhat.com> -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com libguestfs lets you edit virtual machines. Supports shell scripting, bindings from many languages. http://libguestfs.org _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
