From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:56813) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCHtj-00042J-8Q for qemu-devel@nongnu.org; Thu, 13 Sep 2012 18:28:36 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCHtf-0003lu-Pu for qemu-devel@nongnu.org; Thu, 13 Sep 2012 18:28:35 -0400 Received: from mout.web.de ([212.227.15.3]:55312) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCHtf-0003ig-FU for qemu-devel@nongnu.org; Thu, 13 Sep 2012 18:28:31 -0400 Message-ID: <50525DF1.9030708@web.de> Date: Fri, 14 Sep 2012 00:28:01 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1347515701-5513-1-git-send-email-hpoussin@reactos.org> <1347515701-5513-3-git-send-email-hpoussin@reactos.org> <50519BCF.2080001@siemens.com> <50523A5F.3000606@reactos.org> In-Reply-To: <50523A5F.3000606@reactos.org> Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="------------enig34CA8A500234002D0CDB4A1F" Subject: Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= Cc: "qemu-devel@nongnu.org" This is an OpenPGP/MIME signed message (RFC 2440 and 3156) --------------enig34CA8A500234002D0CDB4A1F Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2012-09-13 21:56, Herv=C3=A9 Poussineau wrote: > Jan Kiszka a =C3=A9crit : >> On 2012-09-13 07:55, Herv=C3=A9 Poussineau wrote: >>> This option is described in RFC 1783. As this is only an optional fie= ld, >>> we may ignore it in some situations and handle it in some others. >>> >>> However, MS Windows 2003 PXE boot client requests a block size of the= >>> MTU >>> (most of the times 1472 bytes), and doesn't work if the option is not= >>> acknowledged (with whatever value). >>> >>> According to the RFC 1783, we cannot acknowledge the option with a >>> bigger >>> value than the requested one. >>> >>> As current implementation is using 512 bytes by block, accept the opt= ion >>> with a value of 512 if the option was specified, and don't >>> acknowledge it >>> if it is not present or less than 512 bytes. >>> >>> Signed-off-by: Herv=C3=A9 Poussineau >>> --- >>> slirp/tftp.c | 42 +++++++++++++++++++++++++++++++++--------- >>> 1 file changed, 33 insertions(+), 9 deletions(-) >>> >>> diff --git a/slirp/tftp.c b/slirp/tftp.c >>> index c6a5df2..37b0387 100644 >>> --- a/slirp/tftp.c >>> +++ b/slirp/tftp.c >>> @@ -120,13 +120,13 @@ static int tftp_read_data(struct tftp_session >>> *spt, uint32_t block_nr, >>> } >>> =20 >>> static int tftp_send_oack(struct tftp_session *spt, >>> - const char *key, uint32_t value, >>> + const char *keys[], uint32_t values[], int= >>> nb, >>> struct tftp_t *recv_tp) >>> { >>> struct sockaddr_in saddr, daddr; >>> struct mbuf *m; >>> struct tftp_t *tp; >>> - int n =3D 0; >>> + int i, n =3D 0; >>> =20 >>> m =3D m_get(spt->slirp); >>> =20 >>> @@ -140,10 +140,12 @@ static int tftp_send_oack(struct tftp_session >>> *spt, >>> m->m_data +=3D sizeof(struct udpiphdr); >>> =20 >>> tp->tp_op =3D htons(TFTP_OACK); >>> - n +=3D snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s"= , >>> - key) + 1; >>> - n +=3D snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u"= , >>> - value) + 1; >>> + for (i =3D 0; i < nb; i++) { >>> + n +=3D snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, = "%s", >>> + keys[i]) + 1; >>> + n +=3D snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, = "%u", >>> + values[i]) + 1; >>> + } >>> =20 >>> saddr.sin_addr =3D recv_tp->ip.ip_dst; >>> saddr.sin_port =3D recv_tp->udp.uh_dport; >>> @@ -260,6 +262,9 @@ static void tftp_handle_rrq(Slirp *slirp, struct >>> tftp_t *tp, int pktlen) >>> int s, k; >>> size_t prefix_len; >>> char *req_fname; >>> + const char *option_name[2]; >>> + uint32_t option_value[2]; >>> + int nb_options =3D 0; >>> =20 >>> /* check if a session already exists and if so terminate it */ >>> s =3D tftp_session_find(slirp, tp); >>> @@ -337,7 +342,7 @@ static void tftp_handle_rrq(Slirp *slirp, struct >>> tftp_t *tp, int pktlen) >>> return; >>> } >>> =20 >>> - while (k < pktlen) { >>> + while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) { >>> const char *key, *value; >>> =20 >>> key =3D &tp->x.tp_buf[k]; >>> @@ -364,11 +369,30 @@ static void tftp_handle_rrq(Slirp *slirp, >>> struct tftp_t *tp, int pktlen) >>> } >>> } >>> =20 >>> - tftp_send_oack(spt, "tsize", tsize, tp); >>> - return; >>> + option_name[nb_options] =3D "tsize"; >>> + option_value[nb_options] =3D tsize; >>> + nb_options++; >>> + } else if (strcasecmp(key, "blksize") =3D=3D 0) { >>> + int blksize =3D atoi(value); >>> + >>> + /* If blksize option is bigger than what we will >>> + * emit, accept the option with our packet size. >>> + * Otherwise, simply do as we didn't see the option. >>> + */ >>> + if (blksize >=3D 512) { >>> + option_name[nb_options] =3D "blksize"; >>> + option_value[nb_options] =3D 512; >>> + nb_options++; >>> + } >>> } >>> } >>> =20 >>> + if (nb_options > 0) { >>> + assert(nb_options <=3D ARRAY_SIZE(option_name)); >> >> I think you didn't answer my question: What if the guest sends a bogus= >> request with multiple tsize or blksize option entries so that nb_optio= ns >> becomes > 2? That would crash QEMU, no? Even worse, that would not >> require a privileged guest process. >> >> Please explain why I'm wrong or make the code robust. >=20 >=20 > + int nb_options =3D 0; > ... > + while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) { > ... > + option_value[nb_options] =3D ... > + nb_options++; > ... > + if (nb_options > 0) { > + assert(nb_options <=3D ARRAY_SIZE(option_name)); >=20 >=20 > We're leaving the loop which fills options array if the array is alread= y > filled (ie nb_options is 2). This won't cause any buffer overflow. Oh, someone was blind... > Then, the assert is not needed, but it was only to make things clear, > and to prevent a potential bug later, if loop code is rewritten/changed= =2E >=20 > If guest sends a bogus request with two tsize and one blksize, the two > tsize answers will fill the options array and the blksize option won't > be processed, but I don't think it is a big problem. >=20 > I hope it will answer your questions. Yes. I applied it, will send a pull request soon. Thanks, Jan --------------enig34CA8A500234002D0CDB4A1F 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.0.16 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://www.enigmail.net/ iEYEARECAAYFAlBSXfQACgkQitSsb3rl5xQa6ACeIENZc78jRu3OUrEj6ou42y/z o3cAn3mpqxDj53BQS+HYDBe0csK2jJj0 =OYBs -----END PGP SIGNATURE----- --------------enig34CA8A500234002D0CDB4A1F--