Re: [PATCH net-next v18 15/15] quic: add packet parser base

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] An ICMP Packet Too Big has no effect when PLPMTUD is disabled.
- [Medium] In quic_packet_listen_process() (net/quic/packet.c:749-766),
  the new request is published before its first packet is queued, and…
- [Medium] quic_packet_rcv_err() accepts an ICMP PTB/Frag-Needed based
  only on the quoted IP addresses and UDP ports.
- [Low] The comment on `struct quic_skb_cb::backlog`
  (net/quic/common.h:71, `u8 backlog:1; /* Enqueued into backlog list…
- [Low] In quic_listen_sock_lookup() (net/quic/socket.c:248-249), the
  comment says "Address specificity always takes precedence over ALPN…

--

quic: add packet parser base

This patch adds the QUIC receive path packet parser. It looks up sockets
by connection ID, address and ALPN, dispatches packets by socket state,
and handles client Initial packets on listen sockets with request
sockets. It also adds ICMP Packet Too Big handling to update the path
MTU, and a per-socket workqueue for packets that need a sleepable
context.

[ ... ]

[Severity: High]
Does this update_pmtu() call ever reach the peer's route? With a NULL
skb, the routing layer builds the flow key from the socket's inet
fields.

For IPv4, ip_rt_update_pmtu()->ip_rt_build_flow_key()->build_sk_flow_key():

net/ipv4/route.c:build_sk_flow_key() {
    ...
	__be32 daddr = inet->inet_daddr;
    ...
}

For IPv6, __ip6_rt_update_pmtu() with iph == NULL:

net/ipv6/route.c:__ip6_rt_update_pmtu() {
    ...
		daddr = &sk->sk_v6_daddr;
		saddr = &inet6_sk(sk)->saddr;
    ...
}

As far as I can tell, nothing in net/quic/ sets inet_daddr, inet_saddr,
sk_v6_daddr or the IPv6 saddr. Instead, quic_v4_flow_route() and
quic_v6_flow_route() fill a private flowi from the path addresses:

	fl4->daddr = da->v4.sin_addr.s_addr;

That means the exception gets created for 0.0.0.0 (IPv4, through
fib_lookup() and update_or_create_fnhe(), and only if a default route
matches) or for :: (IPv6). It is never created for the real peer.

After __sk_dst_reset(), the next quic_packet_route() looks up the peer
route again, gets the old dst_mtu(), and calls quic_packet_mss_update()
with the same value as before.

plpmtud_interval doesn't appear to be set anywhere in net/quic/ yet, so
every socket takes this branch. packet->ipfragok is also 0 here, so DF
packets keep getting dropped at the smaller-MTU hop.

Would this leave the connection in a PMTU black hole? The commit message
says:

  "When PLPMTUD is disabled, update the MSS using the MTU accepted by
  the routing layer"

Other callers that pass a NULL skb make sure the socket daddr is valid
first. For example, SCTP's sctp_transport_update_pmtu() sets it
temporarily with pf->to_sk_daddr().

The ICMP skb in quic_packet_rcv_err() also carries the quoted inner IP
header. ipv4_sk_update_pmtu() and ip6_sk_update_pmtu() use that header
to build a correct key, but it isn't passed down here.

There is also a side effect. The IPv4 case can create a stray fnhe for
0.0.0.0 on the default nexthop, which marks its cached routes stale. The
IPv6 case can insert an RTF_CACHE exception for ::.

[Severity: Medium]
Should the quoted QUIC DCID be checked before this ICMP is trusted? The
lookup passes dcid == NULL, so the socket is matched only on the quoted
IP addresses and UDP ports. quic_get_mtu_info() and quic_sk_accept_pmtu()
only look at the ICMP type and the pmtudisc setting.

The quoted QUIC header follows the UDP header in the ICMP payload. RFC
9000 section 14.2.1 recommends validating it when possible.

quic_packet_rcv_err_pmtu() also does this:

	info = clamp(paths->mtu_info, QUIC_PATH_MIN_PMTU, QUIC_PATH_MAX_PMTU);

That raises claims below 1200 bytes up to 1200 instead of ignoring them.
The same RFC section says such messages must be ignored.

With PLPMTUD enabled, a spoofed claim such as 68 becomes roughly
1136-1156 once hlen and taglen are subtracted. That is above
QUIC_MIN_PLPMTU, so quic_path_pl_toobig() moves a path in the search or
complete state back to the base 1200-byte PMTU, and
quic_packet_mss_update() shrinks the MSS. Without the clamp, 68 - 44 = 24
would have been ignored.

Without PLPMTUD, the clamped 1200 is passed to update_pmtu(). That is a
no-op for the peer today only because of the flow key issue above. Once
that is fixed, the same spoofed ICMP would install a 1200-byte exception
toward the peer.

Could an off-path attacker who guesses the 4-tuple (the server port plus
a 16-bit client port) use this to keep forcing a connection down to
1200-byte datagrams?

[ ... ]

[Severity: Low]
This isn't a bug, but the comment on this field in net/quic/common.h no
longer matches how the field is used:

	u8 backlog:1;    /* Enqueued into backlog list */

Here the bit is set when the skb goes onto deferred_list for the
workqueue, and the check above reads it as "already in a sleepable
context". quic_accept_sock_exists() clears it right before a possible
sk_add_backlog(). quic_crypto_decrypt() reads it as:

	if (!cb->backlog) /* Key update requires process context. */

Could the comment say that the bit means the skb is being processed from
a process-context queue where sleeping is allowed?

+	skb_set_owner_r(skb, sk);
+	__skb_queue_tail(&quic_packet(sk)->deferred_list, skb);

[ ... ]

[Severity: Medium]
What happens to the new request if queueing its first packet fails? By
this point quic_request_sock_create() has already published it:

net/quic/socket.c:quic_request_sock_create() {
    ...
	list_add_tail(&req->list, quic_reqs(sk));
	sk_acceptq_added(sk);
	sk->sk_data_ready(sk);
    ...
}

quic_request_sock_backlog_tail() can fail on that very first packet:

net/quic/socket.c:quic_request_sock_backlog_tail() {
    ...
	if (req->blen + len > limit || !__sk_rmem_schedule(sk, len, false)) {
		QUIC_INC_STATS(sock_net(sk), QUIC_MIB_PKT_RCVDROP);
		kfree_skb(skb);
		return -ENOBUFS;
	}
    ...
}

limit is max(sk_rcvbuf / sk_max_ack_backlog, QUIC_MIN_UDP_PAYLOAD * 4).
Any skb with a truesize above 4800 bytes fails that check, for example
on a driver that uses a whole page per packet on a 64K page system. The
rmem schedule can also fail under memory pressure.

On this error path, req is not removed with list_del() or freed,
sk_acceptq_removed() is not called, and accept() has already been woken.

Does this leave a request with an empty backlog_list (no Client Initial)
holding an accept queue slot?

The only way it recovers is if a retransmitted Initial matches it in
quic_request_sock_lookup() and gets queued successfully. A spoofing peer
never retransmits. In the truesize case, every retransmit fails the same
check, so the connection can never be established.

[ ... ]

[ ... ]

[ ... ]

[ ... ]

[Severity: Low]
This isn't a bug, but the comment suggests the client's ALPN preference
order is used as a secondary criterion, and the loop doesn't seem to do
that. alpn_match is a plain boolean, and which client ALPN matched is
not recorded.

Among specific-address listeners, the first match in the hash chain wins
through goto out. Among same-family wildcard-address listeners, each
later match replaces the candidate, so the last one wins.

For example, if the client offers "smb,h3" and listeners A (h3) and B
(smb) are bound to the same address, the choice depends on bind/listen
order and not on the client's preference.

Could the comment be updated, or could the preference ordering be
restored? It seems to have been dropped when the two lookups were merged
in v15.

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