upgt: add some overflow checks
Needs ReviewPublic
Actions

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

Tests Skipped

Event Timeline

avos created this revision.Jul 30 2017, 10:37 PM

Herald added a subscriber: imp. · View Herald TranscriptJul 30 2017, 10:37 PM

• hselasky added inline comments.Jul 31 2017, 6:55 AM

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.
1838	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)))

avos added inline comments.Aug 13 2017, 8:22 PM

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

Add memory wrap check.

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

Revision Contents
Changeset List

Path

Size

sys/

dev/

usb/

wlan/

if_upgt.c

125 lines

Diff 32327

View Options

sys/dev/usb/wlan/if_upgt.c

	# $FreeBSD$			# $FreeBSD$
	# Autogenerated - do NOT edit!			# Autogenerated - do NOT edit!

	DIRDEPS = \			DIRDEPS = \
	include \			include \
	include/xlocale \			include/xlocale \
	lib/${CSU_DIR} \			lib/${CSU_DIR} \
	lib/libc \			lib/libc \
	lib/libcompiler_rt \			lib/libcompiler_rt \
	lib/liblzma \			lib/liblzma \
	lib/libthr \			lib/libthr \
	lib/libz \			lib/libz \
				lib/libzstd \


	.include <dirdeps.mk>			.include <dirdeps.mk>

	.if ${DEP_RELDIR} == ${_DEP_RELDIR}			.if ${DEP_RELDIR} == ${_DEP_RELDIR}
	# local dependencies - needed for -jN in clean tree			# local dependencies - needed for -jN in clean tree
	.endif			.endif

upgt: add some overflow checksNeeds ReviewPublicActions