From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:36945) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TC4xn-0001zs-Vs for qemu-devel@nongnu.org; Thu, 13 Sep 2012 04:39:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TC4xh-0006z6-Ir for qemu-devel@nongnu.org; Thu, 13 Sep 2012 04:39:55 -0400 Received: from goliath.siemens.de ([192.35.17.28]:29509) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TC4xh-0006z0-94 for qemu-devel@nongnu.org; Thu, 13 Sep 2012 04:39:49 -0400 Message-ID: <50519BCF.2080001@siemens.com> Date: Thu, 13 Sep 2012 10:39:43 +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> In-Reply-To: <1347515701-5513-3-git-send-email-hpoussin@reactos.org> Content-Type: text/plain; charset=UTF-8 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: =?UTF-8?B?SGVydsOpIFBvdXNzaW5lYXU=?= Cc: "qemu-devel@nongnu.org" On 2012-09-13 07:55, Herv=C3=A9 Poussineau wrote: > This option is described in RFC 1783. As this is only an optional field= , > we may ignore it in some situations and handle it in some others. >=20 > However, MS Windows 2003 PXE boot client requests a block size of the M= TU > (most of the times 1472 bytes), and doesn't work if the option is not > acknowledged (with whatever value). >=20 > According to the RFC 1783, we cannot acknowledge the option with a bigg= er > value than the requested one. >=20 > As current implementation is using 512 bytes by block, accept the optio= n > 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. >=20 > Signed-off-by: Herv=C3=A9 Poussineau > --- > slirp/tftp.c | 42 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 33 insertions(+), 9 deletions(-) >=20 > 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 *sp= t, 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 n= b, > 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 *sp= t, > 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 tf= tp_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 tf= tp_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_options 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. Jan > + tftp_send_oack(spt, option_name, option_value, nb_options, tp); > + return; > + } > + > spt->block_nr =3D 0; > tftp_send_next_block(spt, tp); > } >=20 --=20 Siemens AG, Corporate Technology, CT RTC ITP SDP-DE Corporate Competence Center Embedded Linux