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

[Thomas Huth]

On 27.06.2016 22:29, Samuel Thibault wrote:
> 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.

Ok, I'll add a check.

>> +    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];

We don't support sizeof(int) == 2 in QEMU, so IMHO it does not make
sense to add such ugly casts here. I think one can safely assume that
sizeof(int) is at least 4 on every modern C compiler (unless you're
compiling code for an embedded 16- or 8-bit system, but QEMU won't work
there anyway).

BTW, there are also other places in the slirp code where shifting with
size bigger than 16 is done without cast ( grep -r "<< 16" slirp/*.c ),
so I really think there is no need for these casts here.

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

Not sure whether I like that here (since we're only touching 3 bytes),
but I can give it a try to see how it looks like...

>> +/**
>> + * 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.

Ok.

>> +        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).

Ok.

>> +            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?

Ok, I'll add a default case here, too.

>> +/**
>> + * 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...)

I'm afraid that won't work very well, too: The options in the DHCPv6
packet do not have any alignment requirement, so the can also start at
uneven addresses. For example if the CLIENTID option has an even length,
the following option (DNS_SERVERS if requested) will start at an uneven
offset.
So if I'd use a struct pointer here to fill in the values, this might
still work OK on x86, but it would fail on all other architectures that
can not do unaligned memory accesses (IIRC Sparc is one of those
architectures, and older versions of ARM chips).
So I'd like to keep the current code, but I can add some more comments
if you think that helps to understand the "0" for example.

>> +        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?

True, I'll change that.

>> +                 "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);
>> +}

 Thomas