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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message reads as if the stack overflow in
  ca8210_get_ed()/hwme_get_request_sync() can happen at runtime.
- [Low] The patch turns `hw_attribute_length` in hwme_get_request_sync()
  into an in/out parameter, but the kernel-doc was not updated.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it is a real
  memory-safety race in the same sync-exchange path.

--

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

This patch initializes lenvar to 1 in ca8210_get_ed(). It also makes
hwme_get_request_sync() return IEEE802154_SYSTEM_ERROR when the device
reports an hw_attribute_length larger than the caller's buffer. The goal
is to stop a stack overflow of the 1-byte level buffer in ca8210_get_ed().

> 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

[ ... ]

> 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

[Severity: Low]
Can this overflow actually be reached at runtime? The commit message reads
as if it describes live stack corruption. It shows a splat from task init/1
and carries a stable tag.

hwme_get_request_sync() is static. Its only caller is ca8210_get_ed(), and
ca8210_get_ed() is only referenced as the .ed callback in ca8210_phy_ops:

	.ed = ca8210_get_ed,

As far as I can tell, mac802154 never calls ops->ed. The only reference is
the NULL check in ieee802154_alloc_hw():

net/mac802154/main.c:ieee802154_alloc_hw() {
	...
	if (WARN_ON(!ops || !(ops->xmit_async || ops->xmit_sync) || !ops->ed ||
	...
}

nl802154_trigger_scan() also accepts only NL802154_SCAN_ACTIVE and
NL802154_SCAN_PASSIVE. So an energy-detection scan from userspace can't
get there either.

Even if there were a caller, the device would have to answer
HWME_GET(HWME_EDVALUE) with hw_attribute_length > 1.

Could the commit message say that the .ed op is not currently invoked, and
that a misbehaving CA8210 would be needed? That would help maintainers
choose between net and net-next, and decide whether a stable backport is
needed.

> diff --git a/drivers/net/ieee802154/ca8210.c b/drivers/net/ieee802154/ca8210.c
> index a990a0f347ad..8aa7ffeb22d7 100644
> --- a/drivers/net/ieee802154/ca8210.c
> +++ b/drivers/net/ieee802154/ca8210.c
> @@ -1677,6 +1677,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;

[Severity: Low]
Should the kernel-doc for hwme_get_request_sync() be updated too?

*hw_attribute_length is now read as the size of hw_attribute_value, and
then overwritten with the copied length. That makes it an in/out
parameter. The comment still says:

 * @hw_attribute_length: Attribute length
 ...
 * Return: 802.15.4 status code of HWME-GET.confirm

Before this patch the argument was only written, which is why
ca8210_get_ed() could pass an uninitialized lenvar. A future caller that
follows the kernel-doc and does the same would compare against a random
value, and the overflow would come back.

The only caller meets the new requirement with a bare "u8 lenvar = 1;".
Nothing ties that 1 to sizeof(*level).

The Return: text is now also wrong in one more case. This
IEEE802154_SYSTEM_ERROR comes from the driver, not from the device's
confirm, and link_to_linux_err() maps it to -EPROTO. Two older
SYSTEM_ERROR returns had already made that text inaccurate.

>  		*hw_attribute_length =
>  			response.pdata.hwme_get_cnf.hw_attribute_length;
>  		memcpy(

[Severity: High]
This isn't a bug introduced by this patch, but there seems to be a race in
the sync exchange path that this length check can't protect against.

priv->sync_command_response is per-device state shared by all sync
callers, and no lock covers it. ca8210_spi_exchange() publishes the
caller's stack buffer, waits interruptibly with a timeout, and then clears
the pointer:

drivers/net/ieee802154/ca8210.c: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;
	}
	...
cleanup:
	priv->sync_command_response = NULL;
	return status;
}

ca8210_rx_done() runs from the SPI completion callback
ca8210_spi_transfer_complete(). It tests the pointer and then copies into
it, with no lock:

drivers/net/ieee802154/ca8210.c:ca8210_rx_done() {
	...
	if (buf[0] & SPI_SYN) {
		if (priv->sync_command_response) {
			memcpy(priv->sync_command_response, buf, len);
			complete(&priv->sync_exchange_complete);
	...
}

Suppose the waiter gets -ERESTARTSYS or -ETIME after ca8210_rx_done() has
loaded the non-NULL pointer but before the memcpy. Can that memcpy then
write into the stack frame of a caller that has already returned from
hwme_get_request_sync()?

Sync exchanges also don't appear to be serialized.
ca8210_mlme_reset_worker() calls mlme_reset_request_sync() from a
workqueue without rtnl, and ca8210_rx_done() queues it on
TRANSACTION_OVERFLOW. It can run at the same time as rtnl-held
ieee802154_ops callbacks such as set_channel or set_txpower, which also
use the *_request_sync() helpers.

Could two callers overwrite each other's response pointer, or consume each
other's completion? One caller would get the other command's confirm. The
other would either time out or return with its response buffer untouched,
because hwme_get_request_sync() doesn't preset command_id.

Would it make sense to serialize sync exchanges with a mutex? The handoff
of the response buffer would also need to be synchronized against
ca8210_rx_done(). Ideally, the confirm ID would also be checked against
the request that was issued.

[ ... ]

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