Richard W.M. Jones
2022-Jan-14 14:06 UTC
[Libguestfs] [v2v PATCH 1/2] lib/utils: get_disk_allocated: assert that NBD block length is positive
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 at redhat.com> > --- > 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.g30247aa5d201I'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? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v
Eric Blake
2022-Jan-14 20:57 UTC
[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 at redhat.com> > > --- > > 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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Laszlo Ersek
2022-Jan-17 09:57 UTC
[Libguestfs] [v2v PATCH 1/2] lib/utils: get_disk_allocated: assert that NBD block length is positive
On 01/14/22 15:06, 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 at redhat.com> >> --- >> 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."Still making progress" is not an excuse I think; if the NBD server can insert *one* zero-length entry, it can then insert *infinitely* many. I don't think we should tolerate that. Thanks! Laszlo> Eric: any thoughts on this? > > Rich. >