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
