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

["Hervé Poussineau" <hpoussin@reactos.org>]

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é