[PATCH v2 0/2] Fix inadverent sharing of struct ieee80211_supported_band data

[PATCH v2 0/2] Fix inadverent sharing of struct ieee80211_supported_band data

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

This is a series of patches requested by Ping-Ke Shih in response to
https://lore.kernel.org/lkml/20250427002414.410791-1-megi@xff.cz/

Please take a look.

(hw->wiphy->bands[*] are no longer being NULLed when probe fails and on
remove(), but I guess that should not be an issue? I tried unbinding the
device and it worked fine without any crash)

thank you and regards,
	Ondrej Jirman

Changes since v1:
- added patch to convert some memory allocations to be devm_* managed
- check for NULL from kmemdup()
- rename rtw89_copy_sband
- drop some kfree due to them not being needed because failed
  rtw89_core_set_supported_band() results in abandoned probe()
  and devm_* will take care of that
- add error return to rtw89_init_he_eht_cap and check for it

Ondrej Jirman (2):
  wifi: rtw89: Convert rtw89_core_set_supported_band to use devm_*
  wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band
    data

 drivers/net/wireless/realtek/rtw89/core.c | 124 +++++++++++-----------
 1 file changed, 60 insertions(+), 64 deletions(-)

-- 
2.49.0

[PATCH v2 1/2] wifi: rtw89: Convert rtw89_core_set_supported_band to use devm_*

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

The code can be simplified by using device managed memory
allocations. Simplify it.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/net/wireless/realtek/rtw89/core.c | 99 +++++++++--------------
 1 file changed, 36 insertions(+), 63 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index cc9b014457ac..b164bc767e82 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -4361,17 +4361,18 @@ static void rtw89_init_eht_cap(struct rtw89_dev *rtwdev,
 
 #define RTW89_SBAND_IFTYPES_NR 2
 
-static void rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
-				  enum nl80211_band band,
-				  struct ieee80211_supported_band *sband)
+static int rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
+				 enum nl80211_band band,
+				 struct ieee80211_supported_band *sband)
 {
 	struct ieee80211_sband_iftype_data *iftype_data;
 	enum nl80211_iftype iftype;
 	int idx = 0;
 
-	iftype_data = kcalloc(RTW89_SBAND_IFTYPES_NR, sizeof(*iftype_data), GFP_KERNEL);
+	iftype_data = devm_kcalloc(rtwdev->dev, RTW89_SBAND_IFTYPES_NR,
+				   sizeof(*iftype_data), GFP_KERNEL);
 	if (!iftype_data)
-		return;
+		return -ENOMEM;
 
 	for (iftype = 0; iftype < NUM_NL80211_IFTYPES; iftype++) {
 		switch (iftype) {
@@ -4396,77 +4397,52 @@ static void rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
 	}
 
 	_ieee80211_set_sband_iftype_data(sband, iftype_data, idx);
+	return 0;
 }
 
 static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
 {
 	struct ieee80211_hw *hw = rtwdev->hw;
-	struct ieee80211_supported_band *sband_2ghz = NULL, *sband_5ghz = NULL;
-	struct ieee80211_supported_band *sband_6ghz = NULL;
+	struct ieee80211_supported_band *sband;
 	u32 size = sizeof(struct ieee80211_supported_band);
 	u8 support_bands = rtwdev->chip->support_bands;
+	struct device *dev = rtwdev->dev;
+	int ret;
 
 	if (support_bands & BIT(NL80211_BAND_2GHZ)) {
-		sband_2ghz = kmemdup(&rtw89_sband_2ghz, size, GFP_KERNEL);
-		if (!sband_2ghz)
-			goto err;
-		rtw89_init_ht_cap(rtwdev, &sband_2ghz->ht_cap);
-		rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_2GHZ, sband_2ghz);
-		hw->wiphy->bands[NL80211_BAND_2GHZ] = sband_2ghz;
+		sband = devm_kmemdup(dev, &rtw89_sband_2ghz, size, GFP_KERNEL);
+		if (!sband)
+			return -ENOMEM;
+		rtw89_init_ht_cap(rtwdev, &sband->ht_cap);
+		ret = rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_2GHZ, sband);
+		if (ret)
+			return ret;
+		hw->wiphy->bands[NL80211_BAND_2GHZ] = sband;
 	}
 
 	if (support_bands & BIT(NL80211_BAND_5GHZ)) {
-		sband_5ghz = kmemdup(&rtw89_sband_5ghz, size, GFP_KERNEL);
-		if (!sband_5ghz)
-			goto err;
-		rtw89_init_ht_cap(rtwdev, &sband_5ghz->ht_cap);
-		rtw89_init_vht_cap(rtwdev, &sband_5ghz->vht_cap);
-		rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_5GHZ, sband_5ghz);
-		hw->wiphy->bands[NL80211_BAND_5GHZ] = sband_5ghz;
+		sband = devm_kmemdup(dev, &rtw89_sband_5ghz, size, GFP_KERNEL);
+		if (!sband)
+			return -ENOMEM;
+		rtw89_init_ht_cap(rtwdev, &sband->ht_cap);
+		rtw89_init_vht_cap(rtwdev, &sband->vht_cap);
+		ret = rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_5GHZ, sband);
+		if (ret)
+			return ret;
+		hw->wiphy->bands[NL80211_BAND_5GHZ] = sband;
 	}
 
 	if (support_bands & BIT(NL80211_BAND_6GHZ)) {
-		sband_6ghz = kmemdup(&rtw89_sband_6ghz, size, GFP_KERNEL);
-		if (!sband_6ghz)
-			goto err;
-		rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_6GHZ, sband_6ghz);
-		hw->wiphy->bands[NL80211_BAND_6GHZ] = sband_6ghz;
+		sband = devm_kmemdup(dev, &rtw89_sband_6ghz, size, GFP_KERNEL);
+		if (!sband)
+			return -ENOMEM;
+		ret = rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_6GHZ, sband);
+		if (ret)
+			return ret;
+		hw->wiphy->bands[NL80211_BAND_6GHZ] = sband;
 	}
 
 	return 0;
