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

["dongchenchen (A)" <dongchenchen2@huawei.com>]

> 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

>>   	}
>>