mirrors/passt
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

icmp: Don't attempt to match host IDs to guest IDs

Browse source 
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 <david@gibson.dropbear.id.au>
Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
This commit is contained in:
David Gibson 2024-01-16 16:16:11 +11:00 committed by Stefano Brivio
parent 8534cdbfd1
commit 43713af50e
1 changed files with 10 additions and 12 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

22
icmp.c
View file

@ -80,11 +80,12 @@ void icmp_sock_handler(const struct ctx *c, union epoll_ref ref)
if (n < 0) if (n < 0)
return; return;
id = ntohs(ih->un.echo.id);
seq = ntohs(ih->un.echo.sequence); seq = ntohs(ih->un.echo.sequence);
if (id != ref.icmp.id) /* Adjust the packet to have the ID the guest was using, rather than the
ih->un.echo.id = htons(ref.icmp.id); * host chosen value */
id = ref.icmp.id;
ih->un.echo.id = htons(id);
if (c->mode == MODE_PASTA) { if (c->mode == MODE_PASTA) {
if (icmp_id_map[V4][id].seq == seq) 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) if (n < 0)
return; return;
id = ntohs(ih->icmp6_identifier);
seq = ntohs(ih->icmp6_sequence); seq = ntohs(ih->icmp6_sequence);
/* If bind() fails e.g. because of a broken SELinux policy, /* Adjust the packet to have the ID the guest was using, rather than the
* this might happen. Fix up the identifier to match the sent * host chosen value */
* one. id = ref.icmp.id;
*/ ih->icmp6_identifier = htons(id);
if (id != ref.icmp.id)
ih->icmp6_identifier = htons(ref.icmp.id);
/* In PASTA mode, we'll get any reply we send, discard them. */ /* In PASTA mode, we'll get any reply we send, discard them. */
if (c->mode == MODE_PASTA) { 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) { if ((s = icmp_id_map[V4][id].sock) <= 0) {
s = sock_l4(c, AF_INET, IPPROTO_ICMP, &c->ip4.addr_out, 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) if (s < 0)
goto fail_sock; goto fail_sock;
if (s > FD_REF_MAX) { 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) { if ((s = icmp_id_map[V6][id].sock) <= 0) {
s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6, s = sock_l4(c, AF_INET6, IPPROTO_ICMPV6,
&c->ip6.addr_out, &c->ip6.addr_out,
c->ip6.ifname_out, id, iref.u32); c->ip6.ifname_out, 0, iref.u32);
if (s < 0) if (s < 0)
goto fail_sock; goto fail_sock;
if (s > FD_REF_MAX) { if (s > FD_REF_MAX) {