[PATCH net v2 1/3] ieee802154: ca8210: prevent stack buffer overflow in ca8210_rx_done()

[PATCH net v2 1/3] ieee802154: ca8210: prevent stack buffer overflow in ca8210_rx_done()

[Hui Peng]

In ca8210_rx_done(), the packet length len = buf[1] + 2 is only checked
against CA8210_SPI_BUF_SIZE (256). When buf[0] & SPI_SYN is set and
priv->sync_command_response is non-NULL, memcpy() copies up to 256 bytes
into priv->sync_command_response, which points to a struct mac_message
object (sizeof(struct mac_message) = 250 bytes) on the synchronous
caller's stack, overflowing the stack buffer by up to 6 bytes:

  BUG: KASAN: stack-out-of-bounds in ca8210_rx_done+0xc4/0xe0
  Write of size 256 at addr ffff888001907660 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+0x3c/0x60
   ca8210_rx_done+0xc4/0xe0
   ca8210_spi_exchange+0x12e/0x190
  ...
  The buggy address belongs to stack of task init/1
   and is located at offset 32 in frame:
   ca8210_spi_exchange+0x0/0x190
  This frame has 1 object:
   [32, 282) 'response'

Bound the synchronous response memcpy() length to
min_t(size_t, len, sizeof(struct mac_message)).

