From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 31CC15A004E for ; Mon, 12 Aug 2024 23:51:35 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1723499491; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+/kQDbJ0Zl3iz2uVtSKYiC0JWAuSJtPF+/3CPsRZpfg=; b=d7ykTz518GpTzUuaanwDLzEgk/CA3L1aSRyZd6R43hkOUK6Lo9QCzdPCEXAirVX2K29Imi fMJvnUXYHWBvubjXqh7b3kuAsWWViz7JklTe5GB4b+P21sPr6w8S6qO62MXI6wM3PD6QG7 XFOdNZ8E0t6MAZSbm8PGPVekkadsgGM= Received: from mail-pl1-f198.google.com (mail-pl1-f198.google.com [209.85.214.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-259-_5bL8YlgNcm2OOpD6LhfYQ-1; Mon, 12 Aug 2024 17:51:26 -0400 X-MC-Unique: _5bL8YlgNcm2OOpD6LhfYQ-1 Received: by mail-pl1-f198.google.com with SMTP id d9443c01a7336-1fc5acc1b96so49419135ad.3 for ; Mon, 12 Aug 2024 14:51:25 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723499484; x=1724104284; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=+/kQDbJ0Zl3iz2uVtSKYiC0JWAuSJtPF+/3CPsRZpfg=; b=EZnAHQW7xUHK6mNUMByuZNSSCQXSy7C0emobWrVdYx/xmrLI5WJccjPw/Fet9YYRgW 8QLcUI1gMZHSBYk6mEqU9aayTX9ZIdD2o+5Anu/+G9vnJXc+7cgG21QTa5CirkgiCV2R FCFhSg6l1r4fabY6hLZhPDNVFJkN1CReTHoNI7tvbB2ZDDAl8iVtWk3ZFboxQyjaNS9h Ary+lFqHgX6zALVSWvHwOqUZet6IraN8mA/AZHNUBQyMcZPU7zaw/RvMtBLCGIYhMdFV JZvRwyQSWnAvX7M2q6fAO0mRMVye3hgHymexn+K5lR7CAhwKctEk1eff4i0UJa1u6iQQ faxw== X-Gm-Message-State: AOJu0YzF8J+RAnU1wctDsI5Gi5wXYFbIwLcbqePgmBB3zmXil+inkcPs sGePrs67rvZWbxG1LLb3OenhpAH346Q9xBL6rP7uQRgqcNKkEeVht0BVJcGADQ5dgwTq0PbRHWK F52cLxegJcp69IxvVruWNNsjnXZ1YJAs+s9Udgj93Z8xJTWUzG8bCKhfkVw== X-Received: by 2002:a17:903:18d:b0:1fd:791d:1437 with SMTP id d9443c01a7336-201ca121892mr18541305ad.6.1723499484081; Mon, 12 Aug 2024 14:51:24 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHSRSUaB/M6gtvm0B3u5g2/1oQWEVoarx+/rNvFNGn8w5ZLlTzj7VZEOpDmiEPQalzL49n/9w== X-Received: by 2002:a17:903:18d:b0:1fd:791d:1437 with SMTP id d9443c01a7336-201ca121892mr18541105ad.6.1723499483436; Mon, 12 Aug 2024 14:51:23 -0700 (PDT) Received: from maya.myfinge.rs (ifcgrfdd.trafficplex.cloud. [176.103.220.4]) by smtp.gmail.com with ESMTPSA id d9443c01a7336-201cd132959sm1474375ad.25.2024.08.12.14.51.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 12 Aug 2024 14:51:22 -0700 (PDT) Date: Mon, 12 Aug 2024 23:51:17 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 3/3] fwd: Rework inconsistencies in translation of inbound flows Message-ID: <20240812235117.6a48a817@elisabeth> In-Reply-To: <20240812095355.1721876-4-david@gibson.dropbear.id.au> References: <20240812095355.1721876-1-david@gibson.dropbear.id.au> <20240812095355.1721876-4-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: LYG5UJ57KKFMTRW3KVWKJZFUZGPPBTF6 X-Message-ID-Hash: LYG5UJ57KKFMTRW3KVWKJZFUZGPPBTF6 X-MailFrom: sbrivio@redhat.com 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: passt-dev@passt.top 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: I applied up to 2/3, I just have one doubt here, and a nit: On Mon, 12 Aug 2024 19:53:55 +1000 David Gibson wrote: > We usually avoid NAT, but in a few cases we need to apply address > translations. The current logic for this on inbound flows has some > inconsistencies: > > * For IPv4 (but not IPv6) we translated unspecified source addresses ...I know we already talked about this, but 0.0.0.0/8 is not just unspecified, it also means "this host on this network" (RFC 6890, 2.2.2), and that's the reason for this apparent inconsistency (:: doesn't). By the way, somebody was reminded of this just recently: https://www.oligo.security/blog/0-0-0-0-day-exploiting-localhost-apis-from-the-browser https://lwn.net/Articles/984838/ Now, if I recall correctly, the kernel translates that anyway to a loopback address before we see it. But still it's a legitimate address to use, so I would keep handling it as a loopback address. > While an unspecified source probably doesn't make sense, it makes no less > sense in the guest context than it does for the host, it's possible there > could be protocols / flow types where this works. At the moment, this > should never happen since the protocols will drop the flow before getting > here. So, don't alter unspecified addresses for either IP version. > > * For IPv6 (but not IPv4) we translated the guest's assigned address > > We always translated the guest's observed address, but for the case where > the assigned address different we should be consistent. Translate the > guest assigned address for IPv4 as well. Hm, right, this looks like an oversight in e58828f34067 ("tcp: Fixes for closing states, spliced connections, out-of-order packets, etc."). > The overall point is that we need to translate here if and only if the host > address is one that's not accessible to the guest. Create some helper > functions to determine this, which will have additional uses later. > > Signed-off-by: David Gibson > --- > fwd.c | 88 +++++++++++++++++++++++++++++++++++++++++++++++++++-------- > 1 file changed, 77 insertions(+), 11 deletions(-) > > diff --git a/fwd.c b/fwd.c > index dea36f6c..24700998 100644 > --- a/fwd.c > +++ b/fwd.c > @@ -170,6 +170,73 @@ static bool is_dns_flow(uint8_t proto, const struct flowside *ini) > ((ini->fport == 53) || (ini->fport == 853)); > } > > +/** > + * fwd_guest_accessible4() - Is IPv4 address guest accessible > + * @c: Execution context > + * @addr: Host visible IPv4 address > + * > + * Return: true if @addr on the host is accessible to the guest without > + * translation, false otherwise > + */ > +static bool fwd_guest_accessible4(const struct ctx *c, > + const struct in_addr *addr) > +{ > + if (IN4_IS_ADDR_LOOPBACK(addr)) > + return false; > + > + if (IN4_ARE_ADDR_EQUAL(addr, &c->ip4.addr)) > + return false; > + > + if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.addr_seen) && > + IN4_ARE_ADDR_EQUAL(addr, &c->ip4.addr_seen)) > + return false; > + > + return true; > +} > + > +/** > + * fwd_guest_accessible6() - Is IPv6 address guest accessible > + * @c: Execution context > + * @addr: Host visible IPv6 address > + * > + * Return: true if @addr on the host is accessible to the guest without > + * translation, false otherwise > + */ > +static bool fwd_guest_accessible6(const struct ctx *c, > + const struct in6_addr *addr) > +{ > + if (IN6_IS_ADDR_LOOPBACK(addr)) > + return false; > + > + if (IN6_ARE_ADDR_EQUAL(addr, &c->ip6.addr)) > + return false; > + > + if (!IN6_IS_ADDR_UNSPECIFIED(&c->ip6.addr_seen) && > + IN6_ARE_ADDR_EQUAL(addr, &c->ip6.addr_seen)) > + return false; > + > + return true; > +} > + > +/** > + * fwd_guest_accessible() - Is IPv[46] address guest accessible > + * @c: Execution context > + * @addr: Host visible IPv[46] address > + * > + * Return: true if @addr on the host is accessible to the guest without > + * translation, false otherwise > + */ > +static bool fwd_guest_accessible(const struct ctx *c, > + const union inany_addr *addr) > +{ > + const struct in_addr *a4 = inany_v4(addr); > + > + if (a4) > + return fwd_guest_accessible4(c, a4); > + > + return fwd_guest_accessible6(c, &addr->a6); > +} > + > /** > * fwd_nat_from_tap() - Determine to forward a flow from the tap interface > * @c: Execution context > @@ -307,21 +374,20 @@ uint8_t fwd_nat_from_host(const struct ctx *c, uint8_t proto, > return PIF_SPLICE; > } > > - tgt->faddr = ini->eaddr; > - tgt->fport = ini->eport; > - > - if (inany_is_loopback4(&tgt->faddr) || > - inany_is_unspecified4(&tgt->faddr) || > - inany_equals4(&tgt->faddr, &c->ip4.addr_seen)) { > - tgt->faddr = inany_from_v4(c->ip4.gw); > - } else if (inany_is_loopback6(&tgt->faddr) || > - inany_equals6(&tgt->faddr, &c->ip6.addr_seen) || > - inany_equals6(&tgt->faddr, &c->ip6.addr)) { > - if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) > + /* These host-visible addresses aren't visible to the guest, so need I guess you wrote this before moving the checks to their own function: it took me a bit to figure out that "These host-visible addresses" are not really mentioned here. Maybe s/These/Some/? > + * translation > + */ > + if (!fwd_guest_accessible(c, &ini->eaddr)) { > + if (inany_v4(&ini->eaddr)) > + tgt->faddr = inany_from_v4(c->ip4.gw); > + else if (IN6_IS_ADDR_LINKLOCAL(&c->ip6.gw)) > tgt->faddr.a6 = c->ip6.gw; > else > tgt->faddr.a6 = c->ip6.addr_ll; > + } else { > + tgt->faddr = ini->eaddr; > } > + tgt->fport = ini->eport; > > if (inany_v4(&tgt->faddr)) { > tgt->eaddr = inany_from_v4(c->ip4.addr_seen); -- Stefano