| Message ID | gqmssj4i57aqfsq4gb7wd6oa3vdestcj7z3qfn7afodfackyra@sjnwgeptu7xr |
|---|---|
| State | New |
| Headers | show |
| Series | Cmnt: [SRU][F/J/N][PATCH 0/1] CVE-2024-56658 | expand |
On Tue, 18 Feb 2025 at 08:04, Koichiro Den <koichiro.den@canonical.com> wrote: > > 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@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@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, I will send a v2 solving the conflict, no worries. Thanks for reviewing. > > [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); > +>>>>>>> > } >
--- 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); +>>>>>>> }