Tested in QEMU with KASAN enabled by passing a 256-byte SPI_SYN response
into ca8210_rx_done().

Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split the ca8210 fixes into three single-issue patches (1/3..3/3).

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

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index 01af4f9..c009930 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -697,7 +697,8 @@ static void ca8210_rx_done(struct cas_control *cas_ctl)
 
 	if (buf[0] & SPI_SYN) {
 		if (priv->sync_command_response) {
-			memcpy(priv->sync_command_response, buf, len);
+			memcpy(priv->sync_command_response, buf,
+			       min_t(size_t, len, sizeof(struct mac_message)));
 			complete(&priv->sync_exchange_complete);
 		} else {
 			if (cascoda_api_upstream)
-- 
2.47.3

[PATCH net v2 2/3] ieee802154: ca8210: prevent stack buffer overflow in hwme_get_request_sync()

[Hui Peng]

In ca8210_get_ed(), an uninitialized u8 lenvar and a pointer to a 1-byte
stack buffer (u8 *level) are passed to hwme_get_request_sync(), which
unconditionally copies response.pdata.hwme_get_cnf.hw_attribute_length
bytes into hw_attribute_value without checking the caller's destination
buffer capacity, overflowing level on the stack when hw_attribute_length
exceeds 1:

  BUG: KASAN: stack-out-of-bounds in hwme_get_request_sync.constprop.0.isra.0+0xf3/0x170
  Write of size 16 at addr ffff888001907780 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+0x3c/0x60
   hwme_get_request_sync.constprop.0.isra.0+0xf3/0x170
   ca8210_get_ed+0x9c/0xf0
  ...
  The buggy address belongs to stack of task init/1
   and is located at offset 48 in frame:
   ca8210_get_ed+0x0/0xf0
  This frame has 2 objects:
   [48, 49) 'level'
   [64, 65) 'lenvar'

Initialize lenvar = 1 in ca8210_get_ed() and return
IEEE802154_SYSTEM_ERROR in hwme_get_request_sync() if
response.pdata.hwme_get_cnf.hw_attribute_length exceeds
*hw_attribute_length.

Tested in QEMU with KASAN enabled by passing an oversized
hw_attribute_length response into ca8210_get_ed().

Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split out as patch 2/3.
- Replaced the temporary stack buffer in ca8210_get_ed() with lenvar = 1
  and an upper-bound check against *hw_attribute_length in
  hwme_get_request_sync() as requested by Miquel Raynal.

 drivers/net/ieee802154/ca8210.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index c009930..1de6314 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -1678,6 +1678,9 @@ static u8 hwme_get_request_sync(
 		return IEEE802154_SYSTEM_ERROR;
 
 	if (response.pdata.hwme_get_cnf.status == IEEE802154_SUCCESS) {
+		if (response.pdata.hwme_get_cnf.hw_attribute_length >
+		    *hw_attribute_length)
+			return IEEE802154_SYSTEM_ERROR;
 		*hw_attribute_length =
 			response.pdata.hwme_get_cnf.hw_attribute_length;
 		memcpy(
@@ -2028,7 +2031,7 @@ static int ca8210_xmit_async(struct ieee802154_hw *hw, struct sk_buff *skb)
  */
 static int ca8210_get_ed(struct ieee802154_hw *hw, u8 *level)
 {
-	u8 lenvar;
+	u8 lenvar = 1;
 	struct ca8210_priv *priv = hw->priv;
 
 	return link_to_linux_err(
-- 
2.47.3

[PATCH net v2 3/3] ieee802154: ca8210: validate data_ind length upfront in ca8210_skb_rx()

[Hui Peng]

In ca8210_skb_rx(), data_ind is read at offsets 22 (msdulen), 23
(mpdulinkquality), 29 + msdulen (hdr.sec.level), 30..39 + msdulen
(security header), and 29 .. 29 + msdulen (payload) without verifying
that the received SPI frame length len covers those offsets, causing an
out-of-bounds read when msdulen exceeds len - 30:

  BUG: KASAN: slab-out-of-bounds in ca8210_skb_rx.constprop.0.isra.0+0x137/0x160
  Read of size 64 at addr ffff888006453ddd 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
   ca8210_skb_rx.constprop.0.isra.0+0x137/0x160
   ca8210_net_rx+0x96/0xc0
  ...
  The buggy address belongs to the object at ffff888006453dc0
   which belongs to the cache kmalloc-32 of size 32
  The buggy address is located 29 bytes inside of
   allocated 32-byte region [ffff888006453dc0, ffff888006453de0)

Consolidate all length and msdulen validations into a single upfront check
at the beginning of ca8210_skb_rx() before allocating the skb.

Tested in QEMU with KASAN enabled by passing a short data_ind buffer with
msdulen = 64 and len = 30 into ca8210_skb_rx().

Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device 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/3.
- Consolidated all length checks in ca8210_skb_rx() into a single place
  at the beginning of the function before dev_alloc_skb() and dropped the
  unrelated hdr.seq assignment as requested by Miquel Raynal.

 drivers/net/ieee802154/ca8210.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
index 1de6314..2aa7d8c 100644
--- a/drivers/net/ieee802154/ca8210.c
+++ b/drivers/net/ieee802154/ca8210.c
@@ -1759,19 +1759,15 @@ static int ca8210_skb_rx(
 	u8                    *data_ind
 )
 {
-	struct ieee802154_hdr hdr;
+	struct ieee802154_hdr hdr = { };
 	int msdulen;
 	int hlen;
-	u8 mpdulinkquality = data_ind[23];
+	u8 mpdulinkquality;
 	struct sk_buff *skb;
 	struct ca8210_priv *priv = hw->priv;
 
-	/* Allocate mtu size buffer for every rx packet */
-	skb = dev_alloc_skb(IEEE802154_MTU + sizeof(hdr));
-	if (!skb)
-		return -ENOMEM;
-
-	skb_reserve(skb, sizeof(hdr));
+	if (len < 30)
+		return -EMSGSIZE;
 
 	msdulen = data_ind[22]; /* msdu_length */
 	if (msdulen > IEEE802154_MTU) {
@@ -1779,9 +1775,25 @@ static int ca8210_skb_rx(
 			&priv->spi->dev,
 			"received erroneously large msdu length!\n"
 		);
-		kfree_skb(skb);
 		return -EMSGSIZE;
 	}
+
+	if (len < 30 + msdulen ||
+	    (!priv->promiscuous && data_ind[29 + msdulen] > 0 &&
+	     len < 29 + msdulen + sizeof(struct secspec))) {
+		dev_err(&priv->spi->dev,
+			"received truncated data indication!\n");
+		return -EMSGSIZE;
+	}
+
+	mpdulinkquality = data_ind[23];
+
+	/* Allocate mtu size buffer for every rx packet */
+	skb = dev_alloc_skb(IEEE802154_MTU + sizeof(hdr));
+	if (!skb)
+		return -ENOMEM;
+
+	skb_reserve(skb, sizeof(hdr));
 	dev_dbg(&priv->spi->dev, "skb buffer length = %d\n", msdulen);
 
 	if (priv->promiscuous)
-- 
2.47.3

Re: [PATCH net v2 1/3] ieee802154: ca8210: prevent stack buffer overflow in ca8210_rx_done()

[David Laight]

On Mon, 21 Sep 2026 07:42:03 +0000
Hui Peng <benquike@gmail.com> wrote:

> In ca8210_rx_done(), the packet length len = buf[1] + 2 is only checked
> against CA8210_SPI_BUF_SIZE (256). When buf[0] & SPI_SYN is set and
> priv->sync_command_response is non-NULL, memcpy() copies up to 256 bytes
> into priv->sync_command_response, which points to a struct mac_message
> object (sizeof(struct mac_message) = 250 bytes) on the synchronous
> caller's stack, overflowing the stack buffer by up to 6 bytes:
> 
>   BUG: KASAN: stack-out-of-bounds in ca8210_rx_done+0xc4/0xe0
>   Write of size 256 at addr ffff888001907660 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+0x3c/0x60
>    ca8210_rx_done+0xc4/0xe0
>    ca8210_spi_exchange+0x12e/0x190
>   ...
>   The buggy address belongs to stack of task init/1
>    and is located at offset 32 in frame:
>    ca8210_spi_exchange+0x0/0x190
>   This frame has 1 object:
>    [32, 282) 'response'
> 
> Bound the synchronous response memcpy() length to
> min_t(size_t, len, sizeof(struct mac_message)).

Why min_t() ?
len is unsigned so min() is fine.

Shouldn't there also be some kind of indication the data was truncated?

David

> 
> Tested in QEMU with KASAN enabled by passing a 256-byte SPI_SYN response
> into ca8210_rx_done().
> 
> Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
> ---
> Changes in v2:
> - Split the ca8210 fixes into three single-issue patches (1/3..3/3).
> 
>  drivers/net/ieee802154/ca8210.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 01af4f9..c009930 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -697,7 +697,8 @@ static void ca8210_rx_done(struct cas_control *cas_ctl)
>  
>  	if (buf[0] & SPI_SYN) {
>  		if (priv->sync_command_response) {
> -			memcpy(priv->sync_command_response, buf, len);
> +			memcpy(priv->sync_command_response, buf,
> +			       min_t(size_t, len, sizeof(struct mac_message)));
>  			complete(&priv->sync_exchange_complete);
>  		} else {
>  			if (cascoda_api_upstream)

Re: [PATCH net v2 1/3] ieee802154: ca8210: prevent stack buffer overflow in ca8210_rx_done()

[netdev-bot+sashiko]

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

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

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's factual basis is wrong.
- [Medium] ca8210_rx_done() still validates the device-declared frame
  length against the software buffer size instead of the number of bytes…

Pre-existing issues:
- [High] Unsynchronized stale stack pointer: ca8210_rx_done() writes
  device data through priv->sync_command_response, a bare pointer to the…
- [High] Sleeping calls in the spi_async() completion callback.
- [Low] Device-supplied length in the synchronous response consumer:
  hwme_get_request_sync() copies…

--

ieee802154: ca8210: prevent stack buffer overflow in ca8210_rx_done()

This patch clamps the synchronous response memcpy() in ca8210_rx_done() to
min_t(size_t, len, sizeof(struct mac_message)) so that a device-declared
length larger than the destination object cannot overwrite the synchronous
caller's stack frame.

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

The commit message states:

>     priv->sync_command_response is non-NULL, memcpy() copies up to 256 bytes
>     into priv->sync_command_response, which points to a struct mac_message
>     object (sizeof(struct mac_message) = 250 bytes) on the synchronous
>     caller's stack, overflowing the stack buffer by up to 6 bytes:

[Severity: Medium]
Is the 250 byte figure correct?  struct mac_message in this file is:

	struct mac_message {
		u8      command_id;
		u8      length;
		union {
			struct mcps_data_request_pset       data_req;
			...
			u8                                  payload[148];
		} pdata;
	};

All members are u8, so the alignment is 1 and the largest union member is
payload[148] (data_req is 1+11+1+1+1+MAX_DATA_SIZE(114) = 129, set_req is
3+MAX_ATTRIBUTE_SIZE(122) = 125, hwme_get_cnf is 3+MAX_HWME_ATTRIBUTE_SIZE(16)
= 19).  That gives 2 + 148 = 150 bytes, not 250.

If that is right, then with len bounded only by CA8210_SPI_BUF_SIZE the
pre-patch worst case wrote 256 bytes into a 150 byte object, i.e. up to 106
bytes out of bounds rather than 6.  Could the description be corrected, since
stable backporters will be triaging this based on the stated magnitude?

The pasted report also says:

>       The buggy address belongs to stack of task init/1
>        and is located at offset 32 in frame:
>        ca8210_spi_exchange+0x0/0x190
>       This frame has 1 object:
>        [32, 282) 'response'

Can this frame come from this driver?  ca8210_spi_exchange() only receives a
pointer:

	static int ca8210_spi_exchange(
		const u8 *buf,
		size_t len,
		u8 *response,
		void *device_ref
	)

The struct mac_message response objects live in the sync helpers
(tdme_setsfr_request_sync(), mlme_*_request_sync(), hwme_*_request_sync()) and
are 150 bytes, so a 250 byte local named response in ca8210_spi_exchange()
does not appear to exist.  Could the actual reproducer output be included
instead?

> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 01af4f9cf7f2..c009930da77a 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -697,7 +697,8 @@ static void ca8210_rx_done(struct cas_control *cas_ctl)
>  
>  	if (buf[0] & SPI_SYN) {
>  		if (priv->sync_command_response) {
> -			memcpy(priv->sync_command_response, buf, len);
> +			memcpy(priv->sync_command_response, buf,
> +			       min_t(size_t, len, sizeof(struct mac_message)));

[Severity: Medium]
Would it be better to fix the length validation instead of clamping one
consumer?  ca8210_rx_done() still validates the device-declared length against
the software buffer size:

	len = buf[1] + 2;
	if (len > CA8210_SPI_BUF_SIZE) {
		dev_crit(&priv->spi->dev,
			 "Received packet len (%u) erroneously long\n", len);
		goto finish;
	}

but every exchange only clocks in sizeof(struct mac_message) bytes, into a
buffer pre-filled with the idle pattern, in ca8210_spi_transfer():

	memset(cas_ctl->tx_in_buf, SPI_IDLE, CA8210_SPI_BUF_SIZE);
	...
	cas_ctl->transfer.len = sizeof(struct mac_message);

So for any declared len between 151 and 256, bytes 150..len-1 are the driver's
own 0xFF fill and were never driven by the CA8210.  The patch bounds the sync
memcpy only, while the same unbounded len is still handed to the other two
consumers in this function:

	cascoda_api_upstream(buf, len, priv->spi);
	...
	ca8210_net_rx(priv->hw, buf, len);

ca8210_net_rx() then calls ca8210_skb_rx(hw, len - 2, command + 2), so up to
106 bytes of idle fill can be delivered as 802.15.4 payload and exported
through the debugfs test FIFO.  This is reachable with well-formed traffic
too: a maximum size MCPS_DATA_indication (msdu 127 bytes at data_ind offset
29, plus an 11 byte secspec) declares len in the 158..169 range, so its tail
is replaced by 0xFF.

Would bounding the existing check by sizeof(struct mac_message) and dropping
the frame close this for all three consumers at once?

On the sync path the truncated copy is also followed unconditionally by
complete(&priv->sync_exchange_complete), so ca8210_spi_exchange() returns 0
and the caller parses a struct mac_message whose length field describes more
payload than was delivered.  Should a truncated confirm be reported as an
error rather than success?

[Severity: High]
This isn't a bug introduced by this patch, but does the clamp cover the
lifetime of the destination as well as its size?  priv->sync_command_response
is a bare pointer to the synchronous caller's on-stack struct mac_message,
published without any lock in ca8210_spi_exchange():

	if ((buf[0] & SPI_SYN) && response) { /* if sync wait for confirm */
		reinit_completion(&priv->sync_exchange_complete);
		priv->sync_command_response = response;
	}

On the -ETIME (CA8210_SYNC_TIMEOUT) and -ERESTARTSYS paths it simply clears
the pointer and returns, without cancelling or flushing the in-flight SPI
message:

	} else if (wait_remaining == 0) {
		dev_err(&spi->dev, "Synchronous confirm timeout\n");
		status = -ETIME;
	}

cleanup:
	priv->sync_command_response = NULL;
	return status;

The write side runs from the spi_async() completion callback,
ca8210_spi_transfer_complete() -> ca8210_rx_done(), and the confirm read is
queued straight from the hard IRQ handler
ca8210_interrupt_handler() -> ca8210_spi_transfer() -> spi_async().  Neither
side takes priv->lock and there is no READ_ONCE() or barrier, so can this
happen?

CPU0 (spi completion)			CPU1 (sync caller)
if (priv->sync_command_response)	/* non-NULL */
					wait ... times out
					priv->sync_command_response = NULL;
					return -ETIME;
					/* tdme_setsfr_request_sync() etc.
					   returns, frame reused */
memcpy(priv->sync_command_response,
       buf, ... );			/* writes dead stack frame */

Since the pointer is a plain load used both for the NULL test and as the
memcpy destination, and memcpy() is expanded inline through fortify-string,
could the compiler also reload it and observe the concurrent NULL store,
giving memcpy(NULL, ...)?

Would landing the confirm in a driver-owned buffer, or quiescing the
completion path before clearing the pointer, be a better fix than bounding the
length?

>  			complete(&priv->sync_exchange_complete);
>  		} else {
>  			if (cascoda_api_upstream)

[Severity: High]
This is a pre-existing issue and not something this patch introduced, but
while this function is being touched: can ca8210_rx_done() sleep in the
context it runs in?  It is called directly from
ca8210_spi_transfer_complete(), which is installed as cas_ctl->msg.complete
and submitted with spi_async().  The spi_async() kernel-doc in
drivers/spi/spi.c states "The completion callback is invoked in a context
which can't sleep", and ca8210_spi_transfer() itself allocates with
GFP_ATOMIC.

Further down in the same function, for
buf[0] == SPI_MCPS_DATA_CONFIRM with buf[3] == IEEE802154_TRANSACTION_OVERFLOW:

	msleep(2000);
	...
	mlme_reset_wpc = kmalloc_obj(*mlme_reset_wpc);

msleep() and the implicit GFP_KERNEL allocation can both sleep, and the test
path reaches ca8210_test_int_driver_write() which does kmemdup(..., GFP_KERNEL)
as well.  On controllers that finalize messages from hard IRQ or DMA
completion context, would this produce a "sleeping function called from
invalid context" splat, and on kthread-pump controllers stall the bus for two
seconds inside the completion callback?

The MLME reset is already deferred to priv->mlme_workqueue, so could the
msleep() and the allocation move into ca8210_mlme_reset_worker()?

[Severity: Low]
This is a pre-existing issue rather than something introduced here, and the
out-of-bounds part of it is already addressed by patch 2/3 of this series,
which adds:

	if (response.pdata.hwme_get_cnf.status == IEEE802154_SUCCESS) {
		if (response.pdata.hwme_get_cnf.hw_attribute_length >
		    *hw_attribute_length)
			return IEEE802154_SYSTEM_ERROR;

together with u8 lenvar = 1 in ca8210_get_ed().  What remains after the series
is that struct mac_message response is not zero-initialized in
hwme_get_request_sync() and the mlme_*_request_sync() helpers, unlike
tdme_setsfr_request_sync() which sets response.command_id = SPI_IDLE.  Since
ca8210_rx_done() now copies only the bytes the device actually sent, can a
short confirm leave response.command_id, status or the attribute bytes as
stale stack data, so that a wrong ED level or status is returned?  Would
zero-initializing response in those helpers close that?

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

Re: [PATCH net v2 2/3] ieee802154: ca8210: prevent stack buffer overflow in hwme_get_request_sync()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 1

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 presents the KASAN stack-out-of-bounds
  splat as a live, device-triggerable overflow (Fixes: + Cc: stable,…
- [Medium] The new bound check in hwme_get_request_sync()
  (drivers/net/ieee802154/ca8210.c:1681-1683) is one-sided: it only…
- [Medium] hwme_get_request_sync() still declares `struct mac_message
  command, response;` with no initialiser (ca8210.c:1663) and then makes…
- [Low] The patch converts `hw_attribute_length` of
  `hwme_get_request_sync()` into an in/out parameter — the caller must…

--

ieee802154: ca8210: prevent stack buffer overflow in hwme_get_request_sync()

This adds an upper-bound check in hwme_get_request_sync() so that a
HWME-GET.confirm reporting a hw_attribute_length larger than the caller's
destination buffer is rejected with IEEE802154_SYSTEM_ERROR, and
initialises lenvar = 1 in ca8210_get_ed() so the helper knows the
capacity of the one-byte level buffer.

A few questions about the change and the changelog below.

>     In ca8210_get_ed(), an uninitialized u8 lenvar and a pointer to a 1-byte
>     stack buffer (u8 *level) are passed to hwme_get_request_sync(), which
>     unconditionally copies response.pdata.hwme_get_cnf.hw_attribute_length
>     bytes into hw_attribute_value without checking the caller's destination
>     buffer capacity, overflowing level on the stack when hw_attribute_length
>     exceeds 1:
>
>       BUG: KASAN: stack-out-of-bounds in hwme_get_request_sync.constprop.0.isra.0+0xf3/0x170
>       Write of size 16 at addr ffff888001907780 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+0x3c/0x60
>        hwme_get_request_sync.constprop.0.isra.0+0xf3/0x170
>        ca8210_get_ed+0x9c/0xf0
>       ...
> [ ... ]
>     Fixes: ded845a781a5 ("ieee802154: Add CA8210 IEEE 802.15.4 device driver")
>     Cc: stable@vger.kernel.org

[Severity: Medium]
Is this splat reachable from anything in tree today?

ca8210_get_ed() is only installed as the .ed member of ca8210_phy_ops, and
the only reference to that member in the 802.15.4 stack is the presence
check in ieee802154_alloc_hw():

net/mac802154/main.c:ieee802154_alloc_hw() {
	if (WARN_ON(!ops || !(ops->xmit_async || ops->xmit_sync) || !ops->ed ||
		    !ops->start || !ops->stop || !ops->set_channel))
		return NULL;
}

There is no drv_* wrapper, cfg802154 op or netlink path that actually calls
ops->ed, and ca8210_get_ed() is the single caller of
hwme_get_request_sync(). The changelog itself notes the trace came from
passing "an oversized hw_attribute_length response into ca8210_get_ed()"
in QEMU.

Could the changelog say that the path is currently only reachable through
the never-invoked .ed callback? With a Fixes: tag, a Cc: stable and a call
trace that is truncated right above ca8210_get_ed(), a stable backporter
would reasonably conclude that shipping kernels have a device-reachable
stack overflow here.

> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index c009930da77a..1de63142a45a 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -1678,6 +1678,9 @@ static u8 hwme_get_request_sync(
>  		return IEEE802154_SYSTEM_ERROR;
>  
>  	if (response.pdata.hwme_get_cnf.status == IEEE802154_SUCCESS) {
> +		if (response.pdata.hwme_get_cnf.hw_attribute_length >
> +		    *hw_attribute_length)
> +			return IEEE802154_SYSTEM_ERROR;
>  		*hw_attribute_length =
>  			response.pdata.hwme_get_cnf.hw_attribute_length;
>  		memcpy(

[Severity: Medium]
Should this check also reject a shorter-than-expected length?

The test only covers lengths larger than the caller's capacity. A confirm
with status == IEEE802154_SUCCESS and hw_attribute_length == 0 still takes
the success path: *hw_attribute_length becomes 0, the memcpy copies
nothing, and IEEE802154_SUCCESS is returned.

ca8210_get_ed() discards lenvar, so link_to_linux_err(IEEE802154_SUCCESS)
returns 0:

drivers/net/ieee802154/ca8210.c:ca8210_get_ed() {
	u8 lenvar = 1;
	struct ca8210_priv *priv = hw->priv;

	return link_to_linux_err(
		hwme_get_request_sync(HWME_EDVALUE, &lenvar, level, priv->spi)
	);
}

Does that report a successful energy-detect read while *level still holds
the caller's uninitialised stack byte, contrary to the ieee802154_ops::ed
contract in include/net/mac802154.h that a 0 return means level was filled
in?

HWME attribute sizes are fixed and known to the caller, so would an exact
length comparison (!= against the caller-supplied expected length) cover
both directions of the mismatch in one check?

[Severity: Medium]
Can this check be made on never-written stack memory?

hwme_get_request_sync() declares its response with no initialiser:

	struct mac_message command, response;

The only writer of that memory is ca8210_rx_done(), which copies exactly as
many bytes as the device claims:

drivers/net/ieee802154/ca8210.c:ca8210_rx_done() {
	buf = cas_ctl->tx_in_buf;
	len = buf[1] + 2;
	...
	if (buf[0] & SPI_SYN) {
		if (priv->sync_command_response) {
			memcpy(priv->sync_command_response, buf,
			       min_t(size_t, len, sizeof(struct mac_message)));
}

SPI_HWME_GET_CONFIRM is HWME_GET_CONFIRM + SPI_S2M + SPI_SYN, so a frame
with buf[0] = 0x73 and buf[1] = 0 passes the

	if (response.command_id != SPI_HWME_GET_CONFIRM)

gate while leaving .status, .hw_attribute_length and .hw_attribute_value[]
as stack garbage.

If the garbage status happens to be 0 and the garbage length happens to be
0 or 1, does this copy up to one byte of uninitialised kernel stack into
the caller's *level and return success? And for other garbage status values,
does link_to_linux_err() map them to an arbitrary errno?

Would zeroing response, or validating response.length against the size of
struct hwme_get_confirm_pset, close this along with the out-of-bounds write
the patch targets?

> @@ -2028,7 +2031,7 @@ static int ca8210_xmit_async(struct ieee802154_hw *hw, struct sk_buff *skb)
>   */
>  static int ca8210_get_ed(struct ieee802154_hw *hw, u8 *level)
>  {
> -	u8 lenvar;
> +	u8 lenvar = 1;
>  	struct ca8210_priv *priv = hw->priv;
>  
>  	return link_to_linux_err(

[Severity: Low]
This isn't a bug, but the kernel-doc above hwme_get_request_sync() no
longer matches the new calling convention:

 * @hw_attribute_length: Attribute length
 * @hw_attribute_value:  Pointer to Attribute Value
 * @device_ref:          Nondescript pointer to target device
 *
 * Return: 802.15.4 status code of HWME-GET.confirm

hw_attribute_length is now an in/out parameter that the caller must
pre-initialise with the capacity of hw_attribute_value, which is what makes
lenvar = 1 load-bearing here, and the function can now return
IEEE802154_SYSTEM_ERROR for a purely local capacity rejection rather than a
device confirm status (which link_to_linux_err() turns into -EPROTO).

Could the kernel-doc be updated to state both, so a future caller reading
only the documented interface does not pass an uninitialised or zero length
and reintroduce the overflow or the silent truncation?

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

Re: [PATCH net v2 3/3] ieee802154: ca8210: validate data_ind length upfront in ca8210_skb_rx()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 1

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 misrepresents the defect being fixed.
- [Medium] The patch silently changes `struct ieee802154_hdr hdr;` to
  `struct ieee802154_hdr hdr = { };` in ca8210_skb_rx()…
- [Medium] The consolidated validation is placed in the callee, but the
  caller already parses the same MCPS_DATA_indication at a fixed offset…
- [Low] The newly added validation and the code it protects read
  priv->promiscuous twice, with no snapshot, lock, or READ_ONCE().

--

ieee802154: ca8210: validate data_ind length upfront in ca8210_skb_rx()

This moves all length and msdulen validation for an MCPS_DATA_indication to
the top of ca8210_skb_rx(), before dev_alloc_skb(), and adds a check that the
declared SPI frame length covers the payload and the optional security header.
The struct ieee802154_hdr on the stack is also changed to be zero-initialised.

First, a question about the commit message itself:

>     In ca8210_skb_rx(), data_ind is read at offsets 22 (msdulen), 23
>     (mpdulinkquality), 29 + msdulen (hdr.sec.level), 30..39 + msdulen
>     (security header), and 29 .. 29 + msdulen (payload) without verifying
>     that the received SPI frame length len covers those offsets, causing an
>     out-of-bounds read when msdulen exceeds len - 30:
>
>       BUG: KASAN: slab-out-of-bounds in ca8210_skb_rx.constprop.0.isra.0+0x137/0x160
>       ...
>       The buggy address belongs to the object at ffff888006453dc0
>        which belongs to the cache kmalloc-32 of size 32

[Severity: Medium]
Can this kmalloc-32 slab-out-of-bounds actually be reached in-tree?  There is
only one caller of ca8210_skb_rx(), and it passes a pointer into a fixed-size
array rather than a len-sized allocation:

drivers/net/ieee802154/ca8210.c:ca8210_net_rx() {
	...
	return ca8210_skb_rx(hw, len - 2, command + 2);
	...
}

command is always cas_ctl->tx_in_buf:

struct cas_control {
	...
	u8 tx_buf[CA8210_SPI_BUF_SIZE];
	u8 tx_in_buf[CA8210_SPI_BUF_SIZE];
	...
};

With msdulen rejected above IEEE802154_MTU (127), the highest byte the parser
can touch is data_ind[39 + 127], i.e. tx_in_buf[168], which is inside the
256-byte array, and the payload copy tops out at tx_in_buf[157].  The same
was true before the patch.

The test note also suggests the splat comes from calling the static function
directly rather than from the driver path:

>     Tested in QEMU with KASAN enabled by passing a short data_ind buffer with
>     msdulen = 64 and len = 30 into ca8210_skb_rx().

What the driver does appear to do without the new checks is consume bytes
beyond the firmware-declared frame length (leftover bytes of the same SPI
exchange, or SPI_IDLE fill) and copy them into the skb handed to
ieee802154_rx_irqsafe() - a stale/garbage frame rather than memory
corruption.  Given the Fixes: tag and Cc: stable, would it be worth
rewording the message so backporters do not triage this as a remotely
triggerable slab out-of-bounds read?

> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index 1de63142a45a..2aa7d8c8ae00 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -1759,19 +1759,15 @@ static int ca8210_skb_rx(
>  	u8                    *data_ind
>  )
>  {
> -	struct ieee802154_hdr hdr;
> +	struct ieee802154_hdr hdr = { };

[Severity: Medium]
Should this hunk be described in the commit message?  The body only mentions
consolidating the length and msdulen validation, but this line looks like a
separate fix with a visible behaviour change.

ca8210_skb_rx() never assigns hdr.seq, hdr.sec.frame_counter, or the fc bits
frame_pending/ack_request/version/reserved, yet ieee802154_hdr_push()
serialises them unconditionally:

net/ieee802154/header_ops.c:ieee802154_hdr_push() {
	...
	buf[pos++] = hdr->seq;
	...
	memcpy(buf, fc, 2);
	...
}

and for security-enabled frames:

net/ieee802154/header_ops.c:ieee802154_hdr_push_sechdr() {
	...
	memcpy(buf + 1, &hdr->frame_counter, 4);
	...
}

So before this change, every non-promiscuous received frame carried a few
bytes of uninitialised stack in the reconstructed MAC header delivered to the
802.15.4 stack.  Zeroing closes that, but also makes the reported sequence
number always 0 and clears the fc bits.  Would it make sense to split this
out with its own description and Fixes: tag?

>  	int msdulen;
>  	int hlen;
> -	u8 mpdulinkquality = data_ind[23];
> +	u8 mpdulinkquality;
>  	struct sk_buff *skb;
>  	struct ca8210_priv *priv = hw->priv;
>  
> -	/* Allocate mtu size buffer for every rx packet */
> -	skb = dev_alloc_skb(IEEE802154_MTU + sizeof(hdr));
> -	if (!skb)
> -		return -ENOMEM;
> -
> -	skb_reserve(skb, sizeof(hdr));
> +	if (len < 30)
> +		return -EMSGSIZE;

[Severity: Medium]
Is the callee the right place for this check, given that the only caller
already parses the same indication at a fixed offset first?

drivers/net/ieee802154/ca8210.c:ca8210_net_rx() {
	...
	spin_lock_irqsave(&priv->lock, flags);
	if (command[26] == priv->last_dsn) {
		...
		return 0;
	}
	priv->last_dsn = command[26];
	spin_unlock_irqrestore(&priv->lock, flags);
	return ca8210_skb_rx(hw, len - 2, command + 2);
	...
}

command[26] is data_ind[24], which is inside the region the new len >= 30
check is meant to guarantee.  len itself comes straight from the device:

drivers/net/ieee802154/ca8210.c:ca8210_rx_done() {
	...
	len = buf[1] + 2;
	if (len > CA8210_SPI_BUF_SIZE) {
	...
}

only the upper bound is checked, so a declared length of 2..31 still reaches
the offset-26 read.  Can that latch priv->last_dsn from a byte outside the
declared frame, so that a later genuine frame whose DSN matches is dropped as
a resend, even though the frame that set it is then rejected with -EMSGSIZE?
Would moving the validation (or the last_dsn update) ahead of the DSN read
close that gap?

>  
>  	msdulen = data_ind[22]; /* msdu_length */
>  	if (msdulen > IEEE802154_MTU) {
> @@ -1779,9 +1775,25 @@ static int ca8210_skb_rx(
>  			&priv->spi->dev,
>  			"received erroneously large msdu length!\n"
>  		);
> -		kfree_skb(skb);
>  		return -EMSGSIZE;
>  	}
> +
> +	if (len < 30 + msdulen ||
> +	    (!priv->promiscuous && data_ind[29 + msdulen] > 0 &&
> +	     len < 29 + msdulen + sizeof(struct secspec))) {

[Severity: Low]
This check and the code it protects read priv->promiscuous twice, with no
snapshot, lock or READ_ONCE().  The stronger bound (len >= 40 + msdulen) is
only applied here when !priv->promiscuous, and the decision to parse the
security header is taken from a second read further down:

	if (priv->promiscuous)
		goto copy_payload;

	/* Populate hdr */
	hdr.sec.level = data_ind[29 + msdulen];
	...
		hdr.sec.key_id = data_ind[39 + msdulen];

priv->promiscuous is a plain bool written from ca8210_set_promiscuous_mode()
in process context and from ca8210_reset_send(), while the RX path runs from
ca8210_spi_transfer_complete() -> ca8210_rx_done() -> ca8210_net_rx() ->
ca8210_skb_rx().

If the flag goes true to false (or is simply reloaded) between the two reads,
does the security header parse then run on a frame only validated to
30 + msdulen bytes, reading up to 10 bytes past the declared frame length and
pushing them into the reconstructed header?  Would taking one snapshot at
entry, or making the len >= 40 + msdulen requirement unconditional whenever
the security-level byte is non-zero, be preferable?

> +		dev_err(&priv->spi->dev,
> +			"received truncated data indication!\n");
> +		return -EMSGSIZE;
> +	}
> +
> +	mpdulinkquality = data_ind[23];
> +
> +	/* Allocate mtu size buffer for every rx packet */
> +	skb = dev_alloc_skb(IEEE802154_MTU + sizeof(hdr));
> +	if (!skb)
> +		return -ENOMEM;
> +
> +	skb_reserve(skb, sizeof(hdr));

[ ... ]

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