[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option |
Date: |
Thu, 13 Sep 2012 10:39:43 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2012-09-13 07:55, Hervé 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.
>
> 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 option
> 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é Poussineau <address@hidden>
> ---
> 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,
> }
>
> 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 = 0;
> + int i, n = 0;
>
> m = m_get(spt->slirp);
>
> @@ -140,10 +140,12 @@ static int tftp_send_oack(struct tftp_session *spt,
> m->m_data += sizeof(struct udpiphdr);
>
> tp->tp_op = htons(TFTP_OACK);
> - n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
> - key) + 1;
> - n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
> - value) + 1;
> + for (i = 0; i < nb; i++) {
> + n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%s",
> + keys[i]) + 1;
> + n += snprintf(tp->x.tp_buf + n, sizeof(tp->x.tp_buf) - n, "%u",
> + values[i]) + 1;
> + }
>
> saddr.sin_addr = recv_tp->ip.ip_dst;
> saddr.sin_port = 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 = 0;
>
> /* check if a session already exists and if so terminate it */
> s = tftp_session_find(slirp, tp);
> @@ -337,7 +342,7 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t
> *tp, int pktlen)
> return;
> }
>
> - while (k < pktlen) {
> + while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) {
> const char *key, *value;
>
> key = &tp->x.tp_buf[k];
> @@ -364,11 +369,30 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t
> *tp, int pktlen)
> }
> }
>
> - tftp_send_oack(spt, "tsize", tsize, tp);
> - return;
> + option_name[nb_options] = "tsize";
> + option_value[nb_options] = tsize;
> + nb_options++;
> + } else if (strcasecmp(key, "blksize") == 0) {
> + int blksize = 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 >= 512) {
> + option_name[nb_options] = "blksize";
> + option_value[nb_options] = 512;
> + nb_options++;
> + }
> }
> }
>
> + if (nb_options > 0) {
> + assert(nb_options <= 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 = 0;
> tftp_send_next_block(spt, tp);
> }
>
--
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux