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

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.

On Fri, Jan 14, 2022 at 02:50:48PM +0100, Laszlo Ersek wrote: > Adapt our "Utils.get_disk_allocated" function to the changed signature of > "NBD.block_status", coming from libnbd patch > > ocaml: map C's uint32_t to OCaml's int64 > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2027598 > Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; > --- > lib/utils.ml | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/utils.ml b/lib/utils.ml > index f599b0e32450..4c8998c28a3c 100644 > --- a/lib/utils.ml > +++ b/lib/utils.ml > @@ -195,10 +195,10 @@ let get_disk_allocated ~dir ~disknr = > (fun ctx offset entries err -> > assert (ctx = alloc_ctx); > for i = 0 to Array.length entries / 2 - 1 do > - let len = Int64.of_int32 entries.(i * 2) > + let len = entries.(i * 2) > and typ = entries.(i * 2 + 1) in > assert (len > 0_L); > - if Int32.logand typ 1_l = 0_l then > + if typ &^ 1_L = 0_L then > allocated := !allocated +^ len; > fetch_offset := !fetch_offset +^ len > done; ACK - even makes the code better :-)

Getting this into RHEL is going to be a pain, I will deal with the chain of builds once you've pushed this upstream.

Thanks, Rich.