Re: [PATCH net-next v15 03/15] quic: provide common utilities and data structures

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] Weak hash seeding in the new QUIC hash helpers:
  `quic_sock_hash()` (net/quic/common.c:40-53) reduces IPv6 addresses to…

--

quic: provide common utilities and data structures

This patch adds net/quic/common.c and net/quic/common.h with the QUIC
packet header types, connection ID and address helpers, the socket and
connection ID hash tables, and the varint/transport-parameter encoding
helpers. quic_init() now initializes and tears down the hash tables.

One question below about the hash helpers.

[ ... ]

+u32 quic_sock_hash(struct net *net, union quic_addr *s, union quic_addr *d)
+{
+	u32 ports = ((__force u32)s->v4.sin_port) << 16 |
+		    (__force u32)d->v4.sin_port;
+	u32 saddr = (s->sa.sa_family == AF_INET6) ?
+		    jhash(&s->v6.sin6_addr, 16, 0) :
+		    (__force u32)s->v4.sin_addr.s_addr;
+	u32 daddr = (d->sa.sa_family == AF_INET6) ?
+		    jhash(&d->v6.sin6_addr, 16, 0) :
+		    (__force u32)d->v4.sin_addr.s_addr;
+	u32 hash = jhash_3words(saddr, daddr, ports, net_hash_mix(net));
+
+	return hash & (quic_sock_hash_size() - 1);
+}

[Severity: Medium]
Should the inner IPv6 fold in quic_sock_hash() be seeded?

The per-netns secret is only mixed in the outer jhash_3words() call, while
the 128-bit address is first reduced to 32 bits with jhash(..., 0). Since
jhash_3words(a, b, c, initval) is a pure function of its inputs, two peer
addresses whose unseeded 32-bit fold is equal produce identical (saddr,
daddr, ports) triples and therefore the same bucket for every value of
net_hash_mix(net).

Does that let a peer controlling an IPv6 prefix pick colliding source
addresses offline, without knowing the secret, and collapse many
connections into one chain of quic_hashinfo.chash (capped at 16K buckets)
that the per-packet lookup then walks?

Compare with sctp_hashfn() in net/sctp/input.c, which the commit message
cites as the model and which passes the seed into the inner fold:

	if (paddr->sa.sa_family == AF_INET6)
		addr = jhash(&paddr->v6.sin6_addr, 16, seed);
	else
		addr = (__force __u32)paddr->v4.sin_addr.s_addr;

	return  jhash_3words(addr, ((__force __u32)paddr->v4.sin_port) << 16 |
			     (__force __u32)lport, net_hash_mix(net), seed);

Would jhash(&s->v6.sin6_addr, 16, net_hash_mix(net)) here be equivalent in
cost?

[ ... ]

+struct quic_shash_head *quic_source_conn_id_head(struct net *net, u8 *scid,
+						 u32 len)
+{
+	u32 hash = jhash_1word(jhash(scid, len, 0), net_hash_mix(net));
+	struct quic_shash_table *ht = &quic_hashinfo.shash;
+
+	return &ht->hash[hash & (ht->size - 1)];
+}

[Severity: Medium]
Same pattern here: the connection ID bytes are folded with jhash(scid, len,
0) before the secret is applied, so any two connection IDs colliding in
that unseeded fold share a bucket for any secret. Would passing
net_hash_mix(net) as the initval of the inner jhash() be preferable?

Note the connection IDs inserted in this series come from
quic_conn_id_generate() using get_random_bytes(), so the practical exposure
is smaller than for the connection socket table.

+u32 quic_addr_hash(struct net *net, union quic_addr *a)
+{
+	u32 addr = (a->sa.sa_family == AF_INET6) ?
+		   jhash(&a->v6.sin6_addr, 16, 0) :
+		   (__force u32)a->v4.sin_addr.s_addr;
+
+	return jhash_2words(addr, (__force u32)a->v4.sin_port,
+			    net_hash_mix(net));
+}

[Severity: Medium]
And the same unseeded sin6_addr fold appears in quic_addr_hash(), so
address-keyed hashing inherits the same property. Should all three helpers
be updated together?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1789393775.git.lucien.xin%40gmail.com