From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49323) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ap1J8-0001pb-76 for qemu-devel@nongnu.org; Sat, 09 Apr 2016 18:24:47 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ap1J4-0004xE-Up for qemu-devel@nongnu.org; Sat, 09 Apr 2016 18:24:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:46369) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ap1J4-0004x7-NG for qemu-devel@nongnu.org; Sat, 09 Apr 2016 18:24:42 -0400 References: <1460153158-21612-1-git-send-email-eblake@redhat.com> <1460153158-21612-13-git-send-email-eblake@redhat.com> From: Eric Blake Message-ID: <57098129.9000101@redhat.com> Date: Sat, 9 Apr 2016 16:24:41 -0600 MIME-Version: 1.0 In-Reply-To: Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="VgiXk7oFfqGPbcijtDdKXS7U58voUnqKn" Subject: Re: [Qemu-devel] [PATCH 12/18] nbd: Less allocation during NBD_OPT_LIST List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh Cc: "qemu-devel@nongnu.org" , Paolo Bonzini This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --VgiXk7oFfqGPbcijtDdKXS7U58voUnqKn Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 04/09/2016 04:41 AM, Alex Bligh wrote: >=20 > On 8 Apr 2016, at 23:05, Eric Blake wrote: >=20 >> Since we know that the maximum name we are willing to accept >> is small enough to stack-allocate, rework the iteration over >> NBD_OPT_LIST responses to reuse a stack buffer rather than >> allocating every time. Furthermore, we don't even have to >> allocate if we know the server's length doesn't match what >> we are searching for. >> >> Not fixed here: Upstream NBD Protocol recently added this >> clarification: >> https://github.com/yoe/nbd/blob/18918eb/doc/proto.md#conventions >> >> Where this document refers to a string, then unless otherwise >> stated, that string is a sequence of UTF-8 code points, which >> is not NUL terminated, MUST NOT contain NUL characters, SHOULD >> be no longer than 256 bytes and MUST be no longer than 4096 >> bytes. This applies to export names and error messages (amongst >> others). >> >> To be fully compliant to that, we need to bump our export name >> limit from 255 to at least 256, and need to decide whether we >> can bump it higher (bumping it all the way to 4096 is annoying >> in that we could no longer safely stack-allocate a worst-case >> string, so we may still want to take the leeway offered by SHOULD >> to force a reasonable smaller limit). >=20 > Is there a limit in qemu-world to safe stack allocation? I thought > that was (in general) only a kernel consideration? (probably my > ignorance here). Even in user space apps, any stack allocation larger than 4096 bytes risks skipping over the guard page on Windows, which makes the difference in whether you get a SIGSEGV (good) or a hard process kill (bad). We're slowly getting qemu to the point where it will compile with gcc's options go guarantee that no one function requires more than 4k stack. >=20 > Otherwise: >=20 >=20 > Reviewed-by: Alex Bligh >=20 And continuing from the things I mentioned in the other mail regarding export name limits... >> - return 1; >> + >> + if (len < sizeof(namelen) || len > NBD_MAX_BUFFER_SIZE) { >> + error_setg(errp, "incorrect option length %"PRIu32, len); >> + return -1; >> + } This is a case of a faulty server stream (whether evil server, or MitM, or whatever else...); if the packet wasn't big enough to include namelen, or if the message size is larger than 32M, the stream is considered corrupt to the point that it is no longer worth talking to the server (hence, the return -1). >> + if (read_sync(ioc, &namelen, sizeof(namelen)) !=3D sizeof(namelen= )) { >> + error_setg(errp, "failed to read option name length"); >> + return -1; >> + } Likewise, anywhere we fail to read the server stream (most often due to stream disconnect causing EOF), returning -1 is fine because we can't recover anyway. >> + namelen =3D be32_to_cpu(namelen); >> + len -=3D sizeof(namelen); >> + if (len < namelen) { >> + error_setg(errp, "incorrect option name length"); >> + return -1; >> + } Likewise, if the server gives a namelen that would read beyond the bounds of the overall packet length, the server can't be trusted for anything else. >> + if (namelen !=3D strlen(want)) { >> + if (drop_sync(ioc, len) !=3D len) { >> + error_setg(errp, "failed to skip export name with wrong l= ength"); >> + return -1; >> + } >> + return 2; >> + } While this gracefully handles any remaining string size, even up to qemu's NBD_MAX_BUFFER_SIZE (32M), well beyond the required 256 or recommended 4096 of the protocol. >> + >> + assert(namelen < sizeof(name)); >> + if (read_sync(ioc, name, namelen) !=3D namelen) { >> + error_setg(errp, "failed to read export name"); >> + return -1; >> + } >> + name[namelen] =3D '\0'; >> + len -=3D namelen; >> + if (drop_sync(ioc, len) !=3D len) { >> + error_setg(errp, "failed to read export description"); >> + return -1; >> + } >> + return strcmp(name, want) =3D=3D 0 ? 3 : 2; So with this patch, I've worked it into allowing any string sizes from the server, while focusing only on the strings that match the length of the name requested by the client; now the audit proceeds to find out whether letting the client request a name longer than 255 makes sense, but at least this part of the client/server interaction is safe, and ready for a later patch to bump the size of the #define for max name length, unless bumping it too large makes the local buf[] exceed preferred stack size. [It's also interesting to note that once the NBD_OPT_GO code is live and supported by more servers, we won't even be hitting this section of NBD_OPT_INFO code in the qemu client] --=20 Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org --VgiXk7oFfqGPbcijtDdKXS7U58voUnqKn 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 Comment: Public key at http://people.redhat.com/eblake/eblake.gpg Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iQEcBAEBCAAGBQJXCYEpAAoJEKeha0olJ0NqJ7oH/0r+4C15uvaBqegacZ5XvN1i z4MMxm3Q2WsXGiPeALhd/wINECmuIsgQ4zVmWN94y/Hlz93iVfHsDznXCzAxXneU x02+SuAqHgBG9eNccJjPkM3hh6zzWv4Z0qYDEHPlmluX5NU67+OzcyGvFFIozI5g +pt8X3iVO+zaQgjcBxO9TX7JpFP/fspyB6fkLtOp+pP74zOrqUKu1og79x6Q8wzw Mfp376BOJv3yuYYmg/sUhyMFd/k1wZ2vqzhk4gKAFnTBu2gLpVl51YxPd2Em9HzM IZZJpLKFQ9hZ38Voymkiqgrc1kl1ZMEiFJNzCnhYmIlb7EJoL4/19GNJVz/ovbY= =Ev6E -----END PGP SIGNATURE----- --VgiXk7oFfqGPbcijtDdKXS7U58voUnqKn--