[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.

>  	}
>

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

[dongchenchen (A)]

> 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.

Yes, indeed.
We need to perform the goto drop operation before xfrm_gro is determined
to fix the skb leakage and the problem that xfrm_gro does not unset the
header. as bellow:

diff --git a/net/xfrm/xfrm_input.c b/net/xfrm/xfrm_input.c
index f65291eba1f6..109ee8893761 100644
--- a/net/xfrm/xfrm_input.c
+++ b/net/xfrm/xfrm_input.c
@@ -750,8 +750,12 @@ int xfrm_input(struct sk_buff *skb, int nexthdr, __be32 spi, int encap_type)
                 err = -EAFNOSUPPORT;
                 rcu_read_lock();
                 afinfo = xfrm_state_afinfo_get_rcu(x->props.family);
-               if (likely(afinfo))
+               if (likely(afinfo)) {
                         err = afinfo->transport_finish(skb, xfrm_gro || async);
+               } else {
+                       rcu_read_unlock();
+                       goto drop;
+               }
                 rcu_read_unlock();
                 if (xfrm_gro) {
                         sp = skb_sec_path(skb);

Thanks a lot for your review! v2 will be submit
-------
Best regards
Dong Chenchen

>>   	}
>>

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

[Herbert Xu]

On Sat, May 09, 2026 at 03:44:43PM +0800, Dong Chenchen wrote:
> 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.

There should never be a an xfrm_state with a NULL afinfo.

That would be a serious bug.

How did you trigger this?

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt