On Sat, 10 Aug 2024 19:00:12 +0300 AbdAlRahman Gad wrote:

- Add structs for NA, RA, NS, MTU, prefix info, option header, link-layer address, RDNSS, DNSSL and link-layer for RA message.

- Turn NA message from purely imperative, going byte by byte, to declarative by filling it's struct.

- Turn part of RA message into declarative.

- Move packet_add() to be before the call of ndp() in tap6_handler() if the protocol of the packet is ICMPv6.

- Add a pool of packets as an additional parameter to ndp().

- Check the size of NS packet with packet_get() before sending an NA packet.

- Add documentation for the structs.

- Add an enum for NDP option types.

Link: https://bugs.passt.top/show_bug.cgi?id=21

Signed-off-by: AbdAlRahman Gad

Thanks! It finally looks like code we don't have to be ashamed of. One actual issue and three very small details:

--- ndp.c | 320 +++++++++++++++++++++++++++++++++++++++++++--------------- ndp.h | 3 +- tap.c | 5 +- 3 files changed, 246 insertions(+), 82 deletions(-)

diff --git a/ndp.c b/ndp.c index cea3df5..5e9cb78 100644 --- a/ndp.c +++ b/ndp.c @@ -38,22 +38,199 @@ #define NS 135 #define NA 136

