| Message ID | 1466928242-14496-1-git-send-email-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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
Thomas Huth, on Tue 28 Jun 2016 09:01:50 +0200, wrote: > The options in the DHCPv6 packet do not have any alignment > requirement, so the can also start at uneven addresses. Aow, OK :/ > 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. Just adding /* 16bit values */ should be enough, yes. Samuel
diff --git a/slirp/Makefile.objs b/slirp/Makefile.objs index 6748e4f..1baa1f1 100644 --- a/slirp/Makefile.objs +++ b/slirp/Makefile.objs @@ -1,5 +1,5 @@ common-obj-y = cksum.o if.o ip_icmp.o ip6_icmp.o ip6_input.o ip6_output.o \ - ip_input.o ip_output.o dnssearch.o + ip_input.o ip_output.o dnssearch.o dhcpv6.o common-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o tcp_output.o common-obj-y += tcp_subr.o tcp_timer.o udp.o udp6.o bootp.o tftp.o arp_table.o \ ndp_table.o diff --git a/slirp/dhcpv6.c b/slirp/dhcpv6.c new file mode 100644 index 0000000..36c20b9 --- /dev/null +++ b/slirp/dhcpv6.c @@ -0,0 +1,197 @@ +/* + * SLIRP stateless DHCPv6 + * + * We only support stateless DHCPv6, e.g. for network booting. + * See RFC 3315, RFC 3646, RFC 3736 and RFC 5970 for details. + * + * Copyright 2016 Thomas Huth, Red Hat Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, + * or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, see <http://www.gnu.org/licenses/>. + */ + +#include "qemu/osdep.h" +#include "qemu/log.h" +#include "slirp.h" +#include "dhcpv6.h" + +/* DHCPv6 message types */ +#define MSGTYPE_REPLY 7 +#define MSGTYPE_INFO_REQUEST 11 + +/* DHCPv6 option types */ +#define OPTION_CLIENTID 1 +#define OPTION_IAADDR 5 +#define OPTION_ORO 6 +#define OPTION_DNS_SERVERS 23 +#define OPTION_BOOTFILE_URL 59 + +struct requested_infos { + uint8_t *client_id; + int client_id_len; + bool want_dns; + bool want_boot_url; +}; + +/** + * 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 */ + 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; + } + 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]) { + case OPTION_DNS_SERVERS: + ri->want_dns = true; + break; + case OPTION_BOOTFILE_URL: + ri->want_boot_url = true; + break; + } + } + break; + default: + DEBUG_MISC((dfd, "dhcpv6 info req: Unsupported option %d, len=%d\n", + option, len)); + } + + odata += len + 4; + olen -= len + 4; + } + + return 0; +} + + +/** + * Handle information request messages + */ +static void dhcpv6_info_request(Slirp *slirp, struct sockaddr_in6 *srcsas, + uint32_t xid, uint8_t *odata, int olen) +{ + struct requested_infos ri = { NULL }; + struct sockaddr_in6 sa6, da6; + struct mbuf *m; + uint8_t *resp; + + if (dhcpv6_parse_info_request(odata, olen, &ri) < 0) { + return; + } + + m = m_get(slirp); + if (!m) { + return; + } + memset(m->m_data, 0, m->m_size); + m->m_data += IF_MAXLINKHDR; + resp = (uint8_t *)m->m_data + sizeof(struct ip6) + sizeof(struct udphdr); + + /* Fill in response */ + *resp++ = MSGTYPE_REPLY; + *resp++ = (uint8_t)(xid >> 16); + *resp++ = (uint8_t)(xid >> 8); + *resp++ = (uint8_t)xid; + + if (ri.client_id) { + *resp++ = 0; + *resp++ = OPTION_CLIENTID; + *resp++ = ri.client_id_len >> 8; + *resp++ = ri.client_id_len; + memcpy(resp, ri.client_id, ri.client_id_len); + resp += ri.client_id_len; + } + if (ri.want_dns) { + *resp++ = 0; + *resp++ = OPTION_DNS_SERVERS; + *resp++ = 0; + *resp++ = 16; /* Length */ + 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, + "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); +} + +/** + * Handle DHCPv6 messages sent by the client + */ +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; + + xid = data[1] << 16 | data[2] << 8 | data[3]; + + switch (data[0]) { + case MSGTYPE_INFO_REQUEST: + dhcpv6_info_request(m->slirp, srcsas, xid, &data[4], data_len - 4); + break; + default: + DEBUG_MISC((dfd, "dhcpv6_input: Unsupported message type 0x%x\n", + data[0])); + } +} diff --git a/slirp/dhcpv6.h b/slirp/dhcpv6.h new file mode 100644 index 0000000..8222fc9 --- /dev/null +++ b/slirp/dhcpv6.h @@ -0,0 +1,8 @@ +#ifndef SLIRP_DHCPV6_H +#define SLIRP_DHCPV6_H + +#define DHCPV6_SERVER_PORT 547 + +void dhcpv6_input(struct sockaddr_in6 *srcsas, struct mbuf *m); + +#endif diff --git a/slirp/ip6.h b/slirp/ip6.h index 8ddfa24..c86be84 100644 --- a/slirp/ip6.h +++ b/slirp/ip6.h @@ -14,6 +14,12 @@ 0x00, 0x00, 0x00, 0x00,\ 0x00, 0x00, 0x00, 0x01 } } +#define ALLDHCP_MULTICAST { .s6_addr = \ + { 0xff, 0x02, 0x00, 0x00,\ + 0x00, 0x00, 0x00, 0x00,\ + 0x00, 0x00, 0x00, 0x00,\ + 0x00, 0x01, 0x00, 0x02 } } + #define SOLICITED_NODE_PREFIX { .s6_addr = \ { 0xff, 0x02, 0x00, 0x00,\ 0x00, 0x00, 0x00, 0x00,\ diff --git a/slirp/udp6.c b/slirp/udp6.c index 94efb13..9fa314b 100644 --- a/slirp/udp6.c +++ b/slirp/udp6.c @@ -7,6 +7,7 @@ #include "qemu-common.h" #include "slirp.h" #include "udp.h" +#include "dhcpv6.h" void udp6_input(struct mbuf *m) { @@ -61,7 +62,17 @@ void udp6_input(struct mbuf *m) lhost.sin6_addr = ip->ip_src; lhost.sin6_port = uh->uh_sport; - /* TODO handle DHCP/BOOTP */ + /* handle DHCPv6 */ + if (ntohs(uh->uh_dport) == DHCPV6_SERVER_PORT && + (in6_equal(&ip->ip_dst, &slirp->vhost_addr6) || + in6_equal(&ip->ip_dst, &(struct in6_addr)ALLDHCP_MULTICAST))) { + m->m_data += iphlen; + m->m_len -= iphlen; + dhcpv6_input(&lhost, m); + m->m_data -= iphlen; + m->m_len += iphlen; + goto bad; + } /* handle TFTP */ if (ntohs(uh->uh_dport) == TFTP_SERVER &&
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). Tested with: qemu-system-ppc64 -nographic -vga none -boot n -net nic \ -net user,ipv6=yes,ipv4=no,tftp=/path/to/tftp,bootfile=ppc64.img Signed-off-by: Thomas Huth <thuth@redhat.com> --- slirp/Makefile.objs | 2 +- slirp/dhcpv6.c | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++++ slirp/dhcpv6.h | 8 +++ slirp/ip6.h | 6 ++ slirp/udp6.c | 13 +++- 5 files changed, 224 insertions(+), 2 deletions(-) create mode 100644 slirp/dhcpv6.c create mode 100644 slirp/dhcpv6.h