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

[Steffen Klassert <steffen.klassert@secunet.com>]

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?