diff mbox series

wifi: mac80211: use __counted_by for the rest of flexible array members

Message ID 20231127112601.42636-1-dmantipov@yandex.ru (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series wifi: mac80211: use __counted_by for the rest of flexible array members | expand

Commit Message

Dmitry Antipov Nov. 27, 2023, 11:25 a.m. UTC 
Following commit 9118796dfa67 ("wifi: mac80211: Add __counted_by for struct
ieee802_11_elems and use struct_size()"), use an incoming '__counted_by()'
attribute for the flexible array members of 'struct probe_resp', 'struct
fils_discovery_data', 'struct unsol_bcast_probe_resp_data', 'struct
ieee80211_mgd_auth_data' and 'struct ieee80211_mgd_assoc_data', as well as
'struct_size()' helper to allocate an instances of them where appropriate.
This also introduces reordering of statements in 'ieee80211_mgd_auth()'
and 'ieee80211_mgd_assoc()' because the counter field should (is better
to?) be adjusted before touching the corresponding '__counted_by()' area.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 net/mac80211/cfg.c         |  7 ++++---
 net/mac80211/ieee80211_i.h | 10 +++++-----
 net/mac80211/mlme.c        | 17 +++++++++--------
 3 files changed, 18 insertions(+), 16 deletions(-)

Comments

Gustavo A. R. Silva Nov. 27, 2023, 6:57 p.m. UTC | #1 
On 11/27/23 05:25, Dmitry Antipov wrote:
> Following commit 9118796dfa67 ("wifi: mac80211: Add __counted_by for struct
> ieee802_11_elems and use struct_size()"), use an incoming '__counted_by()'
> attribute for the flexible array members of 'struct probe_resp', 'struct
> fils_discovery_data', 'struct unsol_bcast_probe_resp_data', 'struct
> ieee80211_mgd_auth_data' and 'struct ieee80211_mgd_assoc_data', as well as
> 'struct_size()' helper to allocate an instances of them where appropriate.
> This also introduces reordering of statements in 'ieee80211_mgd_auth()'
> and 'ieee80211_mgd_assoc()' because the counter field should (is better
> to?) be adjusted before touching the corresponding '__counted_by()' area.
> 
> Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
> ---
>   net/mac80211/cfg.c         |  7 ++++---
>   net/mac80211/ieee80211_i.h | 10 +++++-----
>   net/mac80211/mlme.c        | 17 +++++++++--------
>   3 files changed, 18 insertions(+), 16 deletions(-)

[..]

>   struct ieee80211_sta_tx_tspec {
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index 887b496f2b81..41a4719fb413 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -7322,8 +7322,9 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
>   	}
>   	rcu_read_unlock();
>   
> -	auth_data = kzalloc(sizeof(*auth_data) + req->auth_data_len +
> -			    req->ie_len, GFP_KERNEL);
> +	auth_data = kzalloc(struct_size(auth_data, data,
> +					req->auth_data_len +
> +					req->ie_len), GFP_KERNEL);

I think we can use size_add() here.

>   	if (!auth_data)
>   		return -ENOMEM;
>   
> @@ -7340,9 +7341,9 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
>   			auth_data->sae_trans = le16_to_cpu(pos[0]);
>   			auth_data->sae_status = le16_to_cpu(pos[1]);
>   		}
> +		auth_data->data_len += req->auth_data_len - 4;
>   		memcpy(auth_data->data, req->auth_data + 4,
>   		       req->auth_data_len - 4);
> -		auth_data->data_len += req->auth_data_len - 4;
>   	}
>   
>   	/* Check if continuing authentication or trying to authenticate with the
> @@ -7354,9 +7355,9 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
>   		    ifmgd->auth_data->link_id == req->link_id;
>   
>   	if (req->ie && req->ie_len) {
> +		auth_data->data_len += req->ie_len;
>   		memcpy(&auth_data->data[auth_data->data_len],

This introduces a bug. Now, memcpy() will copy data into `data[]` after the
offset is updated, which will cause an overwrite bug.

 From a piece of code above it seems that

if (req->auth_data_len >= 4)
	memcpy(&auth_data->data[req->auth_data_len - 4], ..);
else
	memcpy(&auth_data->data[0], ..);

So, we should probably use an auxiliary variable like this

+               auth_data->data_len += req->auth_data_len - 4;
                 memcpy(auth_data->data, req->auth_data + 4,
                        req->auth_data_len - 4);
-               auth_data->data_len += req->auth_data_len - 4;
         }
+       aux_len = auth_data->data_len;

and then

         if (req->ie && req->ie_len) {
-               memcpy(&auth_data->data[auth_data->data_len],
-                      req->ie, req->ie_len);
                 auth_data->data_len += req->ie_len;
+               memcpy(&auth_data->data[aux_len], req->ie, req->ie_len);
         }


--
Gustavo

>   		       req->ie, req->ie_len);
> -		auth_data->data_len += req->ie_len;
>   	}
>   
>   	if (req->key && req->key_len) {
> @@ -7637,16 +7638,16 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
>   	struct ieee80211_bss *bss;
>   	bool override;
>   	int i, err;
> -	size_t size = sizeof(*assoc_data) + req->ie_len;
> +	size_t extra = req->ie_len;
>   
>   	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
> -		size += req->links[i].elems_len;
> +		extra += req->links[i].elems_len;
>   
>   	/* FIXME: no support for 4-addr MLO yet */
>   	if (sdata->u.mgd.use_4addr && req->link_id >= 0)
>   		return -EOPNOTSUPP;
>   
> -	assoc_data = kzalloc(size, GFP_KERNEL);
> +	assoc_data = kzalloc(struct_size(assoc_data, ie, extra), GFP_KERNEL);
>   	if (!assoc_data)
>   		return -ENOMEM;
>   
> @@ -7811,8 +7812,8 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
>   	       sizeof(ifmgd->s1g_capa_mask));
>   
>   	if (req->ie && req->ie_len) {
> -		memcpy(assoc_data->ie, req->ie, req->ie_len);
>   		assoc_data->ie_len = req->ie_len;
> +		memcpy(assoc_data->ie, req->ie, req->ie_len);
>   		assoc_data->ie_pos = assoc_data->ie + assoc_data->ie_len;
>   	} else {
>   		assoc_data->ie_pos = assoc_data->ie;
Johannes Berg Nov. 27, 2023, 9:16 p.m. UTC | #2 
On Mon, 2023-11-27 at 14:25 +0300, Dmitry Antipov wrote:
> Following commit 9118796dfa67 ("wifi: mac80211: Add __counted_by for struct
> ieee802_11_elems and use struct_size()"), use an incoming '__counted_by()'
> attribute for the flexible array members of 'struct probe_resp', 'struct
> fils_discovery_data', 'struct unsol_bcast_probe_resp_data', 'struct
> ieee80211_mgd_auth_data' and 'struct ieee80211_mgd_assoc_data', as well as
> 'struct_size()' helper to allocate an instances of them where appropriate.
> This also introduces reordering of statements in 'ieee80211_mgd_auth()'
> and 'ieee80211_mgd_assoc()' because the counter field should (is better
> to?) be adjusted before touching the corresponding '__counted_by()' area.

For the record, regardless of the commments from Gustavo, I'm going to
put this on the back of the list because we have various things in
flight now that touch these areas, particularly mgd_auth/mgd_assoc and
related code. Not convinced that it's worth making more work on other
things for this, when I don't even think (released) compilers have
support yet.

So just FYI, you might want to come back with this in a month or two
when hopefully other things have settled.

johannes
diff mbox series

Patch

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 606b1b2e4123..5eb95fe0aa78 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -929,7 +929,7 @@  ieee80211_set_probe_resp(struct ieee80211_sub_if_data *sdata,
 
 	old = sdata_dereference(link->u.ap.probe_resp, sdata);
 
-	new = kzalloc(sizeof(struct probe_resp) + resp_len, GFP_KERNEL);
+	new = kzalloc(struct_size(new, data, resp_len), GFP_KERNEL);
 	if (!new)
 		return -ENOMEM;
 
@@ -970,7 +970,8 @@  static int ieee80211_set_fils_discovery(struct ieee80211_sub_if_data *sdata,
 		kfree_rcu(old, rcu_head);
 
 	if (params->tmpl && params->tmpl_len) {
-		new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
+		new = kzalloc(struct_size(new, data, params->tmpl_len),
+			      GFP_KERNEL);
 		if (!new)
 			return -ENOMEM;
 		new->len = params->tmpl_len;
@@ -1001,7 +1002,7 @@  ieee80211_set_unsol_bcast_probe_resp(struct ieee80211_sub_if_data *sdata,
 		kfree_rcu(old, rcu_head);
 
 	if (params->tmpl && params->tmpl_len) {
-		new = kzalloc(sizeof(*new) + params->tmpl_len, GFP_KERNEL);
+		new = kzalloc(struct_size(new, data, params->tmpl_len), GFP_KERNEL);
 		if (!new)
 			return -ENOMEM;
 		new->len = params->tmpl_len;
diff --git a/net/mac80211/ieee80211_i.h b/net/mac80211/ieee80211_i.h
index 84df104f272b..7ac3ce4e1070 100644
--- a/net/mac80211/ieee80211_i.h
+++ b/net/mac80211/ieee80211_i.h
@@ -278,19 +278,19 @@  struct probe_resp {
 	struct rcu_head rcu_head;
 	int len;
 	u16 cntdwn_counter_offsets[IEEE80211_MAX_CNTDWN_COUNTERS_NUM];
-	u8 data[];
+	u8 data[] __counted_by(len);
 };
 
 struct fils_discovery_data {
 	struct rcu_head rcu_head;
 	int len;
-	u8 data[];
+	u8 data[] __counted_by(len);
 };
 
 struct unsol_bcast_probe_resp_data {
 	struct rcu_head rcu_head;
 	int len;
-	u8 data[];
+	u8 data[] __counted_by(len);
 };
 
 struct ps_data {
@@ -397,7 +397,7 @@  struct ieee80211_mgd_auth_data {
 
 	u16 sae_trans, sae_status;
 	size_t data_len;
-	u8 data[];
+	u8 data[] __counted_by(data_len);
 };
 
 struct ieee80211_mgd_assoc_data {
@@ -446,7 +446,7 @@  struct ieee80211_mgd_assoc_data {
 
 	size_t ie_len;
 	u8 *ie_pos; /* used to fill ie[] with link[].elems */
-	u8 ie[];
+	u8 ie[] __counted_by(ie_len);
 };
 
 struct ieee80211_sta_tx_tspec {
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 887b496f2b81..41a4719fb413 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -7322,8 +7322,9 @@  int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 	}
 	rcu_read_unlock();
 
-	auth_data = kzalloc(sizeof(*auth_data) + req->auth_data_len +
-			    req->ie_len, GFP_KERNEL);
+	auth_data = kzalloc(struct_size(auth_data, data,
+					req->auth_data_len +
+					req->ie_len), GFP_KERNEL);
 	if (!auth_data)
 		return -ENOMEM;
 
@@ -7340,9 +7341,9 @@  int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 			auth_data->sae_trans = le16_to_cpu(pos[0]);
 			auth_data->sae_status = le16_to_cpu(pos[1]);
 		}
+		auth_data->data_len += req->auth_data_len - 4;
 		memcpy(auth_data->data, req->auth_data + 4,
 		       req->auth_data_len - 4);
-		auth_data->data_len += req->auth_data_len - 4;
 	}
 
 	/* Check if continuing authentication or trying to authenticate with the
@@ -7354,9 +7355,9 @@  int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 		    ifmgd->auth_data->link_id == req->link_id;
 
 	if (req->ie && req->ie_len) {
+		auth_data->data_len += req->ie_len;
 		memcpy(&auth_data->data[auth_data->data_len],
 		       req->ie, req->ie_len);
-		auth_data->data_len += req->ie_len;
 	}
 
 	if (req->key && req->key_len) {
@@ -7637,16 +7638,16 @@  int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_bss *bss;
 	bool override;
 	int i, err;
-	size_t size = sizeof(*assoc_data) + req->ie_len;
+	size_t extra = req->ie_len;
 
 	for (i = 0; i < IEEE80211_MLD_MAX_NUM_LINKS; i++)
-		size += req->links[i].elems_len;
+		extra += req->links[i].elems_len;
 
 	/* FIXME: no support for 4-addr MLO yet */
 	if (sdata->u.mgd.use_4addr && req->link_id >= 0)
 		return -EOPNOTSUPP;
 
-	assoc_data = kzalloc(size, GFP_KERNEL);
+	assoc_data = kzalloc(struct_size(assoc_data, ie, extra), GFP_KERNEL);
 	if (!assoc_data)
 		return -ENOMEM;
 
@@ -7811,8 +7812,8 @@  int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 	       sizeof(ifmgd->s1g_capa_mask));
 
 	if (req->ie && req->ie_len) {
-		memcpy(assoc_data->ie, req->ie, req->ie_len);
 		assoc_data->ie_len = req->ie_len;
+		memcpy(assoc_data->ie, req->ie, req->ie_len);
 		assoc_data->ie_pos = assoc_data->ie + assoc_data->ie_len;
 	} else {
 		assoc_data->ie_pos = assoc_data->ie;