Re: [PATCH net] netfilter: nf_conntrack_reasm: avoid truncating header offset

[netdev-bot+sashiko@kernel.org]

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

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

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] The patch widens prev_nhoff to int, but header offsets in this
  parse-and-reassemble path are still truncated to 16 bits with no range…

--

netfilter: nf_conntrack_reasm: avoid truncating header offset

This patch changes prev_nhoff in find_prev_fhdr() from u8 to int, so a
Next Header offset of 256 or more is no longer truncated. Before the
change, nf_ct_frag6_reasm() used the truncated value from
frag_queue.nhoffset and overwrote the IPv6 header instead of the
preceding extension header's Next Header field.

> diff --git a/net/ipv6/netfilter/nf_conntrack_reasm.c b/net/ipv6/netfilter/nf_conntrack_reasm.c
> index 599c49bf0a0af..be72c4346f8b7 100644
> --- a/net/ipv6/netfilter/nf_conntrack_reasm.c
> +++ b/net/ipv6/netfilter/nf_conntrack_reasm.c
> @@ -398,7 +398,7 @@ find_prev_fhdr(struct sk_buff *skb, u8 *prevhdrp, int *prevhoff, int *fhoff)
>  {
>  	u8 nexthdr = ipv6_hdr(skb)->nexthdr;
>  	const int netoff = skb_network_offset(skb);
> -	u8 prev_nhoff = netoff + offsetof(struct ipv6hdr, nexthdr);
> +	int prev_nhoff = netoff + offsetof(struct ipv6hdr, nexthdr);

[Severity: High]
This fixes the 8-bit truncation. Are the same offsets still truncated to
16 bits later in this path?

find_prev_fhdr() bounds prev_nhoff and fhoff only by skb->len. Both values
then go into 16-bit storage with no range check:

include/net/ipv6_frag.h:struct frag_queue {
    ...
	__u16			nhoffset;
    ...
}

net/ipv6/netfilter/nf_conntrack_reasm.c:nf_ct_frag6_queue() {
    ...
	if (offset == 0) {
		fq->nhoffset = nhoff;
    ...
}

net/ipv6/netfilter/nf_conntrack_reasm.c:nf_ct_frag6_gather() {
    ...
	skb_set_transport_header(skb, fhoff);
	hdr = ipv6_hdr(skb);
	fhdr = (struct frag_hdr *)skb_transport_header(skb);
    ...
}

skb_set_transport_header() adds fhoff to the u16 skb->transport_header.
If headroom + fhoff is above 65535, the value wraps and fhdr no longer
points at the real Fragment Header.

At PRE_ROUTING this seems harmless. ip6_rcv_core() trims skb->len to
payload_len + 40, and nf_ct_frag6_queue() then appears to drop the wrapped
cases because end exceeds IPV6_MAXPLEN.

The LOCAL_OUT raw path looks different. rawv6_send_hdrinc() limits the
length only by dev->mtu, which is 65536 on lo by default. It does not
rewrite payload_len and calls NF_HOOK(NFPROTO_IPV6, NF_INET_LOCAL_OUT, ...)
directly.

lo has 16 bytes of headroom. So a 65536-byte IPV6_HDRINCL packet can put
fhoff at 65528 behind a chain of Destination Options headers.
transport_header then wraps to 8, and fhdr points into the headroom at
skb->head + 8.

The real Fragment Header can carry a nonzero offset, so
ipv6frag_thdr_truncated() still passes. Suppose the stale headroom bytes
decode as offset 0 with MF clear, for example zeroed memory under
init_on_alloc. Would the packet then be reassembled as a single fragment?

If so, could it reach this sequence?

nf_ct_frag6_gather()
  nf_ct_frag6_queue()
  nf_ct_frag6_reasm()
    skb_network_header(skb)[fq->nhoffset] = skb_transport_header(skb)[0];
    ...
    skb->network_header += sizeof(struct frag_hdr);
    ...
    inet_frag_reasm_finish()
      skb_push(head, head->data - skb_network_header(head));

The first line copies a byte of uninitialized headroom into the packet
data.

The network_header increment moves it to head + 24, past skb->data at
head + 16. The skb_push() length is then -8, which becomes 0xFFFFFFF8 as
an unsigned int. Would that trigger skb_under_panic()?

This path needs CAP_NET_RAW, which an unprivileged user may get inside a
user and net namespace. It also needs conntrack defrag active in that
netns, for example through an nft ct rule.

Also, if lo's MTU is raised above 65536, can prev_nhoff reach 65536 or
more? The store into the __u16 nhoffset would then wrap to 0, 8 and so
on, which is the same misdirected Next Header write this commit fixes for
the u8 case.

Should find_prev_fhdr() or nf_ct_frag6_gather() reject nhoff and fhoff
values that the 16-bit consumers cannot hold? For fhoff, that means
including the headroom and sizeof(struct frag_hdr).

>  	int start = netoff + sizeof(struct ipv6hdr);
>  	int len = skb->len - start;
>  	u8 prevhdr = NEXTHDR_IPV6;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260929090027.200041-1-tjdqudcks0424%40naver.com