August 2024 - dev

[PATCH] flow: Don't crash if guest attempts to connect to port 0
by David Gibson 14 Aug '24

14 Aug '24

Using a zero port on TCP or UDP is dubious, and we can't really deal with forwarding such a flow within the constraints of the socket API. Hence we ASSERT()ed that we had non-zero ports in flow_hash(). The intention was to make sure that the protocol code sanitizes such ports before completing a flow entry. Unfortunately, flow_hash() is also called on new packets to see if they have an existing flow, so the unsanitized guest packet can crash passt with the assert. Correct this by moving the assert from flow_hash() to flow_sidx_hash() which is only used on entries already in the table, not on unsanitized data. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- flow.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/flow.c b/flow.c index 687e9fd0..93b687dc 100644 --- a/flow.c +++ b/flow.c @@ -561,12 +561,6 @@ static uint64_t flow_hash(const struct ctx *c, uint8_t proto, uint8_t pif, { struct siphash_state state = SIPHASH_INIT(c->hash_secret); - /* For the hash table to work, we need complete endpoint information, - * and at least a forwarding port. - */ - ASSERT(pif != PIF_NONE && !inany_is_unspecified(&side->eaddr) && - side->eport != 0 && side->fport != 0); - inany_siphash_feed(&state, &side->faddr); inany_siphash_feed(&state, &side->eaddr); @@ -586,8 +580,16 @@ static uint64_t flow_hash(const struct ctx *c, uint8_t proto, uint8_t pif, static uint64_t flow_sidx_hash(const struct ctx *c, flow_sidx_t sidx) { const struct flow_common *f = &flow_at_sidx(sidx)->f; - return flow_hash(c, FLOW_PROTO(f), - f->pif[sidx.sidei], &f->side[sidx.sidei]); + const struct flowside *side = &f->side[sidx.sidei]; + uint8_t pif = f->pif[sidx.sidei]; + + /* For the hash table to work, entries must have complete endpoint + * information, and at least a forwarding port. + */ + ASSERT(pif != PIF_NONE && !inany_is_unspecified(&side->eaddr) && + side->eport != 0 && side->fport != 0); + + return flow_hash(c, FLOW_PROTO(f), pif, side); } /** -- 2.46.0

2 1

[PATCH v2] ndp.c: Turn NDP responder into more declarative implementation
by AbdAlRahman Gad 13 Aug '24

13 Aug '24

- 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 <abdobngad(a)gmail.com> --- ndp.c | 317 ++++++++++++++++++++++++++++++++++++++++++++-------------- ndp.h | 3 +- tap.c | 5 +- 3 files changed, 246 insertions(+), 79 deletions(-) diff --git a/ndp.c b/ndp.c index cea3df5..9a80397 100644 --- a/ndp.c +++ b/ndp.c @@ -38,22 +38,193 @@ #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 + * @var: Variable fields + */ +struct ndp_ra { + struct icmp6hdr ih; + uint32_t reachable; + uint32_t retrans; + struct opt_prefix_info prefix_info; + struct in6_addr prefix; + struct opt_l2_addr source_ll; + + unsigned char var[sizeof(struct mtu_opt) +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 + */ +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, + }, + .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, + }, + .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 +233,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,31 +257,20 @@ 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; + memcpy(&ra.prefix, &c->ip6.addr, sizeof(ra.prefix)); + + ptr = &ra.var[0]; if (c->mtu != -1) { - *p++ = 5; /* type */ - *p++ = 1; /* length */ - p += 2; /* reserved */ - *(uint32_t *)p = htonl(c->mtu); /* MTU */ - p += 4; + struct mtu_opt *mtu = (struct mtu_opt *)ptr; + *mtu = (struct mtu_opt) { + .header = { + .type = OPT_MTU, + .len = 1, + }, + .value = htonl(c->mtu), + }; + ptr += sizeof(struct mtu_opt); } if (c->no_dhcp_dns) @@ -124,59 +278,64 @@ int ndp(struct ctx *c, const struct icmp6hdr *ih, const struct in6_addr *saddr) 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]); 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 +346,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; -- 2.46.0

