[PATCH net v2 1/2] mac802154: rx: fix skb leak in beacon and mac_cmd rx paths

[PATCH net v2 1/2] mac802154: rx: fix skb leak in beacon and mac_cmd rx paths

[Hui Peng]

In __ieee802154_rx_handle_packet(), skb_clone() already creates a new
skb with an initial reference count of 1 before passing it to
ieee802154_subif_frame(). When handling IEEE802154_FC_TYPE_BEACON and
IEEE802154_FC_TYPE_MAC_CMD frames, ieee802154_subif_frame() calls
skb_get(skb) to increment the reference count to 2 before queueing the
work item, and then returns NET_RX_SUCCESS without freeing the skb.
When mac802154_rx_beacon_worker() or mac802154_rx_mac_cmd_worker()
eventually calls kfree_skb(), the reference count only drops to 1,
leaking the cloned skb on every beacon and MAC command frame received.

Remove the redundant skb_get(skb) calls so the worker consumes the
reference from skb_clone().

Tested in QEMU with mac802154_hwsim by transmitting 2000 IEEE 802.15.4
beacon and MAC command frames across two linked hwsim virtual radios,
reducing the SUnreclaim slab delta from +680 kB on the unfixed kernel
to -80 kB (0 leaked skbs) on the fixed kernel.

Fixes: 57588c71177f ("mac802154: Handle passive scanning")
Fixes: d021d218f6d9 ("mac802154: Handle received BEACON_REQ")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split the beacon/mac_cmd skb leak fix and the IEEE802154_HW_RX_OMIT_CKSUM
  tailroom expansion into a two-patch series as suggested by Miquel Raynal.

 net/mac802154/rx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index 19b5382..ad602d2 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -287,7 +287,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		if (!mac_pkt)
 			goto fail;
 
-		mac_pkt->skb = skb_get(skb);
+		mac_pkt->skb = skb;
 		mac_pkt->sdata = sdata;
 		mac_pkt->page = sdata->local->scan_page;
 		mac_pkt->channel = sdata->local->scan_channel;
@@ -304,7 +304,7 @@ ieee802154_subif_frame(struct ieee802154_sub_if_data *sdata,
 		if (!mac_pkt)
 			goto fail;
 
-		mac_pkt->skb = skb_get(skb);
+		mac_pkt->skb = skb;
 		mac_pkt->sdata = sdata;
 		netdev_hold(sdata->dev, &mac_pkt->dev_tracker, GFP_ATOMIC);
 		spin_lock(&sdata->local->rx_lock);
-- 
2.47.3

[PATCH net v2 2/2] mac802154: rx: ensure tailroom before appending CRC

[Hui Peng]

When IEEE802154_HW_RX_OMIT_CKSUM is set, ieee802154_rx() appends a 2-byte
calculated CRC via skb_put(skb, 2) without checking whether the driver
allocated sufficient tailroom in the received skb, triggering
skb_over_panic when skb_tailroom(skb) < 2:

  skbuff: skb_over_panic: text:ffffffffb6e61896 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
   ieee802154_rx+0xb26/0x1d30
   ieee802154_tasklet_handler+0xa9/0xd0
   tasklet_action_common+0x2ff/0x850
   handle_softirqs+0x188/0x4e0

Ensure at least 2 bytes of tailroom are available via pskb_expand_head()
before calling skb_put().

Tested in QEMU with KASAN by injecting a zero-tailroom skb into
ieee802154_rx() with IEEE802154_HW_RX_OMIT_CKSUM set.

Fixes: b7889497d306 ("mac802154: rx: simplify crc receive handling")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>
---
Changes in v2:
- Split from the beacon/mac_cmd skb leak fix into patch 2/2.
- Dropped the redundant skb->len < 2 check as noted by Miquel Raynal.
- Only call pskb_expand_head() when skb_tailroom(skb) < 2.

 net/mac802154/rx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
index ad602d2..6aa5920 100644
--- a/net/mac802154/rx.c
+++ b/net/mac802154/rx.c
@@ -487,6 +487,9 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
 	 * solution because the monitor needs a crc here.
 	 */
 	if (local->hw.flags & IEEE802154_HW_RX_OMIT_CKSUM) {
+		if (skb_tailroom(skb) < 2 &&
+		    pskb_expand_head(skb, 0, 2, GFP_ATOMIC))
+			goto free_skb;
 		crc = crc_ccitt(0, skb->data, skb->len);
 		put_unaligned_le16(crc, skb_put(skb, 2));
 	}
-- 
2.47.3

Re: [PATCH net v2 2/2] mac802154: rx: ensure tailroom before appending CRC

[netdev-bot+sashiko]

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

Critical: 0 · High: 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).

- [High] The new guard in ieee802154_rx() (net/mac802154/rx.c:490-492)
  uses the wrong predicate and the wrong primitive, so it does not close…
