Cmnt: [SRU][F/J/N][PATCH 0/1] CVE-2024-56658

[Koichiro Den]

On Wed, Feb 12, 2025 at 12:43:53PM GMT, Massimiliano Pellizzer wrote:
> [Impact]
> 
> net: defer final 'struct net' free in netns dismantle
> 
> BUG: KASAN: slab-use-after-free in dst_destroy
> 
> Issue is in xfrm6_net_init() and xfrm4_net_init() :
> 
> They copy xfrm[46]_dst_ops_template into net->xfrm.xfrm[46]_dst_ops.
> 
> But net structure might be freed before all the dst callbacks are
> called. So when dst_destroy() calls later :
> 
> if (dst->ops->destroy)
>     dst->ops->destroy(dst);
> 
> dst->ops points to the old net->xfrm.xfrm[46]_dst_ops, which has been freed.
> 
> See a relevant issue fixed in :
> 
> ac888d58869b ("net: do not delay dst_entries_add() in dst_release()")
> 
> A fix is to queue the 'struct net' to be freed after one
> another cleanup_net() round (and existing rcu_barrier())
> 
> [Fix]
> 
> Oracular: Fixed via upstream stable updates (LP: #2097332)
> Noble: Cherry picked from mainline
> Jammy: Backported from mainline
> Focal: Backported from mainline
> 
> [Test case]
> 
> Compiled and boot tested.
> Tested basic net namespace functionalities:
> 
> $ sudo ip netns add client
> $ sudo ip netns add server
> $ sudo ip link add veth1 type veth peer name veth2
> $ sudo ip link set dev veth1 netns server
> $ sudo ip link set dev veth2 netns client
> $ sudo ip netns exec server ip addr add dev veth1 192.168.99.1/24
> $ sudo ip netns exec client ip addr add dev veth2 192.168.99.2/24
> $ sudo ip netns exec server ip link set dev veth1 up
> $ sudo ip netns exec client ip link set dev veth2 up
> $ sudo ip netns exec client ip a
> 3: veth2 at if4: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
>     link/ether 8a:06:9e:c4:14:d0 brd ff:ff:ff:ff:ff:ff link-netns server
>     inet 192.168.99.2/24 scope global veth2
>        valid_lft forever preferred_lft forever
> $ sudo ip netns exec server ip a
> 4: veth1 at if3: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP group default qlen 1000
>     link/ether 42:3d:3b:37:38:93 brd ff:ff:ff:ff:ff:ff link-netns client
>     inet 192.168.99.1/24 scope global veth1
>        valid_lft forever preferred_lft forever
> $ sudo ip netns exec server iperf -s &
> $ sudo ip netns exec client iperf -c 192.168.99.1
> [  1] local 192.168.99.2 port 37994 connected with 192.168.99.1 port 5001 (icwnd/mss/irtt=14/1448/37)
> [ ID] Interval       Transfer     Bandwidth
> [  1] 0.0000-10.0029 sec  34.1 GBytes  29.3 Gbits/sec
> $ sudo ip netns exec server ip link set veth1 netns 1
> $ sudo ip netns exec client ip link set veth2 netns 1
> $ sudo ip netns delete client
> $ sudo ip netns delete server
> 
> [Where problems could occur]
> 
> The fix affects the network namespace subsystem. An issue with this fix
> may lead to incorrect handling of network namespace cleanup and resource
> deallocation. A user might experience problems such as unexpected
> crashes when deleting network namespaces and lingering network
> interfaces that are not properly released. Additionally, network
> isolation between namespaces may be compromised. 
> 
> Eric Dumazet (1):
>   net: defer final 'struct net' free in netns dismantle
> 
>  include/net/net_namespace.h |  1 +
>  net/core/net_namespace.c    | 21 ++++++++++++++++++++-
>  2 files changed, 21 insertions(+), 1 deletion(-)
> 

The backport itself looks good to me.

Unfortunately, the context changed for Focal due to the recent upstream stable
release (LP: #2098439), which includes the backported commit 41467d2ff4dfe
("net: net_namespace: Optimize the code"). This change is causing a small
conflict [1].

Can you please re-send the patch based on the updated master-next? I always
impressed by your excellent and careful work, and I feel sorry for the extra
hassle caused by the delayed review. But I believe it's much safer for you to
resolve the conflict than for someone else who apply the backport to the tree.
If it's ok, please NACK this before re-sending.

Thanks,

[1]:
--- a/net/core/net_namespace.c
+++ b/net/core/net_namespace.c
@@ -449,12 +449,34 @@ static struct net *net_alloc(void)
        goto out;
 }

+static LLIST_HEAD(defer_free_list);
+
+static void net_complete_free(void)
+{
+       struct llist_node *kill_list;
+       struct net *net, *next;
+
+       /* Get the list of namespaces to free from last round. */
+       kill_list = llist_del_all(&defer_free_list);
+
+       llist_for_each_entry_safe(net, next, kill_list, defer_free_list)
+               kmem_cache_free(net_cachep, net);
+
+}
+
 static void net_free(struct net *net)
 {
+<<<<<<<
        if (refcount_dec_and_test(&net->passive)) {
                kfree(rcu_access_pointer(net->gen));
                kmem_cache_free(net_cachep, net);
        }
+=======
+       kfree(rcu_access_pointer(net->gen));
+
+       /* Wait for an extra rcu_barrier() before final free. */
+       llist_add(&net->defer_free_list, &defer_free_list);
+>>>>>>>
 }