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

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

[Dong Chenchen]

xfrm async resumption hold dev ref until after transport_finish.
When afinfo is NULL (e.g. x->props.family is invalid) and xfrm_gro
is not set, dev_put in transport_finish() and xfrm_gro path is skipped.
This will trigger refcnt leak.

Add dev_put in return path to fix it.

Fixes: 1c428b038400 ("xfrm: hold dev ref until after transport_finish NF_HOOK")
Signed-off-by: Dong Chenchen <dongchenchen2@huawei.com>
---
 net/xfrm/xfrm_input.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index f65291eba1f6..c98a29d80a53 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;
 		}
 
+		if (unlikely(!afinfo) && async)
+			dev_put(skb->dev);
 		return err;
 	}
 
-- 
2.25.1

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

[Simon Horman]

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.

>  	}
>