Re: [PATCH] wifi: wilc1000: validate packet bounds in RX buffer parsing

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 1 · High: 1 (1) · Medium: 1 · Low: 0

- [Critical] The `size` that the patched parser now treats as its
  authoritative bound is a raw device-supplied length that is never…
- [High] The new checks bound only the upper end of a record
  (pkt_len/pkt_offset against tp_len, tp_len against size - offset) and…
- [Medium] The new validation still accepts transport records that cannot
  contain their own 4-byte header, and payload offsets that point inside…

Pre-existing issues:
- [High] Short public-action management frames underflow the vendor-IE
  length in wilc_wfi_p2p_rx() (cfg80211.c).

--

wifi: wilc1000: validate packet bounds in RX buffer parsing

The patch converts the record loop in wilc_wlan_handle_rx_buff() from a
do-while into a while loop that requires four header bytes to remain, and
adds checks of tp_len against the remaining buffer plus pkt_len/pkt_offset
against tp_len.  A few questions about how complete the new bounds are.

> diff --git a/drivers/net/wireless/microchip/wilc1000/wlan.c b/drivers/net/wireless/microchip/wilc1000/wlan.c
> index 4b116fe6f9ea9..3ebcd688f2dd5 100644
> --- a/drivers/net/wireless/microchip/wilc1000/wlan.c
> +++ b/drivers/net/wireless/microchip/wilc1000/wlan.c
> @@ -1111,7 +1111,7 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size)
>  	int is_cfg_packet;
>  	u8 *buff_ptr;
>  
> -	do {
> +	while (offset + sizeof(u32) <= size) {
>  		buff_ptr = buffer + offset;
>  		header = get_unaligned_le32(buff_ptr);
>  
> @@ -1123,11 +1123,19 @@ static void wilc_wlan_handle_rx_buff(struct wilc *wilc, u8 *buffer, int size)
>  		if (pkt_len == 0 || tp_len == 0)
>  			break;
>  
> +		if (tp_len > size - offset)
> +			break;

[Severity: Critical]
Is "size" itself known to describe memory inside the RX allocation?  The
only producer is wilc_wlan_handle_isr_ext():

drivers/net/wireless/microchip/wilc1000/wlan.c:wilc_wlan_handle_isr_ext() {
	size = FIELD_GET(WILC_INTERRUPT_DATA_SIZE, int_status) << 2;
	...
	if (size <= 0)
		return;

	if (WILC_RX_BUFF_SIZE - offset < size)
		offset = 0;

	buffer = &wilc->rx_buffer[offset];
	...
	ret = wilc->hif_func->hif_block_rx_ext(wilc, 0, buffer, size);
	...
	offset += size;
	wilc->rx_buffer_offset = offset;
	...
	rqe->buffer = buffer;
	rqe->buffer_size = size;
}

WILC_INTERRUPT_DATA_SIZE is GENMASK(14, 0) and the field is word scaled, so
the chip can advertise 32767 << 2 = 131068 bytes, while wilc->rx_buffer is a
single WILC_RX_BUFF_SIZE (96 * 1024 = 98304) byte allocation.  Since size is
u32 the "size <= 0" test never fires, and for size > 98304 the code only
rewinds offset to 0 and still issues hif_block_rx_ext() for the full size;
wilc_sdio_read() and wilc_spi_read() only reject size <= 4, so nothing clamps
the transfer.  Can this write past the end of wilc->rx_buffer?

There also seems to be a second-order effect: rx_buffer_offset is then set to
offset + size (up to 131068), and on the next interrupt
"WILC_RX_BUFF_SIZE - offset" is evaluated in unsigned arithmetic, so
98304 - 131068 wraps to a huge value that is never < size and the rewind
never triggers again.  Would that leave buffer pointing entirely outside the
allocation for every subsequent block read?

Because rqe->buffer_size carries that same unvalidated length, every new
check added here ("tp_len > size - offset" and friends) can pass while the
parser walks memory outside the allocation.  Would clamping size to
WILC_RX_BUFF_SIZE in wilc_wlan_handle_isr_ext() be needed for the new bounds
in this function to mean anything?

>  		if (pkt_offset & IS_MANAGMEMENT) {
> +			if (tp_len < HOST_HDR_OFFSET || pkt_len > tp_len - HOST_HDR_OFFSET)
> +				break;
>  			buff_ptr += HOST_HDR_OFFSET;
>  			wilc_wfi_mgmt_rx(wilc, buff_ptr, pkt_len,
>  					 pkt_offset & IS_MGMT_AUTH_PKT);
>  		} else {

[Severity: High]
This isn't a bug introduced by this patch, but since the management check
here only caps pkt_len from above, short management frames still reach
wilc_wfi_p2p_rx() in cfg80211.c, where the vendor IE length underflows:

drivers/net/wireless/microchip/wilc1000/cfg80211.c:wilc_wfi_p2p_rx() {
	...
	if (!ieee80211_is_action(mgmt->frame_control) ||
	    !ieee80211_is_public_action((struct ieee80211_hdr *)buff, size))
	...
	d = (struct wilc_p2p_pub_act_frame *)(&mgmt->u.action);
	if (d->oui_subtype != GO_NEG_REQ && d->oui_subtype != GO_NEG_RSP &&
	...
	vendor_ie = cfg80211_find_vendor_ie(WLAN_OUI_WFA, WLAN_OUI_TYPE_WFA_P2P,
					    buff + ie_offset, size - ie_offset);
}

ie_offset is offsetof(struct ieee80211_mgmt, u) + sizeof(*d) = 24 + 8 = 32,
but ieee80211_is_public_action() only guarantees
IEEE80211_MIN_ACTION_SIZE(category), i.e. 25 bytes.  For a 31 byte frame
d->oui_subtype (frame offset 30) is still in bounds, and "size - ie_offset"
with u32 size wraps to roughly 4 GiB.  Does that let cfg80211_find_vendor_ie()
scan far past the RX buffer?

A "size >= ie_offset" guard in wilc_wfi_p2p_rx(), and/or a minimum pkt_len
check for management records here, would close it.

> +			if (pkt_offset > tp_len ||
> +			    pkt_len > tp_len - pkt_offset)
> +				break;

[Severity: High]
The new checks bound the upper end of a record but never require a minimum
length, since only zero pkt_len/tp_len is rejected.  Do the consumers still
read past what has been validated?

For the data path, tp_len = pkt_len = 1 with pkt_offset = 0 passes both
checks, and then:

drivers/net/wireless/microchip/wilc1000/netdev.c:wilc_frmw_to_host() {
	...
	wilc_netdev = get_if_handler(wilc, buff);
	if (!wilc_netdev)
		goto out;

	buff += pkt_offset;
}

get_if_handler() is called with the record start, before pkt_offset is added,
and does:

drivers/net/wireless/microchip/wilc1000/netdev.c:get_if_handler() {
	struct ieee80211_hdr *h = (struct ieee80211_hdr *)mac_header;
	...
			if (ether_addr_equal_unaligned(h->addr2, vif->bssid)) {
}

That touches bytes 4..15 of the record.  With the loop guard only requiring
offset + 4 <= size, a record at offset == size - 4 would read up to 12 bytes
beyond the region described by size.

For the config path, pkt_len == 1 is accepted, but:

drivers/net/wireless/microchip/wilc1000/wlan_cfg.c:wilc_wlan_cfg_indicate_rx() {
	msg_type = frame[0];
	msg_id = frame[1];	/* seq no */
	frame += 4;
	size -= 4;
}

size goes negative, and the info handlers then do:

drivers/net/wireless/microchip/wilc1000/hif.c:wilc_gnrl_async_info_received() {
	id = get_unaligned_le32(&buffer[length - 4]);
}

with a u32 length, so length < 4 indexes at 0xFFFFFFFD.  The same pattern
appears in wilc_network_info_received(), which additionally does
kmemdup(&buffer[9], get_unaligned_le16(&buffer[6]) - 1) and can copy up to
65534 bytes out of a record validated as 1..8 bytes.

On the management side, pkt_len == 1 with tp_len == 5 passes, yet
wilc_wfi_mgmt_rx() reads two bytes:

drivers/net/wireless/microchip/wilc1000/netdev.c:wilc_wfi_mgmt_rx() {
		struct ieee80211_mgmt *mgmt = (struct ieee80211_mgmt *)buff;
		u16 type = le16_to_cpup((__le16 *)buff);
}

Would per-branch minimum lengths be needed here, at least
sizeof(struct ieee80211_hdr) before the data classifier, 4 plus the subtype
requirements for config frames, and the parsed header size for management
frames?

[Severity: Medium]
Can a record smaller than its own header still be accepted?  tp_len is only
rejected when zero or larger than the remaining bytes, so tp_len in 1..3
passes even though four header bytes were already consumed, and this branch
accepts pkt_offset in 0..3.

With tp_len = 1, pkt_offset = 0, pkt_len = 1, one byte of the framing header
itself is handed to wilc_frmw_to_host() as packet data, and

		offset += tp_len;

advances by 1, so the next iteration parses a header overlapping the record
just consumed.  Even with a large tp_len, pkt_offset = 0 makes the four byte
receive header the first bytes of the Ethernet or configuration frame, which
wilc_wlan_cfg_indicate_rx() then reads as msg_type/msg_id.

Should the checks require tp_len >= sizeof(u32), and pkt_offset >=
HOST_HDR_OFFSET for non-management records?

>  			if (!is_cfg_packet) {
>  				wilc_frmw_to_host(wilc, buff_ptr, pkt_len,
>  						  pkt_offset);

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/AS8P251MB0001BF71A8397CE77A82E7E2C8B32%40AS8P251MB0001.EURP251.PROD.OUTLOOK.COM