Re: [Qemu-devel] [PATCH v2 3/3] slirp: Implement TFTP Blocksize option

[Jan Kiszka]

On 2012-09-10 20:05, 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 |   40 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 8 deletions(-)
> 
> diff --git a/slirp/tftp.c b/slirp/tftp.c
> index 75c9030..e02a29b 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;
> @@ -265,6 +267,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);
> @@ -369,11 +374,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 <= sizeof(option_name) / sizeof(option_name[0]));

What prevents that an evil guest triggers this? BTW, we have ARRAY_SIZE.

> +    tftp_send_oack(spt, option_name, option_value, nb_options, tp);
> +    return;

tftp_handle_rrq isn't consistent, but let's indent by 4 relative to the
condition.

> +  }
> +
>    tftp_send_data(spt, 1, tp);
>  }
>  
> 

Jan

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