-
-err:
-	hw->wiphy->bands[NL80211_BAND_2GHZ] = NULL;
-	hw->wiphy->bands[NL80211_BAND_5GHZ] = NULL;
-	hw->wiphy->bands[NL80211_BAND_6GHZ] = NULL;
-	if (sband_2ghz)
-		kfree((__force void *)sband_2ghz->iftype_data);
-	if (sband_5ghz)
-		kfree((__force void *)sband_5ghz->iftype_data);
-	if (sband_6ghz)
-		kfree((__force void *)sband_6ghz->iftype_data);
-	kfree(sband_2ghz);
-	kfree(sband_5ghz);
-	kfree(sband_6ghz);
-	return -ENOMEM;
-}
-
-static void rtw89_core_clr_supported_band(struct rtw89_dev *rtwdev)
-{
-	struct ieee80211_hw *hw = rtwdev->hw;
-
-	if (hw->wiphy->bands[NL80211_BAND_2GHZ])
-		kfree((__force void *)hw->wiphy->bands[NL80211_BAND_2GHZ]->iftype_data);
-	if (hw->wiphy->bands[NL80211_BAND_5GHZ])
-		kfree((__force void *)hw->wiphy->bands[NL80211_BAND_5GHZ]->iftype_data);
-	if (hw->wiphy->bands[NL80211_BAND_6GHZ])
-		kfree((__force void *)hw->wiphy->bands[NL80211_BAND_6GHZ]->iftype_data);
-	kfree(hw->wiphy->bands[NL80211_BAND_2GHZ]);
-	kfree(hw->wiphy->bands[NL80211_BAND_5GHZ]);
-	kfree(hw->wiphy->bands[NL80211_BAND_6GHZ]);
-	hw->wiphy->bands[NL80211_BAND_2GHZ] = NULL;
-	hw->wiphy->bands[NL80211_BAND_5GHZ] = NULL;
-	hw->wiphy->bands[NL80211_BAND_6GHZ] = NULL;
 }
 
 static void rtw89_core_ppdu_sts_init(struct rtw89_dev *rtwdev)
@@ -5337,7 +5313,7 @@ static int rtw89_core_register_hw(struct rtw89_dev *rtwdev)
 	ret = rtw89_regd_setup(rtwdev);
 	if (ret) {
 		rtw89_err(rtwdev, "failed to set up regd\n");
-		goto err_free_supported_band;
+		return ret;
 	}
 
 	hw->wiphy->sar_capa = &rtw89_sar_capa;