2 2

[PATCH] ndp.c: Turn NDP responder into more declarative implementation
by AbdAlRahman Gad 12 Aug '24

12 Aug '24

- 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 <abdobngad(a)gmail.com> --- 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 + */ +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 + }, + .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); + + 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]); 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; -- 2.46.0

2 1

[PATCH] log: Don't prefix message with timestamp on --debug if it's a continuation
by Stefano Brivio 12 Aug '24

12 Aug '24

If we prefix the second part of messages printed through logmsg_perror() by the timestamp, on debug, we'll have two timestamps and a weird separator in the result, such as this beauty: 0.0013: Failed to clone process with detached namespaces0.0013: : Operation not permitted Add a parameter to logmsg() and vlogmsg() which indicates a message continuation. If that's set, don't print the timestamp in vlogmsg(). Link: https://github.com/moby/moby/issues/48257#issuecomment-2282875092 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- flow.c | 3 ++- log.c | 14 ++++++++------ log.h | 22 +++++++++++----------- 3 files changed, 21 insertions(+), 18 deletions(-) diff --git a/flow.c b/flow.c index bd5fa2b..687e9fd 100644 --- a/flow.c +++ b/flow.c @@ -279,7 +279,8 @@ void flow_log_(const struct flow_common *f, int pri, const char *fmt, ...) else type_or_state = FLOW_TYPE(f); - logmsg(true, pri, "Flow %u (%s): %s", flow_idx(f), type_or_state, msg); + logmsg(true, false, pri, + "Flow %u (%s): %s", flow_idx(f), type_or_state, msg); } /** diff --git a/log.c b/log.c index 0c377cc..e7202d0 100644 --- a/log.c +++ b/log.c @@ -253,11 +253,12 @@ static void logfile_write(bool newline, int pri, const struct timespec *now, /** * vlogmsg() - Print or send messages to log or output files as configured * @newline: Append newline at the end of the message, if missing + * @cont: Continuation of a previous message, on the same line * @pri: Facility and level map, same as priority for vsyslog() * @format: Message * @ap: Variable argument list */ -void vlogmsg(bool newline, int pri, const char *format, va_list ap) +void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap) { bool debug_print = (log_mask & LOG_MASK(LOG_DEBUG)) && log_file == -1; const struct timespec *now; @@ -265,7 +266,7 @@ void vlogmsg(bool newline, int pri, const char *format, va_list ap) now = logtime(&ts); - if (debug_print) { + if (debug_print && !cont) { char timestr[LOGTIME_STRLEN]; logtime_fmt(timestr, sizeof(timestr), now); @@ -295,15 +296,16 @@ void vlogmsg(bool newline, int pri, const char *format, va_list ap) /** * logmsg() - vlogmsg() wrapper for variable argument lists * @newline: Append newline at the end of the message, if missing + * @cont: Continuation of a previous message, on the same line * @pri: Facility and level map, same as priority for vsyslog() * @format: Message */ -void logmsg(bool newline, int pri, const char *format, ...) +void logmsg(bool newline, bool cont, int pri, const char *format, ...) { va_list ap; va_start(ap, format); - vlogmsg(newline, pri, format, ap); + vlogmsg(newline, cont, pri, format, ap); va_end(ap); } @@ -318,10 +320,10 @@ void logmsg_perror(int pri, const char *format, ...) va_list ap; va_start(ap, format); - vlogmsg(false, pri, format, ap); + vlogmsg(false, false, pri, format, ap); va_end(ap); - logmsg(true, pri, ": %s", strerror(errno_copy)); + logmsg(true, true, pri, ": %s", strerror(errno_copy)); } /** diff --git a/log.h b/log.h index ba17f79..a30b091 100644 --- a/log.h +++ b/log.h @@ -13,21 +13,21 @@ #define LOGFILE_CUT_RATIO 30 /* When full, cut ~30% size */ #define LOGFILE_SIZE_MIN (5UL * MAX(BUFSIZ, PAGE_SIZE)) -void vlogmsg(bool newline, int pri, const char *format, va_list ap); -void logmsg(bool newline, int pri, const char *format, ...) - __attribute__((format(printf, 3, 4))); +void vlogmsg(bool newline, bool cont, int pri, const char *format, va_list ap); +void logmsg(bool newline, bool cont, int pri, const char *format, ...) + __attribute__((format(printf, 4, 5))); void logmsg_perror(int pri, const char *format, ...) __attribute__((format(printf, 2, 3))); -#define err(...) logmsg(true, LOG_ERR, __VA_ARGS__) -#define warn(...) logmsg(true, LOG_WARNING, __VA_ARGS__) -#define info(...) logmsg(true, LOG_INFO, __VA_ARGS__) -#define debug(...) logmsg(true, LOG_DEBUG, __VA_ARGS__) +#define err(...) logmsg(true, false, LOG_ERR, __VA_ARGS__) +#define warn(...) logmsg(true, false, LOG_WARNING, __VA_ARGS__) +#define info(...) logmsg(true, false, LOG_INFO, __VA_ARGS__) +#define debug(...) logmsg(true, false, LOG_DEBUG, __VA_ARGS__) -#define err_perror(...) logmsg_perror( LOG_ERR, __VA_ARGS__) -#define warn_perror(...) logmsg_perror( LOG_WARNING, __VA_ARGS__) -#define info_perror(...) logmsg_perror( LOG_INFO, __VA_ARGS__) -#define debug_perror(...) logmsg_perror( LOG_DEBUG, __VA_ARGS__) +#define err_perror(...) logmsg_perror( LOG_ERR, __VA_ARGS__) +#define warn_perror(...) logmsg_perror( LOG_WARNING, __VA_ARGS__) +#define info_perror(...) logmsg_perror( LOG_INFO, __VA_ARGS__) +#define debug_perror(...) logmsg_perror( LOG_DEBUG, __VA_ARGS__) #define die(...) \ do { \ -- 2.43.0

