Re: [PATCH net v2 3/4] ieee802154: mcr20a: drop corrupted RX frames instead of reading 125 stale bytes

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message justifies the change with a KASAN slab-out-
  of-bounds splat ("Read of size 125 at addr ffff888002853f40 ... cache…

Pre-existing issues:
- [High] mcr20a_irq_status_complete() unconditionally programs XCVSEQ to
  MCR20A_XCVSEQ_IDLE (mcr20a.c:938-939) before the RX length/buffer read…
- [High] mcr20a_handle_rx_read_buf_complete() strips the two FCS bytes
  from the received PSDU (`len = len - 2; /* get rid of frame check…

--

ieee802154: mcr20a: drop corrupted RX frames instead of reading 125 stale bytes

When the reported PSDU length is invalid, mcr20a_handle_rx_read_buf_complete()
no longer forces len to IEEE802154_MTU and copies the whole buffer; it now
drops the frame and re-arms reception with mcr20a_request_rx(lp).

First, a question about the justification in the commit message:

>     In mcr20a_handle_rx_read_buf_complete(), when
>     !ieee802154_is_valid_psdu_len(len) is true, the driver overwrites len
>     with IEEE802154_MTU (127) and copies 125 bytes from lp->rx_buf into a new
>     skb even though only the original len bytes were transferred over SPI,
>     reading past the valid RX data and leaking up to 125 bytes of stale heap
>     memory to the network stack:
>
>       BUG: KASAN: slab-out-of-bounds in mcr20a_handle_rx_read_buf_complete.constprop.0+0x91/0xc0
>       Read of size 125 at addr ffff888002853f40 by task init/1
>       ...
>       The buggy address belongs to the object at ffff888002853f40
>        which belongs to the cache kmalloc-8 of size 8
>       The buggy address is located 0 bytes inside of
>        allocated 4-byte region [ffff888002853f40, ffff888002853f44)

[Severity: Medium]
Can this KASAN splat actually be produced by this driver?  The memcpy source
in mcr20a_handle_rx_read_buf_complete() is lp->rx_buf, which is a fixed array
embedded in struct mcr20a_local:

drivers/net/ieee802154/mcr20a.c:
	u8 rx_buf[MCR20A_MAX_BUF];

with MCR20A_MAX_BUF defined as 127, and mcr20a_setup_rx_spi_messages() binds
the SPI transfer to it permanently:

	lp->rx_xfer_buf.rx_buf = lp->rx_buf;

struct mcr20a_local itself is the priv blob from ieee802154_alloc_hw(sizeof(*lp),
...), so a 125-byte read from rx_buf stays inside the object and cannot be
reported as a slab-out-of-bounds access, and the source object can never be a
4-byte allocation in kmalloc-8.  The only small heap object in the driver is
lp->buf from devm_kzalloc(SPI_COMMAND_BUFFER), which is never the memcpy
source here.

The defect the patch fixes looks narrower: mcr20a_handle_rx_read_len_complete()
only refreshes lp->rx_xfer_buf.len bytes,

	lp->rx_xfer_buf.len = len;

so with the old "len = IEEE802154_MTU" fallback the driver handed the stale
tail of its own RX buffer (leftovers of previously received frames, or zeros)
to the 802.15.4 stack as if it were received data.  Given the Cc: stable tag,
would it make sense to describe it that way and drop the KASAN block, so that
backporters do not read this as an out-of-bounds read?

> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> index 020d392a98b6..ee3c1e67ef60 100644
> --- a/drivers/net/ieee802154/mcr20a.c
> +++ b/drivers/net/ieee802154/mcr20a.c
> @@ -790,7 +790,8 @@ mcr20a_handle_rx_read_buf_complete(void *context)
>  
>  	if (!ieee802154_is_valid_psdu_len(len)) {
>  		dev_vdbg(&lp->spi->dev, "corrupted frame received\n");
> -		len = IEEE802154_MTU;
> +		mcr20a_request_rx(lp);
> +		return;
>  	}
>  
>  	len = len - 2;  /* get rid of frame check field */

[Severity: High]
This isn't a bug introduced by this patch, but the new branch highlights the
sibling exits of the same RX chain that do not re-arm reception.
mcr20a_irq_status_complete() always parks the sequence manager first:

	regmap_update_bits_async(lp->regmap_dar, DAR_PHY_CTRL1,
				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_IDLE);

so RX only resumes when mcr20a_request_rx() writes MCR20A_XCVSEQ_RX.  A few
lines below the hunk, mcr20a_handle_rx_read_buf_complete() returns without
doing that:

	skb = dev_alloc_skb(len);
	if (!skb)
		return;

and mcr20a_handle_rx_read_len_complete() does the same on a submission
failure:

	ret = spi_async(lp->spi, &lp->rx_buf_msg);
	if (ret)
		dev_err(printdev(lp), "failed to read rx buffer length\n");

With XCVSEQ left at IDLE, does the part generate any further RXIRQ/SEQIRQ that
could restart the chain?  mcr20a_request_rx() is only reachable from
mcr20a_handle_rx_read_buf_complete(), mcr20a_handle_tx_complete() and
mcr20a_start(), so recovery would seem to require a transmit or an interface
down/up, while the netdev stays up and silently deaf - for a receive-only or
monitor configuration there is no such event.  Would it be worth re-arming RX
on these paths too, in the same way the new corrupted-length branch does?

Related: mcr20a_request_rx() issues the re-arm with
regmap_update_bits_async() and returns 0 unconditionally:

	regmap_update_bits_async(lp->regmap_dar, DAR_PHY_CTRL1,
				 DAR_PHY_CTRL1_XCVSEQ_MASK, MCR20A_XCVSEQ_RX);

	return 0;

so is a failed register write distinguishable from a successful re-arm?

[Severity: High]
This is a pre-existing issue, but it sits on the length computation this patch
touches.  The driver removes the two FCS bytes and hands the FCS-less data to
ieee802154_rx_irqsafe(), while mcr20a_hw_setup() advertises:

	hw->flags = IEEE802154_HW_TX_OMIT_CKSUM |
			IEEE802154_HW_AFILT |
			IEEE802154_HW_PROMISCUOUS;

i.e. without IEEE802154_HW_RX_OMIT_CKSUM.  In net/mac802154/rx.c,
ieee802154_rx() only synthesizes an FCS when that flag is set, and then trims
two bytes regardless:

	if (local->hw.flags & IEEE802154_HW_RX_OMIT_CKSUM) {
		crc = crc_ccitt(0, skb->data, skb->len);
		put_unaligned_le16(crc, skb_put(skb, 2));
	}
	...
	skb_trim(skb, skb->len - 2);

Does this mean every frame received through mcr20a loses two bytes of real MAC
payload, and gets dropped by the crc_ccitt() check when
phy->filtering == IEEE802154_FILTERING_NONE?  adf7242, the other driver that
strips the CRC, sets IEEE802154_HW_OMIT_CKSUM in adf7242_hw_init().  Should
mcr20a either advertise IEEE802154_HW_RX_OMIT_CKSUM or keep the FCS in the
skb?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260921074207.2289391-1-benquike%40gmail.com