[PATCH net v3 1/2] xfrm: fix IPv6 first-fragment protocol decode

[PATCH net v3 1/2] xfrm: fix IPv6 first-fragment protocol decode

[Jérémy Jean]

Commit 7a0207094f1b ("xfrm: policy: replace session decode with flow
dissector") replaced XFRM session decoding with the flow dissector.
While the old IPv6 decoder kept walking past an IPv6 Fragment
extension header, the flow dissector stops unless the caller sets
FLOW_DISSECTOR_F_PARSE_1ST_FRAG. For an atomic fragment followed by
extension headers, inbound policy lookup can therefore see the header
after Fragment instead of the actual upper-layer protocol and miss
protocol selectors.

Make the dissector continue through first IPv6 fragments. Keep the
port, ICMPv6 and GRE selector fields zero for all fragments, as the
old decoder did. This also keeps the result the same when a BPF flow
dissector fills ports for a first fragment.

Fixes: 7a0207094f1b ("xfrm: policy: replace session decode with flow dissector")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 net/xfrm/xfrm_policy.c | 35 ++++++++++++++++++++++-------------
 1 file changed, 22 insertions(+), 13 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 9ea60c7717ff8..bda5100c9fdbf 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3480,23 +3480,29 @@ decode_session6(const struct xfrm_flow_keys *flkeys, struct flowi *fl, bool reve
 	if (reverse) {
 		fl6->saddr = flkeys->addrs.ipv6.dst;
 		fl6->daddr = flkeys->addrs.ipv6.src;
-		fl6->fl6_sport = flkeys->ports.dst;
-		fl6->fl6_dport = flkeys->ports.src;
 	} else {
 		fl6->saddr = flkeys->addrs.ipv6.src;
 		fl6->daddr = flkeys->addrs.ipv6.dst;
-		fl6->fl6_sport = flkeys->ports.src;
-		fl6->fl6_dport = flkeys->ports.dst;
 	}
 
-	switch (flkeys->basic.ip_proto) {
-	case IPPROTO_GRE:
-		fl6->fl6_gre_key = flkeys->gre.keyid;
-		break;
-	case IPPROTO_ICMPV6:
-		fl6->fl6_icmp_type = flkeys->icmp.type;
-		fl6->fl6_icmp_code = flkeys->icmp.code;
-		break;
+	if (!(flkeys->control.flags & FLOW_DIS_IS_FRAGMENT)) {
+		if (reverse) {
+			fl6->fl6_sport = flkeys->ports.dst;
+			fl6->fl6_dport = flkeys->ports.src;
+		} else {
+			fl6->fl6_sport = flkeys->ports.src;
+			fl6->fl6_dport = flkeys->ports.dst;
+		}
+
+		switch (flkeys->basic.ip_proto) {
+		case IPPROTO_GRE:
+			fl6->fl6_gre_key = flkeys->gre.keyid;
+			break;
+		case IPPROTO_ICMPV6:
+			fl6->fl6_icmp_type = flkeys->icmp.type;
+			fl6->fl6_icmp_code = flkeys->icmp.code;
+			break;
+		}
 	}
 
 	fl6->flowi6_proto = flkeys->basic.ip_proto;
@@ -3507,10 +3513,13 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
 			  unsigned int family, int reverse)
 {
 	struct xfrm_flow_keys flkeys;
+	unsigned int flags = FLOW_DISSECTOR_F_STOP_AT_ENCAP;
 
 	memset(&flkeys, 0, sizeof(flkeys));
+	if (family == AF_INET6)
+		flags |= FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
 	__skb_flow_dissect(net, skb, &xfrm_session_dissector, &flkeys,
-			   NULL, 0, 0, 0, FLOW_DISSECTOR_F_STOP_AT_ENCAP);
+			   NULL, 0, 0, 0, flags);
 
 	switch (family) {
 	case AF_INET:
-- 
2.47.3

[PATCH net v3 2/2] xfrm: fix selector matching for IPv6 atomic fragments

[Jérémy Jean]

Commit 7e3a42a12c4b ("xfrm6: handling fragment") stopped decoding IPv6
selector fields after a Fragment header. That is correct for real
fragments because later fragments do not carry those fields, yet an
atomic fragment is different because it contains the complete
datagram, which allows XFRM to read its ports, ICMPv6 type and code,
and GRE key. Leaving those fields zero lets a plaintext atomic
fragment miss a policy that matches on one of them.

Leave ports, ICMPv6 type and code, and GRE key unset for real
fragments. For atomic fragments, treat the packet as a complete
datagram during IPv6 session decoding and recover the ports at the
transport offset found by the flow dissector.

Have ipv6_find_hdr() report non-atomic Fragment headers while it walks
the extension header chain. Pull the base IPv6 header before calling
it so nonlinear skbs are safe, and recover selectors only when every
Fragment header it sees is atomic.

Fixes: 7e3a42a12c4b ("xfrm6: handling fragment")
Cc: stable@vger.kernel.org
Signed-off-by: Jérémy Jean <Jeremy.Jean@oss.cyber.gouv.fr>
---
 include/net/ipv6.h      |  1 +
 net/ipv6/exthdrs_core.c |  6 +++++-
 net/xfrm/xfrm_policy.c  | 17 ++++++++++++++++-
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index 3de07e738538f..10785721b013d 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -1110,6 +1110,7 @@ enum {
 	IP6_FH_F_FRAG		= (1 << 0),
 	IP6_FH_F_AUTH		= (1 << 1),
 	IP6_FH_F_SKIP_RH	= (1 << 2),
+	IP6_FH_F_NON_ATOMIC	= (1 << 3),
 };
 
 /* find specified header and get offset to it */
diff --git a/net/ipv6/exthdrs_core.c b/net/ipv6/exthdrs_core.c
index 4a9748338cf40..9b9e2ce72e170 100644
--- a/net/ipv6/exthdrs_core.c
+++ b/net/ipv6/exthdrs_core.c
@@ -183,7 +183,8 @@ EXPORT_SYMBOL_GPL(ipv6_find_tlv);
  * isn't NULL.
  *
  * if flags is not NULL and it's a fragment, then the frag flag
- * IP6_FH_F_FRAG will be set. If it's an AH header, the
+ * IP6_FH_F_FRAG will be set. IP6_FH_F_NON_ATOMIC will be set for a
+ * non-atomic Fragment header. If it's an AH header, the
  * IP6_FH_F_AUTH flag is set and target < 0, then this function will
  * stop at the AH header. If IP6_FH_F_SKIP_RH flag was passed, then this
  * function will skip all those routing headers, where segements_left was 0.
@@ -254,6 +255,9 @@ int ipv6_find_hdr(const struct sk_buff *skb, unsigned int *offset,
 			if (!fp)
 				return -EBADMSG;
 
+			if (flags && (*fp & htons(IP6_OFFSET | IP6_MF)))
+				*flags |= IP6_FH_F_NON_ATOMIC;
+
 			_frag_off = ntohs(*fp) & ~0x7;
 			if (_frag_off) {
 				if (target < 0 &&
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index bda5100c9fdbf..679343b23df35 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -3526,9 +3526,24 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
 		decode_session4(&flkeys, fl, reverse);
 		break;
 #if IS_ENABLED(CONFIG_IPV6)
-	case AF_INET6:
+	case AF_INET6: {
+		unsigned int offset = 0;
+		int fh_flags = 0;
+
+		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
+		    pskb_network_may_pull(skb, sizeof(struct ipv6hdr)) &&
+		    ipv6_find_hdr(skb, &offset, -1, NULL, &fh_flags) >= 0 &&
+		    (fh_flags & IP6_FH_F_FRAG) &&
+		    !(fh_flags & IP6_FH_F_NON_ATOMIC)) {
+			flkeys.control.flags &= ~FLOW_DIS_IS_FRAGMENT;
+			flkeys.ports.ports = skb_flow_get_ports(skb,
+								flkeys.control.thoff,
+								flkeys.basic.ip_proto,
+								NULL, 0);
+		}
 		decode_session6(&flkeys, fl, reverse);
 		break;
+	}
 #endif
 	default:
 		return -EAFNOSUPPORT;
-- 
2.47.3

Re: [PATCH net v3 2/2] xfrm: fix selector matching for IPv6 atomic fragments

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 1 · 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 fix does nothing on the local-input (XFRM_POLICY_IN) path,
  which is the main bypass the commit message says it closes.
- [Medium] The new pskb_network_may_pull() call turns
  __xfrm_decode_session() from a read-only helper into one that can…

--

xfrm: fix selector matching for IPv6 atomic fragments

This patch makes __xfrm_decode_session() recover the ports, the ICMPv6
type and code, and the GRE key for IPv6 atomic fragments. Before this,
those fields were left zero for any packet with a Fragment header. It
adds an IP6_FH_F_NON_ATOMIC flag to ipv6_find_hdr() so the caller can
tell whether every Fragment header in the chain is atomic, and it pulls
the base IPv6 header before calling ipv6_find_hdr().

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index 4f6a5b81e3b5a..b7deeb03276e4 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -3530,9 +3530,24 @@ int __xfrm_decode_session(struct net *net, struct sk_buff *skb, struct flowi *fl
>  		decode_session4(&flkeys, fl, reverse);
>  		break;
>  #if IS_ENABLED(CONFIG_IPV6)
> -	case AF_INET6:
> +	case AF_INET6: {
> +		unsigned int offset = 0;
> +		int fh_flags = 0;
> +
> +		if ((flkeys.control.flags & FLOW_DIS_IS_FRAGMENT) &&
> +		    pskb_network_may_pull(skb, sizeof(struct ipv6hdr)) &&

[Severity: High]
Can this check ever succeed on the local input path?

When xfrm6_policy_check(XFRM_POLICY_IN) runs for a locally delivered
packet, skb->data already points at the transport header.
ip6_protocol_deliver_rcu() does:

    if (!pskb_pull(skb, skb_transport_offset(skb)))
        goto discard;
    ...
        if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb)) {

tcp_v6_rcv() and the UDP and ICMPv6 receive paths run their own IN checks
from the same position.

ipv6_frag_rcv() does not reassemble an atomic fragment. It only moves the
transport header forward:

    if (!(fhdr->frag_off & htons(IP6_OFFSET | IP6_MF))) {
        /* It is not a fragmented frame */
        skb->transport_header += sizeof(struct frag_hdr);

So skb_network_offset(skb) is negative here: -48 for a plain atomic
fragment carrying TCP. pskb_network_may_pull_reason() then computes:

    return pskb_may_pull_reason(skb, skb_network_offset(skb) + len);

Because len is unsigned, -48 + 40 wraps to 0xFFFFFFF8.
pskb_may_pull_reason() then returns SKB_DROP_REASON_PKT_TOO_SMALL from
the len > skb->len check.

The && chain stops at that point and FLOW_DIS_IS_FRAGMENT stays set.
decode_session6() then still leaves the ports, the ICMPv6 type and code,
and the GRE key at zero.

Wouldn't a plaintext atomic fragment sent to a local TCP, UDP or ICMPv6
endpoint still miss an IN policy that selects on those fields? If so,
only the FWD and OUT directions actually get the fix.

With CONFIG_DEBUG_NET, wouldn't each such packet also hit
DEBUG_NET_WARN_ON_ONCE(len > INT_MAX) in pskb_may_pull_reason()? One
remote packet would then be enough to cause a WARN, or a panic with
panic_on_warn set.

Instead of pskb_network_may_pull(), could the base header be read with
skb_header_pointer() at skb_network_offset(skb)? Another option would be
to pass an explicit offset to ipv6_find_hdr().

[Severity: Medium]
Could this pull leave stale header pointers in callers of
xfrm6_policy_check()?

Before this patch, __xfrm_decode_session() only read the skb through the
flow dissector, and the dissector uses skb_header_pointer().

pskb_may_pull_reason() calls skb_might_realloc() before any length check.
Under CONFIG_FAIL_SKB_REALLOC that does
pskb_expand_head(skb, 0, 0, GFP_ATOMIC). __pskb_pull_tail() can also call
pskb_expand_head() when the base header is not linear.

Several callers keep header pointers across the policy check. For
example, tcp_v6_rcv() does:

    th = (const struct tcphdr *)skb->data;
    hdr = ipv6_hdr(skb);
    ...
    if (!xfrm6_policy_check(NULL, XFRM_POLICY_IN, skb))
        goto discard_it;

    tcp_v6_fill_cb(skb, hdr, th);

The TIME_WAIT path in tcp_v6_rcv() follows the same pattern, and
tcp_inbound_hash() is passed &hdr->saddr and &hdr->daddr. ip6_forward()
saves hdr on entry and reads hdr->hop_limit after
xfrm6_policy_check(XFRM_POLICY_FWD).

If the head is reallocated here, wouldn't those reads touch the freed
head?

A concrete trigger was only found with CONFIG_FAIL_SKB_REALLOC.
ip6_rcv_core() already makes the base header linear for forwarded
packets. On local input, the wrapped length fails before
__pskb_pull_tail() is reached.

The pull seems to exist only to protect the ipv6_hdr(skb)->nexthdr read
at the start of ipv6_find_hdr(). Would a skb_header_pointer() read that
doesn't modify the skb, or an explicit start offset, cover that read?

> +		    ipv6_find_hdr(skb, &offset, -1, NULL, &fh_flags) >= 0 &&
> +		    (fh_flags & IP6_FH_F_FRAG) &&
> +		    !(fh_flags & IP6_FH_F_NON_ATOMIC)) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106202910.723098-2-Jeremy.Jean%40oss.cyber.gouv.fr

[syzbot ci] Re: xfrm: fix IPv6 atomic fragment policy handling

[syzbot ci]

syzbot ci has tested the following series

[v3] xfrm: fix IPv6 atomic fragment policy handling
https://lore.kernel.org/all/20261106202910.723098-2-Jeremy.Jean@oss.cyber.gouv.fr
* [PATCH net v3 1/2] xfrm: fix IPv6 first-fragment protocol decode
* [PATCH net v3 2/2] xfrm: fix selector matching for IPv6 atomic fragments

and found the following issue:
WARNING in __xfrm_decode_session

Full report is available here:
https://ci.syzbot.org/series/9d92ff08-0b82-4440-b345-9628353f5181

***

WARNING in __xfrm_decode_session

tree:      net
URL:       https://kernel.googlesource.com/pub/scm/linux/kernel/git/netdev/net.git
base:      2d2200551ccc3b9e444aecb59c4b53287e169266
arch:      amd64
compiler:  Debian clang version 22.1.8 (1~deb13u4), Debian LLD 22.1.8
config:    https://ci.syzbot.org/builds/fe2ead38-2a3d-497a-9bae-e5da0ee59933/config

------------[ cut here ]------------
len > ((int)(~0U >> 1))
WARNING: ./include/linux/skbuff.h:2856 at __xfrm_decode_session+0xafb/0xed0, CPU#0: syz.0.121/6184
Modules linked in:
CPU: 0 UID: 0 PID: 6184 Comm: syz.0.121 Not tainted syzkaller #0 PREEMPT(full) 
Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.16.2-debian-1.16.2-1 04/01/2014
RIP: 0010:__xfrm_decode_session+0xafb/0xed0
Code: bf e0 00 00 00 4d 03 3e 4c 89 f8 48 c1 e8 03 42 0f b6 04 28 84 c0 0f 85 11 03 00 00 45 8b 37 e9 33 fd ff ff e8 c6 c7 30 f7 90 <0f> 0b 90 e9 8c f7 ff ff 44 89 ef 8b 5c 24 20 89 de e8 bf c9 30 f7
RSP: 0018:ffffc9000e926b60 EFLAGS: 00010283
RAX: ffffffff8a9849aa RBX: 1ffff92001d24d74 RCX: 0000000000080000
RDX: ffffc90006ec1000 RSI: 0000000000000c7f RDI: 0000000000000c80
RBP: ffffc9000e926c90 R08: ffff88816dd8bc00 R09: 0000000000000002
R10: 000000000000000a R11: 0000000000000002 R12: ffffc9000e926bc0
R13: 00000000fffffff8 R14: ffff888105f226d8 R15: dffffc0000000000
FS:  00007fc136da96c0(0000) GS:ffff88818be0d000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000200000002cc0 CR3: 000000017256e000 CR4: 00000000000006f0
Call Trace:
 <TASK>
 icmpv6_route_lookup+0x44a/0x610
 icmp6_send+0x114c/0x1b50
 icmpv6_param_prob_reason+0x32/0x50
 ipv6_frag_rcv+0x927/0x2b40
 ip6_protocol_deliver_rcu+0xe8c/0x1680
 ip6_input_finish+0x191/0x370
 NF_HOOK+0x336/0x3c0
 ip6_input+0x16a/0x270
 NF_HOOK+0x336/0x3c0
 netif_receive_skb+0x278/0xbf0
 tun_rx_batched+0x1de/0x790
 tun_get_user+0x2b78/0x4470
 tun_chr_write_iter+0x113/0x200
 vfs_write+0x612/0xba0
 ksys_write+0x150/0x270
 do_syscall_64+0x166/0x520
 entry_SYSCALL_64_after_hwframe+0x77/0x7f
RIP: 0033:0x7fc135f6974e
Code: 08 0f 85 a5 a8 ff ff 49 89 fb 48 89 f0 48 89 d7 48 89 ce 4c 89 c2 4d 89 ca 4c 8b 44 24 08 4c 8b 4c 24 10 4c 89 5c 24 08 0f 05 <c3> 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 80 00 00 00 00 48 83 ec 08
RSP: 002b:00007fc136da8fb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007fc136da96c0 RCX: 00007fc135f6974e
RDX: 000000000000004a RSI: 0000200000000700 RDI: 00000000000000c8
RBP: 00007fc136037f6d R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
R13: 00007fc136226038 R14: 00007fc136225fa0 R15: 00007ffcfeaad798
 </TASK>


***

If these findings have caused you to resend the series or submit a
separate fix, please add the following tag to your commit message:
  Tested-by: syzbot@syzkaller.appspotmail.com

---
This report is generated by a bot. It may contain errors.
syzbot ci engineers can be reached at syzkaller@googlegroups.com.

To test a fix for this bug, please reply with `#syz test`
(on a separate line) and attach the patch to the email.

Notes:
- The patch will be applied on top of the tested series (as an
  incremental fix).
- To test a new version of the whole series, please send it directly
  to syzbot@lists.linux.dev.
- Arguments like custom git repos and branches are not supported.