[PATCH net v2 1/1] xfrm: save input state data before secpath resets

[PATCH net v2 1/1] xfrm: save input state data before secpath resets

[Zhiling Zou]

xfrm_input() keeps the current state reference through the skb secpath
while it performs final transport processing.  Some input paths can reset
the secpath before xfrm_input() is done using data from that state.

Receive callback users such as VTI and XFRM interfaces can scrub packets
that cross network namespaces by resetting the secpath.  The
XFRM_MAX_DEPTH error path can also reset the secpath before the final drop
callback reports the current state's protocol.  If that drops the last
state reference and the state is concurrently deleted, xfrm_input() can
still dereference the freed state.

Save the state protocol on the stack while the state is still valid and use
the already saved address family for transport_finish().  This avoids the
post-reset state dereferences without adding an extra state reference to
every received packet.

Fixes: df3893c176e9 ("vti: Update the ipv4 side to use it's own receive hook.")
Cc: stable@vger.kernel.org
Reported-by: Vega <vega@nebusec.ai>
Signed-off-by: Zhiling Zou <zhilinz@nebusec.ai>
---
changes in v2:
- Avoid per-packet xfrm_state refcounting by saving state data on the stack.
- Use the saved protocol and family after secpath resets instead of dereferencing x.
- v1 Link: https://lore.kernel.org/all/89be67de93958a00193c8c4bd668733c976e2aa9.1785135576.git.zhilinz@nebusec.ai/

 net/xfrm/xfrm_input.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index eecab337bd0a7..3177fa33961cb 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -474,6 +474,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	struct xfrm_state *x = NULL;
 	xfrm_address_t *daddr;
 	u32 mark = skb->mark;
+	u8 xfrm_proto = nexthdr;
 	unsigned int family = AF_UNSPEC;
 	int decaps = 0;
 	int async = 0;
@@ -485,6 +486,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	if (encap_type < 0 || (xo && (xo->flags & XFRM_GRO || encap_type == 0 ||
 				      encap_type == UDP_ENCAP_ESPINUDP))) {
 		x = xfrm_input_state(skb);
+		xfrm_proto = x->type->proto;
 
 		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
 			if (x->km.state == XFRM_STATE_ACQ)
@@ -592,11 +594,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 
 		x = xfrm_input_state_lookup(net, mark, daddr, spi, nexthdr, family);
 		if (x == NULL) {
+			xfrm_proto = nexthdr;
 			secpath_reset(skb);
 			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOSTATES);
 			xfrm_audit_state_notfound(skb, family, spi, seq);
 			goto drop;
 		}
+		xfrm_proto = x->type->proto;
 
 		if (unlikely(x->dir && x->dir != XFRM_SA_DIR_IN)) {
 			secpath_reset(skb);
@@ -604,6 +608,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			xfrm_audit_state_notfound(skb, family, spi, seq);
 			xfrm_state_put(x);
 			x = NULL;
+			xfrm_proto = nexthdr;
 			goto drop;
 		}
 
@@ -728,7 +733,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 	} while (!err);
 
 	rcu_read_lock();
