Re: [PATCH net v2 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 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