* [bug report] staging: Add rtl8723bs sdio wifi driver
@ 2021-10-05 9:06 Dan Carpenter
2021-10-10 10:45 ` Fabio Aiuto
0 siblings, 1 reply; 3+ messages in thread
From: Dan Carpenter @ 2021-10-05 9:06 UTC (permalink / raw)
To: hdegoede; +Cc: linux-staging
Hello Hans de Goede,
The patch 554c0a3abf21: "staging: Add rtl8723bs sdio wifi driver"
from Mar 29, 2017, leads to the following Smatch static checker
warnings:
drivers/staging/rtl8723bs/core/rtw_security.c:1404 rtw_BIP_verify()
warn: not copying enough bytes for '&le_tmp64' (8 vs 6 bytes)
drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4058 collect_bss_info()
warn: not copying enough bytes for '&le32_tmp' (4 vs 2 bytes)
drivers/staging/rtl8723bs/core/rtw_security.c
1372 u32 rtw_BIP_verify(struct adapter *padapter, u8 *precvframe)
1373 {
1374 struct rx_pkt_attrib *pattrib = &((union recv_frame *)precvframe)->u.hdr.attrib;
1375 u8 *pframe;
1376 u8 *BIP_AAD, *p;
1377 u32 res = _FAIL;
1378 uint len, ori_len;
1379 struct ieee80211_hdr *pwlanhdr;
1380 u8 mic[16];
1381 struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
1382 __le16 le_tmp;
1383 __le64 le_tmp64;
^^^^^^^^^^^^^^^
1384
1385 ori_len = pattrib->pkt_len-WLAN_HDR_A3_LEN+BIP_AAD_SIZE;
1386 BIP_AAD = rtw_zmalloc(ori_len);
1387
1388 if (!BIP_AAD)
1389 return _FAIL;
1390
1391 /* PKT start */
1392 pframe = (unsigned char *)((union recv_frame *)precvframe)->u.hdr.rx_data;
1393 /* mapping to wlan header */
1394 pwlanhdr = (struct ieee80211_hdr *)pframe;
1395 /* save the frame body + MME */
1396 memcpy(BIP_AAD+BIP_AAD_SIZE, pframe+WLAN_HDR_A3_LEN, pattrib->pkt_len-WLAN_HDR_A3_LEN);
1397 /* find MME IE pointer */
1398 p = rtw_get_ie(BIP_AAD+BIP_AAD_SIZE, WLAN_EID_MMIE, &len, pattrib->pkt_len-WLAN_HDR_A3_LEN);
1399 /* Baron */
1400 if (p) {
1401 u16 keyid = 0;
1402 u64 temp_ipn = 0;
1403 /* save packet number */
--> 1404 memcpy(&le_tmp64, p+4, 6);
^^^^^^^^^^^^^^^^^
1405 temp_ipn = le64_to_cpu(le_tmp64);
^^^^^^^^
This code is copying 6 bytes into a u64 and then treating it as little
endian data. The problem is that the last two bytes are uninitialized
garbage data. I think if we set "__le64 le_tmp64 = 0;" at the top that
would probably work, right?
I could have sent a patch but this code is weird enough that I was
hoping for a second opinion.
The bug in collect_bss_info() is a similar uninitialized data issue.
1406 /* BIP packet number should bigger than previous BIP packet */
1407 if (temp_ipn <= pmlmeext->mgnt_80211w_IPN_rx)
1408 goto BIP_exit;
1409
1410 /* copy key index */
1411 memcpy(&le_tmp, p+2, 2);
But this part seems totally wrong again because we haven't incremented
p. p + 10?
1412 keyid = le16_to_cpu(le_tmp);
1413 if (keyid != padapter->securitypriv.dot11wBIPKeyid)
1414 goto BIP_exit;
1415
1416 /* clear the MIC field of MME to zero */
1417 memset(p+2+len-8, 0, 8);
1418
1419 /* conscruct AAD, copy frame control field */
1420 memcpy(BIP_AAD, &pwlanhdr->frame_control, 2);
1421 ClearRetry(BIP_AAD);
1422 ClearPwrMgt(BIP_AAD);
1423 ClearMData(BIP_AAD);
1424 /* conscruct AAD, copy address 1 to address 3 */
1425 memcpy(BIP_AAD+2, pwlanhdr->addr1, 18);
1426
1427 if (omac1_aes_128(padapter->securitypriv.dot11wBIPKey[padapter->securitypriv.dot11wBIPKeyid].skey
1428 , BIP_AAD, ori_len, mic))
1429 goto BIP_exit;
1430
1431 /* MIC field should be last 8 bytes of packet (packet without FCS) */
1432 if (!memcmp(mic, pframe+pattrib->pkt_len-8, 8)) {
1433 pmlmeext->mgnt_80211w_IPN_rx = temp_ipn;
1434 res = _SUCCESS;
1435 } else {
1436 }
1437
1438 } else {
1439 res = RTW_RX_HANDLED;
1440 }
1441 BIP_exit:
1442
1443 kfree(BIP_AAD);
1444 return res;
1445 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] staging: Add rtl8723bs sdio wifi driver
2021-10-05 9:06 [bug report] staging: Add rtl8723bs sdio wifi driver Dan Carpenter
@ 2021-10-10 10:45 ` Fabio Aiuto
2021-10-11 12:50 ` Dan Carpenter
0 siblings, 1 reply; 3+ messages in thread
From: Fabio Aiuto @ 2021-10-10 10:45 UTC (permalink / raw)
To: Dan Carpenter; +Cc: hdegoede, linux-staging
Hi Dan,
thank you for your report,
On Tue, Oct 05, 2021 at 12:06:46PM +0300, Dan Carpenter wrote:
> Hello Hans de Goede,
>
> The patch 554c0a3abf21: "staging: Add rtl8723bs sdio wifi driver"
> from Mar 29, 2017, leads to the following Smatch static checker
> warnings:
>
> drivers/staging/rtl8723bs/core/rtw_security.c:1404 rtw_BIP_verify()
> warn: not copying enough bytes for '&le_tmp64' (8 vs 6 bytes)
>
> drivers/staging/rtl8723bs/core/rtw_mlme_ext.c:4058 collect_bss_info()
> warn: not copying enough bytes for '&le32_tmp' (4 vs 2 bytes)
>
> drivers/staging/rtl8723bs/core/rtw_security.c
> 1372 u32 rtw_BIP_verify(struct adapter *padapter, u8 *precvframe)
> 1373 {
> 1374 struct rx_pkt_attrib *pattrib = &((union recv_frame *)precvframe)->u.hdr.attrib;
> 1375 u8 *pframe;
> 1376 u8 *BIP_AAD, *p;
> 1377 u32 res = _FAIL;
> 1378 uint len, ori_len;
> 1379 struct ieee80211_hdr *pwlanhdr;
> 1380 u8 mic[16];
> 1381 struct mlme_ext_priv *pmlmeext = &padapter->mlmeextpriv;
> 1382 __le16 le_tmp;
> 1383 __le64 le_tmp64;
> ^^^^^^^^^^^^^^^
>
> 1384
> 1385 ori_len = pattrib->pkt_len-WLAN_HDR_A3_LEN+BIP_AAD_SIZE;
> 1386 BIP_AAD = rtw_zmalloc(ori_len);
> 1387
> 1388 if (!BIP_AAD)
> 1389 return _FAIL;
> 1390
> 1391 /* PKT start */
> 1392 pframe = (unsigned char *)((union recv_frame *)precvframe)->u.hdr.rx_data;
> 1393 /* mapping to wlan header */
> 1394 pwlanhdr = (struct ieee80211_hdr *)pframe;
> 1395 /* save the frame body + MME */
> 1396 memcpy(BIP_AAD+BIP_AAD_SIZE, pframe+WLAN_HDR_A3_LEN, pattrib->pkt_len-WLAN_HDR_A3_LEN);
> 1397 /* find MME IE pointer */
> 1398 p = rtw_get_ie(BIP_AAD+BIP_AAD_SIZE, WLAN_EID_MMIE, &len, pattrib->pkt_len-WLAN_HDR_A3_LEN);
> 1399 /* Baron */
> 1400 if (p) {
> 1401 u16 keyid = 0;
> 1402 u64 temp_ipn = 0;
> 1403 /* save packet number */
> --> 1404 memcpy(&le_tmp64, p+4, 6);
> ^^^^^^^^^^^^^^^^^
> 1405 temp_ipn = le64_to_cpu(le_tmp64);
> ^^^^^^^^
> This code is copying 6 bytes into a u64 and then treating it as little
> endian data. The problem is that the last two bytes are uninitialized
> garbage data. I think if we set "__le64 le_tmp64 = 0;" at the top that
> would probably work, right?
>
> I could have sent a patch but this code is weird enough that I was
> hoping for a second opinion.
>
> The bug in collect_bss_info() is a similar uninitialized data issue.
You are right I think that it's safer to intitalize to zero
le_tmp64 and le32_tmp.
>
> 1406 /* BIP packet number should bigger than previous BIP packet */
> 1407 if (temp_ipn <= pmlmeext->mgnt_80211w_IPN_rx)
> 1408 goto BIP_exit;
> 1409
> 1410 /* copy key index */
> 1411 memcpy(&le_tmp, p+2, 2);
>
> But this part seems totally wrong again because we haven't incremented
> p. p + 10?
I don't know what you mean. I guess that you are adressing the code above
(lines 1406-1411).
Anyway I think the code it's right. MMIE layout is:
1 byte element_id;
1 byte length;
2 byte key_id;
6 byte IPN;
8 byte MIC;
so to access key_id I have to increment p by 2.
>
> 1412 keyid = le16_to_cpu(le_tmp);
> 1413 if (keyid != padapter->securitypriv.dot11wBIPKeyid)
> 1414 goto BIP_exit;
> 1415
> 1416 /* clear the MIC field of MME to zero */
> 1417 memset(p+2+len-8, 0, 8);
> 1418
> 1419 /* conscruct AAD, copy frame control field */
> 1420 memcpy(BIP_AAD, &pwlanhdr->frame_control, 2);
> 1421 ClearRetry(BIP_AAD);
> 1422 ClearPwrMgt(BIP_AAD);
> 1423 ClearMData(BIP_AAD);
> 1424 /* conscruct AAD, copy address 1 to address 3 */
> 1425 memcpy(BIP_AAD+2, pwlanhdr->addr1, 18);
> 1426
> 1427 if (omac1_aes_128(padapter->securitypriv.dot11wBIPKey[padapter->securitypriv.dot11wBIPKeyid].skey
> 1428 , BIP_AAD, ori_len, mic))
> 1429 goto BIP_exit;
> 1430
> 1431 /* MIC field should be last 8 bytes of packet (packet without FCS) */
> 1432 if (!memcmp(mic, pframe+pattrib->pkt_len-8, 8)) {
> 1433 pmlmeext->mgnt_80211w_IPN_rx = temp_ipn;
> 1434 res = _SUCCESS;
> 1435 } else {
> 1436 }
> 1437
> 1438 } else {
> 1439 res = RTW_RX_HANDLED;
> 1440 }
> 1441 BIP_exit:
> 1442
> 1443 kfree(BIP_AAD);
> 1444 return res;
> 1445 }
>
> regards,
> dan carpenter
>
thank you,
fabio
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [bug report] staging: Add rtl8723bs sdio wifi driver
2021-10-10 10:45 ` Fabio Aiuto
@ 2021-10-11 12:50 ` Dan Carpenter
0 siblings, 0 replies; 3+ messages in thread
From: Dan Carpenter @ 2021-10-11 12:50 UTC (permalink / raw)
To: Fabio Aiuto; +Cc: hdegoede, linux-staging
On Sun, Oct 10, 2021 at 12:45:21PM +0200, Fabio Aiuto wrote:
> > 1406 /* BIP packet number should bigger than previous BIP packet */
> > 1407 if (temp_ipn <= pmlmeext->mgnt_80211w_IPN_rx)
> > 1408 goto BIP_exit;
> > 1409
> > 1410 /* copy key index */
> > 1411 memcpy(&le_tmp, p+2, 2);
> >
> > But this part seems totally wrong again because we haven't incremented
> > p. p + 10?
>
> I don't know what you mean. I guess that you are adressing the code above
> (lines 1406-1411).
>
> Anyway I think the code it's right. MMIE layout is:
>
> 1 byte element_id;
> 1 byte length;
> 2 byte key_id;
> 6 byte IPN;
> 8 byte MIC;
>
> so to access key_id I have to increment p by 2.
>
Oh, yes. You're right. I think I got confused because we are not
parsing the MMIE layout in order. 1, 2, 3 4. It goes 4 then 2.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-10-11 12:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-05 9:06 [bug report] staging: Add rtl8723bs sdio wifi driver Dan Carpenter
2021-10-10 10:45 ` Fabio Aiuto
2021-10-11 12:50 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.