Re: [PATCH net 02/15] ipv6: update *offset for non-first fragments in ipv6_find_hdr()

[netdev-bot+sashiko@kernel.org]

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

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

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).

- [Medium] This patch adds a new success exit to ipv6_find_hdr():
  `*offset = start + sizeof(struct frag_hdr)`, taken for a non-first…
- [Medium] The commit message says "Other target < 0 callers check the
  returned fragment offset or ignore *offset here." That is not true for…
- [Low] The earlier patch in this series, "ipvs: avoid out-of-bounds
  write in ip_vs_nat_icmp_v6", added a compensation block to…

--

ipv6: update *offset for non-first fragments in ipv6_find_hdr()

This commit makes ipv6_find_hdr() set *offset to the byte right after
the Fragment header when it takes the early return for a non-first
fragment with target < 0. Before this, that exit left *offset unchanged,
and IPVS used the initial offset as if it had been updated.

> Other target < 0 callers check the returned fragment offset
> or ignore *offset here.

[Severity: Medium]
Does this hold for every consumer of the offset?

ip6_packet_match() passes &acpar.thoff to ipv6_find_hdr(), and the result
reaches xt targets as par->thoff. nft_set_pktinfo_ipv6() sets pkt->thoff
the same way, and nft_compat forwards it to xt targets.

synproxy_tg6() in ip6t_SYNPROXY.c and nft_synproxy_do_eval() in
nft_synproxy.c do not check par->fragoff or pkt->fragoff:

net/ipv6/netfilter/ip6t_SYNPROXY.c:synproxy_tg6() {
    ...
	if (nf_ip6_checksum(skb, xt_hooknum(par), par->thoff, IPPROTO_TCP))
		return NF_DROP;

	th = skb_header_pointer(skb, par->thoff, sizeof(_th), &_th);
	...
	if (th->syn && !(th->ack || th->fin || th->rst)) {
		/* Initial SYN from client */
    ...
}

Before this patch, thoff stayed at 0 for non-first fragments, so these
targets parsed the IPv6 header as a TCP header. That was also wrong, but
a sender had little control over it.

With this change, thoff points at the fragment payload, and the sender
controls all of it. Could SYNPROXY now treat a non-first fragment that
carries a forged TCP SYN or ACK with a valid checksum as a real segment?
It would then answer with a SYN-cookie SYN-ACK, or send a SYN to the
server.

Earlier port-filtering matches (tcp, multiport, payload) skip these
fragments because they check fragoff. To reach SYNPROXY, the fragment has
to get past nf_defrag_ipv6 without being reassembled. One way is notrack
before defrag, which makes ipv6_defrag() return NF_ACCEPT for
IP_CT_UNTRACKED.

xt_ecn match_tcp() and xt_tcpmss tcpmss_mt() do check par->fragoff and
are not affected. The missing fragoff check in SYNPROXY is older than
this patch.

> diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
> index 764c694fb417a..12908d8ca370f 100644
> --- a/net/ipv6/exthdrs_core.c
> +++ b/net/ipv6/exthdrs_core.c

[ ... ]

> @@ -262,6 +265,7 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
>  				     hp->nexthdr == NEXTHDR_NONE)) {
>  					if (fragoff)
>  						*fragoff = _frag_off;
> +					*offset = start + sizeof(struct frag_hdr);

[Severity: Medium]
Can this set *offset past the end of the skb?

At this point only the first 4 bytes of the 8-byte Fragment header have
been bounds checked. hp reads 2 bytes at start, and fp reads the 2-byte
frag_off at start + 2. Nothing checks that the Identification field is
inside the packet.

Every other success exit runs this check before start advances:

		if (!found) {
			if (skb->len - start < hdrlen)
				return -EBADMSG;

Take a packet that ends 4 to 7 bytes into the Fragment header, for
example with payload_len = 4. This path now returns success with *offset
up to 4 bytes past skb->len. Before the patch, *offset kept its input
value here.

Remote input can reach this. ip6_rcv_core() only parses hop-by-hop
options. PRE_ROUTING hooks see such packets when nf_defrag_ipv6 is not
loaded, when a chain runs before defrag (priority < -400), or when the
packet is marked notrack before defrag.

nft_set_pktinfo_ipv6() only rejects thoff > U16_MAX. It therefore stores
a pkt->thoff larger than skb->len and sets NFT_PKTINFO_L4PROTO. The
subtraction in nf_trace_fill_pkt_info() then underflows:

		len = min_t(unsigned int, skb->len - nft_thoff(pkt),
			    NFT_TRACETYPE_TRANSPORT_HSIZE);

skb_copy_bits() then fails and nft_trace_notify() jumps to
nla_put_failure. That path hits DEBUG_NET_WARN_ON_ONCE(1) and drops the
trace event.

synproxy_tg6() and nft_synproxy_do_eval() also pass this thoff to
nf_ip6_checksum() and nf_checksum(). For CHECKSUM_NONE and
CHECKSUM_COMPLETE, these call skb_checksum(skb, 0, dataoff, 0) with a
length past skb->len, which hits the DEBUG_NET_WARN_ON_ONCE(1) in
skb_checksum(). They also pass skb->len - dataoff to csum_ipv6_magic(),
and that value underflows.

Would it be better to require the whole Fragment header before setting
*offset? For example:

	if (skb->len - start < sizeof(struct frag_hdr))
		return -EBADMSG;

[Severity: Low]
The earlier patch in this series, "ipvs: avoid out-of-bounds write in
ip_vs_nat_icmp_v6", added a compensation block to ip_vs_out_icmp_v6()
and ip_vs_in_icmp_v6(). Does this change make that block dead code?

net/netfilter/ipvs/ip_vs_core.c:ip_vs_out_icmp_v6() {
    ...
	/* ipv6_find_hdr() does not include the embedded header for
	 * non-first fragments, add it so that ESP can pass and the
	 * NAT writable checks cover the rewritten addresses
	 */
	if (ciph.len == ciph.off)
		ciph.len += sizeof(struct ipv6hdr);
    ...
}

ip_vs_fill_iph_skb_off() sets iphdr->len = offset, which is nonzero.
ipv6_find_hdr() therefore starts at *offset + sizeof(struct ipv6hdr).
Flags start at 0 and target is -1, so every success exit now sets
*offset to at least ciph.off + 40. As a result, ciph.len == ciph.off can
no longer be true.

The comment at both sites also describes behaviour that ipv6_find_hdr()
no longer has, and it contradicts the kernel-doc added by this patch.
Both copies are still there, unchanged, at the end of the series.

For ESP non-first fragments, ciph.len now also covers the extension
headers and the full 8-byte Fragment header, not just off + 40.

Should the block and its comment be removed or updated in both functions?

>  					return hp->nexthdr;
>  				}
>  				if (!found)

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113011447.2357709-2-pablo%40netfilter.org