| Message ID | 20220310183513.28589-1-linus.luessing@c0d3.blue (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [net] mac80211: fix potential double free on mesh join | expand |
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@simonwunderlich.de> > > 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@fit.fraunhofer.de> > Signed-off-by: Linus Lüssing <ll@simonwunderlich.de> > --- > 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;
Tested-by: Mathias Kretschmer <mathias.kretschmer@fit.fraunhofer.de>
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;