Re: [PATCH wireless-next] wifi: cfg80211: Nullify freed pointers in cfg80211_sinfo_release_content()

[Dan Carpenter <dan.carpenter@xxxxxxxxxx>]

On Wed, Jul 02, 2025 at 06:41:31PM +0200, Johannes Berg wrote:
> On Wed, 2025-07-02 at 21:55 +0530, Sarika Sharma wrote:
> > Currently, cfg80211_sinfo_release_content() frees dynamically
> > allocated memory but does not reset the associated pointers.
> > This results in double free issues in nl80211_dump_station(),
> > where both link_sinfo and link_sinfo->pertid are released twice,
> > once after the send_station() call and again in the error handling path.
> > 
> > Hence, to fix accidental dereferencing of dangling pointers, explicitly
> > set the freed pointers to NULL.
> > 
> 
> Do we have to fix it this way? It feels like perhaps it should rather be
> fixed by only having one call to cfg80211_sinfo_release_content() in
> each path.

That was my bad.  I suggested it because the diff from changing the
callers was a bit larger.  Technically I suggested just memsetting the
struct to zero, but that approach doesn't work because of
batadv_v_elp_get_throughput().

Re-working the callers is totally doable.  It would look something like
the diff below.

regards,
dan carpenter

diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index 70ca74a75f22..2066aefc05c7 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -6996,10 +6996,8 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 	int link_id;
 
 	hdr = nl80211hdr_put(msg, portid, seq, flags, cmd);
-	if (!hdr) {
-		cfg80211_sinfo_release_content(sinfo);
+	if (!hdr)
 		return -1;
-	}
 
 	if (nla_put_u32(msg, NL80211_ATTR_IFINDEX, dev->ifindex) ||
 	    nla_put(msg, NL80211_ATTR_MAC, ETH_ALEN, mac_addr) ||
@@ -7247,7 +7245,6 @@ static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 	return 0;
 
  nla_put_failure:
-	cfg80211_sinfo_release_content(sinfo);
 	genlmsg_cancel(msg, hdr);
 	return -EMSGSIZE;
 }
@@ -7530,34 +7527,38 @@ static int nl80211_get_station(struct sk_buff *skb, struct genl_info *info)
 	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++) {
 		sinfo.links[i] = kzalloc(sizeof(*sinfo.links[0]), GFP_KERNEL);
 		if (!sinfo.links[i]) {
-			cfg80211_sinfo_release_content(&sinfo);
-			return -ENOMEM;
+			err = -ENOMEM;
+			goto err_free_sinfo;
 		}
 	}
 
 	err = rdev_get_station(rdev, dev, mac_addr, &sinfo);
-	if (err) {
-		cfg80211_sinfo_release_content(&sinfo);
-		return err;
-	}
+	if (err)
+		goto err_free_sinfo;
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
 	if (!msg) {
-		cfg80211_sinfo_release_content(&sinfo);
-		return -ENOMEM;
+		err = -ENOMEM;
+		goto err_free_sinfo;
 	}
 
 	if (sinfo.valid_links)
 		cfg80211_sta_set_mld_sinfo(&sinfo);
 
-	if (nl80211_send_station(msg, NL80211_CMD_NEW_STATION,
+	err = nl80211_send_station(msg, NL80211_CMD_NEW_STATION,
 				 info->snd_portid, info->snd_seq, 0,
-				 rdev, dev, mac_addr, &sinfo) < 0) {
-		nlmsg_free(msg);
-		return -ENOBUFS;
-	}
+				 rdev, dev, mac_addr, &sinfo);
+	if (err < 0)
+		goto err_free_msg;
 
 	return genlmsg_reply(msg, info);
+
+err_free_msg:
+	nlmsg_free(msg);
+err_free_sinfo:
+	cfg80211_sinfo_release_content(&sinfo);
+
+	return err;
 }
 
 int cfg80211_check_station_change(struct wiphy *wiphy,
@@ -19558,11 +19559,15 @@ void cfg80211_new_sta(struct net_device *dev, const u8 *mac_addr,
 	trace_cfg80211_new_sta(dev, mac_addr, sinfo);
 
 	msg = nlmsg_new(NLMSG_DEFAULT_SIZE, gfp);
-	if (!msg)
+	if (!msg) {
+		// This bugfix needs a mention in the commit message
+		cfg80211_sinfo_release_content(sinfo);
 		return;
+	}
 
 	if (nl80211_send_station(msg, NL80211_CMD_NEW_STATION, 0, 0, 0,
 				 rdev, dev, mac_addr, sinfo) < 0) {
+		cfg80211_sinfo_release_content(sinfo);
 		nlmsg_free(msg);
 		return;
 	}
@@ -19593,6 +19598,7 @@ void cfg80211_del_sta_sinfo(struct net_device *dev, const u8 *mac_addr,
 
 	if (nl80211_send_station(msg, NL80211_CMD_DEL_STATION, 0, 0, 0,
 				 rdev, dev, mac_addr, sinfo) < 0) {
+		cfg80211_sinfo_release_content(sinfo);
 		nlmsg_free(msg);
 		return;
 	}