@@ -5345,7 +5321,7 @@ static int rtw89_core_register_hw(struct rtw89_dev *rtwdev)
 	ret = ieee80211_register_hw(hw);
 	if (ret) {
 		rtw89_err(rtwdev, "failed to register hw\n");
-		goto err_free_supported_band;
+		return ret;
 	}
 
 	ret = rtw89_regd_init_hint(rtwdev);
@@ -5360,8 +5336,6 @@ static int rtw89_core_register_hw(struct rtw89_dev *rtwdev)
 
 err_unregister_hw:
 	ieee80211_unregister_hw(hw);
-err_free_supported_band:
-	rtw89_core_clr_supported_band(rtwdev);
 
 	return ret;
 }
@@ -5372,7 +5346,6 @@ static void rtw89_core_unregister_hw(struct rtw89_dev *rtwdev)
 
 	rtw89_rfkill_polling_deinit(rtwdev);
 	ieee80211_unregister_hw(hw);
-	rtw89_core_clr_supported_band(rtwdev);
 }
 
 int rtw89_core_register(struct rtw89_dev *rtwdev)
-- 
2.49.0

RE: [PATCH v2 1/2] wifi: rtw89: Convert rtw89_core_set_supported_band to use devm_*

[Ping-Ke Shih]

Ondřej Jirman <megi@xff.cz> wrote:
> 
> The code can be simplified by using device managed memory
> allocations. Simplify it.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>

Acked-by: Ping-Ke Shih <pkshih@realtek.com>

By the way, currently we work with NIPA in linux-wireless community [1].
Please specify rtw-next tree, when you send patch for Realtek WiFi driver
next time. i.e. "[PATCH rtw-next]"

[1] https://patchwork.kernel.org/project/linux-wireless/patch/20250429022046.1656056-2-megi@xff.cz/

[PATCH v2 2/2] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data

[Ondřej Jirman]

From: Ondrej Jirman <megi@xff.cz>

Internally wiphy writes to individual channels in this structure,
so we must not share one static definition of channel list between
multiple device instances, because that causes hard to debug
breakage.

For example, with two rtw89 driven devices in the system, channel
information may get incoherent, preventing channel use.

Signed-off-by: Ondrej Jirman <megi@xff.cz>
---
 drivers/net/wireless/realtek/rtw89/core.c | 33 +++++++++++++++++++----
 1 file changed, 28 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
index b164bc767e82..48e21a3549ff 100644
--- a/drivers/net/wireless/realtek/rtw89/core.c
+++ b/drivers/net/wireless/realtek/rtw89/core.c
@@ -4400,17 +4400,40 @@ static int rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
 	return 0;
 }
 
