[PATCH net v2 1/4] ieee802154: cc2520: ensure tailroom before skb_put() in promiscuous TX

[PATCH net v2 1/4] ieee802154: cc2520: ensure tailroom before skb_put() in promiscuous TX

[Hui Peng]

In cc2520_tx(), when priv->promiscuous is enabled, skb_put(skb, 2) is
called unconditionally to append the 2-byte software CRC without checking
whether the skb has at least 2 bytes of tailroom, triggering
skb_over_panic when skb_tailroom(skb) < 2:

  skbuff: skb_over_panic: text:ffffffffc080071a len:386 put:2
  kernel BUG at net/core/skbuff.c:214!
  Oops: invalid opcode: 0000 [#1] SMP KASAN PTI
  RIP: 0010:skb_panic+0x170/0x172
  Call Trace:
   <TASK>
   skb_put.cold+0x23/0x23
   cc2520_tx.constprop.0.isra.0+0x8a/0x1d0

Ensure at least 2 bytes of tailroom via pskb_expand_head() before calling
skb_put(skb, 2).

Tested in QEMU with KASAN enabled by passing a zero-tailroom skb to
cc2520_tx() with promiscuous mode enabled.

Fixes: 59869ebfe7a7 ("ieee802154: cc2520: Check CRC & add promiscuous")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split the cc2520 and mcr20a fixes into four single-issue patches
  (1/4..4/4) and documented each change in its own commit message as
  requested by Miquel Raynal.

 drivers/net/ieee802154/cc2520.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index abfcfe0..5454872 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -482,8 +482,14 @@ cc2520_tx(struct ieee802154_hw *hw, struct sk_buff *skb)
 	 * values on RX. This means we need to manually add the CRC on TX.
 	 */
 	if (priv->promiscuous) {
-		u16 crc = crc_ccitt(0, skb->data, skb->len);
+		u16 crc;
 
+		if (skb_tailroom(skb) < 2 &&
+		    pskb_expand_head(skb, 0, 2, GFP_KERNEL)) {
+			rc = -ENOMEM;
+			goto err_tx;
+		}
+		crc = crc_ccitt(0, skb->data, skb->len);
 		put_unaligned_le16(crc, skb_put(skb, 2));
 		pkt_len = skb->len;
 	} else {
-- 
2.47.3

[PATCH net v2 2/4] ieee802154: cc2520: flush fifop_irqwork before destroying buffer_mutex in probe

[Hui Peng]

In cc2520_probe()'s err_hw_init path, mutex_destroy(&priv->buffer_mutex)
is called before flush_work(&priv->fifop_irqwork). If fifop_irq fires
after devm_request_irq() and schedules priv->fifop_irqwork before probe
fails, cc2520_fifop_irqwork() can lock priv->buffer_mutex after it has
already been destroyed:

  BUG: KASAN: slab-use-after-free in assign_work+0x2f1/0x340
  Read of size 8 at addr ffff88800246b0b0 by task kworker/1:1/43
  Workqueue:  0x0 (events)
  Call Trace:
   <TASK>
   dump_stack_lvl+0x70/0xa0
   print_report+0x153/0x4c6
   kasan_report+0xf1/0x120
   assign_work+0x2f1/0x340
   worker_thread+0x2b8/0xb70

Flush priv->fifop_irqwork before mutex_destroy(&priv->buffer_mutex),
matching the teardown order in cc2520_remove().

Tested in QEMU with KASAN enabled by scheduling fifop_irqwork on the
cc2520_probe() error path.

Fixes: ff5891b266a7 ("ieee802154: cc2520: fix FIFOP work use-after-free")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out as patch 2/4 and documented the probe cleanup ordering fix and
  Fixes: tag as requested by Miquel Raynal.

 drivers/net/ieee802154/cc2520.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
index 5454872..d2c8484 100644
--- a/drivers/net/ieee802154/cc2520.c
+++ b/drivers/net/ieee802154/cc2520.c
@@ -1153,8 +1153,8 @@ static int cc2520_probe(struct spi_device *spi)
 	return 0;
 
 err_hw_init:
-	mutex_destroy(&priv->buffer_mutex);
 	flush_work(&priv->fifop_irqwork);
+	mutex_destroy(&priv->buffer_mutex);
 	return ret;
 }
 
-- 
2.47.3

Re: [PATCH net v2 2/4] ieee802154: cc2520: flush fifop_irqwork before destroying buffer_mutex in probe

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 2 (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).

- [High] Incomplete fix: the cc2520_probe() error path
  (drivers/net/ieee802154/cc2520.c:1155-1158) still uses…
- [Medium] The commit message's quoted evidence does not match the defect
  it describes or the change it makes.
- [Medium] Incorrect Fixes: tag / wrong stable backport range.

Pre-existing issues:
- [Medium] Pre-existing: the cc2520_probe() error path does not roll back
  the hardware power/reset ramp.

--

ieee802154: cc2520: flush fifop_irqwork before destroying buffer_mutex in probe

This swaps two statements on the err_hw_init path of cc2520_probe() so that
flush_work(&priv->fifop_irqwork) runs before mutex_destroy(&priv->buffer_mutex),
with the stated goal of matching the teardown order used by cc2520_remove().

A few questions about the change and the commit log below.

>From the commit message:

>     In cc2520_probe()'s err_hw_init path, mutex_destroy(&priv->buffer_mutex)
>     is called before flush_work(&priv->fifop_irqwork). If fifop_irq fires
>     after devm_request_irq() and schedules priv->fifop_irqwork before probe
>     fails, cc2520_fifop_irqwork() can lock priv->buffer_mutex after it has
>     already been destroyed:
>
>       BUG: KASAN: slab-use-after-free in assign_work+0x2f1/0x340
>       Read of size 8 at addr ffff88800246b0b0 by task kworker/1:1/43
>       Workqueue:  0x0 (events)
>       Call Trace:
>        <TASK>
>        dump_stack_lvl+0x70/0xa0
>        print_report+0x153/0x4c6
>        kasan_report+0xf1/0x120
>        assign_work+0x2f1/0x340
>        worker_thread+0x2b8/0xb70

[Severity: Medium]
Does this trace actually correspond to the ordering problem being described?

For !CONFIG_DEBUG_MUTEXES, include/linux/mutex.h has:

	static inline void mutex_destroy(struct mutex *lock) {}

so on production kernels the reorder is a no-op.  With
CONFIG_DEBUG_MUTEXES, kernel/locking/mutex-debug.c does:

	void mutex_destroy(struct mutex *lock)
	{
		DEBUG_LOCKS_WARN_ON(mutex_is_locked(lock));
		lock->magic = NULL;
	}

which can produce a DEBUG_LOCKS warning on a subsequent mutex_lock(), but
does not free any memory.

The pasted report is a use-after-free on the work_struct inside the
workqueue core (assign_work()/worker_thread()), i.e. priv freed while the
work was still queued, which is a different failure from
cc2520_fifop_irqwork() locking a destroyed buffer_mutex.  Could the commit
message be adjusted so the quoted symptom matches the described defect,
given that the log carries Cc: stable@vger.kernel.org?

Similarly, does "Tested in QEMU with KASAN enabled by scheduling
fifop_irqwork on the cc2520_probe() error path" demonstrate the described
mutex ordering issue, or the queued-work-after-free one?

>     Fixes: ff5891b266a7 ("ieee802154: cc2520: fix FIFOP work use-after-free")

[Severity: Medium]
Is this the right commit to blame?  ff5891b266a7 changes only
cc2520_remove(), where it replaced the flush with:

	disable_work_sync(&priv->fifop_irqwork);

git blame of the err_hw_init sequence being reordered here points at
0da6bc8cc341 ("ieee802154: cc2520: adds driver for TI CC2520 radio") from
2014, not ff5891b266a7.  With the current tag, stable trees that predate
ff5891b266a7 would not pick this up.

> diff --git a/drivers/net/ieee802154/cc2520.c b/drivers/net/ieee802154/cc2520.c
> index 545487223838..d2c8484d8fea 100644
> --- a/drivers/net/ieee802154/cc2520.c
> +++ b/drivers/net/ieee802154/cc2520.c
> @@ -1153,8 +1153,8 @@ static int cc2520_probe(struct spi_device *spi)
>  	return 0;
>  
>  err_hw_init:
> -	mutex_destroy(&priv->buffer_mutex);
>  	flush_work(&priv->fifop_irqwork);
> +	mutex_destroy(&priv->buffer_mutex);
>  	return ret;
>  }