-	err = xfrm_rcv_cb(skb, family, x->type->proto, 0);
+	err = xfrm_rcv_cb(skb, family, xfrm_proto, 0);
 	if (err) {
 		rcu_read_unlock();
 		goto drop;
@@ -753,7 +758,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 			xfrm_gro = xo->flags & XFRM_GRO;
 
 		err = -EAFNOSUPPORT;
-		afinfo = xfrm_state_afinfo_get_rcu(x->props.family);
+		afinfo = xfrm_state_afinfo_get_rcu(family);
 		if (likely(afinfo))
 			err = afinfo->transport_finish(skb, xfrm_gro || async);
 		if (xfrm_gro) {
@@ -776,7 +781,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
 drop:
 	if (async)
 		dev_put(dev);
-	xfrm_rcv_cb(skb, family, x && x->type ? x->type->proto : nexthdr, -1);
+	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
 	kfree_skb(skb);
 	return 0;
 }
-- 
2.43.0

Re: [PATCH net v2 1/1] xfrm: save input state data before secpath resets

[Steffen Klassert]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
--- Patch [1]: [PATCH net v2 1/1] xfrm: save input state data before secpath resets ---
commit 00c0af70a4245da8443da16c81f86becd446039e
Author: Zhiling Zou <zhilinz@nebusec.ai>

xfrm: save input state data before secpath resets

This snapshots the current state's protocol into a local variable early in
xfrm_input() so the later xfrm_rcv_cb() calls no longer dereference x after
a secpath reset, and switches transport_finish() to use the already saved
family instead of x->props.family.

Link: https://lore.kernel.org/all/89be67de93958a00193c8c4bd668733c976e2aa9.1785135576.git.zhilinz@nebusec.ai/

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index eecab337bd0a..3177fa33961c 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -474,6 +474,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  	struct xfrm_state *x = NULL;
>  	xfrm_address_t *daddr;
>  	u32 mark = skb->mark;
> +	u8 xfrm_proto = nexthdr;
>  	unsigned int family = AF_UNSPEC;
>  	int decaps = 0;
>  	int async = 0;
> @@ -485,6 +486,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  	if (encap_type < 0 || (xo && (xo->flags & XFRM_GRO || encap_type == 0 ||
>  				      encap_type == UDP_ENCAP_ESPINUDP))) {
>  		x = xfrm_input_state(skb);
> +		xfrm_proto = x->type->proto;
>
>  		if (unlikely(x->km.state != XFRM_STATE_VALID)) {
>  			if (x->km.state == XFRM_STATE_ACQ)

Can x->type be NULL at this point?  The snapshot is taken before the
km.state check that immediately follows, and that check exists precisely to
handle XFRM_STATE_ACQ states.

x->type is only ever assigned in __xfrm_init_state():

net/xfrm/xfrm_state.c:__xfrm_init_state() {
	...
	x->type = xfrm_get_type(x->id.proto, family);
	if (x->type == NULL) {
	...
}

A larval ACQUIRE state never reaches that function, and xfrm_state_alloc()
uses kmem_cache_zalloc(), so x->type stays NULL.  xfrm_state_gc_destroy()
also guards with "if (x->type)", which suggests a NULL type is a legitimate
state for hash-reachable SAs.

On the GRO path, esp4_gro_receive() installs the looked-up state into the
secpath with only an x->dir sanity check and then calls xfrm_input() with
encap_type == 0, so no km.state or type validation happens before the new
dereference.

> @@ -592,11 +594,13 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>
>  		x = xfrm_input_state_lookup(net, mark, daddr, spi, nexthdr, family);
>  		if (x == NULL) {
> +			xfrm_proto = nexthdr;
>  			secpath_reset(skb);
>  			XFRM_INC_STATS(net, LINUX_MIB_XFRMINNOSTATES);
>  			xfrm_audit_state_notfound(skb, family, spi, seq);
>  			goto drop;
>  		}
> +		xfrm_proto = x->type->proto;
>
>  		if (unlikely(x->dir && x->dir != XFRM_SA_DIR_IN)) {
>  			secpath_reset(skb);

Same question for this snapshot: it runs right after a successful lookup,
before the x->dir check, before spin_lock(&x->lock), and long before the
km.state != XFRM_STATE_VALID / XFRM_STATE_ACQ handling at the process:
label.

Does the lookup filter out larval states?  __xfrm_state_lookup() only
compares family, spi, proto, daddr and mark:

net/xfrm/xfrm_state.c:__xfrm_state_lookup() {
	hlist_for_each_entry_rcu(x, state_ptrs->byspi + h, byspi) {
		if (x->props.family != family ||
		    x->id.spi       != spi ||
		    x->id.proto     != proto ||
		    !xfrm_addr_equal(&x->id.daddr, daddr, family))
			continue;
	...
}

and xfrm_input_state_lookup() explicitly documents returning a state that
is "about to be destroyed".

An ACQ state becomes reachable through that hash during the normal IKE
flow, XFRM_MSG_ALLOCSPI -> xfrm_alloc_userspi() -> xfrm_find_acq(create=1)
-> xfrm_alloc_spi():

net/xfrm/xfrm_state.c:xfrm_alloc_spi() {
	...
	x->id.spi = newspi;
	h = xfrm_spi_hash(net, &x->id.daddr, newspi, x->id.proto, x->props.family);
	XFRM_STATE_INSERT(byspi, &x->byspi,
			  xfrm_state_deref_prot(net->xfrm.state_byspi, net) + h,
			  x->xso.type);
	...
}

So an inbound ESP/AH/IPCOMP packet whose SPI matches the just-allocated
larval SA, for example a peer data packet or retransmission arriving between
XFRM_MSG_ALLOCSPI and XFRM_MSG_UPDSA, would reach this dereference in
softirq context instead of taking the existing LINUX_MIB_XFRMACQUIREERROR
drop path.

Would deferring the snapshot until after the km.state validation, or writing
it as "x->type ? x->type->proto : nexthdr", keep the stated goal without
this change in behaviour?

[ ... ]

> @@ -776,7 +781,7 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  drop:
>  	if (async)
>  		dev_put(dev);
> -	xfrm_rcv_cb(skb, family, x && x->type ? x->type->proto : nexthdr, -1);
> +	xfrm_rcv_cb(skb, family, xfrm_proto, -1);
>  	kfree_skb(skb);
>  	return 0;
>  }

This drops the "x && x->type" test that used to be here.  The commit message
describes saving the protocol on the stack and using the saved family, but
does not mention removing this guard.  Was the removal intentional, and if
x->type can never be NULL, could that be stated in the changelog so the
earlier snapshots are justified too?