Re: [Libguestfs] [v2v PATCH 1/2] lib/utils: get_disk_allocated: assert that NBD block length is positive

On Fri, Jan 14, 2022 at 02:06:33PM +0000, Richard W.M. Jones wrote: > On Fri, Jan 14, 2022 at 02:50:47PM +0100, Laszlo Ersek wrote: >> It makes no sense for the NBD server to return a zero-length block, plus >> it used to be a bug in the libnbd OCaml bindings to wrap 32-bit block >> lengths with the MSB set around to negative (signed) 32-bit integers >> (which would then be widened to negative (signed) 64-bit integers). >> >> Any non-positive "len" value breaks the progression of "fetch_offset", >> potentially leading to an infinite loop. >> >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 >> Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; >> --- >> lib/utils.ml | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/lib/utils.ml b/lib/utils.ml >> index 4c43a4b5161d..f599b0e32450 100644 >> --- a/lib/utils.ml >> +++ b/lib/utils.ml >> @@ -197,6 +197,7 @@ let get_disk_allocated ~dir ~disknr = >> for i = 0 to Array.length entries / 2 - 1 do >> let len = Int64.of_int32 entries.(i * 2) >> and typ = entries.(i * 2 + 1) in >> + assert (len > 0_L); >> if Int32.logand typ 1_l = 0_l then >> allocated := !allocated +^ len; >> fetch_offset := !fetch_offset +^ len >> -- >> 2.19.1.3.g30247aa5d201 > > I'm inclined to ACK, but it does leave us open to the possibility that > virt-v2v will crash if an NBD server returned a zero length block. > That would be sort of wrong, but could still happen in a > semi-well-behaved custom nbdkit plugin which is returning other blocks > and so still making process. Eric: any thoughts on this? The NBD protocol is clear that a block status result cannot be zero-length; such a server is buggy. While it does seem harsh that a MitM attacker could thus crash v2v by sending a server response that violates the protocol, that can only affect unencrypted connections (so it's no worse than any other MitM attack, and equally mitigated). I'm also inclined to ACK the patch.