[Severity: High]
Can flush_work() close this window at all while the FIFOP interrupt is
still armed?

The FIFOP handler re-queues unconditionally on every edge:

	static irqreturn_t cc2520_fifop_isr(int irq, void *data)
	{
		struct cc2520_private *priv = data;

		schedule_work(&priv->fifop_irqwork);

		return IRQ_HANDLED;
	}

flush_work() waits for a queued or running instance but does not prevent a
new submission, and the irq was requested with devm_request_irq(), so it
stays registered until the driver core unwinds devres after probe returns:

cc2520_probe() [err_hw_init]
    flush_work(&priv->fifop_irqwork);      /* CPU0 */
                                           /* CPU1: FIFOP edge ->
                                              cc2520_fifop_isr() ->
                                              schedule_work() re-queues */
    mutex_destroy(&priv->buffer_mutex);
    return ret;
really_probe()
    device_unbind_cleanup()
        devres_release_all()               /* frees the irq and the
                                              devm_kzalloc'ed priv */

priv embeds both fifop_irqwork and buffer_mutex, so the worker pool then
touches a freed work_struct, and cc2520_fifop_irqwork() can dereference
freed priv:

	if (gpiod_get_value(priv->fifo_pin))
		cc2520_rx(priv);
	...
	cc2520_cmd_strobe(priv, CC2520_CMD_SFLUSHRX);

