From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:52680) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCFWU-0000jm-P4 for qemu-devel@nongnu.org; Thu, 13 Sep 2012 15:56:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TCFWQ-0005pK-Bz for qemu-devel@nongnu.org; Thu, 13 Sep 2012 15:56:26 -0400 Received: from smtp1-g21.free.fr ([212.27.42.1]:54327) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TCFWP-0005nC-QX for qemu-devel@nongnu.org; Thu, 13 Sep 2012 15:56:22 -0400 Message-ID: <50523A5F.3000606@reactos.org> Date: Thu, 13 Sep 2012 21:56:15 +0200 From: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= 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> In-Reply-To: <50519BCF.2080001@siemens.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable 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: Jan Kiszka Cc: "qemu-devel@nongnu.org" 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 fiel= d, >> 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 big= ger >> value than the requested one. >> >> As current implementation is using 512 bytes by block, accept the opti= on >> 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 *s= pt, 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 *s= pt, >> 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 t= ftp_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 t= ftp_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)); >=20 > 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_option= s > becomes > 2? That would crash QEMU, no? Even worse, that would not > require a privileged guest process. >=20 > Please explain why I'm wrong or make the code robust. + 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)); We're leaving the loop which fills options array if the array is already=20 filled (ie nb_options is 2). This won't cause any buffer overflow. Then, the assert is not needed, but it was only to make things clear,=20 and to prevent a potential bug later, if loop code is rewritten/changed. If guest sends a bogus request with two tsize and one blksize, the two=20 tsize answers will fill the options array and the blksize option won't=20 be processed, but I don't think it is a big problem. I hope it will answer your questions. Herv=C3=A9