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
