Re: [Qemu-devel] [PATCH] slirp: Add support for stateless DHCPv6

[Samuel Thibault <samuel.thibault@gnu.org>]

Hello,

Thomas Huth, on Sun 26 Jun 2016 10:04:02 +0200, wrote:
> Provide basic support for stateless DHCPv6 (see RFC 3736) so
> that guests can also automatically boot via IPv6 with SLIRP
> (for IPv6 network booting, see RFC 5970 for details).

Cool :)

I'm here commenting in my reading order, not the file order.

> +void dhcpv6_input(struct sockaddr_in6 *srcsas, struct mbuf *m)
> +{
> +    uint8_t *data = (uint8_t *)m->m_data + sizeof(struct udphdr);
> +    int data_len = m->m_len - sizeof(struct udphdr);
> +    uint32_t xid;

We need to make sure that data_len >= 4 here.

> +    xid = data[1] << 16 | data[2] << 8 | data[3];

Mmm, strictly speaking, this breaks on systems where int is 16bit only.
I guess in the context of qemu we are fine, but it's probably better
to avoid leaving code like this, in case somebody copies it for e.g. a
64bit value. It's a bit tedious, but

    xid = (uint32_t) data[1] << 16 |
          (uint32_t) data[2] << 8 |
	  data[3];

would thus be preferrable. Or we could cast to uint32_t*, use ntohs, and
mask out the high 8 bits.

> +/**
> + * Analyze the info request message sent by the client to
> + * see what data it provided and what it wants to have.
> + */
> +static int dhcpv6_parse_info_request(uint8_t *odata, int olen,
> +                                     struct requested_infos *ri)
> +{
> +    int i;
> +
> +    while (olen > 0) {
> +        /* Parse one option */

Here we need to check that olen >= 4.

> +        int option = odata[0] << 8 | odata[1];
> +        int len = odata[2] << 8 | odata[3];
> +
> +        if (len + 4 > olen) {
> +            qemu_log_mask(LOG_GUEST_ERROR, "Guest sent bad DHCPv6 packet!\n");
> +            return -E2BIG;
> +        }
> +
> +        switch (option) {
> +        case OPTION_IAADDR:
> +            /* According to RFC3315, we must discard requests with IA option */
> +            return -EINVAL;
> +        case OPTION_CLIENTID:
> +            if (len > 256) {
> +                /* Avoid very long IDs which could cause problems later */
> +                return -E2BIG;
> +            }

Maybe document in the comment of the dhcpv6_parse_info_request function
that it fills struct requested_infos with pointers withing odata (and
thus the validity is not beyond the odata liveness).

> +            ri->client_id = odata + 4;
> +            ri->client_id_len = len;
> +            break;
> +        case OPTION_ORO:        /* Option request option */
> +            if (len & 1) {
> +                return -EINVAL;
> +            }
> +            /* Check which options the client wants to have */
> +            for (i = 0; i < len; i += 2) {
> +                switch (odata[4 + i * 2] << 8 | odata[4 + i * 2 + 1]) {

Ok, this time this is always valid in C, that looks fine enough to me :)

> +                case OPTION_DNS_SERVERS:
> +                    ri->want_dns = true;
> +                    break;
> +                case OPTION_BOOTFILE_URL:
> +                    ri->want_boot_url = true;
> +                    break;

Perhaps add a DEBUG_MISC for unsupported option requests?

> +/**
> + * Handle information request messages
> + */
> +static void dhcpv6_info_request(Slirp *slirp, struct sockaddr_in6 *srcsas,
> +                                uint32_t xid, uint8_t *odata, int olen)
> +{
[...]
> +    if (ri.client_id) {
> +        *resp++ = 0;
> +        *resp++ = OPTION_CLIENTID;
> +        *resp++ = ri.client_id_len >> 8;
> +        *resp++ = ri.client_id_len;

That does not look good.  I'd say introduce an explicit struct with
uint16_t, and using htons(), I think it will look much nicer (at first I
was even wondering what that 0 standed for...)

> +        memcpy(resp, ri.client_id, ri.client_id_len);
> +        resp += ri.client_id_len;
G +    }
> +    if (ri.want_dns) {
> +        *resp++ = 0;
> +        *resp++ = OPTION_DNS_SERVERS;
> +        *resp++ = 0;
> +        *resp++ = 16;                   /* Length */

(ditto for the structure)

> +        memcpy(resp, &slirp->vnameserver_addr6, 16);
> +        resp += 16;
> +    }
> +    if (ri.want_boot_url) {
> +        uint8_t *sa = slirp->vhost_addr6.s6_addr;
> +        int slen;
> +
> +        *resp++ = 0;
> +        *resp++ = OPTION_BOOTFILE_URL;
> +        snprintf((char *)resp + 2, (uint8_t *)m->m_data + IF_MTU - resp - 2,

We'd need an extra -1 for the trailing \0 used by the following strlen,
don't we?

But we could as well use the value returned by snprintf which is exactly
what we want to have in slen, don't we?

> +                 "tftp://[%02x%02x:%02x%02x:%02x%02x:%02x%02x:"
> +                         "%02x%02x:%02x%02x:%02x%02x:%02x%02x]/%s",
> +                 sa[0], sa[1], sa[2], sa[3], sa[4], sa[5], sa[6], sa[7],
> +                 sa[8], sa[9], sa[10], sa[11], sa[12], sa[13], sa[14], sa[15],
> +                 slirp->bootp_filename);
> +        slen = strlen((char *)resp + 2);
> +        *resp++ = slen >> 8;
> +        *resp++ = slen;
> +        resp += slen;
> +    }
> +
> +    sa6.sin6_addr = slirp->vhost_addr6;
> +    sa6.sin6_port = DHCPV6_SERVER_PORT;
> +    da6.sin6_addr = srcsas->sin6_addr;
> +    da6.sin6_port = srcsas->sin6_port;
> +    m->m_data += sizeof(struct ip6) + sizeof(struct udphdr);
> +    m->m_len = resp - (uint8_t *)m->m_data;
> +    udp6_output(NULL, m, &sa6, &da6);
> +}

Thanks!
Samuel