reaching cc2520_cmd_strobe(), which locks the destroyed and freed
buffer_mutex.  That is the same assign_work()/worker_thread() signature
quoted in the commit message.

Would disable_work_sync() be appropriate here, the way ff5891b266a7 handled
cc2520_remove() for exactly this devm-irq-outlives-teardown case?
Alternatively, releasing or masking the FIFOP irq before draining the work.

There is also the cc2520_register() failure case, where priv->hw is freed
but not cleared:

	err_free_device:
		ieee802154_free_hw(priv->hw);

so a surviving work item can reach a freed hw through
cc2520_rx()->ieee802154_rx_irqsafe().

For reference, no later patch in this series changes this path; err_hw_init
still reads flush_work() followed by mutex_destroy() at the end of the
series.

[Severity: Medium]
This isn't a bug introduced by this patch, but while the err_hw_init path is
being touched: should it also unwind the power and reset ramp done earlier in
cc2520_probe()?

	gpiod_set_value(vreg, HIGH);
	usleep_range(100, 150);

	gpiod_set_value(reset, HIGH);
	usleep_range(200, 250);

err_hw_init only flushes the work and destroys the mutex.  Releasing a gpio
descriptor through devres does not restore its output level, so after a
failure in cc2520_hw_init(), either devm_request_irq(), or cc2520_register(),
the CC2520 stays powered and out of reset with no driver bound.  On a later
re-bind, reset is already HIGH, so does cc2520_hw_init() then program a chip
that never saw a reset pulse?  This dates back to the original driver
(0da6bc8cc341).

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

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

[Hui Peng]

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
  Call Trace:
   <TASK>
   dump_stack_lvl+0x70/0xa0
   print_report+0x153/0x4c6
   kasan_report+0xf1/0x120
   kasan_check_range+0x125/0x200
   __asan_memcpy+0x23/0x60
   mcr20a_handle_rx_read_buf_complete.constprop.0+0x91/0xc0
  ...
  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)

Drop the corrupted frame, re-arm reception via mcr20a_request_rx(lp), and
return early.

Tested in QEMU with KASAN enabled by passing a corrupted 4-byte RX frame
length to mcr20a_handle_rx_read_buf_complete().

Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out as patch 3/4 covering only the mcr20a RX corrupted frame
  handling as requested by Miquel Raynal.

 drivers/net/ieee802154/mcr20a.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index 020d392a98b6..d01b277d33d2 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 */
-- 
2.47.3

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

[netdev-bot+sashiko]

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

[PATCH net v2 4/4] ieee802154: mcr20a: fix 1-byte out-of-bounds read in mcr20a_handle_tx()

[Hui Peng]

