Re: [PATCH net 1/1] ipv6: xfrm6: release dst on error in xfrm6_rcv_encap()

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ipv6: xfrm6: release dst on error in xfrm6_rcv_encap()

This commit fixes a dst entry leak in xfrm6_rcv_encap() by ensuring
the dst is released when ip6_route_input_lookup() returns an error route.

> diff --git a/net/ipv6/xfrm6_protocol.c b/net/ipv6/xfrm6_protocol.c
> index ea2f805d3b014..9b586fcec4850 100644
> --- a/net/ipv6/xfrm6_protocol.c
> +++ b/net/ipv6/xfrm6_protocol.c
> @@ -88,8 +88,10 @@ int xfrm6_rcv_encap(struct sk_buff *skb, int nexthdr, __be32 spi,
>  
>  		dst = ip6_route_input_lookup(dev_net(skb->dev), skb->dev, &fl6,
>  					     skb, flags);

This isn't a bug introduced by this patch, but is it possible for skb->dev
to be NULL here?

When ESP packets are delivered over TCP, they are processed asynchronously
via handle_esp() in net/xfrm/espintcp.c. This dynamically resolves skb->dev
using dev_get_by_index_rcu(). If the interface was removed before this runs,
it returns NULL.

Because TCP receive paths drop the skb's dst entry, skb_dst(skb) is NULL.
When handle_esp() passes the skb to xfrm6_rcv_encap(), it enters the
!skb_dst(skb) block and unconditionally accesses skb->dev->ifindex and
calls dev_net(skb->dev).

Could this result in a NULL pointer dereference if the interface is torn
down while packets are being processed asynchronously?

> -		if (dst->error)
> +		if (dst->error) {
> +			dst_release(dst);
>  			goto drop;
> +		}
>  		skb_dst_set(skb, dst);
>  	}
>