Re: [PATCH net] net: xfrm: Fix dev refcnt leak in async resumption

[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
---
net: xfrm: Fix dev refcnt leak in async resumption

This commit adds a dev_put() to the return path of xfrm_input() when afinfo
is NULL during asynchronous resumption, aiming to prevent a device reference
leak.

> diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
> index f65291eba1f68..c98a29d80a534 100644
> --- a/net/xfrm/xfrm_input.c
> +++ b/net/xfrm/xfrm_input.c
> @@ -765,6 +765,8 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
>  			return err;

This return is reached when xfrm_gro is true and gro_cells_receive() has
just consumed the skb.

If afinfo is NULL, err remains -EAFNOSUPPORT (-97).

When a negative error is returned back to the synchronous IPv4 input path
(ip_protocol_deliver_rcu), doesn't the IP stack typically resubmit the
packet using the negated error code (-err = 97) as the protocol number?

Could this cause a use-after-free when the IP stack attempts to dereference
the already-consumed skb?

Also, since transport_finish() was skipped, could this inject an skb with
unreset headers into the GRO engine?

Would it be safer to jump to the drop label immediately if !afinfo?

>  		}
>  
> +		if (unlikely(!afinfo) && async)
> +			dev_put(skb->dev);
>  		return err;

In the asynchronous path, callers like the bottom-half crypto completion
callbacks return void and ignore the return value of xfrm_input():

net/ipv4/esp4.c:esp_input_done2() {
    ...
    xfrm_input_resume(skb, err);
}

Does this code leak the skb memory?

Returning an error here drops the device reference but appears to leave the
skb un-freed, as it is never passed to the next protocol layer or explicitly
freed via kfree_skb() or by jumping to the drop label.

>  	}
>