From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id 612AD5A027B for ; Thu, 21 Dec 2023 07:53:35 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1703141610; bh=KiK/F3u9bZymlAfSsHwS/pKUDyiDSirjehRZGqjaNWE=; h=From:To:Cc:Subject:Date:In-Reply-To:References:From; b=Qm4O9wZt8vTeUg8BiW9dcvbfeXWl03RaYT3VTDXjXSQMe4CjTt5m17EpLg6ZsvBKR vl0kkq8CTVTtAb2fdETg0lIYd2JHZS0/J2s1geYxOdg1N3N73PkC3HWjbTSusaTxA0 +H1ItJR4LIJNi5QjGhD5v5COf/RoTnmdDpvH1PzdwOBi6Lw7VZkABKBSNsVO8Ro6mm e7cCITFOZKhrHJkDMyypJCkl7GBqj27vAml9W7+g5FgIMu1CJKbIoEvkWT4Gbj2Ck1 WjPpXSBBFtCyzOzOaYXhHZ8keGdsuZISJEYtZXivYZIHpknGvz6by+TUmPN3/TapQB hq1roDxGZ6A1A== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Swh3G4pHMz4xS7; Thu, 21 Dec 2023 17:53:30 +1100 (AEDT) From: David Gibson To: Stefano Brivio , passt-dev@passt.top Subject: [PATCH v2 05/12] icmp: Don't attempt to match host IDs to guest IDs Date: Thu, 21 Dec 2023 17:53:20 +1100 Message-ID: <20231221065327.1307827-6-david@gibson.dropbear.id.au> X-Mailer: git-send-email 2.43.0 In-Reply-To: <20231221065327.1307827-1-david@gibson.dropbear.id.au> References: <20231221065327.1307827-1-david@gibson.dropbear.id.au> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Message-ID-Hash: TNZULHN3F6TF3UV7B2LSECE7TW7AG6VQ X-Message-ID-Hash: TNZULHN3F6TF3UV7B2LSECE7TW7AG6VQ X-MailFrom: dgibson@gandalf.ozlabs.org X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: David Gibson X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: When forwarding pings from tap, currently we create a ping socket with a socket address whose port is set to the ID of the ping received from the guest. This causes the socket to send pings with the same ID on the host. Although this seems look a good idea for maximum transparency, it's probably unwise. First, it's fallible - the bind() could fail, and we already have fallback logic which will overwrite the packets with the expected guest id if the id we get on replies doesn't already match. We might as well do that unconditionally. But more importantly, we don't know what else on the host might be using ping sockets, so we could end up with an ID that's the same as an existing socket. You'd expect that to fail the bind() with EADDRINUSE, which would be fine: we'd fall back to rewriting the reply ids. However it appears the kernel (v6.6.3 at least), does *not* fail the bind() and instead it's "last socket wins" in terms of who gets the replies. So we could accidentally intercept ping replies for something else on the host. So, instead of using bind() to set the id, just let the kernel pick one and expect to translate the replies back. Although theoretically this makes the passt/pasta link a bit less "transparent", essentially nothing cares about specific ping IDs, much like TCP source ports, which we also don't preserve. Signed-off-by: David Gibson --- icmp.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/icmp.c b/icmp.c index 1f41440..7a505b4 100644 --- a/icmp.c +++ b/icmp.c @@ -80,11 +80,12 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref) if (n < 0) return; - id = ntohs(ih->un.echo.id); seq = ntohs(ih->un.echo.sequence); - if (id != ref.icmp.id) - ih->un.echo.id = htons(ref.icmp.id); + /* Adjust the packet to have the ID the guest was using, rather than the + * host chosen value */ + id = ref.icmp.id; + ih->un.echo.id = htons(id); if (c->mode == MODE_PASTA) { if (icmp_id_map[V4][id].seq == seq) @@ -119,15 +120,12 @@ void icmpv6_sock_handler(const struct ctx *c, union epoll_ref ref) if (n < 0) return; - id = ntohs(ih->icmp6_identifier); seq = ntohs(ih->icmp6_sequence); - /* If bind() fails e.g. because of a broken SELinux policy, - * this might happen. Fix up the identifier to match the sent - * one. - */ - if (id != ref.icmp.id) - ih->icmp6_identifier = htons(ref.icmp.id); + /* Adjust the packet to have the ID the guest was using, rather than the + * host chosen value */ + id = ref.icmp.id; + ih->icmp6_identifier = htons(id); /* In PASTA mode, we'll get any reply we send, discard them. */ if (c->mode == MODE_PASTA) { @@ -183,7 +181,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af, if ((s = icmp_id_map[V4][id].sock) <= 0) { s = sock_l4(c, AF_INET, IPPROTO_ICMP, &c->ip4.addr_out, - c->ip4.ifname_out, id, iref.u32); + c->ip4.ifname_out, 0, iref.u32); if (s < 0) goto fail_sock; if (s > FD_REF_MAX) { @@ -226,7 +224,7 @@ int icmp_tap_handler(const struct ctx *c, uint8_t pif, int af, if ((s = icmp_id_map[V6][id].sock) <= 0) { s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6, &c->ip6.addr_out, - c->ip6.ifname_out, id, iref.u32); + c->ip6.ifname_out, 0, iref.u32); if (s < 0) goto fail_sock; if (s > FD_REF_MAX) { -- 2.43.0