Received-SPF: pass (google.com: domain of linux-wireless+bounces-124-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <d556055f-64f0-4f98-9d1a-6ea1f74f8461@embeddedor.com>
Date: Mon, 27 Nov 2023 12:57:25 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] wifi: mac80211: use __counted_by for the rest of flexible
 array members
Content-Language: en-US
To: Dmitry Antipov <dmantipov@yandex.ru>,
 Johannes Berg <johannes@sipsolutions.net>
Cc: "Gustavo A . R . Silva" <gustavoars@kernel.org>,
 linux-wireless@vger.kernel.org
References: <20231127112601.42636-1-dmantipov@yandex.ru>
From: "Gustavo A. R. Silva" <gustavo@embeddedor.com>
In-Reply-To: <20231127112601.42636-1-dmantipov@yandex.ru>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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;