Re: [PATCH 01/14] xfrm: retry inexact policy lookup after node reinsertion

Re: [PATCH 01/14] xfrm: retry inexact policy lookup after node reinsertion

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

[PATCH 0/14] pull request (net): ipsec 2026-11-02

[Steffen Klassert]

1) xfrm: retry inexact policy lookup after node reinsertion
   Detect a concurrent change of the inexact policy tree while it is
   searched and retry the lookup, so an insertion can't make it return
   the wrong policy.

2) xfrm: interface: validate the IP header on xmit
   Validate the IP header in xfrmi_xmit() like the IP receive path
   does, so packets injected into an xfrm interface can't reach the
   output path with a malformed header.

3) xfrm: zero-initialise km_event in the replay-notify path
   Zero-initialise the replay event so the netlink header of a
   kernel-originated replay notification no longer carries
   uninitialised stack values.

4) xfrm: espintcp: reorder operations within espintcp_init_sk
   Publish the ESP-in-TCP context before installing the socket
   callbacks, so an early callback can't dereference a context that
   isn't set up yet.

5) xfrm: state: reject mark with bits outside its mask on add
   Reject a mark with bits outside its mask on state insert instead
   of silently truncating it, so a lookup can't be given a mark that
   never matches.

6) xfrm: state: reject mark with bits outside its mask on ALLOCSPI
   Apply the same mark validation to ALLOCSPI, keeping the state and
   policy insert paths consistent.

7) xfrm: policy: reject mark with bits outside its mask on add
   Apply the same mark validation to policy insert, so a policy can't
   be stored with a mark that never matches.

8) xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups
   Match mark and mask exactly in the SPI-keyed netlink SA lookups, so
   a catch-all SA can no longer shadow a more specific one.

9) xfrm: fix use-after-free of migrated state in xfrm_do_migrate_state()
   Hold the cloned state for the whole migration, so a concurrent
   delete can't free it while it is still in use.

10) xfrm: fix hw offload state leak on xfrm_do_migrate_state() error path
    Remove the hardware offload of the cloned state on the migration
    error path, so the offloaded SA entry isn't leaked.

11) xfrm: include mark in MIGRATE_STATE SA collision check
    Include the mark in the SA collision check, so a migration that
    only changes the mark can't install a duplicate SA tuple.

12) xfrm: pass extack through to xfrm_init_replay() from xfrm_init_state()
    Pass the extack to xfrm_init_replay(), so replay window errors can
    be reported to userspace.

13) docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple
    Document the mark as part of the EEXIST tuple of the SA migration
    request.

14) xfrm4: use a full socket for local errors
    Resolve the socket owner with skb_to_full_sk() in the IPv4 local
    error path, so a TCP request socket isn't used as a full socket.

Please pull or let me know if there are problems.

Thanks!

