Re: [PATCH net] mac80211: fix potential double free on mesh join

[Kretschmer, Mathias]

Yep, that's correct. We do usually not go through a NETDEV_DOWN/NETDEV_UP cycle,
when, for example, reassigning channels.

Will test your patch and report back.

BR,

Mathias

On Thu, 2022-03-10 at 19:35 +0100, Linus Lüssing wrote:
> From: Linus Lüssing <ll@xxxxxxxxxxxxxxxxxx>
> 
> While commit 6a01afcf8468 ("mac80211: mesh: Free ie data when leaving
> mesh") fixed a memory leak on mesh leave / teardown it introduced a
> potential memory corruption caused by a double free when rejoining the
> mesh:
> 
>   ieee80211_leave_mesh()
>   -> kfree(sdata->u.mesh.ie);
>   ...
>   ieee80211_join_mesh()
>   -> copy_mesh_setup()
>      -> old_ie = ifmsh->ie;
>      -> kfree(old_ie);
> 
> This double free / kernel panics can be reproduced by using wpa_supplicant
> with an encrypted mesh (if set up without encryption via "iw" then
> ifmsh->ie is always NULL, which avoids this issue). And then calling:
> 
>   $ iw dev mesh0 mesh leave
>   $ iw dev mesh0 mesh join my-mesh
> 
> Note that typically these commands are not used / working when using
> wpa_supplicant. And it seems that wpa_supplicant or wpa_cli are going
> through a NETDEV_DOWN/NETDEV_UP cycle between a mesh leave and mesh join
> where the NETDEV_UP resets the mesh.ie to NULL via a memcpy of
> default_mesh_setup in cfg80211_netdev_notifier_call, which then avoids
> the memory corruption, too.
> 
> The issue was first observed in an application which was not using
> wpa_supplicant but "Senf" instead, which implements its own calls to
> nl80211.
> 
> Fixing the issue by removing the kfree()'ing of the mesh IE in the mesh
> join function and leaving it solely up to the mesh leave to free the
> mesh IE.
> 
> Link: https://gitlab.fit.fraunhofer.de/wiback/senf
> Fixes: 6a01afcf8468 ("mac80211: mesh: Free ie data when leaving mesh")
> Reported-by: Matthias Kretschmer <mathias.kretschmer@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Linus Lüssing <ll@xxxxxxxxxxxxxxxxxx>
> ---
>  net/mac80211/cfg.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 87a208089caf..58ff57dc669c 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -2148,14 +2148,12 @@ static int copy_mesh_setup(struct ieee80211_if_mesh
> *ifmsh,
>                 const struct mesh_setup *setup)
>  {
>         u8 *new_ie;
> -       const u8 *old_ie;
>         struct ieee80211_sub_if_data *sdata = container_of(ifmsh,
>                                         struct ieee80211_sub_if_data, u.mesh);
>         int i;
>  
>         /* allocate information elements */
>         new_ie = NULL;
> -       old_ie = ifmsh->ie;
>  
>         if (setup->ie_len) {
>                 new_ie = kmemdup(setup->ie, setup->ie_len,
> @@ -2165,7 +2163,6 @@ static int copy_mesh_setup(struct ieee80211_if_mesh
> *ifmsh,
>         }
>         ifmsh->ie_len = setup->ie_len;
>         ifmsh->ie = new_ie;
> -       kfree(old_ie);
>  
>         /* now copy the rest of the setup parameters */
>         ifmsh->mesh_id_len = setup->mesh_id_len;