[bug report] mac802154: Move an skb free within the rx path

[bug report] mac802154: Move an skb free within the rx path

[Dan Carpenter]

Hello Miquel Raynal,

The patch 4d1c7d87030b: "mac802154: Move an skb free within the rx
path" from Oct 26, 2022, leads to the following Smatch static checker
warning:

	net/mac802154/rx.c:307 ieee802154_rx()
	warn: 'skb' was already freed.

net/mac802154/rx.c
    271 void ieee802154_rx(struct ieee802154_local *local, struct sk_buff *skb)
    272 {
    273         u16 crc;
    274 
    275         WARN_ON_ONCE(softirq_count() == 0);
    276 
    277         if (local->suspended)
    278                 goto free_skb;
    279 
    280         /* TODO: When a transceiver omits the checksum here, we
    281          * add an own calculated one. This is currently an ugly
    282          * solution because the monitor needs a crc here.
    283          */
    284         if (local->hw.flags & IEEE802154_HW_RX_OMIT_CKSUM) {
    285                 crc = crc_ccitt(0, skb->data, skb->len);
    286                 put_unaligned_le16(crc, skb_put(skb, 2));
    287         }
    288 
    289         rcu_read_lock();
    290 
    291         ieee802154_monitors_rx(local, skb);
    292 
    293         /* Level 1 filtering: Check the FCS by software when relevant */
    294         if (local->hw.phy->filtering == IEEE802154_FILTERING_NONE) {
    295                 crc = crc_ccitt(0, skb->data, skb->len);
    296                 if (crc)
    297                         goto drop;
    298         }
    299         /* remove crc */
    300         skb_trim(skb, skb->len - 2);
    301 
    302         __ieee802154_rx_handle_packet(local, skb);

This frees skb.

    303 
    304 drop:
    305         rcu_read_unlock();
    306 free_skb:
--> 307         kfree_skb(skb);

Double free.

    308 }

regards,
dan carpenter

Re: [bug report] mac802154: Move an skb free within the rx path

[Miquel Raynal]

Hi Dan,

- wireless@ (not relevant for net/ieee802154 and net/mac802154 changes)
+ Alex and Stefan, the 802154 maintainers

error27@gmail.com wrote on Thu, 15 Dec 2022 11:05:47 +0300:

> Hello Miquel Raynal,
> 
> The patch 4d1c7d87030b: "mac802154: Move an skb free within the rx
> path" from Oct 26, 2022, leads to the following Smatch static checker
> warning:
> 
> 	net/mac802154/rx.c:307 ieee802154_rx()
> 	warn: 'skb' was already freed.

It took me a good minute to figure this one out, actually the main
purpose of commit 4d1c7d87030b ("mac802154: Move an skb free within the
rx path") is to do the freeing in one part, that's why we no longer need to
do it in __ieee802154_rx_handle_packet(). But, well, I forgot the very
first check at the top which was still freeing the skb upon parsing error.

I will immediately send a fix, thanks for the report.

Miquèl

> net/mac802154/rx.c
>     271 void ieee802154_rx(struct ieee802154_local *local, struct
> sk_buff *skb) 272 {
>     273         u16 crc;
>     274 
>     275         WARN_ON_ONCE(softirq_count() == 0);
>     276 
>     277         if (local->suspended)
>     278                 goto free_skb;
>     279 
>     280         /* TODO: When a transceiver omits the checksum here,
> we 281          * add an own calculated one. This is currently an ugly
>     282          * solution because the monitor needs a crc here.
>     283          */
>     284         if (local->hw.flags & IEEE802154_HW_RX_OMIT_CKSUM) {
>     285                 crc = crc_ccitt(0, skb->data, skb->len);
>     286                 put_unaligned_le16(crc, skb_put(skb, 2));
>     287         }
>     288 
>     289         rcu_read_lock();
>     290 
>     291         ieee802154_monitors_rx(local, skb);
>     292 
>     293         /* Level 1 filtering: Check the FCS by software when
> relevant */ 294         if (local->hw.phy->filtering ==
> IEEE802154_FILTERING_NONE) { 295                 crc = crc_ccitt(0,
> skb->data, skb->len); 296                 if (crc)
>     297                         goto drop;
>     298         }
>     299         /* remove crc */
>     300         skb_trim(skb, skb->len - 2);
>     301 
>     302         __ieee802154_rx_handle_packet(local, skb);
> 
> This frees skb.
> 
>     303 
>     304 drop:
>     305         rcu_read_unlock();
>     306 free_skb:
> --> 307         kfree_skb(skb);  
> 
> Double free.
> 
>     308 }
> 
> regards,
> dan carpenter