public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: David Gibson <david@gibson.dropbear.id.au>
To: Stefano Brivio <sbrivio@redhat.com>, passt-dev@passt.top
Cc: Matt Hamilton <matt@thmail.io>,
	David Gibson <david@gibson.dropbear.id.au>
Subject: [PATCH] flow: Don't crash if guest attempts to connect to port 0
Date: Wed, 14 Aug 2024 20:03:33 +1000	[thread overview]
Message-ID: <20240814100333.706977-1-david@gibson.dropbear.id.au> (raw)

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@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);
 }
 
 /**
-- 
@@ -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


             reply	other threads:[~2024-08-14 10:03 UTC|newest]

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-08-14 10:03 David Gibson [this message]
2024-08-14 11:03 ` [PATCH] flow: Don't crash if guest attempts to connect to port 0 Stefano Brivio

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240814100333.706977-1-david@gibson.dropbear.id.au \
    --to=david@gibson.dropbear.id.au \
    --cc=matt@thmail.io \
    --cc=passt-dev@passt.top \
    --cc=sbrivio@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).