Re: [PATCH net] net: free the policies if a socket's clone fails late

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] net: xfrm policy leak in sk_clone() when xfrm_sk_clone_policy() fails
--

commit a4aae636f8ebbd9f602d754c2bcb096a55363b3d
Author: Josef Bacik <josef@toxicpanda.com>

net: free the policies if a socket's clone fails late

This commit calls xfrm_sk_free_policy() in sk_clone() when
bpf_sk_storage_clone() fails so that cloned IPsec socket policies are not
leaked when freeing the new socket.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 1302c1e848b2a..cdbd49ff71b0a 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2578,8 +2578,10 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  		goto free;
>  	}
>  
> -	if (bpf_sk_storage_clone(sk, newsk))
> +	if (bpf_sk_storage_clone(sk, newsk)) {
> +		xfrm_sk_free_policy(newsk);
>  		goto free;
> +	}
>  
>  	/* Clear sk_user_data if parent had the pointer tagged
>  	 * as not suitable for copying when cloning.

[Severity: Medium]
Can newsk->sk_policy[0] still leak if xfrm_sk_clone_policy() fails on the
second policy?

In net/core/sock.c:sk_clone(), just above this block:

	if (unlikely(!is_charged || xfrm_sk_clone_policy(newsk, sk))) {
		if (!is_charged)
			RCU_INIT_POINTER(newsk->sk_filter, NULL);

		goto free;
	}

When is_charged is true and the parent socket has both sk_policy[0] and
sk_policy[1] installed, xfrm_sk_clone_policy() clears newsk->sk_policy[0..1]
and calls __xfrm_sk_clone_policy():

net/xfrm/xfrm_policy.c:__xfrm_sk_clone_policy() {
	for (i = 0; i < 2; i++) {
		p = rcu_dereference(osk->sk_policy[i]);
		if (p) {
			np = clone_policy(p, i);
			if (unlikely(!np)) {
				ret = -ENOMEM;
				break;
			}
			rcu_assign_pointer(sk->sk_policy[i], np);
		}
	}
}

If clone_policy(p, 0) succeeds (allocating the policy, linking it into
net->xfrm.policy_all, and incrementing net->xfrm.policy_count[XFRM_POLICY_MAX])
and clone_policy(p, 1) then fails, __xfrm_sk_clone_policy() returns -ENOMEM
with newsk->sk_policy[0] still populated.

Because sk_clone() jumps directly to free without calling
xfrm_sk_free_policy(newsk) on that path, wouldn't newsk->sk_policy[0] still
be leaked?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261017013912.396296-1-alexei.starovoitov@gmail.com?part=1