In mcr20a_handle_tx(), the 1-byte psduLength prefix (lp->tx_len[0]) is
already sent in a separate SPI transfer (lp->tx_xfer_len), while
lp->tx_xfer_buf transfers the payload from lp->tx_skb->data. Setting
lp->tx_xfer_buf.len = lp->tx_skb->len + 1 causes the SPI transfer to read
1 byte past the end of lp->tx_skb->data:

  BUG: KASAN: slab-out-of-bounds in mcr20a_handle_tx+0xf5/0x150
  Read of size 17 at addr ffff8880057f7500 by task init/1
  Call Trace:
   <TASK>
   dump_stack_lvl+0x70/0xa0
   print_report+0x153/0x4c6
   kasan_report+0xf1/0x120
   kasan_check_range+0x125/0x200
   __asan_memcpy+0x23/0x60
   mcr20a_handle_tx+0xf5/0x150
  ...
  The buggy address belongs to the object at ffff8880057f7500
   which belongs to the cache kmalloc-16 of size 16
  The buggy address is located 0 bytes inside of
   allocated 16-byte region [ffff8880057f7500, ffff8880057f7510)

Set lp->tx_xfer_buf.len to lp->tx_skb->len.

Tested in QEMU with KASAN enabled by transferring a 16-byte tx_skb->data
buffer in mcr20a_handle_tx().