The following changes since commit c9151088f1674fd29ff26a20f5fc687acf53a2f0:

  Merge branch 'net-psp-avoid-conflicts-with-skb-decrypted-and-sk_validate_xmit_skb' (2026-09-16 19:18:26 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/klassert/ipsec.git tags/ipsec-2026-11-02

for you to fetch changes up to 5def91015ba7b59de62a81384572da7281b8a033:

  xfrm4: use a full socket for local errors (2026-10-27 09:09:36 +0000)

----------------------------------------------------------------
ipsec-2026-11-02

----------------------------------------------------------------
Antony Antony (9):
      xfrm: state: reject mark with bits outside its mask on add
      xfrm: state: reject mark with bits outside its mask on ALLOCSPI
      xfrm: policy: reject mark with bits outside its mask on add
      xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups
      xfrm: fix use-after-free of migrated state in xfrm_do_migrate_state()
      xfrm: fix hw offload state leak on xfrm_do_migrate_state() error path
      xfrm: include mark in MIGRATE_STATE SA collision check
      xfrm: pass extack through to xfrm_init_replay() from xfrm_init_state()
      docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

Chengfeng Ye (1):
      xfrm: retry inexact policy lookup after node reinsertion

Fourie Zhang (1):
      xfrm4: use a full socket for local errors

Qihang (1):
      xfrm: interface: validate the IP header on xmit

Sabrina Dubroca (1):
      xfrm: espintcp: reorder operations within espintcp_init_sk

Shubham Antil (1):
      xfrm: zero-initialise km_event in the replay-notify path

Steffen Klassert (1):
      Merge branch 'xfrm: state: exact mark/mask match for control-plane SA lookups'

 .../networking/xfrm/xfrm_migrate_state.rst         |  25 +++--
 include/net/espintcp.h                             |   5 +-
 include/net/xfrm.h                                 |   7 ++
 net/ipv4/xfrm4_output.c                            |   7 +-
 net/xfrm/espintcp.c                                |  43 +++++----
 net/xfrm/xfrm_interface_core.c                     |   8 ++
 net/xfrm/xfrm_policy.c                             |  14 ++-
 net/xfrm/xfrm_replay.c                             |   6 +-
 net/xfrm/xfrm_state.c                              | 101 +++++++++++++++++----
 net/xfrm/xfrm_user.c                               |  81 ++++++++++++-----
 10 files changed, 220 insertions(+), 77 deletions(-)

[PATCH 01/14] xfrm: retry inexact policy lookup after node reinsertion

[Steffen Klassert]

From: Chengfeng Ye <nicoyip.dev@gmail.com>

An inexact policy lookup first records pointers to candidate hlist heads
and then traverses the lists.  A concurrent policy insertion can merge
inexact tree nodes between those operations:

  lookup                         policy insertion
  ------                         ----------------
  find inexact candidates
    save obsolete hlist head
                                 write_seqcount_begin(&bin->count)
                                 merge inexact tree nodes
                                   hlist_del_rcu(&policy->bydst)
                                   reinsert policy->bydst in survivor
                                 write_seqcount_end(&bin->count)
  evaluate saved candidate list
    miss the moved policy

The merge immediately reinserts the same hlist node into the surviving
tree node.  RCU keeps the policy alive, but it does not provide a
consistent view while its list node is moved.  A lookup that selected the
obsolete list can observe it empty.  A lookup already traversing a moved
policy can instead follow the next pointer rewritten by the reinsertion.
Either case can return an incorrect IPsec policy result.

The per-bin sequence counter already brackets calls to
xfrm_policy_inexact_insert_node(), including node merges.  The read side,
however, currently validates the counter only while searching an
individual rb-tree.  A successful search returns without validation, and
the later candidate-list traversal is outside that read-side section.

Snapshot the per-bin sequence before discovering candidate heads and
validate it after evaluating all candidate lists.   Retry the lookup if
the inexact policy tree changes while it is being searched.

Fixes: 9cf545ebd591 ("xfrm: policy: store inexact policies in a tree ordered by destination address")
Cc: stable@vger.kernel.org
Signed-off-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index 513c9f228334..f6f40ba713d5 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -2157,6 +2157,7 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 	struct xfrm_pol_inexact_bin *bin;
 	struct xfrm_policy *pol, *ret;
 	struct hlist_head *chain;
+	unsigned int inexact_sequence;
 	unsigned int sequence;
 	int err;
 
@@ -2191,12 +2192,18 @@ static struct xfrm_policy *xfrm_policy_lookup_bytype(struct net *net, u8 type,
 		goto skip_inexact;
 
 	bin = xfrm_policy_inexact_lookup_rcu(net, type, family, dir, if_id);
-	if (!bin || !xfrm_policy_find_inexact_candidates(&cand, bin, saddr,
-							 daddr))
+	if (!bin)
+		goto skip_inexact;
+
+	inexact_sequence = read_seqcount_begin(&bin->count);
+	if (!xfrm_policy_find_inexact_candidates(&cand, bin, saddr, daddr))
 		goto skip_inexact;
 
 	pol = xfrm_policy_eval_candidates(&cand, ret, fl, type,
 					  family, if_id);
+	if (read_seqcount_retry(&bin->count, inexact_sequence))
+		goto retry;
+
 	if (pol) {
 		ret = pol;
 		if (IS_ERR(pol))
-- 
2.53.0

[PATCH 02/14] xfrm: interface: validate the IP header on xmit

[Steffen Klassert]

From: Qihang <q.h.hack.winter@gmail.com>

Packets injected into an xfrm interface (e.g. AF_PACKET on an xfrmi
device) reach the xfrm output path without IP header validation, but
xfrm4_transport_output() uses iph->ihl for __skb_pull() and memmove()
and expects the header to be valid.

vti_tunnel_xmit() already calls pskb_inet_may_pull() before handing
packets to the xfrm output path.  Do the same in xfrmi_xmit() and
perform the header checks ip_rcv_core()/ip6_rcv_core() do on entry
to the IP stack: reject wrong IP versions and malformed IPv4 header
lengths.

Fixes: f203b76d7809 ("xfrm: Add virtual xfrm interfaces")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Qihang <q.h.hack.winter@gmail.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_interface_core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
index 688306bf62c5..9835fbe63ccb 100644
--- a/net/xfrm/xfrm_interface_core.c
+++ b/net/xfrm/xfrm_interface_core.c
@@ -531,8 +531,13 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	memset(&fl, 0, sizeof(fl));
 
+	if (!pskb_inet_may_pull(skb))
+		goto tx_err;
+
 	switch (skb->protocol) {
 	case htons(ETH_P_IPV6):
+		if (ipv6_hdr(skb)->version != 6)
+			goto tx_err;
 		memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
 		xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6);
 		if (!dst) {
@@ -548,6 +553,9 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev)
 		}
 		break;
 	case htons(ETH_P_IP):
+		if (ip_hdr(skb)->version != 4 || ip_hdr(skb)->ihl < 5 ||
+		    !pskb_network_may_pull(skb, ip_hdr(skb)->ihl * 4))
+			goto tx_err;
 		memset(IPCB(skb), 0, sizeof(*IPCB(skb)));
 		xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET);
 		if (!dst) {
-- 
2.53.0

[PATCH 03/14] xfrm: zero-initialise km_event in the replay-notify path

[Steffen Klassert]

From: Shubham Antil <shubham@octane.security>

xfrm_replay_notify(), xfrm_replay_notify_bmp() and
xfrm_replay_notify_esn() declare a struct km_event on the stack and
initialise only its .event and .data.aevent fields, leaving .seq and
.portid uninitialised. build_aevent() then builds the XFRM_MSG_NEWAE
message with those fields:

  nlmsg_put(skb, c->portid, c->seq, ...)

so the netlink header of a replay event carries uninitialised stack
values rather than defined ones. This was confirmed at runtime rather
than by inspection alone: the .seq and .portid carried in that header
hold values that nothing in the replay path assigns.

The request-driven paths set these header fields from the requester
(xfrm_get_ae() / xfrm_new_ae()); only the kernel-originated replay path
leaves them unset. Zero-initialise the event so the header fields are 0,
the correct value for a kernel-originated notification.

Fixes: f8cd54884e67 ("[IPSEC]: Sync series - core changes")
Reported-by: Giovanni Vignone <gio@octane.security>
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Shubham Antil <shubham@octane.security>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_replay.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_replay.c b/net/xfrm/xfrm_replay.c
index dbdf8a39dffe..9394953247ab 100644
--- a/net/xfrm/xfrm_replay.c
+++ b/net/xfrm/xfrm_replay.c
@@ -40,7 +40,7 @@ static void xfrm_replay_notify_esn(struct xfrm_state *x, int event);
 
 void xfrm_replay_notify(struct xfrm_state *x, int event)
 {
-	struct km_event c;
+	struct km_event c = {};
 	/* we send notify messages in case
 	 *  1. we updated on of the sequence numbers, and the seqno difference
 	 *     is at least x->replay_maxdiff, in this case we also update the
@@ -304,7 +304,7 @@ static void xfrm_replay_advance_bmp(struct xfrm_state *x, __be32 net_seq)
 
 static void xfrm_replay_notify_bmp(struct xfrm_state *x, int event)
 {
-	struct km_event c;
+	struct km_event c = {};
 	struct xfrm_replay_state_esn *replay_esn = x->replay_esn;
 	struct xfrm_replay_state_esn *preplay_esn = x->preplay_esn;
 
@@ -356,7 +356,7 @@ static void xfrm_replay_notify_bmp(struct xfrm_state *x, int event)
 static void xfrm_replay_notify_esn(struct xfrm_state *x, int event)
 {
 	u32 seq_diff, oseq_diff;
-	struct km_event c;
+	struct km_event c = {};
 	struct xfrm_replay_state_esn *replay_esn = x->replay_esn;
 	struct xfrm_replay_state_esn *preplay_esn = x->preplay_esn;
 
-- 
2.53.0

[PATCH 04/14] xfrm: espintcp: reorder operations within espintcp_init_sk

[Steffen Klassert]

From: Sabrina Dubroca <sd@queasysnail.net>

When enabled on a socket, espintcp sets the socket callbacks to its
own before it has finished setting up its context and published it as
icsk_ulp_data. Any one of those callbacks that gets called before will
dereference a NULL icsk_ulp_data.

Fix this by reording the operations, and add memory barriers to ensure
callers will have a struct espintcp_ctx available. Add a few
WRITE_ONCE/READ_ONCE too while we're there.

Fixes: e27cca96cd68 ("xfrm: add espintcp (RFC 8229)")
Cc: stable@vger.kernel.org
Reported-by: Yuan Tan <yuantan098@gmail.com>
Reported-by: Yifan Wu <yifanwucs@gmail.com>
Reported-by: Juefei Pu <tomapufckgml@gmail.com>
Reported-by: Xin Liu <bird@lzu.edu.cn>
Reported-by: Peihan Liu <ronbogo@outlook.com>
Reported-by: Yilin Zhu <zylzyl2333@gmail.com>
Reported-by: Ren Wei <n05ec@lzu.edu.cn>
Reported-by: Eulgyu Kim <eulgyukim@snu.ac.kr>
Reported-by: Jaeyoung Chung <jjy600901@snu.ac.kr>
Link: https://lore.kernel.org/all/c30b645074a1b379e0f7fe297f917c66137d9964.1778464688.git.zylzyl2333@gmail.com/
Link: https://lore.kernel.org/all/20260819155349.3555804-1-jjy600901@snu.ac.kr
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/espintcp.h |  5 +++--
 net/xfrm/espintcp.c    | 43 +++++++++++++++++++++++++-----------------
 2 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/include/net/espintcp.h b/include/net/espintcp.h
index c70efd704b6d..7cad3c6822df 100644
--- a/include/net/espintcp.h
+++ b/include/net/espintcp.h
@@ -34,7 +34,8 @@ static inline struct espintcp_ctx *espintcp_getctx(const struct sock *sk)
 {
 	const struct inet_connection_sock *icsk = inet_csk(sk);
 
-	/* RCU is only needed for diag */
-	return (__force void *)icsk->icsk_ulp_data;
+	/* pairs with smp_wmb() in espintcp_init_sk() */
+	smp_rmb();
+	return (__force void *)READ_ONCE(icsk->icsk_ulp_data);
 }
 #endif
diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
index 3e72b9f067b9..4052ae5bbb2e 100644
--- a/net/xfrm/espintcp.c
+++ b/net/xfrm/espintcp.c
@@ -434,7 +434,9 @@ static void espintcp_destruct(struct sock *sk)
 
 bool tcp_is_ulp_esp(struct sock *sk)
 {
-	return sk->sk_prot == &espintcp_prot || sk->sk_prot == &espintcp6_prot;
+	const struct proto *prot = READ_ONCE(sk->sk_prot);
+
+	return prot == &espintcp_prot || prot == &espintcp6_prot;
 }
 EXPORT_SYMBOL_GPL(tcp_is_ulp_esp);
 
@@ -466,34 +468,41 @@ static int espintcp_init_sk(struct sock *sk)
 
 	__sk_dst_reset(sk);
 
-	strp_check_rcv(&ctx->strp);
 	skb_queue_head_init(&ctx->ike_queue);
 	skb_queue_head_init(&ctx->out_queue);
+	INIT_WORK(&ctx->work, espintcp_tx_work);
+
+	ctx->saved_data_ready = sk->sk_data_ready;
+	ctx->saved_write_space = sk->sk_write_space;
+	ctx->saved_destruct = sk->sk_destruct;
+
+	/* avoid using task_frag */
+	sk->sk_allocation = GFP_ATOMIC;
+	sk->sk_use_task_frag = false;
+
+	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
+
+	/* pairs with smp_rmb() in espintcp_getctx() */
+	smp_wmb();
+
+	WRITE_ONCE(sk->sk_data_ready, espintcp_data_ready);
+	WRITE_ONCE(sk->sk_write_space, espintcp_write_space);
+	sk->sk_destruct = espintcp_destruct;
 
 	if (sk->sk_family == AF_INET) {
-		sk->sk_prot = &espintcp_prot;
-		sk->sk_socket->ops = &espintcp_ops;
+		WRITE_ONCE(sk->sk_prot, &espintcp_prot);
+		WRITE_ONCE(sk->sk_socket->ops, &espintcp_ops);
 	} else {
 		mutex_lock(&tcpv6_prot_mutex);
 		if (!espintcp6_prot.recvmsg)
 			build_protos(&espintcp6_prot, &espintcp6_ops, sk->sk_prot, sk->sk_socket->ops);
 		mutex_unlock(&tcpv6_prot_mutex);
 
-		sk->sk_prot = &espintcp6_prot;
-		sk->sk_socket->ops = &espintcp6_ops;
+		WRITE_ONCE(sk->sk_prot, &espintcp6_prot);
+		WRITE_ONCE(sk->sk_socket->ops, &espintcp6_ops);
 	}
-	ctx->saved_data_ready = sk->sk_data_ready;
-	ctx->saved_write_space = sk->sk_write_space;
-	ctx->saved_destruct = sk->sk_destruct;
-	sk->sk_data_ready = espintcp_data_ready;
-	sk->sk_write_space = espintcp_write_space;
-	sk->sk_destruct = espintcp_destruct;
-	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
-	INIT_WORK(&ctx->work, espintcp_tx_work);
 
-	/* avoid using task_frag */
-	sk->sk_allocation = GFP_ATOMIC;
-	sk->sk_use_task_frag = false;
+	strp_check_rcv(&ctx->strp);
 
 	return 0;
 
-- 
2.53.0

[PATCH 05/14] xfrm: state: reject mark with bits outside its mask on add

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

A mark/mask like 0x101/0xff is silently truncated to 0x01/0xff on
insert, so the exact-match lookups added later in this series (DELSA,
GETSA, GETAE, NEWAE, EXPIRE, MIGRATE_STATE) could not find the SA.
Reject it instead.

The mark is no longer sanitized on state add.

Also reject an invalid XFRMA_MARK (the new mark) in
XFRM_MSG_MIGRATE_STATE.

Fixes: 0b91fda3a1f0 ("xfrm: Sanitize marks before insert")
Cc: <stable+noautosel@kernel.org> # needs exact-match SA lookups from this series
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c |  3 ---
 net/xfrm/xfrm_user.c  | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index e45aa1ed5b96..602404ffc4d4 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1732,9 +1732,6 @@ static void __xfrm_state_insert(struct xfrm_state *x)
 
 	list_add(&x->km.all, &net->xfrm.state_all);
 
-	/* Sanitize mark before store */
-	x->mark.v &= x->mark.m;
-
 	h = xfrm_dst_hash(net, &x->id.daddr, &x->props.saddr,
 			  x->props.reqid, x->props.family);
 	XFRM_STATE_INSERT(bydst, &x->bydst,
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index a2587c7e796b..0723d791b8b7 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -314,6 +314,22 @@ static int verify_selector_prefixlen(u16 family,
 	}
 }
 
+static int verify_mark(struct nlattr **attrs, struct netlink_ext_ack *extack)
+{
+	const struct xfrm_mark *m;
+
+	if (!attrs[XFRMA_MARK])
+		return 0;
+
+	m = nla_data(attrs[XFRMA_MARK]);
+	if ((m->v & m->m) != m->v) {
+		NL_SET_ERR_MSG(extack, "Invalid mark value/mask combination");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int verify_newsa_info(struct xfrm_usersa_info *p,
 			     struct nlattr **attrs,
 			     struct netlink_ext_ack *extack)
@@ -333,6 +349,10 @@ static int verify_newsa_info(struct xfrm_usersa_info *p,
 	if (err)
 		goto out;
 
+	err = verify_mark(attrs, extack);
+	if (err)
+		goto out;
+
 	err = -EINVAL;
 	switch (p->id.proto) {
 	case IPPROTO_AH:
@@ -3370,6 +3390,10 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 			return err;
 	}
 
+	err = verify_mark(attrs, extack);
+	if (err)
+		return err;
+
 	copy_from_user_migrate_state(&m, um);
 
 	x = xfrm_state_lookup(net, m.old_mark.v & m.old_mark.m,
-- 
2.53.0

[PATCH 06/14] xfrm: state: reject mark with bits outside its mask on ALLOCSPI

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

Like state add and policy add in this series, ALLOCSPI rejects a mark
whose value has bits outside its mask.

Fixes: bd55775c8dd6 ("xfrm: SA lookups signature with mark")
Cc: <stable+noautosel@kernel.org> # avoid breaking existing userspace ABI
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 0723d791b8b7..6b53373168b0 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1916,6 +1916,9 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	x = NULL;
 
+	err = verify_mark(attrs, extack);
+	if (err)
+		goto out_noput;
 	mark = xfrm_mark_get(attrs, &m);
 
 	if (attrs[XFRMA_IF_ID])
-- 
2.53.0

[PATCH 07/14] xfrm: policy: reject mark with bits outside its mask on add

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

Same issue as states: an invalid mark/mask is silently truncated on
insert, so GETPOLICY and DELPOLICY can no longer find the policy by
id. Reject it instead.

The mark is no longer sanitized on policy insert.

Fixes: 0b91fda3a1f0 ("xfrm: Sanitize marks before insert")
Cc: <stable+noautosel@kernel.org> # avoid breaking existing userspace ABI
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_policy.c | 3 ---
 net/xfrm/xfrm_user.c   | 3 +++
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f6f40ba713d5..0f6fcea28bcd 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1575,9 +1575,6 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
 	struct xfrm_policy *delpol;
 	struct hlist_head *chain;
 
-	/* Sanitize mark before store */
-	policy->mark.v &= policy->mark.m;
-
 	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
 	chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
 	if (chain)
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 6b53373168b0..c1571c7a2315 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -2307,6 +2307,9 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
 	if (err)
 		return err;
 	err = verify_sec_ctx_len(attrs, extack);
+	if (err)
+		return err;
+	err = verify_mark(attrs, extack);
 	if (err)
 		return err;
 
-- 
2.53.0

[PATCH 08/14] xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

Add __xfrm_state_lookup_exact(); wire it into DELSA/GETSA, GETAE/NEWAE,
EXPIRE, MIGRATE_STATE.  xfrm_state_update() (UPDSA) keeps wildcard match
because ALLOCSPI's larval state may be unmarked (mark 0/0), so an
exact-match UPDSA would not find it and the ADD fallback would then
fail with -EEXIST via xfrm_state_add().

Fixes: 3d6acfa7641f ("xfrm: SA lookups with mark")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/xfrm.h    |  3 +++
 net/xfrm/xfrm_state.c | 56 +++++++++++++++++++++++++++++++++++--------
 net/xfrm/xfrm_user.c  | 29 +++++++++++-----------
 3 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index a6d69aaa6cd2..9e053388aa4b 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1748,6 +1748,9 @@ struct xfrm_state *xfrm_state_lookup_byaddr(struct net *net, u32 mark,
 					    const xfrm_address_t *saddr,
 					    u8 proto,
 					    unsigned short family);
+struct xfrm_state *xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
+					   const xfrm_address_t *daddr, __be32 spi,
+					   u8 proto, unsigned short family);
 #ifdef CONFIG_XFRM_SUB_POLICY
 void xfrm_tmpl_sort(struct xfrm_tmpl **dst, struct xfrm_tmpl **src, int n,
 		    unsigned short family);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 602404ffc4d4..f1e04df8d501 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1177,11 +1177,19 @@ static struct xfrm_state *__xfrm_state_lookup_all(const struct xfrm_hash_state_p
 	return NULL;
 }
 
-static struct xfrm_state *__xfrm_state_lookup(const struct xfrm_hash_state_ptrs *state_ptrs,
-					      u32 mark,
-					      const xfrm_address_t *daddr,
-					      __be32 spi, u8 proto,
-					      unsigned short family)
+static bool xfrm_state_mark_matches(const struct xfrm_state *x, u32 mark, u32 mask, bool exact)
+{
+	if (exact)
+		return x->mark.v == mark && x->mark.m == mask;
+	return (mark & x->mark.m) == x->mark.v;
+}
+
+static struct xfrm_state *
+__xfrm_state_lookup(const struct xfrm_hash_state_ptrs *state_ptrs,
+		    u32 mark, u32 mask, bool exact,
+		    const xfrm_address_t *daddr,
+		    __be32 spi, u8 proto,
+		    unsigned short family)
 {
 	unsigned int h = __xfrm_spi_hash(daddr, spi, proto, family, state_ptrs->hmask);
 	struct xfrm_state *x;
@@ -1193,7 +1201,7 @@ static struct xfrm_state *__xfrm_state_lookup(const struct xfrm_hash_state_ptrs
 		    !xfrm_addr_equal(&x->id.daddr, daddr, family))
 			continue;
 
-		if ((mark & x->mark.m) != x->mark.v)
+		if (!xfrm_state_mark_matches(x, mark, mask, exact))
 			continue;
 		if (!xfrm_state_hold_rcu(x))
 			continue;
@@ -1203,6 +1211,17 @@ static struct xfrm_state *__xfrm_state_lookup(const struct xfrm_hash_state_ptrs
 	return NULL;
 }
 
+static struct xfrm_state *
+__xfrm_state_lookup_exact(const struct xfrm_hash_state_ptrs *state_ptrs,
+			  const struct xfrm_mark *mark,
+			  const xfrm_address_t *daddr,
+			  __be32 spi, u8 proto,
+			  unsigned short family)
+{
+	return __xfrm_state_lookup(state_ptrs, mark->v, mark->m, true,
+				   daddr, spi, proto, family);
+}
+
 struct xfrm_state *xfrm_input_state_lookup(struct net *net, u32 mark,
 					   const xfrm_address_t *daddr,
 					   __be32 spi, u8 proto,
@@ -1233,7 +1252,7 @@ struct xfrm_state *xfrm_input_state_lookup(struct net *net, u32 mark,
 
 	xfrm_hash_ptrs_get(net, &state_ptrs);
 
-	x = __xfrm_state_lookup(&state_ptrs, mark, daddr, spi, proto, family);
+	x = __xfrm_state_lookup(&state_ptrs, mark, 0, false, daddr, spi, proto, family);
 	if (x) {
 		spin_lock(&net->xfrm.xfrm_state_lock);
 		if (x->km.state != XFRM_STATE_VALID) {
@@ -1283,7 +1302,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(const struct xfrm_hash_stat
 	return NULL;
 }
 
-static inline struct xfrm_state *
+static struct xfrm_state *
 __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
 {
 	struct xfrm_hash_state_ptrs state_ptrs;
@@ -1293,7 +1312,7 @@ __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
 	xfrm_hash_ptrs_get(net, &state_ptrs);
 
 	if (use_spi)
-		return __xfrm_state_lookup(&state_ptrs, mark, &x->id.daddr,
+		return __xfrm_state_lookup(&state_ptrs, mark, 0, false, &x->id.daddr,
 					   x->id.spi, x->id.proto, family);
 	else
 		return __xfrm_state_lookup_byaddr(&state_ptrs, mark,
@@ -2380,7 +2399,7 @@ xfrm_state_lookup(struct net *net, u32 mark, const xfrm_address_t *daddr, __be32
 	rcu_read_lock();
 	xfrm_hash_ptrs_get(net, &state_ptrs);
 
-	x = __xfrm_state_lookup(&state_ptrs, mark, daddr, spi, proto, family);
+	x = __xfrm_state_lookup(&state_ptrs, mark, 0, false, daddr, spi, proto, family);
 	rcu_read_unlock();
 	return x;
 }
@@ -2404,6 +2423,23 @@ xfrm_state_lookup_byaddr(struct net *net, u32 mark,
 }
 EXPORT_SYMBOL(xfrm_state_lookup_byaddr);
 
+struct xfrm_state *
+xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
+			const xfrm_address_t *daddr, __be32 spi,
+			u8 proto, unsigned short family)
+{
+	struct xfrm_hash_state_ptrs state_ptrs;
+	struct xfrm_state *x;
+
+	rcu_read_lock();
+	xfrm_hash_ptrs_get(net, &state_ptrs);
+
+	x = __xfrm_state_lookup_exact(&state_ptrs, mark, daddr, spi, proto, family);
+	rcu_read_unlock();
+	return x;
+}
+EXPORT_SYMBOL(xfrm_state_lookup_exact);
+
 struct xfrm_state *
 xfrm_find_acq(struct net *net, const struct xfrm_mark *mark, u8 mode, u32 reqid,
 	      u32 if_id, u32 pcpu_num, u8 proto, const xfrm_address_t *daddr,
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index c1571c7a2315..5a9f729b4fcb 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -1109,11 +1109,12 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
 	struct xfrm_state *x = NULL;
 	struct xfrm_mark m;
 	int err;
-	u32 mark = xfrm_mark_get(attrs, &m);
+
+	xfrm_mark_get(attrs, &m);
 
 	if (xfrm_id_proto_match(p->proto, IPSEC_PROTO_ANY)) {
 		err = -ESRCH;
-		x = xfrm_state_lookup(net, mark, &p->daddr, p->spi, p->proto, p->family);
+		x = xfrm_state_lookup_exact(net, &m, &p->daddr, p->spi, p->proto, p->family);
 	} else {
 		xfrm_address_t *saddr = NULL;
 
@@ -1124,7 +1125,7 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
 		}
 
 		err = -ESRCH;
-		x = xfrm_state_lookup_byaddr(net, mark,
+		x = xfrm_state_lookup_byaddr(net, m.v & m.m,
 					     &p->daddr, saddr,
 					     p->proto, p->family);
 	}
@@ -2821,14 +2822,13 @@ static int xfrm_get_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct sk_buff *r_skb;
 	int err;
 	struct km_event c;
-	u32 mark;
 	struct xfrm_mark m;
 	struct xfrm_aevent_id *p = nlmsg_data(nlh);
 	struct xfrm_usersa_id *id = &p->sa_id;
 
-	mark = xfrm_mark_get(attrs, &m);
+	xfrm_mark_get(attrs, &m);
 
-	x = xfrm_state_lookup(net, mark, &id->daddr, id->spi, id->proto, id->family);
+	x = xfrm_state_lookup_exact(net, &m, &id->daddr, id->spi, id->proto, id->family);
 	if (x == NULL)
 		return -ESRCH;
 
@@ -2869,7 +2869,6 @@ static int xfrm_new_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct xfrm_state *x;
 	struct km_event c;
 	int err = -EINVAL;
-	u32 mark = 0;
 	struct xfrm_mark m;
 	struct xfrm_aevent_id *p = nlmsg_data(nlh);
 	struct nlattr *rp = attrs[XFRMA_REPLAY_VAL];
@@ -2889,9 +2888,10 @@ static int xfrm_new_ae(struct sk_buff *skb, struct nlmsghdr *nlh,
 		return err;
 	}
 
-	mark = xfrm_mark_get(attrs, &m);
+	xfrm_mark_get(attrs, &m);
 
-	x = xfrm_state_lookup(net, mark, &p->sa_id.daddr, p->sa_id.spi, p->sa_id.proto, p->sa_id.family);
+	x = xfrm_state_lookup_exact(net, &m, &p->sa_id.daddr, p->sa_id.spi,
+				    p->sa_id.proto, p->sa_id.family);
 	if (x == NULL)
 		return -ESRCH;
 
@@ -3025,9 +3025,10 @@ static int xfrm_add_sa_expire(struct sk_buff *skb, struct nlmsghdr *nlh,
 	struct xfrm_user_expire *ue = nlmsg_data(nlh);
 	struct xfrm_usersa_info *p = &ue->state;
 	struct xfrm_mark m;
-	u32 mark = xfrm_mark_get(attrs, &m);
 
-	x = xfrm_state_lookup(net, mark, &p->id.daddr, p->id.spi, p->id.proto, p->family);
+	xfrm_mark_get(attrs, &m);
+
+	x = xfrm_state_lookup_exact(net, &m, &p->id.daddr, p->id.spi, p->id.proto, p->family);
 
 	err = -ENOENT;
 	if (x == NULL)
@@ -3402,9 +3403,9 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	copy_from_user_migrate_state(&m, um);
 
-	x = xfrm_state_lookup(net, m.old_mark.v & m.old_mark.m,
-			      &um->id.daddr, um->id.spi,
-			      um->id.proto, um->id.family);
+	x = xfrm_state_lookup_exact(net, &m.old_mark,
+				    &um->id.daddr, um->id.spi,
+				    um->id.proto, um->id.family);
 	if (!x) {
 		NL_SET_ERR_MSG(extack, "Can not find state");
 		return -ESRCH;
-- 
2.53.0

[PATCH 09/14] xfrm: fix use-after-free of migrated state in xfrm_do_migrate_state()

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

A concurrent delete can free the migrated state before
xfrm_do_migrate_state() finishes using it.

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 5a9f729b4fcb..8640838dcda0 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3509,6 +3509,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 	__xfrm_state_delete(x);
 	spin_unlock_bh(&x->lock);
 
+	xfrm_state_hold(xc);
 	err = xfrm_state_migrate_install(x, xc, &m, extack);
 	if (err < 0) {
 		/*
@@ -3516,6 +3517,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 		 * free under xfrm_cfg_mutex. Both SAs are gone if it does;
 		 * restoring x would risk SN/IV reuse.
 		 */
+		xfrm_state_put(xc);
 		goto out;
 	}
 
@@ -3534,6 +3536,7 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 		err = 0;
 	}
 
+	xfrm_state_put(xc);
 out:
 	xfrm_state_put(x);
 	return err;
-- 
2.53.0

[PATCH 10/14] xfrm: fix hw offload state leak on xfrm_do_migrate_state() error path

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

In the error path, the cloned state is dropped without removing its
hardware offload, leaking the offloaded SA entry.

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_user.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 8640838dcda0..4de8c401eb44 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3541,6 +3541,8 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 	xfrm_state_put(x);
 	return err;
 out_xc:
+	if (m.xuo)
+		xfrm_dev_state_delete(xc);
 	xc->km.state = XFRM_STATE_DEAD;
 	xfrm_state_put(xc);
 	xfrm_state_put(x);
-- 
2.53.0

[PATCH 11/14] xfrm: include mark in MIGRATE_STATE SA collision check

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

The SA lookup tuple is (daddr, spi, proto, family, mark), but the
EEXIST pre-check and the xfrm_state_insert() vs xfrm_state_add()
decision only considered daddr and family, ignoring mark.
A migration that only changes the mark inserts a duplicate SA tuple
into the hash tables.

The collision check is implemented as xfrm_state_mark_collides(),
which walks the SPI hash bucket for the target tuple and excludes the
state being migrated by pointer, rather than doing a separate
exact-match lookup followed by a wildcard lookup and comparing the
result against self.

Before:
root@west:~# ip xfrm state add src 10.1.1.1 dst 10.1.1.2 proto esp \
        spi 0x1000  reqid 100 mode tunnel aead "rfc4106(gcm(aes))" \
        0x1111111111111111111111111111111111111111 96 \
        mark 0x1 mask 0xff

root@west:~# ip xfrm state add src 10.1.1.1 dst 10.1.1.2 proto esp \
        spi 0x1000  reqid 100 mode tunnel aead "rfc4106(gcm(aes))" \
        0x1111111111111111111111111111111111111111 96 \
        mark 0x2 mask 0xff
root@west:~# ip xfrm state migrate dst 10.1.1.2 proto esp spi 0x1000 \
        mark 0x1 mask 0xff \
        new-dst 10.1.1.2 new-src 10.1.1.1 new-reqid 100 \
        new-mark 0x2 mask 0xff
ip x s
src 10.1.1.1 dst 10.1.1.2
    proto esp spi 0x00001000 reqid 100 mode tunnel
    replay-window 0
    mark 0x2/0xff
    aead rfc4106(gcm(aes)) 0x1111111111111111111111111111111111111111 96
    anti-replay context: seq 0x0, oseq 0x0, bitmap 0x00000000
src 10.1.1.1 dst 10.1.1.2
    proto esp spi 0x00001000 reqid 100 mode tunnel
    replay-window 0
    mark 0x2/0xff
    aead rfc4106(gcm(aes)) 0x1111111111111111111111111111111111111111 96
    anti-replay context: seq 0x0, oseq 0x0, bitmap 0x00000000
    sel src 0.0.0.0/0 dst 0.0.0.0/0

Notice two states with same mark 0x2/0xff.
After:

Error: New SA tuple already occupied.

Fixes: a9d155ea9b44 ("xfrm: add XFRM_MSG_MIGRATE_STATE for single SA migration")
Reported-by: Sashiko <sashiko-bot@kernel.org>
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/net/xfrm.h    |  4 ++++
 net/xfrm/xfrm_state.c | 40 +++++++++++++++++++++++++++++++++++++---
 net/xfrm/xfrm_user.c  | 17 ++++++++---------
 3 files changed, 49 insertions(+), 12 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 9e053388aa4b..26dd4b570588 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -1751,6 +1751,10 @@ struct xfrm_state *xfrm_state_lookup_byaddr(struct net *net, u32 mark,
 struct xfrm_state *xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
 					   const xfrm_address_t *daddr, __be32 spi,
 					   u8 proto, unsigned short family);
+bool xfrm_state_mark_collides(struct net *net, u32 mark,
+			      const xfrm_address_t *daddr, __be32 spi,
+			      u8 proto, unsigned short family,
+			      const struct xfrm_state *self);
 #ifdef CONFIG_XFRM_SUB_POLICY
 void xfrm_tmpl_sort(struct xfrm_tmpl **dst, struct xfrm_tmpl **src, int n,
 		    unsigned short family);
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index f1e04df8d501..65663c7ba836 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2202,10 +2202,12 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
 			       struct netlink_ext_ack *extack)
 {
 	if (m->new_family == m->old_family &&
-	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
+	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) &&
+	    xc->mark.v == x->mark.v && xc->mark.m == x->mark.m) {
 		/*
-		 * Care is needed when the destination address of the state is
-		 * to be updated as it is a part of triplet.
+		 * Care is needed when the destination address or mark of the
+		 * state is to be updated, as they are part of the lookup
+		 * triplet.
 		 */
 		xfrm_state_insert(xc);
 	} else {
@@ -2440,6 +2442,38 @@ xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
 }
 EXPORT_SYMBOL(xfrm_state_lookup_exact);
 
+/* True if some OTHER state at this tuple would wildcard-match "mark".
+ * Used by MIGRATE_STATE, which must exclude the state being migrated.
+ */
+bool xfrm_state_mark_collides(struct net *net, u32 mark,
+			      const xfrm_address_t *daddr, __be32 spi,
+			      u8 proto, unsigned short family,
+			      const struct xfrm_state *self)
+{
+	struct xfrm_hash_state_ptrs state_ptrs;
+	unsigned int h;
+	struct xfrm_state *x;
+	bool collides = false;
+
+	rcu_read_lock();
+	xfrm_hash_ptrs_get(net, &state_ptrs);
+	h = __xfrm_spi_hash(daddr, spi, proto, family, state_ptrs.hmask);
+
+	hlist_for_each_entry_rcu(x, state_ptrs.byspi + h, byspi) {
+		if (x != self && x->props.family == family &&
+		    x->id.spi == spi && x->id.proto == proto &&
+		    xfrm_addr_equal(&x->id.daddr, daddr, family) &&
+		    (mark & x->mark.m) == x->mark.v) {
+			collides = true;
+			break;
+		}
+	}
+	rcu_read_unlock();
+
+	return collides;
+}
+EXPORT_SYMBOL(xfrm_state_mark_collides);
+
 struct xfrm_state *
 xfrm_find_acq(struct net *net, const struct xfrm_mark *mark, u8 mode, u32 reqid,
 	      u32 if_id, u32 pcpu_num, u8 proto, const xfrm_address_t *daddr,
diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 4de8c401eb44..a5fbe38839c4 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -3476,15 +3476,14 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
 						       x->nat_keepalive_interval);
 
 	if (m.new_family != um->id.family ||
-	    !xfrm_addr_equal(&m.new_daddr, &um->id.daddr, um->id.family)) {
-		u32 new_mark_key = m.new_mark ? m.new_mark->v & m.new_mark->m :
-						m.old_mark.v & m.old_mark.m;
-		struct xfrm_state *x_new;
-
-		x_new = xfrm_state_lookup(net, new_mark_key, &m.new_daddr,
-					  um->id.spi, um->id.proto, m.new_family);
-		if (x_new) {
-			xfrm_state_put(x_new);
+	    !xfrm_addr_equal(&m.new_daddr, &um->id.daddr, um->id.family) ||
+	    (m.new_mark && (m.new_mark->v != x->mark.v ||
+			   m.new_mark->m != x->mark.m))) {
+		const struct xfrm_mark *new_mark = m.new_mark ? m.new_mark : &x->mark;
+
+		if (xfrm_state_mark_collides(net, new_mark->v & new_mark->m,
+					     &m.new_daddr, um->id.spi,
+					     um->id.proto, m.new_family, x)) {
 			NL_SET_ERR_MSG(extack, "New SA tuple already occupied");
 			err = -EEXIST;
 			goto out;
-- 
2.53.0

[PATCH 12/14] xfrm: pass extack through to xfrm_init_replay() from xfrm_init_state()

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

xfrm_init_state() takes an extack parameter but doesn't pass it to
xfrm_init_replay(), so replay-window errors can't be reported via
extack.

Fixes: 231a1744dc43 ("xfrm: add extack to xfrm_init_state")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/xfrm/xfrm_state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 65663c7ba836..0555e5d796ed 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -3384,7 +3384,7 @@ int xfrm_init_state(struct xfrm_state *x, struct netlink_ext_ack *extack)
 	if (err)
 		return err;
 
-	err = xfrm_init_replay(x, NULL);
+	err = xfrm_init_replay(x, extack);
 	if (err)
 		return err;
 
-- 
2.53.0

[PATCH 13/14] docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

[Steffen Klassert]

From: Antony Antony <antony.antony@secunet.com>

Document mark as part of the EEXIST tuple and update the SA lookup
description to match.

Fixes: c13c0cc6f52e ("xfrm: add documentation for XFRM_MSG_MIGRATE_STATE")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 .../networking/xfrm/xfrm_migrate_state.rst    | 25 +++++++++++++------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/Documentation/networking/xfrm/xfrm_migrate_state.rst b/Documentation/networking/xfrm/xfrm_migrate_state.rst
index 9d53cb22b007..ca2c9e5aaf33 100644
--- a/Documentation/networking/xfrm/xfrm_migrate_state.rst
+++ b/Documentation/networking/xfrm/xfrm_migrate_state.rst
@@ -27,15 +27,20 @@ SA Identification
 =================
 
 The struct is defined in ``include/uapi/linux/xfrm.h``. The SA is looked
-up using ``xfrm_state_lookup()`` with ``id.spi``,
-``id.daddr``, ``id.proto``, ``id.family``, and
-``old_mark.v & old_mark.m`` as the mark key::
+up using ``xfrm_state_lookup_exact()`` with ``id.spi``, ``id.daddr``,
+``id.proto``, ``id.family``, and an exact match against ``old_mark.v``
+and ``old_mark.m``. Unlike the data path, which uses a masked
+comparison, this requires the SA's mark and mask to equal ``old_mark``
+exactly, so a broad-mask SA is never matched when a more specific one
+was intended. If no such SA exists, ``-ESRCH`` is returned.
+
+The layout is::
 
     struct xfrm_user_migrate_state {
         struct xfrm_usersa_id  id;       /* spi, daddr, proto, family */
         xfrm_address_t         new_daddr;
         xfrm_address_t         new_saddr;
-        struct xfrm_mark       old_mark; /* SA lookup: key = v & m */
+        struct xfrm_mark       old_mark; /* SA lookup key (exact v/m match) */
         struct xfrm_selector   new_sel;  /* new selector (see Flags) */
         __u32                  new_reqid;
         __u32                  flags;    /* XFRM_MIGRATE_STATE_* */
@@ -72,8 +77,8 @@ inherits the value from the existing SA (omit-to-inherit).
      - Description
    * - ``XFRMA_MARK``
      - Mark on the migrated SA (``struct xfrm_mark``). Absent inherits
-       ``old_mark``. To use no mark on the new SA, send ``XFRMA_MARK``
-       with ``{0, 0}``.
+       the mark of the existing SA. To use no mark on the new SA, send
+       ``XFRMA_MARK`` with ``{0, 0}``.
    * - ``XFRMA_ENCAP``
      - UDP encapsulation template; only ``UDP_ENCAP_ESPINUDP`` is supported.
        Set ``encap_type=0`` to remove encap.
@@ -259,8 +264,12 @@ Attributes in the notification
 Error Handling
 ==============
 
-If the target SA tuple (new daddr, SPI, proto, new family) is already
-occupied, the operation returns ``-EEXIST`` before the migration begins.
+If the target SA tuple (new daddr, SPI, proto, new family, mark) is
+already occupied, the operation returns ``-EEXIST`` before the migration
+begins. "Occupied" includes wildcard shadowing: an existing SA with a
+broader mask (e.g. mark 0/0) claims every mark value, so it blocks
+migrating to any more specific mark at the same tuple, not just an
+exact mark/mask duplicate.
 The old SA remains intact and the operation is safe to retry after
 resolving the conflict.
 
-- 
2.53.0

[PATCH 14/14] xfrm4: use a full socket for local errors

[Steffen Klassert]

From: Fourie Zhang <fouriezhang@tencent.com>

xfrm4_local_error() passes skb->sk to inet_sk() and ip_local_error()
without ensuring it points to a full socket. TCP SYN-ACK skbs can be owned
by TCP_NEW_SYN_RECV request sockets.

xfrm4_tunnel_check_size() already checks sk_fullsock() since commit
1620c88887b1 ("xfrm: Fix the usage of skb->sk"), so iptfs_is_too_big() is
the only remaining IPv4 caller passing a request socket here: the sk it
tests is the listener, while skb->sk is the request socket.

When IP-TFS rejects such an skb because it exceeds its payload MTU, the
local error path treats the request socket as a struct inet_sock. KASAN
reports a slab-out-of-bounds read in ip_local_error() beyond the
request_sock_TCP allocation.

Resolve the skb owner with skb_to_full_sk() before accessing inet_sock
state or reporting the error. This maps a request socket to its listener
and rejects sockets that cannot be used as full sockets, matching what
commit 6973a21ee73c ("ipv6: xfrm: use full sockets in local error paths")
did for xfrm6_local_error().

Fixes: 0e4fbf013fa5 ("xfrm: iptfs: add user packet (tunnel ingress) handling")
Cc: stable@vger.kernel.org
Reported-by: TencentOS Corvus AI <corvus@tencent.com>
Link: https://lore.kernel.org/netdev/20260908224856.1591189-1-kuba@kernel.org/
Reviewed-by: Eric Dumazet <edumazet@kernel.org>
Assisted-by: tencentos-corvus-ai:hy4-preview
Signed-off-by: Fourie Zhang <fouriezhang@tencent.com>
Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 net/ipv4/xfrm4_output.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
index 0ae67d537499..fffa3c4f456e 100644
--- a/net/ipv4/xfrm4_output.c
+++ b/net/ipv4/xfrm4_output.c
@@ -38,9 +38,12 @@ int xfrm4_output(struct net *net, struct sock *sk, struct sk_buff *skb)
 
 void xfrm4_local_error(struct sk_buff *skb, u32 mtu)
 {
+	struct sock *sk = skb_to_full_sk(skb);
 	struct iphdr *hdr;
 
+	if (!sk)
+		return;
+
 	hdr = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
-	ip_local_error(skb->sk, EMSGSIZE, hdr->daddr,
-		       inet_sk(skb->sk)->inet_dport, mtu);
+	ip_local_error(sk, EMSGSIZE, hdr->daddr, inet_sk(sk)->inet_dport, mtu);
 }
-- 
2.53.0

Re: [PATCH 02/14] xfrm: interface: validate the IP header on xmit

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] In xfrmi_xmit() (net/xfrm/xfrm_interface_core.c), the new
  header checks look only at skb->protocol.

--

xfrm: interface: validate the IP header on xmit

This patch adds a pskb_inet_may_pull() call to xfrmi_xmit(). It also
drops packets whose IP version does not match skb->protocol, and IPv4
packets with a malformed ihl. The aim is to stop xfrm4_transport_output()
from working on an unvalidated header when packets are injected into an
xfrmi device.

> diff --git a/net/xfrm/xfrm_interface_core.c b/net/xfrm/xfrm_interface_core.c
> index 688306bf62c5a..9835fbe63ccb8 100644
> --- a/net/xfrm/xfrm_interface_core.c
> +++ b/net/xfrm/xfrm_interface_core.c
> @@ -531,8 +531,13 @@ static netdev_tx_t xfrmi_xmit(struct sk_buff *skb, struct net_device *dev)
>  
>  	memset(&fl, 0, sizeof(fl));
>  
> +	if (!pskb_inet_may_pull(skb))
> +		goto tx_err;
> +
>  	switch (skb->protocol) {
>  	case htons(ETH_P_IPV6):
> +		if (ipv6_hdr(skb)->version != 6)
> +			goto tx_err;
>  		memset(IP6CB(skb), 0, sizeof(*IP6CB(skb)));
>  		xfrm_decode_session(dev_net(dev), skb, &fl, AF_INET6);
>  		if (!dst) {

[Severity: Medium]
Can an skb marked ETH_P_IPV6 pass this check and still reach
xfrm4_transport_output()?

The new check in xfrmi_xmit() looks only at skb->protocol. The transform
that actually runs is picked by the dst family, and then by
x->props.family.

When skb_dst(skb) is already set, xfrmi_xmit() skips the route lookup.
The dst family is never compared with the header that was just checked.

In collect_md mode, xfrmi_xmit2() also swaps in a different dst after
this check:

net/xfrm/xfrm_interface_core.c:xfrmi_xmit2() {
    ...
		if (md_info->dst_orig) {
			struct dst_entry *tmp_dst = dst;

			dst = md_info->dst_orig;
			skb_dst_set(skb, dst);
    ...
}

xfrm_lookup_with_ifid() then takes the policy/state family from that
dst, not from the packet:

net/xfrm/xfrm_policy.c:xfrm_lookup_with_ifid() {
    ...
	u16 family = dst_orig->ops->family;
    ...
}

A tc BPF program could reach this with the following sequence:

bpf_skb_set_xfrm_info()
    info->dst_orig = skb_dst(skb);    /* IPv4 rtable */
    attaches the per-cpu metadata dst
bpf_skb_change_proto(skb, ETH_P_IPV6)
    skb_valid_dst() is false for DST_METADATA, so the metadata dst
    and the IPv4 dst_orig are both kept
program writes an IPv6 header with version 6
xfrmi_xmit()                  /* dst != NULL, only version check runs */
  xfrmi_xmit2()               /* collect_md swaps in IPv4 dst_orig */
    xfrm_lookup_with_ifid()   /* family is AF_INET */
    ...
    xfrm_outer_mode_output()
      xfrm4_transport_output()

A plain IPv4 rtable does not survive bpf_skb_change_proto(), because it
drops non-metadata dsts. So this appears limited to the metadata dst
ordering above.

With an AF_INET transport mode state, xfrm4_transport_output() takes ihl
from the low nibble of the IPv6 header's first byte. That nibble is the
top four bits of the traffic class, so it can be anything from 0 to 15:

net/xfrm/xfrm_output.c:xfrm4_transport_output() {
	struct iphdr *iph = ip_hdr(skb);
	int ihl = iph->ihl * 4;
    ...
	__skb_pull(skb, ihl);
	memmove(skb_network_header(skb), iph, ihl);
    ...
}

pskb_inet_may_pull() only made 40 bytes linear for IPv6. For a small
packet, skb->len can wrap and the memmove() can read past the end of the
data. If the skb has paged data and a linear length under 60 bytes,
__skb_pull() hits BUG() instead.

One step has not been confirmed end to end. It is not certain that an
AF_INET policy/state lookup matches a flowi that xfrm_decode_session()
filled in as IPv6. It looks plausible with a 0/0 selector, because the
ip4 address fields overlay ip6 address bytes that the sender controls.

This path also needs CAP_BPF/CAP_NET_ADMIN, a collect_md xfrmi, and an
AF_INET transport mode SA and policy.

The commit message says:

  "xfrm4_transport_output() uses iph->ihl for __skb_pull() and memmove()
  and expects the header to be valid."

Would it make sense to also drop packets whose final dst family (after
the collect_md dst_orig swap) does not match skb->protocol, so this path
is covered too?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 04/14] xfrm: espintcp: reorder operations within espintcp_init_sk

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch annotates only part of the concurrent access to
  sk->sk_prot.

Pre-existing issues:
- [Critical] esp_find_tcp_sk() (net/ipv4/esp4.c:138-145) and
  esp6_find_tcp_sk() (net/ipv6/esp6.c:155-162) drop the lookup reference…
- [Medium] espintcp_close() (net/xfrm/espintcp.c:536) always restores
  `sk->sk_prot = &tcp_prot`, even for AF_INET6 sockets.
- [Medium] On the AF_INET6 path, the first espintcp_init_sk() call after
  boot fills the static espintcp6_prot/espintcp6_ops with…

--

xfrm: espintcp: reorder operations within espintcp_init_sk

This commit reorders espintcp_init_sk() so the espintcp_ctx is fully
set up and published as icsk_ulp_data before the socket callbacks and
proto/ops are switched to the espintcp versions. An smp_wmb()/smp_rmb()
pair is added between espintcp_init_sk() and espintcp_getctx(), along
with some READ_ONCE/WRITE_ONCE annotations on sk_prot, sk_socket->ops
and the callbacks.

Link: https://lore.kernel.org/all/c30b645074a1b379e0f7fe297f917c66137d9964.1778464688.git.zylzyl2333@gmail.com/
Link: https://lore.kernel.org/all/20260819155349.3555804-1-jjy600901@snu.ac.kr

> diff --git a/net/xfrm/espintcp.c b/net/xfrm/espintcp.c
> index 3e72b9f067b9b..4052ae5bbb2ea 100644
> --- a/net/xfrm/espintcp.c
> +++ b/net/xfrm/espintcp.c
> @@ -434,7 +434,9 @@ static void espintcp_destruct(struct sock *sk)
>  
>  bool tcp_is_ulp_esp(struct sock *sk)
>  {
> -	return sk->sk_prot == &espintcp_prot || sk->sk_prot == &espintcp6_prot;
> +	const struct proto *prot = READ_ONCE(sk->sk_prot);
> +
> +	return prot == &espintcp_prot || prot == &espintcp6_prot;
>  }

[Severity: Low]

tcp_is_ulp_esp() now reads sk->sk_prot with READ_ONCE(), and
espintcp_init_sk() installs it with WRITE_ONCE(). Should the restore in
espintcp_close() get the same treatment?

net/xfrm/espintcp.c:espintcp_close() {
	...
	strp_stop(&ctx->strp);

	sk->sk_prot = &tcp_prot;

	synchronize_rcu();
	...
}

This plain store is the one that actually runs at the same time as the
lockless reader. espintcp_close() is reached through
inet_release()->sk->sk_prot->close() without lock_sock. esp_find_tcp_sk()
and esp6_find_tcp_sk() call tcp_is_ulp_esp() holding only
rcu_read_lock(). The synchronize_rcu() right after the store exists
because of those readers.

The TLS ULP marks its restore path with WRITE_ONCE(sk->sk_prot,
ctx->sk_proto) in net/tls/tls_main.c. Would WRITE_ONCE(sk->sk_prot,
&tcp_prot) in espintcp_close() complete the annotation?

[Severity: Critical]

This is a pre-existing issue, but since this patch touches
tcp_is_ulp_esp(): can its callers call sock_put() on a minisocket?

net/ipv4/esp4.c:esp_find_tcp_sk() {
	...
	sk = inet_lookup_established(net, x->id.daddr.a4, dport,
				     x->props.saddr.a4, sport, 0);
	if (!sk)
		return ERR_PTR(-ENOENT);

	if (!tcp_is_ulp_esp(sk)) {
		sock_put(sk);
		return ERR_PTR(-EINVAL);
	}
	...
}

esp6_find_tcp_sk() in net/ipv6/esp6.c follows the same pattern.

__inet_lookup_established() walks the ehash chain, which also contains
TIME_WAIT and NEW_SYN_RECV sockets, and takes a reference with
refcount_inc_not_zero(). Its own mismatch path drops that reference
with sock_gen_put().

A timewait socket's tw_prot is sk_prot_creator (tcp_prot or
tcpv6_prot), so tcp_is_ulp_esp() always returns false for it and the
sock_put() path runs.

Here is one possible sequence. The espintcp socket was actively closed,
so its 4-tuple is in TIME_WAIT with tw_refcnt == 3:

CPU1 (ESP output)
esp_output_tcp_finish()
  esp_find_tcp_sk()
    __inet_lookup_established()
      refcount_inc_not_zero()        /* 3 -> 4 */

CPU2 (tw timer)
tw_timer_handler()
  inet_twsk_kill()
    sk_nulls_del_node_init_rcu()     /* 4 -> 3 */
    inet_twsk_bind_unhash()          /* 3 -> 2 */
    inet_twsk_put()                  /* 2 -> 1 */

CPU1
    tcp_is_ulp_esp(tw) returns false
    sock_put()                       /* 1 -> 0, sk_free(tw) */

sk_free() then decrements sk_wmem_alloc and may run __sk_free() on
memory that is not a full struct sock. inet_twsk_free() is skipped.

A NEW_SYN_RECV request socket on the same 4-tuple, which a remote SYN
can create, would hit the same wrong-destructor path.

Should esp_find_tcp_sk() and esp6_find_tcp_sk() use sock_gen_put()
here instead?

>  EXPORT_SYMBOL_GPL(tcp_is_ulp_esp);
>  
> @@ -466,34 +468,41 @@ static int espintcp_init_sk(struct sock *sk)

[ ... ]

> +	rcu_assign_pointer(icsk->icsk_ulp_data, ctx);
> +
> +	/* pairs with smp_rmb() in espintcp_getctx() */
> +	smp_wmb();
> +
> +	WRITE_ONCE(sk->sk_data_ready, espintcp_data_ready);
> +	WRITE_ONCE(sk->sk_write_space, espintcp_write_space);
> +	sk->sk_destruct = espintcp_destruct;
>  
>  	if (sk->sk_family == AF_INET) {
> -		sk->sk_prot = &espintcp_prot;
> -		sk->sk_socket->ops = &espintcp_ops;
> +		WRITE_ONCE(sk->sk_prot, &espintcp_prot);
> +		WRITE_ONCE(sk->sk_socket->ops, &espintcp_ops);
>  	} else {
>  		mutex_lock(&tcpv6_prot_mutex);
>  		if (!espintcp6_prot.recvmsg)
>  			build_protos(&espintcp6_prot, &espintcp6_ops, sk->sk_prot, sk->sk_socket->ops);
>  		mutex_unlock(&tcpv6_prot_mutex);
>  
> -		sk->sk_prot = &espintcp6_prot;
> -		sk->sk_socket->ops = &espintcp6_ops;
> +		WRITE_ONCE(sk->sk_prot, &espintcp6_prot);
> +		WRITE_ONCE(sk->sk_socket->ops, &espintcp6_ops);
>  	}

[Severity: Medium]

This is a pre-existing issue, but the new smp_wmb() comes before the
AF_INET6 branch. On the first IPv6 espintcp conversion after boot,
build_protos() fills espintcp6_prot and espintcp6_ops using memcpy()
and plain stores. The two WRITE_ONCE() calls above then publish them.

Only mutex_unlock() sits between the two sets of stores. It is a
release, so it doesn't order the later publication stores after the
earlier build_protos() stores.

Can another thread on the same socket see the new pointer before the
struct contents are visible?

sock_recvmsg_nosec() calls READ_ONCE(sock->ops)->recvmsg. inet6_recvmsg()
and inet6_sendmsg() call prot->recvmsg/sendmsg after
READ_ONCE(sk->sk_prot). Neither takes lock_sock. If one of them sees a
still-zero ->recvmsg or ->sendmsg, it calls a NULL function pointer.

Would moving the smp_wmb() after the AF_INET6 build block, or
publishing with smp_store_release(), order both the ctx and the proto
contents?

[Severity: Medium]

This isn't a bug introduced by this patch, but for AF_INET6 sockets
this branch installs espintcp6_prot, which is copied from tcpv6_prot.
espintcp_close() always restores the IPv4 proto:

net/xfrm/espintcp.c:espintcp_close() {
	...
	sk->sk_prot = &tcp_prot;
	...
}

struct espintcp_ctx doesn't save the original proto, so close cannot
restore the per-family one. Does an IPv6 espintcp socket then run
tcp_close() with tcp_prot?

inet_unhash() and inet_twsk_hashdance_schedule() both call:

	sock_prot_inuse_add(sock_net(sk), sk->sk_prot, -1);

The socket was counted under tcpv6_prot's inuse_idx, which
espintcp6_prot copies, but it gets decremented on tcp_prot's. "TCP:
inuse" in /proc/net/sockstat would be undercounted and "TCP6: inuse"
would leak.

Could the ctx save the original sk_prot, as tls_context->sk_proto
does, and restore it in espintcp_close()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 05/14] xfrm: state: reject mark with bits outside its mask on add

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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 is a regression at this commit only, and a later patch in
  the series fixes it.
- [Low] At this commit, XFRM_MSG_ALLOCSPI still accepts XFRMA_MARK
  without calling verify_mark().

--

xfrm: state: reject mark with bits outside its mask on add

This commit adds verify_mark(), which rejects an XFRMA_MARK whose value
has bits outside its mask. It is called from verify_newsa_info() and
xfrm_do_migrate_state(). The commit also removes the mark sanitization
from __xfrm_state_insert(), so such marks are rejected instead of being
silently truncated.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index e45aa1ed5b965..602404ffc4d4b 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1732,9 +1732,6 @@ static void __xfrm_state_insert(struct xfrm_state *x)
>  
>  	list_add(&x->km.all, &net->xfrm.state_all);
>  
> -	/* Sanitize mark before store */
> -	x->mark.v &= x->mark.m;
> -

[Severity: Low]
At this commit, can a mark with bits outside its mask still get into the
SAD through XFRM_MSG_ALLOCSPI?

xfrm_alloc_userspi() does not call verify_mark(). xfrm_mark_get() masks
only its return value, and m keeps the raw attribute:

net/xfrm/xfrm_user.c:xfrm_alloc_userspi() {
    ...
	mark = xfrm_mark_get(attrs, &m);
    ...
}

xfrm_find_acq() -> __find_acq_core() then stores the mark unchanged and
adds the larval state to the hash tables directly:

net/xfrm/xfrm_state.c:__find_acq_core() {
    ...
		x->mark.v = m->v;
		x->mark.m = m->m;
    ...
}

So ALLOCSPI with XFRMA_MARK {0x101, 0xff} creates a larval SA that no
masked lookup can match. That part is older than this patch, because
__find_acq_core() never went through __xfrm_state_insert().

The legacy XFRM_MSG_MIGRATE path is newly affected.
xfrm_migrate_copy_old() copies the existing SA's mark into the clone:

net/xfrm/xfrm_policy.c:xfrm_migrate_copy_old() {
    ...
	mp->new_mark               = &x->mark;
}

Before this patch, __xfrm_state_insert() trimmed the clone's mark. With
the sanitization removed, the clone keeps 0x101/0xff.

The next commit in the series, "xfrm: state: reject mark with bits
outside its mask on ALLOCSPI", adds a verify_mark() call to
xfrm_alloc_userspi() before xfrm_mark_get(), which closes this. Could
that change be folded into this patch, or moved ahead of it?

[ ... ]

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index a2587c7e796b4..0723d791b8b70 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c

[ ... ]

> @@ -3370,6 +3390,10 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>  			return err;
>  	}
>  
> +	err = verify_mark(attrs, extack);
> +	if (err)
> +		return err;
> +
>  	copy_from_user_migrate_state(&m, um);
>  
>  	x = xfrm_state_lookup(net, m.old_mark.v & m.old_mark.m,

[Severity: Medium]
verify_mark() checks only attrs[XFRMA_MARK] and returns 0 when that
attribute is absent. Is um->old_mark left unchecked here?

copy_from_user_migrate_state() copies it unchanged:

	ma->old_mark = um->old_mark;

The old SA is looked up with the masked value. So old_mark
{v=0x101, m=0xff} still finds an existing 0x01/0xff SA.

If XFRMA_MARK is not supplied, xfrm_state_clone_and_setup() gives the
clone the raw old_mark:

net/xfrm/xfrm_state.c:xfrm_state_clone_and_setup() {
    ...
	x->mark = m->new_mark ? *m->new_mark : m->old_mark;
    ...
}

Before this patch, __xfrm_state_insert() trimmed that to 0x01/0xff. With
the sanitization removed, the path
xfrm_state_migrate_install() -> xfrm_state_insert() stores the clone as
0x101/0xff.

The masked lookups test (mark & x->mark.m) == x->mark.v, which can never
be true for that mark. Does that leave the original SA deleted and the
migrated SA unreachable?

The later commit "xfrm: state: exact mark/mask match for SPI-keyed
control-plane SA lookups" changes this to
xfrm_state_lookup_exact(net, &m.old_mark, ...). After that change, an
old_mark with bits outside its mask matches no stored SA, and the
request fails with -ESRCH.

That limits the problem to the commits between this one and that one.
Even so, could um->old_mark be checked here too, or could the exact
lookup patch be placed earlier in the series?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 07/14] xfrm: policy: reject mark with bits outside its mask on add

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · 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).

- [Medium] This patch makes XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY
  reject input that mainline currently accepts and that works.

--

xfrm: policy: reject mark with bits outside its mask on add

This patch adds a verify_mark() check to xfrm_add_policy(). The check
rejects XFRMA_MARK values that have bits set outside the mask. It also
removes the mark sanitization from xfrm_policy_insert(), which commit
0b91fda3a1f0 ("xfrm: Sanitize marks before insert") had added.

> diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
> index cc206017d0f50..cea6842e6454d 100644
> --- a/net/xfrm/xfrm_policy.c
> +++ b/net/xfrm/xfrm_policy.c
> @@ -1575,9 +1575,6 @@ int xfrm_policy_insert(int dir, struct xfrm_policy *policy, int excl)
>  	struct xfrm_policy *delpol;
>  	struct hlist_head *chain;
>  
> -	/* Sanitize mark before store */
> -	policy->mark.v &= policy->mark.m;
> -
>  	spin_lock_bh(&net->xfrm.xfrm_policy_lock);
>  	chain = policy_hash_bysel(net, &policy->selector, policy->family, dir);
>  	if (chain)
> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 6b53373168b02..c1571c7a23153 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -2307,6 +2307,9 @@ static int xfrm_add_policy(struct sk_buff *skb, struct nlmsghdr *nlh,
>  	if (err)
>  		return err;
>  	err = verify_sec_ctx_len(attrs, extack);
> +	if (err)
> +		return err;
> +	err = verify_mark(attrs, extack);
>  	if (err)
>  		return err;

[Severity: Medium]
Will this reject XFRM_MSG_NEWPOLICY and XFRM_MSG_UPDPOLICY requests that
current mainline accepts, and that work?

Since 0b91fda3a1f0, a mark such as 0xf42/0xf00 is stored by
xfrm_policy_insert() as 0xf00/0xf00. The policy then matches traffic in
xfrm_policy_match():

net/xfrm/xfrm_policy.c:xfrm_policy_match() {
    ...
	    (fl->flowi_mark & pol->mark.m) != pol->mark.v ||
    ...
}

With this patch, the same request fails in xfrm_add_policy()->verify_mark()
with -EINVAL and "Invalid mark value/mask combination". A userspace IPsec
config that loads and passes traffic on current kernels would no longer
load.

The commit message only says "Reject it instead". It doesn't mention
that this input has worked since 0b91fda3a1f0, or why breaking it in
mainline is acceptable.

The Cc: <stable+noautosel@kernel.org> # avoid breaking existing userspace
ABI trailer suggests this is known to be an ABI change. However, keeping
the patch out of stable doesn't prevent the change in mainline.

Could the lookup failure be fixed without rejecting any input? The GET
and DEL failures seem to come from the exact compare in
xfrm_policy_mark_match():

net/xfrm/xfrm_policy.c:xfrm_policy_mark_match() {
	return mark->v == pol->mark.v && mark->m == pol->mark.m;
}

Applying the same v &= m to the user-supplied lookup mark, for example
in xfrm_mark_get(), would let existing inserts keep working.

Also, the commit message says the policy can't be found "by id". Doesn't
the same compare affect selector lookups through xfrm_policy_bysel_ctx()
as well as xfrm_policy_byid()?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 06/14] xfrm: state: reject mark with bits outside its mask on ALLOCSPI

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] XFRM_MSG_ACQUIRE (xfrm_add_acquire in net/xfrm/xfrm_user.c) still
  reads XFRMA_MARK with xfrm_mark_get(attrs, &mark) and never calls…

--

xfrm: state: reject mark with bits outside its mask on ALLOCSPI

This patch adds a verify_mark() call to xfrm_alloc_userspi(), so that
XFRM_MSG_ALLOCSPI rejects an XFRMA_MARK whose value has bits outside
its mask. This matches the state add and policy add checks added
earlier in the series.

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 0723d791b8b70..6b53373168b02 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1916,6 +1916,9 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	x = NULL;
>  
> +	err = verify_mark(attrs, extack);
> +	if (err)
> +		goto out_noput;

[Severity: Low]
Should XFRM_MSG_ACQUIRE get the same check?

xfrm_add_acquire() in net/xfrm/xfrm_user.c still reads XFRMA_MARK
without verify_mark(), and this is still the case at the end of the
series:

net/xfrm/xfrm_user.c:xfrm_add_acquire() {
    ...
	xfrm_mark_get(attrs, &mark);
    ...
	xp->mark.m = x->mark.m = mark.m;
	xp->mark.v = x->mark.v = mark.v;
    ...
		err = km_query(x, t, xp);
    ...
}

The raw value/mask is then put into the ACQUIRE multicast unchanged by
build_acquire():

	if (!err)
		err = xfrm_mark_put(skb, &xp->mark);

Here is a possible sequence. A CAP_NET_ADMIN caller sends
XFRM_MSG_ACQUIRE with a mark such as 0x101/0xff. A key manager gets
the ACQUIRE and replies with XFRM_MSG_ALLOCSPI, echoing the same mark.
With this patch, that ALLOCSPI now fails:

xfrm_add_acquire()->km_query()->xfrm_send_acquire()->build_acquire()
  -> daemon sends XFRM_MSG_ALLOCSPI with 0x101/0xff
  -> xfrm_alloc_userspi()->verify_mark() returns -EINVAL

Before this patch, ALLOCSPI accepted that mark.

So the kernel would send a mark in one message that it then refuses in
the follow-up command. NEWSA/UPDSA, ALLOCSPI, NEWPOLICY/UPDPOLICY and
MIGRATE_STATE all reject such marks after this series. ACQUIRE seems to
be the only XFRMA_MARK consumer left without the check.

ACQUIREs that the kernel generates itself from xfrm_state_find() use
policy marks, and those are validated by the end of the series. Only
the privileged injection path through xfrm_add_acquire() is affected.

Would adding verify_mark(attrs, extack) to xfrm_add_acquire() keep this
consistent?

>  	mark = xfrm_mark_get(attrs, &m);
>  
>  	if (attrs[XFRMA_IF_ID])

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 08/14] xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 4 · 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] The patch changes netlink request semantics without saying so.
- [Medium] The commit says ALLOCSPI larval states may be unmarked (0/0)
  and keeps UPDSA on the wildcard match for that reason.
- [Medium] UPDSA (xfrm_state_update()) still finds its target through
  __xfrm_state_locate(), which calls the wildcard…
- [Medium] The commit says it wires exact matching into DELSA/GETSA, but
  xfrm_user_state_lookup() converts only the SPI branch.
- [Low] At this commit,
  Documentation/networking/xfrm/xfrm_migrate_state.rst still says…

--

xfrm: state: exact mark/mask match for SPI-keyed control-plane SA lookups

This adds xfrm_state_lookup_exact(), which matches an SA only when both
the mark value and the mask are identical. It is used for the SPI-keyed
lookups in DELSA/GETSA, GETAE/NEWAE, EXPIRE and MIGRATE_STATE. UPDSA
keeps the wildcard match so that unmarked ALLOCSPI larval states can
still be found.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 602404ffc4d4b..f1e04df8d5011 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -1177,11 +1177,19 @@ static struct xfrm_state *__xfrm_state_lookup_all(const struct xfrm_hash_state_p
>  	return NULL;
>  }
>
> -static struct xfrm_state *__xfrm_state_lookup(const struct xfrm_hash_state_ptrs *state_ptrs,
> -					      u32 mark,
> -					      const xfrm_address_t *daddr,
> -					      __be32 spi, u8 proto,
> -					      unsigned short family)
> +static bool xfrm_state_mark_matches(const struct xfrm_state *x, u32 mark, u32 mask, bool exact)
> +{
> +	if (exact)
> +		return x->mark.v == mark && x->mark.m == mask;

[Severity: Medium]
Does this change the netlink request semantics for existing userspace?

Before this patch, DELSA, GETSA, GETAE, NEWAE, EXPIRE and MIGRATE_STATE
matched an SA when ((m.v & m.m) & x->mark.m) == x->mark.v. The exact
branch now requires x->mark.v == m.v && x->mark.m == m.m. Requests that
used to work can now fail with -ESRCH, or -ENOENT for EXPIRE.

Two examples:

An SA is added with mark 0x10 mask 0xf0, followed by "ip xfrm state
get/delete ... mark 0x10". iproute2 sets mask 0xffffffff when none is
given. The old key was 0x10, and 0x10 & 0xf0 == 0x10, so this matched.
With the exact branch it does not.

An SA is added with mark 0 (0/0xffffffff), followed by DELSA/GETSA with
no XFRMA_MARK (0/0). That used to match and now fails.

The commit message lists the converted handlers. It does not say which
wrong-SA selection is being fixed, or that userspace must now pass the
SA's exact (v, m) pair.

The patch also carries a bare "Fixes: 3d6acfa7641f", so it qualifies for
automatic stable backport. Other ABI-changing patches in this series were
tagged "Cc: <stable+noautosel@kernel.org> # avoid breaking existing
userspace ABI".

Should this patch be tagged the same way? Should the commit message
describe the change that userspace will see?

> +	return (mark & x->mark.m) == x->mark.v;
> +}

[ ... ]

> @@ -1283,7 +1302,7 @@ static struct xfrm_state *__xfrm_state_lookup_byaddr(const struct xfrm_hash_stat
>  	return NULL;
>  }
>
> -static inline struct xfrm_state *
> +static struct xfrm_state *
>  __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
>  {
>  	struct xfrm_hash_state_ptrs state_ptrs;
> @@ -1293,7 +1312,7 @@ __xfrm_state_locate(struct xfrm_state *x, int use_spi, int family)
>  	xfrm_hash_ptrs_get(net, &state_ptrs);
>
>  	if (use_spi)
> -		return __xfrm_state_lookup(&state_ptrs, mark, &x->id.daddr,
> +		return __xfrm_state_lookup(&state_ptrs, mark, 0, false, &x->id.daddr,
>  					   x->id.spi, x->id.proto, family);

[Severity: Medium]
UPDSA still picks its target through this wildcard lookup. Can
xfrm_state_update() still update the wrong SA when the states are VALID?

Overlapping SAs can coexist. The duplicate check in xfrm_state_add()
also goes through __xfrm_state_locate(), so the result depends on
insertion order:

  NEWSA A {mark 5, mask 0xff}
  NEWSA B {mark 0, mask 0}, same daddr/spi/proto
    key is 0, (0 & 0xff) != 5, so the add succeeds

XFRM_STATE_INSERT puts newer software states ahead of older ones in the
byspi chain, so B sits before A. Then:

  UPDSA with XFRMA_MARK 5/0xff
    xfrm_add_sa()->xfrm_state_update()->__xfrm_state_locate()
      key is 5, B matches first since (5 & 0) == 0

In the VALID branch, xfrm_state_update() then overwrites B's encap,
coaddr, lifetimes, smark and if_id, and restarts B's timer. A, the SA
named in the request, stays unchanged.

The commit message gives unmarked ALLOCSPI larval states as the reason
to keep the wildcard here. That reason only covers XFRM_STATE_ACQ
states.

Would it work to try an exact match first, and fall back to the wildcard
only for XFRM_STATE_ACQ larval states?

[ ... ]

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index c1571c7a23153..5a9f729b4fcb0 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -1109,11 +1109,12 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
>  	struct xfrm_state *x = NULL;
>  	struct xfrm_mark m;
>  	int err;
> -	u32 mark = xfrm_mark_get(attrs, &m);
> +
> +	xfrm_mark_get(attrs, &m);
>
>  	if (xfrm_id_proto_match(p->proto, IPSEC_PROTO_ANY)) {
>  		err = -ESRCH;
> -		x = xfrm_state_lookup(net, mark, &p->daddr, p->spi, p->proto, p->family);
> +		x = xfrm_state_lookup_exact(net, &m, &p->daddr, p->spi, p->proto, p->family);

[Severity: Medium]
The commit message says ALLOCSPI larval states may be unmarked (0/0).
Can DELSA still find those larval states after this change?

xfrm_alloc_userspi() passes the request's mark to xfrm_find_acq(), and
__find_acq_core() copies it into the new ACQ state:

net/xfrm/xfrm_state.c:__find_acq_core() {
    ...
		x->mark.v = m->v;
		x->mark.m = m->m;
    ...
}

So an ALLOCSPI without XFRMA_MARK creates a 0/0 larval state that holds
the allocated SPI.

Suppose negotiation fails, or the CHILD_SA is torn down before UPDSA. An
IKE daemon then sends DELSA for that SPI with its configured inbound
mark, for example 5/0xffffffff. strongSwan behaves this way: its
ALLOCSPI carries no mark, and its child_sa destroy deletes my_spi with
mark_in.

Before this patch the key was 5. Since (5 & 0) == 0, the larval state
was found and deleted.

Now xfrm_del_sa()->xfrm_user_state_lookup()->xfrm_state_lookup_exact()
requires v == 5 && m == 0xffffffff, so DELSA returns -ESRCH. No delete
notification is sent. The larval state keeps the SPI reserved until its
acquire hard timer (sysctl_acq_expires) fires.

>  	} else {
>  		xfrm_address_t *saddr = NULL;
>
> @@ -1124,7 +1125,7 @@ static struct xfrm_state *xfrm_user_state_lookup(struct net *net,
>  		}
>
>  		err = -ESRCH;
> -		x = xfrm_state_lookup_byaddr(net, mark,
> +		x = xfrm_state_lookup_byaddr(net, m.v & m.m,
>  					     &p->daddr, saddr,
>  					     p->proto, p->family);

[Severity: Medium]
The commit message says DELSA/GETSA now use exact matching. This branch
still does a wildcard lookup and drops the request's mask. Should the
non-SPI path be converted as well?

This branch handles protocols where xfrm_id_proto_match(p->proto,
IPSEC_PROTO_ANY) is false, such as MIPv6 IPPROTO_ROUTING and
IPPROTO_DSTOPTS states. __xfrm_state_lookup_byaddr() still does:

		if ((mark & x->mark.m) != x->mark.v)
			continue;

Overlapping states can coexist here too:

  NEWSA A {mark 5, mask 0xff}
  NEWSA B {mark 0, mask 0}, same daddr/saddr/proto
    duplicate check key is 0, (0 & 0xff) != 5, so the add succeeds

XFRM_STATE_INSERT puts B ahead of A in the bysrc chain. Then:

  DELSA with XFRMA_MARK 5/0xff and XFRMA_SRCADDR
    xfrm_del_sa()->xfrm_user_state_lookup()->xfrm_state_lookup_byaddr()
      key is 5, B matches first since (5 & 0) == 0
    xfrm_state_delete(B)

As a result B is deleted, not the SA named in the request. GETSA returns
the wrong SA in the same way.

[ ... ]

> @@ -3402,9 +3403,9 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>
>  	copy_from_user_migrate_state(&m, um);
>
> -	x = xfrm_state_lookup(net, m.old_mark.v & m.old_mark.m,
> -			      &um->id.daddr, um->id.spi,
> -			      um->id.proto, um->id.family);
> +	x = xfrm_state_lookup_exact(net, &m.old_mark,
> +				    &um->id.daddr, um->id.spi,
> +				    um->id.proto, um->id.family);

[Severity: Low]
At this commit, Documentation/networking/xfrm/xfrm_migrate_state.rst
still describes the old lookup. It says the SA is found with
xfrm_state_lookup() using old_mark.v & old_mark.m as the mark key. The
struct comment there reads:

        struct xfrm_mark       old_mark; /* SA lookup: key = v & m */

xfrm_do_migrate_state() now needs an exact {v, m} match.

A later patch in this series, "docs: xfrm: include mark in
XFRM_MSG_MIGRATE_STATE EEXIST tuple", rewrites this section to describe
xfrm_state_lookup_exact(). It changes the comment to "/* SA lookup key
(exact v/m match) */", so the mismatch is gone by the end of the series.

Could the documentation update be folded into this patch, so that code
and docs agree at every commit?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 11/14] xfrm: include mark in MIGRATE_STATE SA collision check

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0 (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] MIGRATE_STATE checks for a collision with one key and installs
  with another, so for non-SPI protocols (e.g. IPPROTO_IPIP) the…
- [Medium] The new xfrm_state_mark_collides() checks for collisions
  without the state lock.

Pre-existing issues:
- [Low] Discarded: the issue predates this patch and a later patch in
  this series fixes it.

--

xfrm: include mark in MIGRATE_STATE SA collision check

This commit makes XFRM_MSG_MIGRATE_STATE take the SA mark into account
when it checks the new tuple for collisions. It adds
xfrm_state_mark_collides(), which walks the SPI hash bucket for the target
tuple and skips the state being migrated. It also makes
xfrm_state_migrate_install() use xfrm_state_add() instead of
xfrm_state_insert() when the mark changes.

> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index f1e04df8d5011..65663c7ba836a 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2202,10 +2202,12 @@ int xfrm_state_migrate_install(const struct xfrm_state *x,
>  			       struct netlink_ext_ack *extack)
>  {
>  	if (m->new_family == m->old_family &&
> -	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family)) {
> +	    xfrm_addr_equal(&x->id.daddr, &m->new_daddr, m->new_family) &&
> +	    xc->mark.v == x->mark.v && xc->mark.m == x->mark.m) {

[Severity: Medium]

With the added mark comparison, a migration that changes the mark but
keeps the same daddr now goes through xfrm_state_add() instead of
xfrm_state_insert(). For protocols that don't use the SPI for lookup, can
this fail after the original SA has already been deleted?

xfrm_do_migrate_state() doesn't restrict um->id.proto. The pre-check in
xfrm_state_mark_collides() always walks the byspi bucket for (new_daddr,
spi, proto, family).

For IPPROTO_IPIP, use_spi is false in xfrm_state_add(), so
__xfrm_state_locate() calls __xfrm_state_lookup_byaddr() with (daddr,
saddr, proto, mark).

IPIP states created by ipcomp_tunnel_create() have a non-zero SPI:

    t->id.spi = x->props.saddr.a4;

That puts them in byspi, so xfrm_state_lookup_exact() can find them. Take
these two states:

  state A: IPIP, src SA, dst D, spi SA, mark 1/0xff
  state B: IPIP, src SB, dst D, spi SB, mark 2/0xff

and migrate A with new_saddr=SB, new_daddr=D and XFRMA_MARK=2/0xff:

xfrm_do_migrate_state()
  xfrm_state_mark_collides()     /* only the (D, spi SA) bucket, misses B */
  xfrm_state_migrate_create()    /* ipip_init_state() accepts the clone */
  __xfrm_state_delete(x)         /* A is unhashed */
  xfrm_state_migrate_install()
    xfrm_state_add(xc)
      __xfrm_state_locate()      /* use_spi == 0 */
        __xfrm_state_lookup_byaddr()   /* finds B */
      returns -EEXIST

A is gone, the clone is dropped, and userspace gets -EEXIST. That case
contradicts the "Should not occur" comment in xfrm_do_migrate_state(). It
also contradicts the series documentation, which says the old SA stays
intact and the operation is safe to retry.

Should the pre-check use the same key that xfrm_state_add() uses for
non-SPI protocols? Alternatively, should MIGRATE_STATE reject protocols
that don't use the SPI for lookup?

>  		/*
> -		 * Care is needed when the destination address of the state is
> -		 * to be updated as it is a part of triplet.
> +		 * Care is needed when the destination address or mark of the
> +		 * state is to be updated, as they are part of the lookup
> +		 * triplet.
>  		 */
>  		xfrm_state_insert(xc);
>  	} else {
> @@ -2440,6 +2442,38 @@ xfrm_state_lookup_exact(struct net *net, const struct xfrm_mark *mark,
>  }
>  EXPORT_SYMBOL(xfrm_state_lookup_exact);
>  
> +/* True if some OTHER state at this tuple would wildcard-match "mark".
> + * Used by MIGRATE_STATE, which must exclude the state being migrated.
> + */
> +bool xfrm_state_mark_collides(struct net *net, u32 mark,
> +			      const xfrm_address_t *daddr, __be32 spi,
> +			      u8 proto, unsigned short family,
> +			      const struct xfrm_state *self)
> +{
> +	struct xfrm_hash_state_ptrs state_ptrs;
> +	unsigned int h;
> +	struct xfrm_state *x;
> +	bool collides = false;
> +
> +	rcu_read_lock();
> +	xfrm_hash_ptrs_get(net, &state_ptrs);
> +	h = __xfrm_spi_hash(daddr, spi, proto, family, state_ptrs.hmask);
> +
> +	hlist_for_each_entry_rcu(x, state_ptrs.byspi + h, byspi) {
> +		if (x != self && x->props.family == family &&
> +		    x->id.spi == spi && x->id.proto == proto &&
> +		    xfrm_addr_equal(&x->id.daddr, daddr, family) &&
> +		    (mark & x->mark.m) == x->mark.v) {
> +			collides = true;
> +			break;
> +		}
> +	}
> +	rcu_read_unlock();
> +
> +	return collides;

[Severity: Medium]

This walk runs under rcu_read_lock() only. The result is stale as soon as
the RCU lock is dropped, but xfrm_do_migrate_state() treats it as
authoritative. The check that actually decides the outcome, in
xfrm_state_add(), only runs after __xfrm_state_delete(x). Is that
ordering safe?

xfrm_hash_ptrs_get() takes a seqcount-protected snapshot of the table
pointers. The bucket walk itself is not covered by the seqcount, and
nothing re-checks the generation afterwards.

The comment in xfrm_do_migrate_state() relies on xfrm_cfg_mutex. At least
two byspi writers don't take that mutex.

The first is xfrm_hash_resize(). It runs from a work item and holds only
xfrm_state_lock and the hash generation seqcount. xfrm_hash_transfer()
relinks every node into the new tables, so a reader in the old bucket can
follow a rewritten next pointer and skip the colliding entry.

The second is xfrm_state_find() on the packet path. When the template has
a static SPI, it inserts ACQ states into byspi under xfrm_state_lock only:

    if (x->id.spi) {
        h = xfrm_spi_hash(net, &x->id.daddr, x->id.spi, x->id.proto, encap_family);
        XFRM_STATE_INSERT(byspi, &x->byspi, ...
    }

__xfrm_state_lookup() doesn't filter out ACQ states, so xfrm_state_add()
would hit one that was inserted after the pre-check.

If either race happens:

xfrm_do_migrate_state()
  xfrm_state_mark_collides()      /* returns false */
  xfrm_state_migrate_create()     /* allocations, xfrm_init_state(), offload */
  __xfrm_state_delete(x)
  xfrm_state_migrate_install()
    xfrm_state_add(xc)            /* returns -EEXIST */
  xfrm_state_put(xc)

both the original SA and the clone are lost.

Before this patch, mark-only migrations used xfrm_state_insert(), which
can't fail. They now reach this path too. The same lockless pre-check
followed by a destructive install already existed for daddr and family
changes.

Could the final collision check and the removal of x happen within the
same xfrm_state_lock section?

> +}
> +EXPORT_SYMBOL(xfrm_state_mark_collides);
> +

[ ... ]

> diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
> index 4de8c401eb442..a5fbe38839c49 100644
> --- a/net/xfrm/xfrm_user.c
> +++ b/net/xfrm/xfrm_user.c
> @@ -3476,15 +3476,14 @@ static int xfrm_do_migrate_state(struct sk_buff *skb, struct nlmsghdr *nlh,
>  						       x->nat_keepalive_interval);
>  
>  	if (m.new_family != um->id.family ||
> -	    !xfrm_addr_equal(&m.new_daddr, &um->id.daddr, um->id.family)) {
> -		u32 new_mark_key = m.new_mark ? m.new_mark->v & m.new_mark->m :
> -						m.old_mark.v & m.old_mark.m;
> -		struct xfrm_state *x_new;
> -
> -		x_new = xfrm_state_lookup(net, new_mark_key, &m.new_daddr,
> -					  um->id.spi, um->id.proto, m.new_family);
> -		if (x_new) {
> -			xfrm_state_put(x_new);
> +	    !xfrm_addr_equal(&m.new_daddr, &um->id.daddr, um->id.family) ||
> +	    (m.new_mark && (m.new_mark->v != x->mark.v ||
> +			   m.new_mark->m != x->mark.m))) {
> +		const struct xfrm_mark *new_mark = m.new_mark ? m.new_mark : &x->mark;

[Severity: Low]

This isn't a bug introduced by this patch, but the SA Identification
section of Documentation/networking/xfrm/xfrm_migrate_state.rst is out of
date at this commit. It says the SA is looked up with xfrm_state_lookup(),
using old_mark.v & old_mark.m as the mark key. That is a wildcard match.

xfrm_do_migrate_state() actually does an exact lookup:

	x = xfrm_state_lookup_exact(net, &m.old_mark,
				    &um->id.daddr, um->id.spi,
				    um->id.proto, um->id.family);

xfrm_state_clone_and_setup() sets xc->mark from m->old_mark. The &x->mark
fallback here matches xc->mark only because that lookup is exact. So the
code is consistent, and only the documentation is stale.

A later commit in the series, "docs: xfrm: include mark in
XFRM_MSG_MIGRATE_STATE EEXIST tuple", appears to fix this. It rewrites the
section to describe xfrm_state_lookup_exact() with an exact match against
old_mark.v and old_mark.m.

> +
> +		if (xfrm_state_mark_collides(net, new_mark->v & new_mark->m,
> +					     &m.new_daddr, um->id.spi,
> +					     um->id.proto, m.new_family, x)) {
>  			NL_SET_ERR_MSG(extack, "New SA tuple already occupied");
>  			err = -EEXIST;
>  			goto out;

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 13/14] docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The new wildcard-shadowing text in the Error Handling section of
  Documentation/networking/xfrm/xfrm_migrate_state.rst does not say that…

--

docs: xfrm: include mark in XFRM_MSG_MIGRATE_STATE EEXIST tuple

This patch updates the XFRM_MSG_MIGRATE_STATE documentation. Mark is now
listed as part of the tuple that can return -EEXIST. The SA lookup text now
describes the exact mark/mask match done by xfrm_state_lookup_exact(). The
error handling section also gains a description of wildcard shadowing.

> diff --git a/Documentation/networking/xfrm/xfrm_migrate_state.rst b/Documentation/networking/xfrm/xfrm_migrate_state.rst
> index 9d53cb22b0075..ca2c9e5aaf33a 100644
> --- a/Documentation/networking/xfrm/xfrm_migrate_state.rst
> +++ b/Documentation/networking/xfrm/xfrm_migrate_state.rst

[ ... ]

> @@ -259,8 +264,12 @@ Attributes in the notification
>  Error Handling
>  ==============
>  
> -If the target SA tuple (new daddr, SPI, proto, new family) is already
> -occupied, the operation returns ``-EEXIST`` before the migration begins.
> +If the target SA tuple (new daddr, SPI, proto, new family, mark) is
> +already occupied, the operation returns ``-EEXIST`` before the migration
> +begins. "Occupied" includes wildcard shadowing: an existing SA with a
> +broader mask (e.g. mark 0/0) claims every mark value, so it blocks
> +migrating to any more specific mark at the same tuple, not just an
> +exact mark/mask duplicate.

[Severity: Low]
Should this text say that the SA being migrated is excluded from the check?

The SA being migrated is also an existing SA. Read literally, the new text
says that a lone 0/0 SA cannot be given a specific mark at the same daddr,
SPI, proto and family, and that the request returns -EEXIST.

The code allows this. In xfrm_do_migrate_state(), the source SA x is passed
in as self:

net/xfrm/xfrm_user.c:xfrm_do_migrate_state() {
    ...
		if (xfrm_state_mark_collides(net, new_mark->v & new_mark->m,
					     &m.new_daddr, um->id.spi,
					     um->id.proto, m.new_family, x)) {
    ...
}

xfrm_state_mark_collides() only looks at entries where x != self. Its
comment says:

/* True if some OTHER state at this tuple would wildcard-match "mark".
 * Used by MIGRATE_STATE, which must exclude the state being migrated.
 */

Because of this, the source SA's own 0/0 wildcard never causes a rejection.
The migration then goes ahead, and the new SA is installed with the
requested mark.

Could the wording say that the conflict is with another SA at the target
tuple, for example "another existing SA with a broader mask"?

>  The old SA remains intact and the operation is safe to retry after
>  resolving the conflict.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com

Re: [PATCH 14/14] xfrm4: use a full socket for local errors

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] This patch adds the full-socket check inside the callee,
  xfrm4_local_error(), not in the IP-TFS caller.
- [Low] The commit message's root-cause explanation is wrong for IPv4.

Pre-existing issues:
- [Critical] IPv6 version of the same IP-TFS too-big path: a NULL pointer
  dereference of skb->sk is still reachable.

--

xfrm4: use a full socket for local errors

This commit changes xfrm4_local_error() so it resolves the skb owner with
skb_to_full_sk() before it calls inet_sk() and ip_local_error(). A request
socket is mapped to its listener, and the function returns early when no
full socket is available. This fixes a slab-out-of-bounds read when IP-TFS
reports a SYN-ACK that exceeds its payload MTU.

Link: https://lore.kernel.org/netdev/20260908224856.1591189-1-kuba@kernel.org/

> xfrm4_tunnel_check_size() already checks sk_fullsock() since commit
> 1620c88887b1 ("xfrm: Fix the usage of skb->sk"), so iptfs_is_too_big() is
> the only remaining IPv4 caller passing a request socket here: the sk it
> tests is the listener, while skb->sk is the request socket.

[Severity: Low]
Does this description hold for IPv4?

For IPv4 SYN-ACKs, tcp_make_synack() makes the request socket the owner:

net/ipv4/tcp_output.c:tcp_make_synack() {
    ...
	case TCP_SYNACK_NORMAL:
	case TCP_SYNACK_RETRANS:
		skb_set_owner_edemux(skb, req_to_sk(req));
		break;
    ...
}

tcp_v4_send_synack()->ip_build_and_send_pkt() then passes skb->sk as the
dst_output sk:

net/ipv4/ip_output.c:ip_build_and_send_pkt() {
    ...
	return ip_local_out(net, skb->sk, skb);
}

So the sk that reaches iptfs_output_collect()->iptfs_is_too_big() looks
like the request socket itself, which is the same pointer as skb->sk. The
listener versus request socket split happens on IPv6, where
tcp_v6_send_synack() calls ip6_xmit() with the listener.

Would it be more accurate to say that iptfs_is_too_big() only checks that
sk is non-NULL and never calls sk_fullsock()?

> diff --git a/net/ipv4/xfrm4_output.c b/net/ipv4/xfrm4_output.c
> index 0ae67d537499a..fffa3c4f456ee 100644
> --- a/net/ipv4/xfrm4_output.c
> +++ b/net/ipv4/xfrm4_output.c
> @@ -38,9 +38,12 @@ int xfrm4_output(struct net *net, struct sock *sk, struct sk_buff *skb)
>  
>  void xfrm4_local_error(struct sk_buff *skb, u32 mtu)
>  {
> +	struct sock *sk = skb_to_full_sk(skb);
>  	struct iphdr *hdr;
>  
> +	if (!sk)
> +		return;
> +

[Severity: Low]
Can this early return drop a too-big packet without sending any feedback?

iptfs_is_too_big() in net/xfrm/xfrm_iptfs.c chooses between a local error
and ICMP by testing the sk passed to dst_output(), not the skb's own
socket:

net/xfrm/xfrm_iptfs.c:iptfs_is_too_big() {
    ...
	if (sk)
		xfrm_local_error(skb, pmtu);
	else if (ip_hdr(skb)->version == 4)
		icmp_send(skb, ICMP_DEST_UNREACH, ICMP_FRAG_NEEDED, htonl(pmtu));
    ...
}

When that sk is non-NULL but skb_to_full_sk(skb) returns NULL, the code
now returns here. No error is queued, no ICMP_FRAG_NEEDED is sent, and the
skb is freed with SKB_DROP_REASON_PKT_TOO_BIG. Two IPv4 cases appear to
reach this:

TIME_WAIT replies: ip_send_unicast_reply() calls
skb_set_owner_edemux(nskb, orig_sk) with a TW socket, and ip_send_skb()
passes skb->sk to ip_local_out(). sk_to_full_sk() maps TW to NULL.

UDP tunnels: iptunnel_xmit() calls ip_local_out(net, sk, skb) with the
tunnel socket for forwarded skbs, where skb->sk is NULL.

Both peer callers gate on the skb's own socket and fall back to ICMP:

net/xfrm/xfrm_output.c:xfrm4_tunnel_check_size() {
    ...
		if (skb->sk && sk_fullsock(skb->sk))
			xfrm_local_error(skb, mtu);
		else
			icmp_send(skb, ICMP_DEST_UNREACH,
				  ICMP_FRAG_NEEDED, htonl(mtu));
    ...
}

xfrm6_tunnel_check_size() does the same with sk = skb_to_full_sk(skb).

Before this patch, these paths were a NULL dereference or an
out-of-bounds inet_sock read, so ICMP was never sent here either.

Could the check go in iptfs_is_too_big() instead, gating on
skb_to_full_sk(skb) as the peers do, so the ICMP fallback is kept?

>  	hdr = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
> -	ip_local_error(skb->sk, EMSGSIZE, hdr->daddr,
> -		       inet_sk(skb->sk)->inet_dport, mtu);
> +	ip_local_error(sk, EMSGSIZE, hdr->daddr, inet_sk(sk)->inet_dport, mtu);
>  }

[Severity: Critical]
This is a pre-existing issue, but is the IPv6 version of this IP-TFS
too-big path still open to a NULL dereference of skb->sk?

The commit message points to 6973a21ee73c for xfrm6_local_error(). Before
that code runs, though, the xfrm_local_error() dispatcher reads
skb->sk->sk_family without a NULL check:

net/xfrm/xfrm_output.c:xfrm_local_error() {
    ...
	else if (skb->protocol == htons(ETH_P_IPV6) &&
		 skb->sk->sk_family == AF_INET6)
		proto = AF_INET6;
    ...
}

Since iptfs_is_too_big() tests the dst_output sk, there are IPv6 paths
where that sk is non-NULL but skb->sk is NULL:

tcp_v6_send_response() for an RST with no matching socket. buff comes
from alloc_skb() with no owner and is sent with
ip6_xmit(ctl_sk, buff, ...).

tcp_v6_send_synack() in syncookie mode. tcp_make_synack() does not
attach the skb for TCP_SYNACK_COOKIE, but ip6_xmit() gets the listener.

udp_tunnel6_xmit_skb()->ip6tunnel_xmit(sk, ...) for forwarded VXLAN or
Geneve traffic.

In each case skb->protocol is ETH_P_IPV6, and the path is:

dst_output()
  iptfs_output_collect(net, sk, skb)
    iptfs_is_too_big(sk, skb, pmtu)
      xfrm_local_error()
        skb->sk->sk_family   <-- skb->sk is NULL

This needs an IPv6 IP-TFS SA with XFRMA_IPTFS_DONT_FRAG and a packet
larger than pmtu. A remote SYN to a closed port or a SYN flood could then
trigger it from softirq context.

The commit message limits this fix to IPv4, so this was not introduced
here. Gating iptfs_is_too_big() on skb_to_full_sk(skb), as
xfrm6_tunnel_check_size() and xfrm4_tunnel_check_size() do, would cover
both families. Another option is a NULL check on skb->sk in
xfrm_local_error() before it reads sk_family.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261102100451.2505200-2-steffen.klassert%40secunet.com