Re: [PATCH] wifi: mm81x: validate packet length in mm81x_yaps_read_pkt()

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Incomplete fix / wrong ordering: the header-claimed-length
  validation added by this patch runs ~30 lines AFTER…

Pre-existing issues:
- [High] Missing minimum-payload validation lets an undersized device
  command response reach mm81x_cmd_resp_process()…
- [Medium] Two RX parse sites disagree on the meaning of struct
  mm81x_skb_hdr's `offset` field.
- [Medium] skb leak on the -ENOMEM path of mm81x_yaps_hw_read_pkts().

--

wifi: mm81x: validate packet length in mm81x_yaps_read_pkt()

The patch adds two length checks to mm81x_yaps_read_pkt(): a minimum
length check before the struct mm81x_skb_hdr cast, and a check that the
header-claimed total length does not exceed skb->len before the packet is
trimmed and enqueued.  A few questions about the placement of the second
check and about the paths that consume the validated packet.

> diff --git a/drivers/net/wireless/morsemicro/mm81x/yaps.c b/drivers/net/wireless/morsemicro/mm81x/yaps.c
> index e98a2a58726f5..da2c88901d64c 100644
> --- a/drivers/net/wireless/morsemicro/mm81x/yaps.c
> +++ b/drivers/net/wireless/morsemicro/mm81x/yaps.c
> @@ -147,6 +147,13 @@ static int mm81x_yaps_read_pkt(struct mm81x_yaps *yaps, struct sk_buff *skb)
>  
>  	__skb_queue_head_init(&skbq);
>  
> +	if (skb->len < sizeof(*hdr)) {
> +		dev_err(mors->dev, "packet too short for header (%u < %zu)",
> +			skb->len, sizeof(*hdr));
> +		ret = -EINVAL;
> +		goto exit_return_page;
> +	}
> +
>  	hdr = (struct mm81x_skb_hdr *)skb->data;
>  	if (hdr->sync != MM81X_SKB_HEADER_SYNC) {
>  		dev_err(mors->dev, "sync value error [0xAA:%d], hdr.len %d",

> @@ -186,6 +193,14 @@ static int mm81x_yaps_read_pkt(struct mm81x_yaps *yaps, struct sk_buff *skb)
>  	}
>  
>  	skb_len = sizeof(*hdr) + hdr->offset + le16_to_cpu(hdr->len);
> +	if (skb_len > skb->len) {
> +		dev_err(mors->dev,
> +			"header claims %d bytes but packet is only %u",
> +			skb_len, skb->len);
> +		ret = -EINVAL;
> +		goto exit_return_page;
> +	}
> +

[Severity: High]
Does this check come too late to protect the checksum walk?  In
mm81x_yaps_read_pkt() the checksum helper is called earlier, between the
sync check and the channel switch, while hdr->len is still unvalidated:

	if (yaps->mors->hif.validate_skb_checksum &&
	    !mm81x_skbq_validate_checksum(skb->data)) {

mm81x_skbq_validate_checksum() in skbq.c takes no buffer length at all and
derives its read extent from the device-supplied header:

	u16 len = le16_to_cpu(skb_hdr->len) + sizeof(*skb_hdr);
	u32 *data_to_xor = (u32 *)data;
	...
	for (i = 0; i < len; i += 4) {
		xor ^= *data_to_xor;
		data_to_xor++;
	}

The real bound on the buffer is the YAPS delimiter size, which is
independent of hdr->len, in mm81x_yaps_hw_read_pkts():

	pkts[i].skb = dev_alloc_skb(pkt_size);
	...
	skb_put(pkts[i].skb, pkt_size);

So with pkt_size == sizeof(struct mm81x_skb_hdr) and hdr->len == 0xFFFF,
can this read roughly 64 KiB past the end of the skb data allocation before
the new skb_len check runs?

Separately, the same helper dereferences an 802.11 header placed just after
the mm81x header:

	struct ieee80211_hdr *hdr =
		(struct ieee80211_hdr *)(data + sizeof(*skb_hdr));
	...
	    (ieee80211_is_data(hdr->frame_control) ||

with the new minimum check accepting skb->len == sizeof(*hdr) exactly, is
that frame_control read (and the clamped span sizeof(*skb_hdr) +
sizeof(struct ieee80211_qos_hdr) + IEEE80211_CCMP_HDR_LEN) also outside the
packet data?

The path looks live: mors->hif.validate_skb_checksum is set from the
firmware host table in mm81x_fw_update_validate_skb_checksum():

	mors->hif.validate_skb_checksum =
		validate_checksum->insert_and_validate_checksum;

Would computing and validating skb_len before the
mm81x_skbq_validate_checksum() call, or passing the available length into
that helper and bounding its loop there, close this?

[Severity: High]
This isn't a bug introduced by this patch, but since these checks are the
new choke point for RX length validation: is a minimum payload length also
needed per channel?  hdr->len == 0 passes both new checks, and
mm81x_skbq_dispatch_work() then strips the header entirely:

	/* Remove mm81x header and padding */
	__skb_pull(pfirst, sizeof(*hdr) + hdr->offset);

	switch (channel) {
	case MM81X_SKB_CHAN_COMMAND:
		mm81x_cmd_resp_process(mors, pfirst);

mm81x_cmd_resp_process() in command.c dereferences the response header with
no skb->len check at all:

	struct host_cmd_resp *src_resp = (struct host_cmd_resp *)(skb->data);
	...
	u16 resp_message_id = le16_to_cpu(src_resp->hdr.message_id);
	u16 resp_host_id = le16_to_cpu(src_resp->hdr.host_id);

and on the matched-response path the memcpy source length is bounded only
by the destination size and a second device-supplied length:

	length = min_t(int, length,
		       le16_to_cpu(src_resp->hdr.len) +
			       sizeof(struct host_cmd_header));
	memcpy(dest_resp, src_resp, length);

For a zero-length command payload with a large embedded hdr.len, does this
copy adjacent slab memory into the driver's command response structure?

[Severity: Medium]
This is a pre-existing issue, but the length formula being validated here
includes hdr->offset while the checksum helper does not.  The dispatch path
and the TX producer both treat offset as padding between the header and the
payload:

skbq.c:mm81x_skbq_dispatch_work()
	__skb_pull(pfirst, sizeof(*hdr) + hdr->offset);

skbq.c:mm81x_skbq_skb_tx()
	hdr.len = cpu_to_le16(skb->len);
	hdr.offset = data - (aligned_head + sizeof(hdr));

whereas mm81x_skbq_validate_checksum() ignores offset:

	struct ieee80211_hdr *hdr =
		(struct ieee80211_hdr *)(data + sizeof(*skb_hdr));
	u16 len = le16_to_cpu(skb_hdr->len) + sizeof(*skb_hdr);

For an RX packet with hdr->offset != 0, does the helper then read padding
bytes as frame_control and XOR a byte range shifted by offset, producing a
checksum verdict over the wrong bytes?

>  	skb_bytes_remaining = mm81x_skbq_space(mq);
>  
>  	if (skb_len > skb_bytes_remaining) {

[Severity: Medium]
This isn't a bug introduced by this patch, but while looking at the caller
of mm81x_yaps_read_pkt(): can the -ENOMEM path of
mm81x_yaps_hw_read_pkts() leak the already allocated RX skbs?

yaps_hw.c:mm81x_yaps_hw_read_pkts() bumps the count after each successful
packet and bails out on allocation failure:

		if (pkts[i].skb)
			dev_err(yaps->mors->dev, "yaps packet leak");

		/* SKB doesn't want padding */
		pkts[i].skb = dev_alloc_skb(pkt_size);
		if (!pkts[i].skb) {
			ret = -ENOMEM;
			dev_err(yaps->mors->dev, "yaps no mem for skb");
			goto exit;
		}

yaps.c:mm81x_yaps_rx_handler() then skips the only loop that takes
ownership of them:

	if (ret && ret != -EAGAIN) {
		dev_err(yaps->mors->dev, "YAPS read_pkts fail: %d", ret);
		goto exit;
	}

	for (i = 0; i < num_pks_received; ++i) {
		mm81x_yaps_read_pkt(yaps, yaps->hw.from_chip_pkts[i].skb);
		yaps->hw.from_chip_pkts[i].skb = NULL;
	}

Since ownership is only released inside mm81x_yaps_read_pkt() (enqueue or
dev_kfree_skb()), and rx_handler returns true for -ENOMEM so it is called
again, does the next pass simply hit the "yaps packet leak" dev_err and
overwrite the stale pointers?  The same appears to apply to the
mm81x_dm_read() overhang error path.

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