2 1

[PATCH v3] conf: Stop parsing options at first non-option argument
by Stefano Brivio 09 Aug '24

09 Aug '24

Given that pasta supports specifying a command to be executed on the command line, even without the usual -- separator as long as there's no ambiguity, we shouldn't eat up options that are not meant for us. Paul reports, for instance, that with: pasta --config-net ip -6 route -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. That's because getopt_long(), by default, shuffles the argument list to shift non-option arguments at the end. Avoid that by adding '+' at the beginning of 'optstring'. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v3: Use '+' in optstring and drop first non-option tracking v2: Instead of overriding 'name' in the getopt_long() loop, to force exiting the loop, adjust the exit condition conf.c | 4 ++-- util.c | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/conf.c b/conf.c index 2f5d649..a79e7a6 100644 --- a/conf.c +++ b/conf.c @@ -1253,9 +1253,9 @@ void conf(struct ctx *c, int argc, char **argv) if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "+dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; } else { - optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "+dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; diff --git a/util.c b/util.c index 7761bd3..0b41404 100644 --- a/util.c +++ b/util.c @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name, rc; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "+:F", optfd, NULL); if (name == 'F') { errno = 0; -- 2.43.0

3 7

[PATCH v2 00/22] RFC: Proof-of-concept based exeter+Avocado tests
by David Gibson 09 Aug '24

09 Aug '24

Here's a rough proof of concept showing how we could run tests for passt with Avocado and the exeter library I recently created. It includes Cleber's patch adding some basic Avocado tests and builds on that. The current draft is pretty janky: * The build rules to download and install the necessary pieces are messy * We create the Avocado job files from the exeter sources in the Makefile. Ideally Avocado would eventually be extended to handle this itself * The names that Avocado sees for each test are overlong * There's some hacks to make sure things are executed from the right working directory But, it's a starting point. Stefano, If you could look particularly at 6/22 and 22/22 which add the real tests for passt/pasta, that would be great. The more specific you can be about what you find ugly about how the tests are written, then better I can try to address that. I suspect it will be easier to actually apply the series, then look at the new test files (test/build/build.py, and test/pasta/pasta.py particularly). From there you can look at as much of the support library as you need to, rather than digging through the actual patches to look for that. Cleber, If you could look at 4..6/22 particularly to review how I'm connecting the actual tests to the Avocado runner, that would be helpful. Cleber Rosa (1): test: run static checkers with Avocado and JSON definitions David Gibson (21): nstool: Fix some trivial typos nstool: Propagate SIGTERM to processes executed in the namespace test: Extend make targets to run Avocado tests test: Exeter based static tests test: Add exeter+Avocado based build tests test: Add linters for Python code tasst: Introduce library of common test helpers tasst: "snh" module for simulated network hosts tasst: Add helper to get network interface names for a site tasst: Add helpers to run commands with nstool tasst: Add ifup and network address helpers to SimNetHost tasst: Helper for creating veth devices between namespaces tasst: Add helper for getting MTU of a network interface tasst: Add helper to wait for IP address to appear tasst: Add helpers for getting a SimNetHost's routes tasst: Helpers to test transferring data between sites tasst: IP address allocation helpers tasst: Helpers for testing NDP behaviour tasst: Helpers for testing DHCP & DHCPv6 behaviour tasst: Helpers to construct a simple network environment for tests avocado: Convert basic pasta tests test/.gitignore | 2 + test/Makefile | 63 ++++++- test/avocado/static_checkers.json | 16 ++ test/build/.gitignore | 2 + test/build/build.py | 105 +++++++++++ test/build/static_checkers.sh | 28 +++ test/meta/.gitignore | 1 + test/meta/lint.sh | 28 +++ test/nstool.c | 30 ++- test/pasta/.gitignore | 1 + test/pasta/pasta.py | 138 ++++++++++++++ test/run_avocado | 51 ++++++ test/tasst/.gitignore | 1 + test/tasst/__init__.py | 11 ++ test/tasst/__main__.py | 22 +++ test/tasst/address.py | 79 ++++++++ test/tasst/dhcp.py | 132 ++++++++++++++ test/tasst/dhcpv6.py | 89 +++++++++ test/tasst/ndp.py | 116 ++++++++++++ test/tasst/nstool.py | 186 +++++++++++++++++++ test/tasst/pasta.py | 52 ++++++ test/tasst/scenario/__init__.py | 12 ++ test/tasst/scenario/simple.py | 109 +++++++++++ test/tasst/selftest/__init__.py | 16 ++ test/tasst/selftest/static_ifup.py | 60 ++++++ test/tasst/selftest/veth.py | 106 +++++++++++ test/tasst/snh.py | 283 +++++++++++++++++++++++++++++ test/tasst/transfer.py | 194 ++++++++++++++++++++ 28 files changed, 1928 insertions(+), 5 deletions(-) create mode 100644 test/avocado/static_checkers.json create mode 100644 test/build/.gitignore create mode 100644 test/build/build.py create mode 100644 test/build/static_checkers.sh create mode 100644 test/meta/.gitignore create mode 100644 test/meta/lint.sh create mode 100644 test/pasta/.gitignore create mode 100644 test/pasta/pasta.py create mode 100755 test/run_avocado create mode 100644 test/tasst/.gitignore create mode 100644 test/tasst/__init__.py create mode 100644 test/tasst/__main__.py create mode 100644 test/tasst/address.py create mode 100644 test/tasst/dhcp.py create mode 100644 test/tasst/dhcpv6.py create mode 100644 test/tasst/ndp.py create mode 100644 test/tasst/nstool.py create mode 100644 test/tasst/pasta.py create mode 100644 test/tasst/scenario/__init__.py create mode 100644 test/tasst/scenario/simple.py create mode 100644 test/tasst/selftest/__init__.py create mode 100644 test/tasst/selftest/static_ifup.py create mode 100644 test/tasst/selftest/veth.py create mode 100644 test/tasst/snh.py create mode 100644 test/tasst/transfer.py -- 2.45.2

2 30

[PATCH v7] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 08 Aug '24

08 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v7: (yes, seriously) don't close STDERR_FILENO in the general case, start from STDERR_FILENO + 1 v6: (seriously?) fix STDERR_FILENO comparison in conf() v5: Reject any --fd matching standard streams v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..2f5d649 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt <= STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..7761bd3 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO + 1, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

3 2

[PATCH v2] conf: Stop parsing options at first non-option argument
by Stefano Brivio 08 Aug '24

08 Aug '24

Given that pasta supports specifying a command to be executed on the command line, even without the usual -- separator as long as there's no ambiguity, we shouldn't eat up options that are not meant for us. Paul reports, for instance, that with: pasta --config-net ip -6 route -6 is taken by pasta to mean --ipv6-only, and we execute 'ip route'. That's because getopt_long(), by default, shuffles the argument list to shift non-option arguments at the end. Avoid that by adding '-' at the beginning of 'optstring', and mark the position of the first non-option argument (getopt_long() will now return the character code 1 once we hit it), so that we can use that as command to run, or as PID for the target namespace. Reported-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v2: Instead of overriding 'name' in the getopt_long() loop, to force exiting the loop, adjust the exit condition conf.c | 23 +++++++++++++++-------- util.c | 2 +- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/conf.c b/conf.c index be5259e..1a30b47 100644 --- a/conf.c +++ b/conf.c @@ -1240,22 +1240,22 @@ void conf(struct ctx *c, int argc, char **argv) enum fwd_ports_mode fwd_default = FWD_NONE; bool v4_only = false, v6_only = false; struct fqdn *dnss = c->dns_search; + int name, ret, first_nonopt = -1; unsigned int ifi4 = 0, ifi6 = 0; const char *logfile = NULL; const char *optstring; size_t logsize = 0; char *runas = NULL; long fd_tap_opt; - int name, ret; uid_t uid; gid_t gid; if (c->mode == MODE_PASTA) { c->no_dhcp_dns = c->no_dhcp_dns_search = 1; fwd_default = FWD_AUTO; - optstring = "dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; + optstring = "-dqfel:hF:I:p:P:m:a:n:M:g:i:o:D:S:46t:u:T:U:"; } else { - optstring = "dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; + optstring = "-dqfel:hs:F:p:P:m:a:n:M:g:i:o:D:S:461t:u:"; } c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; @@ -1269,6 +1269,9 @@ void conf(struct ctx *c, int argc, char **argv) case -1: case 0: break; + case 1: + first_nonopt = optind - 1; + break; case 2: if (c->mode != MODE_PASTA) die("--userns is for pasta mode only"); @@ -1629,7 +1632,10 @@ void conf(struct ctx *c, int argc, char **argv) usage(argv[0], stderr, EXIT_FAILURE); break; } - } while (name != -1); + } while (name != -1 && name != 1); + + if (first_nonopt == -1) + first_nonopt = optind; if (c->mode == MODE_PASTA && !c->pasta_conf_ns) { if (copy_routes_opt) @@ -1689,9 +1695,9 @@ void conf(struct ctx *c, int argc, char **argv) } while (name != -1); if (c->mode == MODE_PASTA) - conf_pasta_ns(&netns_only, userns, netns, optind, argc, argv); - else if (optind != argc) - die("Extra non-option argument: %s", argv[optind]); + conf_pasta_ns(&netns_only, userns, netns, first_nonopt, argc, argv); + else if (first_nonopt != argc) + die("Extra non-option argument: %s", argv[first_nonopt]); conf_open_files(c); /* Before any possible setuid() / setgid() */ @@ -1705,7 +1711,8 @@ void conf(struct ctx *c, int argc, char **argv) pasta_open_ns(c, netns); } else { pasta_start_ns(c, uid, gid, - argc - optind, argv + optind); + argc - first_nonopt, + argv + first_nonopt); } } diff --git a/util.c b/util.c index 9c6be6a..13524e6 100644 --- a/util.c +++ b/util.c @@ -710,7 +710,7 @@ void close_open_files(int argc, char **argv) int name; do { - name = getopt_long(argc, argv, ":F", optfd, NULL); + name = getopt_long(argc, argv, "-:F", optfd, NULL); if (name == 'F') { errno = 0; -- 2.43.0

2 3

[PATCH v6] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 08 Aug '24

08 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v6: (seriously?) fix STDERR_FILENO comparison in conf() v5: Reject any --fd matching standard streams v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..2f5d649 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt <= STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..fe9f436 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

2 1

[PATCH v5] passt, util: Close any open file that the parent might have leaked
by Stefano Brivio 07 Aug '24

07 Aug '24

If a parent accidentally or due to implementation reasons leaks any open file, we don't want to have access to them, except for the file passed via --fd, if any. This is the case for Podman when Podman's parent leaks files into Podman: it's not practical for Podman to close unrelated files before starting pasta, as reported by Paul. Use close_range(2) to close all open files except for standard streams and the one from --fd. Given that parts of conf() depend on other files to be already opened, such as the epoll file descriptor, we can't easily defer this to a more convenient point, where --fd was already parsed. Introduce a minimal, duplicate version of --fd parsing to keep this simple. As we need to check that the passed --fd option doesn't exceed INT_MAX, because we'll parse it with strtol() but file descriptor indices are signed ints (regardless of the arguments close_range() take), extend the existing check in the actual --fd parsing in conf(), also rejecting file descriptors numbers that match standard streams, while at it. Suggested-by: Paul Holzinger <pholzing(a)redhat.com> Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- v5: Reject any --fd matching standard streams v4: c->fd_tap, as used in conf(), is an int: don't assign to it directly from strtol(), or we won't catch overflows v3: Handle --fd 3 case, and don't overflow if the --fd number exceeds UINT_MAX: add an explicit check to ensure it's less than INT_MAX v2: Move call to close_open_files() to isolate_initial() conf.c | 8 ++++++-- isolation.c | 12 +++++++++--- isolation.h | 2 +- passt.c | 2 +- util.c | 41 +++++++++++++++++++++++++++++++++++++++++ util.h | 1 + 6 files changed, 59 insertions(+), 7 deletions(-) diff --git a/conf.c b/conf.c index 14d8ece..b4ea7a5 100644 --- a/conf.c +++ b/conf.c @@ -1245,6 +1245,7 @@ void conf(struct ctx *c, int argc, char **argv) const char *optstring; size_t logsize = 0; char *runas = NULL; + long fd_tap_opt; int name, ret; uid_t uid; gid_t gid; @@ -1260,6 +1261,7 @@ void conf(struct ctx *c, int argc, char **argv) c->tcp.fwd_in.mode = c->tcp.fwd_out.mode = FWD_UNSET; c->udp.fwd_in.mode = c->udp.fwd_out.mode = FWD_UNSET; + optind = 1; do { name = getopt_long(argc, argv, optstring, options, NULL); @@ -1424,11 +1426,13 @@ void conf(struct ctx *c, int argc, char **argv) break; case 'F': errno = 0; - c->fd_tap = strtol(optarg, NULL, 0); + fd_tap_opt = strtol(optarg, NULL, 0); - if (c->fd_tap < 0 || errno) + if (errno || + fd_tap_opt < STDERR_FILENO || fd_tap_opt > INT_MAX) die("Invalid --fd: %s", optarg); + c->fd_tap = fd_tap_opt; c->one_off = true; *c->sock_path = 0; break; diff --git a/isolation.c b/isolation.c index 4956d7e..45fba1e 100644 --- a/isolation.c +++ b/isolation.c @@ -29,7 +29,8 @@ * * Executed immediately after startup, drops capabilities we don't * need at any point during execution (or which we gain back when we - * need by joining other namespaces). + * need by joining other namespaces), and closes any leaked file we + * might have inherited from the parent process. * * 2. isolate_user() * ================= @@ -166,14 +167,17 @@ static void clamp_caps(void) } /** - * isolate_initial() - Early, config independent self isolation + * isolate_initial() - Early, mostly config independent self isolation + * @argc: Argument count + * @argv: Command line options: only --fd (if present) is relevant here * * Should: * - drop unneeded capabilities + * - close all open files except for standard streams and the one from --fd * Musn't: * - remove filesytem access (we need to access files during setup) */ -void isolate_initial(void) +void isolate_initial(int argc, char **argv) { uint64_t keep; @@ -207,6 +211,8 @@ void isolate_initial(void) keep |= BIT(CAP_SETFCAP) | BIT(CAP_SYS_PTRACE); drop_caps_ep_except(keep); + + close_open_files(argc, argv); } /** diff --git a/isolation.h b/isolation.h index 846b2af..80bb68d 100644 --- a/isolation.h +++ b/isolation.h @@ -7,7 +7,7 @@ #ifndef ISOLATION_H #define ISOLATION_H -void isolate_initial(void); +void isolate_initial(int argc, char **argv); void isolate_user(uid_t uid, gid_t gid, bool use_userns, const char *userns, enum passt_modes mode); int isolate_prefork(const struct ctx *c); diff --git a/passt.c b/passt.c index ea5bece..4b3c306 100644 --- a/passt.c +++ b/passt.c @@ -211,7 +211,7 @@ int main(int argc, char **argv) arch_avx2_exec(argv); - isolate_initial(); + isolate_initial(argc, argv); c.pasta_netns_fd = c.fd_tap = c.pidfile_fd = -1; diff --git a/util.c b/util.c index 07fb21c..fe9f436 100644 --- a/util.c +++ b/util.c @@ -26,6 +26,7 @@ #include <errno.h> #include <stdbool.h> #include <linux/errqueue.h> +#include <getopt.h> #include "util.h" #include "iov.h" @@ -694,3 +695,43 @@ const char *str_ee_origin(const struct sock_extended_err *ee) return "<invalid>"; } + +/** + * close_open_files() - Close leaked files, but not --fd, stdin, stdout, stderr + * @argc: Argument count + * @argv: Command line options, as we need to skip any file given via --fd + */ +void close_open_files(int argc, char **argv) +{ + const struct option optfd[] = { { "fd", required_argument, NULL, 'F' }, + { 0 }, + }; + long fd = -1; + int name, rc; + + do { + name = getopt_long(argc, argv, ":F", optfd, NULL); + + if (name == 'F') { + errno = 0; + fd = strtol(optarg, NULL, 0); + + if (errno || fd <= STDERR_FILENO || fd > INT_MAX) + die("Invalid --fd: %s", optarg); + } + } while (name != -1); + + if (fd == -1) { + rc = close_range(STDERR_FILENO, ~0U, CLOSE_RANGE_UNSHARE); + } else if (fd == STDERR_FILENO + 1) { /* Still a single range */ + rc = close_range(STDERR_FILENO + 2, ~0U, CLOSE_RANGE_UNSHARE); + } else { + rc = close_range(STDERR_FILENO + 1, fd - 1, + CLOSE_RANGE_UNSHARE); + if (!rc) + rc = close_range(fd + 1, ~0U, CLOSE_RANGE_UNSHARE); + } + + if (rc) + die_perror("Failed to close files leaked by parent"); +} diff --git a/util.h b/util.h index 83d2b53..cb4d181 100644 --- a/util.h +++ b/util.h @@ -183,6 +183,7 @@ int __daemon(int pidfile_fd, int devnull_fd); int fls(unsigned long x); int write_file(const char *path, const char *buf); int write_remainder(int fd, const struct iovec *iov, size_t iovcnt, size_t skip); +void close_open_files(int argc, char **argv); /** * af_name() - Return name of an address family -- 2.43.0

1 0