[libnbd PATCH] generator: Fix assertion with ill-formed 64-bit block status reply
If a server replies to a block status command with an invalid length 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. Symptoms seen during a fuzzing run: │ $ ./fuzzing/libnbd-fuzz-wrapper │+id\:000001\,sig\:06\,src\:000396\,time\:16888628\,execs\:54159419\,op\:havoc\,rep\:4 │ libnbd-fuzz-wrapper: generator/states-reply-chunk.c:698: enter_STATE_REPLY_CHUNK_REPLY_FINISH: │+Assertion `h->payload_left == 0' failed. │ Aborted (core dumped) │ read: Connection reset by peer 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 all other assignments to h->rlen in that file was able to find corresponding assignments to h->payload_left (often the next statement, but sometimes split across states based on what made the next state easier to code). Requires a non-compliant server, but I was able to come up with a one-line tweak to pending qemu patches that could trigger it. Not creating a CVE as it only appears in unstable releases. Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status") Thanks: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- I'm investigating another crash that Rich sent me off-list, but I suspect it will be a similar non-CVE situation caused by my recent 64-bit extension patches. I'll wait to apply this one for just a bit more, in case I can get the corpus file or two from Rich's fuzzing run to add to fuzzing/testcase_dir. 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 2cebe456..a5d3aefe 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; } -- 2.41.0
On Mon, Sep 18, 2023 at 02:31:28PM -0500, Eric Blake wrote:
If a server replies to a block status command with an invalid length 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. Symptoms seen during a fuzzing run:
│ $ ./fuzzing/libnbd-fuzz-wrapper │+id\:000001\,sig\:06\,src\:000396\,time\:16888628\,execs\:54159419\,op\:havoc\,rep\:4 │ libnbd-fuzz-wrapper: generator/states-reply-chunk.c:698: enter_STATE_REPLY_CHUNK_REPLY_FINISH: │+Assertion `h->payload_left == 0' failed. │ Aborted (core dumped) │ read: Connection reset by peer
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 all other assignments to h->rlen in that file was able to find corresponding assignments to h->payload_left (often the next statement, but sometimes split across states based on what made the next state easier to code).
Requires a non-compliant server, but I was able to come up with a one-line tweak to pending qemu patches that could trigger it. Not creating a CVE as it only appears in unstable releases.
Fixes: ab992766cd ("block_status: Accept 64-bit extents during block status") Thanks: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Eric Blake <eblake@redhat.com> ---
I'm investigating another crash that Rich sent me off-list, but I suspect it will be a similar non-CVE situation caused by my recent 64-bit extension patches.
I'll wait to apply this one for just a bit more, in case I can get the corpus file or two from Rich's fuzzing run to add to fuzzing/testcase_dir.
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 2cebe456..a5d3aefe 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; }
ACK, thanks. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit
participants (2)
-
Eric Blake -
Richard W.M. Jones