+enum ndp_option_types { + OPT_SRC_L2_ADDR = 1, + OPT_TARGET_L2_ADDR = 2, + OPT_PREFIX_INFO = 3, + OPT_MTU = 5, + OPT_RDNSS_TYPE = 25, + OPT_DNSSL_TYPE = 31, +}; + +/** + * struct opt_header - Option header + * @type: Option type + * @len: Option length, in units of 8 bytes +*/ +struct opt_header { + uint8_t type; + uint8_t len; +} __attribute__((packed)); + +/** + * struct opt_l2_addr - Link-layer address + * @header: Option header + * @mac: MAC address + */ +struct opt_l2_addr { + struct opt_header header; + unsigned char mac[ETH_ALEN]; +} __attribute__((packed)); + +/** + * struct ndp_na - NDP Neighbor Advertisement (NA) message + * @ih: ICMPv6 header + * @target_addr: Target IPv6 address + * @target_l2_addr: Target link-layer address + */ +struct ndp_na { + struct icmp6hdr ih; + struct in6_addr target_addr; + struct opt_l2_addr target_l2_addr; +} __attribute__((packed)); + +/** + * struct opt_prefix_info - Prefix Information option + * @header: Option header + * @prefix_len: The number of leading bits in the Prefix that are valid + * @prefix_flags: Flags associated with the prefix + * @valid_lifetime: Valid lifetime (ms) + * @pref_lifetime: Preferred lifetime (ms) + * @reserved: Unused + */ +struct opt_prefix_info { + struct opt_header header; + uint8_t prefix_len; + uint8_t prefix_flags; + uint32_t valid_lifetime; + uint32_t pref_lifetime; + uint32_t reserved; +} __attribute__((packed)); + +/** + * struct mtu_opt - Maximum transmission unit (MTU) option + * @header: Option header + * @reserved: Unused + * @value: MTU value, network order + */ +struct mtu_opt { + struct opt_header header; + uint16_t reserved; + uint32_t value; +} __attribute__((packed)); + +/** + * struct rdnss - Recursive DNS Server (RDNSS) option + * @header: Option header + * @reserved: Unused + * @lifetime: Validity time (s) + * @dns: List of DNS server addresses + */ +struct opt_rdnss { + struct opt_header header; + uint16_t reserved; + uint32_t lifetime; + struct in6_addr dns[MAXNS + 1]; +} __attribute__((packed)); + +/** + * struct dnssl - DNS Search List (DNSSL) option + * @header: Option header + * @reserved: Unused + * @lifetime: Validity time (s) + * @domains: List of NULL-seperated search domains + */ +struct opt_dnssl { + struct opt_header header; + uint16_t reserved; + uint32_t lifetime; + unsigned char domains[MAXDNSRCH * NS_MAXDNAME]; +} __attribute__((packed)); + +/** + * struct ndp_ra - NDP Router Advertisement (RA) message + * @ih: ICMPv6 header + * @reachable: Reachability time, after confirmation (ms) + * @retrans: Time between retransmitted NS messages (ms) + * @prefix_info: Prefix Information option + * @prefix: IPv6 prefix + * @mtu: MTU option + * @source_ll: Target link-layer address + * @dns: RDNSS and DNSSL options + */ +struct ndp_ra { + struct icmp6hdr ih; + uint32_t reachable; + uint32_t retrans; + struct opt_prefix_info prefix_info; + struct in6_addr prefix; + struct mtu_opt mtu; + struct opt_l2_addr source_ll; + + unsigned char dns[sizeof(struct opt_rdnss) + sizeof(struct opt_dnssl)]; +} __attribute__((packed)); + +/** + * struct ndp_ns - NDP Neighbor Solicitation (NS) message + * @ih: ICMPv6 header + * @target_addr: Target IPv6 address

Here and in the line above, for consistency, I'd rather use two tabs instead of a tab and a space.

+ */ +struct ndp_ns { + struct icmp6hdr ih; + struct in6_addr target_addr; +} __attribute__((packed)); + /** * ndp() - Check for NDP solicitations, reply as needed * @c: Execution context * @ih: ICMPv6 header - * @saddr Source IPv6 address + * @saddr: Source IPv6 address + * @p: Packet pool * * Return: 0 if not handled here, 1 if handled, -1 on failure */ -int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr) +int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr, + const struct pool *p) { + struct ndp_na na = { + .ih = { + .icmp6_type = NA, + .icmp6_code = 0, + .icmp6_router = 1, + .icmp6_solicited = 1, + .icmp6_override = 1, + }, + .target_l2_addr = { + .header = { + .type = OPT_TARGET_L2_ADDR, + .len = 1, + }, + } + }; + struct ndp_ra ra = { + .ih = { + .icmp6_type = RA, + .icmp6_code = 0, + .icmp6_hop_limit = 255, + /* RFC 8319 */ + .icmp6_rt_lifetime = htons_constant(65535), + .icmp6_addrconf_managed = 1

A comma at the end of this initialisation is not needed, but since you added it to all the other lines (I also do), you could also have it here for consistency.

+ }, + .prefix_info = { + .header = { + .type = OPT_PREFIX_INFO, + .len = 4, + }, + .prefix_len = 64, + .prefix_flags = 0xc0, /* prefix flags: L, A */ + .valid_lifetime = ~0U, + .pref_lifetime = ~0U, + }, + .mtu = { + .header = { + .type = OPT_MTU, + .len = 1, + }, + }, + .source_ll = { + .header = { + .type = OPT_SRC_L2_ADDR, + .len = 1, + }, + }, + }; const struct in6_addr *rsaddr; /* src addr for reply */ - char buf[BUFSIZ] = { 0 }; - struct ipv6hdr *ip6hr; - struct icmp6hdr *ihr; - struct ethhdr *ehr; - unsigned char *p; + unsigned char *ptr = NULL; size_t dlen;

if (ih->icmp6_type < RS || ih->icmp6_type > NA) @@ -62,28 +239,22 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr) if (c->no_ndp) return 1;

- ehr = (struct ethhdr *)buf; - ip6hr = (struct ipv6hdr *)(ehr + 1); - ihr = (struct icmp6hdr *)(ip6hr + 1); - if (ih->icmp6_type == NS) { + struct ndp_ns *ns = packet_get(p, 0, 0, sizeof(struct ndp_ns), + NULL); + + if (!ns) + return -1; + if (IN6_IS_ADDR_UNSPECIFIED(saddr)) return 1;

info("NDP: received NS, sending NA"); - ihr->icmp6_type = NA; - ihr->icmp6_code = 0; - ihr->icmp6_router = 1; - ihr->icmp6_solicited = 1; - ihr->icmp6_override = 1; - - p = (unsigned char *)(ihr + 1); - memcpy(p, ih + 1, sizeof(struct in6_addr)); /* target address */ - p += 16; - *p++ = 2; /* target ll */ - *p++ = 1; /* length */ - memcpy(p, c->mac, ETH_ALEN); - p += 6; + + memcpy(&na.target_addr, &ns->target_addr, + sizeof(na.target_addr)); + memcpy(na.target_l2_addr.mac, c->mac, ETH_ALEN); + } else if (ih->icmp6_type == RS) { size_t dns_s_len = 0; int i, n; @@ -92,91 +263,76 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr) return 1;

