2018-09-30 10:53:36

by Stefan Seyfried

Subject: [PATCH] cfg80211: fix wext-compat memory leak

From: Stefan Seyfried <[email protected]>

cfg80211_wext_giwrate and sinfo.pertid might allocate sinfo.pertid via
rdev_get_station(), but never release it

Signed-off-by: Stefan Seyfried <[email protected]>
---
net/wireless/wext-compat.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
index 167f7025ac98..f462336aac1c 100644
--- a/net/wireless/wext-compat.c
+++ b/net/wireless/wext-compat.c
@@ -1277,12 +1277,16 @@ static int cfg80211_wext_giwrate(struct net_device *dev,
err = rdev_get_station(rdev, dev, addr, &sinfo);
if (err)
return err;
-
if (!(sinfo.filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE)))
return -EOPNOTSUPP;

rate->value = 100000 * cfg80211_calculate_bitrate(&sinfo.txrate);

+ /* sta_set_sinfo(), called from ieee80211_get_station(), called from
+ * rdev_get_station via rdev->ops->get_station, allocates pertid struct
+ * which we do not use here. */
+ kfree(sinfo.pertid);
+
return 0;
}

@@ -1293,7 +1297,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
/* we are under RTNL - globally locked - so can use static structs */
static struct iw_statistics wstats;
- static struct station_info sinfo;
+ static struct station_info sinfo = {};
u8 bssid[ETH_ALEN];

if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_STATION)
@@ -1352,6 +1356,9 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_TX_FAILED))
wstats.discard.retries = sinfo.tx_failed;

+ /* see cfg80211_wext_giwrate() above */
+ kfree(sinfo.pertid);
+
return &wstats;
}

--
2.19.0

2018-10-01 07:13:22

by Johannes Berg

Subject: Re: [PATCH] cfg80211: fix wext-compat memory leak

Hi Stefan,

Thanks for the investigation & fix. I've changed the fix to use
cfg80211_sinfo_release_content(), take care of the error path in
cfg80211_wext_giwrate() and added a Fixes tag :-)

Applied with those changes.

johannes


On Sun, 2018-09-30 at 12:53 +0200, [email protected] wrote:
> From: Stefan Seyfried <[email protected]>
>
> cfg80211_wext_giwrate and sinfo.pertid might allocate sinfo.pertid via
> rdev_get_station(), but never release it
>
> Signed-off-by: Stefan Seyfried <[email protected]>
> ---
> net/wireless/wext-compat.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/net/wireless/wext-compat.c b/net/wireless/wext-compat.c
> index 167f7025ac98..f462336aac1c 100644
> --- a/net/wireless/wext-compat.c
> +++ b/net/wireless/wext-compat.c
> @@ -1277,12 +1277,16 @@ static int cfg80211_wext_giwrate(struct net_device *dev,
> err = rdev_get_station(rdev, dev, addr, &sinfo);
> if (err)
> return err;
> -
> if (!(sinfo.filled & BIT_ULL(NL80211_STA_INFO_TX_BITRATE)))
> return -EOPNOTSUPP;
>
> rate->value = 100000 * cfg80211_calculate_bitrate(&sinfo.txrate);
>
> + /* sta_set_sinfo(), called from ieee80211_get_station(), called from
> + * rdev_get_station via rdev->ops->get_station, allocates pertid struct
> + * which we do not use here. */
> + kfree(sinfo.pertid);
> +
> return 0;
> }
>
> @@ -1293,7 +1297,7 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
> struct cfg80211_registered_device *rdev = wiphy_to_rdev(wdev->wiphy);
> /* we are under RTNL - globally locked - so can use static structs */
> static struct iw_statistics wstats;
> - static struct station_info sinfo;
> + static struct station_info sinfo = {};
> u8 bssid[ETH_ALEN];
>
> if (dev->ieee80211_ptr->iftype != NL80211_IFTYPE_STATION)
> @@ -1352,6 +1356,9 @@ static struct iw_statistics *cfg80211_wireless_stats(struct net_device *dev)
> if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_TX_FAILED))
> wstats.discard.retries = sinfo.tx_failed;
>
> + /* see cfg80211_wext_giwrate() above */
> + kfree(sinfo.pertid);
> +
> return &wstats;
> }
>

2018-10-01 08:56:35

by Stefan Seyfried

Subject: Re: [PATCH] cfg80211: fix wext-compat memory leak

Hi Johannes,

Am 01.10.18 um 09:12 schrieb Johannes Berg:
> Hi Stefan,
>
> Thanks for the investigation & fix. I've changed the fix to use
> cfg80211_sinfo_release_content(),

Being totally unfamiliar with the code, I was not sure about that :-)

> take care of the error path in
> cfg80211_wext_giwrate() and added a Fixes tag :-)

That's certainly much better.

Thanks!

(kudos to Vlastimil Babka who pointed out trace-cmd to me on the
opensuse-kernel malinglist, which made the investigation quite easy)
--
Stefan Seyfried

"For a successful technology, reality must take precedence over
public relations, for nature cannot be fooled." -- Richard Feynman