From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:32948) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqorW-0001Vz-U4 for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:31:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aqorT-0000MJ-OD for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:31:42 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35415) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aqorT-0000Kk-HN for qemu-devel@nongnu.org; Thu, 14 Apr 2016 17:31:39 -0400 References: <1460077777-31004-1-git-send-email-eblake@redhat.com> From: Max Reitz Message-ID: <57100C37.5090807@redhat.com> Date: Thu, 14 Apr 2016 23:31:35 +0200 MIME-Version: 1.0 In-Reply-To: <1460077777-31004-1-git-send-email-eblake@redhat.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="VT60lKMHj3GC4H5Loa6E57MUrk1fNmUMj" Subject: Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, alex@alex.org.uk This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --VT60lKMHj3GC4H5Loa6E57MUrk1fNmUMj Content-Type: multipart/mixed; boundary="R8hjHQuRon1g2T88kCLNM5GRndciWfCmf" From: Max Reitz To: Eric Blake , qemu-devel@nongnu.org Cc: pbonzini@redhat.com, alex@alex.org.uk Message-ID: <57100C37.5090807@redhat.com> Subject: Re: [Qemu-devel] [PATCH for-2.6] nbd: Don't fail handshake on NBD_OPT_LIST descriptions References: <1460077777-31004-1-git-send-email-eblake@redhat.com> In-Reply-To: <1460077777-31004-1-git-send-email-eblake@redhat.com> --R8hjHQuRon1g2T88kCLNM5GRndciWfCmf Content-Type: text/plain; charset=iso-8859-15 Content-Transfer-Encoding: quoted-printable On 08.04.2016 03:09, Eric Blake wrote: > The NBD Protocol states that NBD_REP_SERVER may set > 'length > sizeof(namelen) + namelen'; in which case the rest > of the packet is a UTF-8 description of the export. While we > don't know of any NBD servers that send this description yet, > we had better consume the data so we don't choke when we start > to talk to such a server. >=20 > Also, a (buggy/malicious) server that replies with length < > sizeof(namelen) would cause us to block waiting for bytes that > the server is not sending, Well, you can still set length to anything and just send less... Both is equally non-compliant. But I agree that the check makes sense. > and one that replies with super-huge > lengths could cause us to temporarily allocate up to 4G memory. > Sanity check things before blindly reading incorrectly. >=20 > Signed-off-by: Eric Blake > --- >=20 > Yet another case of code introduced in 2.6 that doesn't play > nicely with spec-compliant servers... >=20 > Hopefully I've squashed them all now? >=20 > nbd/client.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) >=20 > diff --git a/nbd/client.c b/nbd/client.c > index 6777e58..48f2a21 100644 > --- a/nbd/client.c > +++ b/nbd/client.c > @@ -192,13 +192,18 @@ static int nbd_receive_list(QIOChannel *ioc, char= **name, Error **errp) > return -1; > } > } else if (type =3D=3D NBD_REP_SERVER) { > + if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) { > + error_setg(errp, "incorrect option length"); > + return -1; > + } > if (read_sync(ioc, &namelen, sizeof(namelen)) !=3D sizeof(name= len)) { > error_setg(errp, "failed to read option name length"); > return -1; > } > namelen =3D be32_to_cpu(namelen); > - if (len !=3D (namelen + sizeof(namelen))) { > - error_setg(errp, "incorrect option mame length"); > + len -=3D sizeof(namelen); > + if (len < namelen) { > + error_setg(errp, "incorrect option name length"); > return -1; > } > if (namelen > 255) { > @@ -214,6 +219,20 @@ static int nbd_receive_list(QIOChannel *ioc, char = **name, Error **errp) > return -1; > } > (*name)[namelen] =3D '\0'; > + len -=3D namelen; > + if (len) { > + char *buf =3D g_malloc(len + 1); > + if (read_sync(ioc, buf, len) !=3D len) { > + error_setg(errp, "failed to read export description");= > + g_free(*name); > + g_free(buf); > + *name =3D NULL; > + return -1; > + } > + buf[len] =3D '\0'; > + TRACE("Ignoring export description: %s", buf); I find this funny, somehow. Perhaps it's because this may explicitly print something while explaining that it's being ignored. > + g_free(buf); > + } > } else { > error_setg(errp, "Unexpected reply type %x expected %x", > type, NBD_REP_SERVER); >=20 Thanks Eric, I applied this patch to my block branch (for 2.6). If this was not your intention, please speak up. :-) https://github.com/XanClic/qemu/commits/block Max --R8hjHQuRon1g2T88kCLNM5GRndciWfCmf-- --VT60lKMHj3GC4H5Loa6E57MUrk1fNmUMj Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJXEAw3AAoJEDuxQgLoOKytYLIIAJQOMfj9AwDtD+YvDV0CTCtV 4hYl8vovSF7ORivMgeEWx/mP4rl8DT/Fp2BrrAI0gKx1N/y7F2zv1eSa+PC/5ChD mToglrTouuPqAApUmS49KH4t4knA1WSY8+rYbC8+wjSaLoEp2XsyU4jbHLF0zkhJ p5LXRypB1uKbG+thIX+XCy5lbb6JRUyuu9I5XjgGLs/XzCX/C3IsqWQhkN4KslfX QBRgAh+lgXk6oHIB7rbBl8ZFCTRRl/MNvoh9gm1sYfIUSO1iL+IAgizrr1SBwuek m+EyvXKjaKm+hiQRgUtHJB/Ys1XjfE1/UAY6QwGOTXVY/hVM/KDGPsUK/6lKHWY= =r6uB -----END PGP SIGNATURE----- --VT60lKMHj3GC4H5Loa6E57MUrk1fNmUMj--