+static struct ieee80211_supported_band *
+rtw89_core_sband_dup(struct rtw89_dev *rtwdev,
+		     const struct ieee80211_supported_band *sband)
+{
+	struct ieee80211_supported_band *dup;
+
+	dup = devm_kmemdup(rtwdev->dev, sband, sizeof(*sband), GFP_KERNEL);
+	if (!dup)
+		return NULL;
+
+	dup->channels = devm_kmemdup(rtwdev->dev, sband->channels,
+				     sizeof(struct ieee80211_channel) * sband->n_channels,
+				     GFP_KERNEL);
+	if (!dup->channels)
+		return NULL;
+
+	dup->bitrates = devm_kmemdup(rtwdev->dev, sband->bitrates,
+				     sizeof(struct ieee80211_rate) * sband->n_bitrates,
+				     GFP_KERNEL);
+	if (!dup->bitrates)
+		return NULL;
+
+	return dup;
+}
+
 static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
 {
 	struct ieee80211_hw *hw = rtwdev->hw;
 	struct ieee80211_supported_band *sband;
-	u32 size = sizeof(struct ieee80211_supported_band);
 	u8 support_bands = rtwdev->chip->support_bands;
-	struct device *dev = rtwdev->dev;
 	int ret;
 
 	if (support_bands & BIT(NL80211_BAND_2GHZ)) {
-		sband = devm_kmemdup(dev, &rtw89_sband_2ghz, size, GFP_KERNEL);
+		sband = rtw89_core_sband_dup(rtwdev, &rtw89_sband_2ghz);
 		if (!sband)
 			return -ENOMEM;
 		rtw89_init_ht_cap(rtwdev, &sband->ht_cap);
@@ -4421,7 +4444,7 @@ static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
 	}
 
 	if (support_bands & BIT(NL80211_BAND_5GHZ)) {
-		sband = devm_kmemdup(dev, &rtw89_sband_5ghz, size, GFP_KERNEL);
+		sband = rtw89_core_sband_dup(rtwdev, &rtw89_sband_5ghz);
 		if (!sband)
 			return -ENOMEM;
 		rtw89_init_ht_cap(rtwdev, &sband->ht_cap);
@@ -4433,7 +4456,7 @@ static int rtw89_core_set_supported_band(struct rtw89_dev *rtwdev)
 	}
 
 	if (support_bands & BIT(NL80211_BAND_6GHZ)) {
-		sband = devm_kmemdup(dev, &rtw89_sband_6ghz, size, GFP_KERNEL);
+		sband = rtw89_core_sband_dup(rtwdev, &rtw89_sband_6ghz);
 		if (!sband)
 			return -ENOMEM;
 		ret = rtw89_init_he_eht_cap(rtwdev, NL80211_BAND_6GHZ, sband);
-- 
2.49.0

RE: [PATCH v2 2/2] wifi: rtw89: Fix inadverent sharing of struct ieee80211_supported_band data

[Ping-Ke Shih]

Ondřej Jirman <megi@xff.cz> wrote:
> Internally wiphy writes to individual channels in this structure,
> so we must not share one static definition of channel list between
> multiple device instances, because that causes hard to debug
> breakage.
> 
> For example, with two rtw89 driven devices in the system, channel
> information may get incoherent, preventing channel use.
> 
> Signed-off-by: Ondrej Jirman <megi@xff.cz>
> ---
>  drivers/net/wireless/realtek/rtw89/core.c | 33 +++++++++++++++++++----
>  1 file changed, 28 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/realtek/rtw89/core.c b/drivers/net/wireless/realtek/rtw89/core.c
> index b164bc767e82..48e21a3549ff 100644
> --- a/drivers/net/wireless/realtek/rtw89/core.c
> +++ b/drivers/net/wireless/realtek/rtw89/core.c
> @@ -4400,17 +4400,40 @@ static int rtw89_init_he_eht_cap(struct rtw89_dev *rtwdev,
>         return 0;
>  }
> 
> +static struct ieee80211_supported_band *
> +rtw89_core_sband_dup(struct rtw89_dev *rtwdev,
> +                    const struct ieee80211_supported_band *sband)
> +{
> +       struct ieee80211_supported_band *dup;
> +
> +       dup = devm_kmemdup(rtwdev->dev, sband, sizeof(*sband), GFP_KERNEL);
> +       if (!dup)
> +               return NULL;
> +
> +       dup->channels = devm_kmemdup(rtwdev->dev, sband->channels,
> +                                    sizeof(struct ieee80211_channel) * sband->n_channels,

sizeof(*sband->channels) * sband->n_channels,

> +                                    GFP_KERNEL);
> +       if (!dup->channels)
> +               return NULL;
> +
> +       dup->bitrates = devm_kmemdup(rtwdev->dev, sband->bitrates,
> +                                    sizeof(struct ieee80211_rate) * sband->n_bitrates,

sizeof(*sband->bitrates) * sband->n_bitrates,

> +                                    GFP_KERNEL);
> +       if (!dup->bitrates)
> +               return NULL;
> +
> +       return dup;
> +}
> +

[...]

RE: [PATCH v2 0/2] Fix inadverent sharing of struct ieee80211_supported_band data

[Ping-Ke Shih]

Ondřej Jirman <megi@xff.cz> wrote:
> 
> This is a series of patches requested by Ping-Ke Shih in response to
> https://lore.kernel.org/lkml/20250427002414.410791-1-megi@xff.cz/
> 
> Please take a look.
> 
> (hw->wiphy->bands[*] are no longer being NULLed when probe fails and on
> remove(), but I guess that should not be an issue? I tried unbinding the
> device and it worked fine without any crash)

The original code set bands[] to NULL, because the error path could call
free function twice, so set NULL to prevent double free. After using
devm_ series, it becomes unnecessary.