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

[Massimiliano Pellizzer]

On Tue, 18 Feb 2025 at 08:04, Koichiro Den <koichiro.den at 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 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,

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);
> +>>>>>>>
>  }
>

-- 
Massimiliano Pellizzer