On 10/07/2019 14:52, Sabrina Dubroca wrote:
> When __netif_receive_skb_core handles a shared skb, it can be
> reallocated in a few different places:
> - the device's rx_handler
> - vlan_do_receive
> - skb_vlan_untag
>
> To deal with that, rx_handlers and vlan_do_receive get passed a
> reference to the skb, and skb_vlan_untag just returns the new
> skb. This was not a problem until commit 88eb1944e18c ("net: core:
> propagate SKB lists through packet_type lookup"), which moved the
> final handling of the skb via pt_prev out of
> __netif_receive_skb_core. After this commit, when the skb is
> reallocated by __netif_receive_skb_core, KASAN reports a
> use-after-free on the old skb:
>
> BUG: KASAN: use-after-free in __netif_receive_skb_one_core+0x15c/0x180
> Call Trace:
> <IRQ>
> __netif_receive_skb_one_core+0x15c/0x180
> process_backlog+0x1b5/0x630
> ? net_rx_action+0x247/0xd00
> net_rx_action+0x3fa/0xd00
> ? napi_complete_done+0x360/0x360
> __do_softirq+0x257/0xa0b
> do_softirq_own_stack+0x2a/0x40
> </IRQ>
> ? __dev_queue_xmit+0x12ba/0x3120
> do_softirq+0x5d/0x60
> [...]
>
> Allocated by task 505:
> __kasan_kmalloc.constprop.0+0xd6/0x140
> kmem_cache_alloc+0xd4/0x2e0
> skb_clone+0x106/0x300
> deliver_clone+0x3f/0xa0
> maybe_deliver+0x1c0/0x2b0
> br_flood+0xd4/0x320
> br_dev_xmit+0xbc0/0x1080
> dev_hard_start_xmit+0x139/0x750
> __dev_queue_xmit+0x24eb/0x3120
> packet_sendmsg+0x1bfa/0x50e0
> [...]
>
> Freed by task 505:
> __kasan_slab_free+0x138/0x1e0
> kmem_cache_free+0xa2/0x2e0
> macsec_handle_frame+0xa24/0x2e60
> __netif_receive_skb_core+0xe2a/0x2c90
> __netif_receive_skb_one_core+0x96/0x180
> process_backlog+0x1b5/0x630
> net_rx_action+0x3fa/0xd00
> __do_softirq+0x257/0xa0b
>
> The solution is to pass a reference to the skb to
> __netif_receive_skb_core, as we already do with the rx_handlers, so
> that its callers use the new skb.
>
> Fixes: 88eb1944e18c ("net: core: propagate SKB lists through packet_type
> lookup")
> Reported-by: Andreas Steinmetz <a...@domdv.de>
> Signed-off-by: Sabrina Dubroca <s...@queasysnail.net>
> ---
> net/core/dev.c | 26 ++++++++++++++++++++------
> 1 file changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index d6edd218babd..0bbf6d2a9c32 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4809,11 +4809,12 @@ static inline int nf_ingress(struct sk_buff *skb,
> struct packet_type **pt_prev,
> return 0;
> }
>
> -static int __netif_receive_skb_core(struct sk_buff *skb, bool pfmemalloc,
> +static int __netif_receive_skb_core(struct sk_buff **pskb, bool pfmemalloc,
> struct packet_type **ppt_prev)
> {
> struct packet_type *ptype, *pt_prev;
> rx_handler_func_t *rx_handler;
> + struct sk_buff *skb = *pskb;
Would it not be simpler just to change all users of skb to *pskb?
Then you avoid having to keep doing "*pskb = skb;" whenever skb changes
(with concomitant risk of bugs if one gets missed).
-Ed
