Re: [ath6kl:pending-ath11k 198/205] drivers/net/wireless/ath/ath11k/mac.c:1274 ath11k_peer_assoc_h_he() error: memcpy() 'he_cap->he_cap_elem.mac_cap_info' too small (6 vs 8)

From: John Crispin <john@phrozen.org>
To: ath11k@lists.infradead.org
Subject: Re: [ath6kl:pending-ath11k 198/205] drivers/net/wireless/ath/ath11k/mac.c:1274 ath11k_peer_assoc_h_he() error: memcpy() 'he_cap->he_cap_elem.mac_cap_info' too small (6 vs 8)
Date: Tue, 18 Jun 2019 13:13:54 +0200	[thread overview]
Message-ID: <448c4cf0-9469-8ca3-8334-e56ec98377a3@phrozen.org> (raw)
In-Reply-To: <87r27r9mqd.fsf@kamboji.qca.qualcomm.com>

On 18/06/2019 13:06, Kalle Valo wrote:
> (moving from ath10k to ath11k list)
>
> John Crispin <john@phrozen.org> writes:
>
>> On 18/06/2019 08:53, kbuild test robot wrote:
>>> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git pending-ath11k
>>> head:   0f82fec5679664bb91d6c167fd1a146f113e4197
>>> commit: cbdb3159fdf450b7b3999a06600aa0e1fb78383f [198/205] ath11k:
>>> set additional values inside wmi_peer_assoc_complete_cmd
>>>
>>> If you fix the issue, kindly add following tag
>>> Reported-by: kbuild test robot <lkp@intel.com>
>>> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> New smatch warnings:
>>> drivers/net/wireless/ath/ath11k/mac.c:1274 ath11k_peer_assoc_h_he()
>>> error: memcpy() 'he_cap->he_cap_elem.mac_cap_info' too small (6 vs
>>> 8)
>>>
>>> Old smatch warnings:
>>> drivers/net/wireless/ath/ath11k/mac.c:1276 ath11k_peer_assoc_h_he()
>>> error: memcpy() 'he_cap->he_cap_elem.phy_cap_info' too small (11 vs
>>> 12)
>>>
>>> #
>>> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?id=cbdb3159fdf450b7b3999a06600aa0e1fb78383f
>>> git remote add ath6kl https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git
>>> git remote update ath6kl
>>> git checkout cbdb3159fdf450b7b3999a06600aa0e1fb78383f
>>> vim +1274 drivers/net/wireless/ath/ath11k/mac.c
>>>
>>> 258bbf52 Kalle Valo   2019-02-05  1260
>>> 258bbf52 Kalle Valo 2019-02-05 1261 static void
>>> ath11k_peer_assoc_h_he(struct ath11k *ar,
>>> 258bbf52 Kalle Valo 2019-02-05 1262 struct ieee80211_vif *vif,
>>> 258bbf52 Kalle Valo 2019-02-05 1263 struct ieee80211_sta *sta,
>>> 258bbf52 Kalle Valo 2019-02-05 1264 struct peer_assoc_params *arg)
>>> 258bbf52 Kalle Valo   2019-02-05  1265  {
>>> 17aca2d9 John Crispin 2019-06-03 1266 const struct
>>> ieee80211_sta_he_cap *he_cap = &sta->he_cap;
>>> 3db59a23 Kalle Valo   2019-06-12  1267  	u16 v;
>>> 17aca2d9 John Crispin 2019-06-03  1268
>>> 17aca2d9 John Crispin 2019-06-03  1269  	if (!he_cap->has_he)
>>> 17aca2d9 John Crispin 2019-06-03  1270  		return;
>>> 17aca2d9 John Crispin 2019-06-03  1271
>>> 17aca2d9 John Crispin 2019-06-03  1272  	arg->he_flag = true;
>>> 17aca2d9 John Crispin 2019-06-03  1273
>>> 17aca2d9 John Crispin 2019-06-03 @1274
>>> memcpy(&arg->peer_he_cap_macinfo, he_cap->he_cap_elem.mac_cap_info,
>>> 17aca2d9 John Crispin 2019-06-03 1275
>>> sizeof(arg->peer_he_cap_macinfo));
>>>
>>> Smatch thinks these are different sizes...  I don't have a copy of
>>> struct peer_assoc_params so I can't check.
>> Hi,
>>
>> its he_cap->he_cap_elem.mac_cap_info[6] and
>> arg->peer_he_cap_macinfo[2] and we only copy the first 2 elements as
>> the FW only cares for the first 2 bytes.
> Are the remaining bytes to zero? (ie. does it follow "the reserved
> fields should be zero" rule). In general this makes it safer to have
> changes in firmware interface in the future.

We copy the first 2 bytes of a 6 byte buffer into a 2 byte buffer. these 
2 bytes are later copied into 2 non-consecutive fields inside a WMI cmd

_______________________________________________
ath11k mailing list
ath11k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath11k