- [Medium] The commit message presents an observed `skb_over_panic` BUG
  splat (`skbuff: skb_over_panic: text:... len:386 put:2` / `kernel BUG…

--

mac802154: rx: ensure tailroom before appending CRC

This adds a tailroom check in ieee802154_rx() before the 2-byte CRC is
appended with skb_put() when IEEE802154_HW_RX_OMIT_CKSUM is set, calling
pskb_expand_head() when skb_tailroom(skb) is less than 2 and dropping the
frame if the expansion fails.

A couple of questions about the commit message and the new guard.

>     When IEEE802154_HW_RX_OMIT_CKSUM is set, ieee802154_rx() appends a 2-byte
>     calculated CRC via skb_put(skb, 2) without checking whether the driver
>     allocated sufficient tailroom in the received skb, triggering
>     skb_over_panic when skb_tailroom(skb) < 2:
>
>       skbuff: skb_over_panic: text:ffffffffb6e61896 len:386 put:2
>       kernel BUG at net/core/skbuff.c:214!
>
>     Tested in QEMU with KASAN by injecting a zero-tailroom skb into
>     ieee802154_rx() with IEEE802154_HW_RX_OMIT_CKSUM set.
>
>     Fixes: b7889497d306 ("mac802154: rx: simplify crc receive handling")
>     Cc: stable@vger.kernel.org

[Severity: Medium]
Can this splat be reproduced with an in-tree driver?

IEEE802154_HW_RX_OMIT_CKSUM only reaches drivers through the combined
IEEE802154_HW_OMIT_CKSUM macro in include/net/mac802154.h, and only
adf7242 and ca8210 set it.  The remaining drivers (at86rf230, atusb,
cc2520, mcr20a, mrf24j40) set just IEEE802154_HW_TX_OMIT_CKSUM, and
mac802154_hwsim sets only IEEE802154_HW_PROMISCUOUS in hwsim_add_one(),
so none of them enter the patched branch.

adf7242_rx() guarantees two bytes of tailroom by construction:

drivers/net/ieee802154/adf7242.c:adf7242_rx() {
	skb = dev_alloc_skb(len);
	...
	skb_put(skb, len);
	...
	skb_trim(skb, len - 2);	/* Don't put RSSI/LQI or CRC into the frame */

	ieee802154_rx_irqsafe(lp->hw, skb, lqi);
}

ca8210_skb_rx() allocates dev_alloc_skb(IEEE802154_MTU + sizeof(hdr)),
reserves sizeof(hdr), and rejects msdulen > IEEE802154_MTU, so the skb it
hands up has hundreds of bytes of tailroom.

The len:386 value in the splat also looks inconsistent with this
subsystem, since 802.15.4 frames are bounded by IEEE802154_MTU (127); the
largest skb->len reaching this point is around 127 for adf7242 and around
150 for ca8210 including the pushed header.

Together with the note that the trace comes from "injecting a
zero-tailroom skb into ieee802154_rx()", should the Fixes: tag and the
Cc: stable be dropped and the change described as hardening the
ieee802154_rx_irqsafe() contract instead?  Otherwise, could the concrete
driver and path that delivers a short-tailroom skb be named?

> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index ad602d222e86..6aa59202f088 100644
> --- a/net/mac802154/rx.c
> +++ b/net/mac802154/rx.c
> @@ -487,6 +487,9 @@ void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
>  	 * solution because the monitor needs a crc here.
>  	 */
>  	if (local->hw.flags & IEEE802154_HW_RX_OMIT_CKSUM) {
> +		if (skb_tailroom(skb) < 2 &&
> +		    pskb_expand_head(skb, 0, 2, GFP_ATOMIC))
> +			goto free_skb;

[Severity: High]
Does this guard still leave a BUG() reachable for a non-linear skb?

skb_tailroom() reports zero for any skb with data_len > 0:

include/linux/skbuff.h:skb_tailroom() {
	return skb_is_nonlinear(skb) ? 0 : skb->end - skb->tail;
}

so the first half of the condition is always true in that case, and
pskb_expand_head() only grows the head buffer:

net/core/skbuff.c:pskb_expand_head() {
	memcpy(data + nhead, skb->head, skb_tail_pointer(skb) - skb->head);
	memcpy((struct skb_shared_info *)(data + size), skb_shinfo(skb),
	       offsetof(struct skb_shared_info, frags[skb_shinfo(skb)->nr_frags]));
	...
}

It preserves frags, frag_list and skb->data_len, so it does not linearize
and it returns 0, meaning the goto free_skb is not taken.

>  		crc = crc_ccitt(0, skb->data, skb->len);
>  		put_unaligned_le16(crc, skb_put(skb, 2));

With the guard passed, crc_ccitt() then reads skb->len bytes starting at
skb->data even though only skb_headlen(skb) bytes live in the linear area.
Is that a slab out-of-bounds read?  The resulting CRC is appended and
cloned to every monitor interface:

net/mac802154/rx.c:ieee802154_monitors_rx() {
	skb2 = skb_clone(skb, GFP_ATOMIC);
	if (skb2) {
		skb2->dev = sdata->dev;
		ieee802154_deliver_skb(skb2);
	}
}

which would make the over-read bytes visible from user space via a packet
socket bound to the monitor wpan netdev.

skb_put() then still aborts before any tail/end arithmetic:

net/core/skbuff.c:skb_put() {
	void *tmp = skb_tail_pointer(skb);
	SKB_LINEAR_ASSERT(skb);
	...
}

include/linux/skbuff.h:
#define SKB_LINEAR_ASSERT(skb)  BUG_ON(skb_is_nonlinear(skb))

So the failure signature the commit message describes, a kernel BUG
reached from skb_put() inside ieee802154_rx(), appears to remain, just
from a different assertion and now preceded by an out-of-bounds read.
There is no CONFIG guard on SKB_LINEAR_ASSERT, so this holds in all
configurations.

Also, for a non-linear skb the condition is true on every received frame,
so does this add a head reallocation per frame?

Would a predicate that matches what skb_put() actually requires be better
here, for example skb_cow()/skb_linearize(), skb_copy_expand(), or an
explicit skb_is_nonlinear() rejection?  The TX counterpart in
net/mac802154/tx.c:ieee802154_tx() implements the same "reserve FCS
tailroom then skb_put" contract with skb_copy_expand(skb, 0,
IEEE802154_FCS_LEN, GFP_ATOMIC), which yields a linear unshared buffer and
also spells the size as IEEE802154_FCS_LEN rather than a hard-coded 2.

>  	}

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