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

On Sat, Aug 22, 2026 at 3:03 PM Zhiling Zou <[email protected]> wrote:
>
> From: Zhiling Zou <[email protected]>
>
> 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

Hi Sashiko,

Thanks for the review. The finding is correct.

vti_rcv_cb() calls skb_scrub_packet(), which resets the skb extensions
before checking xnet. Therefore, the VTI receive path drops the secpath
regardless of whether the packet crosses network namespaces.

The cross-network-namespace qualifier only applies to the XFRM interface
path, where xfrmi_scrub_packet() calls secpath_reset() only when xnet is
true.

I will update the wording in the next revision to distinguish the two
cases. No code change is needed for this finding.

Thanks,
Zhiling

> 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(). A larval
> XFRM_STATE_ACQ state has no type, so retain nexthdr as its protocol. This
> preserves the existing drop-path fallback while avoiding 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: [email protected]
> Reported-by: Vega <[email protected]>
> Signed-off-by: Zhiling Zou <[email protected]>
> ---
> changes in v3:
> - Retain nexthdr for larval XFRM_STATE_ACQ entries, whose x->type is NULL.
> - Avoid dereferencing x->type before the existing state validity checks.
> - v2 Link: https://lore.kernel.org/all/e59b617d27acb3a38a3523649d52b90f022fa28d.1785288865.git.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..77b9f4d4568d6 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 ? x->type->proto : nexthdr;
>
>                 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 ? x->type->proto : nexthdr;
>
>                 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