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

[Jan Kiszka <jan.kiszka@web.de>]

[-- 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 --]