Re: [PATCH] mac802154: rx: fix beacon/mac_cmd skb leak and short skb_trim underflow

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 19/09/2026 at 21:36:36 GMT, Hui Peng <benquike@gmail.com> wrote:

> Fix two sk_buff handling bugs in `net/mac802154/rx.c`:
>
> 1. In `ieee802154_subif_frame()`, the `skb` passed by
>    `__ieee802154_rx_handle_packet()` is already a dedicated
>    `skb_clone()` owned by `ieee802154_subif_frame()` (with `skb->users
>    == 1`), and all normal return paths consume it without extra
>    reference increments. However, the `IEEE802154_FC_TYPE_BEACON` and
>    `IEEE802154_FC_TYPE_MAC_CMD` branches call `mac_pkt->skb =
>    skb_get(skb)` (incrementing `skb->users` to `2`) and return
>    `NET_RX_SUCCESS`. When the worker later calls
>    `kfree_skb(mac_pkt->skb)`, `skb->users` only drops from `2` to `1`,
>    permanently leaking every received Beacon and MAC Command `sk_buff`.
>    Assign `mac_pkt->skb = skb` directly.

This is okay and should probably an independent fix.

> 2. In `ieee802154_rx()`, when `IEEE802154_HW_RX_OMIT_CKSUM` is set,
>    `skb_put(skb, 2)` is called without ensuring 2 bytes of tailroom, and
>    when `IEEE802154_HW_RX_OMIT_CKSUM` is not set,
>    `__ieee802154_rx_handle_packet()` calls `skb_trim(skb, skb->len - 2)`
>    without verifying `skb->len >= 2`. Ensure tailroom with
>    `pskb_expand_head(skb, 0, 2, GFP_ATOMIC)` and drop frames shorter
>    than 2 bytes.
>
> Fixes: 57588c71177f ("mac802154: Handle passive scanning")
> Fixes: d021d218f6d9 ("mac802154: Handle received BEACON_REQ")
> Assisted-by: LLM
> Signed-off-by: Hui Peng <benquike@gmail.com>
>
> ---
>  net/mac802154/rx.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/net/mac802154/rx.c b/net/mac802154/rx.c
> index 19b5382e85a8..dbdaf5962ca7 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);
> @@ -487,10 +487,15 @@ 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 (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));

This, why not.

>  	}
>  
> +	if (skb->len < 2)
> +		goto free_skb;

But this? This is a situation that cannot happen, right? Either the PHY
adds the checksum, or it does not and we add it above.

> +
>  	rcu_read_lock();
>  
>  	ieee802154_monitors_rx(local, skb);

Miquèl