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)

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)

[Kalle Valo]

(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.

-- 
Kalle Valo

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

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)

[John Crispin]

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

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)

[Kalle Valo]

John Crispin <john@phrozen.org> writes:

> 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

Ah, good.

-- 
Kalle Valo

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

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)

[John Crispin]

On 18/06/2019 13:47, Dan Carpenter wrote:
> On Tue, Jun 18, 2019 at 08:59:55AM +0200, John Crispin wrote:
>> 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.
>>
> I did download the latest git.  The problem is that
> he_cap->he_cap_elem.mac_cap_info[] is six bytes and arg->peer_he_cap_macinfo[]
> is two u32s or eight bytes.  So we are reading beyond the end of the
> array.
>
> regards,
> dan carpenter

correct, thanks for the catch, will send a fix to kalle

     John



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