info("NDP: received RS, sending RA"); - ihr->icmp6_type = RA; - ihr->icmp6_code = 0; - ihr->icmp6_hop_limit = 255; - ihr->icmp6_rt_lifetime = htons(65535); /* RFC 8319 */ - ihr->icmp6_addrconf_managed = 1; - - p = (unsigned char *)(ihr + 1); - p += 8; /* reachable, retrans time */ - *p++ = 3; /* prefix */ - *p++ = 4; /* length */ - *p++ = 64; /* prefix length */ - *p++ = 0xc0; /* prefix flags: L, A */ - *(uint32_t *)p = (uint32_t)~0U; /* lifetime */ - p += 4; - *(uint32_t *)p = (uint32_t)~0U; /* preferred lifetime */ - p += 8; - memcpy(p, &c->ip6.addr, 8); /* prefix */ - p += 16; - - if (c->mtu != -1) { - *p++ = 5; /* type */ - *p++ = 1; /* length */ - p += 2; /* reserved */ - *(uint32_t *)p = htonl(c->mtu); /* MTU */ - p += 4; - } + memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix)); + + if (c->mtu != -1) + ra.mtu.value = htonl(c->mtu);

I just realised that this might send a zero-valued MTU option. If c->mtu is -1, it means we don't want to assign any value. This happens if the user specifies an option for --mtu / -m, with 0 as value. From conf(): if (!c->mtu) { c->mtu = -1; break; } ...but according to RFC 4861, there's no special value, so sending an MTU option with value 0 (that's how this is initialised) is not really something we should be doing. The original code was omitting the option altogether, and I think we should keep doing it. I tested this with -m 0, by the way, and the Linux kernel is happy with it. It just ignores it, and I get 1500 bytes as MTU (default value), as expected. But it might break different kernel versions or other operating systems. So I think we should also handle this option in a similar way as we deal with the DNS options: if c->mtu is -1, we shouldn't add the option at all. You could replace the current 'dns' field in struct ndp_ra with something like 'var' (for variable fields), or 'opts' (opt_opts sounds pretty bad), which also includes the size of the MTU option. You could then point ptr to it, and increase it if the MTU option is present.

+ + ptr = &ra.dns[0];

if (c->no_dhcp_dns) goto dns_done;

for (n = 0; !IN6_IS_ADDR_UNSPECIFIED(&c->ip6.dns[n]); n++); if (n) { - *p++ = 25; /* RDNSS */ - *p++ = 1 + 2 * n; /* length */ - p += 2; /* reserved */ - *(uint32_t *)p = (uint32_t)~0U; /* lifetime */ - p += 4; - + struct opt_rdnss *rdnss = (struct opt_rdnss *)ptr; + *rdnss = (struct opt_rdnss) { + .header = { + .type = OPT_RDNSS_TYPE, + .len = 1 + 2 * n, + }, + .lifetime = ~0U, + }; for (i = 0; i < n; i++) { - memcpy(p, &c->ip6.dns[i], 16); /* address */ - p += 16; + memcpy(&rdnss->dns[i], &c->ip6.dns[i], + sizeof(rdnss->dns[i])); } + ptr += offsetof(struct opt_rdnss, dns) + + i * sizeof(rdnss->dns[0]);

In the Linux kernel-like coding style, we align things logically, so if i * sizeof(rdnss->dns[0]) is an operand of the same operation as offsetof(struct opt_rdnss, dns), they should be aligned, like this: ptr += offsetof(struct opt_rdnss, dns) + i * sizeof(rdnss->dns[0]);

for (n = 0; *c->dns_search[n].n; n++) dns_s_len += strlen(c->dns_search[n].n) + 2; }

if (!c->no_dhcp_dns_search && dns_s_len) { - *p++ = 31; /* DNSSL */ - *p++ = (dns_s_len + 8 - 1) / 8 + 1; /* length */ - p += 2; /* reserved */ - *(uint32_t *)p = (uint32_t)~0U; /* lifetime */ - p += 4; + struct opt_dnssl *dnssl = (struct opt_dnssl *)ptr; + *dnssl = (struct opt_dnssl) { + .header = { + .type = OPT_DNSSL_TYPE, + .len = DIV_ROUND_UP(dns_s_len, 8) + 1, + }, + .lifetime = ~0U, + }; + ptr = dnssl->domains;

for (i = 0; i < n; i++) { + size_t len; char *dot;

- *(p++) = '.'; + *(ptr++) = '.';

- strncpy((char *)p, c->dns_search[i].n, - sizeof(buf) - - ((intptr_t)p - (intptr_t)buf)); - for (dot = (char *)p - 1; *dot; dot++) { + len = sizeof(dnssl->domains) - + (ptr - dnssl->domains); + + strncpy((char *)ptr, c->dns_search[i].n, len); + for (dot = (char *)ptr - 1; *dot; dot++) { if (*dot == '.') *dot = strcspn(dot + 1, "."); } - p += strlen(c->dns_search[i].n); - *(p++) = 0; + ptr += strlen(c->dns_search[i].n); + *(ptr++) = 0; }

- memset(p, 0, 8 - dns_s_len % 8); /* padding */ - p += 8 - dns_s_len % 8; + memset(ptr, 0, 8 - dns_s_len % 8); /* padding */ + ptr += 8 - dns_s_len % 8; }

dns_done: - *p++ = 1; /* source ll */ - *p++ = 1; /* length */ - memcpy(p, c->mac, ETH_ALEN); - p += 6; + memcpy(&ra.source_ll.mac, c->mac, ETH_ALEN); } else { return 1; }

- dlen = (uintptr_t)p - (uintptr_t)ihr - sizeof(*ihr); - if (IN6_IS_ADDR_LINKLOCAL(saddr)) c->ip6.addr_ll_seen = *saddr; else @@ -187,7 +343,13 @@ dns_done: else rsaddr = &c->ip6.addr_ll;

- tap_icmp6_send(c, rsaddr, saddr, ihr, dlen + sizeof(*ihr)); + if (ih->icmp6_type == NS) { + dlen = sizeof(struct ndp_na); + tap_icmp6_send(c, rsaddr, saddr, &na, dlen); + } else if (ih->icmp6_type == RS) { + dlen = ptr - (unsigned char *)&ra; + tap_icmp6_send(c, rsaddr, saddr, &ra, dlen); + }

return 1; } diff --git a/ndp.h b/ndp.h index b33cd69..a786441 100644 --- a/ndp.h +++ b/ndp.h @@ -6,6 +6,7 @@ #ifndef NDP_H #define NDP_H

-int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr); +int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr, + const struct pool *p);

#endif /* NDP_H */ diff --git a/tap.c b/tap.c index 5852705..87be3a6 100644 --- a/tap.c +++ b/tap.c @@ -808,12 +808,13 @@ resume: if (l4len < sizeof(struct icmp6hdr)) continue;

- if (ndp(c, (struct icmp6hdr *)l4h, saddr)) + packet_add(pkt, l4len, l4h); + + if (ndp(c, (struct icmp6hdr *)l4h, saddr, pkt)) continue;

tap_packet_debug(NULL, ip6h, NULL, proto, NULL, 1);

- packet_add(pkt, l4len, l4h); icmp_tap_handler(c, PIF_TAP, AF_INET6, saddr, daddr, pkt, now); continue;

The rest looks great to me. -- Stefano