Fixes: 8c6ad9cc5157 ("ieee802154: Add NXP MCR20A IEEE 802.15.4 transceiver driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out as patch 4/4 covering only the mcr20a TX SPI transfer length
  fix as requested by Miquel Raynal.

 drivers/net/ieee802154/mcr20a.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
index d01b277d33d2..d7e076397550 100644
--- a/drivers/net/ieee802154/mcr20a.c
+++ b/drivers/net/ieee802154/mcr20a.c
@@ -867,8 +867,7 @@ mcr20a_handle_tx(struct mcr20a_local *lp)
 	/* add 2 bytes of FCS */
 	lp->tx_len[0]		= lp->tx_skb->len + 2;
 	lp->tx_xfer_buf.tx_buf	= lp->tx_skb->data;
-	/* add 1 byte psduLength */
-	lp->tx_xfer_buf.len	= lp->tx_skb->len + 1;
+	lp->tx_xfer_buf.len	= lp->tx_skb->len;
 
 	ret = spi_async(lp->spi, &lp->tx_buf_msg);
 	if (ret) {
-- 
2.47.3

Re: [PATCH net v2 4/4] ieee802154: mcr20a: fix 1-byte out-of-bounds read in mcr20a_handle_tx()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 (1) · 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 of "ieee802154: mcr20a: fix 1-byte out-of-
  bounds read in mcr20a_handle_tx()" misrepresents the severity of the…

Pre-existing issues:
- [High] Unhandled spi_async() failure in the TX state machine leaks the
  skb and wedges the TX queue forever.

--

ieee802154: mcr20a: fix 1-byte out-of-bounds read in mcr20a_handle_tx()

This drops the "+ 1" from lp->tx_xfer_buf.len in mcr20a_handle_tx(), so the
payload SPI transfer now clocks out exactly lp->tx_skb->len bytes instead of
one extra byte. The 1-byte psduLength prefix is already carried by the
separate lp->tx_xfer_len transfer.

A couple of questions below, one about the commit message and one about the
error path that the hunk touches.

>     In mcr20a_handle_tx(), the 1-byte psduLength prefix (lp->tx_len[0]) is
>     already sent in a separate SPI transfer (lp->tx_xfer_len), while
>     lp->tx_xfer_buf transfers the payload from lp->tx_skb->data. Setting
>     lp->tx_xfer_buf.len = lp->tx_skb->len + 1 causes the SPI transfer to read
>     1 byte past the end of lp->tx_skb->data:
>
>       BUG: KASAN: slab-out-of-bounds in mcr20a_handle_tx+0xf5/0x150
>       Read of size 17 at addr ffff8880057f7500 by task init/1
>       ...
>       The buggy address belongs to the object at ffff8880057f7500
>        which belongs to the cache kmalloc-16 of size 16

[Severity: Medium]
Can this splat actually be produced by the driver's own TX path?

lp->tx_skb comes from mac802154 via mcr20a_xmit(), and an skb head allocated
by __alloc_skb()/kmalloc_reserve() is always sized as the data area plus
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) inside one slab object.
sizeof(struct skb_shared_info) alone is a few hundred bytes, so an skb head
cannot live in kmalloc-16, and skb->data[skb->len] always lands inside the
same allocation (tailroom or shared info) where KASAN has no redzone to
report.

The last paragraph says:

>     Tested in QEMU with KASAN enabled by transferring a 16-byte tx_skb->data
>     buffer in mcr20a_handle_tx().

Would it be clearer to state up front that the trace comes from an
instrumented/synthetic buffer rather than from a normal transmit, and to
describe the real effect instead?

As far as I can tell the real effect is that one byte of unrelated
in-allocation memory was clocked into the transceiver packet buffer at the
first FCS position, which the chip then overwrites because mcr20a_hw_setup()
sets IEEE802154_HW_TX_OMIT_CKSUM and the hardware generates the FCS itself.
That reads as a correctness fix with no kernel memory-safety violation and
no over-the-air leak, which matters for how stable maintainers judge the
Cc: stable tag.

The code change itself looks right: mcr20a_setup_tx_spi_messages() already
sends psduLength as its own transfer:

	lp->tx_xfer_len.len = 1;
	lp->tx_xfer_len.tx_buf = lp->tx_len;

and at86rf230_write_frame() uses the same layout (length byte = skb->len + 2,
payload = skb->len bytes).

> diff --git a/drivers/net/ieee802154/mcr20a.c b/drivers/net/ieee802154/mcr20a.c
> index ee3c1e67ef60..d7e076397550 100644
> --- a/drivers/net/ieee802154/mcr20a.c
> +++ b/drivers/net/ieee802154/mcr20a.c
> @@ -867,8 +867,7 @@ mcr20a_handle_tx(struct mcr20a_local *lp)
>  	/* add 2 bytes of FCS */
>  	lp->tx_len[0]		= lp->tx_skb->len + 2;
>  	lp->tx_xfer_buf.tx_buf	= lp->tx_skb->data;
> -	/* add 1 byte psduLength */
> -	lp->tx_xfer_buf.len	= lp->tx_skb->len + 1;
> +	lp->tx_xfer_buf.len	= lp->tx_skb->len;
>  
>  	ret = spi_async(lp->spi, &lp->tx_buf_msg);
>  	if (ret) {

[Severity: High]
This isn't a bug introduced by this patch, but since the hunk sits right
above it: does this error path leak lp->tx_skb and stall the TX queue for
good?

	ret = spi_async(lp->spi, &lp->tx_buf_msg);
	if (ret) {
		dev_err(printdev(lp), "SPI write Failed for TX buf\n");
		return ret;
	}

On failure the message is never submitted, so no completion callback runs.
lp->tx_skb is neither completed nor freed and lp->is_tx stays 1. The only
caller discards the return value:

mcr20a_irq_clean_complete() {
	...
	case (DAR_IRQSTS1_SEQIRQ):
		if (lp->is_tx) {
			dev_dbg(printdev(lp), "TX is starting\n");
			mcr20a_handle_tx(lp);
	...
}

Meanwhile ieee802154_tx() in net/mac802154/tx.c already did
ieee802154_hold_queue() and atomic_inc(&local->phy->ongoing_txs) when the
earlier drv_xmit_async() (mcr20a_xmit()) returned 0, and only
mcr20a_handle_tx_complete() -> ieee802154_xmit_complete() undoes that. So
the queue is never woken and ongoing_txs stays non-zero.

Does that then make ieee802154_sync_queue() wait forever?

net/mac802154/tx.c:ieee802154_sync_queue() {
	...
	wait_event(local->phy->sync_txq, !atomic_read(&local->phy->ongoing_txs));
	...
}

There is no timeout and no alternative wake condition, so any MLME operation
going through ieee802154_mlme_op_pre() would block indefinitely.

spi_async() can fail for real reasons: -EBUSY when ctlr->bus_lock_flag is
set by a concurrent spi_bus_lock() user, or an error propagated from
spi_maybe_optimize_message()/__spi_async().

Would ieee802154_xmit_error(lp->hw, lp->tx_skb, IEEE802154_SYSTEM_ERROR),
or ieee802154_wake_queue() plus kfree_skb(), together with clearing
lp->is_tx, be the right handling here?

mcr20a_write_tx_buf_complete() looks to have the same gap for the SEQ TX
register write:

	ret = spi_async(lp->spi, &lp->reg_msg);
	if (ret)
		dev_err(printdev(lp), "failed to set SEQ TX\n");

Should that path also release the skb and the queue?

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