[Qemu-devel] [PATCH v3 0/2] slirp: tftp server improvements

[Qemu-devel] [PATCH v3 0/2] slirp: tftp server improvements

[Hervé Poussineau]

These patches have already been sent in April 2011, and contain some fixes for the internal TFTP server.
With these patches, MS Windows PE can be booted via PXE, and 32MB file limitation has been removed.

This has been tested with MS Windows 2003 PXE boot client, PXELINUX and gPXE.

Indentation seems a little bit off in patch 2/2, because surrounding code indentation is one tab followed by spaces. I've indented new lines by replacing the tab by 8 spaces.
Patch 1/2 also has indentation errors reported by checkpatch.pl, but I kept indentation consistent.

Changes v1 -> v2
- rebased
- updated with Aurélien Jarno remarks
- improved commit messages

Changes v2 -> v3
- removed patch 1/3, already applied to slirp queue
- updated with Jan Kiszka remarks

Hervé Poussineau (2):
  slirp: Handle more than 65535 blocks in TFTP transfers
  slirp: Implement TFTP Blocksize option

 slirp/tftp.c |   66 ++++++++++++++++++++++++++++++++++++++--------------------
 slirp/tftp.h |    1 +
 2 files changed, 44 insertions(+), 23 deletions(-)

-- 
1.7.10.4

[Qemu-devel] [PATCH v3 1/2] slirp: Handle more than 65535 blocks in TFTP transfers

[Hervé Poussineau]

RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers
implement it to be able to transmit big files, so do it also.

Current block size is 512 bytes, so TFTP files were limited to 32 MB.

Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
---
 slirp/tftp.c |   24 ++++++++++--------------
 slirp/tftp.h |    1 +
 2 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/slirp/tftp.c b/slirp/tftp.c
index 520dbd6..c6a5df2 100644
--- a/slirp/tftp.c
+++ b/slirp/tftp.c
@@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
   return -1;
 }
 
-static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
+static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
                           uint8_t *buf, int len)
 {
     int bytes_read = 0;
@@ -197,19 +197,14 @@ out:
   tftp_session_terminate(spt);
 }
 
-static int tftp_send_data(struct tftp_session *spt,
-                          uint16_t block_nr,
-			  struct tftp_t *recv_tp)
+static int tftp_send_next_block(struct tftp_session *spt,
+                                struct tftp_t *recv_tp)
 {
   struct sockaddr_in saddr, daddr;
   struct mbuf *m;
   struct tftp_t *tp;
   int nobytes;
 
-  if (block_nr < 1) {
-    return -1;
-  }
-
   m = m_get(spt->slirp);
 
   if (!m) {
@@ -223,7 +218,7 @@ static int tftp_send_data(struct tftp_session *spt,
   m->m_data += sizeof(struct udpiphdr);
 
   tp->tp_op = htons(TFTP_DATA);
-  tp->x.tp_data.tp_block_nr = htons(block_nr);
+  tp->x.tp_data.tp_block_nr = htons((spt->block_nr + 1) & 0xffff);
 
   saddr.sin_addr = recv_tp->ip.ip_dst;
   saddr.sin_port = recv_tp->udp.uh_dport;
@@ -231,7 +226,7 @@ static int tftp_send_data(struct tftp_session *spt,
   daddr.sin_addr = spt->client_ip;
   daddr.sin_port = spt->client_port;
 
-  nobytes = tftp_read_data(spt, block_nr - 1, tp->x.tp_data.tp_buf, 512);
+  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf, 512);
 
   if (nobytes < 0) {
     m_free(m);
@@ -255,6 +250,7 @@ static int tftp_send_data(struct tftp_session *spt,
     tftp_session_terminate(spt);
   }
 
+  spt->block_nr++;
   return 0;
 }
 
@@ -373,7 +369,8 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
       }
   }
 
-  tftp_send_data(spt, 1, tp);
+  spt->block_nr = 0;
+  tftp_send_next_block(spt, tp);
 }
 
 static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
@@ -386,9 +383,8 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
     return;
   }
 
-  if (tftp_send_data(&slirp->tftp_sessions[s],
-		     ntohs(tp->x.tp_data.tp_block_nr) + 1,
-		     tp) < 0) {
+  if (tftp_send_next_block(&slirp->tftp_sessions[s],
+                           tp) < 0) {
     return;
   }
 }
diff --git a/slirp/tftp.h b/slirp/tftp.h
index 9c364ea..51704e4 100644
--- a/slirp/tftp.h
+++ b/slirp/tftp.h
@@ -37,6 +37,7 @@ struct tftp_session {
 
     struct in_addr client_ip;
     uint16_t client_port;
+    uint32_t block_nr;
 
     int timestamp;
 };
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v3 1/2] slirp: Handle more than 65535 blocks in TFTP transfers

[Jan Kiszka]

