Re: [PATCH net-next v16 04/15] quic: provide family ops for address and protocol

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · 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 unprivileged sticky IPv6 routing header makes the route
  lookup and the transmitted packet disagree, so packets go to the real…
- [Medium] IP_TTL and IPV6_UNICAST_HOPS are accepted but ignored on
  transmit.
- [Low] The commit message leaves out user-visible changes and describes
  the code inaccurately.
- [Low] IP_OPTIONS is accepted and stored but never sent.

--

quic: provide family ops for address and protocol

This commit adds net/quic/family.{c,h} with IPv4/IPv6 helpers for route
lookup, UDP tunnel transmit, address parsing and comparison, and UDP port
config setup. It also connects quic_inet_getname() and the setsockopt()
and getsockopt() levels other than SOL_QUIC to the inet and inet6 code.

> Introduce QUIC address and protocol family operations to handle IPv4/IPv6
> specifics consistently, similar to SCTP. The new quic_family.{c,h} provide
> helpers for routing, skb transmit handling, address parsing and comparison
> and UDP socket config initializing etc.

[Severity: Low]
This isn't a bug, but the patch creates net/quic/family.{c,h}, not
quic_family.{c,h}.

The wording "family operations ... similar to SCTP" looks left over from
before v3. That version replaced the ops abstraction with if statements.

Could the message also mention the changes userspace can see?
quic_inet_getname() used to return -EOPNOTSUPP. It now returns real
addresses through inet_getname() or inet6_getname().

quic_setsockopt() and quic_getsockopt() now send every level other than
SOL_QUIC to ip_setsockopt()/ipv6_setsockopt() and
ip_getsockopt()/ipv6_getsockopt(). All SOL_IP and SOL_IPV6 options can
now be set on QUIC sockets. An unknown level also returns -ENOPROTOOPT
now, where it used to return -EOPNOTSUPP.

> diff --git a/net/quic/family.c b/net/quic/family.c
> new file mode 100644
> index 0000000000000..f5d859b337b10
> --- /dev/null
> +++ b/net/quic/family.c
> @@ -0,0 +1,449 @@

[ ... ]

> +static int quic_v6_flow_route(struct sock *sk, union quic_addr *da,
> +			      union quic_addr *sa, struct flowi *fl)
> +{
> +	struct ipv6_pinfo *np = inet6_sk(sk);

[ ... ]

> +	security_sk_classify_flow(sk, flowi6_to_flowi_common(fl6));
> +
> +	rcu_read_lock();
> +	final_p = fl6_update_dst(fl6, rcu_dereference(np->opt), &final);
> +	rcu_read_unlock();
> +
> +	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_p);
> +	if (IS_ERR(dst))
> +		return PTR_ERR(dst);

[Severity: High]
Can a sticky routing header make the route lookup disagree with the packet
that is actually sent?

quic_common_setsockopt() now passes SOL_IPV6 to ipv6_setsockopt(). That
lets an unprivileged user set IPV6_RTHDR, because ipv6_set_opt_hdr() only
requires CAP_NET_RAW for the other option headers:

net/ipv6/ipv6_sockglue.c:ipv6_set_opt_hdr() {
    ...
	/* hop-by-hop / destination options are privileged option */
	if (optname != IPV6_RTHDR && !sockopt_ns_capable(net->user_ns, CAP_NET_RAW))
		return -EPERM;
    ...
}

Suppose np->opt holds a type-4 SRH or a type-2 routing header. Then
fl6_update_dst() replaces fl6->daddr with a segment Y that the user picks.
ip6_dst_lookup_flow() looks up a route for Y and then puts the real
destination X back into fl6->daddr. ip6_dst_store() caches that dst.

quic_v6_lower_xmit() then calls udp_tunnel6_xmit_skb(). That helper builds
a plain IPv6 header addressed to X and never adds the np->opt extension
headers:

net/ipv6/ip6_udp_tunnel.c:udp_tunnel6_xmit_skb() {
    ...
	ip6h->nexthdr     = IPPROTO_UDP;
	ip6h->hop_limit   = ttl;
	ip6h->daddr	  = *daddr;
	ip6h->saddr	  = *saddr;

	ip6tunnel_xmit(sk, skb, dev, ip6cb_flags);
}

Doesn't this send packets to X using the device, gateway and MTU of the
route for Y? Neighbour discovery for X would then run on Y's link. Local
route policy for X, such as prohibit, blackhole, unreachable or policy
routes, would also be bypassed.

UDP and SCTP avoid this because they pass opt to
ip6_append_data()/ip6_xmit(), which put the routing header on the wire.

The transmit path is only connected in the next patchset. This code is
the same at the end of this series.

Should fl6_update_dst() be dropped here, since the tunnel transmit cannot
send extension headers? Alternatively, should QUIC sockets reject sticky
routing headers?

[ ... ]

> +static void quic_v4_lower_xmit(struct sock *sk, struct sk_buff *skb,
> +			       struct flowi *fl)
> +{

[ ... ]

> +	if (cb->ecn)
> +		tos = (tos & ~INET_ECN_MASK) | cb->ecn;
> +	ttl = (u8)ip4_dst_hoplimit(dst);

[Severity: Medium]
Are IP_TTL and IPV6_UNICAST_HOPS silently ignored here?

Through quic_common_setsockopt(), setsockopt(SOL_IP, IP_TTL) stores its
value in inet->uc_ttl. setsockopt(SOL_IPV6, IPV6_UNICAST_HOPS) stores its
value in np->hop_limit. Both calls succeed, and getsockopt(IP_TTL) returns
the stored value.

However, this function always takes the TTL from the route.
quic_v6_lower_xmit() does the same:

	ttl = (u8)ip6_dst_hoplimit(dst);

UDP and SCTP apply these options through ip_select_ttl() and
ip6_sk_dst_hoplimit(). These functions already apply IP_TOS and
IPV6_TCLASS through inet_sk(sk)->tos and inet6_sk(sk)->tclass.

Should the hop limit use the socket setting as well?

> +	udp_tunnel_xmit_skb((struct rtable *)dst, sk, skb, fl4->saddr,
> +			    fl4->daddr, tos, ttl, df, fl4->fl4_sport,
> +			    fl4->fl4_dport, false, false, 0);
> +}

[Severity: Low]
What happens to IP_OPTIONS on a PF_INET QUIC socket?

ip_setsockopt(IP_OPTIONS) parses the options and stores them in
inet->inet_opt, and getsockopt(IP_OPTIONS) returns them.

udp_tunnel_xmit_skb() ends up in iptunnel_xmit(). That function never
reads inet->inet_opt and builds a header with no options:

net/ipv4/ip_tunnel_core.c:iptunnel_xmit() {
    ...
	memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
    ...
	iph->ihl	=	sizeof(struct iphdr) >> 2;
    ...
}

Options such as Record Route or Timestamp are reported as set, but they
never appear in any packet. quic_encap_len() also reserves no room for
them.

Should QUIC sockets reject IP_OPTIONS?

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