Page MenuHomeFreeBSD

upgt: add some overflow checks
Needs ReviewPublic

Authored by avos on Jul 30 2017, 10:37 PM.

Details

Reviewers
adrian
hselasky
Summary

In upgt_eeprom_parse():

  • change type of 'option_len' from uint16_t to uint32_t to prevent multiplication overflow;
  • check for 'eeprom_option' overflow (maybe useless?).

In upgt_fw_verify():

  • fix Boot Record Area presence check;
  • reject firmware if 'bra_option_len' overflows.
Test Plan

Untested

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sys/dev/usb/wlan/if_upgt.c
1384–1385

missing range checks for eeprom_len being valid.

1413–1414

missing range check for header->header1.len being valid.

1457

uint32_t pkglen

1472–1486

if (pkglen > (MCLBYTES - ETHER_ALIGN - IEEE80211_CRC_LEN ))

xxx error;
1487–1488

Remove this KASSERT() - we don't want to panic on corrupt USB data!

1753

Please add checks here for frame_start wrapping around.

1806–1808

The for-loop condition should read: (offset + sizeof(*uc)) <= fw->datasize

Else you might read data which is not inside the BRA.

1813

Same here.

1837

Please also add this check:

if (sizeof(struct upgt_fw_bra_option) > (size_t)(fw->datasize - offset) ||
    (bra_option_len > (size_t)(fw->datasize - offset - sizeof(struct upgt_fw_bra_option)))
sys/dev/usb/wlan/if_upgt.c
2393

IMHO, this code path will never work: typically error handling duplicates USB_ST_TRANSFERRED event with non-zero status code passed to ieee80211_tx_complete().
Here 1) mbuf is not freed - memory leak? 2) data buffer is not moved from active to inactive ring, so Tx will stop.

Turn some KASSERT's into checks + add more size checks.

avos marked 7 inline comments as done.Aug 21 2017, 2:47 AM
avos marked an inline comment as done.Aug 21 2017, 8:13 AM

Add some checks against actlen.

avos marked an inline comment as done.Aug 22 2017, 9:48 PM