On 2012-09-13 07:55, Hervé Poussineau wrote:
> RFC 1350 does not mention block count roll-over. However, a lot of TFTP servers
> implement it to be able to transmit big files, so do it also.
> 
> Current block size is 512 bytes, so TFTP files were limited to 32 MB.
> 
> Signed-off-by: Hervé Poussineau <hpoussin@reactos.org>
> ---
>  slirp/tftp.c |   24 ++++++++++--------------
>  slirp/tftp.h |    1 +
>  2 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 520dbd6..c6a5df2 100644
> --- a/slirp/tftp.c
> +++ b/slirp/tftp.c
> @@ -97,7 +97,7 @@ static int tftp_session_find(Slirp *slirp, struct tftp_t *tp)
>    return -1;
>  }
>  
> -static int tftp_read_data(struct tftp_session *spt, uint16_t block_nr,
> +static int tftp_read_data(struct tftp_session *spt, uint32_t block_nr,
>                            uint8_t *buf, int len)
>  {
>      int bytes_read = 0;
> @@ -197,19 +197,14 @@ out:
>    tftp_session_terminate(spt);
>  }
>  
> -static int tftp_send_data(struct tftp_session *spt,
> -                          uint16_t block_nr,
> -			  struct tftp_t *recv_tp)
> +static int tftp_send_next_block(struct tftp_session *spt,
> +                                struct tftp_t *recv_tp)
>  {
>    struct sockaddr_in saddr, daddr;
>    struct mbuf *m;
>    struct tftp_t *tp;
>    int nobytes;
>  
> -  if (block_nr < 1) {
> -    return -1;
> -  }
> -
>    m = m_get(spt->slirp);
>  
>    if (!m) {
> @@ -223,7 +218,7 @@ static int tftp_send_data(struct tftp_session *spt,
>    m->m_data += sizeof(struct udpiphdr);
>  
>    tp->tp_op = htons(TFTP_DATA);
> -  tp->x.tp_data.tp_block_nr = htons(block_nr);
> +  tp->x.tp_data.tp_block_nr = htons((spt->block_nr + 1) & 0xffff);
>  
>    saddr.sin_addr = recv_tp->ip.ip_dst;
>    saddr.sin_port = recv_tp->udp.uh_dport;
> @@ -231,7 +226,7 @@ static int tftp_send_data(struct tftp_session *spt,
>    daddr.sin_addr = spt->client_ip;
>    daddr.sin_port = spt->client_port;
>  
> -  nobytes = tftp_read_data(spt, block_nr - 1, tp->x.tp_data.tp_buf, 512);
> +  nobytes = tftp_read_data(spt, spt->block_nr, tp->x.tp_data.tp_buf, 512);
>  
>    if (nobytes < 0) {
>      m_free(m);
> @@ -255,6 +250,7 @@ static int tftp_send_data(struct tftp_session *spt,
>      tftp_session_terminate(spt);
>    }
>  
> +  spt->block_nr++;
>    return 0;
>  }
>  
> @@ -373,7 +369,8 @@ static void tftp_handle_rrq(Slirp *slirp, struct tftp_t *tp, int pktlen)
>        }
>    }
>  
> -  tftp_send_data(spt, 1, tp);
> +  spt->block_nr = 0;
> +  tftp_send_next_block(spt, tp);
>  }
>  
>  static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
> @@ -386,9 +383,8 @@ static void tftp_handle_ack(Slirp *slirp, struct tftp_t *tp, int pktlen)
>      return;
>    }
>  
> -  if (tftp_send_data(&slirp->tftp_sessions[s],
> -		     ntohs(tp->x.tp_data.tp_block_nr) + 1,
> -		     tp) < 0) {
> +  if (tftp_send_next_block(&slirp->tftp_sessions[s],
> +                           tp) < 0) {
>      return;
>    }
>  }
> diff --git a/slirp/tftp.h b/slirp/tftp.h
> index 9c364ea..51704e4 100644
> --- a/slirp/tftp.h
> +++ b/slirp/tftp.h
> @@ -37,6 +37,7 @@ struct tftp_session {
>  
>      struct in_addr client_ip;
>      uint16_t client_port;
> +    uint32_t block_nr;
>  
>      int timestamp;
>  };
> 

Thanks, applied. I added a patch on top to remove the useless return
value of tftp_send_next_block.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
Corporate Competence Center Embedded Linux

[Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option

[Hervé Poussineau]

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 <hpoussin@reactos.org>
---
 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));
+      tftp_send_oack(spt, option_name, option_value, nb_options, tp);
+      return;
+  }
+
   spt->block_nr = 0;
   tftp_send_next_block(spt, tp);
 }
-- 
1.7.10.4

Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option

[Jan Kiszka]

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 <hpoussin@reactos.org>
> ---
>  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

Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option

[Hervé Poussineau]

Jan Kiszka a écrit :
> 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 <hpoussin@reactos.org>
>> ---
>>  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.


+  int nb_options = 0;
...
+  while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) {
...
+              option_value[nb_options] = ...
+              nb_options++;
...
+  if (nb_options > 0) {
+      assert(nb_options <= ARRAY_SIZE(option_name));


We're leaving the loop which fills options array if the array is already 
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, 
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 
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.

I hope it will answer your questions.

Hervé

Re: [Qemu-devel] [PATCH v3 2/2] slirp: Implement TFTP Blocksize option

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 5500 bytes --]

On 2012-09-13 21:56, Hervé Poussineau wrote:
> Jan Kiszka a écrit :
>> 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 <hpoussin@reactos.org>
>>> ---
>>>  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.
> 
> 
> +  int nb_options = 0;
> ...
> +  while (k < pktlen && nb_options < ARRAY_SIZE(option_name)) {
> ...
> +              option_value[nb_options] = ...
> +              nb_options++;
> ...
> +  if (nb_options > 0) {
> +      assert(nb_options <= ARRAY_SIZE(option_name));
> 
> 
> We're leaving the loop which fills options array if the array is already
> 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.
> 
> 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.
> 
> I hope it will answer your questions.

Yes. I applied it, will send a pull request soon.

Thanks,
Jan



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]